> -----Original Message----- > From: Kevin Hilman [mailto:khilman@xxxxxxxxxxxxxxxxxxx] > Sent: Thursday, March 12, 2009 5:31 AM > 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: > > >> > >> 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. > > Thanks, some comments below. > > > 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... > > Yeah, I looked at that too, but it currently doesn't have a > concept of valid states, so for now I recommend we implement > that in the OMAP-specific code as you have done. > > > 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--) { > ^^^^^^^^ > > I think you mean idx >= 1 here. [sp] Yes. A typo. Still haven't tried this patch myself. > > Also, while you're working on this, could you fix this up so > the omap3_power_states[] array is zero based insted of > 1-based, it would make this code and the other code walking > this array easier to follow. > > That means defining OMAP_STATE_C1 = 0 and so on. [sp] Definitely. > > > + 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) { > > How about calling this omap3_cpuidle_update_states() and > taking no arguments. Rather than the 'flag' argument, > internally it just checks the global 'enable_off_mode.' > > This allows for potential further expansion down the road of > other reasons to update CPUidle valid states. For example, > we've talked about updating the CPUidle state latencies on > the fly depending on various other chip settings. > > > + 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; > > + } > > +} > > + > > Rather than set set specific OMAP3_STATE_Cx values, it would > be better to just walk the array, and check for > [mpu|core]_state = PWRDM_POWER_OFF. > If the state has either set, then update the valid flag. > > > 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); > > + > > Move the definition into pm.h as > omap3_cpuidle_update_states(void) as described above. > > > 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); > > Then, don't modify pm.c, rather just call > omap3_cpuidle_update_states() from omap3_pm_off_mode_enable(). > > Kevin > > -- 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
