> -----Original Message-----
> From: Kevin Hilman [mailto:[email protected]]
> Sent: Tuesday, January 26, 2010 1:25 AM
> To: Premi, Sanjeev
> Cc: [email protected]
> Subject: Re: [PATCHv2 1/1] omap3: cpuidle: Update statistics
> for correct state
>
> Sanjeev Premi <[email protected]> writes:
>
> > When 'enable_off_mode' is 0, the target power state for MPU
> > and CORE was locally changed to PWRDM_POWER_RET but, the
> > statistics are updated for idle state originally selected
> > by the governor.
> >
> > This patch 'invalidates' the idle states that lead either of
> > MPU or Core to PWRDM_POWER_OFF state when 'enable_off_mode'
> > is '0'. The states are valid once 'enable_off_mode' is set
> > to '1'.
> >
> > Added function next_valid_state() to check if current state
> > is valid; else get the next valid state. It is called from
> > omap3_enter_idle_bm().
> >
> > Signed-off-by: Sanjeev Premi <[email protected]>
>
> Hi Sanjeev,
>
> I quite like this approach, but have a few minor comments. First,
> please fix the checkpatch errors and warnings, as well as a few minor
> things below...
>
> > ---
> > arch/arm/mach-omap2/cpuidle34xx.c | 98
> ++++++++++++++++++++++++++++++++++---
> > arch/arm/mach-omap2/pm.h | 1 +
> > arch/arm/mach-omap2/pm34xx.c | 4 ++
> > 3 files changed, 95 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/arm/mach-omap2/cpuidle34xx.c
> b/arch/arm/mach-omap2/cpuidle34xx.c
> > index fdfa1d5..308865c 100644
> > --- a/arch/arm/mach-omap2/cpuidle34xx.c
> > +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> > @@ -45,6 +45,8 @@
> > #define OMAP3_STATE_C6 5 /* C6 - MPU OFF + Core RET */
> > #define OMAP3_STATE_C7 6 /* C7 - MPU OFF + Core OFF */
> >
> > +#define OMAP3_STATE_MAX OMAP3_STATE_C7
> > +
> > struct omap3_processor_cx {
> > u8 valid;
> > u8 type;
> > @@ -128,13 +130,6 @@ static int omap3_enter_idle(struct
> cpuidle_device *dev,
> > local_irq_disable();
> > local_fiq_disable();
> >
> > - 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;
> > - }
> > -
> > pwrdm_set_next_pwrst(mpu_pd, mpu_state);
> > pwrdm_set_next_pwrst(core_pd, core_state);
> >
> > @@ -164,6 +159,65 @@ return_sleep_time:
> > return (u32)timespec_to_ns(&ts_idle)/1000;
> > }
> >
> > +
> > +/* next_valid_state - Find next valid c-state
> > + * @dev: cpuidle device
> > + * @state: Currently selected c-state
> > + *
> > + * If the current state is valid, it is returned back to
> the caller.
> > + * Else, this function searches for a lower c-state which is still
> > + * valid (as defined in omap3_power_states[]).
> > + */
>
> Please fix kerneldoc style. Opening comment should be '/**'
>
> > +static struct cpuidle_state* next_valid_state(struct
> cpuidle_device *dev,
> > + struct
> cpuidle_state *curr)
> > +{
> > + struct cpuidle_state *next = NULL;
> > + struct omap3_processor_cx *cx;
> > +
> > + cx = (struct omap3_processor_cx *)cpuidle_get_statedata(curr);
> > +
> > + /* Check if current state is valid */
> > + if (cx->valid) {
> > + next = curr;
>
> could just return curr here
>
> > + }
> > + else {
> > + u8 idx = OMAP3_STATE_MAX;
> > +
> > + /* Reach the current state starting at highest
> C-state */
> > + for (; idx >= OMAP3_STATE_C1; idx--) {
> > + if (&dev->states[idx] == curr) {
> > + next = &dev->states[idx];
> > + break;
> > + }
> > + }
> > +
> > + /*
> > + * Should never hit this condition.
> > + */
> > + BUG_ON(next == NULL);
>
> This shouldn't trigger a panic. Please fail more cleanly
> using WARN_ON()
>
> > + /* Drop to next valid state.
> > + * Start search from the next (lower) state.
> > + */
>
> fix multiline comment. Should start with '/*' on its own line.
> (c.f. Documentation/CodingStyle, search for 'multi-line')
>
> > + idx--;
> > + for (; idx >= OMAP3_STATE_C1; idx--) {
> > + if (((struct omap3_processor_cx *)
> > + cpuidle_get_statedata(
> > + &dev->states[idx]))->valid) {
> > + next = &dev->states[idx];
> > + break;
> > + }
> > + }
>
> Due to line-wrapping etc., this has readabiliyt problems.
> Consider this
> instead:
>
> for (; idx >= OMAP3_STATE_C1; idx--) {
> struct omap3_processor_cx *cx;
>
> cx = cpuidle_get_statedata(&dev->states[idx]);
> if (cx->valid) {
> next = &dev->states[idx];
> break;
> }
> }
>
> > + /*
> > + * C1 and C2 are always valid.
> > + * So, no need to check for 'next==NULL'
> outside this loop.
> > + */
> > + }
> > +
> > + return next;
> > +}
> > +
> > +
> > /**
> > * omap3_enter_idle_bm - Checks for any bus activity
> > * @dev: cpuidle device
> > @@ -176,7 +230,7 @@ return_sleep_time:
> > static int omap3_enter_idle_bm(struct cpuidle_device *dev,
> > struct cpuidle_state *state)
> > {
> > - struct cpuidle_state *new_state = state;
> > + struct cpuidle_state *new_state= next_valid_state(dev, state);
> ^
>
> missing space
>
> >
> > if ((state->flags & CPUIDLE_FLAG_CHECK_BM) &&
> omap3_idle_bm_check()) {
> > BUG_ON(!dev->safe_state);
> > @@ -207,6 +261,32 @@ void omap3_pm_init_cpuidle(struct
> cpuidle_params *cpuidle_board_params)
> > return;
> > }
> >
> > +/**
> > + * omap3_cpuidle_update_states - Update the cpuidle states.
> > + *
> > + * Currently, this function toggles the validity of idle
> states based upon
> > + * the flag 'enable_off_mode'. When the flag is set all
> states are valid.
> > + * Else, states leading to OFF state set to be invalid.
> > + */
> > +void omap3_cpuidle_update_states(void)
> > +{
> > + int i;
> > +
> > + for (i = OMAP3_STATE_C1; i < OMAP3_MAX_STATES; i++) {
> > + if (enable_off_mode) {
> > + omap3_power_states[i].valid = 1;
> > + }
> > + else {
> > + if ((omap3_power_states[i].mpu_state
> > + == PWRDM_POWER_OFF)
> > + || (omap3_power_states[i].core_state
> > + == PWRDM_POWER_OFF))
> > + omap3_power_states[i].valid = 0;
> > + }
> > + }
> > +}
>
> more readability problems due to wrapping. consider:
>
> void omap3_cpuidle_update_states(void)
> {
> int i;
>
> for (i = OMAP3_STATE_C1; i < OMAP3_MAX_STATES; i++) {
> struct omap3_processor_cx *cx = &omap3_power_states[i]
>
> if (enable_off_mode)
> cx->valid = 1;
> else
> if ((cx->mpu_state == PWRDM_POWER_OFF) ||
> (cx->core_state == PWRDM_POWER_OFF))
> cx->valid = 0;
> }
> }
>
> Kevin
Thanks for comments Kevin.
Updated just started to test the updated patch. Will be posting soon.
Best regards,
Sanjeev
>
>
> > +
> > /* omap3_init_power_states - Initialises the OMAP3
> specific C states.
> > *
> > * Below is the desciption of each C state.
> > @@ -365,6 +445,8 @@ int __init omap3_idle_init(void)
> > return -EINVAL;
> > dev->state_count = count;
> >
> > + omap3_cpuidle_update_states();
> > +
> > if (cpuidle_register_device(dev)) {
> > printk(KERN_ERR "%s: CPUidle register device failed\n",
> > __func__);
> > diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
> > index b9421e8..9351490 100644
> > --- a/arch/arm/mach-omap2/pm.h
> > +++ b/arch/arm/mach-omap2/pm.h
> > @@ -51,6 +51,7 @@ struct cpuidle_params {
> > extern void omap3_pm_init_vc(struct prm_setup_vc *setup_vc);
> > #ifdef CONFIG_CPU_IDLE
> > extern void omap3_pm_init_cpuidle(struct cpuidle_params
> *cpuidle_board_params);
> > +extern void omap3_cpuidle_update_states(void);
> > #else
> > static inline void omap3_pm_init_cpuidle(
> > struct cpuidle_params *cpuidle_board_params)
> > diff --git a/arch/arm/mach-omap2/pm34xx.c
> b/arch/arm/mach-omap2/pm34xx.c
> > index 90c4949..d9f0626 100644
> > --- a/arch/arm/mach-omap2/pm34xx.c
> > +++ b/arch/arm/mach-omap2/pm34xx.c
> > @@ -981,6 +981,10 @@ void omap3_pm_off_mode_enable(int enable)
> > else
> > state = PWRDM_POWER_RET;
> >
> > +#ifdef CONFIG_CPU_IDLE
> > + omap3_cpuidle_update_states();
> > +#endif
> > +
> > #ifdef CONFIG_OMAP_PM_SRF
> > resource_lock_opp(VDD1_OPP);
> > resource_lock_opp(VDD2_OPP);
> > --
> > 1.6.2.2
> >
> > --
> > 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
