[U-Boot] [PATCH] ppc4xx fix unstable 440EPx bootstrap options

[U-Boot] [PATCH] ppc4xx fix unstable 440EPx bootstrap options

[Rupjyoti Sarmah]

440EPx fixed bootstrap options A, B, D, and E sets PLL FWDVA to a value = 1.
This results in the PLLOUTB being greater than the CPU clock frequency
resulting unstable 440EPx operation resulting in various software hang
conditions.

Signed-off-by: Rupjyoti Sarmah <rsarmah@appliedmicro.com>
Acked-by : Victor Gallardo <vgallardo@appliedmicro.com>
---
 cpu/ppc4xx/cpu_init.c |   67 +++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 62 insertions(+), 5 deletions(-)

diff --git a/cpu/ppc4xx/cpu_init.c b/cpu/ppc4xx/cpu_init.c
index ccd9993..9b7caff 100644
--- a/cpu/ppc4xx/cpu_init.c
+++ b/cpu/ppc4xx/cpu_init.c
@@ -35,6 +35,10 @@ DECLARE_GLOBAL_DATA_PTR;
 #ifndef CONFIG_SYS_PLL_RECONFIG
 #define CONFIG_SYS_PLL_RECONFIG	0
 #endif
+#define BOOT_STRAP_OPTION_A  0x00000000
+#define BOOT_STRAP_OPTION_B  0x00000001
+#define BOOT_STRAP_OPTION_D  0x00000003
+#define BOOT_STRAP_OPTION_E  0x00000004
 
 void reconfigure_pll(u32 new_cpu_freq)
 {
@@ -111,17 +115,70 @@ void reconfigure_pll(u32 new_cpu_freq)
 			mtcpr(CPR0_SPCID, reg);
 			reset_needed = 1;
 		}
+	}
+	/* Get current value of FWDVA.*/
+	mfcpr(CPR0_PLLD, reg);
+	temp = (reg & PLLD_FWDVA_MASK) >> 16;
+	/*
+	 * Check to see if FWDVA has been set to value of 1. if it has we must
+	 * modify it.
+	 */
+	if (temp == 1) {
+		mfcpr(CPR0_PLLD, reg);
+		/* Get current value of fbdv.  */
+		temp = (reg & PLLD_FBDV_MASK) >> 24;
+		fbdv = temp ? temp : 32;
+		/* Get current value of lfbdv. */
+		temp = (reg & PLLD_LFBDV_MASK);
+		lfbdv = temp ? temp : 64;
+		/*
+		 * Load register that contains current boot strapping option.
+		 */
+		mfcpr(CPR0_ICFG, reg);
+		/* Shift strapping option into low 3 bits.*/
+		reg = (reg >> 28);
+
+		if (reg == BOOT_STRAP_OPTION_A || reg == BOOT_STRAP_OPTION_B ||
+		    reg == BOOT_STRAP_OPTION_D || reg == BOOT_STRAP_OPTION_E) {
+			/*
+			 * Get current value of FWDVA. Assign current FWDVA to
+			 * new FWDVB.
+			 */
+			mfcpr(CPR0_PLLD, reg);
+			target_fwdvb = (reg & PLLD_FWDVA_MASK) >> 16;
+			fwdvb = target_fwdvb ? target_fwdvb : 8;
+			/*
+			 * Get current value of FWDVB. Assign current FWDVB to
+			 * new FWDVA.
+			 */
+			target_fwdva = (reg & PLLD_FWDVB_MASK) >> 8;
+			fwdva = target_fwdva ? target_fwdva : 16;
+			/*
+			 * Update CPR0_PLLD with switched FWDVA and FWDVB.
+			 */
+			reg &= ~(PLLD_FWDVA_MASK | PLLD_FWDVB_MASK |
+				PLLD_FBDV_MASK | PLLD_LFBDV_MASK);
+			reg |= ((fwdva == 16 ? 0 : fwdva) << 16) |
+			((fwdvb == 8 ? 0 : fwdvb) << 8) |
+			((fbdv == 32 ? 0 : fbdv) << 24) |
+			(lfbdv == 64 ? 0 : lfbdv);
+			mtcpr(CPR0_PLLD, reg);
+			/* Acknowledge that a reset is required. */
+			reset_needed = 1;
+		}
+	}
 
-		/* Set reload inhibit so configuration will persist across
-		 * processor resets */
+	if (reset_needed) {
+		/*
+		 * Set reload inhibit so configuration will persist across
+		 * processor resets
+		 */
 		mfcpr(CPR0_ICFG, reg);
 		reg &= ~CPR0_ICFG_RLI_MASK;
 		reg |= 1 << 31;
 		mtcpr(CPR0_ICFG, reg);
-	}
 
-	/* Reset processor if configuration changed */
-	if (reset_needed) {
+		/* Reset processor if configuration changed */
 		__asm__ __volatile__ ("sync; isync");
 		mtspr(SPRN_DBCR0, 0x20000000);
 	}
-- 
1.5.6.3

[U-Boot] [PATCH] ppc4xx fix unstable 440EPx bootstrap options

[Stefan Roese]

On Thursday 25 February 2010 19:00:17 Rupjyoti Sarmah wrote:
> 440EPx fixed bootstrap options A, B, D, and E sets PLL FWDVA to a value =
> 1. This results in the PLLOUTB being greater than the CPU clock frequency
> resulting unstable 440EPx operation resulting in various software hang
> conditions.

Thanks. Sorry for the late review. I still have some minor mostly coding style
related issues. Please see below.
 
> Signed-off-by: Rupjyoti Sarmah <rsarmah@appliedmicro.com>
> Acked-by : Victor Gallardo <vgallardo@appliedmicro.com>
> ---
>  cpu/ppc4xx/cpu_init.c |   67
> +++++++++++++++++++++++++++++++++++++++++++++--- 1 files changed, 62
> insertions(+), 5 deletions(-)
> 
> diff --git a/cpu/ppc4xx/cpu_init.c b/cpu/ppc4xx/cpu_init.c
> index ccd9993..9b7caff 100644
> --- a/cpu/ppc4xx/cpu_init.c
> +++ b/cpu/ppc4xx/cpu_init.c
> @@ -35,6 +35,10 @@ DECLARE_GLOBAL_DATA_PTR;
>  #ifndef CONFIG_SYS_PLL_RECONFIG
>  #define CONFIG_SYS_PLL_RECONFIG	0
>  #endif
> +#define BOOT_STRAP_OPTION_A  0x00000000
> +#define BOOT_STRAP_OPTION_B  0x00000001
> +#define BOOT_STRAP_OPTION_D  0x00000003
> +#define BOOT_STRAP_OPTION_E  0x00000004

Please move these defines into a header. Are they 440EPx specific? Or common for all 
4xx variants? If 440EPx specific I suggest to move them to include/ppc440.h. If 
common then include/ppc4xx.h please.
 
>  void reconfigure_pll(u32 new_cpu_freq)
>  {
> @@ -111,17 +115,70 @@ void reconfigure_pll(u32 new_cpu_freq)
>  			mtcpr(CPR0_SPCID, reg);
>  			reset_needed = 1;
>  		}
> +	}
> +	/* Get current value of FWDVA.*/
> +	mfcpr(CPR0_PLLD, reg);
> +	temp = (reg & PLLD_FWDVA_MASK) >> 16;

I would prefer, if you would insert an empty line before this new statement
complex (after the "}"): Like this:

	}
> +
	/* Get current value of FWDVA.*/
	mfcpr(CPR0_PLLD, reg);
	temp = (reg & PLLD_FWDVA_MASK) >> 16;

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

Again, an empty line makes it easier to read for my taste. Like this:

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

> +		mfcpr(CPR0_PLLD, reg);
> +		/* Get current value of fbdv.  */
> +		temp = (reg & PLLD_FBDV_MASK) >> 24;
> +		fbdv = temp ? temp : 32;
> +		/* Get current value of lfbdv. */
> +		temp = (reg & PLLD_LFBDV_MASK);
> +		lfbdv = temp ? temp : 64;
> +		/*
> +		 * Load register that contains current boot strapping option.
> +		 */
> +		mfcpr(CPR0_ICFG, reg);
> +		/* Shift strapping option into low 3 bits.*/
> +		reg = (reg >> 28);
> +
> +		if (reg == BOOT_STRAP_OPTION_A || reg == BOOT_STRAP_OPTION_B ||
> +		    reg == BOOT_STRAP_OPTION_D || reg == BOOT_STRAP_OPTION_E) {

I prefer to have parentheses around all the statements:

		if ((reg == BOOT_STRAP_OPTION_A) || (reg == BOOT_STRAP_OPTION_B) ||
		    (reg == BOOT_STRAP_OPTION_D) || (reg == BOOT_STRAP_OPTION_E)) {

> +			/*
> +			 * Get current value of FWDVA. Assign current FWDVA to
> +			 * new FWDVB.
> +			 */
> +			mfcpr(CPR0_PLLD, reg);
> +			target_fwdvb = (reg & PLLD_FWDVA_MASK) >> 16;
> +			fwdvb = target_fwdvb ? target_fwdvb : 8;
> +			/*
> +			 * Get current value of FWDVB. Assign current FWDVB to
> +			 * new FWDVA.
> +			 */
> +			target_fwdva = (reg & PLLD_FWDVB_MASK) >> 8;
> +			fwdva = target_fwdva ? target_fwdva : 16;
> +			/*
> +			 * Update CPR0_PLLD with switched FWDVA and FWDVB.
> +			 */
> +			reg &= ~(PLLD_FWDVA_MASK | PLLD_FWDVB_MASK |
> +				PLLD_FBDV_MASK | PLLD_LFBDV_MASK);
> +			reg |= ((fwdva == 16 ? 0 : fwdva) << 16) |
> +			((fwdvb == 8 ? 0 : fwdvb) << 8) |
> +			((fbdv == 32 ? 0 : fbdv) << 24) |
> +			(lfbdv == 64 ? 0 : lfbdv);

Indentation not correct:

			reg |= ((fwdva == 16 ? 0 : fwdva) << 16) |
				((fwdvb == 8 ? 0 : fwdvb) << 8) |
				((fbdv == 32 ? 0 : fbdv) << 24) |
				(lfbdv == 64 ? 0 : lfbdv);


Please fix and resubmit. Thanks.

Cheers,
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@denx.de