| Message ID | 1363357630-22214-3-git-send-email-daniel.lezcano@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 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> 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 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> > > 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 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> >> >> 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 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> >>> >>> 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: > + * > + * 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); > 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;a=shortlog;h=refs/heads/linux-pm-next 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
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);
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