[PATCH] ARM: suspend: use flush range instead of flush all

[PATCH] ARM: suspend: use flush range instead of flush all

[Shilimkar, Santosh]

+ Lorenzo,

On Wed, Sep 12, 2012 at 12:48 PM, wzch <wzch@marvell.com> wrote:
> From: Wenzeng Chen <wzch@marvell.com>
>
> In cpu suspend function __cpu_suspend_save(), we save cp15 registers,
> resume function, sp and suspend_pgd, then flush the data to DDR, but
> no need to flush all D cache, only need to flush range.
>
> Change-Id: I591a1fde929f3f987c69306b601843ed975d3e41
You should drop above.

> Signed-off-by: Wenzeng Chen <wzch@marvell.com>
> ---
Lorenzo and myself discussed about the above expensive flush and he
is planning to post a similar patch but with small difference.

>  arch/arm/kernel/suspend.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/kernel/suspend.c b/arch/arm/kernel/suspend.c
> index 1794cc3..bb582d8 100644
> --- a/arch/arm/kernel/suspend.c
> +++ b/arch/arm/kernel/suspend.c
> @@ -17,6 +17,7 @@ extern void cpu_resume_mmu(void);
>   */
>  void __cpu_suspend_save(u32 *ptr, u32 ptrsz, u32 sp, u32 *save_ptr)
>  {
> +       u32 *ptr_orig = ptr;
>         *save_ptr = virt_to_phys(ptr);
>
>         /* This must correspond to the LDM in cpu_resume() assembly */
> @@ -26,7 +27,8 @@ void __cpu_suspend_save(u32 *ptr, u32 ptrsz, u32 sp, u32 *save_ptr)
>
>         cpu_do_suspend(ptr);
>
> -       flush_cache_all();
Lorenzo's patch was limiting above flush to local cache (LOUs) instead
of dropping
it completely.

> +       __cpuc_flush_dcache_area((void *)ptr_orig, ptrsz);
> +       __cpuc_flush_dcache_area((void *)save_ptr, sizeof(*save_ptr));
>         outer_clean_range(*save_ptr, *save_ptr + ptrsz);
>         outer_clean_range(virt_to_phys(save_ptr),
>                           virt_to_phys(save_ptr) + sizeof(*save_ptr));

Just thinking bit more, even in case we drop the flush_cache_all()
completely, it should be safe since the suspend_finisher()  takes
care of the cache maintenance already.

Regards
Santosh

[PATCH] ARM: suspend: use flush range instead of flush all

[Russell King - ARM Linux]

On Wed, Sep 12, 2012 at 01:13:33PM +0530, Shilimkar, Santosh wrote:
> On Wed, Sep 12, 2012 at 12:48 PM, wzch <wzch@marvell.com> wrote:
> >  void __cpu_suspend_save(u32 *ptr, u32 ptrsz, u32 sp, u32 *save_ptr)
> >  {
> > +       u32 *ptr_orig = ptr;
> >         *save_ptr = virt_to_phys(ptr);
> >
> >         /* This must correspond to the LDM in cpu_resume() assembly */
> > @@ -26,7 +27,8 @@ void __cpu_suspend_save(u32 *ptr, u32 ptrsz, u32 sp, u32 *save_ptr)
> >
> >         cpu_do_suspend(ptr);
> >
> > -       flush_cache_all();
> Lorenzo's patch was limiting above flush to local cache (LOUs) instead
> of dropping it completely.

Err, that is wrong.  Normally, when CPUs go into suspend, the L1 cache is
lost entirely.  This is the only flush which many CPUs see of the L1
cache.

So removing this flush _will_ break suspend to RAM on existing CPUs.

[PATCH] ARM: suspend: use flush range instead of flush all

[Shilimkar, Santosh]

On Wed, Sep 12, 2012 at 2:24 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Sep 12, 2012 at 01:13:33PM +0530, Shilimkar, Santosh wrote:
>> On Wed, Sep 12, 2012 at 12:48 PM, wzch <wzch@marvell.com> wrote:
>> >  void __cpu_suspend_save(u32 *ptr, u32 ptrsz, u32 sp, u32 *save_ptr)
>> >  {
>> > +       u32 *ptr_orig = ptr;
>> >         *save_ptr = virt_to_phys(ptr);
>> >
>> >         /* This must correspond to the LDM in cpu_resume() assembly */
>> > @@ -26,7 +27,8 @@ void __cpu_suspend_save(u32 *ptr, u32 ptrsz, u32 sp, u32 *save_ptr)
>> >
>> >         cpu_do_suspend(ptr);
>> >
>> > -       flush_cache_all();
>> Lorenzo's patch was limiting above flush to local cache (LOUs) instead
>> of dropping it completely.
>
> Err, that is wrong.  Normally, when CPUs go into suspend, the L1 cache is
> lost entirely.  This is the only flush which many CPUs see of the L1
> cache.
>
> So removing this flush _will_ break suspend to RAM on existing CPUs.

As mentioned, keeping that flush till inner shareability domain(L1) should be
enough. In fact if that part gets pushed down to the finisher() which any
way needs to take care of the cache maintenance, we can get rid of completely.

At least limiting the flush to local cache instead of all cache levels should
be fixed.

Regards
Santosh

[PATCH] ARM: suspend: use flush range instead of flush all

[Russell King - ARM Linux]

On Wed, Sep 12, 2012 at 02:40:45PM +0530, Shilimkar, Santosh wrote:
> On Wed, Sep 12, 2012 at 2:24 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Wed, Sep 12, 2012 at 01:13:33PM +0530, Shilimkar, Santosh wrote:
> >> On Wed, Sep 12, 2012 at 12:48 PM, wzch <wzch@marvell.com> wrote:
> >> >  void __cpu_suspend_save(u32 *ptr, u32 ptrsz, u32 sp, u32 *save_ptr)
> >> >  {
> >> > +       u32 *ptr_orig = ptr;
> >> >         *save_ptr = virt_to_phys(ptr);
> >> >
> >> >         /* This must correspond to the LDM in cpu_resume() assembly */
> >> > @@ -26,7 +27,8 @@ void __cpu_suspend_save(u32 *ptr, u32 ptrsz, u32 sp, u32 *save_ptr)
> >> >
> >> >         cpu_do_suspend(ptr);
> >> >
> >> > -       flush_cache_all();
> >> Lorenzo's patch was limiting above flush to local cache (LOUs) instead
> >> of dropping it completely.
> >
> > Err, that is wrong.  Normally, when CPUs go into suspend, the L1 cache is
> > lost entirely.  This is the only flush which many CPUs see of the L1
> > cache.
> >
> > So removing this flush _will_ break suspend to RAM on existing CPUs.
> 
> As mentioned, keeping that flush till inner shareability domain(L1) should be
> enough. In fact if that part gets pushed down to the finisher() which any
> way needs to take care of the cache maintenance, we can get rid of completely.

It is difficult to call the cache maintenance functions from assembly.
Why not have the generic code do the inner shareability flush, and then
leave the responsibility for any further cache maintenance caused by the
actions of the finisher to the finisher to deal with - as it is now.

That way we end up with more generic code, and don't go back to the
rediculous situation where we had everyone implementing this crap in
their own broken way time and time again in their platform code.

[PATCH] ARM: suspend: use flush range instead of flush all

[Shilimkar, Santosh]

On Wed, Sep 12, 2012 at 3:16 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Sep 12, 2012 at 02:40:45PM +0530, Shilimkar, Santosh wrote:
>> On Wed, Sep 12, 2012 at 2:24 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > On Wed, Sep 12, 2012 at 01:13:33PM +0530, Shilimkar, Santosh wrote:
>> >> On Wed, Sep 12, 2012 at 12:48 PM, wzch <wzch@marvell.com> wrote:
>> >> >  void __cpu_suspend_save(u32 *ptr, u32 ptrsz, u32 sp, u32 *save_ptr)
>> >> >  {
>> >> > +       u32 *ptr_orig = ptr;
>> >> >         *save_ptr = virt_to_phys(ptr);
>> >> >
>> >> >         /* This must correspond to the LDM in cpu_resume() assembly */
>> >> > @@ -26,7 +27,8 @@ void __cpu_suspend_save(u32 *ptr, u32 ptrsz, u32 sp, u32 *save_ptr)
>> >> >
>> >> >         cpu_do_suspend(ptr);
>> >> >
>> >> > -       flush_cache_all();
>> >> Lorenzo's patch was limiting above flush to local cache (LOUs) instead
>> >> of dropping it completely.
>> >
>> > Err, that is wrong.  Normally, when CPUs go into suspend, the L1 cache is
>> > lost entirely.  This is the only flush which many CPUs see of the L1
>> > cache.
>> >
>> > So removing this flush _will_ break suspend to RAM on existing CPUs.
>>
>> As mentioned, keeping that flush till inner shareability domain(L1) should be
>> enough. In fact if that part gets pushed down to the finisher() which any
>> way needs to take care of the cache maintenance, we can get rid of completely.
>
> It is difficult to call the cache maintenance functions from assembly.
> Why not have the generic code do the inner shareability flush, and then
> leave the responsibility for any further cache maintenance caused by the
> actions of the finisher to the finisher to deal with - as it is now.
>
> That way we end up with more generic code, and don't go back to the
> rediculous situation where we had everyone implementing this crap in
> their own broken way time and time again in their platform code.

Fully agree with you.
Leaving the local cache flush should be better choice and that
is the additional bit Lorenzo's patch(yet to be posted) is doing on top
of the $subject patch. If the platform has special need like secure
cache maintenance, they can take care of that additionally in the
finisher.

Regards
Santosh

[PATCH] ARM: suspend: use flush range instead of flush all

[Raul Xiong]

2012/9/12 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Wed, Sep 12, 2012 at 02:40:45PM +0530, Shilimkar, Santosh wrote:
>> On Wed, Sep 12, 2012 at 2:24 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > On Wed, Sep 12, 2012 at 01:13:33PM +0530, Shilimkar, Santosh wrote:
>> >> On Wed, Sep 12, 2012 at 12:48 PM, wzch <wzch@marvell.com> wrote:
>> >> >  void __cpu_suspend_save(u32 *ptr, u32 ptrsz, u32 sp, u32 *save_ptr)
>> >> >  {
>> >> > +       u32 *ptr_orig = ptr;
>> >> >         *save_ptr = virt_to_phys(ptr);
>> >> >
>> >> >         /* This must correspond to the LDM in cpu_resume() assembly */
>> >> > @@ -26,7 +27,8 @@ void __cpu_suspend_save(u32 *ptr, u32 ptrsz, u32 sp, u32 *save_ptr)
>> >> >
>> >> >         cpu_do_suspend(ptr);
>> >> >
>> >> > -       flush_cache_all();
>> >> Lorenzo's patch was limiting above flush to local cache (LOUs) instead
>> >> of dropping it completely.
>> >
>> > Err, that is wrong.  Normally, when CPUs go into suspend, the L1 cache is
>> > lost entirely.  This is the only flush which many CPUs see of the L1
>> > cache.
>> >
>> > So removing this flush _will_ break suspend to RAM on existing CPUs.
>>
>> As mentioned, keeping that flush till inner shareability domain(L1) should be
>> enough. In fact if that part gets pushed down to the finisher() which any
>> way needs to take care of the cache maintenance, we can get rid of completely.
>
> It is difficult to call the cache maintenance functions from assembly.
> Why not have the generic code do the inner shareability flush, and then
> leave the responsibility for any further cache maintenance caused by the
> actions of the finisher to the finisher to deal with - as it is now.
>
> That way we end up with more generic code, and don't go back to the
> rediculous situation where we had everyone implementing this crap in
> their own broken way time and time again in their platform code.
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Hi Russell/All,

I have several questions:
1. Even we call flush_calche_all in __cpu_suspend_save, since later we
will outer_clean_range which may cause cache operations so we still
need to flush again in finisher, right?
2. Not every platform the L1 will be lost, we have the option to keep
L1 retentive, right? Just consider single core CA9. So we may not need
to flush all every time.
3. Why it's difficult to call cache maintenance in assembly? Moreover,
not every finisher is assembly, right?

[PATCH] ARM: suspend: use flush range instead of flush all

[Russell King - ARM Linux]

On Thu, Sep 13, 2012 at 10:20:30AM +0800, Raul Xiong wrote:
> I have several questions:
> 1. Even we call flush_calche_all in __cpu_suspend_save, since later we
> will outer_clean_range which may cause cache operations so we still
> need to flush again in finisher, right?

Why would it?  Any data which would be pulled back into the L1 cache
from that point is either going to be clean (in other words, a copy of
it exists elsewhere in the system) or it is going to be dirty (in
which case, it's been explicitly written to - and that's the stuff the
finisher needs to take care of.

We do not care abou the dirty data created by calling subsequent
functions as this is not used for resuming.

> 2. Not every platform the L1 will be lost, we have the option to keep
> L1 retentive, right? Just consider single core CA9. So we may not need
> to flush all every time.

That's where you start paying the price for having such a complex
architecture, and the need to have generic code to keep things simple.

Rather than thinking "we need to eliminate that flush and move it into
the finisher" start thinking "we need to communicate what parts of the
system are lost over suspend and get the suspend code to use that to
determine what it needs to do".

> 3. Why it's difficult to call cache maintenance in assembly? Moreover,
> not every finisher is assembly, right?

Have you tried to see whether a function called 'flush_cache_all'
actually exists in your System.map file?  It doesn't.  What you have
is a bunch of cache specific functions, one of which either gets aliased
to that name at build time, or that name gets aliased to a function
pointer (in a structure) to point at the relevant cache specific
function.  And there are platforms where hard-coding just one is not
correct.

What we have today is a massive improvement over what we had earlier on
where every platform was implementing all the suspend/resume stuff
themselves.  I'm not going back to the situation where we have that
again.

[PATCH] ARM: suspend: use flush range instead of flush all

[Lorenzo Pieralisi]

On Wed, Sep 12, 2012 at 08:43:33AM +0100, Shilimkar, Santosh wrote:
> + Lorenzo,
> 
> On Wed, Sep 12, 2012 at 12:48 PM, wzch <wzch@marvell.com> wrote:
> > From: Wenzeng Chen <wzch@marvell.com>
> >
> > In cpu suspend function __cpu_suspend_save(), we save cp15 registers,
> > resume function, sp and suspend_pgd, then flush the data to DDR, but
> > no need to flush all D cache, only need to flush range.
> >
> > Change-Id: I591a1fde929f3f987c69306b601843ed975d3e41
> You should drop above.
> 
> > Signed-off-by: Wenzeng Chen <wzch@marvell.com>
> > ---
> Lorenzo and myself discussed about the above expensive flush and he
> is planning to post a similar patch but with small difference.
> 
> >  arch/arm/kernel/suspend.c |    4 +++-
> >  1 files changed, 3 insertions(+), 1 deletions(-)
> >
> > diff --git a/arch/arm/kernel/suspend.c b/arch/arm/kernel/suspend.c
> > index 1794cc3..bb582d8 100644
> > --- a/arch/arm/kernel/suspend.c
> > +++ b/arch/arm/kernel/suspend.c
> > @@ -17,6 +17,7 @@ extern void cpu_resume_mmu(void);
> >   */
> >  void __cpu_suspend_save(u32 *ptr, u32 ptrsz, u32 sp, u32 *save_ptr)
> >  {
> > +       u32 *ptr_orig = ptr;
> >         *save_ptr = virt_to_phys(ptr);
> >
> >         /* This must correspond to the LDM in cpu_resume() assembly */
> > @@ -26,7 +27,8 @@ void __cpu_suspend_save(u32 *ptr, u32 ptrsz, u32 sp, u32 *save_ptr)
> >
> >         cpu_do_suspend(ptr);
> >
> > -       flush_cache_all();
> Lorenzo's patch was limiting above flush to local cache (LOUs) instead
> of dropping
> it completely.

Because if we remove it completely we have to make sure that every given
suspend finisher calls flush_cache_all(), hence from my viewpoint this
patch is incomplete. Either we remove it, and add it to ALL suspend
finisher (or just make sure it is there) or we leave it but it should use
the new LoUIS API we are going to add.

> 
> > +       __cpuc_flush_dcache_area((void *)ptr_orig, ptrsz);
> > +       __cpuc_flush_dcache_area((void *)save_ptr, sizeof(*save_ptr));
> >         outer_clean_range(*save_ptr, *save_ptr + ptrsz);
> >         outer_clean_range(virt_to_phys(save_ptr),
> >                           virt_to_phys(save_ptr) + sizeof(*save_ptr));
> 
> Just thinking bit more, even in case we drop the flush_cache_all()
> completely, it should be safe since the suspend_finisher()  takes
> care of the cache maintenance already.

We already discussed this. Fine by me, but we have to make sure it is
called on all suspend finishers in the mainline.

Lorenzo

[PATCH] ARM: suspend: use flush range instead of flush all

[Shilimkar, Santosh]

On Wed, Sep 12, 2012 at 2:28 PM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Wed, Sep 12, 2012 at 08:43:33AM +0100, Shilimkar, Santosh wrote:
>> + Lorenzo,
>>
>> On Wed, Sep 12, 2012 at 12:48 PM, wzch <wzch@marvell.com> wrote:
>> > From: Wenzeng Chen <wzch@marvell.com>
>> >
>> > In cpu suspend function __cpu_suspend_save(), we save cp15 registers,
>> > resume function, sp and suspend_pgd, then flush the data to DDR, but
>> > no need to flush all D cache, only need to flush range.
>> >
>> > Change-Id: I591a1fde929f3f987c69306b601843ed975d3e41
>> You should drop above.
>>
>> > Signed-off-by: Wenzeng Chen <wzch@marvell.com>
>> > ---
>> Lorenzo and myself discussed about the above expensive flush and he
>> is planning to post a similar patch but with small difference.
>>
>> >  arch/arm/kernel/suspend.c |    4 +++-
>> >  1 files changed, 3 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/arch/arm/kernel/suspend.c b/arch/arm/kernel/suspend.c
>> > index 1794cc3..bb582d8 100644
>> > --- a/arch/arm/kernel/suspend.c
>> > +++ b/arch/arm/kernel/suspend.c
>> > @@ -17,6 +17,7 @@ extern void cpu_resume_mmu(void);
>> >   */
>> >  void __cpu_suspend_save(u32 *ptr, u32 ptrsz, u32 sp, u32 *save_ptr)
>> >  {
>> > +       u32 *ptr_orig = ptr;
>> >         *save_ptr = virt_to_phys(ptr);
>> >
>> >         /* This must correspond to the LDM in cpu_resume() assembly */
>> > @@ -26,7 +27,8 @@ void __cpu_suspend_save(u32 *ptr, u32 ptrsz, u32 sp, u32 *save_ptr)
>> >
>> >         cpu_do_suspend(ptr);
>> >
>> > -       flush_cache_all();
>> Lorenzo's patch was limiting above flush to local cache (LOUs) instead
>> of dropping
>> it completely.
>
> Because if we remove it completely we have to make sure that every given
> suspend finisher calls flush_cache_all(), hence from my viewpoint this
> patch is incomplete. Either we remove it, and add it to ALL suspend
> finisher (or just make sure it is there) or we leave it but it should use
> the new LoUIS API we are going to add.
>
Yep.

>>
>> > +       __cpuc_flush_dcache_area((void *)ptr_orig, ptrsz);
>> > +       __cpuc_flush_dcache_area((void *)save_ptr, sizeof(*save_ptr));
>> >         outer_clean_range(*save_ptr, *save_ptr + ptrsz);
>> >         outer_clean_range(virt_to_phys(save_ptr),
>> >                           virt_to_phys(save_ptr) + sizeof(*save_ptr));
>>
>> Just thinking bit more, even in case we drop the flush_cache_all()
>> completely, it should be safe since the suspend_finisher()  takes
>> care of the cache maintenance already.
>
> We already discussed this. Fine by me, but we have to make sure it is
> called on all suspend finishers in the mainline.
>
I agree. As mentioned in reply to Russell, am ok to limit this
flush to LoUIS to start with.

Regards
Santosh