Hi Rup,

thanks for this update. Only some minor issue which should be fixed before I 
push this patch:

- You changed the subject from
  "ppc4xx fix unstable 440EPx bootstrap options" to
  "ppc4xx fix unstable 440EPX boostrap options"
  This now has a spelling error and makes it harder to see that this is a new
  revision of this patch. Please use the original subject in the next patch
  version again. To differentiate from the first patch, add "v3" to
  "[PATCH]". The complete subject should look like this:
  "[PATCH v3] ppc4xx fix unstable 440EPx bootstrap options"
  And please add a small description on what you really changed below the
  "---" line in the patch.

One more nitpicking comment below.

On Monday 15 March 2010 13:58:04 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.
> 
> Signed-off-by: Rupjyoti Sarmah <rsar...@appliedmicro.com>
> Acked-by : Victor Gallardo <vgalla...@appliedmicro.com>
> ---
>  cpu/ppc4xx/cpu_init.c |   65
> +++++++++++++++++++++++++++++++++++++++++++++---- include/ppc440.h      | 
>   6 ++++
>  2 files changed, 66 insertions(+), 5 deletions(-)
> 
> diff --git a/cpu/ppc4xx/cpu_init.c b/cpu/ppc4xx/cpu_init.c
> index ccd9993..8a6e545 100644
> --- a/cpu/ppc4xx/cpu_init.c
> +++ b/cpu/ppc4xx/cpu_init.c
> @@ -111,17 +111,72 @@ 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;
> 
> -             /* Set reload inhibit so configuration will persist across
> -              * processor resets */
> +     /*
> +      * 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;
> +             }
> +     }
> +
> +     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);
>       }
> diff --git a/include/ppc440.h b/include/ppc440.h
> index e60fa13..4182944 100644
> --- a/include/ppc440.h
> +++ b/include/ppc440.h
> @@ -68,6 +68,12 @@
>  #define CPR0_SPCID   0x0120
>  #define CPR0_ICFG    0x0140
> 
> +/* 440EPX boot strap options */
> +#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 don't indent using spaces. Use tabs here.

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

Reply via email to