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