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

[Stefan Roese <sr@denx.de>]

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