[PATCH] OMAP: clockdomain: Wait for powerdomain to be ON when using clockdomain force wakeup

[Santosh]

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 at ti.com>
>>>
>>> Signed-off-by: Santosh Shilimkar<santosh.shilimkar at ti.com>
>>> Signed-off-by: Rajendra Nayak<rnayak at ti.com>
>>> Reported-by: Patrick Titiano<p-titiano at ti.com>
>>> Cc: Kevin Hilman<khilman at ti.com>
>>> Cc: Benoit Cousson<b-cousson at ti.com>
>>> Cc: Paul Walmsley<paul at 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