Robert Lee | 14 Dec 08:02 2011

[RFC PATCH v2 0/2] Add common cpuidle code for consolidation.

Based on 3.2-rc4

Tested on i.MX51 Babbage Board

v1 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 (2):
  cpuidle: Add common init interface and idle functionality
  ARM: imx: Add mx5 cpuidle implmentation

 arch/arm/mach-at91/cpuidle.c        |   98 +++++++++---------------
 arch/arm/mach-davinci/cpuidle.c     |  143 ++++++++++-------------------------
 arch/arm/mach-exynos/cpuidle.c      |   73 +++---------------
 arch/arm/mach-kirkwood/cpuidle.c    |   94 ++++++++---------------
 arch/arm/mach-mx5/Makefile          |    3 +-
 arch/arm/mach-mx5/clock-mx51-mx53.c |    3 +
 arch/arm/mach-mx5/cpuidle.c         |   65 ++++++++++++++++
 arch/arm/mach-shmobile/cpuidle.c    |   40 ++--------
 drivers/cpuidle/Makefile            |    2 +-
 drivers/cpuidle/common.c            |  124 ++++++++++++++++++++++++++++++
 include/linux/cpuidle.h             |   26 ++++++
 11 files changed, 347 insertions(+), 324 deletions(-)
(Continue reading)

Robert Lee | 14 Dec 08:02 2011

[RFC PATCH v2 2/2] ARM: imx: Add mx5 cpuidle implmentation

Add mx5 cpuidle implmentation (based upon the new common cpuidle code).

Signed-off-by: Robert Lee <rob.lee <at> linaro.org>
---
 arch/arm/mach-mx5/Makefile          |    3 +-
 arch/arm/mach-mx5/clock-mx51-mx53.c |    3 ++
 arch/arm/mach-mx5/cpuidle.c         |   65 +++++++++++++++++++++++++++++++++++
 3 files changed, 70 insertions(+), 1 deletions(-)
 create mode 100644 arch/arm/mach-mx5/cpuidle.c

diff --git a/arch/arm/mach-mx5/Makefile b/arch/arm/mach-mx5/Makefile
index 0fc6080..2c348b4 100644
--- a/arch/arm/mach-mx5/Makefile
+++ b/arch/arm/mach-mx5/Makefile
 <at>  <at>  -3,8 +3,9  <at>  <at> 
 #

 # Object file lists.
-obj-y   := cpu.o mm.o clock-mx51-mx53.o ehci.o system.o

+obj-y   := cpu.o mm.o clock-mx51-mx53.o ehci.o system.o
+obj-$(CONFIG_CPU_IDLE) += cpuidle.o
 obj-$(CONFIG_PM) += pm-imx5.o
 obj-$(CONFIG_CPU_FREQ_IMX)    += cpu_op-mx51.o
 obj-$(CONFIG_MACH_MX51_BABBAGE) += board-mx51_babbage.o
diff --git a/arch/arm/mach-mx5/clock-mx51-mx53.c b/arch/arm/mach-mx5/clock-mx51-mx53.c
index 4cb2769..12c8a2b 100644
--- a/arch/arm/mach-mx5/clock-mx51-mx53.c
+++ b/arch/arm/mach-mx5/clock-mx51-mx53.c
 <at>  <at>  -1533,6 +1533,7  <at>  <at>  static struct clk_lookup mx53_lookups[] = {
(Continue reading)

Mark Brown | 22 Dec 18:50 2011

Re: [RFC PATCH v2 2/2] ARM: imx: Add mx5 cpuidle implmentation

On Wed, Dec 14, 2011 at 01:02:06AM -0600, Robert Lee wrote:

> +	clk_enable(&gpc_dvfs_clk);

Should these enables be in the cpuidle code?  The device appears to have
been working fine without them thus far...  Alternatively, if they
should be on anyway does this need to be split out and sent as a bug
fix?
Rob Lee | 5 Jan 00:35 2012

Re: [RFC PATCH v2 2/2] ARM: imx: Add mx5 cpuidle implmentation

On 22 December 2011 11:50, Mark Brown
<broonie <at> opensource.wolfsonmicro.com> wrote:
> On Wed, Dec 14, 2011 at 01:02:06AM -0600, Robert Lee wrote:
>
>> +     clk_enable(&gpc_dvfs_clk);
>
> Should these enables be in the cpuidle code?  The device appears to have
> been working fine without them thus far...  Alternatively, if they
> should be on anyway does this need to be split out and sent as a bug
> fix?

This clock is used by the existing pm_suspend code for i.MX51 and
other future code being worked on.  Since it uses extremely minimal
power and is required to be enabled during low power modes, it seemed
cleanest to just enable it during clock init.  But I forgot to remove
it from it's enabling from i.MX51 pm_suspend code so I can do that for
v3.
Shawn Guo | 5 Jan 04:21 2012

Re: [RFC PATCH v2 2/2] ARM: imx: Add mx5 cpuidle implmentation

On Wed, Jan 04, 2012 at 05:35:39PM -0600, Rob Lee wrote:
> On 22 December 2011 11:50, Mark Brown
> <broonie <at> opensource.wolfsonmicro.com> wrote:
> > On Wed, Dec 14, 2011 at 01:02:06AM -0600, Robert Lee wrote:
> >
> >> +     clk_enable(&gpc_dvfs_clk);
> >
> > Should these enables be in the cpuidle code?  The device appears to have
> > been working fine without them thus far...  Alternatively, if they
> > should be on anyway does this need to be split out and sent as a bug
> > fix?
> 
> This clock is used by the existing pm_suspend code for i.MX51 and
> other future code being worked on.  Since it uses extremely minimal
> power and is required to be enabled during low power modes, it seemed
> cleanest to just enable it during clock init.

+1

Regards,
Shawn

> But I forgot to remove
> it from it's enabling from i.MX51 pm_suspend code so I can do that for
> v3.
> 
Mark Brown | 5 Jan 06:55 2012

Re: [RFC PATCH v2 2/2] ARM: imx: Add mx5 cpuidle implmentation

On Wed, Jan 04, 2012 at 05:35:39PM -0600, Rob Lee wrote:
> On 22 December 2011 11:50, Mark Brown
> > On Wed, Dec 14, 2011 at 01:02:06AM -0600, Robert Lee wrote:

> >> +     clk_enable(&gpc_dvfs_clk);

> > Should these enables be in the cpuidle code?  The device appears to have
> > been working fine without them thus far...  Alternatively, if they
> > should be on anyway does this need to be split out and sent as a bug
> > fix?

> This clock is used by the existing pm_suspend code for i.MX51 and
> other future code being worked on.  Since it uses extremely minimal
> power and is required to be enabled during low power modes, it seemed
> cleanest to just enable it during clock init.  But I forgot to remove
> it from it's enabling from i.MX51 pm_suspend code so I can do that for
> v3.

Sounds like it's worth splitting out and getting it merged as quickly as
possible then?  It wasn't the code I was querying, it was the way it is
being merged.
Rob Lee | 6 Jan 22:10 2012

Re: [RFC PATCH v2 2/2] ARM: imx: Add mx5 cpuidle implmentation

On 4 January 2012 23:55, Mark Brown <broonie <at> opensource.wolfsonmicro.com> wrote:
> On Wed, Jan 04, 2012 at 05:35:39PM -0600, Rob Lee wrote:
>> On 22 December 2011 11:50, Mark Brown
>> > On Wed, Dec 14, 2011 at 01:02:06AM -0600, Robert Lee wrote:
>
>> >> +     clk_enable(&gpc_dvfs_clk);
>
>> > Should these enables be in the cpuidle code?  The device appears to have
>> > been working fine without them thus far...  Alternatively, if they
>> > should be on anyway does this need to be split out and sent as a bug
>> > fix?
>
>> This clock is used by the existing pm_suspend code for i.MX51 and
>> other future code being worked on.  Since it uses extremely minimal
>> power and is required to be enabled during low power modes, it seemed
>> cleanest to just enable it during clock init.  But I forgot to remove
>> it from it's enabling from i.MX51 pm_suspend code so I can do that for
>> v3.
>
> Sounds like it's worth splitting out and getting it merged as quickly as
> possible then?  It wasn't the code I was querying, it was the way it is
> being merged.

Sure, I can split this portion out as a separate patch.  I'd prefer to
keep it as part of this patch series though as the existing usage and
implementation of this clock works ok, it's just not the best
implementation once another driver needs to use this clock.  And it
may raise concern if implemented by itself with a pressing reason
being shown.  At least that's my thoughts.  I'm fairly new to the
community so please tolerate anything dumb I say and help understand
(Continue reading)

Robert Lee | 14 Dec 08:02 2011

[RFC PATCH v2 1/2] cpuidle: Add common init interface and idle functionality

The patch adds some cpuidle initialization functionality commonly
duplicated by many platforms.  The duplicate cpuidle init code of
various platfroms has been consolidated to use this common code
and successfully rebuilt.

Signed-off-by: Robert Lee <rob.lee <at> linaro.org>
---
 arch/arm/mach-at91/cpuidle.c     |   98 ++++++++++----------------
 arch/arm/mach-davinci/cpuidle.c  |  143 +++++++++++---------------------------
 arch/arm/mach-exynos/cpuidle.c   |   73 +++----------------
 arch/arm/mach-kirkwood/cpuidle.c |   94 ++++++++-----------------
 arch/arm/mach-shmobile/cpuidle.c |   40 ++---------
 drivers/cpuidle/Makefile         |    2 +-
 drivers/cpuidle/common.c         |  124 +++++++++++++++++++++++++++++++++
 include/linux/cpuidle.h          |   26 +++++++
 8 files changed, 277 insertions(+), 323 deletions(-)
 create mode 100644 drivers/cpuidle/common.c

diff --git a/arch/arm/mach-at91/cpuidle.c b/arch/arm/mach-at91/cpuidle.c
index a851e6c..b162aab 100644
--- a/arch/arm/mach-at91/cpuidle.c
+++ b/arch/arm/mach-at91/cpuidle.c
 <at>  <at>  -1,6 +1,4  <at>  <at> 
 /*
- * based on arch/arm/mach-kirkwood/cpuidle.c
- *
  * CPU idle support for AT91 SoC
  *
  * This file is licensed under the terms of the GNU General Public
 <at>  <at>  -17,84 +15,60  <at>  <at> 
(Continue reading)

Mark Brown | 22 Dec 19:09 2011

Re: [RFC PATCH v2 1/2] cpuidle: Add common init interface and idle functionality

On Wed, Dec 14, 2011 at 01:02:05AM -0600, Robert Lee wrote:
> The patch adds some cpuidle initialization functionality commonly
> duplicated by many platforms.  The duplicate cpuidle init code of
> various platfroms has been consolidated to use this common code
> and successfully rebuilt.
> 
> Signed-off-by: Robert Lee <rob.lee <at> linaro.org>

Reviewed-by: Mark Brown <broonie <at> opensource.wolfsonmicro.com>

This looks good to me with one small comment:

> +int cpuidle_def_idle(struct cpuidle_device *dev,
> +			       struct cpuidle_driver *drv, int index) {
> +	cpu_do_idle();
> +	return index;
> +}

Odd indentation here.

I'll move my s3c64xx cpuidle code over to this, though I think it'll end
up being an incremental patch as Kukjin just said he'd apply it and it'd
be nice to get the support into 3.3 if we can.
Rob Lee | 5 Jan 00:21 2012

Re: [RFC PATCH v2 1/2] cpuidle: Add common init interface and idle functionality

Mark, thanks again for your review.  Will fix the indention in v3.

On 22 December 2011 12:09, Mark Brown
<broonie <at> opensource.wolfsonmicro.com> wrote:
> On Wed, Dec 14, 2011 at 01:02:05AM -0600, Robert Lee wrote:
>> The patch adds some cpuidle initialization functionality commonly
>> duplicated by many platforms.  The duplicate cpuidle init code of
>> various platfroms has been consolidated to use this common code
>> and successfully rebuilt.
>>
>> Signed-off-by: Robert Lee <rob.lee <at> linaro.org>
>
> Reviewed-by: Mark Brown <broonie <at> opensource.wolfsonmicro.com>
>
> This looks good to me with one small comment:
>
>> +int cpuidle_def_idle(struct cpuidle_device *dev,
>> +                            struct cpuidle_driver *drv, int index) {
>> +     cpu_do_idle();
>> +     return index;
>> +}
>
> Odd indentation here.
>
> I'll move my s3c64xx cpuidle code over to this, though I think it'll end
> up being an incremental patch as Kukjin just said he'd apply it and it'd
> be nice to get the support into 3.3 if we can.
Rob Herring | 22 Dec 23:57 2011
Picon

Re: [RFC PATCH v2 1/2] cpuidle: Add common init interface and idle functionality

On 12/14/2011 01:02 AM, Robert Lee wrote:
> The patch adds some cpuidle initialization functionality commonly
> duplicated by many platforms.  The duplicate cpuidle init code of
> various platfroms has been consolidated to use this common code
> and successfully rebuilt.
> 
> Signed-off-by: Robert Lee <rob.lee <at> linaro.org>

Generally it helps if you copy the reviewers of the previous version as
I missed this one until today.

Looks pretty good. A few comments below.

> ---
>  arch/arm/mach-at91/cpuidle.c     |   98 ++++++++++----------------
>  arch/arm/mach-davinci/cpuidle.c  |  143 +++++++++++---------------------------
>  arch/arm/mach-exynos/cpuidle.c   |   73 +++----------------
>  arch/arm/mach-kirkwood/cpuidle.c |   94 ++++++++-----------------
>  arch/arm/mach-shmobile/cpuidle.c |   40 ++---------
>  drivers/cpuidle/Makefile         |    2 +-
>  drivers/cpuidle/common.c         |  124 +++++++++++++++++++++++++++++++++
>  include/linux/cpuidle.h          |   26 +++++++
>  8 files changed, 277 insertions(+), 323 deletions(-)
>  create mode 100644 drivers/cpuidle/common.c
> 
> diff --git a/arch/arm/mach-at91/cpuidle.c b/arch/arm/mach-at91/cpuidle.c
> index a851e6c..b162aab 100644
> --- a/arch/arm/mach-at91/cpuidle.c
> +++ b/arch/arm/mach-at91/cpuidle.c
>  <at>  <at>  -1,6 +1,4  <at>  <at> 
(Continue reading)

Rob Lee | 5 Jan 00:15 2012

Re: [RFC PATCH v2 1/2] cpuidle: Add common init interface and idle functionality

Rob, thanks again for the thorough review.  Comments below.

On 22 December 2011 16:57, Rob Herring <robherring2 <at> gmail.com> wrote:
> On 12/14/2011 01:02 AM, Robert Lee wrote:
>> The patch adds some cpuidle initialization functionality commonly
>> duplicated by many platforms.  The duplicate cpuidle init code of
>> various platfroms has been consolidated to use this common code
>> and successfully rebuilt.
>>
>> Signed-off-by: Robert Lee <rob.lee <at> linaro.org>
>
> Generally it helps if you copy the reviewers of the previous version as
> I missed this one until today.
>

Gotcha.  Sorry about that.

> Looks pretty good. A few comments below.
>
>> ---
>>  arch/arm/mach-at91/cpuidle.c     |   98 ++++++++++----------------
>>  arch/arm/mach-davinci/cpuidle.c  |  143 +++++++++++---------------------------
>>  arch/arm/mach-exynos/cpuidle.c   |   73 +++----------------
>>  arch/arm/mach-kirkwood/cpuidle.c |   94 ++++++++-----------------
>>  arch/arm/mach-shmobile/cpuidle.c |   40 ++---------
>>  drivers/cpuidle/Makefile         |    2 +-
>>  drivers/cpuidle/common.c         |  124 +++++++++++++++++++++++++++++++++
>>  include/linux/cpuidle.h          |   26 +++++++
>>  8 files changed, 277 insertions(+), 323 deletions(-)
>>  create mode 100644 drivers/cpuidle/common.c
(Continue reading)

Turquette, Mike | 5 Jan 00:35 2012
Picon

Re: [RFC PATCH v2 1/2] cpuidle: Add common init interface and idle functionality

On Wed, Jan 4, 2012 at 3:15 PM, Rob Lee <rob.lee <at> linaro.org> wrote:
> On 22 December 2011 16:57, Rob Herring <robherring2 <at> gmail.com> wrote:
>> On 12/14/2011 01:02 AM, Robert Lee wrote:
>>> +static struct cpuidle_driver exynos4_idle_driver = {
>>> +     .name           = "exynos4_idle",
>>> +     .owner          = THIS_MODULE,
>>> +     .states[0] = {
>>> +             .enter                  = cpuidle_def_idle,
>>>               .exit_latency           = 1,
>>>               .target_residency       = 100000,
>>>               .flags                  = CPUIDLE_FLAG_TIME_VALID,
>>>               .name                   = "IDLE",
>>>               .desc                   = "ARM clock gating(WFI)",
>>>       },
>>
>> As this is just plain wfi and shouldn't really be different per
>> platform, it would be nice to get rid of all of this state info. Perhaps
>> a macro with all the data since each driver needs to init each state.
>> The target residency value looks kind of suspect. You should only go to
>> wfi if you expect to be idle for 100ms?
>>
>
> Ok, I'll look at add a ARM specific macro for a generic WFI state in v3.
>
> For the target_residency value, I don't understand why the values
> being used are there other than some of the original ARM platform
> implementations were based on the Intel implementations per their
> comments, and the Intel value was used.  From the comments in
> drivers/cpuidle/governors/menu.c: "state entry and exit have an energy
> cost, and a certain amount of time in the  C state is required to
(Continue reading)

Rob Herring | 5 Jan 00:51 2012
Picon

Re: [RFC PATCH v2 1/2] cpuidle: Add common init interface and idle functionality

On 01/04/2012 05:35 PM, Turquette, Mike wrote:
> On Wed, Jan 4, 2012 at 3:15 PM, Rob Lee <rob.lee <at> linaro.org> wrote:
>> On 22 December 2011 16:57, Rob Herring <robherring2 <at> gmail.com> wrote:
>>> On 12/14/2011 01:02 AM, Robert Lee wrote:
>>>> +static struct cpuidle_driver exynos4_idle_driver = {
>>>> +     .name           = "exynos4_idle",
>>>> +     .owner          = THIS_MODULE,
>>>> +     .states[0] = {
>>>> +             .enter                  = cpuidle_def_idle,
>>>>               .exit_latency           = 1,
>>>>               .target_residency       = 100000,
>>>>               .flags                  = CPUIDLE_FLAG_TIME_VALID,
>>>>               .name                   = "IDLE",
>>>>               .desc                   = "ARM clock gating(WFI)",
>>>>       },
>>>
>>> As this is just plain wfi and shouldn't really be different per
>>> platform, it would be nice to get rid of all of this state info. Perhaps
>>> a macro with all the data since each driver needs to init each state.
>>> The target residency value looks kind of suspect. You should only go to
>>> wfi if you expect to be idle for 100ms?
>>>
>>
>> Ok, I'll look at add a ARM specific macro for a generic WFI state in v3.
>>
>> For the target_residency value, I don't understand why the values
>> being used are there other than some of the original ARM platform
>> implementations were based on the Intel implementations per their
>> comments, and the Intel value was used.  From the comments in
>> drivers/cpuidle/governors/menu.c: "state entry and exit have an energy
(Continue reading)

Turquette, Mike | 5 Jan 21:11 2012
Picon

Re: [RFC PATCH v2 1/2] cpuidle: Add common init interface and idle functionality

On Wed, Jan 4, 2012 at 3:51 PM, Rob Herring <robherring2 <at> gmail.com> wrote:
> On 01/04/2012 05:35 PM, Turquette, Mike wrote:
>> On Wed, Jan 4, 2012 at 3:15 PM, Rob Lee <rob.lee <at> linaro.org> wrote:
>>> On 22 December 2011 16:57, Rob Herring <robherring2 <at> gmail.com> wrote:
>>>> On 12/14/2011 01:02 AM, Robert Lee wrote:
>>>>> +static struct cpuidle_driver exynos4_idle_driver = {
>>>>> +     .name           = "exynos4_idle",
>>>>> +     .owner          = THIS_MODULE,
>>>>> +     .states[0] = {
>>>>> +             .enter                  = cpuidle_def_idle,
>>>>>               .exit_latency           = 1,
>>>>>               .target_residency       = 100000,
>>>>>               .flags                  = CPUIDLE_FLAG_TIME_VALID,
>>>>>               .name                   = "IDLE",
>>>>>               .desc                   = "ARM clock gating(WFI)",
>>>>>       },
>>>>
>>>> As this is just plain wfi and shouldn't really be different per
>>>> platform, it would be nice to get rid of all of this state info. Perhaps
>>>> a macro with all the data since each driver needs to init each state.
>>>> The target residency value looks kind of suspect. You should only go to
>>>> wfi if you expect to be idle for 100ms?
>>>>
>>>
>>> Ok, I'll look at add a ARM specific macro for a generic WFI state in v3.
>>>
>>> For the target_residency value, I don't understand why the values
>>> being used are there other than some of the original ARM platform
>>> implementations were based on the Intel implementations per their
>>> comments, and the Intel value was used.  From the comments in
(Continue reading)

Russell King - ARM Linux | 5 Jan 10:32 2012
Picon

Re: [RFC PATCH v2 1/2] cpuidle: Add common init interface and idle functionality

On Wed, Dec 14, 2011 at 01:02:05AM -0600, Robert Lee wrote:
> +	asm("b 1f; .align 5; 1:");
> +	asm("mcr p15, 0, r0, c7, c10, 4");	/* drain write buffer */

Err, no.  The compiler is free to add whatever instructions it sees
fit between these two asm() statements.

If you want to ask the compiler to issue a set of assembly instructions
as a block, you have to use just one asm() statement.
Rob Lee | 5 Jan 17:42 2012

Re: [RFC PATCH v2 1/2] cpuidle: Add common init interface and idle functionality

On 5 January 2012 03:32, Russell King - ARM Linux
<linux <at> arm.linux.org.uk> wrote:
> On Wed, Dec 14, 2011 at 01:02:05AM -0600, Robert Lee wrote:
>> +     asm("b 1f; .align 5; 1:");
>> +     asm("mcr p15, 0, r0, c7, c10, 4");      /* drain write buffer */
>
> Err, no.  The compiler is free to add whatever instructions it sees
> fit between these two asm() statements.
>
> If you want to ask the compiler to issue a set of assembly instructions
> as a block, you have to use just one asm() statement.

Thanks.  I will fix that in v3.

Gmane