RE: [PATCHv2] PM : cpuidle - Update statistics for correct state

["Premi, Sanjeev" <premi@ti.com>]

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com] 
> Sent: Monday, March 09, 2009 11:31 PM
> To: Premi, Sanjeev
> Cc: Högander Jouni; linux-omap@vger.kernel.org
> Subject: Re: [PATCHv2] PM : cpuidle - Update statistics for 
> correct state
> 
> "Premi, Sanjeev" <premi@ti.com> writes:
> 
> >  
> >
> >> -----Original Message-----
> >> From: Högander Jouni [mailto:jouni.hogander@nokia.com]
> >> Sent: Monday, March 09, 2009 4:07 PM
> >> To: Premi, Sanjeev
> >> Cc: linux-omap@vger.kernel.org
> >> Subject: Re: [PATCHv2] PM : cpuidle - Update statistics 
> for correct 
> >> state
> >> 
> >> "ext Premi, Sanjeev" <premi@ti.com> writes:
> >> 
> >> >> -----Original Message-----
> >> >> From: Högander Jouni [mailto:jouni.hogander@nokia.com]
> >> >> Sent: Monday, March 09, 2009 3:38 PM
> >> >> To: Premi, Sanjeev
> >> >> Cc: linux-omap@vger.kernel.org
> >> >> Subject: Re: [PATCHv2] PM : cpuidle - Update statistics
> >> for correct
> >> >> state
> >> >> 
> >> >> ext Sanjeev Premi <premi@ti.com> writes:
> >> >> 
> >> >> > When 'enable_off_mode' is 0, and (mpu_state <
> >> PWRDM_POWER_RET) the
> >> >> > local variables mpu_state and core_state are modified; but
> >> >> the usage
> >> >> > count for the original state selected by the governor
> >> are updated.
> >> >> >
> >> >> > This patch updates the 'last_state' in the cpuidle
> >> driver to ensure
> >> >> > that statistics for the correct state are updated.
> >> >> >
> >> >> > Signed-off-by: Sanjeev Premi <premi@ti.com>
> >> >> > ---
> >> >> >  arch/arm/mach-omap2/cpuidle34xx.c |   29 
> >> >> +++++++++++++++++++----------
> >> >> >  1 files changed, 19 insertions(+), 10 deletions(-)
> >> >> >
> >> >> > diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
> >> >> > b/arch/arm/mach-omap2/cpuidle34xx.c
> >> >> > index 62fbb2e..b138abd 100644
> >> >> > --- a/arch/arm/mach-omap2/cpuidle34xx.c
> >> >> > +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> >> >> > @@ -76,23 +76,32 @@ static int omap3_enter_idle(struct
> >> >> cpuidle_device
> >> >> > *dev,  {
> >> >> >  	struct omap3_processor_cx *cx =
> >> cpuidle_get_statedata(state);
> >> >> >  	struct timespec ts_preidle, ts_postidle, ts_idle;
> >> >> > -	u32 mpu_state = cx->mpu_state, core_state = 
> >> cx->core_state;
> >> >> > -
> >> >> > -	current_cx_state = *cx;
> >> >> > +	u32 mpu_state, core_state;
> >> >> >  
> >> >> >  	/* Used to keep track of the total time in idle */
> >> >> >  	getnstimeofday(&ts_preidle);
> >> >> >  
> >> >> > -	local_irq_disable();
> >> >> > -	local_fiq_disable();
> >> >> > -
> >> >> > +	/*
> >> >> > +	 * Adjust the idle state (if required).
> >> >> > +	 * Also, ensure that usage statistics of correct state
> >> >> are updated.
> >> >> > +	 */
> >> >> >  	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;
> >> >> > +		if (cx->type > OMAP3_STATE_C4) {
> >> >> > +			state =
> >> &(dev->states[OMAP3_STATE_C4 - 1]);
> >> >> > +			dev->last_state = state ;
> >> >> > +
> >> >> > +			cx = cpuidle_get_statedata(state);
> >> >> 
> >> >> There is still C3 where OFF is used for MPU. This needs to
> >> be taken
> >> >> into account.
> >> >
> >> > [sp] Thanks. Good catch!
> >> >      I wasn't happy doing the "OMAP3_STATEn - 1"; but could
> >> not find a better way.
> >> >      It should be C2 as defined now.
> >> 
> >> This means C4 is not used if off mode is not enabled? I 
> think this is 
> >> not wanted. Would it be possible to remove "OFF" C states when 
> >> enable_off_mode is written to 0 and add them back when 1 written?
> >
> > [sp] That should be possible. We could use the 'valid' field
> >      for the purpose.
> 
> I would gladly take a patch which uses the 'valid' field for 
> this and then the enter hook whould drop to the next lower 
> valid state if the state requested is not valid.
> 
> Kevin

[sp] I have not yet tested it (working offline for sometime).
     But, wanted to share the changes for an early review.

     Initially, I was trying see if the CPUIDLE framework could
     use ".valid" as an additional argument in cpuidle_state.
     But, may be that's for later...

diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx
index c25158c..9493553 100644
--- a/arch/arm/mach-omap2/cpuidle34xx.c
+++ b/arch/arm/mach-omap2/cpuidle34xx.c
@@ -88,16 +88,19 @@ static int omap3_enter_idle(struct cpuidle_device *dev,
                goto return_sleep_time;

        /*
+        * Check if the chosen idle state is valid.
+        * If no, drop down to a lower valid state. Expects the lowest
+        * state will always be active.
         */
+       if (!cx->valid) {
+               for (idx = (cx->type - 1); idx == 1; idx--) {
+                       if (omap3_power_states[idx].valid)
+                               break;
                }
+               state = &(dev->states[idx]);
+               dev->last_state = state ;
+
+               cx = cpuidle_get_statedata(state);
        }

        current_cx_state = *cx;
@@ -150,6 +153,26 @@ static int omap3_enter_idle_bm(struct cpuidle_device *dev,
        return omap3_enter_idle(dev, new_state);
 }

+/**
+ * omap3_toggle_off_states - Enable / Disable validity of idle states
+ * @flag: Enable/ Disable support for OFF mode
+ *
+ * Called as result of change to "enable_off_mode".
+ */
+void omap3_toggle_off_states(unsigned short flag)
+{
+       if (flag) {
+               omap3_power_states[OMAP3_STATE_C3].valid = 1;
+               omap3_power_states[OMAP3_STATE_C5].valid = 1;
+               omap3_power_states[OMAP3_STATE_C6].valid = 1;
+       }
+       else {
+               omap3_power_states[OMAP3_STATE_C3].valid = 0;
+               omap3_power_states[OMAP3_STATE_C5].valid = 0;
+               omap3_power_states[OMAP3_STATE_C6].valid = 0;
+       }
+}
+
 DEFINE_PER_CPU(struct cpuidle_device, omap3_idle_dev);

 /* omap3_init_power_states - Initialises the OMAP3 specific C states.
diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
index 61c6dfb..6785850 100644
--- a/arch/arm/mach-omap2/pm.c
+++ b/arch/arm/mach-omap2/pm.c
@@ -47,6 +47,8 @@ atomic_t sleep_block = ATOMIC_INIT(0);
 static int vdd1_locked;
 static int vdd2_locked;

+extern void omap3_toggle_off_states(unsigned short);
+
 static ssize_t idle_show(struct kobject *, struct kobj_attribute *, char *);
 static ssize_t idle_store(struct kobject *k, struct kobj_attribute *,
                          const char *buf, size_t n);
@@ -112,6 +114,8 @@ static ssize_t idle_store(struct kobject *kobj, struct kobj_
        } else if (attr == &enable_off_mode_attr) {
                enable_off_mode = value;
                omap3_pm_off_mode_enable(enable_off_mode);
+               if (cpu_is_omap34xx())
+                       omap3_toggle_off_states(value);
        } else if (attr == &voltage_off_while_idle_attr) {
                voltage_off_while_idle = value;
                if (voltage_off_while_idle)

> 
> 
> >
> >> >
> >> >      On another note, would it make sense to swap the
> >> definitions for C3 and C4.
> >> >      C3 : MPU CSWR + CORE CSWR
> >> >      C4 : MPU OFF + CORE Actove
> >> 
> >> No it doesn't. They are organized by latency.
> >
> > [sp] Okay. That was a loud thinking from my side :)
> >> 
> >> One grounding for current implementation is that 
> enable_off_mode is 
> >> more or less testing interface. In final solution it might be even 
> >> removed. Adjusting states directly still shows guite accurate 
> >> information on used C-states.
> >> 
> >> >
> >> >> 
> >> >> > +		}
> >> >> >  	}
> >> >> >  
> >> >> > +	current_cx_state = *cx;
> >> >> > +
> >> >> > +	mpu_state = cx->mpu_state;
> >> >> > +	core_state = cx->core_state;
> >> >> > +
> >> >> > +	local_irq_disable();
> >> >> > +	local_fiq_disable();
> >> >> > +
> >> >> >  	pwrdm_set_next_pwrst(mpu_pd, mpu_state);
> >> >> >  	pwrdm_set_next_pwrst(core_pd, core_state);
> >> >> >  
> >> >> > --
> >> >> > 1.5.6
> >> >> >
> >> >> > --
> >> >> > 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
> >> >> 
> >> >> --
> >> >> Jouni Högander
> >> >> 
> >> >> 
> >> 
> >> --
> >> Jouni Högander
> >> 
> >> --
> > 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