Re: [U-Boot] [PATCH v1] PPC405EX CHIP_21 erratum
Thanks, I can confirm this fixes the issue for me (405EX Rev. D with security). Still checkpatch finds some whitespace issues: .dotest/patch:13: space before tab in indent. defined(CONFIG_405EX_CHIP21_PVR_REV_D) && \ .dotest/patch:27: trailing whitespace. * warning: 2 lines add whitespace errors. > 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_D0x0 >/* 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 */ I don't like defining expected PVR values in the board config file. Maybe something like CONFIG_405EX_CHIP21_SECURITY and CONFIG_405EX_CHIP21_NO_SECURITY would be more pleasant. Then you could place #ifdef CONFIG_405EX_CHIP21_SECURITY #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 */ #endif #ifdef CONFIG_405EX_CHIP21_NO_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 */ #endif in a more appropriate place. Cheers Dirk ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1] PPC405EX CHIP_21 erratum
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
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, 0x3000);
> +
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_PFC10x4101
> #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(0x8000 >> 31)
>
> #define SDR0_SRST_DMC(0x8000 >> 10)
> diff --git a/include/configs/kilauea.h b/include/configs/kilauea.h
> index 031f8fb..2d3efba 10
Re: [U-Boot] [PATCH v1] PPC405EX CHIP_21 erratum
On 05/04/2011 04:36 AM, Stefan Roese wrote:
> 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_C0x1291147d /* EX
>> without
> security */
>> #define CONFIG_405EX_CHIP21_PVR_REV_D0x12911473 /* EX
>> without
> security */
>> #define CONFIG_405EX_CHIP21_ECID3_REV_D 0x1/* EX
> without security */
>>
>> Signed-off-by: Steve Falco
>
> Thanks for the quick patch. Please find some comments below.
Thanks to both of you for the comments. New version will be posted soon.
Steve
>
>> ---
>>
>> 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, 0x3000);
>> +
>
> 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_MFR0x4300 /* SDR0_MFR reg */
>>
>> +#define SDR0_ECID0 0x0080
>> +#define SDR0_ECID1 0x0081
>> +#define SDR0_ECID2 0x0082
