From mboxrd@z Thu Jan 1 00:00:00 1970 From: santosh.shilimkar@ti.com (Santosh Shilimkar) Date: Sat, 16 Jul 2011 00:48:55 -0700 Subject: [PATCH] OMAP: clockdomain: Wait for powerdomain to be ON when using clockdomain force wakeup In-Reply-To: References: <1310527588-13022-1-git-send-email-santosh.shilimkar@ti.com> <1310527588-13022-2-git-send-email-santosh.shilimkar@ti.com> Message-ID: <4E214267.2090700@ti.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 >> >> Signed-off-by: Santosh Shilimkar >> Signed-off-by: Rajendra Nayak >> Reported-by: Patrick Titiano >> Cc: Kevin Hilman >> Cc: Benoit Cousson >> Cc: Paul Walmsley >> --- >> 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