| Message ID | 20121212103338.GB23022@e102568-lin.cambridge.arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Dec 12, 2012 at 10:33:38AM +0000, Lorenzo Pieralisi wrote: > On Tue, Dec 11, 2012 at 11:27:39PM +0000, Stephen Boyd wrote: > > On 12/11/12 08:38, Will Deacon wrote: > > > diff --git a/arch/arm/mm/cache-v7.S b/arch/arm/mm/cache-v7.S > > > index cd95664..f58248f 100644 > > > --- a/arch/arm/mm/cache-v7.S > > > +++ b/arch/arm/mm/cache-v7.S > > > @@ -44,7 +44,8 @@ ENDPROC(v7_flush_icache_all) > > > ENTRY(v7_flush_dcache_louis) > > > dmb @ ensure ordering with previous memory accesses > > > mrc p15, 1, r0, c0, c0, 1 @ read clidr, r0 = clidr > > > - ands r3, r0, #0xe00000 @ extract LoUIS from clidr > > > + ALT_SMP(ands r3, r0, #(7 << 21)) @ extract LoUIS from clidr > > > + ALT_UP(ands r3, r0, #(7 << 27)) @ extract LoUU from clidr > > > mov r3, r3, lsr #20 @ r3 = LoUIS * 2 > > > > You need to fix this mov as well, right? > > And after doing that I think the suspend finisher will still have > to call flush_cache_all() since LoUU == 1 on A8, L2 is not cleaned > and that's probably what we want if it can be retained. At some point we probably want to describe the level of flushing required in the device tree as a property of the CPU node (or something similar). That would allow us to have *one* function for flushing, e.g. cpu_suspend_flush_cache which flushes to the appropriate level. Then we could remove the louis flush from the CPU suspend code and instead make it the finisher's responsibility to call our flushing function when it's done, which helps to avoid over/under-flushing the cache. In the meantime, fixing louis as we've suggested should work. Back to the case in hand.... Lorenzo just pointed out to me that the finished in question (sh7372_do_idle_sysc) calls v7_flush_dcache_all, so the louis stuff should be irrelevant. The problem may actually be that the finisher disables the L2 cache prior to cleaning/invalidating it, which is the opposite order to that described by the A8 TRM. Guennadi -- can you try moving the kernel_flush call before the L2 disable in sh7372_do_idle_sysc please? Will
Hi Lorenzo On Wed, 12 Dec 2012, Lorenzo Pieralisi wrote: > On Tue, Dec 11, 2012 at 11:27:39PM +0000, Stephen Boyd wrote: > > On 12/11/12 08:38, Will Deacon wrote: > > > diff --git a/arch/arm/mm/cache-v7.S b/arch/arm/mm/cache-v7.S > > > index cd95664..f58248f 100644 > > > --- a/arch/arm/mm/cache-v7.S > > > +++ b/arch/arm/mm/cache-v7.S > > > @@ -44,7 +44,8 @@ ENDPROC(v7_flush_icache_all) > > > ENTRY(v7_flush_dcache_louis) > > > dmb @ ensure ordering with previous memory accesses > > > mrc p15, 1, r0, c0, c0, 1 @ read clidr, r0 = clidr > > > - ands r3, r0, #0xe00000 @ extract LoUIS from clidr > > > + ALT_SMP(ands r3, r0, #(7 << 21)) @ extract LoUIS from clidr > > > + ALT_UP(ands r3, r0, #(7 << 27)) @ extract LoUU from clidr > > > mov r3, r3, lsr #20 @ r3 = LoUIS * 2 > > > > You need to fix this mov as well, right? > > And after doing that I think the suspend finisher will still have > to call flush_cache_all() since LoUU == 1 on A8, L2 is not cleaned > and that's probably what we want if it can be retained. > > What about this (compile tested) ? Works too. Thanks Guennadi > > Lorenzo > > --->8 > > diff --git a/arch/arm/mm/cache-v7.S b/arch/arm/mm/cache-v7.S > index cd95664..036f80f 100644 > --- a/arch/arm/mm/cache-v7.S > +++ b/arch/arm/mm/cache-v7.S > @@ -44,8 +44,9 @@ ENDPROC(v7_flush_icache_all) > ENTRY(v7_flush_dcache_louis) > dmb @ ensure ordering with previous memory accesses > mrc p15, 1, r0, c0, c0, 1 @ read clidr, r0 = clidr > - ands r3, r0, #0xe00000 @ extract LoUIS from clidr > - mov r3, r3, lsr #20 @ r3 = LoUIS * 2 > + ALT_SMP(lsr r3, r0, #20) @ r3 = clidr[31:20] > + ALT_UP(lsr r3, r0, #26) @ r3 = clidr[31:26] > + ands r3, r3, #0xe @ r3 = LoUIS/LoUU * 2 > moveq pc, lr @ return if level == 0 > mov r10, #0 @ r10 (starting level) = 0 > b flush_levels @ start flushing cache levels > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/
On Wed, 12 Dec 2012, Will Deacon wrote: > On Wed, Dec 12, 2012 at 10:33:38AM +0000, Lorenzo Pieralisi wrote: > > On Tue, Dec 11, 2012 at 11:27:39PM +0000, Stephen Boyd wrote: > > > On 12/11/12 08:38, Will Deacon wrote: > > > > diff --git a/arch/arm/mm/cache-v7.S b/arch/arm/mm/cache-v7.S > > > > index cd95664..f58248f 100644 > > > > --- a/arch/arm/mm/cache-v7.S > > > > +++ b/arch/arm/mm/cache-v7.S > > > > @@ -44,7 +44,8 @@ ENDPROC(v7_flush_icache_all) > > > > ENTRY(v7_flush_dcache_louis) > > > > dmb @ ensure ordering with previous memory accesses > > > > mrc p15, 1, r0, c0, c0, 1 @ read clidr, r0 = clidr > > > > - ands r3, r0, #0xe00000 @ extract LoUIS from clidr > > > > + ALT_SMP(ands r3, r0, #(7 << 21)) @ extract LoUIS from clidr > > > > + ALT_UP(ands r3, r0, #(7 << 27)) @ extract LoUU from clidr > > > > mov r3, r3, lsr #20 @ r3 = LoUIS * 2 > > > > > > You need to fix this mov as well, right? > > > > And after doing that I think the suspend finisher will still have > > to call flush_cache_all() since LoUU == 1 on A8, L2 is not cleaned > > and that's probably what we want if it can be retained. > > At some point we probably want to describe the level of flushing required in > the device tree as a property of the CPU node (or something similar). That > would allow us to have *one* function for flushing, > e.g. cpu_suspend_flush_cache which flushes to the appropriate level. Then > we could remove the louis flush from the CPU suspend code and instead make > it the finisher's responsibility to call our flushing function when it's > done, which helps to avoid over/under-flushing the cache. > > In the meantime, fixing louis as we've suggested should work. > > Back to the case in hand.... Lorenzo just pointed out to me that the > finished in question (sh7372_do_idle_sysc) calls v7_flush_dcache_all, so > the louis stuff should be irrelevant. The problem may actually be that the > finisher disables the L2 cache prior to cleaning/invalidating it, which is > the opposite order to that described by the A8 TRM. > > Guennadi -- can you try moving the kernel_flush call before the L2 disable > in sh7372_do_idle_sysc please? Yes, this works too. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/
On Thu, Dec 13, 2012 at 08:09:33AM +0000, Guennadi Liakhovetski wrote: > On Wed, 12 Dec 2012, Will Deacon wrote: > > Back to the case in hand.... Lorenzo just pointed out to me that the > > finished in question (sh7372_do_idle_sysc) calls v7_flush_dcache_all, so > > the louis stuff should be irrelevant. The problem may actually be that the > > finisher disables the L2 cache prior to cleaning/invalidating it, which is > > the opposite order to that described by the A8 TRM. > > > > Guennadi -- can you try moving the kernel_flush call before the L2 disable > > in sh7372_do_idle_sysc please? > > Yes, this works too. That's good to know. Please can you send a patch for that? The sequence currently being used by the finisher *is* buggy, and should be fixed independently of the louis stuff. Cheers, Will
diff --git a/arch/arm/mm/cache-v7.S b/arch/arm/mm/cache-v7.S index cd95664..036f80f 100644 --- a/arch/arm/mm/cache-v7.S +++ b/arch/arm/mm/cache-v7.S @@ -44,8 +44,9 @@ ENDPROC(v7_flush_icache_all) ENTRY(v7_flush_dcache_louis) dmb @ ensure ordering with previous memory accesses mrc p15, 1, r0, c0, c0, 1 @ read clidr, r0 = clidr - ands r3, r0, #0xe00000 @ extract LoUIS from clidr - mov r3, r3, lsr #20 @ r3 = LoUIS * 2 + ALT_SMP(lsr r3, r0, #20) @ r3 = clidr[31:20] + ALT_UP(lsr r3, r0, #26) @ r3 = clidr[31:26] + ands r3, r3, #0xe @ r3 = LoUIS/LoUU * 2 moveq pc, lr @ return if level == 0 mov r10, #0 @ r10 (starting level) = 0 b flush_levels @ start flushing cache levels