[RFC PATCH v4 2/4] ARM: omap: Remove cpuidle timekeeping and irq enable/disable

[Rob Lee]

On Thu, Feb 2, 2012 at 10:21 AM, Jean Pihet <jean.pihet at newoldbits.com> wrote:
> Rob,
>
> On Wed, Feb 1, 2012 at 4:00 AM, Robert Lee <rob.lee at linaro.org> wrote:
>> Now that the core cpuidle driver keeps time and handles irq enabling,
>> remove this functionality.  Also, remove irq disabling as all paths to
>> cpuidle_idle_call already call local_irq_disable.  Also, restructure
>> idle functions as needed by the cpuidle core driver changes.
>>
>> Signed-off-by: Robert Lee <rob.lee at linaro.org>
>> ---
>>  arch/arm/mach-omap2/cpuidle34xx.c |   96 ++++++++++++++++--------------------
>>  1 files changed, 43 insertions(+), 53 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c
>> index 464cffd..9ecded5 100644
>> --- a/arch/arm/mach-omap2/cpuidle34xx.c
>> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
> ...
>
>> @@ -100,23 +107,20 @@ static int omap3_enter_idle(struct cpuidle_device *dev,
>>                                struct cpuidle_driver *drv,
>>                                int index)
>>  {
>> -       struct omap3_idle_statedata *cx =
>> -                       cpuidle_get_statedata(&dev->states_usage[index]);
>> -       struct timespec ts_preidle, ts_postidle, ts_idle;
>> -       u32 mpu_state = cx->mpu_state, core_state = cx->core_state;
>> -       int idle_time;
>> +       struct omap3_idle_drvdata *dd = dev->states_usage[index].driver_data
> A build error is triggered by the missing ";".

Argh, last minute change and I didn't build afterward.

>
> ...
>
>> @@ -147,18 +151,12 @@ static int omap3_enter_idle(struct cpuidle_device *dev,
>>                pwrdm_for_each_clkdm(core_pd, _cpuidle_allow_idle);
>>        }
>>
>> -return_sleep_time:
>> -       getnstimeofday(&ts_postidle);
>> -       ts_idle = timespec_sub(ts_postidle, ts_preidle);
>> -
>> -       local_irq_enable();
>> +leave:
>>        local_fiq_enable();
>>
>> -       idle_time = ts_idle.tv_nsec / NSEC_PER_USEC + ts_idle.tv_sec * \
>> -                                                               USEC_PER_SEC;
>> -
>> -       /* Update cpuidle counters */
>> -       dev->last_residency = idle_time;
>> +       /* Restore original PER state if it was modified */
>> +       if (dd->per_next_state != dd->per_saved_state)
>> +               pwrdm_set_next_pwrst(per_pd, dd->per_saved_state);
> This code is not necessarily balanced with the PER state change in
> omap3_enter_idle_bm (cf. "/* Are we changing PER target state? */"
> here below), since in the core cpuidle code there is no guarantee that
> the calls to pre_enter and enter callbacks are balanced.
> In general I fear that splitting the code in two functions introduces
> a risk of programming non-coherent settings in the PRCM.

Agree, in general it does introduce that new risk.  For the platform
code changes, I tried to keep the code paths the same as before.  I
see now where I created a problem though:

...
	if (target_state->pre_enter) {
		idx = target_state->
			pre_enter(dev, drv, idx);
	}

	if (idx < 0) {
		local_irq_enable();
		return idx;
	}

	if (need_resched()) {
		local_irq_enable();
		return -EBUSY;
	}

...

The only way the core cpuidle pre_enter can get called without enter
without enter is if the omap3 next_valid_state() returned a negative
value.  If this ever happened in the existing omap3 code, it would
cause it to break anyway.  But, this particular need_resched() call
could cause an exit that results in difference behavior than before.
One solution to that is just to move the need_resched check before the
pre_enter call.

>
> ...
>
>> @@ -255,8 +255,9 @@ static int omap3_enter_idle_bm(struct cpuidle_device *dev,
>>                               int index)
>>  {
>>        int new_state_idx;
>> -       u32 core_next_state, per_next_state = 0, per_saved_state = 0, cam_state;
>> -       struct omap3_idle_statedata *cx;
>> +       u32 core_next_state, cam_state;
>> +       struct omap3_idle_drvdata *dd = dev->states_usage[index].driver_data;
>> +       struct omap3_idle_statedata *cx = &dd->state_data[index];
>>        int ret;
> The build throws a few warnings about unused variables:
>
> arch/arm/mach-omap2/cpuidle34xx.c: In function 'omap3_enter_idle_bm':
> arch/arm/mach-omap2/cpuidle34xx.c:261: warning: unused variable 'ret'
> arch/arm/mach-omap2/cpuidle34xx.c:257: warning: unused variable 'new_state_idx'
>
> ...
>

Got it, not sure why I missed this.

>>
>>  DEFINE_PER_CPU(struct cpuidle_device, omap3_idle_dev);
>> @@ -337,7 +329,8 @@ static inline void _fill_cstate(struct cpuidle_driver *drv,
>>        state->exit_latency     = cpuidle_params_table[idx].exit_latency;
>>        state->target_residency = cpuidle_params_table[idx].target_residency;
>>        state->flags            = CPUIDLE_FLAG_TIME_VALID;
>> -       state->enter            = omap3_enter_idle_bm;
>> +       state->pre_enter        = omap3_enter_idle_bm;
>> +       state->enter            = omap3_enter_idle;
>>        sprintf(state->name, "C%d", idx + 1);
>>        strncpy(state->desc, descr, CPUIDLE_DESC_LEN);
>>
> ...
>
> Also the line at 373 is not needed anymore in omap3_idle_init, since
> the enter callback is filled in in the _fill_cstate function:
>        /* C1 . MPU WFI + Core active */
>        _fill_cstate(drv, 0, "MPU ON + CORE ON");
>        (&drv->states[0])->enter = omap3_enter_idle;             <==
> not needed anymore
>        drv->safe_state_index = 0;
>
> More testing on OMAP3 is needed. Let me come back with the results asap.
>

Understand and thanks for reviewing.

> Regards,
> Jean