Hi Steve,
On Tuesday 03 May 2011 18:59:00 Steven A. Falco wrote:
> APM errata CHIP_21 for the 405EX/EXr (from the rev 1.09 document dated
> 4/27/11) states that rev D processors may wake up with the wrong feature
> set. I've personally seen that happen. This patch implements the
> APM-proposed workaround.
>
> Note that you cannot blindly use this workaround. You must add/customize
> the following three defines to match your hardware. If you get this
> wrong, the processor will go into an infinite reset loop, and JTAG will be
> required to recover.
>
> #define CONFIG_405EX_CHIP21_PVR_REV_C 0x1291147d /* EX without
security */
> #define CONFIG_405EX_CHIP21_PVR_REV_D 0x12911473 /* EX without
security */
> #define CONFIG_405EX_CHIP21_ECID3_REV_D 0x1 /* EX
without security */
>
> Signed-off-by: Steve Falco <sfa...@harris.com>
Thanks for the quick patch. Please find some comments below.
> ---
>
> diff --git a/arch/powerpc/cpu/ppc4xx/cpu_init.c
> b/arch/powerpc/cpu/ppc4xx/cpu_init.c index bf208ad..89a577b 100644
> --- a/arch/powerpc/cpu/ppc4xx/cpu_init.c
> +++ b/arch/powerpc/cpu/ppc4xx/cpu_init.c
> @@ -221,6 +221,69 @@ void reconfigure_pll(u32 new_cpu_freq)
> #endif
> }
>
> +#if defined(CONFIG_405EX_CHIP21_PVR_REV_C) && \
> + defined(CONFIG_405EX_CHIP21_PVR_REV_D) && \
> + defined(CONFIG_405EX_CHIP21_ECID3_REV_D)
> +void
> +chip_21_errata (void)
> +{
> + /*
> + * See rev 1.09 of the 405EX/405EXr errata. CHIP_21 says that
> + * sometimes reading the PVR and/or SDR0_ECID results in incorrect
> + * values. Since the rev-D chip uses the SDR0_ECID bits to control
> + * internal features, that means the second PCIe or ethernet of an EX
> + * variant could fail to work. Also, security features of both EX and
> + * EXr might be incorrectly disabled.
> + *
> + * The suggested workaround is as follows (covering rev-C and rev-D):
> + *
> + * 1.Read the PVR and SDR0_ECID3.
> + *
> + * 2.If the PVR matches an expected Revision C PVR value AND if
> + * SDR0_ECID3[12:15] is different from PVR[28:31], then – processor is
> + * Revision C: continue executing the initialization code (no reset
> + * required). else – go to step 3.
> + *
> + * 3.If the PVR matches an expected Revision D PVR value AND if
> + * SDR0_ECID3[10:11] matches its expected value, then – continue
> + * executing initialization code, no reset required. else – write
> + * DBCR0[RST] = 0b11 to generate a SysReset.
> + */
> +
> + u32 pvr;
> + u32 pvr_28_31;
> + u32 ecid3;
> + u32 ecid3_10_11;
> + u32 ecid3_12_15;
> +
> + // Step 1:
Incorrect comment style, please use:
+ /* Step 1: */
And please fix this globally.
> + pvr = get_pvr();
> + mfsdr(SDR0_ECID3, ecid3);
> +
> + // Step 2:
> + pvr_28_31 = pvr & 0xf;
> + ecid3_10_11 = (ecid3 >> 20) & 0x3;
> + ecid3_12_15 = (ecid3 >> 16) & 0xf;
> + if((pvr == CONFIG_405EX_CHIP21_PVR_REV_C) &&
Space after "if". Please fix globally.
> + (pvr_28_31 != ecid3_12_15)) {
> + // No reset required.
> + return;
> + }
> +
> + // Step 3:
> + if((pvr == CONFIG_405EX_CHIP21_PVR_REV_D) &&
> + (ecid3_10_11 == CONFIG_405EX_CHIP21_ECID3_REV_D)) {
> + // No reset required.
> + return;
> + }
> +
> + // Reset required.
> + __asm__ __volatile__ ("sync; isync");
> + mtspr(SPRN_DBCR0, 0x30000000);
> +
Empty line should be removed.
> +}
> +#endif
> +
> /*
> * Breath some life into the CPU...
> *
> @@ -235,6 +298,12 @@ cpu_init_f (void)
> u32 val;
> #endif
>
> +#if defined(CONFIG_405EX_CHIP21_PVR_REV_C) && \
> + defined(CONFIG_405EX_CHIP21_PVR_REV_D) && \
> + defined(CONFIG_405EX_CHIP21_ECID3_REV_D)
> + chip_21_errata();
> +#endif
> +
Hmmm. I don't really like this "#if" here. How about something like this:
#ifdef CONFIG_SYS_4xx_CHIP_21_ERRATA
chip_21_errata();
#endif
And then define CONFIG_SYS_4xx_CHIP_21_ERRATA in your board config header.
What do you think?
> reconfigure_pll(CONFIG_SYS_PLL_RECONFIG);
>
> #if (defined(CONFIG_405EP) || defined (CONFIG_405EX)) && \
> diff --git a/arch/powerpc/include/asm/ppc405ex.h
> b/arch/powerpc/include/asm/ppc405ex.h index 36d3149..8070385 100644
> --- a/arch/powerpc/include/asm/ppc405ex.h
> +++ b/arch/powerpc/include/asm/ppc405ex.h
> @@ -43,6 +43,11 @@
> #define SDR0_PFC1 0x4101
> #define SDR0_MFR 0x4300 /* SDR0_MFR reg */
>
> +#define SDR0_ECID0 0x0080
> +#define SDR0_ECID1 0x0081
> +#define SDR0_ECID2 0x0082
> +#define SDR0_ECID3 0x0083
> +
> #define SDR0_SDCS_SDD (0x80000000 >> 31)
>
> #define SDR0_SRST_DMC (0x80000000 >> 10)
> diff --git a/include/configs/kilauea.h b/include/configs/kilauea.h
> index 031f8fb..2d3efba 100644
> --- a/include/configs/kilauea.h
> +++ b/include/configs/kilauea.h
> @@ -44,6 +44,17 @@
> #endif
>
> /*
> + * CHIP_21 errata
> + */
> +//#define CONFIG_405EX_CHIP21_PVR_REV_C 0x1291147f /* EX with
security */
> +//#define CONFIG_405EX_CHIP21_PVR_REV_D 0x12911475 /* EX with
security */
> +//#define CONFIG_405EX_CHIP21_ECID3_REV_D 0x0 /* EX with
security */
> +
> +#define CONFIG_405EX_CHIP21_PVR_REV_C 0x1291147d /* EX
without security
> */ +#define CONFIG_405EX_CHIP21_PVR_REV_D 0x12911473 /* EX
without
> security */ +#define CONFIG_405EX_CHIP21_ECID3_REV_D 0x1 /*
EX without
> security */
As Dirk already mentioned, these defines should not be placed in the board
config header. We already have those PVR defines in
arch/powerpc/include/asm/processor.h. Please use those defines instead.
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
