| Message ID | 1310527588-13022-2-git-send-email-santosh.shilimkar@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
cc'ing Patrick Hi Rajendra, Santosh, some comments here: On Wed, 13 Jul 2011, Santosh Shilimkar wrote: > While using clockdomain force wakeup method, not waiting for powerdomain > to be effectively ON may end up locking the clockdomain FSM until a > next wakeup event occurs. > > One such issue was seen on OMAP4430, where L4_PER was periodically > getting stuck in in-transition state when transitioning from from OSWR to ON. > > This issue was reported and investigated by Patrick Titiano <p-titiano@ti.com> > > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > Signed-off-by: Rajendra Nayak <rnayak@ti.com> > Reported-by: Patrick Titiano <p-titiano@ti.com> > Cc: Kevin Hilman <khilman@ti.com> > Cc: Benoit Cousson <b-cousson@ti.com> > Cc: Paul Walmsley <paul@pwsan.com> > --- > arch/arm/mach-omap2/clockdomain.c | 7 ++++++- > 1 files changed, 6 insertions(+), 1 deletions(-) > > diff --git a/arch/arm/mach-omap2/clockdomain.c b/arch/arm/mach-omap2/clockdomain.c > index b98a972..583cc3d 100644 > --- a/arch/arm/mach-omap2/clockdomain.c > +++ b/arch/arm/mach-omap2/clockdomain.c > @@ -718,6 +718,8 @@ int clkdm_sleep(struct clockdomain *clkdm) > */ > int clkdm_wakeup(struct clockdomain *clkdm) > { > + int ret; > + > if (!clkdm) > return -EINVAL; > > @@ -732,7 +734,10 @@ int clkdm_wakeup(struct clockdomain *clkdm) > > pr_debug("clockdomain: forcing wakeup on %s\n", clkdm->name); > > - return arch_clkdm->clkdm_wakeup(clkdm); > + ret = arch_clkdm->clkdm_wakeup(clkdm); > + ret |= pwrdm_wait_transition(clkdm->pwrdm.ptr); Seems like this should just call pwrdm_state_switch() or pwrdm_clkdm_state_switch()? (This second function looks superfluous, we should probably get rid of it.) Shouldn't this be added to all of clkdm_{wakeup,sleep,allow_idle,deny_idle}() if it isn't there already? > + > + return ret; > } > > /** > -- > 1.7.4.1 > - Paul -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Paul, On 7/15/2011 1:03 AM, Paul Walmsley wrote: > cc'ing Patrick > > Hi Rajendra, Santosh, > > some comments here: > > On Wed, 13 Jul 2011, Santosh Shilimkar wrote: > >> While using clockdomain force wakeup method, not waiting for powerdomain >> to be effectively ON may end up locking the clockdomain FSM until a >> next wakeup event occurs. >> >> One such issue was seen on OMAP4430, where L4_PER was periodically >> getting stuck in in-transition state when transitioning from from OSWR to ON. >> >> This issue was reported and investigated by Patrick Titiano<p-titiano@ti.com> >> >> Signed-off-by: Santosh Shilimkar<santosh.shilimkar@ti.com> >> Signed-off-by: Rajendra Nayak<rnayak@ti.com> >> Reported-by: Patrick Titiano<p-titiano@ti.com> >> Cc: Kevin Hilman<khilman@ti.com> >> Cc: Benoit Cousson<b-cousson@ti.com> >> Cc: Paul Walmsley<paul@pwsan.com> >> --- >> arch/arm/mach-omap2/clockdomain.c | 7 ++++++- >> 1 files changed, 6 insertions(+), 1 deletions(-) >> >> diff --git a/arch/arm/mach-omap2/clockdomain.c b/arch/arm/mach-omap2/clockdomain.c >> index b98a972..583cc3d 100644 >> --- a/arch/arm/mach-omap2/clockdomain.c >> +++ b/arch/arm/mach-omap2/clockdomain.c >> @@ -718,6 +718,8 @@ int clkdm_sleep(struct clockdomain *clkdm) >> */ >> int clkdm_wakeup(struct clockdomain *clkdm) >> { >> + int ret; >> + >> if (!clkdm) >> return -EINVAL; >> >> @@ -732,7 +734,10 @@ int clkdm_wakeup(struct clockdomain *clkdm) >> >> pr_debug("clockdomain: forcing wakeup on %s\n", clkdm->name); >> >> - return arch_clkdm->clkdm_wakeup(clkdm); >> + ret = arch_clkdm->clkdm_wakeup(clkdm); >> + ret |= pwrdm_wait_transition(clkdm->pwrdm.ptr); > > Seems like this should just call pwrdm_state_switch() or > pwrdm_clkdm_state_switch()? (This second function looks superfluous, we > should probably get rid of it.) > This comment was expected since initially we thought of using pwrdm_clkdm_state_switch pwrdm_clkdm_state_switch() |--> pwrdm_wait_transition() |--> pwrdm_state_switch() What we need is only first function to ensure that we don't proceed when PD is in middle of transition. The second function is actually just doing those debug counter updates and we wanted to avoid that since it's a live path. Rajendra observed some huge latencies while doing the profiling and the suspect was "pwrdm_state_switch()" which actually keeps adding overhead because of those counter updates. > Shouldn't this be added to all of > clkdm_{wakeup,sleep,allow_idle,deny_idle}() if it isn't there already? > clkdm_allow_idle() already has the power-domain wait. This patch adds it for clkdm_wakeup() function. clkdm_sleep(), this shouldn't be applicable since power domain sleep transition is depdent on other clockdomains in the PD and it's a lazy operation. That leaves clkdm_deny_idle() and I guess we should address this API as well. If you agree with above, I can update the patch to address remaining clkdm_deny_idle() function. Regards Santosh Regards Santosh -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Paul, On Saturday 16 July 2011 01:18 PM, Santosh Shilimkar wrote: > Hi Paul, > > On 7/15/2011 1:03 AM, Paul Walmsley wrote: >> cc'ing Patrick >> >> Hi Rajendra, Santosh, >> >> some comments here: >> >> On Wed, 13 Jul 2011, Santosh Shilimkar wrote: >> >>> While using clockdomain force wakeup method, not waiting for powerdomain >>> to be effectively ON may end up locking the clockdomain FSM until a >>> next wakeup event occurs. >>> >>> One such issue was seen on OMAP4430, where L4_PER was periodically >>> getting stuck in in-transition state when transitioning from from >>> OSWR to ON. >>> >>> This issue was reported and investigated by Patrick >>> Titiano<p-titiano@ti.com> >>> >>> Signed-off-by: Santosh Shilimkar<santosh.shilimkar@ti.com> >>> Signed-off-by: Rajendra Nayak<rnayak@ti.com> >>> Reported-by: Patrick Titiano<p-titiano@ti.com> >>> Cc: Kevin Hilman<khilman@ti.com> >>> Cc: Benoit Cousson<b-cousson@ti.com> >>> Cc: Paul Walmsley<paul@pwsan.com> >>> --- >>> arch/arm/mach-omap2/clockdomain.c | 7 ++++++- >>> 1 files changed, 6 insertions(+), 1 deletions(-) >>> >>> diff --git a/arch/arm/mach-omap2/clockdomain.c >>> b/arch/arm/mach-omap2/clockdomain.c >>> index b98a972..583cc3d 100644 >>> --- a/arch/arm/mach-omap2/clockdomain.c >>> +++ b/arch/arm/mach-omap2/clockdomain.c >>> @@ -718,6 +718,8 @@ int clkdm_sleep(struct clockdomain *clkdm) >>> */ >>> int clkdm_wakeup(struct clockdomain *clkdm) >>> { >>> + int ret; >>> + >>> if (!clkdm) >>> return -EINVAL; >>> >>> @@ -732,7 +734,10 @@ int clkdm_wakeup(struct clockdomain *clkdm) >>> >>> pr_debug("clockdomain: forcing wakeup on %s\n", clkdm->name); >>> >>> - return arch_clkdm->clkdm_wakeup(clkdm); >>> + ret = arch_clkdm->clkdm_wakeup(clkdm); >>> + ret |= pwrdm_wait_transition(clkdm->pwrdm.ptr); >> >> Seems like this should just call pwrdm_state_switch() or >> pwrdm_clkdm_state_switch()? (This second function looks superfluous, we >> should probably get rid of it.) >> > This comment was expected since initially we thought of using > pwrdm_clkdm_state_switch > > pwrdm_clkdm_state_switch() > |--> pwrdm_wait_transition() > |--> pwrdm_state_switch() > > What we need is only first function to ensure that we don't > proceed when PD is in middle of transition. > > The second function is actually just doing those debug counter > updates and we wanted to avoid that since it's a live path. > Rajendra observed some huge latencies while doing the > profiling and the suspect was "pwrdm_state_switch()" which > actually keeps adding overhead because of those counter > updates. > >> Shouldn't this be added to all of >> clkdm_{wakeup,sleep,allow_idle,deny_idle}() if it isn't there already? >> > clkdm_allow_idle() already has the power-domain wait. > This patch adds it for clkdm_wakeup() function. > clkdm_sleep(), this shouldn't be applicable since power > domain sleep transition is depdent on other clockdomains > in the PD and it's a lazy operation. > That leaves clkdm_deny_idle() and I guess we should > address this API as well. > > If you agree with above, I can update the patch to address > remaining clkdm_deny_idle() function. > Ping? Regards Santosh -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
(cc Tero) Hi Santosh, On Thu, 11 Aug 2011, Santosh wrote: > On Saturday 16 July 2011 01:18 PM, Santosh Shilimkar wrote: > > On 7/15/2011 1:03 AM, Paul Walmsley wrote: > > > On Wed, 13 Jul 2011, Santosh Shilimkar wrote: > > > > > > > - return arch_clkdm->clkdm_wakeup(clkdm); > > > > + ret = arch_clkdm->clkdm_wakeup(clkdm); > > > > + ret |= pwrdm_wait_transition(clkdm->pwrdm.ptr); > > > > > > Seems like this should just call pwrdm_state_switch() or > > > pwrdm_clkdm_state_switch()? (This second function looks superfluous, we > > > should probably get rid of it.) > > > > > This comment was expected since initially we thought of using > > pwrdm_clkdm_state_switch > > > > pwrdm_clkdm_state_switch() > > |--> pwrdm_wait_transition() > > |--> pwrdm_state_switch() > > > > What we need is only first function to ensure that we don't > > proceed when PD is in middle of transition. > > > > The second function is actually just doing those debug counter > > updates and we wanted to avoid that since it's a live path. Those counters are now also used by pwrdm_get_context_loss_count() which ultimately is exposed to device drivers as omap_pm_get_dev_context_loss_count(). If clkdm_wakeup() causes the powerdomain to wake up from a state that caused context to be lost, don't we need to update the counters due to that dependency? > > Rajendra observed some huge latencies while doing the > > profiling and the suspect was "pwrdm_state_switch()" which > > actually keeps adding overhead because of those counter > > updates. Probably we'd better deal with the latency issue in a separate patch. If I recall correctly, the high latency operation in the counters is the PREPWSTST register read. That could probably be optimized out under many circumstances. The counter functionality could be made dependent on CONFIG_PM_DEBUG or something similar. > > > Shouldn't this be added to all of > > > clkdm_{wakeup,sleep,allow_idle,deny_idle}() if it isn't there already? > > > > > clkdm_allow_idle() already has the power-domain wait. > > This patch adds it for clkdm_wakeup() function. > > clkdm_sleep(), this shouldn't be applicable since power > > domain sleep transition is depdent on other clockdomains > > in the PD and it's a lazy operation. That's a good question. Could you ping the PRCM designers on that issue to see if there's any risk that not waiting for a clockdomain force-sleep might cause some FSM issue? We should probably handle this in a separate patch anyway so it doesn't retard this patch. > > That leaves clkdm_deny_idle() and I guess we should > > address this API as well. > > > > If you agree with above, I can update the patch to address I'd propose: 1. changing the patch to use pwrdm_state_switch() 2. adding that to clkdm_deny_idle() also What do you think? - Paul -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/mach-omap2/clockdomain.c b/arch/arm/mach-omap2/clockdomain.c index b98a972..583cc3d 100644 --- a/arch/arm/mach-omap2/clockdomain.c +++ b/arch/arm/mach-omap2/clockdomain.c @@ -718,6 +718,8 @@ int clkdm_sleep(struct clockdomain *clkdm) */ int clkdm_wakeup(struct clockdomain *clkdm) { + int ret; + if (!clkdm) return -EINVAL; @@ -732,7 +734,10 @@ int clkdm_wakeup(struct clockdomain *clkdm) pr_debug("clockdomain: forcing wakeup on %s\n", clkdm->name); - return arch_clkdm->clkdm_wakeup(clkdm); + ret = arch_clkdm->clkdm_wakeup(clkdm); + ret |= pwrdm_wait_transition(clkdm->pwrdm.ptr); + + return ret; } /**