[PATCH 0/3] ARM: OMAP3: PM: optimize cpuidle C1 state latency

[PATCH 0/3] ARM: OMAP3: PM: optimize cpuidle C1 state latency

[Jean Pihet]

The C1 state latency can be improved by optimizing the cpuidle low
level code.

The first patch is a precaution fix for patch 2.
Patches 2 & 3 are optimization changes.

Rebased on top of the for_3.6/pm/performance branch of
khilman's tree [1].

Tested on Beagleboard using a DMA-enabled copy from NAND flash
to /dev/null.

[1] git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-omap-pm.git

Jean Pihet (3):
  ARM: OMAP3: PM: cpuidle: default to C1 in next_valid_state
  ARM: OMAP3: PM: cpuidle: optimize the PER latency in C1 state
  ARM: OMAP3: PM: cpuidle: optimize the clkdm idle latency in C1 state

 arch/arm/mach-omap2/cpuidle34xx.c |   71 ++++++++++++++-----------------------
 1 files changed, 27 insertions(+), 44 deletions(-)

-- 
1.7.7.6

[PATCH 1/3] ARM: OMAP3: PM: cpuidle: default to C1 in next_valid_state

[Jean Pihet]

If the next state is no found in the next_valid_state function,
fallback to the default value of C1 (which is state 0).
This prevents the use of a bogus state -1 in the rest of the cpuidle
code.

Signed-off-by: Jean Pihet <j-pihet@ti.com>
---
 arch/arm/mach-omap2/cpuidle34xx.c |    8 +-------
 1 files changed, 1 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c
index 207bc1c..f619a92 100644
--- a/arch/arm/mach-omap2/cpuidle34xx.c
+++ b/arch/arm/mach-omap2/cpuidle34xx.c
@@ -178,7 +178,7 @@ static int next_valid_state(struct cpuidle_device *dev,
 	u32 mpu_deepest_state = PWRDM_POWER_RET;
 	u32 core_deepest_state = PWRDM_POWER_RET;
 	int idx;
-	int next_index = -1;
+	int next_index = 0; /* C1 is the default value */
 
 	if (enable_off_mode) {
 		mpu_deepest_state = PWRDM_POWER_OFF;
@@ -209,12 +209,6 @@ static int next_valid_state(struct cpuidle_device *dev,
 		}
 	}
 
-	/*
-	 * C1 is always valid.
-	 * So, no need to check for 'next_index == -1' outside
-	 * this loop.
-	 */
-
 	return next_index;
 }
 
-- 
1.7.7.6

[PATCH 2/3] ARM: OMAP3: PM: cpuidle: optimize the PER latency in C1 state

[Jean Pihet]

One of the main contributors of the low power code latency is
the PER power domain. To optimize the high-performance and
low-latency C1 state, prevent any PER state which is lower
than the CORE state in C1.

Reported and suggested by Kevin Hilman.

Reported-by: Kevin Hilman <khilman@ti.com>
Signed-off-by: Jean Pihet <j-pihet@ti.com>
---
 arch/arm/mach-omap2/cpuidle34xx.c |   41 +++++++++++++++++++-----------------
 1 files changed, 22 insertions(+), 19 deletions(-)

diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c
index f619a92..2e2f1c6 100644
--- a/arch/arm/mach-omap2/cpuidle34xx.c
+++ b/arch/arm/mach-omap2/cpuidle34xx.c
@@ -222,23 +222,22 @@ static int next_valid_state(struct cpuidle_device *dev,
  * the device to the specified or a safer state.
  */
 static int omap3_enter_idle_bm(struct cpuidle_device *dev,
-				struct cpuidle_driver *drv,
+			       struct cpuidle_driver *drv,
 			       int index)
 {
 	int new_state_idx;
-	u32 core_next_state, per_next_state = 0, per_saved_state = 0, cam_state;
+	u32 core_next_state, per_next_state = 0, per_saved_state = 0;
 	struct omap3_idle_statedata *cx;
 	int ret;
 
 	/*
-	 * Prevent idle completely if CAM is active.
+	 * Use only C1 if CAM is active.
 	 * CAM does not have wakeup capability in OMAP3.
 	 */
-	cam_state = pwrdm_read_pwrst(cam_pd);
-	if (cam_state == PWRDM_POWER_ON) {
+	if (pwrdm_read_pwrst(cam_pd) == PWRDM_POWER_ON)
 		new_state_idx = drv->safe_state_index;
-		goto select_state;
-	}
+	else
+		new_state_idx = next_valid_state(dev, drv, index);
 
 	/*
 	 * FIXME: we currently manage device-specific idle states
@@ -248,24 +247,28 @@ static int omap3_enter_idle_bm(struct cpuidle_device *dev,
 	 *        its own code.
 	 */
 
-	/*
-	 * Prevent PER off if CORE is not in retention or off as this
-	 * would disable PER wakeups completely.
-	 */
-	cx = &omap3_idle_data[index];
+	/* Program PER state */
+	cx = &omap3_idle_data[new_state_idx];
 	core_next_state = cx->core_state;
 	per_next_state = per_saved_state = pwrdm_read_next_pwrst(per_pd);
-	if ((per_next_state == PWRDM_POWER_OFF) &&
-	    (core_next_state > PWRDM_POWER_RET))
-		per_next_state = PWRDM_POWER_RET;
+	if (new_state_idx == 0) {
+		/* In C1 do not allow PER state lower than CORE state */
+		if (per_next_state < core_next_state)
+			per_next_state = core_next_state;
+	} else {
+		/*
+		 * Prevent PER OFF if CORE is not in RETention or OFF as this
+		 * would disable PER wakeups completely.
+		 */
+		if ((per_next_state == PWRDM_POWER_OFF) &&
+		    (core_next_state > PWRDM_POWER_RET))
+			per_next_state = PWRDM_POWER_RET;
+	}
 
 	/* Are we changing PER target state? */
 	if (per_next_state != per_saved_state)
 		pwrdm_set_next_pwrst(per_pd, per_next_state);
 
-	new_state_idx = next_valid_state(dev, drv, index);
-
-select_state:
 	ret = omap3_enter_idle(dev, drv, new_state_idx);
 
 	/* Restore original PER state if it was modified */
@@ -282,7 +285,7 @@ struct cpuidle_driver omap3_idle_driver = {
 	.owner = 	THIS_MODULE,
 	.states = {
 		{
-			.enter		  = omap3_enter_idle,
+			.enter		  = omap3_enter_idle_bm,
 			.exit_latency	  = 2 + 2,
 			.target_residency = 5,
 			.flags		  = CPUIDLE_FLAG_TIME_VALID,
-- 
1.7.7.6

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

[Jean Pihet]

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.

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:
-- 
1.7.7.6

Re: [PATCH 0/3] ARM: OMAP3: PM: optimize cpuidle C1 state latency

[Kevin Hilman]

Hi Jean,

Jean Pihet <jean.pihet@newoldbits.com> writes:

> The C1 state latency can be improved by optimizing the cpuidle low
> level code.
>
> The first patch is a precaution fix for patch 2.
> Patches 2 & 3 are optimization changes.
>
> Rebased on top of the for_3.6/pm/performance branch of
> khilman's tree [1].
>
> Tested on Beagleboard using a DMA-enabled copy from NAND flash
> to /dev/null.

Thanks for following this through.

Adding this series to my for_3.6/pm/performance branch.

Also FYI, combining your series with the various fixes for CORE
retention, I'm seeing CORE hit retention in idle just fine on 3430/n900
and 3530/Overo.

Thanks!

Kevin

Re: [PATCH 0/3] ARM: OMAP3: PM: optimize cpuidle C1 state latency

[Jean Pihet]

On Fri, Jun 1, 2012 at 6:26 PM, Kevin Hilman <khilman@ti.com> wrote:
> Hi Jean,
>
> Jean Pihet <jean.pihet@newoldbits.com> writes:
>
>> The C1 state latency can be improved by optimizing the cpuidle low
>> level code.
>>
>> The first patch is a precaution fix for patch 2.
>> Patches 2 & 3 are optimization changes.
>>
>> Rebased on top of the for_3.6/pm/performance branch of
>> khilman's tree [1].
>>
>> Tested on Beagleboard using a DMA-enabled copy from NAND flash
>> to /dev/null.
>
> Thanks for following this through.
>
> Adding this series to my for_3.6/pm/performance branch.
Great!

>
> Also FYI, combining your series with the various fixes for CORE
> retention, I'm seeing CORE hit retention in idle just fine on 3430/n900
> and 3530/Overo.
Nice, thx for testing!

>
> Thanks!
>
> Kevin

Regards,
Jean

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

[Rajendra Nayak]

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.
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?

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:

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

[Jean Pihet]

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.

> 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.

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:
>
>
--
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

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

[Rajendra Nayak]

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:
>>
>>

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

[Rajendra Nayak]

Hi Jean,

On Wednesday 20 June 2012 02:16 PM, Rajendra Nayak wrote:
> 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.

never mind, I looked at the patch again and then the cpuidle code and
figured what you are doing. Makes sense to me now :-)

regards,
Rajendra
>
>>
>>> 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:
>>>
>>>
>

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

[Rajendra Nayak]

Jean,

On Wednesday 20 June 2012 02:22 PM, Rajendra Nayak wrote:
> Hi Jean,
>
> On Wednesday 20 June 2012 02:16 PM, Rajendra Nayak wrote:
>> 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.
>
> never mind, I looked at the patch again and then the cpuidle code and
> figured what you are doing. Makes sense to me now :-)

How do you like this updated changelog, I just added one more line.

"
For a power domain to idle all the clock domains in it must idle.
Denying just *one* clkdm in a pwrdm from idling should have the
same effect as denying *all*.
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.
"

regards,
Rajendra

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

[Jean Pihet]

Hi Rajendra,

On Wed, Jun 20, 2012 at 10:57 AM, Rajendra Nayak <rnayak@ti.com> wrote:
> Jean,
>
>
> On Wednesday 20 June 2012 02:22 PM, Rajendra Nayak wrote:
>>
>> Hi Jean,
>>
>> On Wednesday 20 June 2012 02:16 PM, Rajendra Nayak wrote:
>>>
>>> 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.
>>
>>
>> never mind, I looked at the patch again and then the cpuidle code and
>> figured what you are doing. Makes sense to me now :-)
Ok!

>
>
> How do you like this updated changelog, I just added one more line.
>
>
> "
> For a power domain to idle all the clock domains in it must idle.
> Denying just *one* clkdm in a pwrdm from idling should have the
> same effect as denying *all*.
>
> 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.
> "
That looks great!

Kevin,
Can you take this change still in your for_3.6/pm/performance branch?

Thanks & regards,
Jean

>
> regards,
> Rajendra
> --
> 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
--
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

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

[Kevin Hilman]

Jean Pihet <jean.pihet@newoldbits.com> writes:

> Hi Rajendra,
>
> On Wed, Jun 20, 2012 at 10:57 AM, Rajendra Nayak <rnayak@ti.com> wrote:
>> Jean,
>>
>>
>> On Wednesday 20 June 2012 02:22 PM, Rajendra Nayak wrote:
>>>
>>> Hi Jean,
>>>
>>> On Wednesday 20 June 2012 02:16 PM, Rajendra Nayak wrote:
>>>>
>>>> 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.
>>>
>>>
>>> never mind, I looked at the patch again and then the cpuidle code and
>>> figured what you are doing. Makes sense to me now :-)
> Ok!
>
>>
>>
>> How do you like this updated changelog, I just added one more line.
>>
>>
>> "
>> For a power domain to idle all the clock domains in it must idle.
>> Denying just *one* clkdm in a pwrdm from idling should have the
>> same effect as denying *all*.
>>
>> 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.
>> "
> That looks great!
>
> Kevin,
> Can you take this change still in your for_3.6/pm/performance branch?
>

Sorry, too late.

Kevin