* [PATCH 0/2] ARM: OMAP2+: PM: miscellaneous powerdomain-related improvements
@ 2012-01-30 9:43 Paul Walmsley
2012-01-30 9:43 ` [PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst() Paul Walmsley
` (2 more replies)
0 siblings, 3 replies; 28+ messages in thread
From: Paul Walmsley @ 2012-01-30 9:43 UTC (permalink / raw)
To: linux-arm-kernel
This series optimizes some of the powerdomain-related code in
arch/arm/mach-omap2/pm*, and fixes a bug or two. These were noticed while
working on the functional powerstate code.
Rajendra and Santosh, if you have a spare moment, could you please
peek at the OMAP4 LOWPOWERSTATECHANGE part of the second patch? It
makes sense to me in theory, but you both would probably know better
than I :-)
Kevin, want to take this series (assuming folks are happy with it) ?
Tested dynamic idle with and without CPUIdle, with and without
off-mode, on a Beagleboard 35xx.
This series is available via git at git://git.pwsan.com/linux-2.6 in
the branch "pm_cleanup_3.4".
- Paul
---
pm_cleanup_3.4
text data bss dec hex filename
6592771 678492 5590684 12861947 c441fb vmlinux.orig
6592771 678492 5590684 12861947 c441fb vmlinux.patched
Paul Walmsley (2):
ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst()
ARM: OMAP2+: PM: clean up omap_set_pwrdm_state()
arch/arm/mach-omap2/pm.c | 39 +++++++++++++++++----------------------
arch/arm/mach-omap2/pm34xx.c | 5 -----
2 files changed, 17 insertions(+), 27 deletions(-)
^ permalink raw reply [flat|nested] 28+ messages in thread* [PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst() 2012-01-30 9:43 [PATCH 0/2] ARM: OMAP2+: PM: miscellaneous powerdomain-related improvements Paul Walmsley @ 2012-01-30 9:43 ` Paul Walmsley 2012-01-30 10:54 ` Shilimkar, Santosh 2012-01-31 0:14 ` Kevin Hilman 2012-01-30 9:43 ` [PATCH 2/2] ARM: OMAP2+: PM: clean up omap_set_pwrdm_state() Paul Walmsley 2012-01-30 21:27 ` [PATCH 0/2] ARM: OMAP2+: PM: miscellaneous powerdomain-related improvements Kevin Hilman 2 siblings, 2 replies; 28+ messages in thread From: Paul Walmsley @ 2012-01-30 9:43 UTC (permalink / raw) To: linux-arm-kernel Remove some superfluous calls to pwrdm_clear_all_prev_pwrst(). pwrdm_pre_transition(), which appears a few lines after these calls, invokes pwrdm_clear_all_prev_pwrst() on each powerdomain -- there's no need to do it twice. N.B.: some of us have observed that accesses to the previous powerstate registers seem to be quite slow. Although the writes removed by this patch should be buffered by the write buffer, there is a read to a PRM register immediately afterwards. That will block the OMAP3 MPU until all of those writes complete. So this patch should result in a minor performance improvement during idle entry. Signed-off-by: Paul Walmsley <paul@pwsan.com> Cc: Kevin Hilman <khilman@ti.com> Cc: Rajendra Nayak <rnayak@ti.com> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com> Cc: Tero Kristo <t-kristo@ti.com> --- arch/arm/mach-omap2/pm34xx.c | 5 ----- 1 files changed, 0 insertions(+), 5 deletions(-) diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c index fc69875..463eb6c 100644 --- a/arch/arm/mach-omap2/pm34xx.c +++ b/arch/arm/mach-omap2/pm34xx.c @@ -290,11 +290,6 @@ void omap_sram_idle(void) int core_prev_state, per_prev_state; u32 sdrc_pwr = 0; - pwrdm_clear_all_prev_pwrst(mpu_pwrdm); - pwrdm_clear_all_prev_pwrst(neon_pwrdm); - pwrdm_clear_all_prev_pwrst(core_pwrdm); - pwrdm_clear_all_prev_pwrst(per_pwrdm); - mpu_next_state = pwrdm_read_next_pwrst(mpu_pwrdm); switch (mpu_next_state) { case PWRDM_POWER_ON: ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst() 2012-01-30 9:43 ` [PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst() Paul Walmsley @ 2012-01-30 10:54 ` Shilimkar, Santosh 2012-01-31 0:14 ` Kevin Hilman 1 sibling, 0 replies; 28+ messages in thread From: Shilimkar, Santosh @ 2012-01-30 10:54 UTC (permalink / raw) To: linux-arm-kernel On Mon, Jan 30, 2012 at 3:13 PM, Paul Walmsley <paul@pwsan.com> wrote: > Remove some superfluous calls to pwrdm_clear_all_prev_pwrst(). > pwrdm_pre_transition(), which appears a few lines after these calls, > invokes pwrdm_clear_all_prev_pwrst() on each powerdomain -- there's no > need to do it twice. > > N.B.: some of us have observed that accesses to the previous > powerstate registers seem to be quite slow. ?Although the writes > removed by this patch should be buffered by the write buffer, there is > a read to a PRM register immediately afterwards. ?That will block the > OMAP3 MPU until all of those writes complete. ?So this patch should > result in a minor performance improvement during idle entry. > > Signed-off-by: Paul Walmsley <paul@pwsan.com> > Cc: Kevin Hilman <khilman@ti.com> > Cc: Rajendra Nayak <rnayak@ti.com> > Cc: Santosh Shilimkar <santosh.shilimkar@ti.com> > Cc: Tero Kristo <t-kristo@ti.com> > --- Nice clean-up Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com> ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst() 2012-01-30 9:43 ` [PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst() Paul Walmsley 2012-01-30 10:54 ` Shilimkar, Santosh @ 2012-01-31 0:14 ` Kevin Hilman 2012-01-31 3:53 ` Rajendra Nayak ` (2 more replies) 1 sibling, 3 replies; 28+ messages in thread From: Kevin Hilman @ 2012-01-31 0:14 UTC (permalink / raw) To: linux-arm-kernel Paul Walmsley <paul@pwsan.com> writes: > Remove some superfluous calls to pwrdm_clear_all_prev_pwrst(). > pwrdm_pre_transition(), which appears a few lines after these calls, > invokes pwrdm_clear_all_prev_pwrst() on each powerdomain -- there's no > need to do it twice. It looks like these two for OMAP4 are surpurfluous since the immediately follow a call to pwrdm_pre_transition() as well. Santosh/Rajendra, please confirm/ack. Kevin diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c b/arch/arm/mach-omap2/omap-mpuss-lowpower.c index 1d5d010..bbabe1d 100644 --- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c +++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c @@ -263,12 +263,10 @@ int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state) * In MPUSS OSWR or device OFF, interrupt controller contest is lost. */ mpuss_clear_prev_logic_pwrst(); - pwrdm_clear_all_prev_pwrst(mpuss_pd); if ((pwrdm_read_next_pwrst(mpuss_pd) == PWRDM_POWER_RET) && (pwrdm_read_logic_retst(mpuss_pd) == PWRDM_POWER_OFF)) save_state = 2; - clear_cpu_prev_pwrst(cpu); cpu_clear_prev_logic_pwrst(cpu); set_cpu_next_pwrst(cpu, power_state); set_cpu_wakeup_addr(cpu, virt_to_phys(omap4_cpu_resume)); ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst() 2012-01-31 0:14 ` Kevin Hilman @ 2012-01-31 3:53 ` Rajendra Nayak 2012-01-31 6:57 ` Shilimkar, Santosh 2012-01-31 17:29 ` Kevin Hilman 2 siblings, 0 replies; 28+ messages in thread From: Rajendra Nayak @ 2012-01-31 3:53 UTC (permalink / raw) To: linux-arm-kernel On Tuesday 31 January 2012 05:44 AM, Kevin Hilman wrote: > Paul Walmsley<paul@pwsan.com> writes: > >> Remove some superfluous calls to pwrdm_clear_all_prev_pwrst(). >> pwrdm_pre_transition(), which appears a few lines after these calls, >> invokes pwrdm_clear_all_prev_pwrst() on each powerdomain -- there's no >> need to do it twice. > > It looks like these two for OMAP4 are surpurfluous since the immediately > follow a call to pwrdm_pre_transition() as well. > > Santosh/Rajendra, please confirm/ack. I agree, looks like they should be removed. Acked-by: Rajendra Nayak <rnayak@ti.com> > > Kevin > > > diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c b/arch/arm/mach-omap2/omap-mpuss-lowpower.c > index 1d5d010..bbabe1d 100644 > --- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c > +++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c > @@ -263,12 +263,10 @@ int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state) > * In MPUSS OSWR or device OFF, interrupt controller contest is lost. > */ > mpuss_clear_prev_logic_pwrst(); > - pwrdm_clear_all_prev_pwrst(mpuss_pd); > if ((pwrdm_read_next_pwrst(mpuss_pd) == PWRDM_POWER_RET)&& > (pwrdm_read_logic_retst(mpuss_pd) == PWRDM_POWER_OFF)) > save_state = 2; > > - clear_cpu_prev_pwrst(cpu); > cpu_clear_prev_logic_pwrst(cpu); > set_cpu_next_pwrst(cpu, power_state); > set_cpu_wakeup_addr(cpu, virt_to_phys(omap4_cpu_resume)); ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst() 2012-01-31 0:14 ` Kevin Hilman 2012-01-31 3:53 ` Rajendra Nayak @ 2012-01-31 6:57 ` Shilimkar, Santosh 2012-01-31 7:15 ` Paul Walmsley 2012-01-31 17:29 ` Kevin Hilman 2 siblings, 1 reply; 28+ messages in thread From: Shilimkar, Santosh @ 2012-01-31 6:57 UTC (permalink / raw) To: linux-arm-kernel On Tue, Jan 31, 2012 at 5:44 AM, Kevin Hilman <khilman@ti.com> wrote: > Paul Walmsley <paul@pwsan.com> writes: > >> Remove some superfluous calls to pwrdm_clear_all_prev_pwrst(). >> pwrdm_pre_transition(), which appears a few lines after these calls, >> invokes pwrdm_clear_all_prev_pwrst() on each powerdomain -- there's no >> need to do it twice. > > It looks like these two for OMAP4 are surpurfluous since the immediately > follow a call to pwrdm_pre_transition() as well. > > Santosh/Rajendra, please confirm/ack. > > Kevin > > > diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c b/arch/arm/mach-omap2/omap-mpuss-lowpower.c > index 1d5d010..bbabe1d 100644 > --- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c > +++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c > @@ -263,12 +263,10 @@ int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state) > ? ? ? ? * In MPUSS OSWR or device OFF, interrupt controller ?contest is lost. > ? ? ? ? */ > ? ? ? ?mpuss_clear_prev_logic_pwrst(); > - ? ? ? pwrdm_clear_all_prev_pwrst(mpuss_pd); > ? ? ? ?if ((pwrdm_read_next_pwrst(mpuss_pd) == PWRDM_POWER_RET) && > ? ? ? ? ? ? ? ?(pwrdm_read_logic_retst(mpuss_pd) == PWRDM_POWER_OFF)) > ? ? ? ? ? ? ? ?save_state = 2; > > - ? ? ? clear_cpu_prev_pwrst(cpu); > ? ? ? ?cpu_clear_prev_logic_pwrst(cpu); > ? ? ? ?set_cpu_next_pwrst(cpu, power_state); > ? ? ? ?set_cpu_wakeup_addr(cpu, virt_to_phys(omap4_cpu_resume)); The power domain pre-transition and post transition calls were kept under the CONFIG_PM_DEBUG in the first few versions of the patches. These pre and post transition use to take lot of time once in while when measurement were done and the only useful thing they were doing was the counter updates. Later I removed the PM_DEBUG as per your suggestion. As per Pauls comment they are optimised, so that should be good. In this code the need is to clear only CPU and MPUPD, and hence they are explicitly cleared since the pre/post transition calls can be moved to PM_DEBUG in production kernels. But as you stated in current mainline kernel they are superfluous since the pre/post calls are not under PM debug. So I am ok either way Regards Santosh ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst() 2012-01-31 6:57 ` Shilimkar, Santosh @ 2012-01-31 7:15 ` Paul Walmsley 2012-01-31 7:23 ` Shilimkar, Santosh 0 siblings, 1 reply; 28+ messages in thread From: Paul Walmsley @ 2012-01-31 7:15 UTC (permalink / raw) To: linux-arm-kernel Hi just a few thoughts. On Tue, 31 Jan 2012, Shilimkar, Santosh wrote: > In this code the need is to clear only CPU and MPUPD, and hence they are > explicitly cleared since the pre/post transition calls can be moved to PM_DEBUG > in production kernels. > > But as you stated in current mainline kernel they are superfluous since the > pre/post calls are not under PM debug. So I am ok either way We're using the PM counters for the context restore skip optimization now in pwrdm_get_context_loss_count(), so they've suddenly become needed even when PM_DEBUG=n. But those counters are only needed when there are devices in the CPU* powerdomains to track that can lose logic context. I don't think that's the case on OMAP4, but you would probably know better than I. Another aspect is that previous powerstate accesses for the CPU* powerdomains would theoretically go to the MPU local PRM rather than the system PRCM. So they may actually return quickly. Haven't tested this. The MPUSS powerdomain might be another case, though. There are a bunch of devices in there: the local timers, APB debug devices, etc. We probably have to worry about restoring context for some of those at some point. - Paul ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst() 2012-01-31 7:15 ` Paul Walmsley @ 2012-01-31 7:23 ` Shilimkar, Santosh 2012-01-31 7:27 ` Paul Walmsley 0 siblings, 1 reply; 28+ messages in thread From: Shilimkar, Santosh @ 2012-01-31 7:23 UTC (permalink / raw) To: linux-arm-kernel On Tue, Jan 31, 2012 at 12:45 PM, Paul Walmsley <paul@pwsan.com> wrote: > Hi > > just a few thoughts. > > On Tue, 31 Jan 2012, Shilimkar, Santosh wrote: > >> In this code the need is to clear only CPU and MPUPD, and hence they are >> explicitly cleared since the pre/post transition calls can be moved to PM_DEBUG >> in production kernels. >> >> But as you stated in current mainline kernel they are superfluous since the >> pre/post calls are not under PM debug. So I am ok either way > > We're using the PM counters for the context restore skip optimization now > in pwrdm_get_context_loss_count(), so they've suddenly become needed even > when PM_DEBUG=n. ?But those counters are only needed when there are > devices in the CPU* powerdomains to track that can lose logic context. ?I > don't think that's the case on OMAP4, but you would probably know better > than I. > Good point so the pre/post calls are must then. We should kill those extra calls form OMAP4 code then. > Another aspect is that previous powerstate accesses for the CPU* > powerdomains would theoretically go to the MPU local PRM rather than the > system PRCM. ?So they may actually return quickly. ?Haven't tested this. > You are correct. They do. > The MPUSS powerdomain might be another case, though. ?There are a bunch of > devices in there: the local timers, APB debug devices, etc. ?We probably > have to worry about restoring context for some of those at some point. > MPUSS PD is at the global PRCM level but that one doesn't take time either. You need few more cycles to reach there. IIRC, other power domains use to take time because of the wait_transition calls we had. Mostly it was because of the BUGs you fixed. When get some time I will profile these calls and see if we still have that issue. Regards Santosh ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst() 2012-01-31 7:23 ` Shilimkar, Santosh @ 2012-01-31 7:27 ` Paul Walmsley 2012-01-31 7:34 ` Shilimkar, Santosh 0 siblings, 1 reply; 28+ messages in thread From: Paul Walmsley @ 2012-01-31 7:27 UTC (permalink / raw) To: linux-arm-kernel On Tue, 31 Jan 2012, Shilimkar, Santosh wrote: > On Tue, Jan 31, 2012 at 12:45 PM, Paul Walmsley <paul@pwsan.com> wrote: > > On Tue, 31 Jan 2012, Shilimkar, Santosh wrote: > > > >> In this code the need is to clear only CPU and MPUPD, and hence they are > >> explicitly cleared since the pre/post transition calls can be moved to PM_DEBUG > >> in production kernels. > >> > >> But as you stated in current mainline kernel they are superfluous since the > >> pre/post calls are not under PM debug. So I am ok either way > > > > We're using the PM counters for the context restore skip optimization now > > in pwrdm_get_context_loss_count(), so they've suddenly become needed even > > when PM_DEBUG=n. ?But those counters are only needed when there are > > devices in the CPU* powerdomains to track that can lose logic context. ?I > > don't think that's the case on OMAP4, but you would probably know better > > than I. > > > Good point so the pre/post calls are must then. We should kill those extra calls > form OMAP4 code then. I wonder if we should track how many hwmods are present in a powerdomain? We could check that counter to determine if we could skip the access. > > Another aspect is that previous powerstate accesses for the CPU* > > powerdomains would theoretically go to the MPU local PRM rather than the > > system PRCM. ?So they may actually return quickly. ?Haven't tested this. > > > You are correct. They do. Cool. > > The MPUSS powerdomain might be another case, though. ?There are a bunch of > > devices in there: the local timers, APB debug devices, etc. ?We probably > > have to worry about restoring context for some of those at some point. > > > MPUSS PD is at the global PRCM level but that one doesn't take time either. > You need few more cycles to reach there. > IIRC, other power domains use to take time because of the wait_transition > calls we had. Mostly it was because of the BUGs you fixed. When get some > time I will profile these calls and see if we still have that issue. That would be interesting. I was guessing that the previous powerstate registers in the global PRCM might be located in some clockdomain that was clocked at sys_clk or the 32KiHZ clock. But if it's really just due to the wait_transition(), that might be something we can optimize... - Paul ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst() 2012-01-31 7:27 ` Paul Walmsley @ 2012-01-31 7:34 ` Shilimkar, Santosh 2012-01-31 7:49 ` Paul Walmsley 0 siblings, 1 reply; 28+ messages in thread From: Shilimkar, Santosh @ 2012-01-31 7:34 UTC (permalink / raw) To: linux-arm-kernel On Tue, Jan 31, 2012 at 12:57 PM, Paul Walmsley <paul@pwsan.com> wrote: > On Tue, 31 Jan 2012, Shilimkar, Santosh wrote: > >> On Tue, Jan 31, 2012 at 12:45 PM, Paul Walmsley <paul@pwsan.com> wrote: >> > On Tue, 31 Jan 2012, Shilimkar, Santosh wrote: >> > >> >> In this code the need is to clear only CPU and MPUPD, and hence they are >> >> explicitly cleared since the pre/post transition calls can be moved to PM_DEBUG >> >> in production kernels. >> >> >> >> But as you stated in current mainline kernel they are superfluous since the >> >> pre/post calls are not under PM debug. So I am ok either way >> > >> > We're using the PM counters for the context restore skip optimization now >> > in pwrdm_get_context_loss_count(), so they've suddenly become needed even >> > when PM_DEBUG=n. ?But those counters are only needed when there are >> > devices in the CPU* powerdomains to track that can lose logic context. ?I >> > don't think that's the case on OMAP4, but you would probably know better >> > than I. >> > >> Good point so the pre/post calls are must then. We should kill those extra calls >> form OMAP4 code then. > > I wonder if we should track how many hwmods are present in a powerdomain? > We could check that counter to determine if we could skip the access. > That's good idea since these calls are in critical patha nd faster we can do better it is.. A week back I was discussing with Benoit and Tony about having some infrastructure like "unused clocks" so that we can shutdown those modules, and if possible some power domains. That way they get removed at one single place and they the power domain calls need not iterate over the once which are no longer needed. Regards Santosh ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst() 2012-01-31 7:34 ` Shilimkar, Santosh @ 2012-01-31 7:49 ` Paul Walmsley 2012-01-31 8:37 ` Shilimkar, Santosh 0 siblings, 1 reply; 28+ messages in thread From: Paul Walmsley @ 2012-01-31 7:49 UTC (permalink / raw) To: linux-arm-kernel On Tue, 31 Jan 2012, Shilimkar, Santosh wrote: > A week back I was discussing with Benoit and Tony about having some > infrastructure like "unused clocks" so that we can shutdown those > modules, and if possible some power domains. That way they get > removed at one single place and they the power domain calls > need not iterate over the once which are no longer needed. Sure, if you have some ideas, that sounds promising. I'd assume the main performance issues would be related to global PRCM register reads, rather than raw CPU cycles. So that's probably where it would make sense to focus the effort. We should also be able to optimize out a fair number of the powerdomain register reads if we cache them in the struct powerdomain. The current powerstate and next powerstate seem to be hit a lot, and those should both be cacheable. Will probably do some of this as part of the functional powerstate code. - Paul ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst() 2012-01-31 7:49 ` Paul Walmsley @ 2012-01-31 8:37 ` Shilimkar, Santosh 0 siblings, 0 replies; 28+ messages in thread From: Shilimkar, Santosh @ 2012-01-31 8:37 UTC (permalink / raw) To: linux-arm-kernel On Tue, Jan 31, 2012 at 1:19 PM, Paul Walmsley <paul@pwsan.com> wrote: > On Tue, 31 Jan 2012, Shilimkar, Santosh wrote: > >> A week back I was discussing with Benoit and Tony about having some >> infrastructure like "unused clocks" so that we can shutdown those >> modules, and if possible some power domains. That way they get >> removed at one single place and they the power domain calls >> need not iterate over the once which are no longer needed. > > Sure, if you have some ideas, that sounds promising. ?I'd assume the main > performance issues would be related to global PRCM register reads, rather > than raw CPU cycles. ?So that's probably where it would make sense to > focus the effort. > I fully agree that PRCM register read time is the main issue. Would discuss this with Benoit and Rajendra bit more on what we can be done. > We should also be able to optimize out a fair number of the powerdomain > register reads if we cache them in the struct powerdomain. ?The current > powerstate and next powerstate seem to be hit a lot, and those should both > be cacheable. ?Will probably do some of this as part of the functional > powerstate code. > PD register caching seems to be a good optimization. Regards Santosh ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst() 2012-01-31 0:14 ` Kevin Hilman 2012-01-31 3:53 ` Rajendra Nayak 2012-01-31 6:57 ` Shilimkar, Santosh @ 2012-01-31 17:29 ` Kevin Hilman 2012-02-01 19:27 ` Paul Walmsley 2 siblings, 1 reply; 28+ messages in thread From: Kevin Hilman @ 2012-01-31 17:29 UTC (permalink / raw) To: linux-arm-kernel Paul, Kevin Hilman <khilman@ti.com> writes: > Paul Walmsley <paul@pwsan.com> writes: > >> Remove some superfluous calls to pwrdm_clear_all_prev_pwrst(). >> pwrdm_pre_transition(), which appears a few lines after these calls, >> invokes pwrdm_clear_all_prev_pwrst() on each powerdomain -- there's no >> need to do it twice. > > It looks like these two for OMAP4 are surpurfluous since the immediately > follow a call to pwrdm_pre_transition() as well. > > Santosh/Rajendra, please confirm/ack. So after the discussion, do you want to fold this into the original patch, or do you want a separate patch? Kevin ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst() 2012-01-31 17:29 ` Kevin Hilman @ 2012-02-01 19:27 ` Paul Walmsley 2012-02-02 7:13 ` Shilimkar, Santosh 2012-02-02 18:14 ` Kevin Hilman 0 siblings, 2 replies; 28+ messages in thread From: Paul Walmsley @ 2012-02-01 19:27 UTC (permalink / raw) To: linux-arm-kernel On Tue, 31 Jan 2012, Kevin Hilman wrote: > Kevin Hilman <khilman@ti.com> writes: > > > Paul Walmsley <paul@pwsan.com> writes: > > > >> Remove some superfluous calls to pwrdm_clear_all_prev_pwrst(). > >> pwrdm_pre_transition(), which appears a few lines after these calls, > >> invokes pwrdm_clear_all_prev_pwrst() on each powerdomain -- there's no > >> need to do it twice. > > > > It looks like these two for OMAP4 are surpurfluous since the immediately > > follow a call to pwrdm_pre_transition() as well. > > > > Santosh/Rajendra, please confirm/ack. > > So after the discussion, do you want to fold this into the original > patch, or do you want a separate patch? Your changes make sense to me. I am fine with you adding them into the original patch and adding some credit for you into the commit message. Or you can create a separate patch. N.B., I haven't looked at this file before. There are a few other things that don't look right that hopefully someone can take the initiative to fix. For example, those calls to mpuss_clear_prev_logic_pwrst() and cpu_clear_prev_logic_pwrst() should be removed as well. That should be done by pwrdm_clear_all_prev_pwrst() in mach-omap2/powerdomain44xx.c. Currently it's not, so that needs to be patched. Also all of the open-coded powerdomain register accesses in omap-mpuss-lowpower.c should be removed - those should all go through pwrdm_*() functions... - Paul ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst() 2012-02-01 19:27 ` Paul Walmsley @ 2012-02-02 7:13 ` Shilimkar, Santosh 2012-02-02 8:33 ` Paul Walmsley 2012-02-02 18:14 ` Kevin Hilman 1 sibling, 1 reply; 28+ messages in thread From: Shilimkar, Santosh @ 2012-02-02 7:13 UTC (permalink / raw) To: linux-arm-kernel On Thu, Feb 2, 2012 at 12:57 AM, Paul Walmsley <paul@pwsan.com> wrote: > On Tue, 31 Jan 2012, Kevin Hilman wrote: > >> Kevin Hilman <khilman@ti.com> writes: >> >> > Paul Walmsley <paul@pwsan.com> writes: >> > >> >> Remove some superfluous calls to pwrdm_clear_all_prev_pwrst(). >> >> pwrdm_pre_transition(), which appears a few lines after these calls, >> >> invokes pwrdm_clear_all_prev_pwrst() on each powerdomain -- there's no >> >> need to do it twice. >> > >> > It looks like these two for OMAP4 are surpurfluous since the immediately >> > follow a call to pwrdm_pre_transition() as well. >> > >> > Santosh/Rajendra, please confirm/ack. >> >> So after the discussion, do you want to fold this into the original >> patch, or do you want a separate patch? > > Your changes make sense to me. ?I am fine with you adding them into the > original patch and adding some credit for you into the commit message. > Or you can create a separate patch. > > N.B., I haven't looked at this file before. ?There are a few other things > that don't look right that hopefully someone can take the initiative to > fix. ?For example, those calls to mpuss_clear_prev_logic_pwrst() and > cpu_clear_prev_logic_pwrst() should be removed as well. ?That should be > done by pwrdm_clear_all_prev_pwrst() in mach-omap2/powerdomain44xx.c. > Currently it's not, so that needs to be patched. > We(rajendra and myself) discussed this some time back with Benoit and we agreed that for CPU and MPU power domains which are needed very early in the PM power up sequence, we can do direct APIs.. These calls were in critical path and the PD API's proposed for context clear and logic clear were use to iterate over all power domains. It is only recently we got clear context etc some reasonable form even though it still iterates over all hwmod etc which is really overhead. Also you need to create 'pdev' etc for context-loss kind of APIs and we all thought that that makes things un-ncessary complicated and lengthy. > Also all of the open-coded powerdomain register accesses in > omap-mpuss-lowpower.c should be removed - those should all go through > pwrdm_*() functions... > I will have another look at the code with your recent power domain clean-ups and see what all can be moved to generic APIs. Regards Santosh ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst() 2012-02-02 7:13 ` Shilimkar, Santosh @ 2012-02-02 8:33 ` Paul Walmsley 2012-02-02 8:59 ` Shilimkar, Santosh 0 siblings, 1 reply; 28+ messages in thread From: Paul Walmsley @ 2012-02-02 8:33 UTC (permalink / raw) To: linux-arm-kernel Hi On Thu, 2 Feb 2012, Shilimkar, Santosh wrote: > On Thu, Feb 2, 2012 at 12:57 AM, Paul Walmsley <paul@pwsan.com> wrote: > > > N.B., I haven't looked at this file before. ?There are a few other things > > that don't look right that hopefully someone can take the initiative to > > fix. ?For example, those calls to mpuss_clear_prev_logic_pwrst() and > > cpu_clear_prev_logic_pwrst() should be removed as well. ?That should be > > done by pwrdm_clear_all_prev_pwrst() in mach-omap2/powerdomain44xx.c. > > Currently it's not, so that needs to be patched. > > > We(rajendra and myself) discussed this some time back with Benoit and we agreed > that for CPU and MPU power domains which are needed very early in the PM power > up sequence, we can do direct APIs.. Looking at the call sites, don't these calls occur quite late in boot, after everything is initialized? I see calls from a late_initcall() and a suspend entry function. Am I missing something? > These calls were in critical path and the PD API's proposed for context clear > and logic clear were use to iterate over all power domains. The code iterates over all powerdomains anyway a few lines above those calls, right? If the code only needs to iterate over a subset, let's discuss what you need and a different iterator can be implemented in the powerdomain code. > It is only recently we got clear context etc some reasonable form > even though it still iterates over all hwmod etc which is really overhead. Which mainline PM calls iterate over all hwmods? > Also you need to create 'pdev' etc for context-loss kind of APIs and > we all thought that that makes things un-ncessary complicated and > lengthy. Could you point out the code that you are referring to? I seem to be missing it :-( If the existing powerdomain API is missing something you need, there's no problem to add it. It makes things easier to debug and more portable in the long term if there is a clean, standard interface. Another hope is that more of the PM code can be shared. In terms of performance, it seems pretty unlikely that one would be able to measure a meaningful performance difference, unless we are missing an API call that was needed? As we were discussing before, device reads are likely to dominate the profile. Flipping through the code, I see that code is getting duplicated again. We now have three identical copies of clkdms_setup(). The OMAP4 pwrdms_setup() is doing string comparisons to skip powerdomains -- that's also pretty fragile; that should be controlled through powerdomain flags or some similar mechanism. Once that's fixed, it looks to me like that function could be shared with the OMAP34xx pwrdms_setup()? Looks like we could also share omap{3,4}_pm_suspend() and omap{2,3,4}_pm_{begin,end}? It would be good if we can keep working towards making as much of this code as common as possible. If already-tested code can be reused, that should cut down on bugs and maintainer workload. Also, we can avoid adding unnecessary lines of diff; we are trying to cut that down too.. > > Also all of the open-coded powerdomain register accesses in > > omap-mpuss-lowpower.c should be removed - those should all go through > > pwrdm_*() functions... > > > I will have another look at the code with your recent power domain > clean-ups and see what all can be moved to generic APIs. That would be great! - Paul ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst() 2012-02-02 8:33 ` Paul Walmsley @ 2012-02-02 8:59 ` Shilimkar, Santosh 2012-02-02 10:05 ` Paul Walmsley 0 siblings, 1 reply; 28+ messages in thread From: Shilimkar, Santosh @ 2012-02-02 8:59 UTC (permalink / raw) To: linux-arm-kernel On Thu, Feb 2, 2012 at 2:03 PM, Paul Walmsley <paul@pwsan.com> wrote: > Hi > > On Thu, 2 Feb 2012, Shilimkar, Santosh wrote: > >> On Thu, Feb 2, 2012 at 12:57 AM, Paul Walmsley <paul@pwsan.com> wrote: >> >> > N.B., I haven't looked at this file before. ?There are a few other things >> > that don't look right that hopefully someone can take the initiative to >> > fix. ?For example, those calls to mpuss_clear_prev_logic_pwrst() and >> > cpu_clear_prev_logic_pwrst() should be removed as well. ?That should be >> > done by pwrdm_clear_all_prev_pwrst() in mach-omap2/powerdomain44xx.c. >> > Currently it's not, so that needs to be patched. >> > >> We(rajendra and myself) discussed this some time back with Benoit and we agreed >> that for CPU and MPU power domains which are needed very early in the PM power >> up sequence, we can do direct APIs.. > > Looking at the call sites, don't these calls occur quite late in boot, > after everything is initialized? ?I see calls from a late_initcall() and a > suspend entry function. ?Am I missing something? > The code has changed now because of the recent ARM suspend code and CPU PM idle notifiers consolidation. O.w before we had to check the last power state as early as MMU is with identity mapping and we use to check the CPU last power state. Then for the interrupt controller enable/disable etc as well we use to check MPUSS PD context lost state etc. Thanks to the consolidation, some of those situations have reduced now. >> These calls were in critical path and the PD API's proposed for context clear >> and logic clear were use to iterate over all power domains. > > The code iterates over all powerdomains anyway a few lines above those > calls, right? > As I said in other thread, that code was under PM debug. > If the code only needs to iterate over a subset, let's discuss what you > need and a different iterator can be implemented in the powerdomain code. > >> It is only recently we got clear context etc some reasonable form >> even though it still iterates over all hwmod etc which is really overhead. > > Which mainline PM calls iterate over all hwmods? > >> Also you need to create 'pdev' etc for context-loss kind of APIs and >> we all thought that that makes things un-ncessary complicated and >> lengthy. > > Could you point out the code that you are referring to? ?I seem to be > missing it :-( > > If the existing powerdomain API is missing something you need, there's no > problem to add it. ?It makes things easier to debug and more portable in > the long term if there is a clean, standard interface. ?Another hope is > that more of the PM code can be shared. > Yes, we do have issue with below APIs for OMAP4 and onwards design because of the PRCM change. pwrdm_clear_all_prev_pwrst pwrdm_*_mem_* There use to be a single power state and memory state register till OMAP3 at power domain level which can give you the logic and memory last test which is needed for OSWR. On OMAP4, we have registers per modules and it becomes difficult to model this difference in APIs. Initially we tried to fix that with [1] and then later realized that's not going to work on OMAP4 + devices because of mentioned issue. Regards Santosh [1] http://permalink.gmane.org/gmane.linux.ports.arm.omap/42564 > In terms of performance, it seems pretty unlikely that one would be able > to measure a meaningful performance difference, unless we are missing an > API call that was needed? ?As we were discussing before, device reads are > likely to dominate the profile. > > Flipping through the code, I see that code is getting duplicated again. > We now have three identical copies of clkdms_setup(). ?The OMAP4 > pwrdms_setup() is doing string comparisons to skip powerdomains -- that's > also pretty fragile; that should be controlled through powerdomain flags > or some similar mechanism. ?Once that's fixed, it looks to me like that > function could be shared with the OMAP34xx pwrdms_setup()? ?Looks like we > could also share omap{3,4}_pm_suspend() and omap{2,3,4}_pm_{begin,end}? > > It would be good if we can keep working towards making as much of this > code as common as possible. ?If already-tested code can be reused, that > should cut down on bugs and maintainer workload. ?Also, we can avoid > adding unnecessary lines of diff; we are trying to cut that down too.. > >> > Also all of the open-coded powerdomain register accesses in >> > omap-mpuss-lowpower.c should be removed - those should all go through >> > pwrdm_*() functions... >> > >> ?I will have another look at the code with your recent power domain >> clean-ups and see what all can be moved to generic APIs. > > That would be great! > > > - Paul ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst() 2012-02-02 8:59 ` Shilimkar, Santosh @ 2012-02-02 10:05 ` Paul Walmsley 2012-02-02 10:17 ` Shilimkar, Santosh 0 siblings, 1 reply; 28+ messages in thread From: Paul Walmsley @ 2012-02-02 10:05 UTC (permalink / raw) To: linux-arm-kernel On Thu, 2 Feb 2012, Shilimkar, Santosh wrote: > Yes, we do have issue with below APIs for OMAP4 and onwards design > because of the PRCM change. > > pwrdm_clear_all_prev_pwrst > pwrdm_*_mem_* > > There use to be a single power state and memory state register till OMAP3 > at power domain level which can give you the logic and memory last test > which is needed for OSWR. > > On OMAP4, we have registers per modules and it becomes difficult to model > this difference in APIs. Initially we tried to fix that with [1] and > then later realized > that's not going to work on OMAP4 + devices because of mentioned issue. If the registers are per-module, it seems like the best place for those would be the hwmod layer. Do you think that makes sense? So, something like omap_hwmod_clear_all_prev_pwrst(), etc.? Seems like that should be pretty efficient. - Paul ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst() 2012-02-02 10:05 ` Paul Walmsley @ 2012-02-02 10:17 ` Shilimkar, Santosh 2012-02-02 15:24 ` Shilimkar, Santosh 0 siblings, 1 reply; 28+ messages in thread From: Shilimkar, Santosh @ 2012-02-02 10:17 UTC (permalink / raw) To: linux-arm-kernel On Thu, Feb 2, 2012 at 3:35 PM, Paul Walmsley <paul@pwsan.com> wrote: > On Thu, 2 Feb 2012, Shilimkar, Santosh wrote: > >> Yes, we do have issue with below APIs for OMAP4 and onwards design >> because of the PRCM change. >> >> pwrdm_clear_all_prev_pwrst >> pwrdm_*_mem_* >> >> There use to be a single power state and memory state register till OMAP3 >> at power domain level which can give you the logic and memory last test >> which is needed for OSWR. >> >> On OMAP4, we have registers per modules and it becomes difficult to model >> this difference in APIs. Initially we tried to fix that with [1] and >> then later realized >> that's not going to work on OMAP4 + devices because of mentioned issue. > > If the registers are per-module, it seems like the best place for those > would be the hwmod layer. ?Do you think that makes sense? ?So, something > like omap_hwmod_clear_all_prev_pwrst(), etc.? ?Seems like that should be > pretty efficient. > But all these are power domain registers after all. Rajendra did one version of " pwrdm_clear_all_prev_pwrst" API and inside used hwmod. But then there he has iterate over all the modules belongs to that power domain. And if you use hwmod or omap_device kind of API, then you need to build those devices in init or some where. All of that was not looking so elegant and hence the other path was chosen. The mess is, the registers are still part of power domains though they are specific to module. And Fundamentally power domain is stil the central entity decides the fate of all the modules within that PD, in terms of context loss/ret etc. Regards santosh ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst() 2012-02-02 10:17 ` Shilimkar, Santosh @ 2012-02-02 15:24 ` Shilimkar, Santosh 2012-02-02 19:59 ` Paul Walmsley 0 siblings, 1 reply; 28+ messages in thread From: Shilimkar, Santosh @ 2012-02-02 15:24 UTC (permalink / raw) To: linux-arm-kernel Paul, On Thu, Feb 2, 2012 at 3:47 PM, Shilimkar, Santosh <santosh.shilimkar@ti.com> wrote: > On Thu, Feb 2, 2012 at 3:35 PM, Paul Walmsley <paul@pwsan.com> wrote: >> On Thu, 2 Feb 2012, Shilimkar, Santosh wrote: >> >>> Yes, we do have issue with below APIs for OMAP4 and onwards design >>> because of the PRCM change. >>> >>> pwrdm_clear_all_prev_pwrst >>> pwrdm_*_mem_* >>> >>> There use to be a single power state and memory state register till OMAP3 >>> at power domain level which can give you the logic and memory last test >>> which is needed for OSWR. >>> >>> On OMAP4, we have registers per modules and it becomes difficult to model >>> this difference in APIs. Initially we tried to fix that with [1] and >>> then later realized >>> that's not going to work on OMAP4 + devices because of mentioned issue. >> >> If the registers are per-module, it seems like the best place for those >> would be the hwmod layer. ?Do you think that makes sense? ?So, something >> like omap_hwmod_clear_all_prev_pwrst(), etc.? ?Seems like that should be >> pretty efficient. >> > But all these are power domain registers after all. Rajendra did one version of > " pwrdm_clear_all_prev_pwrst" ?API and inside used hwmod. But then there he > has iterate over all the modules belongs to that power domain. > > And if you use hwmod or omap_device kind of API, then ?you need to > build those devices in init or some where. > > All of that was not looking so elegant and hence the other path was chosen. > The mess is, the registers are still part of power domains though they > are specific > to module. And Fundamentally power domain is stil the central entity > decides the fate of all the modules within that PD, in terms of context > loss/ret etc. > I looked at the MPUSS file. There are 3 functions which access power domain registers directly. - mpuss_clear_prev_logic_pwrst - cpu_clear_prev_logic_pwrst - omap4_mpuss_read_prev_context_state All of these functions access the context registers which we don't have support today at power domian APIs for mentioned issue. This file is using power domain APIs in all possible scenario's except the OSWR scenario which needs the context register accesses. if the context clear and read can be handled part of PD APIs, then we can certainly kill this functions. Regards santosh ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst() 2012-02-02 15:24 ` Shilimkar, Santosh @ 2012-02-02 19:59 ` Paul Walmsley 0 siblings, 0 replies; 28+ messages in thread From: Paul Walmsley @ 2012-02-02 19:59 UTC (permalink / raw) To: linux-arm-kernel Hi Santosh On Thu, 2 Feb 2012, Shilimkar, Santosh wrote: > On Thu, Feb 2, 2012 at 3:47 PM, Shilimkar, Santosh > <santosh.shilimkar@ti.com> wrote: > > On Thu, Feb 2, 2012 at 3:35 PM, Paul Walmsley <paul@pwsan.com> wrote: > >> On Thu, 2 Feb 2012, Shilimkar, Santosh wrote: > >> > >>> Yes, we do have issue with below APIs for OMAP4 and onwards design > >>> because of the PRCM change. > >>> > >>> pwrdm_clear_all_prev_pwrst > >>> pwrdm_*_mem_* > >>> > >>> There use to be a single power state and memory state register till OMAP3 > >>> at power domain level which can give you the logic and memory last test > >>> which is needed for OSWR. > >>> > >>> On OMAP4, we have registers per modules and it becomes difficult to model > >>> this difference in APIs. Initially we tried to fix that with [1] and > >>> then later realized > >>> that's not going to work on OMAP4 + devices because of mentioned issue. > >> > >> If the registers are per-module, it seems like the best place for those > >> would be the hwmod layer. ?Do you think that makes sense? ?So, something > >> like omap_hwmod_clear_all_prev_pwrst(), etc.? ?Seems like that should be > >> pretty efficient. > >> > > But all these are power domain registers after all. Rajendra did one version of > > " pwrdm_clear_all_prev_pwrst" ?API and inside used hwmod. But then there he > > has iterate over all the modules belongs to that power domain. > > > > And if you use hwmod or omap_device kind of API, then ?you need to > > build those devices in init or some where. > > > > All of that was not looking so elegant and hence the other path was chosen. > > The mess is, the registers are still part of power domains though they > > are specific > > to module. And Fundamentally power domain is stil the central entity > > decides the fate of all the modules within that PD, in terms of context > > loss/ret etc. > > > I looked at the MPUSS file. There are 3 functions which access > power domain registers directly. > > - mpuss_clear_prev_logic_pwrst > - cpu_clear_prev_logic_pwrst > - omap4_mpuss_read_prev_context_state > > All of these functions access the context registers which > we don't have support today at power domian APIs for > mentioned issue. This file is using power domain APIs > in all possible scenario's except the OSWR scenario > which needs the context register accesses. > > if the context clear and read can be handled part of > PD APIs, then we can certainly kill this functions. That sounds great. Maybe we can add powerdomain functions for these purposes that take both a powerdomain pointer and a hwmod pointer. That should hopefully work. - Paul ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst() 2012-02-01 19:27 ` Paul Walmsley 2012-02-02 7:13 ` Shilimkar, Santosh @ 2012-02-02 18:14 ` Kevin Hilman 1 sibling, 0 replies; 28+ messages in thread From: Kevin Hilman @ 2012-02-02 18:14 UTC (permalink / raw) To: linux-arm-kernel Paul Walmsley <paul@pwsan.com> writes: > On Tue, 31 Jan 2012, Kevin Hilman wrote: > >> Kevin Hilman <khilman@ti.com> writes: >> >> > Paul Walmsley <paul@pwsan.com> writes: >> > >> >> Remove some superfluous calls to pwrdm_clear_all_prev_pwrst(). >> >> pwrdm_pre_transition(), which appears a few lines after these calls, >> >> invokes pwrdm_clear_all_prev_pwrst() on each powerdomain -- there's no >> >> need to do it twice. >> > >> > It looks like these two for OMAP4 are surpurfluous since the immediately >> > follow a call to pwrdm_pre_transition() as well. >> > >> > Santosh/Rajendra, please confirm/ack. >> >> So after the discussion, do you want to fold this into the original >> patch, or do you want a separate patch? > > Your changes make sense to me. I am fine with you adding them into the > original patch and adding some credit for you into the commit message. > Or you can create a separate patch. OK, I'll fold it in. Thanks, Kevin ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 2/2] ARM: OMAP2+: PM: clean up omap_set_pwrdm_state() 2012-01-30 9:43 [PATCH 0/2] ARM: OMAP2+: PM: miscellaneous powerdomain-related improvements Paul Walmsley 2012-01-30 9:43 ` [PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst() Paul Walmsley @ 2012-01-30 9:43 ` Paul Walmsley 2012-01-30 12:17 ` Shilimkar, Santosh 2012-01-31 3:46 ` Rajendra Nayak 2012-01-30 21:27 ` [PATCH 0/2] ARM: OMAP2+: PM: miscellaneous powerdomain-related improvements Kevin Hilman 2 siblings, 2 replies; 28+ messages in thread From: Paul Walmsley @ 2012-01-30 9:43 UTC (permalink / raw) To: linux-arm-kernel Clean up a few different parts of omap_set_pwrdm_state(): - Remove a superfluous call to pwrdm_state_switch(). Not needed unless LOWPOWERSTATECHANGE is used, because the state switch code is called by either clkdm_sleep() or clkdm_allow_idle(). - Add code to wait for the power state transition in the OMAP4+ low power state change. This is speculative, so I would particularly appreciate feedback on this part. - Remove a superfluous call to pwrdm_read_pwrst(). - Update variable names to be more meaningful (hopefully) and precise. - Fix an error path bug that would not place the clockdomain back into hardware-supervised idle or sleep mode if the power state could not be programmed. The documentation for this function still needs major improvements; that's left for a later patch. Signed-off-by: Paul Walmsley <paul@pwsan.com> Cc: Kevin Hilman <khilman@ti.com> Cc: Rajendra Nayak <rnayak@ti.com> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com> Cc: Tero Kristo <t-kristo@ti.com> --- arch/arm/mach-omap2/pm.c | 39 +++++++++++++++++---------------------- 1 files changed, 17 insertions(+), 22 deletions(-) diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c index 1881fe9..137d2d9 100644 --- a/arch/arm/mach-omap2/pm.c +++ b/arch/arm/mach-omap2/pm.c @@ -72,28 +72,27 @@ static void omap2_init_processor_devices(void) * This sets pwrdm state (other than mpu & core. Currently only ON & * RET are supported. */ -int omap_set_pwrdm_state(struct powerdomain *pwrdm, u32 state) +int omap_set_pwrdm_state(struct powerdomain *pwrdm, u32 pwrst) { - u32 cur_state; - int sleep_switch = -1; - int ret = 0; - int hwsup = 0; + u8 curr_pwrst, next_pwrst; + int sleep_switch = -1, ret = 0, hwsup = 0; - if (pwrdm == NULL || IS_ERR(pwrdm)) + if (!pwrdm || IS_ERR(pwrdm)) return -EINVAL; - while (!(pwrdm->pwrsts & (1 << state))) { - if (state == PWRDM_POWER_OFF) + while (!(pwrdm->pwrsts & (1 << pwrst))) { + if (pwrst == PWRDM_POWER_OFF) return ret; - state--; + pwrst--; } - cur_state = pwrdm_read_next_pwrst(pwrdm); - if (cur_state == state) + next_pwrst = pwrdm_read_next_pwrst(pwrdm); + if (next_pwrst == pwrst) return ret; - if (pwrdm_read_pwrst(pwrdm) < PWRDM_POWER_ON) { - if ((pwrdm_read_pwrst(pwrdm) > state) && + curr_pwrst = pwrdm_read_pwrst(pwrdm); + if (curr_pwrst < PWRDM_POWER_ON) { + if ((curr_pwrst > pwrst) && (pwrdm->flags & PWRDM_HAS_LOWPOWERSTATECHANGE)) { sleep_switch = LOWPOWERSTATE_SWITCH; } else { @@ -103,12 +102,10 @@ int omap_set_pwrdm_state(struct powerdomain *pwrdm, u32 state) } } - ret = pwrdm_set_next_pwrst(pwrdm, state); - if (ret) { - pr_err("%s: unable to set state of powerdomain: %s\n", + ret = pwrdm_set_next_pwrst(pwrdm, pwrst); + if (ret) + pr_err("%s: unable to set power state of powerdomain: %s\n", __func__, pwrdm->name); - goto err; - } switch (sleep_switch) { case FORCEWAKEUP_SWITCH: @@ -119,13 +116,11 @@ int omap_set_pwrdm_state(struct powerdomain *pwrdm, u32 state) break; case LOWPOWERSTATE_SWITCH: pwrdm_set_lowpwrstchange(pwrdm); + pwrdm_wait_transition(pwrdm); + pwrdm_state_switch(pwrdm); break; - default: - return ret; } - pwrdm_state_switch(pwrdm); -err: return ret; } ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 2/2] ARM: OMAP2+: PM: clean up omap_set_pwrdm_state() 2012-01-30 9:43 ` [PATCH 2/2] ARM: OMAP2+: PM: clean up omap_set_pwrdm_state() Paul Walmsley @ 2012-01-30 12:17 ` Shilimkar, Santosh 2012-01-31 3:46 ` Rajendra Nayak 1 sibling, 0 replies; 28+ messages in thread From: Shilimkar, Santosh @ 2012-01-30 12:17 UTC (permalink / raw) To: linux-arm-kernel On Mon, Jan 30, 2012 at 3:13 PM, Paul Walmsley <paul@pwsan.com> wrote: > Clean up a few different parts of omap_set_pwrdm_state(): > > - Remove a superfluous call to pwrdm_state_switch(). ?Not needed > ?unless LOWPOWERSTATECHANGE is used, because the state switch code is > ?called by either clkdm_sleep() or clkdm_allow_idle(). > Indeed > - Add code to wait for the power state transition in the OMAP4+ low > ?power state change. ?This is speculative, so I would particularly > ?appreciate feedback on this part. > > - Remove a superfluous call to pwrdm_read_pwrst(). > > - Update variable names to be more meaningful (hopefully) and precise. > > - Fix an error path bug that would not place the clockdomain back into > ?hardware-supervised idle or sleep mode if the power state could not > ?be programmed. > > The documentation for this function still needs major improvements; > that's left for a later patch. > > Signed-off-by: Paul Walmsley <paul@pwsan.com> > Cc: Kevin Hilman <khilman@ti.com> > Cc: Rajendra Nayak <rnayak@ti.com> > Cc: Santosh Shilimkar <santosh.shilimkar@ti.com> > Cc: Tero Kristo <t-kristo@ti.com> > --- All the changes look fine to me from OMAP4 perspective. Would be good if Tero can try out this patch and test CORE RET on OMAP4. Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com> Regards santosh ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 2/2] ARM: OMAP2+: PM: clean up omap_set_pwrdm_state() 2012-01-30 9:43 ` [PATCH 2/2] ARM: OMAP2+: PM: clean up omap_set_pwrdm_state() Paul Walmsley 2012-01-30 12:17 ` Shilimkar, Santosh @ 2012-01-31 3:46 ` Rajendra Nayak 1 sibling, 0 replies; 28+ messages in thread From: Rajendra Nayak @ 2012-01-31 3:46 UTC (permalink / raw) To: linux-arm-kernel Hi Paul, On Monday 30 January 2012 03:13 PM, Paul Walmsley wrote: > Clean up a few different parts of omap_set_pwrdm_state(): > > - Remove a superfluous call to pwrdm_state_switch(). Not needed > unless LOWPOWERSTATECHANGE is used, because the state switch code is > called by either clkdm_sleep() or clkdm_allow_idle(). > > - Add code to wait for the power state transition in the OMAP4+ low > power state change. This is speculative, so I would particularly > appreciate feedback on this part. > > - Remove a superfluous call to pwrdm_read_pwrst(). > > - Update variable names to be more meaningful (hopefully) and precise. > > - Fix an error path bug that would not place the clockdomain back into > hardware-supervised idle or sleep mode if the power state could not > be programmed. All the changes look good. Thanks. Acked-by: Rajendra Nayak <rnayak@ti.com> regards, Rajendra > ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 0/2] ARM: OMAP2+: PM: miscellaneous powerdomain-related improvements 2012-01-30 9:43 [PATCH 0/2] ARM: OMAP2+: PM: miscellaneous powerdomain-related improvements Paul Walmsley 2012-01-30 9:43 ` [PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst() Paul Walmsley 2012-01-30 9:43 ` [PATCH 2/2] ARM: OMAP2+: PM: clean up omap_set_pwrdm_state() Paul Walmsley @ 2012-01-30 21:27 ` Kevin Hilman 2012-01-31 7:21 ` Tero Kristo 2012-02-01 13:55 ` Tero Kristo 2 siblings, 2 replies; 28+ messages in thread From: Kevin Hilman @ 2012-01-30 21:27 UTC (permalink / raw) To: linux-arm-kernel Paul Walmsley <paul@pwsan.com> writes: > This series optimizes some of the powerdomain-related code in > arch/arm/mach-omap2/pm*, and fixes a bug or two. These were noticed while > working on the functional powerstate code. > > Rajendra and Santosh, if you have a spare moment, could you please > peek at the OMAP4 LOWPOWERSTATECHANGE part of the second patch? It > makes sense to me in theory, but you both would probably know better > than I :-) > > Kevin, want to take this series (assuming folks are happy with it) ? Yes, I'll take this and add the Ack from Santosh. Thanks for the nice cleanup. Will submit after seeing a Tested-by from someone who can test with CORE retention/off on OMAP4 (hint, hint, Tero :) Kevin ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 0/2] ARM: OMAP2+: PM: miscellaneous powerdomain-related improvements 2012-01-30 21:27 ` [PATCH 0/2] ARM: OMAP2+: PM: miscellaneous powerdomain-related improvements Kevin Hilman @ 2012-01-31 7:21 ` Tero Kristo 2012-02-01 13:55 ` Tero Kristo 1 sibling, 0 replies; 28+ messages in thread From: Tero Kristo @ 2012-01-31 7:21 UTC (permalink / raw) To: linux-arm-kernel On Mon, 2012-01-30 at 13:27 -0800, Kevin Hilman wrote: > Paul Walmsley <paul@pwsan.com> writes: > > > This series optimizes some of the powerdomain-related code in > > arch/arm/mach-omap2/pm*, and fixes a bug or two. These were noticed while > > working on the functional powerstate code. > > > > Rajendra and Santosh, if you have a spare moment, could you please > > peek at the OMAP4 LOWPOWERSTATECHANGE part of the second patch? It > > makes sense to me in theory, but you both would probably know better > > than I :-) > > > > Kevin, want to take this series (assuming folks are happy with it) ? > > Yes, I'll take this and add the Ack from Santosh. Thanks for the nice > cleanup. > > Will submit after seeing a Tested-by from someone who can test with CORE > retention/off on OMAP4 (hint, hint, Tero :) Yeah, I can test these out hopefully tomorrow / today, I am busy today creating a release but lets see. :) The patches seem okay to me also at least. -Tero ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 0/2] ARM: OMAP2+: PM: miscellaneous powerdomain-related improvements 2012-01-30 21:27 ` [PATCH 0/2] ARM: OMAP2+: PM: miscellaneous powerdomain-related improvements Kevin Hilman 2012-01-31 7:21 ` Tero Kristo @ 2012-02-01 13:55 ` Tero Kristo 1 sibling, 0 replies; 28+ messages in thread From: Tero Kristo @ 2012-02-01 13:55 UTC (permalink / raw) To: linux-arm-kernel On Mon, 2012-01-30 at 13:27 -0800, Kevin Hilman wrote: > Paul Walmsley <paul@pwsan.com> writes: > > > This series optimizes some of the powerdomain-related code in > > arch/arm/mach-omap2/pm*, and fixes a bug or two. These were noticed while > > working on the functional powerstate code. > > > > Rajendra and Santosh, if you have a spare moment, could you please > > peek at the OMAP4 LOWPOWERSTATECHANGE part of the second patch? It > > makes sense to me in theory, but you both would probably know better > > than I :-) > > > > Kevin, want to take this series (assuming folks are happy with it) ? > > Yes, I'll take this and add the Ack from Santosh. Thanks for the nice > cleanup. > > Will submit after seeing a Tested-by from someone who can test with CORE > retention/off on OMAP4 (hint, hint, Tero :) The patches appear to behave nicely on omap4, there are still a couple of quirks in the basic omap4 support though. l3init pd state is not updated properly once we wake-up from device-off (it remains active for a short while during wake-up before switching to idle with hwauto), and cpu1 pwrdm state is not updated properly during hotplug, but these are minor issues and should be fixed elsewhere. You can add: Tested-by: Tero Kristo <t-kristo@ti.com> > > Kevin ^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2012-02-02 19:59 UTC | newest] Thread overview: 28+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-01-30 9:43 [PATCH 0/2] ARM: OMAP2+: PM: miscellaneous powerdomain-related improvements Paul Walmsley 2012-01-30 9:43 ` [PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst() Paul Walmsley 2012-01-30 10:54 ` Shilimkar, Santosh 2012-01-31 0:14 ` Kevin Hilman 2012-01-31 3:53 ` Rajendra Nayak 2012-01-31 6:57 ` Shilimkar, Santosh 2012-01-31 7:15 ` Paul Walmsley 2012-01-31 7:23 ` Shilimkar, Santosh 2012-01-31 7:27 ` Paul Walmsley 2012-01-31 7:34 ` Shilimkar, Santosh 2012-01-31 7:49 ` Paul Walmsley 2012-01-31 8:37 ` Shilimkar, Santosh 2012-01-31 17:29 ` Kevin Hilman 2012-02-01 19:27 ` Paul Walmsley 2012-02-02 7:13 ` Shilimkar, Santosh 2012-02-02 8:33 ` Paul Walmsley 2012-02-02 8:59 ` Shilimkar, Santosh 2012-02-02 10:05 ` Paul Walmsley 2012-02-02 10:17 ` Shilimkar, Santosh 2012-02-02 15:24 ` Shilimkar, Santosh 2012-02-02 19:59 ` Paul Walmsley 2012-02-02 18:14 ` Kevin Hilman 2012-01-30 9:43 ` [PATCH 2/2] ARM: OMAP2+: PM: clean up omap_set_pwrdm_state() Paul Walmsley 2012-01-30 12:17 ` Shilimkar, Santosh 2012-01-31 3:46 ` Rajendra Nayak 2012-01-30 21:27 ` [PATCH 0/2] ARM: OMAP2+: PM: miscellaneous powerdomain-related improvements Kevin Hilman 2012-01-31 7:21 ` Tero Kristo 2012-02-01 13:55 ` Tero Kristo
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).