From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stefan Roese <sr@denx.de>
Date: Tue, 16 Sep 2008 15:04:41 +0200
Subject: [U-Boot] [PATCH] ppc4xx: Fix DDR2 auto calibration on Kilauea
	600MHz
In-Reply-To: <1221517598-10490-1-git-send-email-vgallardo@amcc.com>
References: <1221517598-10490-1-git-send-email-vgallardo@amcc.com>
Message-ID: <200809161504.41752.sr@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On Tuesday 16 September 2008, Victor Gallardo wrote:
> Signed-off-by: Victor Gallardo <vgallardo@amcc.com>
> Signed-off-by: Adam Graham <agraham@amcc.com>

Please find some comments below.

> ---
>  board/amcc/kilauea/kilauea.c        |   31 +++++++++++++++++++++++++++++++
>  cpu/ppc4xx/4xx_ibm_ddr2_autocalib.c |    5 -----
>  include/asm-ppc/ppc4xx-sdram.h      |    8 ++++++++
>  3 files changed, 39 insertions(+), 5 deletions(-)
>
> diff --git a/board/amcc/kilauea/kilauea.c b/board/amcc/kilauea/kilauea.c
> index 7b10255..8163d16 100644
> --- a/board/amcc/kilauea/kilauea.c
> +++ b/board/amcc/kilauea/kilauea.c
> @@ -374,3 +374,34 @@ int post_hotkeys_pressed(void)
>  	return 0;	/* No hotkeys supported */
>  }
>  #endif /* CONFIG_POST */
> +
> +#if defined(CONFIG_PPC4xx_DDR_AUTOCALIBRATION)
> +/*
> + * This is for quicker auto calibration boot up once WRDTR and CLKTR
> + * values for the kilauea board were determined and are therefore known.
> + *
> + * Use these scan options for PLB bus greater than 200MHz else use
> + * the defaults.
> + */
> +/* List of (SDRAM_WRDTR.[WDTR], SDRAM_CLKTR.[CLKP]) pairs to try */
> +struct sdram_timing quick_scan_options[] = {
> +	{0, 3}, {1, 1}, {1, 2}, {1, 3},
> +	{2, 1}, {2, 2}, {2, 3}, {3, 1},
> +	{3, 2}, {4, 1}, {-1, -1}
> +};

It's not really clear to me, why this reduced list should fix this problem 
seen on the 600MHz boards (it does - I tested successfully on my board). From 
my understanding the scan is now skipping some of the WDTR/CLKTR 
configurations. This will of course result in a quicker scan, but 
autocalibration should still work with the default full list.

I'm probably missing something here so please enlighten me. :)

> +
> +ulong ddr_scan_option(ulong default_val)
> +{
> +	u32 sdram_freq;
> +	PPC4xx_SYS_INFO board_cfg;
> +
> +	get_sys_info(&board_cfg);
> +	sdram_freq = board_cfg.freqPLB;
> +
> +	/* PLB bus greater than 200MHz */
> +	if (sdram_freq >= 0xbebc200)

Why do you write this in hex? Change it to decimal please.

> +		return (ulong)(quick_scan_options);
> +	else
> +		return (ulong)default_val;
> +}
> +#endif /* CONFIG_PPC4xx_DDR_AUTOCALIBRATION */
> diff --git a/cpu/ppc4xx/4xx_ibm_ddr2_autocalib.c
> b/cpu/ppc4xx/4xx_ibm_ddr2_autocalib.c index 83b9883..3ba8176 100644
> --- a/cpu/ppc4xx/4xx_ibm_ddr2_autocalib.c
> +++ b/cpu/ppc4xx/4xx_ibm_ddr2_autocalib.c
> @@ -79,11 +79,6 @@ struct ddrautocal {
>  	u32 flags;
>  };
>
> -struct sdram_timing {
> -	u32 wrdtr;
> -	u32 clktr;
> -};
> -
>  struct sdram_timing_clks {
>  	u32 wrdtr;
>  	u32 clktr;
> diff --git a/include/asm-ppc/ppc4xx-sdram.h
> b/include/asm-ppc/ppc4xx-sdram.h index 8efa557..50148c5 100644
> --- a/include/asm-ppc/ppc4xx-sdram.h
> +++ b/include/asm-ppc/ppc4xx-sdram.h
> @@ -1403,6 +1403,14 @@
>  #endif /* CONFIG_SDRAM_PPC4xx_DENALI_DDR2 */
>
>  #ifndef __ASSEMBLY__
> +
> +#if defined(CONFIG_PPC4xx_DDR_AUTOCALIBRATION)
> +struct sdram_timing {
> +	u32 wrdtr;
> +	u32 clktr;
> +};
> +#endif /* CONFIG_PPC4xx_DDR_AUTOCALIBRATION */

I suggest you remove this unnecessary conditional compilation option here. It 
doesn't hurt to have this struct declared in all configurations.

Please fix and resubmit. Thanks.

Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================