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

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

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

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

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
>

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
>

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

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