[U-Boot] [PATCH v1] PPC405EX CHIP_21 erratum
Steven A. Falco
sfalco at harris.com
Wed May 4 16:26:16 CEST 2011
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_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 <sfalco at harris.com>
>
> 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, 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: office at denx.de
>
--
A: Because it makes the logic of the discussion difficult to follow.
Q: Why shouldn't I top post?
A: No.
Q: Should I top post?
More information about the U-Boot
mailing list