| Message ID | B85A65D85D7EB246BE421B3FB0FBB59301CC1F6C1F@dbde02.ent.ti.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kevin Hilman |
| Headers | show |
"Premi, Sanjeev" <premi@ti.com> 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. 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. > + 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Kevin Hilman [mailto:khilman@deeprootsystems.com] > Sent: Thursday, March 12, 2009 5:31 AM > 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: > > >> > >> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)