* [U-Boot] PPC405EX CHIP_21 erratum [not found] <4DBF1F33.4040101@harris.com> @ 2011-05-02 21:44 ` Steven A. Falco 2011-05-03 13:07 ` Stefan Roese 0 siblings, 1 reply; 6+ messages in thread From: Steven A. Falco @ 2011-05-02 21:44 UTC (permalink / raw) To: u-boot > Hi Steve, > > On Friday 29 April 2011 18:54:02 Steven A. Falco wrote: >> We've been bitten by the PPC405EX CHIP_21 erratum. > > How did it affect you exactly? Was an incorrect PVR detected? Did this result > in some problems (Linux etc)? The problem goes way beyond the PVR value. We have been getting "hangs" at the point that U-Boot is printing out the results of its PCIe discovery. We would get "PCIE1: successfully set as root-complex" and the board would hang forever. We traced this to an infinite loop in 4xx_pcie.c, which we "fixed" as follows: @ -724,8 +724,21 @@ int __ppc4xx_init_pcie_port_hw(int port, int rootport) SDR_WRITE(SDRN_PESDR_RCSSET(port), 0x01101000); /* poll for phy !reset */ - while (!(SDR_READ(SDRN_PESDR_PHYSTA(port)) & 0x00001000)) - ; + { + int i = 0; + + while (!(SDR_READ(SDRN_PESDR_PHYSTA(port)) & 0x00001000)) + { + udelay(1000); + + if (i++ > 2000) + { + printf("PCIE%d phy reset timed out, PHYSTA = 0x%08x\n", + port, SDR_READ(SDRN_PESDR_PHYSTA(port))); + break; + } + } + } /* deassert the PE0_gpl_utl_reset */ SDR_WRITE(SDRN_PESDR_RCSSET(port), 0x00101000); Basically, when we see the PVR issue, we also see that the PCIe phy never becomes ready. With the patch above, we would not hang in U-Boot. Instead we'd move on to load Linux, which would crash with an Oops. The board would then reboot, and begin running normally. This behavior is consistent with CHIP_21, which implies that on the second reboot the chip works properly. Note that this only happens with a few of our boards, and not 100%. We have some boards that are particularly bad, and others that always work perfectly. We contacted AMCC support. Their reply included the following: there was an errata for REVD in 405EX which required soft reset for PCIE to work correctly. Maybe this is a reason. The errata is CHIP_21: Incorrect Value read from the SDR0_ECID0:3 and PVR registers. And it affects PCIE reset as well. So apparently, this is a serious bug that really needs a workaround. Now the question is what is the best way to work around it. > >> I've looked through the >> U-Boot code, but it doesn't appear that there is a work-around for this >> one. > > Correct, there currently is no workaround. > >> The following patch is my adaptation of AMCC's suggestion as to the fix. >> But I have to say that I don't care for it, if for no other reason that it >> will break if a rev E comes out. >> >> Also, I made it #ifdef'd on CONFIG_405EX, but it will not work on a 405EXr. >> And I have seen a 405EX report its PVR as if it were a 405EXr. It's not >> looking good for the suggested work-around. >> >> AMCC claims this can be fixed in hardware, by always doing a double reset. >> Naturally, that is hard to implement and would mean massive rework of >> existing boards. > > Yes, there are many 405EX(r) boards in the field already. Such an hardware > workaround should really be avoided if possible. > >> So I am contemplating a different work-around, whereby software always >> resets the board on a cold boot (if such a thing can be reliably >> detected). That would hopefully be the equivalent of the hardware double >> reset, and would not be dependent on specific PVR values, making it more >> "future proof". >> >> Has anyone else run across this? Do you have similar concerns about the >> patch? > > No, I have not seen this problem before. And yes, I would also prefer your > "alternative suggestion", with the always reboot after powerup optionally > built into the U-Boot image. But as you already mentioned, we would have to > find a way to reliably detect the powerup reset, so that we don't end in an > reset-loop. I have not been able to find a way to distinguish a power-up versus a reboot. So for now, I've implemented the AMCC bug fix more or less as written. To do it right, we'd have to add entries in every U-Boot config file for the 405 EX/EXr to definitively specify the CPU type expected, and to put the list of allowed PVRs in the code. This means that the PVR is essentially useless to distinguish between the EX and EXr, because we have seen one misbehaving EX report as an EXr! If the PVR had been garbage, we could maybe use it, but reporting "wrong yet legal" values makes it worthless. All our boards use the EX, so we are able to live with this, but I realize it is not a solution for the larger community. That is why I have not proposed a definitive patch. Steve > > 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? ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] PPC405EX CHIP_21 erratum 2011-05-02 21:44 ` [U-Boot] PPC405EX CHIP_21 erratum Steven A. Falco @ 2011-05-03 13:07 ` Stefan Roese 2011-05-03 16:59 ` [U-Boot] [PATCH v1] " Steven A. Falco 0 siblings, 1 reply; 6+ messages in thread From: Stefan Roese @ 2011-05-03 13:07 UTC (permalink / raw) To: u-boot On Monday 02 May 2011 23:44:58 Steven A. Falco wrote: > I have not been able to find a way to distinguish a power-up versus a > reboot. So for now, I've implemented the AMCC bug fix more or less as > written. To do it right, we'd have to add entries in every U-Boot config > file for the 405 EX/EXr to definitively specify the CPU type expected, and > to put the list of allowed PVRs in the code. > > This means that the PVR is essentially useless to distinguish between the > EX and EXr, because we have seen one misbehaving EX report as an EXr! > If the PVR had been garbage, we could maybe use it, but reporting "wrong > yet legal" values makes it worthless. > > All our boards use the EX, so we are able to live with this, but I realize > it is not a solution for the larger community. That is why I have not > proposed a definitive patch. I see. Thanks for the explanation. IMHO, it would be best to provide a default list of PVR's that are allowed/correct for 405EX(r). And this list can be overridden by a board specific version (as needed in your case). Not sure how to handle this elegantly in assembler though. We might move this to cpu_init_f(). This is the first C function called via start.S. It already handles similar issues for some 4xx SoC variants (reconfigure_pll()) which require a system reboot. What do you think? Would you be able to prepare a patch with such a CHIP_21 fix? 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH v1] PPC405EX CHIP_21 erratum 2011-05-03 13:07 ` Stefan Roese @ 2011-05-03 16:59 ` Steven A. Falco 2011-05-04 8:03 ` Eibach, Dirk 2011-05-04 8:36 ` Stefan Roese 0 siblings, 2 replies; 6+ messages in thread From: Steven A. Falco @ 2011-05-03 16:59 UTC (permalink / raw) To: u-boot 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@harris.com> --- 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: + 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) && + (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); + +} +#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 + 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 */ + +/* * Include common defines/options for all AMCC eval boards */ #define CONFIG_HOSTNAME kilauea ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH v1] PPC405EX CHIP_21 erratum 2011-05-03 16:59 ` [U-Boot] [PATCH v1] " Steven A. Falco @ 2011-05-04 8:03 ` Eibach, Dirk 2011-05-04 8:36 ` Stefan Roese 1 sibling, 0 replies; 6+ messages in thread From: Eibach, Dirk @ 2011-05-04 8:03 UTC (permalink / raw) To: u-boot 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_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 */ 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH v1] PPC405EX CHIP_21 erratum 2011-05-03 16:59 ` [U-Boot] [PATCH v1] " Steven A. Falco 2011-05-04 8:03 ` Eibach, Dirk @ 2011-05-04 8:36 ` Stefan Roese 2011-05-04 14:26 ` Steven A. Falco 1 sibling, 1 reply; 6+ messages in thread From: Stefan Roese @ 2011-05-04 8:36 UTC (permalink / raw) To: u-boot 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@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: office at denx.de ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH v1] PPC405EX CHIP_21 erratum 2011-05-04 8:36 ` Stefan Roese @ 2011-05-04 14:26 ` Steven A. Falco 0 siblings, 0 replies; 6+ messages in thread From: Steven A. Falco @ 2011-05-04 14:26 UTC (permalink / raw) To: u-boot 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@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? ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-05-04 14:26 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <4DBF1F33.4040101@harris.com>
2011-05-02 21:44 ` [U-Boot] PPC405EX CHIP_21 erratum Steven A. Falco
2011-05-03 13:07 ` Stefan Roese
2011-05-03 16:59 ` [U-Boot] [PATCH v1] " Steven A. Falco
2011-05-04 8:03 ` Eibach, Dirk
2011-05-04 8:36 ` Stefan Roese
2011-05-04 14:26 ` Steven A. Falco
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox