[U-Boot] [PATCH] Add new Elpida memory configuration for mpc5121ads board

[Wolfgang Denk]

Dear Martha M Stan,

In message <12525956963103-git-send-email-mmarx at silicontkx.com> you wrote:
> From: Martha Marx <mmarx at silicontkx.com>
> 
> Rev 3 and earlier stay with Micron settings. Rev 4 boards manufactured
> before Nov-2008 Serial Number #1180 also stay with Micron settings.
> All new boards use a slightly slower Elpida setting.
> CONFIG_SYS_ELPIDA_MICRON_MIX sets up this detection and use.
> 
> Signed-off-by: Martha Marx Stan <mmarx at silicontkx.com>
> ---
>  board/freescale/mpc5121ads/mpc5121ads.c |   34 ++++++++++++
>  cpu/mpc512x/fixed_sdram.c               |   88 +++++++++++++++++++++++-------
>  include/configs/mpc5121ads.h            |   36 ++++++++-----
>  3 files changed, 124 insertions(+), 34 deletions(-)
> 
> diff --git a/board/freescale/mpc5121ads/mpc5121ads.c b/board/freescale/mpc5121ads/mpc5121ads.c
> index a0d7a82..c6bbbad 100644
> --- a/board/freescale/mpc5121ads/mpc5121ads.c
> +++ b/board/freescale/mpc5121ads/mpc5121ads.c
...
> +#ifdef CONFIG_SYS_ELPIDA_MICRON_MIX
> +u32 is_micron(void){

Incorrect brace style.

Why is this u23 instead of a plain int?

> +	ushort brd_rev = *(vu_short *) (CONFIG_SYS_CPLD_BASE + 0x00);

Please use I/O accessors to read from device registers.

> diff --git a/cpu/mpc512x/fixed_sdram.c b/cpu/mpc512x/fixed_sdram.c
> index 5be02f7..063f60a 100644
> --- a/cpu/mpc512x/fixed_sdram.c
> +++ b/cpu/mpc512x/fixed_sdram.c
> @@ -25,6 +25,9 @@
>  #include <asm/io.h>
>  #include <asm/mpc512x.h>
>  
> +#ifdef CONFIG_SYS_ELPIDA_MICRON_MIX
> +extern u32 is_micron(void);
> +#endif

This is ugly. A protoype in a header file should be used to allow for
consistency checing. But stop. This file here has the only place where
this function ever gets called, so why don't you move that function
here? Or is there any special reason to have it in
board/freescale/mpc5121ads/mpc5121ads.c ?

>  /*
>   * fixed sdram init:
>   * The board doesn't use memory modules that have serial presence
> @@ -36,6 +39,11 @@ long int fixed_sdram(void)
>  	u32 msize = CONFIG_SYS_DDR_SIZE * 1024 * 1024;
>  	u32 msize_log2 = __ilog2(msize);
>  	u32 i;
> +#ifdef CONFIG_SYS_ELPIDA_MICRON_MIX
> +	u32 use_micron = is_micron();
> +#else
> +	u32 use_micron = 1;
> +#endif

Is there a special reason for u32 versus int?

Why do you leave use_micron defined in the "#else" case? I think we
could omit this completely, then?

>  	/* Initialize IO Control */
>  	out_be32(&im->io_ctrl.io_control_mem, IOCTRL_MUX_DDR);
> @@ -74,10 +82,19 @@ long int fixed_sdram(void)
>  	out_be32(&im->mddrc.lut_table4_alternate_lower, MDDRCGRP_LUT4_AL);
>  
>  	/* Initialize MDDRC */
> -	out_be32(&im->mddrc.ddr_sys_config, MDDRC_SYS_CFG);
> -	out_be32(&im->mddrc.ddr_time_config0, MDDRC_TIME_CFG0);
> -	out_be32(&im->mddrc.ddr_time_config1, MDDRC_TIME_CFG1);
> -	out_be32(&im->mddrc.ddr_time_config2, MDDRC_TIME_CFG2);
> +	if (use_micron) {
> +		out_be32(&im->mddrc.ddr_sys_config, MDDRC_SYS_CFG);
> +		out_be32(&im->mddrc.ddr_time_config0, MDDRC_TIME_CFG0);
> +		out_be32(&im->mddrc.ddr_time_config1, MDDRC_TIME_CFG1);
> +		out_be32(&im->mddrc.ddr_time_config2, MDDRC_TIME_CFG2);
> +#ifdef CONFIG_SYS_ELPIDA_MICRON_MIX
> +	} else {
> +		out_be32(&im->mddrc.ddr_sys_config, MDDRC_SYS_CFG_ELPIDA);
> +		out_be32(&im->mddrc.ddr_time_config0, MDDRC_TIME_CFG0);
> +		out_be32(&im->mddrc.ddr_time_config1, MDDRC_TIME_CFG1_ELPIDA);
> +		out_be32(&im->mddrc.ddr_time_config2, MDDRC_TIME_CFG2_ELPIDA);
> +#endif

Ditto.

...
> +	if (use_micron) {
> +		out_be32(&im->mddrc.ddr_command, DDR_MICRON_INIT_DEV_OP);
> +		out_be32(&im->mddrc.ddr_command, DDR_NOP);
> +		out_be32(&im->mddrc.ddr_command, DDR_EM2);
> +		out_be32(&im->mddrc.ddr_command, DDR_NOP);
> +		out_be32(&im->mddrc.ddr_command, DDR_PCHG_ALL);
> +		out_be32(&im->mddrc.ddr_command, DDR_EM2);
> +		out_be32(&im->mddrc.ddr_command, DDR_EM3);
> +		out_be32(&im->mddrc.ddr_command, DDR_EN_DLL);
> +		out_be32(&im->mddrc.ddr_command, DDR_MICRON_INIT_DEV_OP);
> +		out_be32(&im->mddrc.ddr_command, DDR_PCHG_ALL);
> +		out_be32(&im->mddrc.ddr_command, DDR_RFSH);
> +		out_be32(&im->mddrc.ddr_command, DDR_MICRON_INIT_DEV_OP);
> +		out_be32(&im->mddrc.ddr_command, DDR_OCD_DEFAULT);
> +		out_be32(&im->mddrc.ddr_command, DDR_PCHG_ALL);
> +		out_be32(&im->mddrc.ddr_command, DDR_NOP);
> +
> +	/* Start MDDRC */
> +		out_be32(&im->mddrc.ddr_time_config0, MDDRC_TIME_CFG0_RUN);

Incorrect indentation.

> +	} else {
> +		out_be32(&im->mddrc.ddr_command, DDR_EM2);
> +		out_be32(&im->mddrc.ddr_command, DDR_EM3);
> +		out_be32(&im->mddrc.ddr_command, DDR_EN_DLL);
> +		out_be32(&im->mddrc.ddr_command, DDR_RES_DLL);
> +		out_be32(&im->mddrc.ddr_command, DDR_PCHG_ALL);
> +		out_be32(&im->mddrc.ddr_command, DDR_RFSH);
> +		out_be32(&im->mddrc.ddr_command, DDR_RFSH);
> +		out_be32(&im->mddrc.ddr_command, DDR_RFSH);
> +		out_be32(&im->mddrc.ddr_command, DDR_ELPIDA_INIT_DEV_OP);
> +		udelay(200);
> +		out_be32(&im->mddrc.ddr_command, DDR_OCD_DEFAULT);
> +		out_be32(&im->mddrc.ddr_command, DDR_OCD_EXIT);
> +		out_be32(&im->mddrc.ddr_command, DDR_PCHG_ALL);
> +		out_be32(&im->mddrc.ddr_command, DDR_NOP);
> +		out_be32(&im->mddrc.ddr_command, DDR_OCD_DEFAULT);
> +		out_be32(&im->mddrc.ddr_command, DDR_OCD_EXIT);
> +		out_be32(&im->mddrc.ddr_command, DDR_PCHG_ALL);
> +		for (i = 0; i < 10; i++)
> +			out_be32(&im->mddrc.ddr_command, DDR_NOP);
>  
>  	/* Start MDDRC */

Incorrect indentation.

> diff --git a/include/configs/mpc5121ads.h b/include/configs/mpc5121ads.h
> index 5df51e3..a618b16 100644
> --- a/include/configs/mpc5121ads.h
> +++ b/include/configs/mpc5121ads.h
...
> +#define MDDRC_SYS_CFG_ELPIDA	 	0xFA802B00
> +#define MDDRC_SYS_CFG_ELPIDA_RUN	0xEA802B00
> +#define MDDRC_TIME_CFG1_ELPIDA		0x690e1189
> +#define MDDRC_TIME_CFG2_ELPIDA		0x35310864

These are all configurable parameters that should be named
CONFIG_SYS_*.

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
A penny saved is a penny to squander.
- Ambrose Bierce