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

Reply via email to