Re: [PATCH 1/1] PM : cpuidle - update statistics for correct state

[Kevin Hilman <khilman@deeprootsystems.com>]

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

>> -----Original Message-----
>> From: Kevin Hilman [mailto:khilman@deeprootsystems.com] 
>> Sent: Thursday, September 10, 2009 11:51 PM
>> To: Premi, Sanjeev
>> Cc: linux-omap@vger.kernel.org
>> Subject: Re: [PATCH 1/1] PM : cpuidle - update statistics for 
>> correct state
>> 
>> Sanjeev Premi <premi@ti.com> writes:
>> 
>> > When 'enable_off_mode' is 0, the target power state for MPU
>> > and Core is 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'.
>> >
>> > Signed-off-by: Sanjeev Premi <premi@ti.com>
>> 
>> This is a good start, but doesn't actually fix the problem.  This is
>> because the 'valid' field is an OMAP specific field and is not checked
>> in any of our  'enter_idle' hooks.
>> 
>> It works in your test case because the code snippet you mentioned in
>> PATCH 0/0 still modifies the target state.
>> 
>> What we need is for the enter_idle_bm() enter function to check the
>> .valid flag.  If it's not valid, then keep dropping states until it
>> finds a valid flag or it hits the safe state.
>
> We do have a omap3_enter_idle_bm(), but the problem is that calculating
> the current index in dev->states will be costly operation. We know the
> pointer to 'target' c-state; but the translating the index back to the
> omap3_power_states[] seems 'intense' operation in idle_bm check.

Maybe this array should be converted to a list.

> I believe the right solution will be to add .valid in the cpuidle framework
> itself. I am submitting a patch for same.

I like this idea better.

Kevin