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