[U-Boot] [PATCHv4 1/3] driver/ddr/altera: Add DDR driver for Altera's SDRAM controller

[Dinh Nguyen]

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 <pavel at denx.de>
> 									Pavel

Thanks,

Dinh