[U-Boot] [PATCH] Streamlined mpc512x fixed_sdram init sequence allowing for

Wolfgang Denk wd at denx.de
Tue Sep 15 23:43:27 CEST 2009


Dear Martha M Stan,

In message <12526727172732-git-send-email-mmarx at silicontkx.com> you wrote:
> Signed-off-by: Martha M Stan <mmarx at silicontkx.com>
> ---
>  board/davedenx/aria/aria.c              |    2 +-
>  board/esd/mecp5123/mecp5123.c           |    2 +-
>  board/freescale/mpc5121ads/mpc5121ads.c |    2 +-
>  cpu/mpc512x/fixed_sdram.c               |   94 ++++++++++++++++++++-----------
>  include/asm-ppc/immap_512x.h            |    5 +-
>  include/asm-ppc/mpc512x.h               |    2 +-
>  include/configs/aria.h                  |    6 +--
>  include/configs/mecp5123.h              |    5 +-
>  include/configs/mpc5121ads.h            |    6 +--
>  9 files changed, 72 insertions(+), 52 deletions(-)
...
> diff --git a/cpu/mpc512x/fixed_sdram.c b/cpu/mpc512x/fixed_sdram.c
> index d906903..eb0811c 100644
> --- a/cpu/mpc512x/fixed_sdram.c
> +++ b/cpu/mpc512x/fixed_sdram.c
> @@ -20,23 +20,73 @@
>   * MA 02111-1307 USA
>   *
>   */
> -
> +#define DEBUG 0xff

Please don't do this in released code.

>  #include <common.h>
>  #include <asm/io.h>
>  #include <asm/mpc512x.h>
>  
> +/* config settings in order of the 4 mddrc cfg registers */
> +u32 default_mddrc_config[4] = {
> +	CONFIG_SYS_MDDRC_SYS_CFG,
> +	CONFIG_SYS_MDDRC_TIME_CFG0,
> +	CONFIG_SYS_MDDRC_TIME_CFG1,
> +	CONFIG_SYS_MDDRC_TIME_CFG2
> +};

As you are playing tricks with the Refresh and Run bits below, this
needs a comment that explains what the values mean, and how to set up
this list.

> +u32 default_init_sequence[] = {

Please add a comment what all this means.

> +	CONFIG_SYS_MICRON_NOP,
> +	CONFIG_SYS_MICRON_NOP,
...
> +	CONFIG_SYS_MICRON_PCHG_ALL,
> +	CONFIG_SYS_MICRON_NOP
> +};
> +
>  /*
>   * fixed sdram init:
>   * The board doesn't use memory modules that have serial presence
>   * detect or similar mechanism for discovery of the DRAM settings
>   */
> -long int fixed_sdram(void)
> +long int fixed_sdram(u32 *mddrc_config, u32 *dram_init_sequence, int seq_table_size)

Line too long.

> @@ -46,7 +96,7 @@ long int fixed_sdram(void)
>  	sync_law(&im->sysconf.ddrlaw.ar);
>  
>  	/* Enable DDR */
> -	out_be32(&im->mddrc.ddr_sys_config, CONFIG_SYS_MDDRC_SYS_CFG_EN);
> +	out_be32(&im->mddrc.ddr_sys_config, MDDRC_SYS_CFG_EN);

Please stick with the CONFIG_SYS_ name.

>  	/* Initialize DDR Priority Manager */
>  	out_be32(&im->mddrc.prioman_config1, CONFIG_SYS_MDDRCGRP_PM_CFG1);
> @@ -74,40 +124,18 @@ long int fixed_sdram(void)
>  	out_be32(&im->mddrc.lut_table4_alternate_lower, CONFIG_SYS_MDDRCGRP_LUT4_AL);
>  
>  	/* Initialize MDDRC */
> -	out_be32(&im->mddrc.ddr_sys_config, CONFIG_SYS_MDDRC_SYS_CFG);
> -	out_be32(&im->mddrc.ddr_time_config0, CONFIG_SYS_MDDRC_TIME_CFG0);
> -	out_be32(&im->mddrc.ddr_time_config1, CONFIG_SYS_MDDRC_TIME_CFG1);
> -	out_be32(&im->mddrc.ddr_time_config2, CONFIG_SYS_MDDRC_TIME_CFG2);
> +	out_be32(&im->mddrc.ddr_sys_config, mddrc_config[0]);
> +	out_be32(&im->mddrc.ddr_time_config0, mddrc_config[1] & MDDRC_TIME_CFG0_RFRSH0);
> +	out_be32(&im->mddrc.ddr_time_config1, mddrc_config[2]);
> +	out_be32(&im->mddrc.ddr_time_config2, mddrc_config[3]);

Why is the " & MDDRC_TIME_CFG0_RFRSH0" needed? Please comment.

>  	/* Initialize DDR */
...
> +	for (i = 0; i < seq_table_size; i++)
> +		out_be32(&im->mddrc.ddr_command, dram_init_sequence[i]);
>  
>  	/* Start MDDRC */
> -	out_be32(&im->mddrc.ddr_time_config0, CONFIG_SYS_MDDRC_TIME_CFG0_RUN);
> -	out_be32(&im->mddrc.ddr_sys_config, CONFIG_SYS_MDDRC_SYS_CFG_RUN);
> +	out_be32(&im->mddrc.ddr_time_config0, mddrc_config[1]);
> +	out_be32(&im->mddrc.ddr_sys_config, mddrc_config[0] & MDDRC_SYS_CFG_RUN);

Please add comment that explains the "& MDDRC_SYS_CFG_RUN" part.

> diff --git a/include/asm-ppc/immap_512x.h b/include/asm-ppc/immap_512x.h
> index 24e6c69..13d3d0e 100644
> --- a/include/asm-ppc/immap_512x.h
> +++ b/include/asm-ppc/immap_512x.h
> @@ -45,7 +45,6 @@
>  #define IMMRBAR_BASE_ADDR	0xFFF00000	/* Base address mask */
>  #define IMMRBAR_RES		~(IMMRBAR_BASE_ADDR)
>  
> -
>  #ifndef __ASSEMBLY__
>  typedef struct law512x {
>  	u32 bar;	/* Base Addr Register */
> @@ -341,6 +340,10 @@ typedef struct ddr512x {
>  	u32 res2[0x3AD];
>  } ddr512x_t;
>  
> +/* MDDRC SYS CFG and Timing CFG0 Registers */
> +#define MDDRC_SYS_CFG_EN	0xF0000000
> +#define MDDRC_SYS_CFG_RUN 	~(0x01 << 28)
> +#define MDDRC_TIME_CFG0_RFRSH0	0x0000FFFF

I find this inverse logic confusing. I recommend to use direct values
here, and "& ~FOO" above, then everybody sees what that means.

> diff --git a/include/asm-ppc/mpc512x.h b/include/asm-ppc/mpc512x.h
> index 20456f5..6a65492 100644
> --- a/include/asm-ppc/mpc512x.h
> +++ b/include/asm-ppc/mpc512x.h
> @@ -50,7 +50,7 @@ static inline void sync_law(volatile void *addr)
>  /*
>   * Prototypes
>   */
> -extern long int fixed_sdram(void);
> +extern long int fixed_sdram(u32 *mddrc_config, u32 *dram_init_sequence, int seq_table_size);

Line too long.

> diff --git a/include/configs/aria.h b/include/configs/aria.h
> index 4211113..a6fa168 100644
> --- a/include/configs/aria.h
> +++ b/include/configs/aria.h
> @@ -143,14 +143,10 @@
>  					(0 <<  0) 	/* FIFO_UV_EN */ \
>  				     )
>  
> -#define CONFIG_SYS_MDDRC_SYS_CFG_RUN	(CONFIG_SYS_MDDRC_SYS_CFG & ~(1 << 28))
> +#define CONFIG_SYS_MDDRC_TIME_CFG0	0x030C3D2E
>  #define CONFIG_SYS_MDDRC_TIME_CFG1	0x55D81189
>  #define CONFIG_SYS_MDDRC_TIME_CFG2	0x34790863
>  
> -#define CONFIG_SYS_MDDRC_SYS_CFG_EN	0xF0000000
> -#define CONFIG_SYS_MDDRC_TIME_CFG0	0x00003D2E
> -#define CONFIG_SYS_MDDRC_TIME_CFG0_RUN	0x030C3D2E
> -
>  #define CONFIG_SYS_MICRON_NOP		0x01380000
>  #define CONFIG_SYS_MICRON_PCHG_ALL	0x01100400
>  #define CONFIG_SYS_MICRON_EMR	     (	(1 << 24) |	/* CMD_REQ */ \

Should not all these CONFIG_SYS_MICRON_* definitions now also be
eliminated / merged? This affects include/configs/aria.h,
include/configs/mecp5123.h, and include/configs/mpc5121ads.h .

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
You don't have to worry about me. I might have been born yesterday...
but I stayed up all night.


More information about the U-Boot mailing list