PPC405EX CHIP_21 erratum

U-Boot New Steven A. Falco 3 participants 6 messages May 2, 2011 – May 4, 2011 1 message not in the archive PowerPC

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Steven A. Falco

author

2011-05-02 21:44 UTC #1 lore raw

Quoted 7 lines

Hi Steve,
Quoted 1 line · 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.

Quoted 28 lines

Quoted 4 lines
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.
Quoted 11 lines
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.
Quoted 6 lines
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".

Quoted 8 lines

Quoted 2 lines
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

Quoted 9 lines

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

Signature · 4 lines

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?

Stefan Roese

2011-05-03 13:07 UTC #2 lore raw

Quoted 6 lines · 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.

Quoted 8 lines

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

Steven A. Falco

author

2011-05-03 16:59 UTC #3 lore raw

PATCH 1/1 PPC405EX CHIP_21 erratum

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 <[email protected]>

---

arch/powerpc/cpu/ppc4xx/cpu_init.c +69 -0

index bf208ad..89a577b 100644
@@ -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)) && \

arch/powerpc/include/asm/ppc405ex.h +5 -0

index 36d3149..8070385 100644
@@ -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)

include/configs/kilauea.h +11 -0

index 031f8fb..2d3efba 100644
@@ -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

Eibach

2011-05-04 08:03 UTC #4 lore raw

PATCH 1/1 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.

Quoted 18 lines

(quoted diff) +0 -0
b/include/configs/kilauea.h index 031f8fb..2d3efba 100644
include/configs/kilauea.h +3 -0
@@ -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 */
include/configs/kilauea.h +3 -0
+//#define CONFIG_405EX_CHIP21_ECID3_REV_D		0x0	
   /* EX with security */
+
+#define CONFIG_405EX_CHIP21_PVR_REV_C		

Quoted 5 lines

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

Stefan Roese

2011-05-04 08:36 UTC #5 lore raw

PATCH 1/1 PPC405EX CHIP_21 erratum

In reply to Steven A. Falco

Hi Steve,

Quoted 11 lines · 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 */

Quoted 1 line

#define CONFIG_405EX_CHIP21_PVR_REV_D 0x12911473 /* EX without

security */

Quoted 1 line

#define CONFIG_405EX_CHIP21_ECID3_REV_D 0x1 /* EX

without security */

Quoted 2 lines

Signed-off-by Steve Falco <[email protected]>

Thanks for the quick patch. Please find some comments below.

Quoted 36 lines

---

(quoted diff) +0 -0

b/arch/powerpc/cpu/ppc4xx/cpu_init.c index bf208ad..89a577b 100644

arch/powerpc/cpu/ppc4xx/cpu_init.c +26 -0

@@ -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

arch/powerpc/cpu/ppc4xx/cpu_init.c void reconfigure_pll(u32 new_cpu_freq) · line 259

Quoted 10 lines

arch/powerpc/cpu/ppc4xx/cpu_init.c +10 -0

+	 * 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.

arch/powerpc/cpu/ppc4xx/cpu_init.c void reconfigure_pll(u32 new_cpu_freq) · line 267

Quoted 8 lines

arch/powerpc/cpu/ppc4xx/cpu_init.c +8 -0

+	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.

Quoted 6 lines

arch/powerpc/cpu/ppc4xx/cpu_init.c +6 -0

+			(pvr_28_31 != ecid3_12_15)) {
+		// No reset required.
+		return;
+	}
+
+	// Step 3:

arch/powerpc/cpu/ppc4xx/cpu_init.c void reconfigure_pll(u32 new_cpu_freq) · line 283

Quoted 10 lines

arch/powerpc/cpu/ppc4xx/cpu_init.c +10 -0

+	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.

Quoted 7 lines

arch/powerpc/cpu/ppc4xx/cpu_init.c +3 -0
+}
+#endif
+
 /*
  * Breath some life into the CPU...
  *

arch/powerpc/cpu/ppc4xx/cpu_init.c cpu_init_f (void) · line 306

Quoted 9 lines

arch/powerpc/cpu/ppc4xx/cpu_init.c +6 -0

@@ -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?

Quoted 24 lines

arch/powerpc/cpu/ppc4xx/cpu_init.c +0 -0

 	reconfigure_pll(CONFIG_SYS_PLL_RECONFIG);
 
 #if (defined(CONFIG_405EP) || defined (CONFIG_405EX)) && \

(quoted diff) +0 -0

b/arch/powerpc/include/asm/ppc405ex.h index 36d3149..8070385 100644

arch/powerpc/include/asm/ppc405ex.h +5 -0

@@ -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)

include/configs/kilauea.h +0 -0

index 031f8fb..2d3efba 100644

include/configs/kilauea.h line 49

Quoted 6 lines

include/configs/kilauea.h +3 -0

@@ -44,6 +44,17 @@
 #endif
 
 /*
+ * CHIP_21 errata
+ */
+//#define CONFIG_405EX_CHIP21_PVR_REV_C		0x1291147f /* EX with

security */

include/configs/kilauea.h line 50

Quoted 1 line

include/configs/kilauea.h +1 -0

+//#define CONFIG_405EX_CHIP21_PVR_REV_D		0x12911475 /* EX with

security */

include/configs/kilauea.h line 51

Quoted 1 line

include/configs/kilauea.h +1 -0

+//#define CONFIG_405EX_CHIP21_ECID3_REV_D		0x0	   /* EX with

security */

include/configs/kilauea.h line 53

Quoted 2 lines

include/configs/kilauea.h +2 -0

+
+#define CONFIG_405EX_CHIP21_PVR_REV_C		0x1291147d /* EX

without security

include/configs/kilauea.h line 54

Quoted 1 line

include/configs/kilauea.h +0 -0

 */ +#define CONFIG_405EX_CHIP21_PVR_REV_D		0x12911473 /* EX

without

Quoted 1 line

security */ +#define CONFIG_405EX_CHIP21_ECID3_REV_D 0x1 /*

EX without

Quoted 1 line

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

Steven A. Falco

author

2011-05-04 14:26 UTC #6 lore raw

PATCH 1/1 PPC405EX CHIP_21 erratum

Quoted 15 lines · Stefan Roese

Hi Steve,
Quoted 11 lines · 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 */

Quoted 8 lines

Quoted 1 line
#define CONFIG_405EX_CHIP21_PVR_REV_D 0x12911473 /* EX without
security */
Quoted 1 line
#define CONFIG_405EX_CHIP21_ECID3_REV_D 0x1 /* EX
without security */
Quoted 2 lines
Signed-off-by Steve Falco <[email protected]>
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

Quoted 166 lines

Quoted 37 lines
---
(quoted diff) +0 -0
b/arch/powerpc/cpu/ppc4xx/cpu_init.c index bf208ad..89a577b 100644
arch/powerpc/cpu/ppc4xx/cpu_init.c +26 -0
@@ -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
arch/powerpc/cpu/ppc4xx/cpu_init.c void reconfigure_pll(u32 new_cpu_freq) · line 259
Quoted 10 lines
arch/powerpc/cpu/ppc4xx/cpu_init.c +10 -0
+	 * 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:
(quoted diff) +1 -0
+	/* Step 1: */
And please fix this globally.
arch/powerpc/cpu/ppc4xx/cpu_init.c void reconfigure_pll(u32 new_cpu_freq) · line 267
Quoted 8 lines
arch/powerpc/cpu/ppc4xx/cpu_init.c +8 -0
+	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.
Quoted 6 lines
arch/powerpc/cpu/ppc4xx/cpu_init.c +6 -0
+			(pvr_28_31 != ecid3_12_15)) {
+		// No reset required.
+		return;
+	}
+
+	// Step 3:
arch/powerpc/cpu/ppc4xx/cpu_init.c void reconfigure_pll(u32 new_cpu_freq) · line 283
Quoted 10 lines
arch/powerpc/cpu/ppc4xx/cpu_init.c +10 -0
+	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.
Quoted 7 lines
arch/powerpc/cpu/ppc4xx/cpu_init.c +3 -0
+}
+#endif
+
 /*
  * Breath some life into the CPU...
  *
arch/powerpc/cpu/ppc4xx/cpu_init.c cpu_init_f (void) · line 306
Quoted 9 lines
arch/powerpc/cpu/ppc4xx/cpu_init.c +6 -0
@@ -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?
Quoted 24 lines
arch/powerpc/cpu/ppc4xx/cpu_init.c +0 -0
 	reconfigure_pll(CONFIG_SYS_PLL_RECONFIG);
 
 #if (defined(CONFIG_405EP) || defined (CONFIG_405EX)) && \
(quoted diff) +0 -0
b/arch/powerpc/include/asm/ppc405ex.h index 36d3149..8070385 100644
arch/powerpc/include/asm/ppc405ex.h +5 -0
@@ -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)
include/configs/kilauea.h +0 -0
index 031f8fb..2d3efba 100644
include/configs/kilauea.h line 49
Quoted 6 lines
include/configs/kilauea.h +3 -0
@@ -44,6 +44,17 @@
 #endif
 
 /*
+ * CHIP_21 errata
+ */
+//#define CONFIG_405EX_CHIP21_PVR_REV_C		0x1291147f /* EX with 
security */
include/configs/kilauea.h line 50
Quoted 1 line
include/configs/kilauea.h +1 -0
+//#define CONFIG_405EX_CHIP21_PVR_REV_D		0x12911475 /* EX with 
security */
include/configs/kilauea.h line 51
Quoted 1 line
include/configs/kilauea.h +1 -0
+//#define CONFIG_405EX_CHIP21_ECID3_REV_D		0x0	   /* EX with 
security */
include/configs/kilauea.h line 53
Quoted 2 lines
include/configs/kilauea.h +2 -0
+
+#define CONFIG_405EX_CHIP21_PVR_REV_C		0x1291147d /* EX 
without security
include/configs/kilauea.h line 54
Quoted 1 line
include/configs/kilauea.h +0 -0
 */ +#define CONFIG_405EX_CHIP21_PVR_REV_D		0x12911473 /* EX 
without
Quoted 1 line
security */ +#define CONFIG_405EX_CHIP21_ECID3_REV_D 0x1 /*
EX without
Quoted 1 line
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

Signature · 4 lines

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?