[U-Boot] PPC405EX CHIP_21 erratum

[U-Boot] PPC405EX CHIP_21 erratum

[Steven A. Falco]

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

[U-Boot] PPC405EX CHIP_21 erratum

[Stefan Roese]

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

[U-Boot] [PATCH v1] PPC405EX CHIP_21 erratum

[Steven A. Falco]

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

[U-Boot] [PATCH v1] PPC405EX CHIP_21 erratum

[Eibach, Dirk]

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

[U-Boot] [PATCH v1] PPC405EX CHIP_21 erratum

[Stefan Roese]

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

[U-Boot] [PATCH v1] PPC405EX CHIP_21 erratum

[Steven A. Falco]

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?