[PATCH v2 0/3] ARM: imx: Add cpuidle for imx platforms

[Rob Lee]

Hello Sascha,

On 20 September 2011 02:16, Sascha Hauer <s.hauer at pengutronix.de> wrote:
> Hi Robert,
>
> On Fri, Sep 16, 2011 at 12:27:47PM -0500, Robert Lee wrote:
>> This patch series adds a common imx cpuidle driver, some common
>> mach-mx5 level cpuidle functionality, and an i.MX51 instance of
>> using this driver.
>>
>> The patch series is based on v3.1-rc2.
>>
>> Changes since v1:
>> * To address all the problems found during review of v1, a complete
>> re-write was needed.
>
> Much better than the last version, thanks.
>

Thanks to Shawn Guo for thoroughly reviewing and providing
feedback on this version and Jason Hui contributed to the reviewing
as well despite both of them being very busy with there own
submissions.

>>
>> Robert Lee (3):
>>   ARM: imx: Add imx cpuidle driver
>>   ARM: imx: Add cpuidle for mach-mx5
>>   ARM: imx: Add cpuidle for i.MX51
>>
>>  arch/arm/mach-mx5/Makefile               |    3 +-
>>  arch/arm/mach-mx5/cpu_op-mx51.c          |   35 +++++++-
>
> Your additions to this file...
>
>>  arch/arm/mach-mx5/cpu_op-mx51.h          |    1 +
>>  arch/arm/mach-mx5/mm.c                   |    4 +
>>  arch/arm/mach-mx5/system.c               |   35 +++++++
>
> And this one should go to arch/arm/mach-mx5/cpuidle.c, then we have
> all i.MX5 specific cpuidle stuff together without ifdeffing the code.
> I assume that cpuidle support for i.MX53 will look identically, right?
>

After considering it further after the first patch, there can be differences
between the i.MX51 and i.MX53 state data as i.MX51 supports an
additional idle state that i.MX53 does not.  This state was not added
to this version of the patch because it's additional power savings is very
small and there is fairly time consuming characterization needed to
come up with accurate cpuidle state data.  But to allow for the
possibility that this state could be added in the future, it seemed the
choice was to add this init data to the an i.MX51 specific file such as
cpu_op-mx51.c/h and add ifdefs for cpuidle and cpufreq.  Or, make
a cpuidle.c file as you suggest and add ifdefs for the different mx5
SoCs.  Just let me know which way you think is better given this
information.

For the call to imx_cpuidle_init from imx51_soc_init in mm.c, this
seems like a good location to initialize for all imx51_soc.  If I don't
call the init from here, can you recommend another location that I
should call it from?

>>  arch/arm/plat-mxc/Makefile               |    1 +
>>  arch/arm/plat-mxc/cpuidle.c              |  151 ++++++++++++++++++++++++++++++
>>  arch/arm/plat-mxc/include/mach/cpuidle.h |   56 +++++++++++
>>  arch/arm/plat-mxc/include/mach/system.h  |    3 +
>>  9 files changed, 286 insertions(+), 3 deletions(-)
>>  create mode 100644 arch/arm/plat-mxc/cpuidle.c
>>  create mode 100644 arch/arm/plat-mxc/include/mach/cpuidle.h
>>
>>
>
> Sascha
>
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>