[U-Boot] [PATCHv2 01/20] arm: socfpga: spl: Add main sdram code

[Marek Vasut]

On Monday, March 02, 2015 at 05:27:49 PM, dinguyen at opensource.altera.com wrote:
> From: Dinh Nguyen <dinguyen at opensource.altera.com>
> 
> This adds the code to configure the SDRAM controller that is found in the
> SoCFGPA Cyclone5 and Arria5 platforms.
> 
> Signed-off-by: Dinh Nguyen <dinguyen at opensource.altera.com>

Hi!

[...]

> diff --git a/arch/arm/include/asm/arch-socfpga/sdram.h
> b/arch/arm/include/asm/arch-socfpga/sdram.h new file mode 100644
> index 0000000..83e45d2
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-socfpga/sdram.h
> @@ -0,0 +1,434 @@
> +/*
> + * Copyright (C) 2014 Altera Corporation <www.altera.com>
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +#ifndef	_SDRAM_H_
> +#define	_SDRAM_H_
> +
> +#ifndef __ASSEMBLY__
> +
> +/* function declaration */
> +unsigned long sdram_calculate_size(void);
> +unsigned sdram_mmr_init_full(unsigned int sdr_phy_reg);
> +int sdram_calibration_full(void);
> +
> +extern int sdram_calibration(void);

Is the "extern" keyword really needed here ?

> +/* Group: sdr.phygrp.sccgrp                                               
> */ +#define SDR_PHYGRP_SCCGRP_ADDRESS 0x0

Can you define these registers using the struct-based access please ?

> +/* Group: sdr.phygrp.phymgrgrp                                            
> */ +#define SDR_PHYGRP_PHYMGRGRP_ADDRESS 0x1000
> +/* Group: sdr.phygrp.rwmgrgrp                                             
> */ +#define SDR_PHYGRP_RWMGRGRP_ADDRESS 0x2000

[...]

> 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..5c3ba86
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-socfpga/sdram_config.h
> @@ -0,0 +1,100 @@
> +/*
> + * Copyright Altera Corporation (C) 2012-2014. All rights reserved
> + *
> + * SPDX-License-Identifier:    BSD-3-Clause
> + */
> +
> +#ifndef __SDRAM_CONFIG_H
> +#define __SDRAM_CONFIG_H
> +
> +#define CONFIG_HPS_SDR_CTRLCFG_CTRLCFG_MEMTYPE			(2)
> +#define CONFIG_HPS_SDR_CTRLCFG_CTRLCFG_MEMBL			(8)
> +#define CONFIG_HPS_SDR_CTRLCFG_CTRLCFG_ADDRORDER		(0)

You can drop those brackets around numeric values, there's no point in them.

[...]

About the rest of the code ... what is your feeling when you look at the code?
What do you think of the code quality ? Do you think it's good or do you think
there are obvious places where it can be improved ?