[U-Boot] [PATCH] Streamlined mpc512x fixed_sdram init sequence.

[Wolfgang Denk]

Dear Martha M Stan,

In message <12535564342520-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               |  104 ++++++++++++++++++++----------
>  include/asm-ppc/immap_512x.h            |    4 +
>  include/asm-ppc/mpc512x.h               |    2 +-
>  include/configs/aria.h                  |   22 +++----
>  include/configs/mecp5123.h              |   23 +++----
>  include/configs/mpc5121ads.h            |   30 ++++-----
>  9 files changed, 109 insertions(+), 82 deletions(-)


Applying: Streamlined mpc512x fixed_sdram init sequence.
/home/wd/git/u-boot/work/.git/rebase-apply/patch:60: trailing whitespace.
/* 
/home/wd/git/u-boot/work/.git/rebase-apply/patch:61: trailing whitespace.
 * MDDRC Config Runtime Settings in order of the 4 MDDRC cfg registers 
/home/wd/git/u-boot/work/.git/rebase-apply/patch:152: trailing whitespace.
        /* 
/home/wd/git/u-boot/work/.git/rebase-apply/patch:154: trailing whitespace.
         *  put MDDRC in CMD mode and 
/home/wd/git/u-boot/work/.git/rebase-apply/patch:155: trailing whitespace.
         *  set the max time between refreshes to 0 during init process 
warning: 5 lines applied after fixing whitespace errors.


Please pay more attention to Coding Style!


> diff --git a/cpu/mpc512x/fixed_sdram.c b/cpu/mpc512x/fixed_sdram.c
> index d906903..63a3035 100644
> --- a/cpu/mpc512x/fixed_sdram.c
> +++ b/cpu/mpc512x/fixed_sdram.c
> @@ -25,18 +25,70 @@
>  #include <asm/io.h>
>  #include <asm/mpc512x.h>
>  
> +/* 
> + * MDDRC Config Runtime 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
> +};

Hm... we should move the first entry down... see below...


> -long int fixed_sdram(void)
> +long int fixed_sdram(u32 *mddrc_config, u32 *dram_init_seq, int seq_sz)
>  {
>  	volatile immap_t *im = (immap_t *)CONFIG_SYS_IMMR;
>  	u32 msize = CONFIG_SYS_DDR_SIZE * 1024 * 1024;
>  	u32 msize_log2 = __ilog2(msize);
>  	u32 i;
>  
> +	/* take default settings and init sequence if necessary */
> +	if (mddrc_config == 0)
> +		mddrc_config = default_mddrc_config;

Please write NULL when you mean a NULL pointer.

> +	if (dram_init_seq == 0) {
> +		dram_init_seq = default_init_seq;
> +		seq_sz = sizeof(default_init_seq)/sizeof(u32);
> +	}

Ditto.

> +	/* 
> +	 * Initialize MDDRC
> +	 *  put MDDRC in CMD mode and 
> +	 *  set the max time between refreshes to 0 during init process 
> +	 */
> +	out_be32(&im->mddrc.ddr_sys_config, mddrc_config[0] | MDDRC_SYS_CFG_CMD_MASK);
> +	out_be32(&im->mddrc.ddr_time_config0, mddrc_config[1] & MDDRC_REFRESH_ZERO_MASK);
> +	out_be32(&im->mddrc.ddr_time_config1, mddrc_config[2]);
> +	out_be32(&im->mddrc.ddr_time_config2, mddrc_config[3]);

I cannot help it, but every time I see this I think the code is
wrong. Guess I have seen too many copy & paste errors in this style.
When I see "...time_config2" I also want to see "mddrc_config[2]", i. e.
identical indices. We should reorder the struct.


To make some progress, I made the changes above, and applied the patch.

Thanks.

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
"The one charm of marriage is that it makes a  life  of  deception  a
neccessity."                                            - Oscar Wilde