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

[Pavel Machek]

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 "*/".

> 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.

> +#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?


> +#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?

> +	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...

> +/*
> + * 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.

Second part of the comment is probably not relevant any more....?

Acked-by: Pavel Machek <pavel at denx.de>
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html