* [PATCH] ARM: suspend: use flush range instead of flush all @ 2012-09-12 7:18 wzch 2012-09-12 7:43 ` Shilimkar, Santosh 0 siblings, 1 reply; 10+ messages in thread From: wzch @ 2012-09-12 7:18 UTC (permalink / raw) To: Russell King, Dave Martin, Catalin Marinas, Will Deacon, linux-arm-kernel, linux-kernel Cc: Wenzeng Chen 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 Signed-off-by: Wenzeng Chen <wzch@marvell.com> --- 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(); + __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)); -- 1.7.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] ARM: suspend: use flush range instead of flush all 2012-09-12 7:18 [PATCH] ARM: suspend: use flush range instead of flush all wzch @ 2012-09-12 7:43 ` Shilimkar, Santosh 2012-09-12 8:54 ` Russell King - ARM Linux 2012-09-12 8:58 ` Lorenzo Pieralisi 0 siblings, 2 replies; 10+ messages in thread From: Shilimkar, Santosh @ 2012-09-12 7:43 UTC (permalink / raw) To: wzch Cc: Russell King, Dave Martin, Catalin Marinas, Will Deacon, linux-arm-kernel, linux-kernel, Lorenzo Pieralisi + 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ARM: suspend: use flush range instead of flush all 2012-09-12 7:43 ` Shilimkar, Santosh @ 2012-09-12 8:54 ` Russell King - ARM Linux 2012-09-12 9:10 ` Shilimkar, Santosh 2012-09-12 8:58 ` Lorenzo Pieralisi 1 sibling, 1 reply; 10+ messages in thread From: Russell King - ARM Linux @ 2012-09-12 8:54 UTC (permalink / raw) To: Shilimkar, Santosh Cc: wzch, Dave Martin, Catalin Marinas, Will Deacon, linux-arm-kernel, linux-kernel, Lorenzo Pieralisi 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ARM: suspend: use flush range instead of flush all 2012-09-12 8:54 ` Russell King - ARM Linux @ 2012-09-12 9:10 ` Shilimkar, Santosh 2012-09-12 9:46 ` Russell King - ARM Linux 0 siblings, 1 reply; 10+ messages in thread From: Shilimkar, Santosh @ 2012-09-12 9:10 UTC (permalink / raw) To: Russell King - ARM Linux Cc: wzch, Dave Martin, Catalin Marinas, Will Deacon, linux-arm-kernel, linux-kernel, Lorenzo Pieralisi 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ARM: suspend: use flush range instead of flush all 2012-09-12 9:10 ` Shilimkar, Santosh @ 2012-09-12 9:46 ` Russell King - ARM Linux 2012-09-12 9:57 ` Shilimkar, Santosh 2012-09-13 2:20 ` Raul Xiong 0 siblings, 2 replies; 10+ messages in thread From: Russell King - ARM Linux @ 2012-09-12 9:46 UTC (permalink / raw) To: Shilimkar, Santosh Cc: wzch, Dave Martin, Catalin Marinas, Will Deacon, linux-arm-kernel, linux-kernel, Lorenzo Pieralisi 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ARM: suspend: use flush range instead of flush all 2012-09-12 9:46 ` Russell King - ARM Linux @ 2012-09-12 9:57 ` Shilimkar, Santosh 2012-09-13 2:20 ` Raul Xiong 1 sibling, 0 replies; 10+ messages in thread From: Shilimkar, Santosh @ 2012-09-12 9:57 UTC (permalink / raw) To: Russell King - ARM Linux Cc: wzch, Dave Martin, Catalin Marinas, Will Deacon, linux-arm-kernel, linux-kernel, Lorenzo Pieralisi 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ARM: suspend: use flush range instead of flush all 2012-09-12 9:46 ` Russell King - ARM Linux 2012-09-12 9:57 ` Shilimkar, Santosh @ 2012-09-13 2:20 ` Raul Xiong 2012-09-13 8:52 ` Russell King - ARM Linux 1 sibling, 1 reply; 10+ messages in thread From: Raul Xiong @ 2012-09-13 2:20 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Shilimkar, Santosh, Dave Martin, Lorenzo Pieralisi, Catalin Marinas, Will Deacon, linux-kernel, wzch, linux-arm-kernel 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@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? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ARM: suspend: use flush range instead of flush all 2012-09-13 2:20 ` Raul Xiong @ 2012-09-13 8:52 ` Russell King - ARM Linux 0 siblings, 0 replies; 10+ messages in thread From: Russell King - ARM Linux @ 2012-09-13 8:52 UTC (permalink / raw) To: Raul Xiong Cc: Shilimkar, Santosh, Dave Martin, Lorenzo Pieralisi, Catalin Marinas, Will Deacon, linux-kernel, wzch, linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ARM: suspend: use flush range instead of flush all 2012-09-12 7:43 ` Shilimkar, Santosh 2012-09-12 8:54 ` Russell King - ARM Linux @ 2012-09-12 8:58 ` Lorenzo Pieralisi 2012-09-12 9:20 ` Shilimkar, Santosh 1 sibling, 1 reply; 10+ messages in thread From: Lorenzo Pieralisi @ 2012-09-12 8:58 UTC (permalink / raw) To: Shilimkar, Santosh Cc: wzch, Russell King, Dave Martin, Catalin Marinas, Will Deacon, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ARM: suspend: use flush range instead of flush all 2012-09-12 8:58 ` Lorenzo Pieralisi @ 2012-09-12 9:20 ` Shilimkar, Santosh 0 siblings, 0 replies; 10+ messages in thread From: Shilimkar, Santosh @ 2012-09-12 9:20 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: wzch, Russell King, Dave Martin, Catalin Marinas, Will Deacon, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2012-09-13 8:52 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-09-12 7:18 [PATCH] ARM: suspend: use flush range instead of flush all wzch 2012-09-12 7:43 ` Shilimkar, Santosh 2012-09-12 8:54 ` Russell King - ARM Linux 2012-09-12 9:10 ` Shilimkar, Santosh 2012-09-12 9:46 ` Russell King - ARM Linux 2012-09-12 9:57 ` Shilimkar, Santosh 2012-09-13 2:20 ` Raul Xiong 2012-09-13 8:52 ` Russell King - ARM Linux 2012-09-12 8:58 ` Lorenzo Pieralisi 2012-09-12 9:20 ` Shilimkar, Santosh
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).