[U-Boot] [PATCH] ARM: MX5: Remove broken leftover TO-2 errata workaround

[U-Boot] [PATCH] ARM: MX5: Remove broken leftover TO-2 errata workaround

[David Jander]

This check is broken. r3 does not contain the silicon revision.

Signed-off-by: David Jander <david@protonic.nl>
---
 arch/arm/cpu/armv7/mx5/lowlevel_init.S |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/arch/arm/cpu/armv7/mx5/lowlevel_init.S b/arch/arm/cpu/armv7/mx5/lowlevel_init.S
index ee4150d..f17d200 100644
--- a/arch/arm/cpu/armv7/mx5/lowlevel_init.S
+++ b/arch/arm/cpu/armv7/mx5/lowlevel_init.S
@@ -39,11 +39,6 @@
 	orr r0, r0, #(1 << 23)		/* disable write allocate combine */
 	orr r0, r0, #(1 << 22)		/* disable write allocate */
 
-	cmp r3, #0x10    /* r3 contains the silicon rev */
-
-	/* disable write combine for TO 2 and lower revs */
-	orrls r0, r0, #(1 << 25)
-
 	mcr 15, 1, r0, c9, c0, 2
 .endm /* init_l2cc */
 
-- 
1.7.4.1

[U-Boot] [PATCH] ARM: MX5: Remove broken leftover TO-2 errata workaround

[Stefano Babic]

On 07/14/2011 09:13 AM, David Jander wrote:
> This check is broken. r3 does not contain the silicon revision.
> 
> Signed-off-by: David Jander <david@protonic.nl>
> ---

Hi David,

>  arch/arm/cpu/armv7/mx5/lowlevel_init.S |    5 -----
>  1 files changed, 0 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv7/mx5/lowlevel_init.S b/arch/arm/cpu/armv7/mx5/lowlevel_init.S
> index ee4150d..f17d200 100644
> --- a/arch/arm/cpu/armv7/mx5/lowlevel_init.S
> +++ b/arch/arm/cpu/armv7/mx5/lowlevel_init.S
> @@ -39,11 +39,6 @@
>  	orr r0, r0, #(1 << 23)		/* disable write allocate combine */
>  	orr r0, r0, #(1 << 22)		/* disable write allocate */
>  
> -	cmp r3, #0x10    /* r3 contains the silicon rev */

You are right. Nobody sets the r3 register, the test can be wrong.

> -
> -	/* disable write combine for TO 2 and lower revs */
> -	orrls r0, r0, #(1 << 25)

However, you also remove the setup for TO2. To fix the TO2 issue, we
should read correctly the revision number (from IIM or from a fixed
address, I do not remember now), and then apply the compare to the read
value.

Best regards,
Stefano Babic

-- 
=====================================================================
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@denx.de
=====================================================================

[U-Boot] [PATCH] ARM: MX5: Remove broken leftover TO-2 errata workaround

[David Jander]

On Thu, 14 Jul 2011 10:16:51 +0200
Stefano Babic <sbabic@denx.de> wrote:

> On 07/14/2011 09:13 AM, David Jander wrote:
> > This check is broken. r3 does not contain the silicon revision.
> > 
> > Signed-off-by: David Jander <david@protonic.nl>
> > ---
> 
> Hi David,
> 
> >  arch/arm/cpu/armv7/mx5/lowlevel_init.S |    5 -----
> >  1 files changed, 0 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/arm/cpu/armv7/mx5/lowlevel_init.S
> > b/arch/arm/cpu/armv7/mx5/lowlevel_init.S index ee4150d..f17d200 100644
> > --- a/arch/arm/cpu/armv7/mx5/lowlevel_init.S
> > +++ b/arch/arm/cpu/armv7/mx5/lowlevel_init.S
> > @@ -39,11 +39,6 @@
> >  	orr r0, r0, #(1 << 23)		/* disable write allocate
> > combine */ orr r0, r0, #(1 << 22)		/* disable write allocate
> > */ 
> > -	cmp r3, #0x10    /* r3 contains the silicon rev */
> 
> You are right. Nobody sets the r3 register, the test can be wrong.
> 
> > -
> > -	/* disable write combine for TO 2 and lower revs */
> > -	orrls r0, r0, #(1 << 25)
> 
> However, you also remove the setup for TO2. To fix the TO2 issue, we
> should read correctly the revision number (from IIM or from a fixed
> address, I do not remember now), and then apply the compare to the read
> value.

Yes, you are right.
But I don't know how to do it correctly.
OTOH, it is broken now for all platforms. My patch fixes it for TO3 and
newer. L2 write-combine has a significant performance impact, and I wonder how
many boards there are still that use such an old (prototype silicon) processor.
IMHO, the vast majority of MX51 users will benefit from this patch, and the
rest shouldn't have any more problems than they have already, so can we just
apply this, please?

Best regards,

-- 
David Jander
Protonic Holland.

[U-Boot] [PATCH] ARM: MX5: Remove broken leftover TO-2 errata workaround

[Stefano Babic]

On 07/14/2011 12:20 PM, David Jander wrote:

>> However, you also remove the setup for TO2. To fix the TO2 issue, we
>> should read correctly the revision number (from IIM or from a fixed
>> address, I do not remember now), and then apply the compare to the read
>> value.
> 
> Yes, you are right.
> But I don't know how to do it correctly.

There is a similar code always in lowlevel_init.S

189         ldr r1, =0x0
190         ldr r3, [r1, #ROM_SI_REV]
191         cmp r3, #0x10

As we can suppose this is correct, the same code can be used in the macro.

> OTOH, it is broken now for all platforms.

Agree we have to fix it. I only dislike to break some boards. As far as
I know, there is many mx51evk boards sold by Freescale with the TO2 chip.

> My patch fixes it for TO3 and
> newer. L2 write-combine has a significant performance impact, and I wonder how
> many boards there are still that use such an old (prototype silicon) processor.

I think only on the evaluation boards, but they were sold and delivered.

> IMHO, the vast majority of MX51 users will benefit from this patch, and the
> rest shouldn't have any more problems than they have already, so can we just
> apply this, please?

Not as it is  - I prefer we fix the test. Can you resubmit with the
proposed changes ?

Best regards,
Stefano Babic

-- 
=====================================================================
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] ARM: MX5: Remove broken leftover TO-2 errata workaround

[David Jander]

On Thu, 14 Jul 2011 12:49:15 +0200
Stefano Babic <sbabic@denx.de> wrote:

> On 07/14/2011 12:20 PM, David Jander wrote:
> 
> >> However, you also remove the setup for TO2. To fix the TO2 issue, we
> >> should read correctly the revision number (from IIM or from a fixed
> >> address, I do not remember now), and then apply the compare to the read
> >> value.
> > 
> > Yes, you are right.
> > But I don't know how to do it correctly.
> 
> There is a similar code always in lowlevel_init.S
> 
> 189         ldr r1, =0x0
> 190         ldr r3, [r1, #ROM_SI_REV]
> 191         cmp r3, #0x10
> 
> As we can suppose this is correct, the same code can be used in the macro.

Hmmm. Hadn't seen that part. Can we trust this?... because I have no means of
testing for the TO2 case.

> > OTOH, it is broken now for all platforms.
> 
> Agree we have to fix it. I only dislike to break some boards. As far as
> I know, there is many mx51evk boards sold by Freescale with the TO2 chip.

Ah, ok. AFAICR, our EVK has a TO3, but I agree there might be low quantities
of EVKs with TO2 still in use somewhere.

> > My patch fixes it for TO3 and
> > newer. L2 write-combine has a significant performance impact, and I wonder
> > how many boards there are still that use such an old (prototype silicon)
> > processor.
> 
> I think only on the evaluation boards, but they were sold and delivered.

Ok.

> > IMHO, the vast majority of MX51 users will benefit from this patch, and the
> > rest shouldn't have any more problems than they have already, so can we
> > just apply this, please?
> 
> Not as it is  - I prefer we fix the test. Can you resubmit with the
> proposed changes ?

Ok, thanks for pointing out the missing code. I will fix and re-submit.

Best regards,

-- 
David Jander
Protonic Holland.