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 <rsar...@appliedmicro.com> > Acked-by : Victor Gallardo <vgalla...@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: off...@denx.de _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot