Re: [PATCH 3/3] ARM: OMAP3: PM: cpuidle: optimize the clkdm idle latency in C1 state

[Rajendra Nayak <rnayak@ti.com>]

On Wednesday 20 June 2012 02:01 PM, Jean Pihet wrote:
> Hi Rajendra,
>
> On Wed, Jun 20, 2012 at 10:19 AM, Rajendra Nayak<rnayak@ti.com>  wrote:
>> Hi Jean,
>>
>>
>> On Friday 01 June 2012 08:41 PM, Jean Pihet wrote:
>>>
>>> For a power domain to idle all the clock domains in it must idle.
>>> This patch implements an optimization of the cpuidle code by
>>> denying and later allowing only the first registered clock domain
>>> of a power domain, and so optimizes the latency of the low power code.
>>
>>
>> How much do we really save doing this? I understand what you are doing
>> by looking at the patch but the changelog seems very confusing.
> The gain is on the registers accesses and the internal PRCM state machine.
> If needed the changelog can be updated.

Can you explain a bit more on which register accesses are you talking
about? and some more on the PRCM state machine.

>
>> What the patch does is get rid of a few function indirection
>> and assumes there is only *one* clkdm for mpu and core. Is that right?
> Not exactly. The patch does not assume that there is only one clkdm
> per power domain. It just allows or denies only one clkdm to idle,
> which has the same effect at the power domain level.

Ok, so lets assume mpu on OMAP3 has 2 clkdms, and you allow only one
of them to idle. Will that have the same effect at the power domain
level?
The first line of the change log says "For a power domain to idle *all*
the clock domains in it must idle" and now you say allowing only *one*
clkdm to idle should have the same effect at the power domain level.
I am confused.

>
> Thanks for reviewing,
> Jean
>
>>
>> regards,
>> Rajendra
>>
>>
>>>
>>> The functions _cpuidle_allow_idle and _cpuidle_deny_idle are
>>> not used anymore and so are removed.
>>>
>>> Signed-off-by: Jean Pihet<j-pihet@ti.com>
>>> ---
>>>   arch/arm/mach-omap2/cpuidle34xx.c |   22 ++++------------------
>>>   1 files changed, 4 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
>>> b/arch/arm/mach-omap2/cpuidle34xx.c
>>> index 2e2f1c6..e6ae3fe 100644
>>> --- a/arch/arm/mach-omap2/cpuidle34xx.c
>>> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
>>> @@ -77,20 +77,6 @@ static struct omap3_idle_statedata omap3_idle_data[] =
>>> {
>>>
>>>   static struct powerdomain *mpu_pd, *core_pd, *per_pd, *cam_pd;
>>>
>>> -static int _cpuidle_allow_idle(struct powerdomain *pwrdm,
>>> -                               struct clockdomain *clkdm)
>>> -{
>>> -       clkdm_allow_idle(clkdm);
>>> -       return 0;
>>> -}
>>> -
>>> -static int _cpuidle_deny_idle(struct powerdomain *pwrdm,
>>> -                               struct clockdomain *clkdm)
>>> -{
>>> -       clkdm_deny_idle(clkdm);
>>> -       return 0;
>>> -}
>>> -
>>>   static int __omap3_enter_idle(struct cpuidle_device *dev,
>>>                                 struct cpuidle_driver *drv,
>>>                                 int index)
>>> @@ -108,8 +94,8 @@ static int __omap3_enter_idle(struct cpuidle_device
>>> *dev,
>>>
>>>         /* Deny idle for C1 */
>>>         if (index == 0) {
>>> -               pwrdm_for_each_clkdm(mpu_pd, _cpuidle_deny_idle);
>>> -               pwrdm_for_each_clkdm(core_pd, _cpuidle_deny_idle);
>>> +               clkdm_deny_idle(mpu_pd->pwrdm_clkdms[0]);
>>> +               clkdm_deny_idle(core_pd->pwrdm_clkdms[0]);
>>>         }
>>>
>>>         /*
>>> @@ -131,8 +117,8 @@ static int __omap3_enter_idle(struct cpuidle_device
>>> *dev,
>>>
>>>         /* Re-allow idle for C1 */
>>>         if (index == 0) {
>>> -               pwrdm_for_each_clkdm(mpu_pd, _cpuidle_allow_idle);
>>> -               pwrdm_for_each_clkdm(core_pd, _cpuidle_allow_idle);
>>> +               clkdm_allow_idle(mpu_pd->pwrdm_clkdms[0]);
>>> +               clkdm_allow_idle(core_pd->pwrdm_clkdms[0]);
>>>         }
>>>
>>>   return_sleep_time:
>>
>>