At the Linaro Connect Asia 2013, a status of the different cpuidle drivers available upstream have been presented [1].
It was statued there is a lot of common code, especially in the init routine, and code duplication (eg. ux500 vs imx6).
The following patchset is the first stone to a single ARM driver consolidating all the common routine used in the different drivers.
The patchset has been tested on ux500 and at91, compiled on all the other platforms.
[1] https://lca-13.zerista.com/event/member/72362
Daniel Lezcano (11): cpuidle : handle clockevent notify from the cpuidle framework cpuidle / arm : a single cpuidle driver cpuidle / ux500 : use common ARM cpuidle driver cpuidle / omap3 : use common ARM cpuidle driver cpuidle / davinci : use common ARM driver cpuidle / at91 : use common ARM cpuidle driver cpuidle / shmobile : use common ARM cpuidle driver cpuidle / imx : use common ARM cpuidle driver cpuidle / s3c64xx : use common ARM cpuidle driver cpuidle / calxeda : use common ARM cpuidle driver cpuidle / kirkwood : use common ARM cpuidle driver
MAINTAINERS | 6 ++ arch/arm/include/asm/cpuidle.h | 3 + arch/arm/mach-at91/cpuidle.c | 15 +---- arch/arm/mach-davinci/cpuidle.c | 20 +------ arch/arm/mach-imx/Makefile | 1 - arch/arm/mach-imx/cpuidle-imx6q.c | 18 +----- arch/arm/mach-imx/cpuidle.c | 80 -------------------------- arch/arm/mach-imx/cpuidle.h | 6 +- arch/arm/mach-imx/pm-imx5.c | 3 +- arch/arm/mach-omap2/cpuidle34xx.c | 18 +----- arch/arm/mach-s3c64xx/cpuidle.c | 15 +---- arch/arm/mach-shmobile/cpuidle.c | 10 +--- arch/arm/mach-ux500/cpuidle.c | 56 +----------------- drivers/cpuidle/Makefile | 1 + drivers/cpuidle/arm-idle.c | 112 ++++++++++++++++++++++++++++++++++++ drivers/cpuidle/cpuidle-calxeda.c | 52 +---------------- drivers/cpuidle/cpuidle-kirkwood.c | 17 +----- drivers/cpuidle/cpuidle.c | 9 +++ include/linux/cpuidle.h | 1 + 19 files changed, 149 insertions(+), 294 deletions(-) delete mode 100644 arch/arm/mach-imx/cpuidle.c create mode 100644 drivers/cpuidle/arm-idle.c
When a cpu enters a deep idle state, the local timers are stopped and the time framework falls back to the timer device used as a broadcast timer.
The different cpuidle drivers are calling clockevents_notify ENTER/EXIT when the idle state stops the local timer.
The proposed patch introduces a new flag CPUIDLE_FLAG_TIMER_STOP to let the cpuidle framework to call clockevents_notify instead of duplicating again and again these lines in all the cpuidle drivers.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- drivers/cpuidle/cpuidle.c | 9 +++++++++ include/linux/cpuidle.h | 1 + 2 files changed, 10 insertions(+)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index eba6929..c500370 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -8,6 +8,7 @@ * This code is licenced under the GPL. */
+#include <linux/clockchips.h> #include <linux/kernel.h> #include <linux/mutex.h> #include <linux/sched.h> @@ -146,12 +147,20 @@ int cpuidle_idle_call(void)
trace_cpu_idle_rcuidle(next_state, dev->cpu);
+ if (drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP) + clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, + &dev->cpu); + if (cpuidle_state_is_coupled(dev, drv, next_state)) entered_state = cpuidle_enter_state_coupled(dev, drv, next_state); else entered_state = cpuidle_enter_state(dev, drv, next_state);
+ if (drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP) + clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, + &dev->cpu); + trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
/* give the governor an opportunity to reflect on the outcome */ diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 480c14d..a837b33 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -57,6 +57,7 @@ struct cpuidle_state { /* Idle State Flags */ #define CPUIDLE_FLAG_TIME_VALID (0x01) /* is residency time measurable? */ #define CPUIDLE_FLAG_COUPLED (0x02) /* state applies to multiple cpus */ +#define CPUIDLE_FLAG_TIMER_STOP (0x04) /* timer is stopped on this state */
#define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000)
The cpuidle drivers are duplicating a lot of code and in most of the case there is a common pattern we can factor out:
* setup the broadcast timers * register the driver * register the devices
This arm driver is the common part between all the ARM cpuidle drivers, with the code factored out.
It does not handle the coupled idle state for now but it is the first step to have everyone to converge to the same code pattern.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- MAINTAINERS | 6 +++ arch/arm/include/asm/cpuidle.h | 3 ++ drivers/cpuidle/Makefile | 1 + drivers/cpuidle/arm-idle.c | 112 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 122 insertions(+) create mode 100644 drivers/cpuidle/arm-idle.c
diff --git a/MAINTAINERS b/MAINTAINERS index 9561658..2c13cf3 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2212,6 +2212,12 @@ S: Maintained F: arch/x86/kernel/cpuid.c F: arch/x86/kernel/msr.c
+CPUIDLE FOR ARM +M: Daniel Lezcano daniel.lezcano@linaro.org +L: linux-pm@vger.kernel.org +S: Maintained +F: drivers/cpuidle/arm-idle.c + CPU POWER MONITORING SUBSYSTEM M: Dominik Brodowski linux@dominikbrodowski.net M: Thomas Renninger trenn@suse.de diff --git a/arch/arm/include/asm/cpuidle.h b/arch/arm/include/asm/cpuidle.h index 2fca60a..107cf23 100644 --- a/arch/arm/include/asm/cpuidle.h +++ b/arch/arm/include/asm/cpuidle.h @@ -9,6 +9,9 @@ static inline int arm_cpuidle_simple_enter(struct cpuidle_device *dev, struct cpuidle_driver *drv, int index) { return -ENODEV; } #endif
+extern int arm_idle_init(struct cpuidle_driver *drv); +extern void arm_idle_exit(struct cpuidle_driver *drv); + /* Common ARM WFI state */ #define ARM_CPUIDLE_WFI_STATE_PWR(p) {\ .enter = arm_cpuidle_simple_enter,\ diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile index d1aba71..4816a78 100644 --- a/drivers/cpuidle/Makefile +++ b/drivers/cpuidle/Makefile @@ -5,5 +5,6 @@ obj-y += cpuidle.o driver.o governor.o sysfs.o governors/ obj-$(CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED) += coupled.o
+obj-$(CONFIG_ARM) += arm-idle.o obj-$(CONFIG_ARCH_HIGHBANK) += cpuidle-calxeda.o obj-$(CONFIG_ARCH_KIRKWOOD) += cpuidle-kirkwood.o diff --git a/drivers/cpuidle/arm-idle.c b/drivers/cpuidle/arm-idle.c new file mode 100644 index 0000000..397ff4c --- /dev/null +++ b/drivers/cpuidle/arm-idle.c @@ -0,0 +1,112 @@ +/* + * Copyright 2012 Linaro Ltd: : Daniel Lezcano daniel.lezcano@linaro.org + * + * The code contained herein is licensed under the GNU General Public + * License. You may obtain a copy of the GNU General Public License + * Version 2 or later at the following locations: + * + * http://www.opensource.org/licenses/gpl-license.html + * http://www.gnu.org/copyleft/gpl.html + */ + +#include <linux/clockchips.h> +#include <linux/module.h> +#include <linux/cpuidle.h> + +static DEFINE_PER_CPU(struct cpuidle_device, cpuidle_device); +static bool use_broadcast_timer = false; + +#ifdef CONFIG_GENERIC_CLOCKEVENTS +static void setup_broadcast_timer(void *arg) +{ + int cpu = smp_processor_id(); + clockevents_notify((int)(arg), &cpu); +} + +static bool __init arm_idle_use_broadcast(struct cpuidle_driver *drv) +{ + int i; + + for (i = 0; i < drv->state_count; i++) + if (drv->states[i].flags & CPUIDLE_FLAG_TIMER_STOP) + return true; + return false; +} + +static inline void arm_idle_timer_broadcast(bool enable) +{ + on_each_cpu(setup_broadcast_timer, enable ? + (void *)CLOCK_EVT_NOTIFY_BROADCAST_ON: + (void *)CLOCK_EVT_NOTIFY_BROADCAST_OFF, 1); +} +#else +static inline bool __init arm_idle_use_broadcast(struct cpuidle_driver *drv) +{ + return false; +} + +static inline void arm_idle_timer_broadcast(bool enable) +{ + ; +} +#endif + +int __init arm_idle_init(struct cpuidle_driver *drv) +{ + int ret, cpu; + struct cpuidle_device *device; + + use_broadcast_timer = arm_idle_use_broadcast(drv); + + if (use_broadcast_timer) + arm_idle_timer_broadcast(true); + + ret = cpuidle_register_driver(drv); + if (ret) { + printk(KERN_ERR "failed to register idle driver '%s'\n", + drv->name); + return ret; + } + + for_each_online_cpu(cpu) { + + device = &per_cpu(cpuidle_device, cpu); + device->cpu = cpu; + ret = cpuidle_register_device(device); + if (ret) { + printk(KERN_ERR "Failed to register cpuidle " + "device for cpu%d\n", cpu); + goto out_unregister; + } + } + +out: + return ret; + +out_unregister: + for_each_online_cpu(cpu) { + device = &per_cpu(cpuidle_device, cpu); + cpuidle_unregister_device(device); + } + + cpuidle_unregister_driver(drv); + goto out; +} +EXPORT_SYMBOL_GPL(arm_idle_init); + +void __exit arm_idle_exit(struct cpuidle_driver *drv) +{ + int cpu; + struct cpuidle_device *device; + + for_each_online_cpu(cpu) { + device = &per_cpu(cpuidle_device, cpu); + cpuidle_unregister_device(device); + } + + cpuidle_unregister_driver(drv); + + if (use_broadcast_timer) + arm_idle_timer_broadcast(false); +} +EXPORT_SYMBOL_GPL(arm_idle_exit);
On Friday 15 March 2013, Daniel Lezcano wrote:
The cpuidle drivers are duplicating a lot of code and in most of the case there is a common pattern we can factor out:
* setup the broadcast timers * register the driver * register the devicesThis arm driver is the common part between all the ARM cpuidle drivers, with the code factored out.
It does not handle the coupled idle state for now but it is the first step to have everyone to converge to the same code pattern.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
Unfortunately, I missed the session in Hong Kong, but I'd like to understand what part of this driver is actually ARM specific. I assume there is nothing in it that depends on 32 bit ARM hardware, right?
Would the same code be used with arch/arm64?
What about other architectures that want to share a cpuidle driver with and ARM SoC using the same hardware? We have a lot of examples of SoC vendors that use similar components on ARM and non-ARM SoCs based on SH, AVR32, Hexagon, C6x, MIPS or PowerPC.
Arnd
On 03/15/2013 03:47 PM, Arnd Bergmann wrote:
On Friday 15 March 2013, Daniel Lezcano wrote:
The cpuidle drivers are duplicating a lot of code and in most of the case there is a common pattern we can factor out:
* setup the broadcast timers * register the driver * register the devicesThis arm driver is the common part between all the ARM cpuidle drivers, with the code factored out.
It does not handle the coupled idle state for now but it is the first step to have everyone to converge to the same code pattern.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
Unfortunately, I missed the session in Hong Kong, but I'd like to understand what part of this driver is actually ARM specific. I assume there is nothing in it that depends on 32 bit ARM hardware, right?
What depends on the 32bits ARM hardware is the drivers themselves. This ARM driver is the first step to consolidate all SoC cpuidle specific code we find in arch/arm. The code is designated to factor out these drivers, as a first step. The second step is to consolidate more this driver (eg. moving the code in arch/arm/kernel/cpuidle.c in this driver).
The last man standing algorithm we have in mach-ux500 and mach-imx is the same, and will be moved in the ARM cpuidle driver.
The more it will be consolidated, the more it will be ARM specific.
The final step will be to use the device tree to be passed to this driver and do the cpuidle driver initialization from there.
Would the same code be used with arch/arm64?
I can't tell but IIUC, a single ARM driver will be used based on the psci for ARM64, but it does not exist for now. It is possible some code pattern could be used from the ARM32 cpuidle driver.
What about other architectures that want to share a cpuidle driver with and ARM SoC using the same hardware? We have a lot of examples of SoC vendors that use similar components on ARM and non-ARM SoCs based on SH, AVR32, Hexagon, C6x, MIPS or PowerPC.
I guess if they share the same code, it could probably go to the cpuidle core framework instead of a specific driver.
On 15 March 2013 15:07, Daniel Lezcano daniel.lezcano@linaro.org wrote:
On 03/15/2013 03:47 PM, Arnd Bergmann wrote:
On Friday 15 March 2013, Daniel Lezcano wrote:
The cpuidle drivers are duplicating a lot of code and in most of the case there is a common pattern we can factor out:
* setup the broadcast timers * register the driver * register the devicesThis arm driver is the common part between all the ARM cpuidle drivers, with the code factored out.
It does not handle the coupled idle state for now but it is the first step to have everyone to converge to the same code pattern.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
Unfortunately, I missed the session in Hong Kong, but I'd like to understand what part of this driver is actually ARM specific. I assume there is nothing in it that depends on 32 bit ARM hardware, right?
What depends on the 32bits ARM hardware is the drivers themselves. This ARM driver is the first step to consolidate all SoC cpuidle specific code we find in arch/arm. The code is designated to factor out these drivers, as a first step. The second step is to consolidate more this driver (eg. moving the code in arch/arm/kernel/cpuidle.c in this driver).
The last man standing algorithm we have in mach-ux500 and mach-imx is the same, and will be moved in the ARM cpuidle driver.
The more it will be consolidated, the more it will be ARM specific.
The final step will be to use the device tree to be passed to this driver and do the cpuidle driver initialization from there.
Would the same code be used with arch/arm64?
I can't tell but IIUC, a single ARM driver will be used based on the psci for ARM64, but it does not exist for now. It is possible some code pattern could be used from the ARM32 cpuidle driver.
While we recommend PSCI if EL3 (secure mode) is available, not all ARMv8 CPU implementations will have this and most likely we'll have to reuse existing cpuidle drivers. So if you can make it as generic as possible it would really help.
On 03/25/2013 07:27 PM, Catalin Marinas wrote:
On 15 March 2013 15:07, Daniel Lezcano daniel.lezcano@linaro.org wrote:
On 03/15/2013 03:47 PM, Arnd Bergmann wrote:
On Friday 15 March 2013, Daniel Lezcano wrote:
The cpuidle drivers are duplicating a lot of code and in most of the case there is a common pattern we can factor out:
* setup the broadcast timers * register the driver * register the devicesThis arm driver is the common part between all the ARM cpuidle drivers, with the code factored out.
It does not handle the coupled idle state for now but it is the first step to have everyone to converge to the same code pattern.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
Unfortunately, I missed the session in Hong Kong, but I'd like to understand what part of this driver is actually ARM specific. I assume there is nothing in it that depends on 32 bit ARM hardware, right?
What depends on the 32bits ARM hardware is the drivers themselves. This ARM driver is the first step to consolidate all SoC cpuidle specific code we find in arch/arm. The code is designated to factor out these drivers, as a first step. The second step is to consolidate more this driver (eg. moving the code in arch/arm/kernel/cpuidle.c in this driver).
The last man standing algorithm we have in mach-ux500 and mach-imx is the same, and will be moved in the ARM cpuidle driver.
The more it will be consolidated, the more it will be ARM specific.
The final step will be to use the device tree to be passed to this driver and do the cpuidle driver initialization from there.
Would the same code be used with arch/arm64?
I can't tell but IIUC, a single ARM driver will be used based on the psci for ARM64, but it does not exist for now. It is possible some code pattern could be used from the ARM32 cpuidle driver.
While we recommend PSCI if EL3 (secure mode) is available, not all ARMv8 CPU implementations will have this and most likely we'll have to reuse existing cpuidle drivers. So if you can make it as generic as possible it would really help.
Sure, I will keep it in mind.
Thanks for the information.
-- Daniel
On Friday 15 March 2013 07:57 PM, Daniel Lezcano wrote:
The cpuidle drivers are duplicating a lot of code and in most of the case there is a common pattern we can factor out:
- setup the broadcast timers
- register the driver
- register the devices
This arm driver is the common part between all the ARM cpuidle drivers, with the code factored out.
It does not handle the coupled idle state for now but it is the first step to have everyone to converge to the same code pattern.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
While I appreciate the effort behind code consolidation, $subject is bit confusing. You are just abstracting the registration code to one common place and I don't know why it has to be limited to arm-idle since it is very generic code. That is true even for the broad-cast notifier setup which is same across all arch's including ARM, X86.
MAINTAINERS | 6 +++ arch/arm/include/asm/cpuidle.h | 3 ++ drivers/cpuidle/Makefile | 1 + drivers/cpuidle/arm-idle.c | 112 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 122 insertions(+) create mode 100644 drivers/cpuidle/arm-idle.c
[..]
diff --git a/drivers/cpuidle/arm-idle.c b/drivers/cpuidle/arm-idle.c new file mode 100644 index 0000000..397ff4c --- /dev/null +++ b/drivers/cpuidle/arm-idle.c @@ -0,0 +1,112 @@ +/*
- Copyright 2012 Linaro Ltd: : Daniel Lezcano daniel.lezcano@linaro.org
- The code contained herein is licensed under the GNU General Public
- License. You may obtain a copy of the GNU General Public License
- Version 2 or later at the following locations:
- */
+#include <linux/clockchips.h> +#include <linux/module.h> +#include <linux/cpuidle.h>
+static DEFINE_PER_CPU(struct cpuidle_device, cpuidle_device); +static bool use_broadcast_timer = false;
+#ifdef CONFIG_GENERIC_CLOCKEVENTS +static void setup_broadcast_timer(void *arg) +{
int cpu = smp_processor_id();clockevents_notify((int)(arg), &cpu);+}
+static bool __init arm_idle_use_broadcast(struct cpuidle_driver *drv) +{
- int i;
- for (i = 0; i < drv->state_count; i++)
if (drv->states[i].flags & CPUIDLE_FLAG_TIMER_STOP)return true;- return false;
+}
+static inline void arm_idle_timer_broadcast(bool enable) +{
- on_each_cpu(setup_broadcast_timer, enable ?
(void *)CLOCK_EVT_NOTIFY_BROADCAST_ON:(void *)CLOCK_EVT_NOTIFY_BROADCAST_OFF, 1);+} +#else +static inline bool __init arm_idle_use_broadcast(struct cpuidle_driver *drv) +{
- return false;
+}
+static inline void arm_idle_timer_broadcast(bool enable) +{
- ;
+} +#endif
+int __init arm_idle_init(struct cpuidle_driver *drv) +{
- int ret, cpu;
- struct cpuidle_device *device;
- use_broadcast_timer = arm_idle_use_broadcast(drv);
- if (use_broadcast_timer)
arm_idle_timer_broadcast(true);- ret = cpuidle_register_driver(drv);
- if (ret) {
printk(KERN_ERR "failed to register idle driver '%s'\n",drv->name);return ret;- }
- for_each_online_cpu(cpu) {
device = &per_cpu(cpuidle_device, cpu);device->cpu = cpu;ret = cpuidle_register_device(device);if (ret) {printk(KERN_ERR "Failed to register cpuidle ""device for cpu%d\n", cpu);goto out_unregister;}- }
+out:
- return ret;
+out_unregister:
- for_each_online_cpu(cpu) {
device = &per_cpu(cpuidle_device, cpu);cpuidle_unregister_device(device);- }
- cpuidle_unregister_driver(drv);
- goto out;
+} +EXPORT_SYMBOL_GPL(arm_idle_init);
+void __exit arm_idle_exit(struct cpuidle_driver *drv) +{
- int cpu;
- struct cpuidle_device *device;
- for_each_online_cpu(cpu) {
device = &per_cpu(cpuidle_device, cpu);cpuidle_unregister_device(device);- }
- cpuidle_unregister_driver(drv);
- if (use_broadcast_timer)
arm_idle_timer_broadcast(false);+} +EXPORT_SYMBOL_GPL(arm_idle_exit);
All above code is completly generic and I would rather create some thing like "drivers/cpuidle/generic-idle.c" where it can handle all the registration stuff for all arch's rather than just ARM. There is nothing ARM specific in above code IMHO.
Regards, Santosh
On 03/26/2013 05:31 AM, Santosh Shilimkar wrote:
On Friday 15 March 2013 07:57 PM, Daniel Lezcano wrote:
The cpuidle drivers are duplicating a lot of code and in most of the case there is a common pattern we can factor out:
- setup the broadcast timers
- register the driver
- register the devices
This arm driver is the common part between all the ARM cpuidle drivers, with the code factored out.
It does not handle the coupled idle state for now but it is the first step to have everyone to converge to the same code pattern.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
While I appreciate the effort behind code consolidation, $subject is bit confusing. You are just abstracting the registration code to one common place and I don't know why it has to be limited to arm-idle since it is very generic code. That is true even for the broad-cast notifier setup which is same across all arch's including ARM, X86.
[ ... ]
Ok, I should have put a subject:
"cpuidle / arm : a single cpuidle driver : step 1"
As I mentioned earlier, these init functions will be modified. This is why I prefer ATM to keep these initialization in this file.
When the consolidation reach an acceptable state, then all the arch will be consolidated in the generic framework for the common parts.
- if (use_broadcast_timer)
arm_idle_timer_broadcast(false);+} +EXPORT_SYMBOL_GPL(arm_idle_exit);
All above code is completly generic and I would rather create some thing like "drivers/cpuidle/generic-idle.c" where it can handle all the registration stuff for all arch's rather than just ARM. There is nothing ARM specific in above code IMHO.
Yes, it seems generic but it won't be.
There is my tree at
http://git.linaro.org/gitweb?p=people/dlezcano/cpuidle-next.git%3Ba=shortlog...
So you can see a part of the evolution of the patchset.
All above code is completly generic and I would rather create some thing like "drivers/cpuidle/generic-idle.c" where it can handle all the registration stuff for all arch's rather than just ARM. There is nothing ARM specific in above code IMHO.
Yes, it seems generic but it won't be.
It very much sounds like you are going in the wrong direction. You should be moving towards generic code, not away from generic code.
Yes, you at some point will need ARM specific code, but that should be a minor part and can be placed somewhere else, and called by a function pointer, or the generic code can be in a library and called from the ARM specific code, etc.
Andrew
On 03/26/2013 12:17 PM, Andrew Lunn wrote:
All above code is completly generic and I would rather create some thing like "drivers/cpuidle/generic-idle.c" where it can handle all the registration stuff for all arch's rather than just ARM. There is nothing ARM specific in above code IMHO.
Yes, it seems generic but it won't be.
It very much sounds like you are going in the wrong direction. You should be moving towards generic code, not away from generic code.
Well, I am going to the right direction but with an intermediate step :)
But indeed, I will introduce this init function in the generic code directly and rebase the patchset.
Rafael, is possible to apply the patches 1 - 5 ? Or shall I resend the subset ?
Yes, you at some point will need ARM specific code, but that should be a minor part and can be placed somewhere else, and called by a function pointer, or the generic code can be in a library and called from the ARM specific code, etc.
Andrew
linaro-kernel mailing list linaro-kernel@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-kernel
On Tuesday, March 26, 2013 12:44:34 PM Daniel Lezcano wrote:
On 03/26/2013 12:17 PM, Andrew Lunn wrote:
All above code is completly generic and I would rather create some thing like "drivers/cpuidle/generic-idle.c" where it can handle all the registration stuff for all arch's rather than just ARM. There is nothing ARM specific in above code IMHO.
Yes, it seems generic but it won't be.
It very much sounds like you are going in the wrong direction. You should be moving towards generic code, not away from generic code.
Well, I am going to the right direction but with an intermediate step :)
But indeed, I will introduce this init function in the generic code directly and rebase the patchset.
Rafael, is possible to apply the patches 1 - 5 ? Or shall I resend the subset ?
Please repost if you can, with all applicable ACKs.
Thanks, Rafael
The ARM generic cpuidle driver handles the initialization and the CPUIDLE_FLAG_TIMER_STOP allows the cpuidle framework to call clockevents_notify.
This patch removes the duplicated code and use the arm_idle_init function.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-ux500/cpuidle.c | 56 +++-------------------------------------- 1 file changed, 3 insertions(+), 53 deletions(-)
diff --git a/arch/arm/mach-ux500/cpuidle.c b/arch/arm/mach-ux500/cpuidle.c index ce91493..36bf4fd 100644 --- a/arch/arm/mach-ux500/cpuidle.c +++ b/arch/arm/mach-ux500/cpuidle.c @@ -22,7 +22,6 @@
static atomic_t master = ATOMIC_INIT(0); static DEFINE_SPINLOCK(master_lock); -static DEFINE_PER_CPU(struct cpuidle_device, ux500_cpuidle_device);
static inline int ux500_enter_idle(struct cpuidle_device *dev, struct cpuidle_driver *drv, int index) @@ -30,8 +29,6 @@ static inline int ux500_enter_idle(struct cpuidle_device *dev, int this_cpu = smp_processor_id(); bool recouple = false;
- clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &this_cpu); - if (atomic_inc_return(&master) == num_online_cpus()) {
/* With this lock, we prevent the other cpu to exit and enter @@ -91,8 +88,6 @@ out: spin_unlock(&master_lock); }
- clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &this_cpu); - return index; }
@@ -106,7 +101,8 @@ static struct cpuidle_driver ux500_idle_driver = { .enter = ux500_enter_idle, .exit_latency = 70, .target_residency = 260, - .flags = CPUIDLE_FLAG_TIME_VALID, + .flags = CPUIDLE_FLAG_TIME_VALID | + CPUIDLE_FLAG_TIMER_STOP, .name = "ApIdle", .desc = "ARM Retention", }, @@ -115,59 +111,13 @@ static struct cpuidle_driver ux500_idle_driver = { .state_count = 2, };
-/* - * For each cpu, setup the broadcast timer because we will - * need to migrate the timers for the states >= ApIdle. - */ -static void ux500_setup_broadcast_timer(void *arg) -{ - int cpu = smp_processor_id(); - clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ON, &cpu); -} - int __init ux500_idle_init(void) { - int ret, cpu; - struct cpuidle_device *device; - /* Configure wake up reasons */ prcmu_enable_wakeups(PRCMU_WAKEUP(ARM) | PRCMU_WAKEUP(RTC) | PRCMU_WAKEUP(ABB));
- /* - * Configure the timer broadcast for each cpu, that must - * be done from the cpu context, so we use a smp cross - * call with 'on_each_cpu'. - */ - on_each_cpu(ux500_setup_broadcast_timer, NULL, 1); - - ret = cpuidle_register_driver(&ux500_idle_driver); - if (ret) { - printk(KERN_ERR "failed to register ux500 idle driver\n"); - return ret; - } - - for_each_online_cpu(cpu) { - device = &per_cpu(ux500_cpuidle_device, cpu); - device->cpu = cpu; - ret = cpuidle_register_device(device); - if (ret) { - printk(KERN_ERR "Failed to register cpuidle " - "device for cpu%d\n", cpu); - goto out_unregister; - } - } -out: - return ret; - -out_unregister: - for_each_online_cpu(cpu) { - device = &per_cpu(ux500_cpuidle_device, cpu); - cpuidle_unregister_device(device); - } - - cpuidle_unregister_driver(&ux500_idle_driver); - goto out; + return arm_idle_init(&ux500_idle_driver); }
device_initcall(ux500_idle_init);
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-omap2/cpuidle34xx.c | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-)
diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c index 80392fc..0112da3c 100644 --- a/arch/arm/mach-omap2/cpuidle34xx.c +++ b/arch/arm/mach-omap2/cpuidle34xx.c @@ -26,6 +26,7 @@ #include <linux/cpuidle.h> #include <linux/export.h> #include <linux/cpu_pm.h> +#include <asm/cpuidle.h>
#include "powerdomain.h" #include "clockdomain.h" @@ -271,8 +272,6 @@ static int omap3_enter_idle_bm(struct cpuidle_device *dev, return ret; }
-static DEFINE_PER_CPU(struct cpuidle_device, omap3_idle_dev); - static struct cpuidle_driver omap3_idle_driver = { .name = "omap3_idle", .owner = THIS_MODULE, @@ -348,8 +347,6 @@ static struct cpuidle_driver omap3_idle_driver = { */ int __init omap3_idle_init(void) { - struct cpuidle_device *dev; - mpu_pd = pwrdm_lookup("mpu_pwrdm"); core_pd = pwrdm_lookup("core_pwrdm"); per_pd = pwrdm_lookup("per_pwrdm"); @@ -358,16 +355,5 @@ int __init omap3_idle_init(void) if (!mpu_pd || !core_pd || !per_pd || !cam_pd) return -ENODEV;
- cpuidle_register_driver(&omap3_idle_driver); - - dev = &per_cpu(omap3_idle_dev, smp_processor_id()); - dev->cpu = 0; - - if (cpuidle_register_device(dev)) { - printk(KERN_ERR "%s: CPUidle register device failed\n", - __func__); - return -EIO; - } - - return 0; + return arm_idle_init(&omap3_idle_driver); }
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-davinci/cpuidle.c | 20 +------------------- 1 file changed, 1 insertion(+), 19 deletions(-)
diff --git a/arch/arm/mach-davinci/cpuidle.c b/arch/arm/mach-davinci/cpuidle.c index 5ac9e93..9968aeb 100644 --- a/arch/arm/mach-davinci/cpuidle.c +++ b/arch/arm/mach-davinci/cpuidle.c @@ -25,7 +25,6 @@
#define DAVINCI_CPUIDLE_MAX_STATES 2
-static DEFINE_PER_CPU(struct cpuidle_device, davinci_cpuidle_device); static void __iomem *ddr2_reg_base; static bool ddr2_pdown;
@@ -81,12 +80,8 @@ static struct cpuidle_driver davinci_idle_driver = {
static int __init davinci_cpuidle_probe(struct platform_device *pdev) { - int ret; - struct cpuidle_device *device; struct davinci_cpuidle_config *pdata = pdev->dev.platform_data;
- device = &per_cpu(davinci_cpuidle_device, smp_processor_id()); - if (!pdata) { dev_err(&pdev->dev, "cannot get platform data\n"); return -ENOENT; @@ -96,20 +91,7 @@ static int __init davinci_cpuidle_probe(struct platform_device *pdev)
ddr2_pdown = pdata->ddr2_pdown;
- ret = cpuidle_register_driver(&davinci_idle_driver); - if (ret) { - dev_err(&pdev->dev, "failed to register driver\n"); - return ret; - } - - ret = cpuidle_register_device(device); - if (ret) { - dev_err(&pdev->dev, "failed to register device\n"); - cpuidle_unregister_driver(&davinci_idle_driver); - return ret; - } - - return 0; + return arm_idle_init(&davinci_idle_driver); }
static struct platform_driver davinci_cpuidle_driver = {
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-at91/cpuidle.c | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-)
diff --git a/arch/arm/mach-at91/cpuidle.c b/arch/arm/mach-at91/cpuidle.c index 0c63815..9dc4f6f 100644 --- a/arch/arm/mach-at91/cpuidle.c +++ b/arch/arm/mach-at91/cpuidle.c @@ -27,8 +27,6 @@
#define AT91_MAX_STATES 2
-static DEFINE_PER_CPU(struct cpuidle_device, at91_cpuidle_device); - /* Actual code that puts the SoC in different idle states */ static int at91_enter_idle(struct cpuidle_device *dev, struct cpuidle_driver *drv, @@ -63,18 +61,7 @@ static struct cpuidle_driver at91_idle_driver = { /* Initialize CPU idle by registering the idle states */ static int at91_init_cpuidle(void) { - struct cpuidle_device *device; - - device = &per_cpu(at91_cpuidle_device, smp_processor_id()); - device->state_count = AT91_MAX_STATES; - - cpuidle_register_driver(&at91_idle_driver); - - if (cpuidle_register_device(device)) { - printk(KERN_ERR "at91_init_cpuidle: Failed registering\n"); - return -EIO; - } - return 0; + return arm_idle_init(&at91_idle_driver); }
device_initcall(at91_init_cpuidle);
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-shmobile/cpuidle.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/arch/arm/mach-shmobile/cpuidle.c b/arch/arm/mach-shmobile/cpuidle.c index 9e05026..11a0542 100644 --- a/arch/arm/mach-shmobile/cpuidle.c +++ b/arch/arm/mach-shmobile/cpuidle.c @@ -23,7 +23,6 @@ int shmobile_enter_wfi(struct cpuidle_device *dev, struct cpuidle_driver *drv, return 0; }
-static struct cpuidle_device shmobile_cpuidle_dev; static struct cpuidle_driver shmobile_cpuidle_default_driver = { .name = "shmobile_cpuidle", .owner = THIS_MODULE, @@ -43,12 +42,5 @@ void shmobile_cpuidle_set_driver(struct cpuidle_driver *drv)
int shmobile_cpuidle_init(void) { - struct cpuidle_device *dev = &shmobile_cpuidle_dev; - - cpuidle_register_driver(cpuidle_drv); - - dev->state_count = cpuidle_drv->state_count; - cpuidle_register_device(dev); - - return 0; + return arm_idle_init(cpuidle_drv); }
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-imx/Makefile | 1 - arch/arm/mach-imx/cpuidle-imx6q.c | 18 +-------- arch/arm/mach-imx/cpuidle.c | 80 ------------------------------------- arch/arm/mach-imx/cpuidle.h | 6 +-- arch/arm/mach-imx/pm-imx5.c | 3 +- 5 files changed, 4 insertions(+), 104 deletions(-) delete mode 100644 arch/arm/mach-imx/cpuidle.c
diff --git a/arch/arm/mach-imx/Makefile b/arch/arm/mach-imx/Makefile index c4ce090..9933955 100644 --- a/arch/arm/mach-imx/Makefile +++ b/arch/arm/mach-imx/Makefile @@ -30,7 +30,6 @@ obj-$(CONFIG_MXC_DEBUG_BOARD) += 3ds_debugboard.o obj-$(CONFIG_CPU_FREQ_IMX) += cpufreq.o
ifeq ($(CONFIG_CPU_IDLE),y) -obj-y += cpuidle.o obj-$(CONFIG_SOC_IMX6Q) += cpuidle-imx6q.o endif
diff --git a/arch/arm/mach-imx/cpuidle-imx6q.c b/arch/arm/mach-imx/cpuidle-imx6q.c index d533e26..f2d71f8 100644 --- a/arch/arm/mach-imx/cpuidle-imx6q.c +++ b/arch/arm/mach-imx/cpuidle-imx6q.c @@ -48,17 +48,6 @@ done: return index; }
-/* - * For each cpu, setup the broadcast timer because local timer - * stops for the states other than WFI. - */ -static void imx6q_setup_broadcast_timer(void *arg) -{ - int cpu = smp_processor_id(); - - clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ON, &cpu); -} - static struct cpuidle_driver imx6q_cpuidle_driver = { .name = "imx6q_cpuidle", .owner = THIS_MODULE, @@ -70,7 +59,7 @@ static struct cpuidle_driver imx6q_cpuidle_driver = { { .exit_latency = 50, .target_residency = 75, - .flags = CPUIDLE_FLAG_TIME_VALID, + .flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_TIMER_STOP, .enter = imx6q_enter_wait, .name = "WAIT", .desc = "Clock off", @@ -88,8 +77,5 @@ int __init imx6q_cpuidle_init(void) /* Set chicken bit to get a reliable WAIT mode support */ imx6q_set_chicken_bit();
- /* Configure the broadcast timer on each cpu */ - on_each_cpu(imx6q_setup_broadcast_timer, NULL, 1); - - return imx_cpuidle_init(&imx6q_cpuidle_driver); + return arm_idle_init(&imx6q_cpuidle_driver); } diff --git a/arch/arm/mach-imx/cpuidle.c b/arch/arm/mach-imx/cpuidle.c deleted file mode 100644 index d4cb511..0000000 --- a/arch/arm/mach-imx/cpuidle.c +++ /dev/null @@ -1,80 +0,0 @@ -/* - * Copyright 2012 Freescale Semiconductor, Inc. - * Copyright 2012 Linaro Ltd. - * - * The code contained herein is licensed under the GNU General Public - * License. You may obtain a copy of the GNU General Public License - * Version 2 or later at the following locations: - * - * http://www.opensource.org/licenses/gpl-license.html - * http://www.gnu.org/copyleft/gpl.html - */ - -#include <linux/cpuidle.h> -#include <linux/err.h> -#include <linux/hrtimer.h> -#include <linux/io.h> -#include <linux/kernel.h> -#include <linux/slab.h> - -static struct cpuidle_device __percpu * imx_cpuidle_devices; - -static void __init imx_cpuidle_devices_uninit(void) -{ - int cpu_id; - struct cpuidle_device *dev; - - for_each_possible_cpu(cpu_id) { - dev = per_cpu_ptr(imx_cpuidle_devices, cpu_id); - cpuidle_unregister_device(dev); - } - - free_percpu(imx_cpuidle_devices); -} - -int __init imx_cpuidle_init(struct cpuidle_driver *drv) -{ - struct cpuidle_device *dev; - int cpu_id, ret; - - if (drv->state_count > CPUIDLE_STATE_MAX) { - pr_err("%s: state_count exceeds maximum\n", __func__); - return -EINVAL; - } - - ret = cpuidle_register_driver(drv); - if (ret) { - pr_err("%s: Failed to register cpuidle driver with error: %d\n", - __func__, ret); - return ret; - } - - imx_cpuidle_devices = alloc_percpu(struct cpuidle_device); - if (imx_cpuidle_devices == NULL) { - ret = -ENOMEM; - goto unregister_drv; - } - - /* initialize state data for each cpuidle_device */ - for_each_possible_cpu(cpu_id) { - dev = per_cpu_ptr(imx_cpuidle_devices, cpu_id); - dev->cpu = cpu_id; - dev->state_count = drv->state_count; - - ret = cpuidle_register_device(dev); - if (ret) { - pr_err("%s: Failed to register cpu %u, error: %d\n", - __func__, cpu_id, ret); - goto uninit; - } - } - - return 0; - -uninit: - imx_cpuidle_devices_uninit(); - -unregister_drv: - cpuidle_unregister_driver(drv); - return ret; -} diff --git a/arch/arm/mach-imx/cpuidle.h b/arch/arm/mach-imx/cpuidle.h index e092d13..a15248c 100644 --- a/arch/arm/mach-imx/cpuidle.h +++ b/arch/arm/mach-imx/cpuidle.h @@ -11,15 +11,11 @@ */
#include <linux/cpuidle.h> +#include <asm/cpuidle.h>
#ifdef CONFIG_CPU_IDLE -extern int imx_cpuidle_init(struct cpuidle_driver *drv); extern int imx6q_cpuidle_init(void); #else -static inline int imx_cpuidle_init(struct cpuidle_driver *drv) -{ - return -ENODEV; -} static inline int imx6q_cpuidle_init(void) { return -ENODEV; diff --git a/arch/arm/mach-imx/pm-imx5.c b/arch/arm/mach-imx/pm-imx5.c index f67fd7e..b2d231c 100644 --- a/arch/arm/mach-imx/pm-imx5.c +++ b/arch/arm/mach-imx/pm-imx5.c @@ -193,8 +193,7 @@ static int __init imx5_pm_common_init(void) /* Set the registers to the default cpu idle state. */ mx5_cpu_lp_set(IMX5_DEFAULT_CPU_IDLE_STATE);
- imx_cpuidle_init(&imx5_cpuidle_driver); - return 0; + return arm_idle_init(&imx5_cpuidle_driver); }
void __init imx51_pm_init(void)
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-s3c64xx/cpuidle.c | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-)
diff --git a/arch/arm/mach-s3c64xx/cpuidle.c b/arch/arm/mach-s3c64xx/cpuidle.c index ead5fab..d50ffd2 100644 --- a/arch/arm/mach-s3c64xx/cpuidle.c +++ b/arch/arm/mach-s3c64xx/cpuidle.c @@ -16,6 +16,7 @@ #include <linux/export.h> #include <linux/time.h>
+#include <asm/cpuidle.h> #include <asm/proc-fns.h>
#include <mach/map.h> @@ -40,8 +41,6 @@ static int s3c64xx_enter_idle(struct cpuidle_device *dev, return index; }
-static DEFINE_PER_CPU(struct cpuidle_device, s3c64xx_cpuidle_device); - static struct cpuidle_driver s3c64xx_cpuidle_driver = { .name = "s3c64xx_cpuidle", .owner = THIS_MODULE, @@ -61,16 +60,6 @@ static struct cpuidle_driver s3c64xx_cpuidle_driver = {
static int __init s3c64xx_init_cpuidle(void) { - int ret; - - cpuidle_register_driver(&s3c64xx_cpuidle_driver); - - ret = cpuidle_register_device(&s3c64xx_cpuidle_device); - if (ret) { - pr_err("Failed to register cpuidle device: %d\n", ret); - return ret; - } - - return 0; + return arm_idle_init(&s3c64xx_cpuidle_driver); } device_initcall(s3c64xx_init_cpuidle);
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- drivers/cpuidle/cpuidle-calxeda.c | 52 +------------------------------------ 1 file changed, 1 insertion(+), 51 deletions(-)
diff --git a/drivers/cpuidle/cpuidle-calxeda.c b/drivers/cpuidle/cpuidle-calxeda.c index e1aab38..5db7935 100644 --- a/drivers/cpuidle/cpuidle-calxeda.c +++ b/drivers/cpuidle/cpuidle-calxeda.c @@ -35,8 +35,6 @@ extern void highbank_set_cpu_jump(int cpu, void *jump_addr); extern void *scu_base_addr;
-static struct cpuidle_device __percpu *calxeda_idle_cpuidle_devices; - static inline unsigned int get_auxcr(void) { unsigned int val; @@ -85,19 +83,6 @@ static int calxeda_pwrdown_idle(struct cpuidle_device *dev, return index; }
-static void calxeda_idle_cpuidle_devices_uninit(void) -{ - int i; - struct cpuidle_device *dev; - - for_each_possible_cpu(i) { - dev = per_cpu_ptr(calxeda_idle_cpuidle_devices, i); - cpuidle_unregister_device(dev); - } - - free_percpu(calxeda_idle_cpuidle_devices); -} - static struct cpuidle_driver calxeda_idle_driver = { .name = "calxeda_idle", .en_core_tk_irqen = 1, @@ -118,44 +103,9 @@ static struct cpuidle_driver calxeda_idle_driver = {
static int __init calxeda_cpuidle_init(void) { - int cpu_id; - int ret; - struct cpuidle_device *dev; - struct cpuidle_driver *drv = &calxeda_idle_driver; - if (!of_machine_is_compatible("calxeda,highbank")) return -ENODEV;
- ret = cpuidle_register_driver(drv); - if (ret) - return ret; - - calxeda_idle_cpuidle_devices = alloc_percpu(struct cpuidle_device); - if (calxeda_idle_cpuidle_devices == NULL) { - ret = -ENOMEM; - goto unregister_drv; - } - - /* initialize state data for each cpuidle_device */ - for_each_possible_cpu(cpu_id) { - dev = per_cpu_ptr(calxeda_idle_cpuidle_devices, cpu_id); - dev->cpu = cpu_id; - dev->state_count = drv->state_count; - - ret = cpuidle_register_device(dev); - if (ret) { - pr_err("Failed to register cpu %u, error: %d\n", - cpu_id, ret); - goto uninit; - } - } - - return 0; - -uninit: - calxeda_idle_cpuidle_devices_uninit(); -unregister_drv: - cpuidle_unregister_driver(drv); - return ret; + return arm_idle_init(&calxeda_idle_driver); } module_init(calxeda_cpuidle_init);
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- drivers/cpuidle/cpuidle-kirkwood.c | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-)
diff --git a/drivers/cpuidle/cpuidle-kirkwood.c b/drivers/cpuidle/cpuidle-kirkwood.c index 670aa1e..60efdc5 100644 --- a/drivers/cpuidle/cpuidle-kirkwood.c +++ b/drivers/cpuidle/cpuidle-kirkwood.c @@ -53,9 +53,6 @@ static struct cpuidle_driver kirkwood_idle_driver = { }, .state_count = KIRKWOOD_MAX_STATES, }; -static struct cpuidle_device *device; - -static DEFINE_PER_CPU(struct cpuidle_device, kirkwood_cpuidle_device);
/* Initialize CPU idle by registering the idle states */ static int kirkwood_cpuidle_probe(struct platform_device *pdev) @@ -70,22 +67,12 @@ static int kirkwood_cpuidle_probe(struct platform_device *pdev) if (!ddr_operation_base) return -EADDRNOTAVAIL;
- device = &per_cpu(kirkwood_cpuidle_device, smp_processor_id()); - device->state_count = KIRKWOOD_MAX_STATES; - - cpuidle_register_driver(&kirkwood_idle_driver); - if (cpuidle_register_device(device)) { - pr_err("kirkwood_init_cpuidle: Failed registering\n"); - return -EIO; - } - return 0; + return arm_idle_init(&kirkwood_idle_driver); }
int kirkwood_cpuidle_remove(struct platform_device *pdev) { - cpuidle_unregister_device(device); - cpuidle_unregister_driver(&kirkwood_idle_driver); - + arm_idle_exit(&kirkwood_idle_driver); return 0; }
linaro-kernel@lists.linaro.org
-
Andrew Lunn -
Arnd Bergmann -
Catalin Marinas -
Daniel Lezcano -
Rafael J. Wysocki -
Santosh Shilimkar