> -----Original Message-----
> From: Kevin Hilman [mailto:[email protected]]
> Sent: Monday, March 09, 2009 11:31 PM
> To: Premi, Sanjeev
> Cc: Högander Jouni; [email protected]
> Subject: Re: [PATCHv2] PM : cpuidle - Update statistics for
> correct state
>
> "Premi, Sanjeev" <[email protected]> writes:
>
> >
> >
> >> -----Original Message-----
> >> From: Högander Jouni [mailto:[email protected]]
> >> Sent: Monday, March 09, 2009 4:07 PM
> >> To: Premi, Sanjeev
> >> Cc: [email protected]
> >> Subject: Re: [PATCHv2] PM : cpuidle - Update statistics
> for correct
> >> state
> >>
> >> "ext Premi, Sanjeev" <[email protected]> writes:
> >>
> >> >> -----Original Message-----
> >> >> From: Högander Jouni [mailto:[email protected]]
> >> >> Sent: Monday, March 09, 2009 3:38 PM
> >> >> To: Premi, Sanjeev
> >> >> Cc: [email protected]
> >> >> Subject: Re: [PATCHv2] PM : cpuidle - Update statistics
> >> for correct
> >> >> state
> >> >>
> >> >> ext Sanjeev Premi <[email protected]> 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 <[email protected]>
> >> >> > ---
> >> >> > 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 [email protected] 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 [email protected] 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
