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

[Chin Liang See <clsee@altera.com>]

Hi,

On Tue, 2015-06-09 at 10:51 -0500, Dinh Nguyen wrote:
> 
> 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.


It is to workaround the computational of SDRAM rows. The info is then
used to calculate the SDRAM size. By doing this, we can remove from
hardcoding the SDRAM size into the code. More info at
https://github.com/altera-opensource/u-boot-socfpga/commit/93815696dce132ff8abc4ab2f4c195339ff821a0. Hope this explains.
 

> 
> > 
> >> +#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@denx.de>
> > 									Pavel
> 
> Thanks,
> 
> Dinh