[PATCH] ARM: imx6: fix v7_invalidate_l1 by adding I-Cache invalidation

[PATCH] ARM: imx6: fix v7_invalidate_l1 by adding I-Cache invalidation

[Shawn Guo]

The recent suspend/resume and reset testing on imx6q discovers that
not only D-Cache but also I-Cache has random data and validity when
the core comes out of a power recycle.

This patch adds I-Cache invalidation into v7_invalidate_l1 to make
sure both D-Cache and I-Cache invalidated on power-up.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 arch/arm/mach-imx/head-v7.S |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-imx/head-v7.S b/arch/arm/mach-imx/head-v7.S
index 6229efb..c844112 100644
--- a/arch/arm/mach-imx/head-v7.S
+++ b/arch/arm/mach-imx/head-v7.S
@@ -33,6 +33,7 @@
  */
 ENTRY(v7_invalidate_l1)
 	mov	r0, #0
+	mcr	p15, 0, r0, c7, c5, 0	@ invalidate I cache
 	mcr	p15, 2, r0, c0, c0, 0
 	mrc	p15, 1, r0, c0, c0, 0
 
-- 
1.7.4.1

[PATCH] ARM: imx6: fix v7_invalidate_l1 by adding I-Cache invalidation

[Jason Liu]

2011/12/30 Shawn Guo <shawn.guo@linaro.org>:
> The recent suspend/resume and reset testing on imx6q discovers that
> not only D-Cache but also I-Cache has random data and validity when
> the core comes out of a power recycle.
>
> This patch adds I-Cache invalidation into v7_invalidate_l1 to make
> sure both D-Cache and I-Cache invalidated on power-up.
>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
> ?arch/arm/mach-imx/head-v7.S | ? ?1 +
> ?1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-imx/head-v7.S b/arch/arm/mach-imx/head-v7.S
> index 6229efb..c844112 100644
> --- a/arch/arm/mach-imx/head-v7.S
> +++ b/arch/arm/mach-imx/head-v7.S
> @@ -33,6 +33,7 @@
> ?*/
> ?ENTRY(v7_invalidate_l1)
> ? ? ? ?mov ? ? r0, #0
> + ? ? ? mcr ? ? p15, 0, r0, c7, c5, 0 ? @ invalidate I cache
> ? ? ? ?mcr ? ? p15, 2, r0, c0, c0, 0
> ? ? ? ?mrc ? ? p15, 1, r0, c0, c0, 0

I'm wondering why arm linux init core code does not try to invalidate
i/d-cache before enable it?
As a formal procedure, we need invalidate i/d cache before actually
enable it. right?

I looked the code: arch/arm/mm/proc-v7.S:

#ifdef HARVARD_CACHE
        mcr     p15, 0, r10, c7, c5, 0          @ I+BTB cache invalidate
#endif
        dsb
#ifdef CONFIG_MMU
        mcr     p15, 0, r10, c8, c7, 0          @ invalidate I + D TLBs
        mcr     p15, 0, r10, c2, c0, 2          @ TTB control register
        ALT_SMP(orr     r4, r4, #TTB_FLAGS_SMP)
        ALT_UP(orr      r4, r4, #TTB_FLAGS_UP)
        mcr     p15, 0, r4, c2, c0, 1           @ load TTB1

It seems that it will try to invalidate when HARVARD_CACHE define. But
HARVARD_CACHE
only defined in v6, why?

Jason Liu
>
> --
> 1.7.4.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] ARM: imx6: fix v7_invalidate_l1 by adding I-Cache invalidation

[Shawn Guo]

On Fri, Dec 30, 2011 at 06:47:05PM +0800, Jason Liu wrote:
> 2011/12/30 Shawn Guo <shawn.guo@linaro.org>:
> > The recent suspend/resume and reset testing on imx6q discovers that
> > not only D-Cache but also I-Cache has random data and validity when
> > the core comes out of a power recycle.
> >
> > This patch adds I-Cache invalidation into v7_invalidate_l1 to make
> > sure both D-Cache and I-Cache invalidated on power-up.
> >
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > ---
> > ?arch/arm/mach-imx/head-v7.S | ? ?1 +
> > ?1 files changed, 1 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/arm/mach-imx/head-v7.S b/arch/arm/mach-imx/head-v7.S
> > index 6229efb..c844112 100644
> > --- a/arch/arm/mach-imx/head-v7.S
> > +++ b/arch/arm/mach-imx/head-v7.S
> > @@ -33,6 +33,7 @@
> > ?*/
> > ?ENTRY(v7_invalidate_l1)
> > ? ? ? ?mov ? ? r0, #0
> > + ? ? ? mcr ? ? p15, 0, r0, c7, c5, 0 ? @ invalidate I cache
> > ? ? ? ?mcr ? ? p15, 2, r0, c0, c0, 0
> > ? ? ? ?mrc ? ? p15, 1, r0, c0, c0, 0
> 
> I'm wondering why arm linux init core code does not try to invalidate
> i/d-cache before enable it?
> As a formal procedure, we need invalidate i/d cache before actually
> enable it. right?

I was ever told by Russell that ARM_ARM permits that cache holds random
data out of a power-up.  But I'm not sure if the validity mark can also
be randomized.  If it can, this issue may need to be addressed in common
place, otherwise it's really just a imx6q specific problem.

Regards,
Shawn

> 
> I looked the code: arch/arm/mm/proc-v7.S:
> 
> #ifdef HARVARD_CACHE
>         mcr     p15, 0, r10, c7, c5, 0          @ I+BTB cache invalidate
> #endif
>         dsb
> #ifdef CONFIG_MMU
>         mcr     p15, 0, r10, c8, c7, 0          @ invalidate I + D TLBs
>         mcr     p15, 0, r10, c2, c0, 2          @ TTB control register
>         ALT_SMP(orr     r4, r4, #TTB_FLAGS_SMP)
>         ALT_UP(orr      r4, r4, #TTB_FLAGS_UP)
>         mcr     p15, 0, r4, c2, c0, 1           @ load TTB1
> 
> It seems that it will try to invalidate when HARVARD_CACHE define. But
> HARVARD_CACHE
> only defined in v6, why?
> 

[PATCH] ARM: imx6: fix v7_invalidate_l1 by adding I-Cache invalidation

[Russell King - ARM Linux]

On Sat, Dec 31, 2011 at 05:21:49PM +0800, Shawn Guo wrote:
> I was ever told by Russell that ARM_ARM permits that cache holds random
> data out of a power-up.  But I'm not sure if the validity mark can also
> be randomized.

It's worse than that: the ARM ARM says that some caches may require a
CPU specific sequence to initialize them after power-up.  It's also
implemenation defined whether entries in the cache can be hit while
the caches are disabled.  I believe that there's nothing mentioned
about the state of the validity bits either (which, from my reading,
could be in a random state.)

As far as the architecture goes, it doesn't matter provided there is
an implementation defined sequence which results in the caches being
properly initialized.

I suspect that an implementation which choses to leave the validity bits
in a random state _and_ which searches the instruction cache with I=0 in
the control register would not be reasonable as each time you power up,
you're playing russian roulette with the contents of the I cache
interfering with the very first instruction to be executed by the CPU.

However, an implementation which guaranteed that there wouldn't be an
I-cache entry at the reset vector, and specified a sequence which would
fit inside one cache line, and which had the properties above _would_
conform.

So, this makes for a really interesting situation: by the time we get
anywhere near the kernel, it may already be too late to invalidate the
caches if it hasn't already been done.

We really need boot loaders to have already initialized the caches.
In the case of secondary CPU bringup, it's possible that the secondary
CPU may start executing from our trampoline code, and this is why
platforms are able to specify their own initial code for this.

[PATCH] ARM: imx6: fix v7_invalidate_l1 by adding I-Cache invalidation

[Russell King - ARM Linux]

On Fri, Dec 30, 2011 at 06:47:05PM +0800, Jason Liu wrote:
> I looked the code: arch/arm/mm/proc-v7.S:
> 
> #ifdef HARVARD_CACHE
>         mcr     p15, 0, r10, c7, c5, 0          @ I+BTB cache invalidate
> #endif
>         dsb
> #ifdef CONFIG_MMU
>         mcr     p15, 0, r10, c8, c7, 0          @ invalidate I + D TLBs
>         mcr     p15, 0, r10, c2, c0, 2          @ TTB control register
>         ALT_SMP(orr     r4, r4, #TTB_FLAGS_SMP)
>         ALT_UP(orr      r4, r4, #TTB_FLAGS_UP)
>         mcr     p15, 0, r4, c2, c0, 1           @ load TTB1
> 
> It seems that it will try to invalidate when HARVARD_CACHE define. But
> HARVARD_CACHE only defined in v6, why?

It's probably a mistake caused when copying proc-v6.S to proc-v7.S and
editing it - which I believe is how proc-v7.S was created.  Suggest you
ask Catalin to find out more details.

[PATCH] ARM: imx6: fix v7_invalidate_l1 by adding I-Cache invalidation

[Catalin Marinas]

On Tue, Jan 03, 2012 at 05:28:45PM +0000, Russell King - ARM Linux wrote:
> On Fri, Dec 30, 2011 at 06:47:05PM +0800, Jason Liu wrote:
> > I looked the code: arch/arm/mm/proc-v7.S:
> > 
> > #ifdef HARVARD_CACHE
> >         mcr     p15, 0, r10, c7, c5, 0          @ I+BTB cache invalidate
> > #endif
> >         dsb
> > #ifdef CONFIG_MMU
> >         mcr     p15, 0, r10, c8, c7, 0          @ invalidate I + D TLBs
> >         mcr     p15, 0, r10, c2, c0, 2          @ TTB control register
> >         ALT_SMP(orr     r4, r4, #TTB_FLAGS_SMP)
> >         ALT_UP(orr      r4, r4, #TTB_FLAGS_UP)
> >         mcr     p15, 0, r4, c2, c0, 1           @ load TTB1
> > 
> > It seems that it will try to invalidate when HARVARD_CACHE define. But
> > HARVARD_CACHE only defined in v6, why?
> 
> It's probably a mistake caused when copying proc-v6.S to proc-v7.S and
> editing it - which I believe is how proc-v7.S was created.  Suggest you
> ask Catalin to find out more details.

It looks like a bug that has been around for nearly 5 years.

-- 
Catalin

[PATCH] ARM: proc-v7: remove harvard cache stuff

[Shawn Guo]

The harvard cache related comment and code in proc-v7.S were copied
from proc-v6.S by mistake, so let's remove them.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 arch/arm/mm/proc-v7.S |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S
index e70a737..59a4077 100644
--- a/arch/arm/mm/proc-v7.S
+++ b/arch/arm/mm/proc-v7.S
@@ -271,10 +271,6 @@ ENDPROC(cpu_v7_do_resume)
  *	Initialise TLB, Caches, and MMU state ready to switch the MMU
  *	on.  Return in r0 the new CP15 C1 control register setting.
  *
- *	We automatically detect if we have a Harvard cache, and use the
- *	Harvard cache control instructions insead of the unified cache
- *	control instructions.
- *
  *	This should be able to cover all ARMv7 cores.
  *
  *	It is assumed that:
@@ -373,9 +369,6 @@ __v7_setup:
 #endif
 
 3:	mov	r10, #0
-#ifdef HARVARD_CACHE
-	mcr	p15, 0, r10, c7, c5, 0		@ I+BTB cache invalidate
-#endif
 	dsb
 #ifdef CONFIG_MMU
 	mcr	p15, 0, r10, c8, c7, 0		@ invalidate I + D TLBs
-- 
1.7.4.1

[PATCH] ARM: proc-v7: remove harvard cache stuff

[Kyungmin Park]

Reviewed-by: Kyungmin Park <kyungmin.park@samsung.com>

On 1/5/12, Shawn Guo <shawn.guo@linaro.org> wrote:
> The harvard cache related comment and code in proc-v7.S were copied
> from proc-v6.S by mistake, so let's remove them.
>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  arch/arm/mm/proc-v7.S |    7 -------
>  1 files changed, 0 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S
> index e70a737..59a4077 100644
> --- a/arch/arm/mm/proc-v7.S
> +++ b/arch/arm/mm/proc-v7.S
> @@ -271,10 +271,6 @@ ENDPROC(cpu_v7_do_resume)
>   *	Initialise TLB, Caches, and MMU state ready to switch the MMU
>   *	on.  Return in r0 the new CP15 C1 control register setting.
>   *
> - *	We automatically detect if we have a Harvard cache, and use the
> - *	Harvard cache control instructions insead of the unified cache
> - *	control instructions.
> - *
>   *	This should be able to cover all ARMv7 cores.
>   *
>   *	It is assumed that:
> @@ -373,9 +369,6 @@ __v7_setup:
>  #endif
>
>  3:	mov	r10, #0
> -#ifdef HARVARD_CACHE
> -	mcr	p15, 0, r10, c7, c5, 0		@ I+BTB cache invalidate
> -#endif
>  	dsb
>  #ifdef CONFIG_MMU
>  	mcr	p15, 0, r10, c8, c7, 0		@ invalidate I + D TLBs
> --
> 1.7.4.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

[PATCH] ARM: proc-v7: remove harvard cache stuff

[Catalin Marinas]

On Thu, Jan 05, 2012 at 06:37:42AM +0000, Shawn Guo wrote:
> The harvard cache related comment and code in proc-v7.S were copied
> from proc-v6.S by mistake, so let's remove them.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  arch/arm/mm/proc-v7.S |    7 -------
>  1 files changed, 0 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S
> index e70a737..59a4077 100644
> --- a/arch/arm/mm/proc-v7.S
> +++ b/arch/arm/mm/proc-v7.S
> @@ -271,10 +271,6 @@ ENDPROC(cpu_v7_do_resume)
>   *	Initialise TLB, Caches, and MMU state ready to switch the MMU
>   *	on.  Return in r0 the new CP15 C1 control register setting.
>   *
> - *	We automatically detect if we have a Harvard cache, and use the
> - *	Harvard cache control instructions insead of the unified cache
> - *	control instructions.
> - *
>   *	This should be able to cover all ARMv7 cores.
>   *
>   *	It is assumed that:
> @@ -373,9 +369,6 @@ __v7_setup:
>  #endif
>  
>  3:	mov	r10, #0
> -#ifdef HARVARD_CACHE
> -	mcr	p15, 0, r10, c7, c5, 0		@ I+BTB cache invalidate
> -#endif

I thought we still need to invalidate the I-cache, maybe moving it
higher up after the v7_flush_dcache_all call.

-- 
Catalin

[PATCH] ARM: proc-v7: remove harvard cache stuff

[Will Deacon]

On Thu, Jan 05, 2012 at 09:48:58AM +0000, Catalin Marinas wrote:
> On Thu, Jan 05, 2012 at 06:37:42AM +0000, Shawn Guo wrote:
> > The harvard cache related comment and code in proc-v7.S were copied
> > from proc-v6.S by mistake, so let's remove them.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > ---
> >  arch/arm/mm/proc-v7.S |    7 -------
> >  1 files changed, 0 insertions(+), 7 deletions(-)
> > 
> > diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S
> > index e70a737..59a4077 100644
> > --- a/arch/arm/mm/proc-v7.S
> > +++ b/arch/arm/mm/proc-v7.S
> > @@ -271,10 +271,6 @@ ENDPROC(cpu_v7_do_resume)
> >   *	Initialise TLB, Caches, and MMU state ready to switch the MMU
> >   *	on.  Return in r0 the new CP15 C1 control register setting.
> >   *
> > - *	We automatically detect if we have a Harvard cache, and use the
> > - *	Harvard cache control instructions insead of the unified cache
> > - *	control instructions.
> > - *
> >   *	This should be able to cover all ARMv7 cores.
> >   *
> >   *	It is assumed that:
> > @@ -373,9 +369,6 @@ __v7_setup:
> >  #endif
> >  
> >  3:	mov	r10, #0
> > -#ifdef HARVARD_CACHE
> > -	mcr	p15, 0, r10, c7, c5, 0		@ I+BTB cache invalidate
> > -#endif
> 
> I thought we still need to invalidate the I-cache, maybe moving it
> higher up after the v7_flush_dcache_all call.

I agree. Although this does seem to be working without the invalidation,
this might just be because everything tends to be shiny when we boot up.

Is anybody planning to follow up on this patch? I can't see it in any of the
trees that I'm tracking.

Thanks,

Will