Re: [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@xxxxxxxxxxx> wrote:
> > From: Wenzeng Chen <wzch@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxx>
> > ---
> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/