diff mbox

ARM: imx6: fix v7_invalidate_l1 by adding I-Cache invalidation

Message ID 1325237200-10610-1-git-send-email-shawn.guo@linaro.org
State Accepted
Commit 5b2acf384c8a8707d32a98106192ee7187e4446d
Headers show

Commit Message

Shawn Guo Dec. 30, 2011, 9:26 a.m. UTC
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(-)

Comments

Jason Liu Dec. 30, 2011, 10:47 a.m. UTC | #1
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@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Shawn Guo Dec. 31, 2011, 9:21 a.m. UTC | #2
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?
>
Russell King - ARM Linux Jan. 3, 2012, 5:28 p.m. UTC | #3
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.
Russell King - ARM Linux Jan. 3, 2012, 5:41 p.m. UTC | #4
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.
Catalin Marinas Jan. 3, 2012, 5:58 p.m. UTC | #5
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.
diff mbox

Patch

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