| Message ID | 1325237200-10610-1-git-send-email-shawn.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
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
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? >
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.
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.
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.
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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
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.
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
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
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(-)