diff mbox

[RFC,v2,3/5] ARM: kernel: update cpu_suspend code to use cache LoUIS operations

Message ID 20121212103338.GB23022@e102568-lin.cambridge.arm.com (mailing list archive)
State Awaiting Upstream
Headers show

Commit Message

Lorenzo Pieralisi Dec. 12, 2012, 10:33 a.m. UTC 
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) ?

Lorenzo

--->8


--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Will Deacon Dec. 12, 2012, 1:36 p.m. UTC | #1 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guennadi Liakhovetski Dec. 12, 2012, 4:43 p.m. UTC | #2 
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/
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guennadi Liakhovetski Dec. 13, 2012, 8:09 a.m. UTC | #3 
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/
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Will Deacon Dec. 13, 2012, 10:51 a.m. UTC | #4 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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