[PATCH v2 1/3] ARM: imx: Add imx cpuidle driver

[Rob Lee]

On 25 September 2011 22:54, Rob Lee <rob.lee at linaro.org> wrote:
> Hello Russell,
>
> On 16 September 2011 16:36, Russell King - ARM Linux
> <linux at arm.linux.org.uk> wrote:
>> On Fri, Sep 16, 2011 at 12:27:48PM -0500, Robert Lee wrote:
>>> Introduce a new cpuidle driver which provides the common cpuidle
>>> functionality necessary for any imx soc cpuidle implementation.
>>
>> I think its probably about time we said no to this duplication of CPU
>> idle infrastructure.  There seems to be a common pattern appearing
>> through all SoCs - they're all doing this:
>>
>>> +static int imx_enter_idle(struct cpuidle_device *dev,
>>> +                            struct cpuidle_state *state)
>>> +{
>>> +     struct timeval before, after;
>>> +     int idle_time;
>>> +
>>> +     local_irq_disable();
>>> +     local_fiq_disable();
>>> +
>>> +     do_gettimeofday(&before);
>>> +
>>> +     mach_cpuidle(dev, state);
>>> +
>>> +     do_gettimeofday(&after);
>>> +
>>> +     local_fiq_enable();
>>> +     local_irq_enable();
>>> +
>>> +     idle_time = (after.tv_sec - before.tv_sec) * USEC_PER_SEC +
>>> +             (after.tv_usec - before.tv_usec);
>>> +
>>> +     return idle_time;
>>> +}
>>
>
> If I understand you correctly, it sounds like you are suggesting
> adding cpuidle initialization functionality that is common for all ARM.
> Did you mean just the above function or also the functionality found
> in imx_cpuidle_init and imx_cpuidle_dev_init?  For this common
> ARM functionality, would a cpuidle.c file in arch/arm/common/ be the
> right location?

After looking at this further today, it appears that there is precedence
for putting a cpuidle driver common to a particular cpu
architecture in drivers/idle.  So perhaps a new file "arm_idle.c" that
has common cpuidle functionality could be added to that location.

>
>> in some form, where 'do_gettimeofday' might be ktime_get() or
>> getnstimeofday().  If we can standardize on which of the many time
>> functions can be used (which would be a definite plus) we should move
>> this out to common code.
>
> Looking into the time keeping functionality more, of the time functions
> you mentioned I think that ktime_get is preferable as its result it effectively
> a monotonic time that won't be changed with calls to do_settimeofday
> unlike the other two functions whose use of xtime only could result in
> an error in the reported time difference on SMP systems.
>
>>
>> Maybe also the initialization code could be standardized and improved
>> too - for instance, what if you boot with maxcpus=1 on a platform
>> supporting 2 CPUs, and you bring CPU1 online from userspace?  When
>> these CPU idle initialization functions are called, only one CPU will
>> be online, and as they use 'for_each_cpu(cpu_id, cpu_online_mask)'
>> CPU1 will be missing the cpu idle init.
>>
>