On 6/9/15 6:55 AM, Pavel Machek wrote:
> Hi!
>
>> +struct sdram_prot_rule {
>> + uint64_t sdram_start; /* SDRAM start address */
>> + uint64_t sdram_end; /* SDRAM end address */
>> + uint32_t rule; /* SDRAM protection rule number: 0-19 */
>> + int valid; /* Rule valid or not? 1 - valid, 0 not*/
>
> There should be space before "*/".
>
Ok...
>> diff --git a/arch/arm/include/asm/arch-socfpga/sdram_config.h
>> b/arch/arm/include/asm/arch-socfpga/sdram_config.h
>> new file mode 100644
>> index 0000000..f6d51ca
>> --- /dev/null
>> +++ b/arch/arm/include/asm/arch-socfpga/sdram_config.h
>> @@ -0,0 +1,100 @@
>> +/*
>> + * Copyright Altera Corporation (C) 2012-2015
>> + *
>> + * SPDX-License-Identifier: BSD-3-Clause
>> + */
>> +
>> +/* This file is autogenerated from tools provided by Altera.*/
>
> Here too.
>
Ok...
>> +#endif /*#ifndef__SDRAM_CONFIG_H*/
>
> You should not need to comment for include guards... (and comment
> style).
>
>> +static int compute_errata_rows(unsigned long long memsize, int cs, int
>> width,
>> + int rows, int banks, int cols)
>> +{
>
> Comment what kind of errata this is working around?
>
I'll have to ask around.
>
>> +#if defined(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_CSBITS) && \
>> +defined(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_ROWBITS) && \
>> +defined(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_BANKBITS) && \
>> +defined(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_COLBITS) && \
>> +defined(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_ROWBITS)
>> +
>
> Hmm? Is this really neccessary? Is it valid to provide configuration
> w/o those defines?
>
These defines are necessary as I want to keep some level of continuity
with the Altera tools that generates these config files to this.
>> + writel(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_ROWBITS,
>> + &sysmgr_regs->iswgrp_handoff[4]);
>> +#endif
>
>> +
>> + /* Restore the SDR PHY Register if valid */
>> + if (sdr_phy_reg != 0xffffffff)
>> + writel(sdr_phy_reg, &sdr_ctrl->phy_ctrl0);
>> +
>> +/***** Final step - apply configuration changes *****/
>
> Comment style...
>
Ok..
>> +/*
>> + * To calculate SDRAM device size based on SDRAM controller parameters.
>> + * Size is specified in bytes.
>> + *
>> + * NOTE:
>> + * This function is compiled and linked into the preloader and
>> + * Uboot (there may be others). So if this function changes, the Preloader
>> + * and UBoot must be updated simultaneously.
>> + */
>> +unsigned long sdram_calculate_size(void)
>> +{
>> + unsigned long temp;
>> + unsigned long row, bank, col, cs, width;
>> +
>> + temp = readl(&sdr_ctrl->dram_addrw);
>> + col = (temp & SDR_CTRLGRP_DRAMADDRW_COLBITS_MASK) >>
>> + SDR_CTRLGRP_DRAMADDRW_COLBITS_LSB;
>> +
>> + /* SDRAM Failure When Accessing Non-Existent Memory
>> + * Use ROWBITS from Quartus/QSys to calculate SDRAM size
>> + * since the FB specifies we modify ROWBITs to work around SDRAM
>> + * controller issue.
>> + *
>> + * If the stored handoff value for rows is 0, it probably means
>> + * the preloader is older than UBoot. Use the
>> + * #define from the SOCEDS Tools per Crucible review
>> + * uboot-socfpga-204. Note that this is not a supported
>> + * configuration and is not tested. The customer
>> + * should be using preloader and uboot built from the
>> + * same tag.
>> + */
>
> U-Boot is normally spelled "U-Boot". You have two different variants
> in comments here.
Thanks for the comment here, and will be more cognizant in the future on
this fact.
>
> Second part of the comment is probably not relevant any more....?
>
removed...
> Acked-by: Pavel Machek <pa...@denx.de>
> Pavel
Thanks,
Dinh
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
