The flag CPUIDLE_FLAG_TIMER_STOP has been introduced in the commit 89878baa73f0f1c679355006bd8632e5d78f96c2.
The flag tells the cpuidle framework the local timer will stop in the idle state.
It is now easy to know if the cpuidle driver will use or not the broadcast timer by looking at the different states for this flag and then setup the broadcast timer consequently.
When we remove the timer initialization duplicated code in the different drivers, we have most of the drivers with the same init function. This init function is changed to be generic and moved in the ARM cpuidle driver and used from the drivers. That cleanups code and removes a lot of annoying duplicated code.
There is still some modification in OMAP4, tegra2, tegra3 and imx, especially around the coupled idle states, but we are more and more closer to a common squeleton for all the ARM drivers.
Daniel Lezcano (15): timer: move enum definition out of ifdef section cpuidle: initialize the broadcast timer framework cpuidle: ux500: remove timer broadcast initialization cpuidle: OMAP4: remove timer broadcast initialization cpuidle: imx6: remove timer broadcast initialization ARM: cpuidle: remove useless declaration ARM: cpuidle: add init/exit routine ARM: ux500: cpuidle: use init/exit common routine ARM: at91: cpuidle: use init/exit common routine ARM: OMAP3: cpuidle: use init/exit common routine ARM: s3c64xx: cpuidle: use init/exit common routine ARM: tegra1: cpuidle: use init/exit common routine ARM: shmobile: pm: fix init sections ARM: shmobile: cpuidle: remove useless WFI function ARM: shmobile: cpuidle: use init/exit common routine
arch/arm/include/asm/cpuidle.h | 11 +++--- arch/arm/kernel/cpuidle.c | 57 +++++++++++++++++++++++++++++++- arch/arm/mach-at91/cpuidle.c | 17 ++-------- arch/arm/mach-imx/cpuidle-imx6q.c | 15 --------- arch/arm/mach-omap2/cpuidle34xx.c | 18 ++-------- arch/arm/mach-omap2/cpuidle44xx.c | 14 -------- arch/arm/mach-s3c64xx/cpuidle.c | 15 ++------- arch/arm/mach-shmobile/cpuidle.c | 22 ++---------- arch/arm/mach-shmobile/pm-sh7372.c | 4 +-- arch/arm/mach-tegra/cpuidle-tegra114.c | 27 +-------------- arch/arm/mach-ux500/cpuidle.c | 50 +--------------------------- drivers/cpuidle/driver.c | 35 ++++++++++++++++++-- include/linux/clockchips.h | 22 ++++++------ include/linux/cpuidle.h | 2 ++ 14 files changed, 120 insertions(+), 189 deletions(-)
The next patch will setup automatically the broadcast timer for the different cpuidle driver when one idle state stops its timer. This will be part of the generic code.
But some ARM boards, like s3c64xx, uses cpuidle but without the CONFIG_GENERIC_CLOCKEVENTS_BUILD set. Hence the cpuidle framework will be compiled with the code supposed to be generic, that is with clockevents_notify and the different enum, but will fail to compile because the enum is not defined.
Also the function clockevents_notify is a noop macro, this is fine except the usual code is:
int cpu = smp_processor_id(); clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ON, &cpu);
and that raises a warning telling the variable 'cpu' is not used.
Move the clock_event_nofitiers enum definition out of the CONFIG_GENERIC_CLOCKEVENTS_BUILD section to prevent a compilation error when these are used in the code.
Change the clockevents_notify macro to a static inline noop function to prevent a compilation warning.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- include/linux/clockchips.h | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h index 6634652..ba661e9 100644 --- a/include/linux/clockchips.h +++ b/include/linux/clockchips.h @@ -8,15 +8,6 @@ #ifndef _LINUX_CLOCKCHIPS_H #define _LINUX_CLOCKCHIPS_H
-#ifdef CONFIG_GENERIC_CLOCKEVENTS_BUILD - -#include <linux/clocksource.h> -#include <linux/cpumask.h> -#include <linux/ktime.h> -#include <linux/notifier.h> - -struct clock_event_device; - /* Clock event mode commands */ enum clock_event_mode { CLOCK_EVT_MODE_UNUSED = 0, @@ -40,6 +31,15 @@ enum clock_event_nofitiers { CLOCK_EVT_NOTIFY_CPU_DEAD, };
+#ifdef CONFIG_GENERIC_CLOCKEVENTS_BUILD + +#include <linux/clocksource.h> +#include <linux/cpumask.h> +#include <linux/ktime.h> +#include <linux/notifier.h> + +struct clock_event_device; + /* * Clock event features */ @@ -173,7 +173,7 @@ extern int tick_receive_broadcast(void); #ifdef CONFIG_GENERIC_CLOCKEVENTS extern void clockevents_notify(unsigned long reason, void *arg); #else -# define clockevents_notify(reason, arg) do { } while (0) +static inline void clockevents_notify(unsigned long reason, void *arg) {} #endif
#else /* CONFIG_GENERIC_CLOCKEVENTS_BUILD */ @@ -181,7 +181,7 @@ extern void clockevents_notify(unsigned long reason, void *arg); static inline void clockevents_suspend(void) {} static inline void clockevents_resume(void) {}
-#define clockevents_notify(reason, arg) do { } while (0) +static inline void clockevents_notify(unsigned long reason, void *arg) {}
#endif
On Monday 25 March 2013 11:25 PM, Daniel Lezcano wrote:
The next patch will setup automatically the broadcast timer for the different cpuidle driver when one idle state stops its timer. This will be part of the generic code.
But some ARM boards, like s3c64xx, uses cpuidle but without the CONFIG_GENERIC_CLOCKEVENTS_BUILD set. Hence the cpuidle framework will be compiled with the code supposed to be generic, that is with clockevents_notify and the different enum, but will fail to compile because the enum is not defined.
Also the function clockevents_notify is a noop macro, this is fine except the usual code is:
int cpu = smp_processor_id(); clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ON, &cpu);
and that raises a warning telling the variable 'cpu' is not used.
Move the clock_event_nofitiers enum definition out of the CONFIG_GENERIC_CLOCKEVENTS_BUILD section to prevent a compilation error when these are used in the code.
Change the clockevents_notify macro to a static inline noop function to prevent a compilation warning.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
So why can't s3c64xx enable "CONFIG_GENERIC_CLOCKEVENTS_BUILD" if they wants to use broadcast notfiers ? May be am missing something.
Regards, Santosh
On 03/26/2013 05:36 AM, Santosh Shilimkar wrote:
On Monday 25 March 2013 11:25 PM, Daniel Lezcano wrote:
The next patch will setup automatically the broadcast timer for the different cpuidle driver when one idle state stops its timer. This will be part of the generic code.
But some ARM boards, like s3c64xx, uses cpuidle but without the CONFIG_GENERIC_CLOCKEVENTS_BUILD set. Hence the cpuidle framework will be compiled with the code supposed to be generic, that is with clockevents_notify and the different enum, but will fail to compile because the enum is not defined.
Also the function clockevents_notify is a noop macro, this is fine except the usual code is:
int cpu = smp_processor_id(); clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ON, &cpu);
and that raises a warning telling the variable 'cpu' is not used.
Move the clock_event_nofitiers enum definition out of the CONFIG_GENERIC_CLOCKEVENTS_BUILD section to prevent a compilation error when these are used in the code.
Change the clockevents_notify macro to a static inline noop function to prevent a compilation warning.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
So why can't s3c64xx enable "CONFIG_GENERIC_CLOCKEVENTS_BUILD" if they wants to use broadcast notfiers ? May be am missing something.
They won't use broadcast notifiers but the cpuidle code 'drivers/cpuidle/cpuidle.c' has a generic code using it. It does not compile because an enum definition is inside the block CONFIG_GENERIC_CLOCKEVENTS_BUILD.
... if (drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP) clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu); ...
clockevents_notify will be a noop (this is ok), but CLOCK_EVT_NOTIFY_BROADCAST_ENTER won't be defined, so error at compile time.
The commit 89878baa73f0f1c679355006bd8632e5d78f96c2 introduced the CPUIDLE_FLAG_TIMER_STOP flag where we specify a specific idle state stops the local timer.
Now use this flag to check at init time if one state will need the broadcast timer and, in this case, setup the broadcast timer framework. That prevents multiple code duplication in the drivers.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- drivers/cpuidle/driver.c | 35 +++++++++++++++++++++++++++++++++-- include/linux/cpuidle.h | 2 ++ 2 files changed, 35 insertions(+), 2 deletions(-)
diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c index 422c7b6..476ce0c 100644 --- a/drivers/cpuidle/driver.c +++ b/drivers/cpuidle/driver.c @@ -11,6 +11,8 @@ #include <linux/mutex.h> #include <linux/module.h> #include <linux/cpuidle.h> +#include <linux/cpumask.h> +#include <linux/clockchips.h>
#include "cpuidle.h"
@@ -19,9 +21,30 @@ DEFINE_SPINLOCK(cpuidle_driver_lock); static void __cpuidle_set_cpu_driver(struct cpuidle_driver *drv, int cpu); static struct cpuidle_driver * __cpuidle_get_cpu_driver(int cpu);
-static void __cpuidle_driver_init(struct cpuidle_driver *drv) +static void cpuidle_setup_broadcast_timer(void *arg) { + int cpu = smp_processor_id(); + clockevents_notify((long)(arg), &cpu); +} + +static void __cpuidle_driver_init(struct cpuidle_driver *drv, int cpu) +{ + int i; + drv->refcnt = 0; + + for (i = drv->state_count - 1; i >= 0 ; i--) { + + if (!(drv->states[i].flags & CPUIDLE_FLAG_TIMER_STOP)) + continue; + + drv->bctimer = 1; + + on_each_cpu_mask(get_cpu_mask(cpu), cpuidle_setup_broadcast_timer, + (void *)CLOCK_EVT_NOTIFY_BROADCAST_ON, 1); + + break; + } }
static int __cpuidle_register_driver(struct cpuidle_driver *drv, int cpu) @@ -35,7 +58,7 @@ static int __cpuidle_register_driver(struct cpuidle_driver *drv, int cpu) if (__cpuidle_get_cpu_driver(cpu)) return -EBUSY;
- __cpuidle_driver_init(drv); + __cpuidle_driver_init(drv, cpu);
__cpuidle_set_cpu_driver(drv, cpu);
@@ -49,6 +72,14 @@ static void __cpuidle_unregister_driver(struct cpuidle_driver *drv, int cpu)
if (!WARN_ON(drv->refcnt > 0)) __cpuidle_set_cpu_driver(NULL, cpu); + + if (drv->bctimer) { + + drv->bctimer = 0; + + on_each_cpu_mask(get_cpu_mask(cpu), cpuidle_setup_broadcast_timer, + (void *)CLOCK_EVT_NOTIFY_BROADCAST_OFF, 1); + } }
#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index a837b33..fc3e580 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -107,6 +107,8 @@ struct cpuidle_driver {
/* set to 1 to use the core cpuidle time keeping (for all states). */ unsigned int en_core_tk_irqen:1; + /* used by the cpuidle framework to setup the broadcast timer */ + unsigned int bctimer:1; /* states array must be ordered in decreasing power consumption */ struct cpuidle_state states[CPUIDLE_STATE_MAX]; int state_count;
On Monday 25 March 2013 11:25 PM, Daniel Lezcano wrote:
The commit 89878baa73f0f1c679355006bd8632e5d78f96c2 introduced the CPUIDLE_FLAG_TIMER_STOP flag where we specify a specific idle state stops the local timer.
Now use this flag to check at init time if one state will need the broadcast timer and, in this case, setup the broadcast timer framework. That prevents multiple code duplication in the drivers.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
drivers/cpuidle/driver.c | 35 +++++++++++++++++++++++++++++++++-- include/linux/cpuidle.h | 2 ++ 2 files changed, 35 insertions(+), 2 deletions(-)
Make sense. Minor comments.
diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c index 422c7b6..476ce0c 100644 --- a/drivers/cpuidle/driver.c +++ b/drivers/cpuidle/driver.c @@ -11,6 +11,8 @@ #include <linux/mutex.h> #include <linux/module.h> #include <linux/cpuidle.h> +#include <linux/cpumask.h> +#include <linux/clockchips.h> #include "cpuidle.h" @@ -19,9 +21,30 @@ DEFINE_SPINLOCK(cpuidle_driver_lock); static void __cpuidle_set_cpu_driver(struct cpuidle_driver *drv, int cpu); static struct cpuidle_driver * __cpuidle_get_cpu_driver(int cpu); -static void __cpuidle_driver_init(struct cpuidle_driver *drv) +static void cpuidle_setup_broadcast_timer(void *arg) {
- int cpu = smp_processor_id();
- clockevents_notify((long)(arg), &cpu);
+}
+static void __cpuidle_driver_init(struct cpuidle_driver *drv, int cpu) +{
- int i;
Un-necessary extra line..
drv->refcnt = 0;
- for (i = drv->state_count - 1; i >= 0 ; i--) {
here as well
if (!(drv->states[i].flags & CPUIDLE_FLAG_TIMER_STOP))continue;drv->bctimer = 1;
ditto
on_each_cpu_mask(get_cpu_mask(cpu), cpuidle_setup_broadcast_timer,(void *)CLOCK_EVT_NOTIFY_BROADCAST_ON, 1);
ditto
break;- }
} static int __cpuidle_register_driver(struct cpuidle_driver *drv, int cpu) @@ -35,7 +58,7 @@ static int __cpuidle_register_driver(struct cpuidle_driver *drv, int cpu) if (__cpuidle_get_cpu_driver(cpu)) return -EBUSY;
- __cpuidle_driver_init(drv);
- __cpuidle_driver_init(drv, cpu);
__cpuidle_set_cpu_driver(drv, cpu); @@ -49,6 +72,14 @@ static void __cpuidle_unregister_driver(struct cpuidle_driver *drv, int cpu) if (!WARN_ON(drv->refcnt > 0)) __cpuidle_set_cpu_driver(NULL, cpu);
- if (drv->bctimer) {
drv->bctimer = 0;
no need of extra line here on_each_cpu_mask(get_cpu_mask(cpu), cpuidle_setup_broadcast_timer,
(void *)CLOCK_EVT_NOTIFY_BROADCAST_OFF, 1);- }
} #ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index a837b33..fc3e580 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -107,6 +107,8 @@ struct cpuidle_driver { /* set to 1 to use the core cpuidle time keeping (for all states). */ unsigned int en_core_tk_irqen:1;
/* used by the cpuidle framework to setup the broadcast timer */- unsigned int bctimer:1; /* states array must be ordered in decreasing power consumption */ struct cpuidle_state states[CPUIDLE_STATE_MAX]; int state_count;
The initialization is done from the cpuidle framework.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-ux500/cpuidle.c | 18 ------------------ 1 file changed, 18 deletions(-)
diff --git a/arch/arm/mach-ux500/cpuidle.c b/arch/arm/mach-ux500/cpuidle.c index 6d0c4b6..1b16d9e 100644 --- a/arch/arm/mach-ux500/cpuidle.c +++ b/arch/arm/mach-ux500/cpuidle.c @@ -11,7 +11,6 @@
#include <linux/module.h> #include <linux/cpuidle.h> -#include <linux/clockchips.h> #include <linux/spinlock.h> #include <linux/atomic.h> #include <linux/smp.h> @@ -112,16 +111,6 @@ 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; @@ -131,13 +120,6 @@ int __init ux500_idle_init(void) 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");
On Mon, Mar 25, 2013 at 6:55 PM, Daniel Lezcano daniel.lezcano@linaro.org wrote:
The initialization is done from the cpuidle framework.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
Acked-by: Linus Walleij linus.walleij@linaro.org
Yours, Linus Walleij
The initialization is done from the cpuidle framework.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-omap2/cpuidle44xx.c | 14 -------------- 1 file changed, 14 deletions(-)
diff --git a/arch/arm/mach-omap2/cpuidle44xx.c b/arch/arm/mach-omap2/cpuidle44xx.c index fe0e025..f4b1b23 100644 --- a/arch/arm/mach-omap2/cpuidle44xx.c +++ b/arch/arm/mach-omap2/cpuidle44xx.c @@ -14,7 +14,6 @@ #include <linux/cpuidle.h> #include <linux/cpu_pm.h> #include <linux/export.h> -#include <linux/clockchips.h>
#include <asm/proc-fns.h>
@@ -158,16 +157,6 @@ fail: return index; }
-/* - * For each cpu, setup the broadcast timer because local timers - * stops for the states above C1. - */ -static void omap_setup_broadcast_timer(void *arg) -{ - int cpu = smp_processor_id(); - clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ON, &cpu); -} - static DEFINE_PER_CPU(struct cpuidle_device, omap4_idle_dev);
static struct cpuidle_driver omap4_idle_driver = { @@ -233,9 +222,6 @@ int __init omap4_idle_init(void) if (!cpu_clkdm[0] || !cpu_clkdm[1]) return -ENODEV;
- /* Configure the broadcast timer on each cpu */ - on_each_cpu(omap_setup_broadcast_timer, NULL, 1); - for_each_cpu(cpu_id, cpu_online_mask) { dev = &per_cpu(omap4_idle_dev, cpu_id); dev->cpu = cpu_id;
On Monday 25 March 2013 11:25 PM, Daniel Lezcano wrote:
The initialization is done from the cpuidle framework.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
arch/arm/mach-omap2/cpuidle44xx.c | 14 -------------- 1 file changed, 14 deletions(-)
Acked-by: Santosh Shilimkar santosh.shilimkar@ti.com
The initialization is done from the cpuidle framework.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-imx/cpuidle-imx6q.c | 15 --------------- 1 file changed, 15 deletions(-)
diff --git a/arch/arm/mach-imx/cpuidle-imx6q.c b/arch/arm/mach-imx/cpuidle-imx6q.c index 5ae22f7..a783a63 100644 --- a/arch/arm/mach-imx/cpuidle-imx6q.c +++ b/arch/arm/mach-imx/cpuidle-imx6q.c @@ -6,7 +6,6 @@ * published by the Free Software Foundation. */
-#include <linux/clockchips.h> #include <linux/cpuidle.h> #include <linux/module.h> #include <asm/cpuidle.h> @@ -43,17 +42,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, @@ -84,8 +72,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); }
On Mon, Mar 25, 2013 at 06:55:30PM +0100, Daniel Lezcano wrote:
The initialization is done from the cpuidle framework.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
arch/arm/mach-imx/cpuidle-imx6q.c | 15 --------------- 1 file changed, 15 deletions(-)
diff --git a/arch/arm/mach-imx/cpuidle-imx6q.c b/arch/arm/mach-imx/cpuidle-imx6q.c index 5ae22f7..a783a63 100644 --- a/arch/arm/mach-imx/cpuidle-imx6q.c +++ b/arch/arm/mach-imx/cpuidle-imx6q.c @@ -6,7 +6,6 @@
- published by the Free Software Foundation.
*/ -#include <linux/clockchips.h>
Removing this causes the compile error below.
CC arch/arm/mach-imx/cpuidle-imx6q.o arch/arm/mach-imx/cpuidle-imx6q.c: In function ‘imx6q_enter_wait’: arch/arm/mach-imx/cpuidle-imx6q.c:25:2: error: implicit declaration of function ‘clockevents_notify’ [-Werror=implicit-function-declaration] arch/arm/mach-imx/cpuidle-imx6q.c:25:21: error: ‘CLOCK_EVT_NOTIFY_BROADCAST_ENTER’ undeclared (first use in this function) arch/arm/mach-imx/cpuidle-imx6q.c:25:21: note: each undeclared identifier is reported only once for each function it appears in arch/arm/mach-imx/cpuidle-imx6q.c:45:21: error: ‘CLOCK_EVT_NOTIFY_BROADCAST_EXIT’ undeclared (first use in this function) cc1: some warnings being treated as errors make[2]: *** [arch/arm/mach-imx/cpuidle-imx6q.o] Error 1
Shawn
#include <linux/cpuidle.h> #include <linux/module.h> #include <asm/cpuidle.h> @@ -43,17 +42,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, @@ -84,8 +72,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);
}
1.7.9.5
-- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/26/2013 08:25 AM, Shawn Guo wrote:
On Mon, Mar 25, 2013 at 06:55:30PM +0100, Daniel Lezcano wrote:
The initialization is done from the cpuidle framework.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
arch/arm/mach-imx/cpuidle-imx6q.c | 15 --------------- 1 file changed, 15 deletions(-)
diff --git a/arch/arm/mach-imx/cpuidle-imx6q.c b/arch/arm/mach-imx/cpuidle-imx6q.c index 5ae22f7..a783a63 100644 --- a/arch/arm/mach-imx/cpuidle-imx6q.c +++ b/arch/arm/mach-imx/cpuidle-imx6q.c @@ -6,7 +6,6 @@
- published by the Free Software Foundation.
*/ -#include <linux/clockchips.h>
Removing this causes the compile error below.
CC arch/arm/mach-imx/cpuidle-imx6q.o arch/arm/mach-imx/cpuidle-imx6q.c: In function ‘imx6q_enter_wait’: arch/arm/mach-imx/cpuidle-imx6q.c:25:2: error: implicit declaration of function ‘clockevents_notify’ [-Werror=implicit-function-declaration] arch/arm/mach-imx/cpuidle-imx6q.c:25:21: error: ‘CLOCK_EVT_NOTIFY_BROADCAST_ENTER’ undeclared (first use in this function) arch/arm/mach-imx/cpuidle-imx6q.c:25:21: note: each undeclared identifier is reported only once for each function it appears in arch/arm/mach-imx/cpuidle-imx6q.c:45:21: error: ‘CLOCK_EVT_NOTIFY_BROADCAST_EXIT’ undeclared (first use in this function) cc1: some warnings being treated as errors make[2]: *** [arch/arm/mach-imx/cpuidle-imx6q.o] Error 1
Hi Shawn,
actually this is because the patch applies on top of:
commit 12e849504b861470e5bf991f79c0be71b97c8dac Author: Daniel Lezcano daniel.lezcano@linaro.org Date: Thu Mar 21 12:21:33 2013 +0000
cpuidle / imx6 : use CPUIDLE_FLAG_TIMER_STOP flag
Use the CPUIDLE_FLAG_TIMER_STOP and let the cpuidle framework to handle the CLOCK_EVT_NOTIFY_BROADCAST_ENTER/EXIT when entering this state.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org Acked-by: Santosh Shilimkar santosh.shilimkar@ti.com Signed-off-by: Rafael J. Wysocki rafael.j.wysocki@intel.com
On Tue, Mar 26, 2013 at 10:43:29AM +0100, Daniel Lezcano wrote:
Hi Shawn,
actually this is because the patch applies on top of:
Ah, ok. It will be helpful for people who want to test the series on their platform, if you can publish a branch for that.
Shawn
commit 12e849504b861470e5bf991f79c0be71b97c8dac Author: Daniel Lezcano daniel.lezcano@linaro.org Date: Thu Mar 21 12:21:33 2013 +0000
cpuidle / imx6 : use CPUIDLE_FLAG_TIMER_STOP flag Use the CPUIDLE_FLAG_TIMER_STOP and let the cpuidle framework to handle the CLOCK_EVT_NOTIFY_BROADCAST_ENTER/EXIT when entering this state. Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
On 03/26/2013 01:40 PM, Shawn Guo wrote:
On Tue, Mar 26, 2013 at 10:43:29AM +0100, Daniel Lezcano wrote:
Hi Shawn,
actually this is because the patch applies on top of:
Ah, ok. It will be helpful for people who want to test the series on their platform, if you can publish a branch for that.
Yes, sure.
It is in Rafael's tree.
https://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git
in linux-next branch.
Thanks for the review
-- Daniel
commit 12e849504b861470e5bf991f79c0be71b97c8dac Author: Daniel Lezcano daniel.lezcano@linaro.org Date: Thu Mar 21 12:21:33 2013 +0000
cpuidle / imx6 : use CPUIDLE_FLAG_TIMER_STOP flag Use the CPUIDLE_FLAG_TIMER_STOP and let the cpuidle framework to handle the CLOCK_EVT_NOTIFY_BROADCAST_ENTER/EXIT when entering this state. Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
On Tuesday, March 26, 2013 01:53:34 PM Daniel Lezcano wrote:
On 03/26/2013 01:40 PM, Shawn Guo wrote:
On Tue, Mar 26, 2013 at 10:43:29AM +0100, Daniel Lezcano wrote:
Hi Shawn,
actually this is because the patch applies on top of:
Ah, ok. It will be helpful for people who want to test the series on their platform, if you can publish a branch for that.
Yes, sure.
It is in Rafael's tree.
https://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git
in linux-next branch.
IOW, the patchset is based on the current linux-next. :-)
Thanks, Rafael
commit 12e849504b861470e5bf991f79c0be71b97c8dac Author: Daniel Lezcano daniel.lezcano@linaro.org Date: Thu Mar 21 12:21:33 2013 +0000
cpuidle / imx6 : use CPUIDLE_FLAG_TIMER_STOP flag Use the CPUIDLE_FLAG_TIMER_STOP and let the cpuidle framework to handle the CLOCK_EVT_NOTIFY_BROADCAST_ENTER/EXIT when entering this state. Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
On Mon, Mar 25, 2013 at 06:55:30PM +0100, Daniel Lezcano wrote:
The initialization is done from the cpuidle framework.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
Acked-by: Shawn Guo shawn.guo@linaro.org
arch/arm/mach-imx/cpuidle-imx6q.c | 15 --------------- 1 file changed, 15 deletions(-)
diff --git a/arch/arm/mach-imx/cpuidle-imx6q.c b/arch/arm/mach-imx/cpuidle-imx6q.c index 5ae22f7..a783a63 100644 --- a/arch/arm/mach-imx/cpuidle-imx6q.c +++ b/arch/arm/mach-imx/cpuidle-imx6q.c @@ -6,7 +6,6 @@
- published by the Free Software Foundation.
*/ -#include <linux/clockchips.h> #include <linux/cpuidle.h> #include <linux/module.h> #include <asm/cpuidle.h> @@ -43,17 +42,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, @@ -84,8 +72,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);
}
1.7.9.5
-- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
The noop functions code is not necessary because the header file is included in files which are compiled when CONFIG_CPU_IDLE is on.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/include/asm/cpuidle.h | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/arch/arm/include/asm/cpuidle.h b/arch/arm/include/asm/cpuidle.h index 2fca60a..7367787 100644 --- a/arch/arm/include/asm/cpuidle.h +++ b/arch/arm/include/asm/cpuidle.h @@ -1,13 +1,8 @@ #ifndef __ASM_ARM_CPUIDLE_H #define __ASM_ARM_CPUIDLE_H
-#ifdef CONFIG_CPU_IDLE extern int arm_cpuidle_simple_enter(struct cpuidle_device *dev, - struct cpuidle_driver *drv, int index); -#else -static inline int arm_cpuidle_simple_enter(struct cpuidle_device *dev, - struct cpuidle_driver *drv, int index) { return -ENODEV; } -#endif + struct cpuidle_driver *drv, int index);
/* Common ARM WFI state */ #define ARM_CPUIDLE_WFI_STATE_PWR(p) {\
The init and exit routine for most of the drivers are the same, that is register the driver and register the device.
Provide a common function to do that in the cpuidle driver for ARM, so we can get rid of a lot of code duplication in the different SOC cpuidle drivers.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/include/asm/cpuidle.h | 4 +++ arch/arm/kernel/cpuidle.c | 57 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 60 insertions(+), 1 deletion(-)
diff --git a/arch/arm/include/asm/cpuidle.h b/arch/arm/include/asm/cpuidle.h index 7367787..83a38ac 100644 --- a/arch/arm/include/asm/cpuidle.h +++ b/arch/arm/include/asm/cpuidle.h @@ -4,6 +4,10 @@ extern int arm_cpuidle_simple_enter(struct cpuidle_device *dev, struct cpuidle_driver *drv, int index);
+extern int arm_cpuidle_init(struct cpuidle_driver *drv); + +extern void arm_cpuidle_exit(struct cpuidle_driver *drv); + /* Common ARM WFI state */ #define ARM_CPUIDLE_WFI_STATE_PWR(p) {\ .enter = arm_cpuidle_simple_enter,\ diff --git a/arch/arm/kernel/cpuidle.c b/arch/arm/kernel/cpuidle.c index 89545f6..13cfe3e 100644 --- a/arch/arm/kernel/cpuidle.c +++ b/arch/arm/kernel/cpuidle.c @@ -9,13 +9,68 @@ * http://www.gnu.org/copyleft/gpl.html */
+#include <linux/module.h> #include <linux/cpuidle.h> #include <asm/proc-fns.h>
+static DEFINE_PER_CPU(struct cpuidle_device, cpuidle_device); + int arm_cpuidle_simple_enter(struct cpuidle_device *dev, - struct cpuidle_driver *drv, int index) + struct cpuidle_driver *drv, int index) { cpu_do_idle();
return index; } + +int __init arm_cpuidle_init(struct cpuidle_driver *drv) +{ + int ret, cpu; + struct cpuidle_device *device; + + 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_cpuidle_init); + +void __exit arm_cpuidle_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); +} +EXPORT_SYMBOL_GPL(arm_cpuidle_exit);
On Mon, Mar 25, 2013 at 06:55:32PM +0100, Daniel Lezcano wrote:
The init and exit routine for most of the drivers are the same, that is register the driver and register the device.
Provide a common function to do that in the cpuidle driver for ARM, so we can get rid of a lot of code duplication in the different SOC cpuidle drivers.
Hi Daniel
Please could you add a comment in the code about which piece is specific to ARM, because its not obvious to me. Its not like there is a reference to WFI for example. It looks like this code could go in drivers/cpuidle/cpuidle.c
Thanks Andrew
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
arch/arm/include/asm/cpuidle.h | 4 +++ arch/arm/kernel/cpuidle.c | 57 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 60 insertions(+), 1 deletion(-)
diff --git a/arch/arm/include/asm/cpuidle.h b/arch/arm/include/asm/cpuidle.h index 7367787..83a38ac 100644 --- a/arch/arm/include/asm/cpuidle.h +++ b/arch/arm/include/asm/cpuidle.h @@ -4,6 +4,10 @@ extern int arm_cpuidle_simple_enter(struct cpuidle_device *dev, struct cpuidle_driver *drv, int index); +extern int arm_cpuidle_init(struct cpuidle_driver *drv);
+extern void arm_cpuidle_exit(struct cpuidle_driver *drv);
/* Common ARM WFI state */ #define ARM_CPUIDLE_WFI_STATE_PWR(p) {\ .enter = arm_cpuidle_simple_enter,\ diff --git a/arch/arm/kernel/cpuidle.c b/arch/arm/kernel/cpuidle.c index 89545f6..13cfe3e 100644 --- a/arch/arm/kernel/cpuidle.c +++ b/arch/arm/kernel/cpuidle.c @@ -9,13 +9,68 @@
*/ +#include <linux/module.h> #include <linux/cpuidle.h> #include <asm/proc-fns.h> +static DEFINE_PER_CPU(struct cpuidle_device, cpuidle_device);
int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
struct cpuidle_driver *drv, int index)
struct cpuidle_driver *drv, int index){ cpu_do_idle(); return index; }
+int __init arm_cpuidle_init(struct cpuidle_driver *drv) +{
int ret, cpu;struct cpuidle_device *device;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_cpuidle_init);
+void __exit arm_cpuidle_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);
+}
+EXPORT_SYMBOL_GPL(arm_cpuidle_exit);
1.7.9.5
On 03/25/2013 07:10 PM, Andrew Lunn wrote:
On Mon, Mar 25, 2013 at 06:55:32PM +0100, Daniel Lezcano wrote:
The init and exit routine for most of the drivers are the same, that is register the driver and register the device.
Provide a common function to do that in the cpuidle driver for ARM, so we can get rid of a lot of code duplication in the different SOC cpuidle drivers.
Hi Daniel
Please could you add a comment in the code about which piece is specific to ARM, because its not obvious to me. Its not like there is a reference to WFI for example. It looks like this code could go in drivers/cpuidle/cpuidle.c
Yes, I agree. At the first glance, the code, as it is, could go in this file but more ARM specific code will be moved to this ARM generic code driver like device tree description and couple idle states. The init function would be more arch specific then.
For this reason, I think it is reasonable to move to arm/kernel/cpuidle.c rather than drivers/cpuidle/cpuidle.c first.
In the future, when all the ARM cpuidle driver will be fully consolidated, that will be easier to identify the common parts across the different arch and then move them to the generic framework.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
arch/arm/include/asm/cpuidle.h | 4 +++ arch/arm/kernel/cpuidle.c | 57 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 60 insertions(+), 1 deletion(-)
diff --git a/arch/arm/include/asm/cpuidle.h b/arch/arm/include/asm/cpuidle.h index 7367787..83a38ac 100644 --- a/arch/arm/include/asm/cpuidle.h +++ b/arch/arm/include/asm/cpuidle.h @@ -4,6 +4,10 @@ extern int arm_cpuidle_simple_enter(struct cpuidle_device *dev, struct cpuidle_driver *drv, int index); +extern int arm_cpuidle_init(struct cpuidle_driver *drv);
+extern void arm_cpuidle_exit(struct cpuidle_driver *drv);
/* Common ARM WFI state */ #define ARM_CPUIDLE_WFI_STATE_PWR(p) {\ .enter = arm_cpuidle_simple_enter,\ diff --git a/arch/arm/kernel/cpuidle.c b/arch/arm/kernel/cpuidle.c index 89545f6..13cfe3e 100644 --- a/arch/arm/kernel/cpuidle.c +++ b/arch/arm/kernel/cpuidle.c @@ -9,13 +9,68 @@
*/ +#include <linux/module.h> #include <linux/cpuidle.h> #include <asm/proc-fns.h> +static DEFINE_PER_CPU(struct cpuidle_device, cpuidle_device);
int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
struct cpuidle_driver *drv, int index)
struct cpuidle_driver *drv, int index){ cpu_do_idle(); return index; }
+int __init arm_cpuidle_init(struct cpuidle_driver *drv) +{
int ret, cpu;struct cpuidle_device *device;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_cpuidle_init);
+void __exit arm_cpuidle_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);
+}
+EXPORT_SYMBOL_GPL(arm_cpuidle_exit);
1.7.9.5
Please could you add a comment in the code about which piece is specific to ARM, because its not obvious to me. Its not like there is a reference to WFI for example. It looks like this code could go in drivers/cpuidle/cpuidle.c
Yes, I agree. At the first glance, the code, as it is, could go in this file but more ARM specific code will be moved to this ARM generic code driver like device tree description and couple idle states. The init function would be more arch specific then.
Hi Daniel
There was a discussion about device tree bindings when i posted the kirkwood cpuidle driver, now in drivers/cpuidle/cpuidle-kirkwood.c.
The conclusion was that pseudo devices, like cpuidle, do not have DT bindings. They can check of_machine_is_compatible(), like cpuidle-calxeda.c does, or they are platform drivers, which is what cpuidle-kirkwood.c is.
Even if DT binding was allowed, it again should not be ARM specific.
Are coupled idle states ARM specific?
Andrew
On 03/25/2013 08:09 PM, Andrew Lunn wrote:
Please could you add a comment in the code about which piece is specific to ARM, because its not obvious to me. Its not like there is a reference to WFI for example. It looks like this code could go in drivers/cpuidle/cpuidle.c
Yes, I agree. At the first glance, the code, as it is, could go in this file but more ARM specific code will be moved to this ARM generic code driver like device tree description and couple idle states. The init function would be more arch specific then.
Hi Daniel
There was a discussion about device tree bindings when i posted the kirkwood cpuidle driver, now in drivers/cpuidle/cpuidle-kirkwood.c.
The conclusion was that pseudo devices, like cpuidle, do not have DT bindings. They can check of_machine_is_compatible(), like cpuidle-calxeda.c does, or they are platform drivers, which is what cpuidle-kirkwood.c is.
Even if DT binding was allowed, it again should not be ARM specific.
If the DT binding was allowed, I *may* not be ARM specific but will certainly used only by the ARM drivers as the x86 platform uses ACPI or static tables.
Are coupled idle states ARM specific?
Well the code is not arch specific but today the idle coupling is ARM specific because it is the only arch using this kind of synchronization. There is also a last man standing algorithm common to ux500 and imx (maybe exynos soon) I would like to merge into this ARM driver.
AFAIK, the cluster shutdown is managed by the hardware on x86 so there is no need for this.
On Tue, Mar 26, 2013 at 12:50 AM, Daniel Lezcano daniel.lezcano@linaro.org wrote:
On 03/25/2013 08:09 PM, Andrew Lunn wrote:
Please could you add a comment in the code about which piece is specific to ARM, because its not obvious to me. Its not like there is a reference to WFI for example. It looks like this code could go in drivers/cpuidle/cpuidle.c
Yes, I agree. At the first glance, the code, as it is, could go in this file but more ARM specific code will be moved to this ARM generic code driver like device tree description and couple idle states. The init function would be more arch specific then.
Hi Daniel
There was a discussion about device tree bindings when i posted the kirkwood cpuidle driver, now in drivers/cpuidle/cpuidle-kirkwood.c.
The conclusion was that pseudo devices, like cpuidle, do not have DT bindings. They can check of_machine_is_compatible(), like cpuidle-calxeda.c does, or they are platform drivers, which is what cpuidle-kirkwood.c is.
Even if DT binding was allowed, it again should not be ARM specific.
If the DT binding was allowed, I *may* not be ARM specific but will certainly used only by the ARM drivers as the x86 platform uses ACPI or static tables.
Are coupled idle states ARM specific?
Well the code is not arch specific but today the idle coupling is ARM specific because it is the only arch using this kind of synchronization. There is also a last man standing algorithm common to ux500 and imx (maybe exynos soon) I would like to merge into this ARM driver.
Nico has developed a last man standing algorithm[1] for big.LITTLE TC2. That too needs to be considered during this consolidation. While it was developed for multi-cluster configurations, I don't see what it shouldn't work here too.
On 03/25/2013 08:31 PM, Amit Kucheria wrote:
On Tue, Mar 26, 2013 at 12:50 AM, Daniel Lezcano daniel.lezcano@linaro.org wrote:
On 03/25/2013 08:09 PM, Andrew Lunn wrote:
Please could you add a comment in the code about which piece is specific to ARM, because its not obvious to me. Its not like there is a reference to WFI for example. It looks like this code could go in drivers/cpuidle/cpuidle.c
Yes, I agree. At the first glance, the code, as it is, could go in this file but more ARM specific code will be moved to this ARM generic code driver like device tree description and couple idle states. The init function would be more arch specific then.
Hi Daniel
There was a discussion about device tree bindings when i posted the kirkwood cpuidle driver, now in drivers/cpuidle/cpuidle-kirkwood.c.
The conclusion was that pseudo devices, like cpuidle, do not have DT bindings. They can check of_machine_is_compatible(), like cpuidle-calxeda.c does, or they are platform drivers, which is what cpuidle-kirkwood.c is.
Even if DT binding was allowed, it again should not be ARM specific.
If the DT binding was allowed, I *may* not be ARM specific but will certainly used only by the ARM drivers as the x86 platform uses ACPI or static tables.
Are coupled idle states ARM specific?
Well the code is not arch specific but today the idle coupling is ARM specific because it is the only arch using this kind of synchronization. There is also a last man standing algorithm common to ux500 and imx (maybe exynos soon) I would like to merge into this ARM driver.
Nico has developed a last man standing algorithm[1] for big.LITTLE TC2. That too needs to be considered during this consolidation. While it was developed for multi-cluster configurations, I don't see what it shouldn't work here too.
I had it in mind when answering but I didn't mention it.
Thanks Amit for the pointer.
-- Daniel
If the DT binding was allowed, I *may* not be ARM specific but will certainly used only by the ARM drivers as the x86 platform uses ACPI or static tables.
And powerpc? Its powerpc that created DT, as far as i understand.
arch/powerpc/platforms/pseries/processor_idle.c
static int pseries_idle_devices_init(void) { int i; struct cpuidle_driver *drv = &pseries_idle_driver; struct cpuidle_device *dev;
pseries_cpuidle_devices = alloc_percpu(struct cpuidle_device); if (pseries_cpuidle_devices == NULL) return -ENOMEM;
for_each_possible_cpu(i) { dev = per_cpu_ptr(pseries_cpuidle_devices, i); dev->state_count = drv->state_count; dev->cpu = i; if (cpuidle_register_device(dev)) { printk(KERN_DEBUG \ "cpuidle_register_device %d failed!\n", i); return -EIO; } }
return 0; }
This looks pretty similar to the code you are consolidating. Can your 'ARM' code be made to work on powerpc?
Andrew
On 03/25/2013 10:42 PM, Andrew Lunn wrote:
If the DT binding was allowed, I *may* not be ARM specific but will certainly used only by the ARM drivers as the x86 platform uses ACPI or static tables.
And powerpc? Its powerpc that created DT, as far as i understand.
arch/powerpc/platforms/pseries/processor_idle.c
static int pseries_idle_devices_init(void) { int i; struct cpuidle_driver *drv = &pseries_idle_driver; struct cpuidle_device *dev;
pseries_cpuidle_devices = alloc_percpu(struct cpuidle_device); if (pseries_cpuidle_devices == NULL) return -ENOMEM; for_each_possible_cpu(i) { dev = per_cpu_ptr(pseries_cpuidle_devices, i); dev->state_count = drv->state_count; dev->cpu = i; if (cpuidle_register_device(dev)) { printk(KERN_DEBUG \ "cpuidle_register_device %d failed!\n", i); return -EIO; } } return 0;}
This looks pretty similar to the code you are consolidating. Can your 'ARM' code be made to work on powerpc?
Yes, it is very similar. I am aware of this code and the cpuidle code of all others archs but for now there are *18* cpuidle drivers for ARM I would like to consolidate in priority into a single one. When all of them will be factored out, I will recheck with the other arch. That will be easier to consolidate four archs: x86, arm, sh and powerpc.
May be in the meantime, someone will cleanup the non-ARM drivers and make my life easier :)
In any case, I keep in mind there are other arch using cpuidle.
Thanks -- Daniel
On Mon, 25 Mar 2013, Daniel Lezcano wrote:
On 03/25/2013 07:10 PM, Andrew Lunn wrote:
On Mon, Mar 25, 2013 at 06:55:32PM +0100, Daniel Lezcano wrote:
The init and exit routine for most of the drivers are the same, that is register the driver and register the device.
Provide a common function to do that in the cpuidle driver for ARM, so we can get rid of a lot of code duplication in the different SOC cpuidle drivers.
Hi Daniel
Please could you add a comment in the code about which piece is specific to ARM, because its not obvious to me. Its not like there is a reference to WFI for example. It looks like this code could go in drivers/cpuidle/cpuidle.c
Yes, I agree. At the first glance, the code, as it is, could go in this file but more ARM specific code will be moved to this ARM generic code driver like device tree description and couple idle states. The init function would be more arch specific then.
For this reason, I think it is reasonable to move to arm/kernel/cpuidle.c rather than drivers/cpuidle/cpuidle.c first.
No.
Please move things under drivers/cpuidle/ upfront.
We do want drivers to be gathered according to their purpose and subsystem, not according to the architecture they belong to.
This is a simple maintenance optimization to do so.
The reason is when someone wishes to improve the subsystem then that someone who might know nothing about the ARM or other architectures won't have to look for obscure drivers buried into arch specific directories. When things are gathered under a common directory it is then much easier to perform wide ranging changes to a subsystem.
And for those who do know the ARM architecture and wish to modify the ARM driver it is not that hard to locate the driver once.
The "downside" for you might be that the activity under drivers/cpuidle/ is more closely scrutinized by more people. But that isn't a bad thing either.
In the future, when all the ARM cpuidle driver will be fully consolidated, that will be easier to identify the common parts across the different arch and then move them to the generic framework.
Nothing prevents you from doing that consolidation work right in drivers/cpuidle/.
Nicolas
On 03/25/2013 09:28 PM, Nicolas Pitre wrote:
On Mon, 25 Mar 2013, Daniel Lezcano wrote:
On 03/25/2013 07:10 PM, Andrew Lunn wrote:
On Mon, Mar 25, 2013 at 06:55:32PM +0100, Daniel Lezcano wrote:
The init and exit routine for most of the drivers are the same, that is register the driver and register the device.
Provide a common function to do that in the cpuidle driver for ARM, so we can get rid of a lot of code duplication in the different SOC cpuidle drivers.
Hi Daniel
Please could you add a comment in the code about which piece is specific to ARM, because its not obvious to me. Its not like there is a reference to WFI for example. It looks like this code could go in drivers/cpuidle/cpuidle.c
Yes, I agree. At the first glance, the code, as it is, could go in this file but more ARM specific code will be moved to this ARM generic code driver like device tree description and couple idle states. The init function would be more arch specific then.
For this reason, I think it is reasonable to move to arm/kernel/cpuidle.c rather than drivers/cpuidle/cpuidle.c first.
No.
Please move things under drivers/cpuidle/ upfront.
We do want drivers to be gathered according to their purpose and subsystem, not according to the architecture they belong to.
This is a simple maintenance optimization to do so.
The reason is when someone wishes to improve the subsystem then that someone who might know nothing about the ARM or other architectures won't have to look for obscure drivers buried into arch specific directories. When things are gathered under a common directory it is then much easier to perform wide ranging changes to a subsystem.
And for those who do know the ARM architecture and wish to modify the ARM driver it is not that hard to locate the driver once.
The "downside" for you might be that the activity under drivers/cpuidle/ is more closely scrutinized by more people. But that isn't a bad thing either.
In the future, when all the ARM cpuidle driver will be fully consolidated, that will be easier to identify the common parts across the different arch and then move them to the generic framework.
Nothing prevents you from doing that consolidation work right in drivers/cpuidle/.
I fully agree with you but I think there is a misunderstanding.
The idea is to consolidate the ARM code in the ARM cpuidle driver which is mostly empty. The init/exit functions could falsely lead someone to think they could be moved in the generic framework but IMO it is too soon because the code consolidation will bring some arch specificity and in this case we will going back and forth. I want to avoid that.
Let me consolidate the ARM driver, cleanup the headers to split the arch specific code from the rest and then move this driver to drivers/cpuidle. This is just a question of a week.
On Mon, 25 Mar 2013, Daniel Lezcano wrote:
On 03/25/2013 09:28 PM, Nicolas Pitre wrote:
On Mon, 25 Mar 2013, Daniel Lezcano wrote:
In the future, when all the ARM cpuidle driver will be fully consolidated, that will be easier to identify the common parts across the different arch and then move them to the generic framework.
Nothing prevents you from doing that consolidation work right in drivers/cpuidle/.
I fully agree with you but I think there is a misunderstanding.
The idea is to consolidate the ARM code in the ARM cpuidle driver which is mostly empty. The init/exit functions could falsely lead someone to think they could be moved in the generic framework but IMO it is too soon because the code consolidation will bring some arch specificity and in this case we will going back and forth. I want to avoid that.
Let me consolidate the ARM driver, cleanup the headers to split the arch specific code from the rest and then move this driver to drivers/cpuidle. This is just a question of a week.
OK.
Nicolas
Remove the duplicate code and use the arm cpuidle driver's common code for initialization.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-ux500/cpuidle.c | 32 +------------------------------- 1 file changed, 1 insertion(+), 31 deletions(-)
diff --git a/arch/arm/mach-ux500/cpuidle.c b/arch/arm/mach-ux500/cpuidle.c index 1b16d9e..ee81d22 100644 --- a/arch/arm/mach-ux500/cpuidle.c +++ b/arch/arm/mach-ux500/cpuidle.c @@ -21,7 +21,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) @@ -113,40 +112,11 @@ static struct cpuidle_driver ux500_idle_driver = {
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));
- 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_cpuidle_init(&ux500_idle_driver); }
device_initcall(ux500_idle_init);
On Mon, Mar 25, 2013 at 6:55 PM, Daniel Lezcano daniel.lezcano@linaro.org wrote:
Remove the duplicate code and use the arm cpuidle driver's common code for initialization.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
Acked-by: Linus Walleij linus.walleij@linaro.org
Yours, Linus Walleij
Remove the duplicate code and use the arm cpuidle driver's common code for initialization.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-at91/cpuidle.c | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-)
diff --git a/arch/arm/mach-at91/cpuidle.c b/arch/arm/mach-at91/cpuidle.c index 0c63815..0aa2c0f 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, @@ -61,20 +59,9 @@ static struct cpuidle_driver at91_idle_driver = { };
/* Initialize CPU idle by registering the idle states */ -static int at91_init_cpuidle(void) +static int __init 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_cpuidle_init(&at91_idle_driver); }
device_initcall(at91_init_cpuidle);
On 03/25/2013 06:55 PM, Daniel Lezcano :
Remove the duplicate code and use the arm cpuidle driver's common code for initialization.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
No objection for at91:
Acked-by: Nicolas Ferre nicolas.ferre@atmel.com
arch/arm/mach-at91/cpuidle.c | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-)
diff --git a/arch/arm/mach-at91/cpuidle.c b/arch/arm/mach-at91/cpuidle.c index 0c63815..0aa2c0f 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, @@ -61,20 +59,9 @@ static struct cpuidle_driver at91_idle_driver = { }; /* Initialize CPU idle by registering the idle states */ -static int at91_init_cpuidle(void) +static int __init 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_cpuidle_init(&at91_idle_driver);
} device_initcall(at91_init_cpuidle);
Remove the duplicate code and use the arm cpuidle driver's common code for initialization.
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..b5a0ba1 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_cpuidle_init(&omap3_idle_driver); }
Remove the duplicate code and use the arm cpuidle driver's common code for initialization.
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..ce56142 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_cpuidle_init(&s3c64xx_cpuidle_driver); } device_initcall(s3c64xx_init_cpuidle);
Remove the duplicate code and use the arm cpuidle driver's common code for initialization.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-tegra/cpuidle-tegra114.c | 27 +-------------------------- 1 file changed, 1 insertion(+), 26 deletions(-)
diff --git a/arch/arm/mach-tegra/cpuidle-tegra114.c b/arch/arm/mach-tegra/cpuidle-tegra114.c index 0f4e8c4..771f6e8 100644 --- a/arch/arm/mach-tegra/cpuidle-tegra114.c +++ b/arch/arm/mach-tegra/cpuidle-tegra114.c @@ -30,32 +30,7 @@ static struct cpuidle_driver tegra_idle_driver = { }, };
-static DEFINE_PER_CPU(struct cpuidle_device, tegra_idle_device); - int __init tegra114_cpuidle_init(void) { - int ret; - unsigned int cpu; - struct cpuidle_device *dev; - struct cpuidle_driver *drv = &tegra_idle_driver; - - ret = cpuidle_register_driver(&tegra_idle_driver); - if (ret) { - pr_err("CPUidle driver registration failed\n"); - return ret; - } - - for_each_possible_cpu(cpu) { - dev = &per_cpu(tegra_idle_device, cpu); - dev->cpu = cpu; - - dev->state_count = drv->state_count; - ret = cpuidle_register_device(dev); - if (ret) { - pr_err("CPU%u: CPUidle device registration failed\n", - cpu); - return ret; - } - } - return 0; + return arm_cpuidle_init(&tegra_idle_driver); }
Add the __init section for the functions which are called at init time.
Signed-off-by: Daniel Lezcano <daniel.linaro.org> --- arch/arm/mach-shmobile/cpuidle.c | 4 ++-- arch/arm/mach-shmobile/pm-sh7372.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/arm/mach-shmobile/cpuidle.c b/arch/arm/mach-shmobile/cpuidle.c index 9e05026..068f9ca 100644 --- a/arch/arm/mach-shmobile/cpuidle.c +++ b/arch/arm/mach-shmobile/cpuidle.c @@ -36,12 +36,12 @@ static struct cpuidle_driver shmobile_cpuidle_default_driver = {
static struct cpuidle_driver *cpuidle_drv = &shmobile_cpuidle_default_driver;
-void shmobile_cpuidle_set_driver(struct cpuidle_driver *drv) +void __init shmobile_cpuidle_set_driver(struct cpuidle_driver *drv) { cpuidle_drv = drv; }
-int shmobile_cpuidle_init(void) +int __init shmobile_cpuidle_init(void) { struct cpuidle_device *dev = &shmobile_cpuidle_dev;
diff --git a/arch/arm/mach-shmobile/pm-sh7372.c b/arch/arm/mach-shmobile/pm-sh7372.c index a0826a4..f6b14ca 100644 --- a/arch/arm/mach-shmobile/pm-sh7372.c +++ b/arch/arm/mach-shmobile/pm-sh7372.c @@ -450,12 +450,12 @@ static struct cpuidle_driver sh7372_cpuidle_driver = { }, };
-static void sh7372_cpuidle_init(void) +static void __init sh7372_cpuidle_init(void) { shmobile_cpuidle_set_driver(&sh7372_cpuidle_driver); } #else -static void sh7372_cpuidle_init(void) {} +static void __init sh7372_cpuidle_init(void) {} #endif
#ifdef CONFIG_SUSPEND
On Mon, Mar 25, 2013 at 06:55:38PM +0100, Daniel Lezcano wrote:
Add the __init section for the functions which are called at init time.
Signed-off-by: Daniel Lezcano <daniel.linaro.org>
Acked-by: Simon Horman horms+renesas@verge.net.au
arch/arm/mach-shmobile/cpuidle.c | 4 ++-- arch/arm/mach-shmobile/pm-sh7372.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/arm/mach-shmobile/cpuidle.c b/arch/arm/mach-shmobile/cpuidle.c index 9e05026..068f9ca 100644 --- a/arch/arm/mach-shmobile/cpuidle.c +++ b/arch/arm/mach-shmobile/cpuidle.c @@ -36,12 +36,12 @@ static struct cpuidle_driver shmobile_cpuidle_default_driver = { static struct cpuidle_driver *cpuidle_drv = &shmobile_cpuidle_default_driver; -void shmobile_cpuidle_set_driver(struct cpuidle_driver *drv) +void __init shmobile_cpuidle_set_driver(struct cpuidle_driver *drv) { cpuidle_drv = drv; } -int shmobile_cpuidle_init(void) +int __init shmobile_cpuidle_init(void) { struct cpuidle_device *dev = &shmobile_cpuidle_dev; diff --git a/arch/arm/mach-shmobile/pm-sh7372.c b/arch/arm/mach-shmobile/pm-sh7372.c index a0826a4..f6b14ca 100644 --- a/arch/arm/mach-shmobile/pm-sh7372.c +++ b/arch/arm/mach-shmobile/pm-sh7372.c @@ -450,12 +450,12 @@ static struct cpuidle_driver sh7372_cpuidle_driver = { }, }; -static void sh7372_cpuidle_init(void) +static void __init sh7372_cpuidle_init(void) { shmobile_cpuidle_set_driver(&sh7372_cpuidle_driver); } #else -static void sh7372_cpuidle_init(void) {} +static void __init sh7372_cpuidle_init(void) {} #endif
#ifdef CONFIG_SUSPEND
1.7.9.5
Remove the shmobile_enter_wfi function which is the same as the common WFI enter function from the arm cpuidle driver defined with the ARM_CPUIDLE_WFI_STATE macro.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-shmobile/cpuidle.c | 8 -------- 1 file changed, 8 deletions(-)
diff --git a/arch/arm/mach-shmobile/cpuidle.c b/arch/arm/mach-shmobile/cpuidle.c index 068f9ca..c872ae8 100644 --- a/arch/arm/mach-shmobile/cpuidle.c +++ b/arch/arm/mach-shmobile/cpuidle.c @@ -16,20 +16,12 @@ #include <asm/cpuidle.h> #include <asm/io.h>
-int shmobile_enter_wfi(struct cpuidle_device *dev, struct cpuidle_driver *drv, - int index) -{ - cpu_do_idle(); - return 0; -} - static struct cpuidle_device shmobile_cpuidle_dev; static struct cpuidle_driver shmobile_cpuidle_default_driver = { .name = "shmobile_cpuidle", .owner = THIS_MODULE, .en_core_tk_irqen = 1, .states[0] = ARM_CPUIDLE_WFI_STATE, - .states[0].enter = shmobile_enter_wfi, .safe_state_index = 0, /* C1 */ .state_count = 1, };
On Mon, Mar 25, 2013 at 06:55:39PM +0100, Daniel Lezcano wrote:
Remove the shmobile_enter_wfi function which is the same as the common WFI enter function from the arm cpuidle driver defined with the ARM_CPUIDLE_WFI_STATE macro.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
Acked-by: Simon Horman horms+renesas@verge.net.au
arch/arm/mach-shmobile/cpuidle.c | 8 -------- 1 file changed, 8 deletions(-)
diff --git a/arch/arm/mach-shmobile/cpuidle.c b/arch/arm/mach-shmobile/cpuidle.c index 068f9ca..c872ae8 100644 --- a/arch/arm/mach-shmobile/cpuidle.c +++ b/arch/arm/mach-shmobile/cpuidle.c @@ -16,20 +16,12 @@ #include <asm/cpuidle.h> #include <asm/io.h> -int shmobile_enter_wfi(struct cpuidle_device *dev, struct cpuidle_driver *drv,
int index)-{
- cpu_do_idle();
- return 0;
-}
static struct cpuidle_device shmobile_cpuidle_dev; static struct cpuidle_driver shmobile_cpuidle_default_driver = { .name = "shmobile_cpuidle", .owner = THIS_MODULE, .en_core_tk_irqen = 1, .states[0] = ARM_CPUIDLE_WFI_STATE,
- .states[0].enter = shmobile_enter_wfi, .safe_state_index = 0, /* C1 */ .state_count = 1,
};
1.7.9.5
Remove the duplicate code and use the arm cpuidle driver's common code for initialization.
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 c872ae8..9744d88 100644 --- a/arch/arm/mach-shmobile/cpuidle.c +++ b/arch/arm/mach-shmobile/cpuidle.c @@ -16,7 +16,6 @@ #include <asm/cpuidle.h> #include <asm/io.h>
-static struct cpuidle_device shmobile_cpuidle_dev; static struct cpuidle_driver shmobile_cpuidle_default_driver = { .name = "shmobile_cpuidle", .owner = THIS_MODULE, @@ -35,12 +34,5 @@ void __init shmobile_cpuidle_set_driver(struct cpuidle_driver *drv)
int __init 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_cpuidle_init(cpuidle_drv); }
On Mon, Mar 25, 2013 at 06:55:40PM +0100, Daniel Lezcano wrote:
Remove the duplicate code and use the arm cpuidle driver's common code for initialization.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
Acked-by: Simon Horman horms+renesas@verge.net.au
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 c872ae8..9744d88 100644 --- a/arch/arm/mach-shmobile/cpuidle.c +++ b/arch/arm/mach-shmobile/cpuidle.c @@ -16,7 +16,6 @@ #include <asm/cpuidle.h> #include <asm/io.h> -static struct cpuidle_device shmobile_cpuidle_dev; static struct cpuidle_driver shmobile_cpuidle_default_driver = { .name = "shmobile_cpuidle", .owner = THIS_MODULE, @@ -35,12 +34,5 @@ void __init shmobile_cpuidle_set_driver(struct cpuidle_driver *drv) int __init 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_cpuidle_init(cpuidle_drv);
}
1.7.9.5
On 03/25/2013 12:55 PM, Daniel Lezcano wrote:
The flag CPUIDLE_FLAG_TIMER_STOP has been introduced in the commit 89878baa73f0f1c679355006bd8632e5d78f96c2.
The flag tells the cpuidle framework the local timer will stop in the idle state.
It is now easy to know if the cpuidle driver will use or not the broadcast timer by looking at the different states for this flag and then setup the broadcast timer consequently.
When we remove the timer initialization duplicated code in the different drivers, we have most of the drivers with the same init function. This init function is changed to be generic and moved in the ARM cpuidle driver and used from the drivers. That cleanups code and removes a lot of annoying duplicated code.
There is still some modification in OMAP4, tegra2, tegra3 and imx, especially around the coupled idle states, but we are more and more closer to a common squeleton for all the ARM drivers.
Daniel Lezcano (15): timer: move enum definition out of ifdef section cpuidle: initialize the broadcast timer framework cpuidle: ux500: remove timer broadcast initialization cpuidle: OMAP4: remove timer broadcast initialization cpuidle: imx6: remove timer broadcast initialization ARM: cpuidle: remove useless declaration ARM: cpuidle: add init/exit routine ARM: ux500: cpuidle: use init/exit common routine ARM: at91: cpuidle: use init/exit common routine ARM: OMAP3: cpuidle: use init/exit common routine ARM: s3c64xx: cpuidle: use init/exit common routine ARM: tegra1: cpuidle: use init/exit common routine ARM: shmobile: pm: fix init sections ARM: shmobile: cpuidle: remove useless WFI function ARM: shmobile: cpuidle: use init/exit common routine
arch/arm/include/asm/cpuidle.h | 11 +++--- arch/arm/kernel/cpuidle.c | 57 +++++++++++++++++++++++++++++++- arch/arm/mach-at91/cpuidle.c | 17 ++-------- arch/arm/mach-imx/cpuidle-imx6q.c | 15 --------- arch/arm/mach-omap2/cpuidle34xx.c | 18 ++-------- arch/arm/mach-omap2/cpuidle44xx.c | 14 -------- arch/arm/mach-s3c64xx/cpuidle.c | 15 ++------- arch/arm/mach-shmobile/cpuidle.c | 22 ++---------- arch/arm/mach-shmobile/pm-sh7372.c | 4 +-- arch/arm/mach-tegra/cpuidle-tegra114.c | 27 +-------------- arch/arm/mach-ux500/cpuidle.c | 50 +--------------------------- drivers/cpuidle/driver.c | 35 ++++++++++++++++++-- include/linux/clockchips.h | 22 ++++++------ include/linux/cpuidle.h | 2 ++ 14 files changed, 120 insertions(+), 189 deletions(-)
What happened to the Calxeda driver?
Rob
On 03/25/2013 09:27 PM, Rob Herring wrote:
On 03/25/2013 12:55 PM, Daniel Lezcano wrote:
The flag CPUIDLE_FLAG_TIMER_STOP has been introduced in the commit 89878baa73f0f1c679355006bd8632e5d78f96c2.
The flag tells the cpuidle framework the local timer will stop in the idle state.
It is now easy to know if the cpuidle driver will use or not the broadcast timer by looking at the different states for this flag and then setup the broadcast timer consequently.
When we remove the timer initialization duplicated code in the different drivers, we have most of the drivers with the same init function. This init function is changed to be generic and moved in the ARM cpuidle driver and used from the drivers. That cleanups code and removes a lot of annoying duplicated code.
There is still some modification in OMAP4, tegra2, tegra3 and imx, especially around the coupled idle states, but we are more and more closer to a common squeleton for all the ARM drivers.
Daniel Lezcano (15): timer: move enum definition out of ifdef section cpuidle: initialize the broadcast timer framework cpuidle: ux500: remove timer broadcast initialization cpuidle: OMAP4: remove timer broadcast initialization cpuidle: imx6: remove timer broadcast initialization ARM: cpuidle: remove useless declaration ARM: cpuidle: add init/exit routine ARM: ux500: cpuidle: use init/exit common routine ARM: at91: cpuidle: use init/exit common routine ARM: OMAP3: cpuidle: use init/exit common routine ARM: s3c64xx: cpuidle: use init/exit common routine ARM: tegra1: cpuidle: use init/exit common routine ARM: shmobile: pm: fix init sections ARM: shmobile: cpuidle: remove useless WFI function ARM: shmobile: cpuidle: use init/exit common routine
arch/arm/include/asm/cpuidle.h | 11 +++--- arch/arm/kernel/cpuidle.c | 57 +++++++++++++++++++++++++++++++- arch/arm/mach-at91/cpuidle.c | 17 ++-------- arch/arm/mach-imx/cpuidle-imx6q.c | 15 --------- arch/arm/mach-omap2/cpuidle34xx.c | 18 ++-------- arch/arm/mach-omap2/cpuidle44xx.c | 14 -------- arch/arm/mach-s3c64xx/cpuidle.c | 15 ++------- arch/arm/mach-shmobile/cpuidle.c | 22 ++---------- arch/arm/mach-shmobile/pm-sh7372.c | 4 +-- arch/arm/mach-tegra/cpuidle-tegra114.c | 27 +-------------- arch/arm/mach-ux500/cpuidle.c | 50 +--------------------------- drivers/cpuidle/driver.c | 35 ++++++++++++++++++-- include/linux/clockchips.h | 22 ++++++------ include/linux/cpuidle.h | 2 ++ 14 files changed, 120 insertions(+), 189 deletions(-)
What happened to the Calxeda driver?
On the way...
linaro-kernel@lists.linaro.org
-
Amit Kucheria -
Andrew Lunn -
Daniel Lezcano -
Linus Walleij -
Nicolas Ferre -
Nicolas Pitre -
Rafael J. Wysocki -
Rob Herring -
Santosh Shilimkar -
Shawn Guo -
Simon Horman