Re: [PATCHv2] PM : cpuidle - Update statistics for correct state

[Kevin Hilman <khilman@deeprootsystems.com>]

"Premi, Sanjeev" <premi@ti.com> writes:

>  
>
>> -----Original Message-----
>> From: Högander Jouni [mailto:jouni.hogander@nokia.com] 
>> Sent: Monday, March 09, 2009 4:07 PM
>> To: Premi, Sanjeev
>> Cc: linux-omap@vger.kernel.org
>> Subject: Re: [PATCHv2] PM : cpuidle - Update statistics for 
>> correct state
>> 
>> "ext Premi, Sanjeev" <premi@ti.com> writes:
>> 
>> >> -----Original Message-----
>> >> From: Högander Jouni [mailto:jouni.hogander@nokia.com]
>> >> Sent: Monday, March 09, 2009 3:38 PM
>> >> To: Premi, Sanjeev
>> >> Cc: linux-omap@vger.kernel.org
>> >> Subject: Re: [PATCHv2] PM : cpuidle - Update statistics 
>> for correct 
>> >> state
>> >> 
>> >> ext Sanjeev Premi <premi@ti.com> 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@ti.com>
>> >> > ---
>> >> >  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


>
>> >
>> >      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@vger.kernel.org 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@vger.kernel.org
> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html