[U-Boot] [PATCH] ppc4xx: /PLLOUTB/CPU clock/ Default bootstrap options A, B, C, D

[Stefan Roese]

Hi Rup,

On Friday 24 July 2009 07:54:01 Rupjyoti Sarmah wrote:
> Unstable 440EPx operation due to default bootsrtap options settings.
> The 440EPx fixed bootstrap options A,B,C,D sets PLL FWDVA to a value 1
> that results PLLOUTB being greater
> than the CPU clock frequency. This results unstable 440EPx operation
> causing hang conditions.
>
> This is a patch fixing this problem. The patch touches two files
> speed.c and cpu_init.c.

Again, you don't have to mention which files are changed. git has all this 
information and we don't need to store it into the commit text.

> Signed off by  Rupjyoti Sarmah < rsarmah at amcc.com > from Applied Micro
> ----------------------------------
>
> diff --git a/a/u-boot-2009.06/cpu/ppc4xx/cpu_init.c
> b/b/u-boot-2009.06/cpu/ppc4xx/cpu_init.c
> old mode 100644
> new mode 100755
> index 577d33f..eb50c3c
> --- a/a/u-boot-2009.06/cpu/ppc4xx/cpu_init.c
> +++ b/b/u-boot-2009.06/cpu/ppc4xx/cpu_init.c
> @@ -1,4 +1,4 @@
> -/*
> + /*

This line is changed incorrectly. Please keep it.

>   * (C) Copyright 2000-2007
>   * Wolfgang Denk, DENX Software Engineering, wd at denx.de.
>   *
> @@ -32,10 +32,19 @@
>  DECLARE_GLOBAL_DATA_PTR;
>  #endif
>
> -#ifndef CONFIG_SYS_PLL_RECONFIG
> -#define CONFIG_SYS_PLL_RECONFIG	0
> +#ifndef CFG_PLL_RECONFIG
> +#define CFG_PLL_RECONFIG	0

Ups. No. We moved from using "CFG_" to "CONFIG_SYS" quite a while ago. Which 
U-Boot version did you base this patch on? Please use current tot (top of 
tree) version as bases for you patches.

>  #endif
>
> +

Don't add empty lines.

> +#define BOOT_STRAP_OPTION_A  0x00000000
> +#define BOOT_STRAP_OPTION_B  0x00000001
> +#define BOOT_STRAP_OPTION_D  0x00000003
> +#define BOOT_STRAP_OPTION_E  0x00000004
> +
> +
> +
> +

And please don't add so many empty lines. One is enough.

>  void reconfigure_pll(u32 new_cpu_freq)
>  {
>  #if defined(CONFIG_440EPX)
> @@ -47,6 +56,7 @@ void reconfigure_pll(u32 new_cpu_freq)
>  		perdv0,	target_perdv0,				/*
> CLK_PERD */

Patch is line wrapped. Please fix. Otherwise it can't be applied.

>  		spcid0,	target_spcid0;				/*
> CLK_SPCID */
>
> +

No more empty lines here.

>  	/* Reconfigure clocks if necessary.
>  	 * See PPC440EPx User's Manual, sections 8.2 and 14 */
>  	if (new_cpu_freq == 667) {
> @@ -111,17 +121,105 @@ void reconfigure_pll(u32 new_cpu_freq)
>  			mtcpr(clk_spcid, reg);
>  			reset_needed = 1;
>  		}
> +	}
> +
> +/*
> +440EPx fixed bootstrap options A, B, D, and E currently set PLL FWDVA
> to a
> +divisor value = 1.  This results in the PLLOUTB being greater than the
> CPU
> +clock frequency which causes unstable 440EPx operation resulting in
> various
> +software hang conditions.  The user manual and the data sheet both
> specify that
> +a FWDVB = 1 is not a valid setting.
> +
> +If the customer uses the IIC attached EEPROM to set bootstrap options,
> this is
> +not a problem. Some customers choose to use one of the fixed bootstrap
> options
> +(A, B, D, or E)and do not have an EEPROM to use for programmable
> bootstrap
> +options. This requires that FWDVA and PRBDV0 be re-programmed early in
> the chip
> +initialization process by software.  The procedure for re-programming
> the PLL
> +is defined in the 440EPx user manual section 8.3, Bootstrap Options.
> +*/
> +
> +
> +
> +
> +
> +
> +
>
> -		/* Set reload inhibit so configuration will persist
> across
> -		 * processor resets */
> +	                                            /* Get current value
> of FWDVA.*/
> +
> +	mfcpr(clk_plld, reg);
> +    temp = (reg & PLLD_FWDVA_MASK) >> 16;

Indentation is incorrect. Use tabs instead of spaces.

> +                                                /* Check to see if
> FWDVA has   */
> +                                                /* been set to a value
> of 1. if*/
> +                                                /* it has we must
> modify it.   */
> +
> +    if (temp == 1) {
> +

Again, please don't add some many empty lines.

I'll stop my review here. Please fix the problems mentioned above. And 
especially rebase your patch against the latest version.

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
=====================================================================