[PATCH 1/1] PM : cpuidle - update statistics for correct state

[PATCH 1/1] PM : cpuidle - update statistics for correct state

[Sanjeev Premi]

When 'enable_off_mode' is 0, the target power state for MPU
and Core is locally changed to PWRDM_POWER_RET but, the
statistics are updated for idle state originally selected
by the governor.

This patch 'invalidates' the idle states that lead either of
MPU or Core to PWRDM_POWER_OFF state when 'enable_off_mode'
is '0'. The states are valid once 'enable_off_mode' is set
to '1'.

Signed-off-by: Sanjeev Premi <premi@ti.com>
---
 arch/arm/mach-omap2/cpuidle34xx.c |   34 +++++++++++++++++++++++++++-------
 arch/arm/mach-omap2/pm.h          |    2 ++
 arch/arm/mach-omap2/pm34xx.c      |    2 ++
 3 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c
index 7bbec90..19273b3 100644
--- a/arch/arm/mach-omap2/cpuidle34xx.c
+++ b/arch/arm/mach-omap2/cpuidle34xx.c
@@ -104,13 +104,6 @@ static int omap3_enter_idle(struct cpuidle_device *dev,
 	local_irq_disable();
 	local_fiq_disable();
 
-	if (!enable_off_mode) {
-		if (mpu_state < PWRDM_POWER_RET)
-			mpu_state = PWRDM_POWER_RET;
-		if (core_state < PWRDM_POWER_RET)
-			core_state = PWRDM_POWER_RET;
-	}
-
 	pwrdm_set_next_pwrst(mpu_pd, mpu_state);
 	pwrdm_set_next_pwrst(core_pd, core_state);
 
@@ -254,6 +247,31 @@ void omap_init_power_states(void)
 				CPUIDLE_FLAG_CHECK_BM;
 }
 
+/**
+ * omap3_cpuidle_update_states - Update the cpuidle states.
+ *
+ * Currently, this function toggles the validity of idle states based upon
+ * the flag 'enable_off_mode'. When the flag is set all states are valid.
+ * Else, states leading to OFF state set to be invalid.
+ */
+void omap3_cpuidle_update_states(void)
+{
+	int i;
+
+	for (i = OMAP3_STATE_C1; i < OMAP3_MAX_STATES; i++) {
+		if (enable_off_mode) {
+			omap3_power_states[i].valid = 1;
+		}
+		else {
+			if ((omap3_power_states[i].mpu_state
+					== PWRDM_POWER_OFF)
+				|| (omap3_power_states[i].core_state
+					== PWRDM_POWER_RET))
+			omap3_power_states[i].valid = 0;
+		}
+	}
+}
+
 struct cpuidle_driver omap3_idle_driver = {
 	.name = 	"omap3_idle",
 	.owner = 	THIS_MODULE,
@@ -303,6 +321,8 @@ int omap3_idle_init(void)
 		return -EINVAL;
 	dev->state_count = count;
 
+	omap3_cpuidle_update_states();
+
 	if (cpuidle_register_device(dev)) {
 		printk(KERN_ERR "%s: CPUidle register device failed\n",
 		       __func__);
diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
index 498f55d..d18fe49 100644
--- a/arch/arm/mach-omap2/pm.h
+++ b/arch/arm/mach-omap2/pm.h
@@ -86,6 +86,8 @@ extern void omap34xx_cpu_suspend(u32 *addr, int save_state);
 extern void save_secure_ram_context(u32 *addr);
 extern void omap3_save_scratchpad_contents(void);
 
+extern void omap3_cpuidle_update_states(void);
+
 extern unsigned int omap24xx_idle_loop_suspend_sz;
 extern unsigned int omap34xx_suspend_sz;
 extern unsigned int save_secure_ram_context_sz;
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 9ce651a..6ebb4d1 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -968,6 +968,8 @@ void omap3_pm_off_mode_enable(int enable)
 	else
 		state = PWRDM_POWER_RET;
 
+	omap3_cpuidle_update_states();
+
 #ifdef CONFIG_OMAP_PM_SRF
 	resource_lock_opp(VDD1_OPP);
 	resource_lock_opp(VDD2_OPP);
-- 
1.6.2.2

Re: [PATCH 1/1] PM : cpuidle - update statistics for correct state

[Kevin Hilman]

Sanjeev Premi <premi@ti.com> writes:

> When 'enable_off_mode' is 0, the target power state for MPU
> and Core is locally changed to PWRDM_POWER_RET but, the
> statistics are updated for idle state originally selected
> by the governor.
>
> This patch 'invalidates' the idle states that lead either of
> MPU or Core to PWRDM_POWER_OFF state when 'enable_off_mode'
> is '0'. The states are valid once 'enable_off_mode' is set
> to '1'.
>
> Signed-off-by: Sanjeev Premi <premi@ti.com>

This is a good start, but doesn't actually fix the problem.  This is
because the 'valid' field is an OMAP specific field and is not checked
in any of our  'enter_idle' hooks.

It works in your test case because the code snippet you mentioned in
PATCH 0/0 still modifies the target state.

What we need is for the enter_idle_bm() enter function to check the
.valid flag.  If it's not valid, then keep dropping states until it
finds a valid flag or it hits the safe state.

Kevin


> ---
>  arch/arm/mach-omap2/cpuidle34xx.c |   34 +++++++++++++++++++++++++++-------
>  arch/arm/mach-omap2/pm.h          |    2 ++
>  arch/arm/mach-omap2/pm34xx.c      |    2 ++
>  3 files changed, 31 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c
> index 7bbec90..19273b3 100644
> --- a/arch/arm/mach-omap2/cpuidle34xx.c
> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> @@ -104,13 +104,6 @@ static int omap3_enter_idle(struct cpuidle_device *dev,
>  	local_irq_disable();
>  	local_fiq_disable();
>  
> -	if (!enable_off_mode) {
> -		if (mpu_state < PWRDM_POWER_RET)
> -			mpu_state = PWRDM_POWER_RET;
> -		if (core_state < PWRDM_POWER_RET)
> -			core_state = PWRDM_POWER_RET;
> -	}
> -
>  	pwrdm_set_next_pwrst(mpu_pd, mpu_state);
>  	pwrdm_set_next_pwrst(core_pd, core_state);
>  
> @@ -254,6 +247,31 @@ void omap_init_power_states(void)
>  				CPUIDLE_FLAG_CHECK_BM;
>  }
>  
> +/**
> + * omap3_cpuidle_update_states - Update the cpuidle states.
> + *
> + * Currently, this function toggles the validity of idle states based upon
> + * the flag 'enable_off_mode'. When the flag is set all states are valid.
> + * Else, states leading to OFF state set to be invalid.
> + */
> +void omap3_cpuidle_update_states(void)
> +{
> +	int i;
> +
> +	for (i = OMAP3_STATE_C1; i < OMAP3_MAX_STATES; i++) {
> +		if (enable_off_mode) {
> +			omap3_power_states[i].valid = 1;
> +		}
> +		else {
> +			if ((omap3_power_states[i].mpu_state
> +					== PWRDM_POWER_OFF)
> +				|| (omap3_power_states[i].core_state
> +					== PWRDM_POWER_RET))
> +			omap3_power_states[i].valid = 0;
> +		}
> +	}
> +}
> +
>  struct cpuidle_driver omap3_idle_driver = {
>  	.name = 	"omap3_idle",
>  	.owner = 	THIS_MODULE,
> @@ -303,6 +321,8 @@ int omap3_idle_init(void)
>  		return -EINVAL;
>  	dev->state_count = count;
>  
> +	omap3_cpuidle_update_states();
> +
>  	if (cpuidle_register_device(dev)) {
>  		printk(KERN_ERR "%s: CPUidle register device failed\n",
>  		       __func__);
> diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
> index 498f55d..d18fe49 100644
> --- a/arch/arm/mach-omap2/pm.h
> +++ b/arch/arm/mach-omap2/pm.h
> @@ -86,6 +86,8 @@ extern void omap34xx_cpu_suspend(u32 *addr, int save_state);
>  extern void save_secure_ram_context(u32 *addr);
>  extern void omap3_save_scratchpad_contents(void);
>  
> +extern void omap3_cpuidle_update_states(void);
> +
>  extern unsigned int omap24xx_idle_loop_suspend_sz;
>  extern unsigned int omap34xx_suspend_sz;
>  extern unsigned int save_secure_ram_context_sz;
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index 9ce651a..6ebb4d1 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -968,6 +968,8 @@ void omap3_pm_off_mode_enable(int enable)
>  	else
>  		state = PWRDM_POWER_RET;
>  
> +	omap3_cpuidle_update_states();
> +
>  #ifdef CONFIG_OMAP_PM_SRF
>  	resource_lock_opp(VDD1_OPP);
>  	resource_lock_opp(VDD2_OPP);
> -- 
> 1.6.2.2
>
> --
> 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 1/1] PM : cpuidle - update statistics for correct state

[Premi, Sanjeev]

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com] 
> Sent: Thursday, September 10, 2009 11:51 PM
> To: Premi, Sanjeev
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [PATCH 1/1] PM : cpuidle - update statistics for 
> correct state
> 
> Sanjeev Premi <premi@ti.com> writes:
> 
> > When 'enable_off_mode' is 0, the target power state for MPU
> > and Core is locally changed to PWRDM_POWER_RET but, the
> > statistics are updated for idle state originally selected
> > by the governor.
> >
> > This patch 'invalidates' the idle states that lead either of
> > MPU or Core to PWRDM_POWER_OFF state when 'enable_off_mode'
> > is '0'. The states are valid once 'enable_off_mode' is set
> > to '1'.
> >
> > Signed-off-by: Sanjeev Premi <premi@ti.com>
> 
> This is a good start, but doesn't actually fix the problem.  This is
> because the 'valid' field is an OMAP specific field and is not checked
> in any of our  'enter_idle' hooks.
> 
> It works in your test case because the code snippet you mentioned in
> PATCH 0/0 still modifies the target state.
> 
> What we need is for the enter_idle_bm() enter function to check the
> .valid flag.  If it's not valid, then keep dropping states until it
> finds a valid flag or it hits the safe state.

We do have a omap3_enter_idle_bm(), but the problem is that calculating
the current index in dev->states will be costly operation. We know the
pointer to 'target' c-state; but the translating the index back to the
omap3_power_states[] seems 'intense' operation in idle_bm check.

I believe the right solution will be to add .valid in the cpuidle framework
itself. I am submitting a patch for same.

Best regards,
Sanjeev

> 
> Kevin
> 
> 
[snip]---[snip]---[snip]

Re: [PATCH 1/1] PM : cpuidle - update statistics for correct state

[Kevin Hilman]

"Premi, Sanjeev" <premi@ti.com> writes:

>> -----Original Message-----
>> From: Kevin Hilman [mailto:khilman@deeprootsystems.com] 
>> Sent: Thursday, September 10, 2009 11:51 PM
>> To: Premi, Sanjeev
>> Cc: linux-omap@vger.kernel.org
>> Subject: Re: [PATCH 1/1] PM : cpuidle - update statistics for 
>> correct state
>> 
>> Sanjeev Premi <premi@ti.com> writes:
>> 
>> > When 'enable_off_mode' is 0, the target power state for MPU
>> > and Core is locally changed to PWRDM_POWER_RET but, the
>> > statistics are updated for idle state originally selected
>> > by the governor.
>> >
>> > This patch 'invalidates' the idle states that lead either of
>> > MPU or Core to PWRDM_POWER_OFF state when 'enable_off_mode'
>> > is '0'. The states are valid once 'enable_off_mode' is set
>> > to '1'.
>> >
>> > Signed-off-by: Sanjeev Premi <premi@ti.com>
>> 
>> This is a good start, but doesn't actually fix the problem.  This is
>> because the 'valid' field is an OMAP specific field and is not checked
>> in any of our  'enter_idle' hooks.
>> 
>> It works in your test case because the code snippet you mentioned in
>> PATCH 0/0 still modifies the target state.
>> 
>> What we need is for the enter_idle_bm() enter function to check the
>> .valid flag.  If it's not valid, then keep dropping states until it
>> finds a valid flag or it hits the safe state.
>
> We do have a omap3_enter_idle_bm(), but the problem is that calculating
> the current index in dev->states will be costly operation. We know the
> pointer to 'target' c-state; but the translating the index back to the
> omap3_power_states[] seems 'intense' operation in idle_bm check.

Maybe this array should be converted to a list.

> I believe the right solution will be to add .valid in the cpuidle framework
> itself. I am submitting a patch for same.

I like this idea better.

Kevin