[U-Boot] [PATCH] powerpc/85xx: Add workaround for errata USB-14 (enable on P204x/P3041/P50x0)

[U-Boot] [PATCH] powerpc/85xx: Add workaround for errata USB-14 (enable on P204x/P3041/P50x0)

[xulei]

On P204x/P304x/P50x0 Rev1.0, USB transmit will result in false internal
multi-bit ECC errors, which has impact on performance, so software should
disable all ECC reporting from USB1 and USB2 by setting bits 16 and 17 to 1
in the register at DCSRBASE + 0x0002_0520.

In formal release document, the errata number should be USB14 instead of USB138.

Signed-off-by: xulei <Lei.Xu@freescale.com>
Signed-off-by: Roy Zang <tie-fei.zang@freescale.com>
Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
---
 arch/powerpc/cpu/mpc85xx/cmd_errata.c     |    3 +++
 arch/powerpc/cpu/mpc85xx/cpu_init.c       |   13 +++++++++++++
 arch/powerpc/include/asm/config_mpc85xx.h |    5 ++++-
 3 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/cpu/mpc85xx/cmd_errata.c b/arch/powerpc/cpu/mpc85xx/cmd_errata.c
index 5d72f4c..422782c 100644
--- a/arch/powerpc/cpu/mpc85xx/cmd_errata.c
+++ b/arch/powerpc/cpu/mpc85xx/cmd_errata.c
@@ -255,6 +255,9 @@ static int do_errata(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 #ifdef CONFIG_SYS_P4080_ERRATUM_PCIE_A003
 	puts("Work-around for Erratum PCIe-A003 enabled\n");
 #endif
+#ifdef CONFIG_SYS_FSL_ERRATUM_USB14
+	puts("Work-around for Erratum USB14 enabled\n");
+#endif
 	return 0;
 }
 
diff --git a/arch/powerpc/cpu/mpc85xx/cpu_init.c b/arch/powerpc/cpu/mpc85xx/cpu_init.c
index de9d916..72c5328 100644
--- a/arch/powerpc/cpu/mpc85xx/cpu_init.c
+++ b/arch/powerpc/cpu/mpc85xx/cpu_init.c
@@ -623,6 +623,19 @@ skip_l2:
 	}
 #endif
 
+	/* On P204x/P304x/P50x0 Rev1.0, USB transmit will result internal
+	 * multi-bit ECC errors, which has impact on performance, so software
+	 * should disable all ECC reporting from USB1 and USB2 by setting bits
+	 * 16 and 17 to 1 in the register at DCSRBASE + 0x0002_0520.
+	 */
+#ifdef CONFIG_SYS_FSL_ERRATUM_USB14
+	if (IS_SVR_REV(get_svr(), 1, 0)) {
+		void *p;
+		p = (void *)CONFIG_SYS_DCSRBAR + 0x20520;
+		setbits_be32(p, 3 << (31 - 17));
+	}
+#endif
+
 #ifdef CONFIG_FMAN_ENET
 	fman_enet_init();
 #endif
diff --git a/arch/powerpc/include/asm/config_mpc85xx.h b/arch/powerpc/include/asm/config_mpc85xx.h
index d57c178..4236835 100644
--- a/arch/powerpc/include/asm/config_mpc85xx.h
+++ b/arch/powerpc/include/asm/config_mpc85xx.h
@@ -333,6 +333,7 @@
 #define CONFIG_SYS_FSL_USB_INTERNAL_UTMI_PHY
 #define CONFIG_SYS_FSL_ERRATUM_ESDHC111
 #define CONFIG_SYS_FSL_ERRATUM_NMG_CPU_A011
+#define CONFIG_SYS_FSL_ERRATUM_USB14
 #define CONFIG_SYS_FSL_ERRATUM_CPU_A003999
 #define CONFIG_SYS_FSL_ERRATUM_DDR_A003474
 #define CONFIG_SYS_FSL_SRIO_PCIE_BOOT_MASTER
@@ -365,6 +366,7 @@
 #define CONFIG_SYS_FSL_USB_INTERNAL_UTMI_PHY
 #define CONFIG_SYS_FSL_ERRATUM_ESDHC111
 #define CONFIG_SYS_FSL_ERRATUM_NMG_CPU_A011
+#define CONFIG_SYS_FSL_ERRATUM_USB14
 #define CONFIG_SYS_FSL_ERRATUM_CPU_A003999
 #define CONFIG_SYS_FSL_ERRATUM_DDR_A003474
 #define CONFIG_SYS_FSL_SRIO_PCIE_BOOT_MASTER
@@ -442,6 +444,7 @@
 #define CONFIG_SYS_FSL_USB2_PHY_ENABLE
 #define CONFIG_SYS_FSL_USB_INTERNAL_UTMI_PHY
 #define CONFIG_SYS_FSL_ERRATUM_ESDHC111
+#define CONFIG_SYS_FSL_ERRATUM_USB14
 #define CONFIG_SYS_FSL_ERRATUM_DDR_A003474
 #define CONFIG_SYS_FSL_SRIO_PCIE_BOOT_MASTER
 #define CONFIG_SYS_FSL_SRIO_MAX_PORTS	2
@@ -473,7 +476,7 @@
 #define CONFIG_SYS_FSL_USB2_PHY_ENABLE
 #define CONFIG_SYS_FSL_USB_INTERNAL_UTMI_PHY
 #define CONFIG_SYS_FSL_ERRATUM_ESDHC111
-#define CONFIG_SYS_FSL_ERRATUM_USB138
+#define CONFIG_SYS_FSL_ERRATUM_USB14
 #define CONFIG_SYS_FSL_ERRATUM_DDR_A003
 #define CONFIG_SYS_FSL_ERRATUM_DDR_A003474
 #define CONFIG_SYS_FSL_ERRATUM_A004699
-- 
1.7.0.4

[U-Boot] [PATCH] powerpc/85xx: Add workaround for errata USB-14 (enable on P204x/P3041/P50x0)

[Wolfgang Denk]

Dear xulei,

In message <1362368146-738-1-git-send-email-B33228@freescale.com> you wrote:
>
> +	/* On P204x/P304x/P50x0 Rev1.0, USB transmit will result internal
> +	 * multi-bit ECC errors, which has impact on performance, so software
> +	 * should disable all ECC reporting from USB1 and USB2 by setting bits
> +	 * 16 and 17 to 1 in the register at DCSRBASE + 0x0002_0520.
> +	 */
> +#ifdef CONFIG_SYS_FSL_ERRATUM_USB14
> +	if (IS_SVR_REV(get_svr(), 1, 0)) {
> +		void *p;
> +		p = (void *)CONFIG_SYS_DCSRBAR + 0x20520;
> +		setbits_be32(p, 3 << (31 - 17));
> +	}
> +#endif

Please move the comment inside the #ifdef block.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I distrust all systematisers, and avoid them. The will  to  a  system
shows a lack of honesty.
- Friedrich Wilhelm Nietzsche _G?tzen-D?mmerung [The Twilight of  the
Idols]_ ``Maxims and Missiles'' no. 26

[U-Boot] [PATCH] powerpc/85xx: Add workaround for errata USB-14 (enable on P204x/P3041/P50x0)

[Wolfgang Denk]

Dear xulei,

In message <1362368146-738-1-git-send-email-B33228@freescale.com> you wrote:
> On P204x/P304x/P50x0 Rev1.0, USB transmit will result in false internal
> multi-bit ECC errors, which has impact on performance, so software should
> disable all ECC reporting from USB1 and USB2 by setting bits 16 and 17 to 1
> in the register at DCSRBASE + 0x0002_0520.
...
> +		void *p;
> +		p = (void *)CONFIG_SYS_DCSRBAR + 0x20520;

Um... and don't we have a proper C struct that describes the registers
in this area?

We don't allow base address + offset addressing like this.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Just because your doctor has a name for your condition  doesn't  mean
he knows what it is.

[U-Boot] [PATCH] powerpc/85xx: Add workaround for errata USB-14 (enable on P204x/P3041/P50x0)

[xulei]

Hello, Wolfgang

	Thank you, will modify it in the next version.

Regards
Lei

On Monday, 2013-03-04 at 10:24 +0100, Wolfgang Denk wrote:
> Dear xulei,
> 
> In message <1362368146-738-1-git-send-email-B33228@freescale.com> you wrote:
> >
> > +	/* On P204x/P304x/P50x0 Rev1.0, USB transmit will result internal
> > +	 * multi-bit ECC errors, which has impact on performance, so software
> > +	 * should disable all ECC reporting from USB1 and USB2 by setting bits
> > +	 * 16 and 17 to 1 in the register at DCSRBASE + 0x0002_0520.
> > +	 */
> > +#ifdef CONFIG_SYS_FSL_ERRATUM_USB14
> > +	if (IS_SVR_REV(get_svr(), 1, 0)) {
> > +		void *p;
> > +		p = (void *)CONFIG_SYS_DCSRBAR + 0x20520;
> > +		setbits_be32(p, 3 << (31 - 17));
> > +	}
> > +#endif
> 
> Please move the comment inside the #ifdef block.
> 
> Best regards,
> 
> Wolfgang Denk
> 

[U-Boot] [PATCH] powerpc/85xx: Add workaround for errata USB-14 (enable on P204x/P3041/P50x0)

[xulei]

Hello, Wolfgang

	Thank you and I agree with you. It is a little ugly but because these
registers info are not publicly, so I did not use C struct to describe
them, then for this case it it ok or other method such as define a
struct but keep all other registers and bits in this register reserved?
or any method better? Thank you.

Regards
Lei

On Monday, 2013-03-04 at 10:26 +0100, Wolfgang Denk wrote:
> Dear xulei,
> 
> In message <1362368146-738-1-git-send-email-B33228@freescale.com> you wrote:
> > On P204x/P304x/P50x0 Rev1.0, USB transmit will result in false internal
> > multi-bit ECC errors, which has impact on performance, so software should
> > disable all ECC reporting from USB1 and USB2 by setting bits 16 and 17 to 1
> > in the register at DCSRBASE + 0x0002_0520.
> ...
> > +		void *p;
> > +		p = (void *)CONFIG_SYS_DCSRBAR + 0x20520;
> 
> Um... and don't we have a proper C struct that describes the registers
> in this area?
> 
> We don't allow base address + offset addressing like this.
> 
> Best regards,
> 
> Wolfgang Denk
> 

[U-Boot] [PATCH] powerpc/85xx: Add workaround for errata USB-14 (enable on P204x/P3041/P50x0)

[Wolfgang Denk]

Dear Xu Lei-B33228,

Please do not top post / full quote.  Thanks.

In message <8CB6A38ADF9E994697FF8A45E96E085338810E@039-SN1MPN1-004.039d.mgd.msft.net> you wrote:
> 
> 	Thank you and I agree with you. It is a little ugly but because these registers info are not publicly, so I did not use C struct to describe them, 
> for this case is it ok, or other method such as define a struct but keep all other registers and bits in this register reserved? Thank you.

I'm not throwing in a formal NAK here, but for reasons of consistency
(and because others are just too eager to quote such patches as
authoritative precedent) I'd prefer the use of a struct.

Thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Space is big. You just won't believe how vastly, hugely, mind-
bogglingly big it is. I mean, you may think it's a long way down the
road to the drug store, but that's just peanuts to space.
                              -- The Hitchhiker's Guide to the Galaxy

[U-Boot] [PATCH] powerpc/85xx: Add workaround for errata USB-14 (enable on P204x/P3041/P50x0)

[Kumar Gala]

On Mar 5, 2013, at 2:40 PM, Wolfgang Denk wrote:

> Dear Xu Lei-B33228,
> 
> Please do not top post / full quote.  Thanks.
> 
> In message <8CB6A38ADF9E994697FF8A45E96E085338810E@039-SN1MPN1-004.039d.mgd.msft.net> you wrote:
>> 
>> 	Thank you and I agree with you. It is a little ugly but because these registers info are not publicly, so I did not use C struct to describe them, 
>> for this case is it ok, or other method such as define a struct but keep all other registers and bits in this register reserved? Thank you.
> 
> I'm not throwing in a formal NAK here, but for reasons of consistency
> (and because others are just too eager to quote such patches as
> authoritative precedent) I'd prefer the use of a struct.

While I'd prefer a struct as well, unfortunately this isn't something FSL has documented and we publish the erratum write ups with address like this.  So when a customer comes and looks for the code its more inline with the erratum writeup.

I'm not sure what value a dummy struct would provide above the explicit addressing utilized in the patch.

Again, I'm not happy about the situation, just not sure what additional value doing something like:

struct dummy_usb_dscr {
	u8 res[0x520];
	u32 erratum_usb14_reg;
	u8 res[4096-0x524];
};

Its going to be a bit more error prone than the method used when the struct needs updating for a new register field and when someone screws up getting the res[] sizes correct.

- k