From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steven A. Falco Date: Wed, 04 May 2011 10:26:16 -0400 Subject: [U-Boot] [PATCH v1] PPC405EX CHIP_21 erratum In-Reply-To: <201105041036.20193.sr@denx.de> References: <4DBF1F33.4040101@harris.com> <201105031507.41274.sr@denx.de> <4DC03454.4070304@harris.com> <201105041036.20193.sr@denx.de> Message-ID: <4DC16208.1020605@harris.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 > > 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?