[U-Boot] [PATCH] Add Elpida Memory Configuration to mpc5121ads Boards

[Wolfgang Denk]

Dear Martha M Stan,

In message <1252706807954-git-send-email-mmarx at silicontkx.com> you wrote:
> Signed-off-by: Martha M Stan <mmarx at silicontkx.com>
> ---
>  board/freescale/mpc5121ads/mpc5121ads.c |   97 ++++++++++++++++++++++++++++++-
>  include/configs/mpc5121ads.h            |   39 ++++++++++++-
>  2 files changed, 132 insertions(+), 4 deletions(-)
...
> +	u32 elpida_init_sequence[] = {
> +		CONFIG_SYS_MICRON_NOP,
> +		CONFIG_SYS_MICRON_NOP,
> +		CONFIG_SYS_MICRON_NOP,
> +		CONFIG_SYS_MICRON_NOP,
> +		CONFIG_SYS_MICRON_NOP,
> +		CONFIG_SYS_MICRON_NOP,
> +		CONFIG_SYS_MICRON_NOP,
> +		CONFIG_SYS_MICRON_NOP,
> +		CONFIG_SYS_MICRON_NOP,
> +		CONFIG_SYS_MICRON_NOP,
> +		CONFIG_SYS_MICRON_PCHG_ALL,
> +		CONFIG_SYS_MICRON_NOP,
> +		CONFIG_SYS_MICRON_RFSH,
> +		CONFIG_SYS_MICRON_NOP,
> +		CONFIG_SYS_MICRON_RFSH,
> +		CONFIG_SYS_MICRON_NOP,
> +		CONFIG_SYS_MICRON_EM2,
> +		CONFIG_SYS_MICRON_EM3,
> +		CONFIG_SYS_MICRON_EN_DLL,
> +		CONFIG_SYS_ELPIDA_RES_DLL,
> +		CONFIG_SYS_MICRON_PCHG_ALL,
> +		CONFIG_SYS_MICRON_RFSH,
> +		CONFIG_SYS_MICRON_RFSH,
> +		CONFIG_SYS_MICRON_RFSH,
> +		CONFIG_SYS_ELPIDA_INIT_DEV_OP,
> +		CONFIG_SYS_MICRON_NOP,
> +		CONFIG_SYS_MICRON_NOP,
> +		CONFIG_SYS_MICRON_NOP,
> +		CONFIG_SYS_MICRON_NOP,
> +		CONFIG_SYS_MICRON_NOP,
> +		CONFIG_SYS_MICRON_NOP,
> +		CONFIG_SYS_MICRON_NOP,
> +		CONFIG_SYS_MICRON_NOP,
> +		CONFIG_SYS_MICRON_NOP,
> +		CONFIG_SYS_MICRON_NOP,
> +		CONFIG_SYS_MICRON_OCD_DEFAULT,
> +		CONFIG_SYS_ELPIDA_OCD_EXIT,
> +		CONFIG_SYS_MICRON_NOP,
> +		CONFIG_SYS_MICRON_NOP,
> +		CONFIG_SYS_MICRON_NOP,
> +		CONFIG_SYS_MICRON_NOP,
> +		CONFIG_SYS_MICRON_NOP,
> +		CONFIG_SYS_MICRON_NOP,
> +		CONFIG_SYS_MICRON_NOP,
> +		CONFIG_SYS_MICRON_NOP,
> +		CONFIG_SYS_MICRON_NOP,
> +		CONFIG_SYS_MICRON_NOP
> +	};

This seems wrong to me. Elpida settings should use CONFIG_SYS_ELPIDA_*
variables only, should't it?

> -	msize = fixed_sdram(NULL, NULL, 0);
> +	u32 msize = 0;
> +	if (is_micron())
> +		msize = fixed_sdram(NULL, NULL, 0);
> +	else
> +		msize = fixed_sdram(elpida_mddrc_config,
> +				elpida_init_sequence,
> +				sizeof(elpida_init_sequence)/sizeof(u32));

Braces needed for multiline statement.

> diff --git a/include/configs/mpc5121ads.h b/include/configs/mpc5121ads.h
> index d96e7f5..fda63dd 100644
> --- a/include/configs/mpc5121ads.h
> +++ b/include/configs/mpc5121ads.h
> @@ -141,14 +141,49 @@
>  #endif
>  #define CONFIG_SYS_MDDRC_TIME_CFG0	0x06183D2E
>  
> +#define CONFIG_SYS_MDDRC_SYS_CFG_ELPIDA	 	0xFA802B00
> +#define CONFIG_SYS_MDDRC_TIME_CFG1_ELPIDA	0x690e1189
> +#define CONFIG_SYS_MDDRC_TIME_CFG2_ELPIDA	0x35310864
> +
> +/* MICRON Configuration and Commands
> + * These also work for Elpida unless explicitly overwritten
> + */

Incorrect multiline comment style.

For clearness, I'd prefer to see separate settings.

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
There is a time in the tides of men, Which, taken at its flood, leads
on to success. On the other hand, don't count on it.   - T. K. Lawson