[RFC PATCH v4 0/4] Consolidate cpuidle timekeeping and irq enabling

[Amit Kucheria]

On Fri, Feb 10, 2012 at 11:32 AM, Rob Lee <rob.lee at linaro.org> wrote:
> Maintainers for drivers/cpuidle, do you have any comments/opinions
> about this patch?

Venki has changed employers (probably needs a patch to MAINTAINERS?).
Cc'ing his new email address.

> Intel cpuidle and acpi cpuidle maintainers, do you have any
> comments/opinions about this patch and the changes to your code?
>
> Any other review and comments welcome.
>
> Summary of positive and negatives as I understand them so far:
>
> version 1, 2, and 3 (Original "wrapper" method of consolidating
> timekeeping and interrupt enabling)
> + opportunistically provides consolidation for simple platform cpuidle
> implementations without disturbing the more complex implementations.
> By simple, I mean those at can be wrapped in the time keeping calls
> and interrupt enabling calls without significantly affecting idle time
> keeping accuracy or interrupt latency
> - Does not provide consolidation for the more complex platform cpuidle
> implementations
> - Adds an additional interface, perhaps unnecessarily if this
> consolidation could be done in cpuidle
>
> version 4 (modifications to drivers/cpuidle/cpuidle.c cpuidle_idle_call)
> + Adds consolidation work to cpuidle_idle_call which allows all
> platform timekeeping / interrupt handling to be consolidated.
> - Requires splitting up of more complex platform cpuidle
> implementations, adding further complexity and risk of breaking
> something.
> ? Allows both pre_enter or enter to change the idle state.  Is there
> an objection to this?
> ? Splitting up the enter functions can require additional function
> calls.  Is there any concern that this is significant additional
> overhead?
>
> Thanks,
> Rob
>
>
> On Tue, Jan 31, 2012 at 7:00 PM, Robert Lee <rob.lee at linaro.org> wrote:
>> This patch series moves the timekeeping and irq enabling from the platform
>> code to the core cpuidle driver.  Also, the platform irq disabling was removed
>> as it appears that all calls into cpuidle_call_idle will have already called
>> local_irq_disable().
>>
>> To save reviewers time, only a few platforms which required the most changes
>> are included in this version.  If these changes are approved, the next version
>> will include the remaining platform code which should require minimal changes.
>>
>> For those who have followed the previous patch versions, as you know, the
>> previous version of this patch series added some helper functionality which
>> used a wrapper function to remove the time keeping and irq enabling/disabling
>> from the platform code.  There was also initialization helper functionality
>> which has now been removed from this version.  If the basic implementation
>> in this version is approved, then a separate patch submission effort can be
>> made to focus on consolidation of initialziation functionality.
>>
>> Based on 3.3-rc1
>>
>> v3 submission can be found here:
>> http://www.spinics.net/lists/arm-kernel/msg156751.html
>> Changes since v3:
>> * Removed drivers/cpuidle/common.c
>> ** Removed the initialization helper functions
>> ** Removed the wrapper used to consolidate time keeping and irq enable/disable
>> * Add time keeping and local_irq_disable handling in cpuidle_call_idle().
>> * Made necessary modifications to a few platforms that required the most changes
>> ** Note on omap3: changed structure of omap3_idle_drvdata and added
>>   per_next_state and per_saved_state vars to accomodate new framework.
>>
>> v2 submission can be found here:
>> http://comments.gmane.org/gmane.linux.ports.arm.kernel/144199
>>
>> Changes since v2:
>> * Made various code organization and style changes as suggested in v1 review.
>> * Removed at91 use of common code.  A separate effort is underway to clean
>> at91 code and the author has offered to convert to common interface as part
>> of those changes (if this common interface is accepted in time).
>> * Made platform cpuidle_driver objects __initdata and dynamically added one
>> persistent instance of this object in common code.
>> * Removed imx5 pm usage of gpc_dvfs clock as it is no longer needed after
>> being enabled during clock initialization.
>> * Re-organized patches.
>>
>> v1 submission can be found here:
>> http://comments.gmane.org/gmane.linux.ports.arm.kernel/142791
>>
>> Changes since v1:
>> * Common interface moved to drivers/cpuidle and made non arch-specific.
>> * Made various fixes and suggested additions to the common cpuidle
>> code from v1 review.
>> * Added callback for filling in driver_data field as needed.
>> * Modified the various platforms with these changes.
>>
>> Robert Lee (4):
>>  cpuidle: Add time keeping and irq enabling
>>  ARM: omap: Remove cpuidle timekeeping and irq enable/disable
>>  acpi: Remove cpuidle timekeeping and irq enable/disable
>>  x86: Remove cpuidle timekeeping and irq enable/disable
>>
>>  arch/arm/mach-omap2/cpuidle34xx.c |   96 ++++++++----------
>>  drivers/acpi/processor_idle.c     |  203 ++++++++++++++++++++++---------------
>>  drivers/cpuidle/cpuidle.c         |   75 +++++++++++---
>>  drivers/idle/intel_idle.c         |  110 ++++++++++++++------
>>  include/linux/cpuidle.h           |   26 +++--
>>  5 files changed, 317 insertions(+), 193 deletions(-)
>>