This patchset series provide some code consolidation across the different cpuidle drivers. It contains two parts, the first one is the removal of the time keeping flag and the second one, is a common initialization routine.
All the drivers use the en_core_tk_irqen flag, which means it is not necessary to make the time computation optional. We can remove this flag and assume the cpuidle framework always manage this operation.
The cpuidle code initialization is duplicated across the different drivers in the same manner.
The repeating pattern is:
SMP:
cpuidle_register_driver(drv); for_each_possible_cpu(cpu) { dev = per_cpu(cpuidle_device, cpu); cpuidle_register_device(dev); }
UP:
cpuidle_register_driver(drv); cpuidle_register_device(dev);
As on a UP machine the macro 'for_each_cpu' is a one iteration loop, using the initialization loop from SMP to UP works.
The patchset does some cleanup for different drivers in order to make the init code the same. Then it introduces a generic function:
cpuidle_register(struct cpuidle_driver *drv, struct cpumask *cpumask)
The cpumask is for the coupled idle states.
The drivers are then modified to take into account this new function and to remove the duplicated code.
The benefit is observable in the diffstat: 332 lines of code removed.
Tested-on: u8500 Tested-on: at91 Tested-on: intel i5 Tested-on: OMAP4
Compiled with and without CPU_IDLE for: u8500, at91, davinci, exynos, imx5, imx6, kirkwood, multi_v7 (for calxeda), omap2plus, s3c64, tegra1, tegra2, tegra3
Daniel Lezcano (18): ARM: OMAP3: remove cpuidle_wrap_enter cpuidle: remove en_core_tk_irqen flag ARM: ux500: cpuidle: replace for_each_online_cpu by for_each_possible_cpu ARM: imx: cpuidle: create separate drivers for imx5/imx6 cpuidle: make a single register function for all 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: cpuidle: use init/exit common routine ARM: OMAP4: cpuidle: use init/exit common routine ARM: tegra2: cpuidle: use init/exit common routine ARM: tegra3: cpuidle: use init/exit common routine ARM: calxeda: cpuidle: use init/exit common routine ARM: kirkwood: cpuidle: use init/exit common routine ARM: davinci: cpuidle: use init/exit common routine ARM: imx: cpuidle: use init/exit common routine
arch/arm/mach-at91/cpuidle.c | 18 +-- arch/arm/mach-davinci/cpuidle.c | 21 +--- arch/arm/mach-exynos/cpuidle.c | 1 - arch/arm/mach-imx/Makefile | 1 + arch/arm/mach-imx/cpuidle-imx5.c | 40 +++++++ arch/arm/mach-imx/cpuidle-imx6q.c | 3 +- arch/arm/mach-imx/cpuidle.c | 80 ------------- arch/arm/mach-imx/cpuidle.h | 10 +- arch/arm/mach-imx/pm-imx5.c | 30 +---- arch/arm/mach-omap2/cpuidle34xx.c | 49 ++------ arch/arm/mach-omap2/cpuidle44xx.c | 23 +--- arch/arm/mach-s3c64xx/cpuidle.c | 15 +-- arch/arm/mach-shmobile/cpuidle.c | 11 +- arch/arm/mach-shmobile/pm-sh7372.c | 1 - arch/arm/mach-tegra/cpuidle-tegra114.c | 27 +---- arch/arm/mach-tegra/cpuidle-tegra20.c | 34 +----- arch/arm/mach-tegra/cpuidle-tegra30.c | 28 +---- arch/arm/mach-ux500/cpuidle.c | 33 +----- arch/powerpc/platforms/pseries/processor_idle.c | 1 - arch/sh/kernel/cpu/shmobile/cpuidle.c | 1 - arch/x86/kernel/apm_32.c | 1 - drivers/acpi/processor_idle.c | 1 - drivers/cpuidle/cpuidle-calxeda.c | 53 +-------- drivers/cpuidle/cpuidle-kirkwood.c | 18 +-- drivers/cpuidle/cpuidle.c | 137 ++++++++++++++--------- drivers/idle/intel_idle.c | 1 - include/linux/cpuidle.h | 20 ++-- 27 files changed, 162 insertions(+), 496 deletions(-) create mode 100644 arch/arm/mach-imx/cpuidle-imx5.c delete mode 100644 arch/arm/mach-imx/cpuidle.c
In a previous commit the en_core_tk_irqen flag has been added but we missed the cpuidle_wrap_enter which was doing the job to measure the time for the 'omap3_enter_idle' function.
Actually, I don't see any reason to use this wrapper in the code. In the better case, the time computation is not correctly done because of the different operations done in omap3_enter_idle_bm which were not taken into account before the en_core_tk_irqen flag was set.
As the time is reflected for the state overridden by the omap3_enter_idle_bm, using the wrapper is pointless now, so removing it.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-omap2/cpuidle34xx.c | 30 +++++++++--------------------- 1 file changed, 9 insertions(+), 21 deletions(-)
diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c index 4f67a5b..a56310a 100644 --- a/arch/arm/mach-omap2/cpuidle34xx.c +++ b/arch/arm/mach-omap2/cpuidle34xx.c @@ -99,11 +99,15 @@ static struct omap3_idle_statedata omap3_idle_data[] = { }, };
-/* Private functions */ - -static int __omap3_enter_idle(struct cpuidle_device *dev, - struct cpuidle_driver *drv, - int index) +/** + * omap3_enter_idle - Programs OMAP3 to enter the specified state + * @dev: cpuidle device + * @drv: cpuidle driver + * @index: the index of state to be entered + */ +static int omap3_enter_idle(struct cpuidle_device *dev, + struct cpuidle_driver *drv, + int index) { struct omap3_idle_statedata *cx = &omap3_idle_data[index];
@@ -149,22 +153,6 @@ return_sleep_time: }
/** - * omap3_enter_idle - Programs OMAP3 to enter the specified state - * @dev: cpuidle device - * @drv: cpuidle driver - * @index: the index of state to be entered - * - * Called from the CPUidle framework to program the device to the - * specified target state selected by the governor. - */ -static inline int omap3_enter_idle(struct cpuidle_device *dev, - struct cpuidle_driver *drv, - int index) -{ - return cpuidle_wrap_enter(dev, drv, index, __omap3_enter_idle); -} - -/** * next_valid_state - Find next valid C-state * @dev: cpuidle device * @drv: cpuidle driver
The en_core_tk_irqen flag is set in all the cpuidle driver which means it is not necessary to specify this flag.
Remove the flag and the code related to it.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-at91/cpuidle.c | 1 - arch/arm/mach-davinci/cpuidle.c | 1 - arch/arm/mach-exynos/cpuidle.c | 1 - arch/arm/mach-imx/cpuidle-imx6q.c | 1 - arch/arm/mach-imx/pm-imx5.c | 1 - arch/arm/mach-omap2/cpuidle34xx.c | 1 - arch/arm/mach-omap2/cpuidle44xx.c | 1 - arch/arm/mach-s3c64xx/cpuidle.c | 1 - arch/arm/mach-shmobile/cpuidle.c | 1 - arch/arm/mach-shmobile/pm-sh7372.c | 1 - arch/arm/mach-tegra/cpuidle-tegra114.c | 1 - arch/arm/mach-tegra/cpuidle-tegra20.c | 1 - arch/arm/mach-tegra/cpuidle-tegra30.c | 1 - arch/arm/mach-ux500/cpuidle.c | 1 - arch/powerpc/platforms/pseries/processor_idle.c | 1 - arch/sh/kernel/cpu/shmobile/cpuidle.c | 1 - arch/x86/kernel/apm_32.c | 1 - drivers/acpi/processor_idle.c | 1 - drivers/cpuidle/cpuidle-calxeda.c | 1 - drivers/cpuidle/cpuidle-kirkwood.c | 1 - drivers/cpuidle/cpuidle.c | 72 ++++++----------------- drivers/idle/intel_idle.c | 1 - include/linux/cpuidle.h | 11 ---- 23 files changed, 18 insertions(+), 86 deletions(-)
diff --git a/arch/arm/mach-at91/cpuidle.c b/arch/arm/mach-at91/cpuidle.c index 0c63815..0130df7 100644 --- a/arch/arm/mach-at91/cpuidle.c +++ b/arch/arm/mach-at91/cpuidle.c @@ -47,7 +47,6 @@ static int at91_enter_idle(struct cpuidle_device *dev, static struct cpuidle_driver at91_idle_driver = { .name = "at91_idle", .owner = THIS_MODULE, - .en_core_tk_irqen = 1, .states[0] = ARM_CPUIDLE_WFI_STATE, .states[1] = { .enter = at91_enter_idle, diff --git a/arch/arm/mach-davinci/cpuidle.c b/arch/arm/mach-davinci/cpuidle.c index 22d6d4a..c2887c5 100644 --- a/arch/arm/mach-davinci/cpuidle.c +++ b/arch/arm/mach-davinci/cpuidle.c @@ -62,7 +62,6 @@ static int davinci_enter_idle(struct cpuidle_device *dev, static struct cpuidle_driver davinci_idle_driver = { .name = "cpuidle-davinci", .owner = THIS_MODULE, - .en_core_tk_irqen = 1, .states[0] = ARM_CPUIDLE_WFI_STATE, .states[1] = { .enter = davinci_enter_idle, diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c index fcfe025..498a7a2 100644 --- a/arch/arm/mach-exynos/cpuidle.c +++ b/arch/arm/mach-exynos/cpuidle.c @@ -58,7 +58,6 @@ static DEFINE_PER_CPU(struct cpuidle_device, exynos4_cpuidle_device); static struct cpuidle_driver exynos4_idle_driver = { .name = "exynos4_idle", .owner = THIS_MODULE, - .en_core_tk_irqen = 1, };
/* Ext-GIC nIRQ/nFIQ is the only wakeup source in AFTR */ diff --git a/arch/arm/mach-imx/cpuidle-imx6q.c b/arch/arm/mach-imx/cpuidle-imx6q.c index a783a63..e2739ad 100644 --- a/arch/arm/mach-imx/cpuidle-imx6q.c +++ b/arch/arm/mach-imx/cpuidle-imx6q.c @@ -45,7 +45,6 @@ done: static struct cpuidle_driver imx6q_cpuidle_driver = { .name = "imx6q_cpuidle", .owner = THIS_MODULE, - .en_core_tk_irqen = 1, .states = { /* WFI */ ARM_CPUIDLE_WFI_STATE, diff --git a/arch/arm/mach-imx/pm-imx5.c b/arch/arm/mach-imx/pm-imx5.c index f67fd7e..4b52b3e 100644 --- a/arch/arm/mach-imx/pm-imx5.c +++ b/arch/arm/mach-imx/pm-imx5.c @@ -164,7 +164,6 @@ static int imx5_cpuidle_enter(struct cpuidle_device *dev, static struct cpuidle_driver imx5_cpuidle_driver = { .name = "imx5_cpuidle", .owner = THIS_MODULE, - .en_core_tk_irqen = 1, .states[0] = { .enter = imx5_cpuidle_enter, .exit_latency = 2, diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c index a56310a..027a787 100644 --- a/arch/arm/mach-omap2/cpuidle34xx.c +++ b/arch/arm/mach-omap2/cpuidle34xx.c @@ -264,7 +264,6 @@ static DEFINE_PER_CPU(struct cpuidle_device, omap3_idle_dev); static struct cpuidle_driver omap3_idle_driver = { .name = "omap3_idle", .owner = THIS_MODULE, - .en_core_tk_irqen = 1, .states = { { .enter = omap3_enter_idle_bm, diff --git a/arch/arm/mach-omap2/cpuidle44xx.c b/arch/arm/mach-omap2/cpuidle44xx.c index f4b1b23..cd188de 100644 --- a/arch/arm/mach-omap2/cpuidle44xx.c +++ b/arch/arm/mach-omap2/cpuidle44xx.c @@ -162,7 +162,6 @@ static DEFINE_PER_CPU(struct cpuidle_device, omap4_idle_dev); static struct cpuidle_driver omap4_idle_driver = { .name = "omap4_idle", .owner = THIS_MODULE, - .en_core_tk_irqen = 1, .states = { { /* C1 - CPU0 ON + CPU1 ON + MPU ON */ diff --git a/arch/arm/mach-s3c64xx/cpuidle.c b/arch/arm/mach-s3c64xx/cpuidle.c index ead5fab..852ff16 100644 --- a/arch/arm/mach-s3c64xx/cpuidle.c +++ b/arch/arm/mach-s3c64xx/cpuidle.c @@ -45,7 +45,6 @@ static DEFINE_PER_CPU(struct cpuidle_device, s3c64xx_cpuidle_device); static struct cpuidle_driver s3c64xx_cpuidle_driver = { .name = "s3c64xx_cpuidle", .owner = THIS_MODULE, - .en_core_tk_irqen = 1, .states = { { .enter = s3c64xx_enter_idle, diff --git a/arch/arm/mach-shmobile/cpuidle.c b/arch/arm/mach-shmobile/cpuidle.c index c872ae8..d671ae9 100644 --- a/arch/arm/mach-shmobile/cpuidle.c +++ b/arch/arm/mach-shmobile/cpuidle.c @@ -20,7 +20,6 @@ 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, .safe_state_index = 0, /* C1 */ .state_count = 1, diff --git a/arch/arm/mach-shmobile/pm-sh7372.c b/arch/arm/mach-shmobile/pm-sh7372.c index f6b14ca..cbc2c36 100644 --- a/arch/arm/mach-shmobile/pm-sh7372.c +++ b/arch/arm/mach-shmobile/pm-sh7372.c @@ -410,7 +410,6 @@ static int sh7372_enter_a4s(struct cpuidle_device *dev, static struct cpuidle_driver sh7372_cpuidle_driver = { .name = "sh7372_cpuidle", .owner = THIS_MODULE, - .en_core_tk_irqen = 1, .state_count = 5, .safe_state_index = 0, /* C1 */ .states[0] = ARM_CPUIDLE_WFI_STATE, diff --git a/arch/arm/mach-tegra/cpuidle-tegra114.c b/arch/arm/mach-tegra/cpuidle-tegra114.c index c527cff..c5fadf9 100644 --- a/arch/arm/mach-tegra/cpuidle-tegra114.c +++ b/arch/arm/mach-tegra/cpuidle-tegra114.c @@ -23,7 +23,6 @@ static struct cpuidle_driver tegra_idle_driver = { .name = "tegra_idle", .owner = THIS_MODULE, - .en_core_tk_irqen = 1, .state_count = 1, .states = { [0] = ARM_CPUIDLE_WFI_STATE_PWR(600), diff --git a/arch/arm/mach-tegra/cpuidle-tegra20.c b/arch/arm/mach-tegra/cpuidle-tegra20.c index b94d76a..f1f6ac4 100644 --- a/arch/arm/mach-tegra/cpuidle-tegra20.c +++ b/arch/arm/mach-tegra/cpuidle-tegra20.c @@ -51,7 +51,6 @@ static int tegra20_idle_lp2_coupled(struct cpuidle_device *dev, static struct cpuidle_driver tegra_idle_driver = { .name = "tegra_idle", .owner = THIS_MODULE, - .en_core_tk_irqen = 1, .states = { ARM_CPUIDLE_WFI_STATE_PWR(600), #ifdef CONFIG_PM_SLEEP diff --git a/arch/arm/mach-tegra/cpuidle-tegra30.c b/arch/arm/mach-tegra/cpuidle-tegra30.c index c4e01fe..f6a0c72 100644 --- a/arch/arm/mach-tegra/cpuidle-tegra30.c +++ b/arch/arm/mach-tegra/cpuidle-tegra30.c @@ -43,7 +43,6 @@ static int tegra30_idle_lp2(struct cpuidle_device *dev, static struct cpuidle_driver tegra_idle_driver = { .name = "tegra_idle", .owner = THIS_MODULE, - .en_core_tk_irqen = 1, #ifdef CONFIG_PM_SLEEP .state_count = 2, #else diff --git a/arch/arm/mach-ux500/cpuidle.c b/arch/arm/mach-ux500/cpuidle.c index 1b16d9e..c29c1bf 100644 --- a/arch/arm/mach-ux500/cpuidle.c +++ b/arch/arm/mach-ux500/cpuidle.c @@ -94,7 +94,6 @@ out: static struct cpuidle_driver ux500_idle_driver = { .name = "ux500_idle", .owner = THIS_MODULE, - .en_core_tk_irqen = 1, .states = { ARM_CPUIDLE_WFI_STATE, { diff --git a/arch/powerpc/platforms/pseries/processor_idle.c b/arch/powerpc/platforms/pseries/processor_idle.c index a197120..4644efa0 100644 --- a/arch/powerpc/platforms/pseries/processor_idle.c +++ b/arch/powerpc/platforms/pseries/processor_idle.c @@ -25,7 +25,6 @@ struct cpuidle_driver pseries_idle_driver = { .name = "pseries_idle", .owner = THIS_MODULE, - .en_core_tk_irqen = 1, };
#define MAX_IDLE_STATE_COUNT 2 diff --git a/arch/sh/kernel/cpu/shmobile/cpuidle.c b/arch/sh/kernel/cpu/shmobile/cpuidle.c index 1ddc876..0986f21 100644 --- a/arch/sh/kernel/cpu/shmobile/cpuidle.c +++ b/arch/sh/kernel/cpu/shmobile/cpuidle.c @@ -55,7 +55,6 @@ static struct cpuidle_device cpuidle_dev; static struct cpuidle_driver cpuidle_driver = { .name = "sh_idle", .owner = THIS_MODULE, - .en_core_tk_irqen = 1, };
void sh_mobile_setup_cpuidle(void) diff --git a/arch/x86/kernel/apm_32.c b/arch/x86/kernel/apm_32.c index 66b5faf..53a4e27 100644 --- a/arch/x86/kernel/apm_32.c +++ b/arch/x86/kernel/apm_32.c @@ -373,7 +373,6 @@ static int apm_cpu_idle(struct cpuidle_device *dev, static struct cpuidle_driver apm_idle_driver = { .name = "apm_idle", .owner = THIS_MODULE, - .en_core_tk_irqen = 1, .states = { { /* entry 0 is for polling */ }, { /* entry 1 is for APM idle */ diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c index ee255c6..f0df2c9 100644 --- a/drivers/acpi/processor_idle.c +++ b/drivers/acpi/processor_idle.c @@ -918,7 +918,6 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev, struct cpuidle_driver acpi_idle_driver = { .name = "acpi_idle", .owner = THIS_MODULE, - .en_core_tk_irqen = 1, };
/** diff --git a/drivers/cpuidle/cpuidle-calxeda.c b/drivers/cpuidle/cpuidle-calxeda.c index e1aab38..a3b56f5 100644 --- a/drivers/cpuidle/cpuidle-calxeda.c +++ b/drivers/cpuidle/cpuidle-calxeda.c @@ -100,7 +100,6 @@ static void calxeda_idle_cpuidle_devices_uninit(void)
static struct cpuidle_driver calxeda_idle_driver = { .name = "calxeda_idle", - .en_core_tk_irqen = 1, .states = { ARM_CPUIDLE_WFI_STATE, { diff --git a/drivers/cpuidle/cpuidle-kirkwood.c b/drivers/cpuidle/cpuidle-kirkwood.c index 53aad73..6f31524 100644 --- a/drivers/cpuidle/cpuidle-kirkwood.c +++ b/drivers/cpuidle/cpuidle-kirkwood.c @@ -41,7 +41,6 @@ static int kirkwood_enter_idle(struct cpuidle_device *dev, static struct cpuidle_driver kirkwood_idle_driver = { .name = "kirkwood_idle", .owner = THIS_MODULE, - .en_core_tk_irqen = 1, .states[0] = ARM_CPUIDLE_WFI_STATE, .states[1] = { .enter = kirkwood_enter_idle, diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index c500370..0da795b 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -43,24 +43,6 @@ void disable_cpuidle(void)
static int __cpuidle_register_device(struct cpuidle_device *dev);
-static inline int cpuidle_enter(struct cpuidle_device *dev, - struct cpuidle_driver *drv, int index) -{ - struct cpuidle_state *target_state = &drv->states[index]; - return target_state->enter(dev, drv, index); -} - -static inline int cpuidle_enter_tk(struct cpuidle_device *dev, - struct cpuidle_driver *drv, int index) -{ - return cpuidle_wrap_enter(dev, drv, index, cpuidle_enter); -} - -typedef int (*cpuidle_enter_t)(struct cpuidle_device *dev, - struct cpuidle_driver *drv, int index); - -static cpuidle_enter_t cpuidle_enter_ops; - /** * cpuidle_play_dead - cpu off-lining * @@ -90,11 +72,27 @@ int cpuidle_play_dead(void) * @next_state: index into drv->states of the state to enter */ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv, - int next_state) + int index) { int entered_state;
- entered_state = cpuidle_enter_ops(dev, drv, next_state); + struct cpuidle_state *target_state = &drv->states[index]; + ktime_t time_start, time_end; + s64 diff; + + time_start = ktime_get(); + + entered_state = target_state->enter(dev, drv, index); + + time_end = ktime_get(); + + local_irq_enable(); + + diff = ktime_to_us(ktime_sub(time_end, time_start)); + if (diff > INT_MAX) + diff = INT_MAX; + + dev->last_residency = (int) diff;
if (entered_state >= 0) { /* Update cpuidle counters */ @@ -231,37 +229,6 @@ void cpuidle_resume(void) mutex_unlock(&cpuidle_lock); }
-/** - * cpuidle_wrap_enter - performs timekeeping and irqen around enter function - * @dev: pointer to a valid cpuidle_device object - * @drv: pointer to a valid cpuidle_driver object - * @index: index of the target cpuidle state. - */ -int cpuidle_wrap_enter(struct cpuidle_device *dev, - struct cpuidle_driver *drv, int index, - int (*enter)(struct cpuidle_device *dev, - struct cpuidle_driver *drv, int index)) -{ - ktime_t time_start, time_end; - s64 diff; - - time_start = ktime_get(); - - index = enter(dev, drv, index); - - time_end = ktime_get(); - - local_irq_enable(); - - diff = ktime_to_us(ktime_sub(time_end, time_start)); - if (diff > INT_MAX) - diff = INT_MAX; - - dev->last_residency = (int) diff; - - return index; -} - #ifdef CONFIG_ARCH_HAS_CPU_RELAX static int poll_idle(struct cpuidle_device *dev, struct cpuidle_driver *drv, int index) @@ -333,9 +300,6 @@ int cpuidle_enable_device(struct cpuidle_device *dev) return ret; }
- cpuidle_enter_ops = drv->en_core_tk_irqen ? - cpuidle_enter_tk : cpuidle_enter; - poll_idle_init(drv);
ret = cpuidle_add_device_sysfs(dev); diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c index 5d66750..48d7584 100644 --- a/drivers/idle/intel_idle.c +++ b/drivers/idle/intel_idle.c @@ -71,7 +71,6 @@ static struct cpuidle_driver intel_idle_driver = { .name = "intel_idle", .owner = THIS_MODULE, - .en_core_tk_irqen = 1, }; /* intel_idle.max_cstate=0 disables driver */ static int max_cstate = CPUIDLE_STATE_MAX - 1; diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index fc3e580..79e3811 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -105,8 +105,6 @@ struct cpuidle_driver { struct module *owner; int refcnt;
- /* 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 */ @@ -132,10 +130,6 @@ extern void cpuidle_pause(void); extern void cpuidle_resume(void); extern int cpuidle_enable_device(struct cpuidle_device *dev); extern void cpuidle_disable_device(struct cpuidle_device *dev); -extern int cpuidle_wrap_enter(struct cpuidle_device *dev, - struct cpuidle_driver *drv, int index, - int (*enter)(struct cpuidle_device *dev, - struct cpuidle_driver *drv, int index)); extern int cpuidle_play_dead(void);
extern struct cpuidle_driver *cpuidle_get_cpu_driver(struct cpuidle_device *dev); @@ -162,11 +156,6 @@ static inline void cpuidle_resume(void) { } static inline int cpuidle_enable_device(struct cpuidle_device *dev) {return -ENODEV; } static inline void cpuidle_disable_device(struct cpuidle_device *dev) { } -static inline int cpuidle_wrap_enter(struct cpuidle_device *dev, - struct cpuidle_driver *drv, int index, - int (*enter)(struct cpuidle_device *dev, - struct cpuidle_driver *drv, int index)) -{ return -ENODEV; } static inline int cpuidle_play_dead(void) {return -ENODEV; } #endif
All the drivers are using, in their initialization function, the for_each_possible_cpu macro.
Using for_each_online_cpu means the driver must handle the initialization of the cpuidle device when a cpu is up which is not the case here.
Change the macro to for_each_possible_cpu as that fix the hotplug initialization and make the initialization routine consistent with the rest of the code in the different drivers.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-ux500/cpuidle.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/mach-ux500/cpuidle.c b/arch/arm/mach-ux500/cpuidle.c index c29c1bf..5657d4a 100644 --- a/arch/arm/mach-ux500/cpuidle.c +++ b/arch/arm/mach-ux500/cpuidle.c @@ -125,7 +125,7 @@ int __init ux500_idle_init(void) return ret; }
- for_each_online_cpu(cpu) { + for_each_possible(cpu) { device = &per_cpu(ux500_cpuidle_device, cpu); device->cpu = cpu; ret = cpuidle_register_device(device); @@ -139,7 +139,7 @@ out: return ret;
out_unregister: - for_each_online_cpu(cpu) { + for_each_possible_cpu(cpu) { device = &per_cpu(ux500_cpuidle_device, cpu); cpuidle_unregister_device(device); }
The code intializes the cpuidle driver at different places. The cpuidle driver for : * imx5 : is in the pm-imx5.c, the init function is in cpuidle.c * imx6 : is in cpuidle-imx6q.c, the init function is in cpuidle.c and cpuidle-imx6q.c
Instead of having the cpuidle code spread across different files, let's write a driver for each SoC and make the code similar.
That implies some code duplication but that will be fixed with the next patches which consolidate the initialization for all the drivers.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-imx/Makefile | 1 + arch/arm/mach-imx/cpuidle-imx5.c | 74 ++++++++++++++++++++++++++++++++++ arch/arm/mach-imx/cpuidle-imx6q.c | 36 ++++++++++++++++- arch/arm/mach-imx/cpuidle.c | 80 ------------------------------------- arch/arm/mach-imx/cpuidle.h | 10 ++--- arch/arm/mach-imx/pm-imx5.c | 29 +------------- 6 files changed, 115 insertions(+), 115 deletions(-) create mode 100644 arch/arm/mach-imx/cpuidle-imx5.c delete mode 100644 arch/arm/mach-imx/cpuidle.c
diff --git a/arch/arm/mach-imx/Makefile b/arch/arm/mach-imx/Makefile index c4ce090..bfd5f5b 100644 --- a/arch/arm/mach-imx/Makefile +++ b/arch/arm/mach-imx/Makefile @@ -31,6 +31,7 @@ obj-$(CONFIG_CPU_FREQ_IMX) += cpufreq.o
ifeq ($(CONFIG_CPU_IDLE),y) obj-y += cpuidle.o +obj-$(CONFIG_SOC_IMX5) += cpuidle-imx5.o obj-$(CONFIG_SOC_IMX6Q) += cpuidle-imx6q.o endif
diff --git a/arch/arm/mach-imx/cpuidle-imx5.c b/arch/arm/mach-imx/cpuidle-imx5.c new file mode 100644 index 0000000..3d9f0c9 --- /dev/null +++ b/arch/arm/mach-imx/cpuidle-imx5.c @@ -0,0 +1,74 @@ +/* + * Copyright (C) 2012 Freescale Semiconductor, Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include <linux/cpuidle.h> +#include <linux/module.h> +#include <asm/system_misc.h> +#include <asm/proc-fns.h> + +#include "hardware.h" + +static DEFINE_PER_CPU(struct cpuidle_device, devices); + +static int imx5_cpuidle_enter(struct cpuidle_device *dev, + struct cpuidle_driver *drv, int index) +{ + arm_pm_idle(); + return index; +} + +static struct cpuidle_driver imx5_cpuidle_driver = { + .name = "imx5_cpuidle", + .owner = THIS_MODULE, + .states[0] = { + .enter = imx5_cpuidle_enter, + .exit_latency = 2, + .target_residency = 1, + .flags = CPUIDLE_FLAG_TIME_VALID, + .name = "IMX5 SRPG", + .desc = "CPU state retained,powered off", + }, + .state_count = 1, +}; + +int __init imx5_cpuidle_init(void) +{ + struct cpuidle_device *dev; + int cpu, ret; + + ret = cpuidle_register_driver(&imx5_cpuidle_driver); + if (ret) { + pr_err("%s: Failed to register cpuidle driver with error: %d\n", + __func__, ret); + return ret; + } + + /* initialize state data for each cpuidle_device */ + for_each_possible_cpu(cpu) { + dev = &per_cpu(devices, cpu); + dev->cpu = cpu; + + ret = cpuidle_register_device(dev); + if (ret) { + pr_err("%s: Failed to register cpu %u, error: %d\n", + __func__, cpu, ret); + goto out_unregister; + } + } + + return 0; + +out_unregister: + for_each_possible_cpu(cpu) { + dev = &per_cpu(devices, cpu); + cpuidle_unregister_device(dev); + } + + cpuidle_unregister_driver(&imx5_cpuidle_driver); + return ret; +} diff --git a/arch/arm/mach-imx/cpuidle-imx6q.c b/arch/arm/mach-imx/cpuidle-imx6q.c index e2739ad..c066b74faa 100644 --- a/arch/arm/mach-imx/cpuidle-imx6q.c +++ b/arch/arm/mach-imx/cpuidle-imx6q.c @@ -14,6 +14,8 @@ #include "common.h" #include "cpuidle.h"
+static DEFINE_PER_CPU(struct cpuidle_device, devices); + static atomic_t master = ATOMIC_INIT(0); static DEFINE_SPINLOCK(master_lock);
@@ -65,11 +67,43 @@ static struct cpuidle_driver imx6q_cpuidle_driver = {
int __init imx6q_cpuidle_init(void) { + struct cpuidle_device *dev; + int cpu, ret; + /* Need to enable SCU standby for entering WAIT modes */ imx_scu_standby_enable();
/* Set chicken bit to get a reliable WAIT mode support */ imx6q_set_chicken_bit();
- return imx_cpuidle_init(&imx6q_cpuidle_driver); + ret = cpuidle_register_driver(&imx6q_cpuidle_driver); + if (ret) { + pr_err("%s: Failed to register cpuidle driver with error: %d\n", + __func__, ret); + return ret; + } + + /* initialize state data for each cpuidle_device */ + for_each_possible_cpu(cpu) { + dev = &per_cpu(devices, cpu); + dev->cpu = cpu; + + ret = cpuidle_register_device(dev); + if (ret) { + pr_err("%s: Failed to register cpu %u, error: %d\n", + __func__, cpu, ret); + goto out_unregister; + } + } + + return 0; + +out_unregister: + for_each_possible_cpu(cpu) { + dev = &per_cpu(devices, cpu); + cpuidle_unregister_device(dev); + } + + cpuidle_unregister_driver(&imx6q_cpuidle_driver); + return ret; } diff --git a/arch/arm/mach-imx/cpuidle.c b/arch/arm/mach-imx/cpuidle.c deleted file mode 100644 index d4cb511..0000000 --- a/arch/arm/mach-imx/cpuidle.c +++ /dev/null @@ -1,80 +0,0 @@ -/* - * Copyright 2012 Freescale Semiconductor, Inc. - * Copyright 2012 Linaro Ltd. - * - * The code contained herein is licensed under the GNU General Public - * License. You may obtain a copy of the GNU General Public License - * Version 2 or later at the following locations: - * - * http://www.opensource.org/licenses/gpl-license.html - * http://www.gnu.org/copyleft/gpl.html - */ - -#include <linux/cpuidle.h> -#include <linux/err.h> -#include <linux/hrtimer.h> -#include <linux/io.h> -#include <linux/kernel.h> -#include <linux/slab.h> - -static struct cpuidle_device __percpu * imx_cpuidle_devices; - -static void __init imx_cpuidle_devices_uninit(void) -{ - int cpu_id; - struct cpuidle_device *dev; - - for_each_possible_cpu(cpu_id) { - dev = per_cpu_ptr(imx_cpuidle_devices, cpu_id); - cpuidle_unregister_device(dev); - } - - free_percpu(imx_cpuidle_devices); -} - -int __init imx_cpuidle_init(struct cpuidle_driver *drv) -{ - struct cpuidle_device *dev; - int cpu_id, ret; - - if (drv->state_count > CPUIDLE_STATE_MAX) { - pr_err("%s: state_count exceeds maximum\n", __func__); - return -EINVAL; - } - - ret = cpuidle_register_driver(drv); - if (ret) { - pr_err("%s: Failed to register cpuidle driver with error: %d\n", - __func__, ret); - return ret; - } - - imx_cpuidle_devices = alloc_percpu(struct cpuidle_device); - if (imx_cpuidle_devices == NULL) { - ret = -ENOMEM; - goto unregister_drv; - } - - /* initialize state data for each cpuidle_device */ - for_each_possible_cpu(cpu_id) { - dev = per_cpu_ptr(imx_cpuidle_devices, cpu_id); - dev->cpu = cpu_id; - dev->state_count = drv->state_count; - - ret = cpuidle_register_device(dev); - if (ret) { - pr_err("%s: Failed to register cpu %u, error: %d\n", - __func__, cpu_id, ret); - goto uninit; - } - } - - return 0; - -uninit: - imx_cpuidle_devices_uninit(); - -unregister_drv: - cpuidle_unregister_driver(drv); - return ret; -} diff --git a/arch/arm/mach-imx/cpuidle.h b/arch/arm/mach-imx/cpuidle.h index e092d13..786f98e 100644 --- a/arch/arm/mach-imx/cpuidle.h +++ b/arch/arm/mach-imx/cpuidle.h @@ -10,18 +10,16 @@ * http://www.gnu.org/copyleft/gpl.html */
-#include <linux/cpuidle.h> - #ifdef CONFIG_CPU_IDLE -extern int imx_cpuidle_init(struct cpuidle_driver *drv); +extern int imx5_cpuidle_init(void); extern int imx6q_cpuidle_init(void); #else -static inline int imx_cpuidle_init(struct cpuidle_driver *drv) +static inline int imx5_cpuidle_init(void) { - return -ENODEV; + return 0; } static inline int imx6q_cpuidle_init(void) { - return -ENODEV; + return 0; } #endif diff --git a/arch/arm/mach-imx/pm-imx5.c b/arch/arm/mach-imx/pm-imx5.c index 4b52b3e..82e79c6 100644 --- a/arch/arm/mach-imx/pm-imx5.c +++ b/arch/arm/mach-imx/pm-imx5.c @@ -149,32 +149,6 @@ static void imx5_pm_idle(void) imx5_cpu_do_idle(); }
-static int imx5_cpuidle_enter(struct cpuidle_device *dev, - struct cpuidle_driver *drv, int idx) -{ - int ret; - - ret = imx5_cpu_do_idle(); - if (ret < 0) - return ret; - - return idx; -} - -static struct cpuidle_driver imx5_cpuidle_driver = { - .name = "imx5_cpuidle", - .owner = THIS_MODULE, - .states[0] = { - .enter = imx5_cpuidle_enter, - .exit_latency = 2, - .target_residency = 1, - .flags = CPUIDLE_FLAG_TIME_VALID, - .name = "IMX5 SRPG", - .desc = "CPU state retained,powered off", - }, - .state_count = 1, -}; - static int __init imx5_pm_common_init(void) { int ret; @@ -192,8 +166,7 @@ static int __init imx5_pm_common_init(void) /* Set the registers to the default cpu idle state. */ mx5_cpu_lp_set(IMX5_DEFAULT_CPU_IDLE_STATE);
- imx_cpuidle_init(&imx5_cpuidle_driver); - return 0; + return imx5_cpuidle_init(); }
void __init imx51_pm_init(void)
On Wed, Apr 10, 2013 at 04:22:09PM +0200, Daniel Lezcano wrote:
The code intializes the cpuidle driver at different places. The cpuidle driver for :
- imx5 : is in the pm-imx5.c, the init function is in cpuidle.c
- imx6 : is in cpuidle-imx6q.c, the init function is in cpuidle.c and cpuidle-imx6q.c
Instead of having the cpuidle code spread across different files, let's write a driver for each SoC and make the code similar.
That implies some code duplication but that will be fixed with the next patches which consolidate the initialization for all the drivers.
IMO, this is unnecessary churn. I agree that we can have cpuidle-imx5.c instead of carrying imx5 cpuidle code in pm-imx5.c. But removing cpuidle.c and duplicating what imx_cpuidle_init() does into imx5 and imx6q driver is a step backward to me.
I suggest simply merging this patch into "[PATCH 18/18] ARM: imx: cpuidle: use init/exit common routine"
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
arch/arm/mach-imx/Makefile | 1 + arch/arm/mach-imx/cpuidle-imx5.c | 74 ++++++++++++++++++++++++++++++++++ arch/arm/mach-imx/cpuidle-imx6q.c | 36 ++++++++++++++++- arch/arm/mach-imx/cpuidle.c | 80 ------------------------------------- arch/arm/mach-imx/cpuidle.h | 10 ++--- arch/arm/mach-imx/pm-imx5.c | 29 +------------- 6 files changed, 115 insertions(+), 115 deletions(-) create mode 100644 arch/arm/mach-imx/cpuidle-imx5.c delete mode 100644 arch/arm/mach-imx/cpuidle.c
diff --git a/arch/arm/mach-imx/Makefile b/arch/arm/mach-imx/Makefile index c4ce090..bfd5f5b 100644 --- a/arch/arm/mach-imx/Makefile +++ b/arch/arm/mach-imx/Makefile @@ -31,6 +31,7 @@ obj-$(CONFIG_CPU_FREQ_IMX) += cpufreq.o ifeq ($(CONFIG_CPU_IDLE),y) obj-y += cpuidle.o
This target should be removed, since arch/arm/mach-imx/cpuidle.c goes away.
Shawn
+obj-$(CONFIG_SOC_IMX5) += cpuidle-imx5.o obj-$(CONFIG_SOC_IMX6Q) += cpuidle-imx6q.o endif
On 04/12/2013 08:05 AM, Shawn Guo wrote:
On Wed, Apr 10, 2013 at 04:22:09PM +0200, Daniel Lezcano wrote:
The code intializes the cpuidle driver at different places. The cpuidle driver for :
- imx5 : is in the pm-imx5.c, the init function is in cpuidle.c
- imx6 : is in cpuidle-imx6q.c, the init function is in cpuidle.c and cpuidle-imx6q.c
Instead of having the cpuidle code spread across different files, let's write a driver for each SoC and make the code similar.
That implies some code duplication but that will be fixed with the next patches which consolidate the initialization for all the drivers.
IMO, this is unnecessary churn. I agree that we can have cpuidle-imx5.c instead of carrying imx5 cpuidle code in pm-imx5.c. But removing cpuidle.c and duplicating what imx_cpuidle_init() does into imx5 and imx6q driver is a step backward to me. I suggest simply merging this patch into "[PATCH 18/18] ARM: imx: cpuidle: use init/exit common routine"
Yes, I am aware that can can look weird but that was to have the different steps to reach the common register function. If I merge this patch with the patch 18, I am afraid the modification won't be obvious to the one who will read the patch later (eg. for a git bisect).
It is quite easy to fold the patches, but with the comment above do you still want me to do that ?
Thanks -- Daniel
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
arch/arm/mach-imx/Makefile | 1 + arch/arm/mach-imx/cpuidle-imx5.c | 74 ++++++++++++++++++++++++++++++++++ arch/arm/mach-imx/cpuidle-imx6q.c | 36 ++++++++++++++++- arch/arm/mach-imx/cpuidle.c | 80 ------------------------------------- arch/arm/mach-imx/cpuidle.h | 10 ++--- arch/arm/mach-imx/pm-imx5.c | 29 +------------- 6 files changed, 115 insertions(+), 115 deletions(-) create mode 100644 arch/arm/mach-imx/cpuidle-imx5.c delete mode 100644 arch/arm/mach-imx/cpuidle.c
diff --git a/arch/arm/mach-imx/Makefile b/arch/arm/mach-imx/Makefile index c4ce090..bfd5f5b 100644 --- a/arch/arm/mach-imx/Makefile +++ b/arch/arm/mach-imx/Makefile @@ -31,6 +31,7 @@ obj-$(CONFIG_CPU_FREQ_IMX) += cpufreq.o ifeq ($(CONFIG_CPU_IDLE),y) obj-y += cpuidle.o
This target should be removed, since arch/arm/mach-imx/cpuidle.c goes away.
Shawn
+obj-$(CONFIG_SOC_IMX5) += cpuidle-imx5.o obj-$(CONFIG_SOC_IMX6Q) += cpuidle-imx6q.o endif
linaro-kernel mailing list linaro-kernel@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-kernel
On Fri, Apr 12, 2013 at 08:58:52AM +0200, Daniel Lezcano wrote:
On 04/12/2013 08:05 AM, Shawn Guo wrote:
On Wed, Apr 10, 2013 at 04:22:09PM +0200, Daniel Lezcano wrote:
The code intializes the cpuidle driver at different places. The cpuidle driver for :
- imx5 : is in the pm-imx5.c, the init function is in cpuidle.c
- imx6 : is in cpuidle-imx6q.c, the init function is in cpuidle.c and cpuidle-imx6q.c
Instead of having the cpuidle code spread across different files, let's write a driver for each SoC and make the code similar.
That implies some code duplication but that will be fixed with the next patches which consolidate the initialization for all the drivers.
IMO, this is unnecessary churn. I agree that we can have cpuidle-imx5.c instead of carrying imx5 cpuidle code in pm-imx5.c. But removing cpuidle.c and duplicating what imx_cpuidle_init() does into imx5 and imx6q driver is a step backward to me. I suggest simply merging this patch into "[PATCH 18/18] ARM: imx: cpuidle: use init/exit common routine"
Yes, I am aware that can can look weird but that was to have the different steps to reach the common register function. If I merge this patch with the patch 18, I am afraid the modification won't be obvious to the one who will read the patch later (eg. for a git bisect).
It is quite easy to fold the patches, but with the comment above do you still want me to do that ?
You can have a separate patch introducing cpuidle-imx5.c, but please do not duplicate what imx_cpuidle_init() does into cpuidle-imx5.c and cpuidle-imx6q.c.
Shawn
On 04/12/2013 09:11 AM, Shawn Guo wrote:
On Fri, Apr 12, 2013 at 08:58:52AM +0200, Daniel Lezcano wrote:
On 04/12/2013 08:05 AM, Shawn Guo wrote:
On Wed, Apr 10, 2013 at 04:22:09PM +0200, Daniel Lezcano wrote:
The code intializes the cpuidle driver at different places. The cpuidle driver for :
- imx5 : is in the pm-imx5.c, the init function is in cpuidle.c
- imx6 : is in cpuidle-imx6q.c, the init function is in cpuidle.c and cpuidle-imx6q.c
Instead of having the cpuidle code spread across different files, let's write a driver for each SoC and make the code similar.
That implies some code duplication but that will be fixed with the next patches which consolidate the initialization for all the drivers.
IMO, this is unnecessary churn. I agree that we can have cpuidle-imx5.c instead of carrying imx5 cpuidle code in pm-imx5.c. But removing cpuidle.c and duplicating what imx_cpuidle_init() does into imx5 and imx6q driver is a step backward to me. I suggest simply merging this patch into "[PATCH 18/18] ARM: imx: cpuidle: use init/exit common routine"
Yes, I am aware that can can look weird but that was to have the different steps to reach the common register function. If I merge this patch with the patch 18, I am afraid the modification won't be obvious to the one who will read the patch later (eg. for a git bisect).
It is quite easy to fold the patches, but with the comment above do you still want me to do that ?
You can have a separate patch introducing cpuidle-imx5.c, but please do not duplicate what imx_cpuidle_init() does into cpuidle-imx5.c and cpuidle-imx6q.c.
Sure.
Thanks !
-- Daniel
The usual scheme to initialize a cpuidle driver on a SMP is:
cpuidle_register_driver(drv); for_each_possible_cpu(cpu) { device = &per_cpu(cpuidle_dev, cpu); cpuidle_register_device(device); }
This code is duplicated in each cpuidle driver.
On UP systems, it is done this way:
cpuidle_register_driver(drv); device = &per_cpu(cpuidle_dev, cpu); cpuidle_register_device(device);
On UP, the macro 'for_each_cpu' does one iteration:
#define for_each_cpu(cpu, mask) \ for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask)
Hence, the initialization loop is the same for UP than SMP.
Beside, we saw different bugs / mis-initialization / return code unchecked in the different drivers, the code is duplicated including bugs. After fixing all these ones, it appears the initialization pattern is the same for everyone.
Let's add a wrapper function doing this initialization with a cpumask parameter for the coupled idle states and use it for all the drivers.
That will save a lot of LOC, consolidate the code, and the modifications in the future could be done in a single place. Another benefit is the consolidation of the cpuidle_device variable which is now in the cpuidle framework and no longer spread accross the different arch specific drivers.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- drivers/cpuidle/cpuidle.c | 65 +++++++++++++++++++++++++++++++++++++++++++++ include/linux/cpuidle.h | 9 +++++-- 2 files changed, 72 insertions(+), 2 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index 0da795b..3a5454b 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -24,6 +24,7 @@ #include "cpuidle.h"
DEFINE_PER_CPU(struct cpuidle_device *, cpuidle_devices); +DEFINE_PER_CPU(struct cpuidle_device, cpuidle_dev);
DEFINE_MUTEX(cpuidle_lock); LIST_HEAD(cpuidle_detected_devices); @@ -453,6 +454,70 @@ void cpuidle_unregister_device(struct cpuidle_device *dev)
EXPORT_SYMBOL_GPL(cpuidle_unregister_device);
+/* + * cpuidle_unregister: unregister a driver and the devices. This function + * can be used only if the driver has been previously registered through + * the cpuidle_register function. + * + * @drv: a valid pointer to a struct cpuidle_driver + */ +void cpuidle_unregister(struct cpuidle_driver *drv) +{ + int cpu; + struct cpuidle_device *device; + + for_each_possible_cpu(cpu) { + device = &per_cpu(cpuidle_dev, cpu); + cpuidle_unregister_device(device); + } + + cpuidle_unregister_driver(drv); +} +EXPORT_SYMBOL_GPL(cpuidle_unregister); + +/** + * cpuidle_register: registers the driver and the cpu devices with the + * coupled_cpus passed as parameter. This function is used for all common + * initialization pattern there are in the arch specific drivers. The + * devices is globally defined in this file. + * + * @drv : a valid pointer to a struct cpuidle_driver + * @coupled_cpus: a cpumask for the coupled states + * + * Returns 0 on success, < 0 otherwise + */ +int cpuidle_register(struct cpuidle_driver *drv, + const struct cpumask *const coupled_cpus) +{ + int ret, cpu; + struct cpuidle_device *device; + + ret = cpuidle_register_driver(drv); + if (ret) { + printk(KERN_ERR "failed to register cpuidle driver\n"); + return ret; + } + + for_each_possible_cpu(cpu) { + device = &per_cpu(cpuidle_dev, cpu); + device->cpu = cpu; +#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED + device->coupled_cpus = *coupled_cpus; +#endif + ret = cpuidle_register_device(device); + if (!ret) + continue; + + printk(KERN_ERR "Failed to register cpuidle " + "device for cpu%d\n", cpu); + cpuidle_unregister(drv); + break; + } + + return 0; +} +EXPORT_SYMBOL_GPL(cpuidle_register); + #ifdef CONFIG_SMP
static void smp_callback(void *v) diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 79e3811..3c86faa 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -123,7 +123,9 @@ extern void cpuidle_driver_unref(void); extern void cpuidle_unregister_driver(struct cpuidle_driver *drv); extern int cpuidle_register_device(struct cpuidle_device *dev); extern void cpuidle_unregister_device(struct cpuidle_device *dev); - +extern int cpuidle_register(struct cpuidle_driver *drv, + const struct cpumask *const coupled_cpus); +extern void cpuidle_unregister(struct cpuidle_driver *drv); extern void cpuidle_pause_and_lock(void); extern void cpuidle_resume_and_unlock(void); extern void cpuidle_pause(void); @@ -148,7 +150,10 @@ static inline void cpuidle_unregister_driver(struct cpuidle_driver *drv) { } static inline int cpuidle_register_device(struct cpuidle_device *dev) {return -ENODEV; } static inline void cpuidle_unregister_device(struct cpuidle_device *dev) { } - +static inline int cpuidle_register(struct cpuidle_driver *drv, + const struct cpumask *const coupled_cpus) +{return -ENODEV; } +static inline void cpuidle_unregister(struct cpuidle_driver *drv) { } static inline void cpuidle_pause_and_lock(void) { } static inline void cpuidle_resume_and_unlock(void) { } static inline void cpuidle_pause(void) { }
On 04/10/2013 09:22 AM, Daniel Lezcano wrote:
The usual scheme to initialize a cpuidle driver on a SMP is:
cpuidle_register_driver(drv); for_each_possible_cpu(cpu) { device = &per_cpu(cpuidle_dev, cpu); cpuidle_register_device(device); }
This code is duplicated in each cpuidle driver.
On UP systems, it is done this way:
cpuidle_register_driver(drv); device = &per_cpu(cpuidle_dev, cpu); cpuidle_register_device(device);
On UP, the macro 'for_each_cpu' does one iteration:
#define for_each_cpu(cpu, mask) \ for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask)
Hence, the initialization loop is the same for UP than SMP.
Beside, we saw different bugs / mis-initialization / return code unchecked in the different drivers, the code is duplicated including bugs. After fixing all these ones, it appears the initialization pattern is the same for everyone.
Let's add a wrapper function doing this initialization with a cpumask parameter for the coupled idle states and use it for all the drivers.
That will save a lot of LOC, consolidate the code, and the modifications in the future could be done in a single place. Another benefit is the consolidation of the cpuidle_device variable which is now in the cpuidle framework and no longer spread accross the different arch specific drivers.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
Perhaps my ack was too quick...
+int cpuidle_register(struct cpuidle_driver *drv,
const struct cpumask *const coupled_cpus)+{
- int ret, cpu;
- struct cpuidle_device *device;
- ret = cpuidle_register_driver(drv);
- if (ret) {
printk(KERN_ERR "failed to register cpuidle driver\n");return ret;- }
- for_each_possible_cpu(cpu) {
device = &per_cpu(cpuidle_dev, cpu);device->cpu = cpu;+#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED
device->coupled_cpus = *coupled_cpus;
This needs a NULL check. CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED is most likely going to be enabled with multi-platform kernels and this will crash.
Also, I think typically struct copies have a note that it is copy.
Rob
On 04/10/2013 04:59 PM, Rob Herring wrote:
On 04/10/2013 09:22 AM, Daniel Lezcano wrote:
The usual scheme to initialize a cpuidle driver on a SMP is:
cpuidle_register_driver(drv); for_each_possible_cpu(cpu) { device = &per_cpu(cpuidle_dev, cpu); cpuidle_register_device(device); }
This code is duplicated in each cpuidle driver.
On UP systems, it is done this way:
cpuidle_register_driver(drv); device = &per_cpu(cpuidle_dev, cpu); cpuidle_register_device(device);
On UP, the macro 'for_each_cpu' does one iteration:
#define for_each_cpu(cpu, mask) \ for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask)
Hence, the initialization loop is the same for UP than SMP.
Beside, we saw different bugs / mis-initialization / return code unchecked in the different drivers, the code is duplicated including bugs. After fixing all these ones, it appears the initialization pattern is the same for everyone.
Let's add a wrapper function doing this initialization with a cpumask parameter for the coupled idle states and use it for all the drivers.
That will save a lot of LOC, consolidate the code, and the modifications in the future could be done in a single place. Another benefit is the consolidation of the cpuidle_device variable which is now in the cpuidle framework and no longer spread accross the different arch specific drivers.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
Perhaps my ack was too quick...
+int cpuidle_register(struct cpuidle_driver *drv,
const struct cpumask *const coupled_cpus)+{
- int ret, cpu;
- struct cpuidle_device *device;
- ret = cpuidle_register_driver(drv);
- if (ret) {
printk(KERN_ERR "failed to register cpuidle driver\n");return ret;- }
- for_each_possible_cpu(cpu) {
device = &per_cpu(cpuidle_dev, cpu);device->cpu = cpu;+#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED
device->coupled_cpus = *coupled_cpus;This needs a NULL check. CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED is most likely going to be enabled with multi-platform kernels and this will crash.
Good point, I will fix it.
Also, I think typically struct copies have a note that it is copy.
Rob
On 04/10/2013 04:59 PM, Rob Herring wrote:
On 04/10/2013 09:22 AM, Daniel Lezcano wrote:
The usual scheme to initialize a cpuidle driver on a SMP is:
cpuidle_register_driver(drv); for_each_possible_cpu(cpu) { device = &per_cpu(cpuidle_dev, cpu); cpuidle_register_device(device); }
This code is duplicated in each cpuidle driver.
On UP systems, it is done this way:
cpuidle_register_driver(drv); device = &per_cpu(cpuidle_dev, cpu); cpuidle_register_device(device);
On UP, the macro 'for_each_cpu' does one iteration:
#define for_each_cpu(cpu, mask) \ for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask)
Hence, the initialization loop is the same for UP than SMP.
Beside, we saw different bugs / mis-initialization / return code unchecked in the different drivers, the code is duplicated including bugs. After fixing all these ones, it appears the initialization pattern is the same for everyone.
Let's add a wrapper function doing this initialization with a cpumask parameter for the coupled idle states and use it for all the drivers.
That will save a lot of LOC, consolidate the code, and the modifications in the future could be done in a single place. Another benefit is the consolidation of the cpuidle_device variable which is now in the cpuidle framework and no longer spread accross the different arch specific drivers.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
Perhaps my ack was too quick...
Hi Rob,
now that I have fixed the routine in V3, shall I consider the patch is acked ?
Thanks -- Daniel
+/**
- cpuidle_register: registers the driver and the cpu devices with the
- coupled_cpus passed as parameter. This function is used for all common
- initialization pattern there are in the arch specific drivers. The
- devices is globally defined in this file.
- @drv : a valid pointer to a struct cpuidle_driver
- @coupled_cpus: a cpumask for the coupled states
- Returns 0 on success, < 0 otherwise
- */
+int cpuidle_register(struct cpuidle_driver *drv,
const struct cpumask *const coupled_cpus)+{
- int ret, cpu;
- struct cpuidle_device *device;
- ret = cpuidle_register_driver(drv);
- if (ret) {
printk(KERN_ERR "failed to register cpuidle driver\n");
pr_err()
return ret;- }
- for_each_possible_cpu(cpu) {
device = &per_cpu(cpuidle_dev, cpu);device->cpu = cpu;+#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED
device->coupled_cpus = *coupled_cpus;+#endif
ret = cpuidle_register_device(device);if (!ret)continue;printk(KERN_ERR "Failed to register cpuidle ""device for cpu%d\n", cpu);
pr_err() and don't split the message over two lines, it makes it harder for somebody to find with
grep -r "Failed to register cpuidle device for cpu" *
cpuidle_unregister(drv);break;- }
- return 0;
You should return an error code, so that the caller can also return an error code. If you look at cpuidle-kirkwood and cpuidle-calxeda, and maybe others, if the registration fails, the probe function returns an error code, as it should. With your change, its always going to return 0, even if it fails.
Andrew
On 04/10/2013 06:55 PM, Andrew Lunn wrote:
+/**
- cpuidle_register: registers the driver and the cpu devices with the
- coupled_cpus passed as parameter. This function is used for all common
- initialization pattern there are in the arch specific drivers. The
- devices is globally defined in this file.
- @drv : a valid pointer to a struct cpuidle_driver
- @coupled_cpus: a cpumask for the coupled states
- Returns 0 on success, < 0 otherwise
- */
+int cpuidle_register(struct cpuidle_driver *drv,
const struct cpumask *const coupled_cpus)+{
- int ret, cpu;
- struct cpuidle_device *device;
- ret = cpuidle_register_driver(drv);
- if (ret) {
printk(KERN_ERR "failed to register cpuidle driver\n");pr_err()
Ok.
return ret;- }
- for_each_possible_cpu(cpu) {
device = &per_cpu(cpuidle_dev, cpu);device->cpu = cpu;+#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED
device->coupled_cpus = *coupled_cpus;+#endif
ret = cpuidle_register_device(device);if (!ret)continue;printk(KERN_ERR "Failed to register cpuidle ""device for cpu%d\n", cpu);pr_err() and don't split the message over two lines, it makes it harder for somebody to find with
grep -r "Failed to register cpuidle device for cpu" *
Ok if the line length is under 80 chars.
cpuidle_unregister(drv);break;- }
- return 0;
You should return an error code, so that the caller can also return an error code. If you look at cpuidle-kirkwood and cpuidle-calxeda, and maybe others, if the registration fails, the probe function returns an error code, as it should. With your change, its always going to return 0, even if it fails.
Right, right :)
it should be 'return ret;'
Thanks for pointing this.
-- Daniel
On Wed, Apr 10, 2013 at 08:04:22PM +0200, Daniel Lezcano wrote:
On 04/10/2013 06:55 PM, Andrew Lunn wrote:
+/**
- cpuidle_register: registers the driver and the cpu devices with the
- coupled_cpus passed as parameter. This function is used for all common
- initialization pattern there are in the arch specific drivers. The
- devices is globally defined in this file.
- @drv : a valid pointer to a struct cpuidle_driver
- @coupled_cpus: a cpumask for the coupled states
- Returns 0 on success, < 0 otherwise
- */
+int cpuidle_register(struct cpuidle_driver *drv,
const struct cpumask *const coupled_cpus)+{
- int ret, cpu;
- struct cpuidle_device *device;
- ret = cpuidle_register_driver(drv);
- if (ret) {
printk(KERN_ERR "failed to register cpuidle driver\n");pr_err()
Ok.
return ret;- }
- for_each_possible_cpu(cpu) {
device = &per_cpu(cpuidle_dev, cpu);device->cpu = cpu;+#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED
device->coupled_cpus = *coupled_cpus;+#endif
ret = cpuidle_register_device(device);if (!ret)continue;printk(KERN_ERR "Failed to register cpuidle ""device for cpu%d\n", cpu);pr_err() and don't split the message over two lines, it makes it harder for somebody to find with
grep -r "Failed to register cpuidle device for cpu" *
Ok if the line length is under 80 chars.
Take a closer look at the CodingStyle documents. This is one except to the rule. Such lines can be longer than 80 characters.
Andrew
On 04/10/2013 08:18 PM, Andrew Lunn wrote:
On Wed, Apr 10, 2013 at 08:04:22PM +0200, Daniel Lezcano wrote:
On 04/10/2013 06:55 PM, Andrew Lunn wrote:
+/**
- cpuidle_register: registers the driver and the cpu devices with the
- coupled_cpus passed as parameter. This function is used for all common
- initialization pattern there are in the arch specific drivers. The
- devices is globally defined in this file.
- @drv : a valid pointer to a struct cpuidle_driver
- @coupled_cpus: a cpumask for the coupled states
- Returns 0 on success, < 0 otherwise
- */
+int cpuidle_register(struct cpuidle_driver *drv,
const struct cpumask *const coupled_cpus)+{
- int ret, cpu;
- struct cpuidle_device *device;
- ret = cpuidle_register_driver(drv);
- if (ret) {
printk(KERN_ERR "failed to register cpuidle driver\n");pr_err()
Ok.
return ret;- }
- for_each_possible_cpu(cpu) {
device = &per_cpu(cpuidle_dev, cpu);device->cpu = cpu;+#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED
device->coupled_cpus = *coupled_cpus;+#endif
ret = cpuidle_register_device(device);if (!ret)continue;printk(KERN_ERR "Failed to register cpuidle ""device for cpu%d\n", cpu);pr_err() and don't split the message over two lines, it makes it harder for somebody to find with
grep -r "Failed to register cpuidle device for cpu" *
Ok if the line length is under 80 chars.
Take a closer look at the CodingStyle documents. This is one except to the rule. Such lines can be longer than 80 characters.
Oh, right !
"However, never break user-visible strings such as printk messages, because that breaks the ability to grep for them."
Will fix it, thanks.
-- Daniel
+int cpuidle_register(struct cpuidle_driver *drv,
const struct cpumask *const coupled_cpus)+{
- int ret, cpu;
- struct cpuidle_device *device;
- ret = cpuidle_register_driver(drv);
- if (ret) {
printk(KERN_ERR "failed to register cpuidle driver\n");return ret;- }
- for_each_possible_cpu(cpu) {
device = &per_cpu(cpuidle_dev, cpu);device->cpu = cpu;+#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED
device->coupled_cpus = *coupled_cpus;+#endif
At least the kirkwood and the calxeda driver set
device->state_count
which you don't appear to do. cpuidle_add_state_sysfs() and cpuidle_remove_state_sysfs() use this. Is it now being set somewhere else?
Thanks Andrew
On 04/10/2013 07:04 PM, Andrew Lunn wrote:
+int cpuidle_register(struct cpuidle_driver *drv,
const struct cpumask *const coupled_cpus)+{
- int ret, cpu;
- struct cpuidle_device *device;
- ret = cpuidle_register_driver(drv);
- if (ret) {
printk(KERN_ERR "failed to register cpuidle driver\n");return ret;- }
- for_each_possible_cpu(cpu) {
device = &per_cpu(cpuidle_dev, cpu);device->cpu = cpu;+#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED
device->coupled_cpus = *coupled_cpus;+#endif
At least the kirkwood and the calxeda driver set
device->state_count
which you don't appear to do. cpuidle_add_state_sysfs() and cpuidle_remove_state_sysfs() use this. Is it now being set somewhere else?
Yes, in cpuidle_enable_device called from cpuidle_register_device:
int cpuidle_enable_device(struct cpuidle_device *dev) { ...
if (!dev->state_count) dev->state_count = drv->state_count;
... }
On Wed, Apr 10, 2013 at 08:02:31PM +0200, Daniel Lezcano wrote:
On 04/10/2013 07:04 PM, Andrew Lunn wrote:
+int cpuidle_register(struct cpuidle_driver *drv,
const struct cpumask *const coupled_cpus)+{
- int ret, cpu;
- struct cpuidle_device *device;
- ret = cpuidle_register_driver(drv);
- if (ret) {
printk(KERN_ERR "failed to register cpuidle driver\n");return ret;- }
- for_each_possible_cpu(cpu) {
device = &per_cpu(cpuidle_dev, cpu);device->cpu = cpu;+#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED
device->coupled_cpus = *coupled_cpus;+#endif
At least the kirkwood and the calxeda driver set
device->state_count
which you don't appear to do. cpuidle_add_state_sysfs() and cpuidle_remove_state_sysfs() use this. Is it now being set somewhere else?
Yes, in cpuidle_enable_device called from cpuidle_register_device:
O.K. It would be nice to add a comment in the change log message about this.
Ah, also, it would be good to update Documentation/cpuidle/drivers.txt with these new functions and update the text.
Thanks Andrew
On 04/10/2013 08:23 PM, Andrew Lunn wrote:
On Wed, Apr 10, 2013 at 08:02:31PM +0200, Daniel Lezcano wrote:
On 04/10/2013 07:04 PM, Andrew Lunn wrote:
+int cpuidle_register(struct cpuidle_driver *drv,
const struct cpumask *const coupled_cpus)+{
- int ret, cpu;
- struct cpuidle_device *device;
- ret = cpuidle_register_driver(drv);
- if (ret) {
printk(KERN_ERR "failed to register cpuidle driver\n");return ret;- }
- for_each_possible_cpu(cpu) {
device = &per_cpu(cpuidle_dev, cpu);device->cpu = cpu;+#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED
device->coupled_cpus = *coupled_cpus;+#endif
At least the kirkwood and the calxeda driver set
device->state_count
which you don't appear to do. cpuidle_add_state_sysfs() and cpuidle_remove_state_sysfs() use this. Is it now being set somewhere else?
Yes, in cpuidle_enable_device called from cpuidle_register_device:
O.K. It would be nice to add a comment in the change log message about this.
Ah, also, it would be good to update Documentation/cpuidle/drivers.txt with these new functions and update the text.
Ok, I will take care of that.
Thanks. -- Daniel
On Wed, Apr 10, 2013 at 7:22 AM, Daniel Lezcano daniel.lezcano@linaro.org wrote:
The usual scheme to initialize a cpuidle driver on a SMP is:
cpuidle_register_driver(drv); for_each_possible_cpu(cpu) { device = &per_cpu(cpuidle_dev, cpu); cpuidle_register_device(device); }This code is duplicated in each cpuidle driver.
On UP systems, it is done this way:
cpuidle_register_driver(drv); device = &per_cpu(cpuidle_dev, cpu); cpuidle_register_device(device);On UP, the macro 'for_each_cpu' does one iteration:
#define for_each_cpu(cpu, mask) \ for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask)
Hence, the initialization loop is the same for UP than SMP.
Beside, we saw different bugs / mis-initialization / return code unchecked in the different drivers, the code is duplicated including bugs. After fixing all these ones, it appears the initialization pattern is the same for everyone.
Let's add a wrapper function doing this initialization with a cpumask parameter for the coupled idle states and use it for all the drivers.
Isn't this going to cause problems for big.LITTLE, which has two independent sets of coupled cpus, and may even have different drivers for the two types of cpu?
On 04/10/2013 08:22 PM, Colin Cross wrote:
On Wed, Apr 10, 2013 at 7:22 AM, Daniel Lezcano daniel.lezcano@linaro.org wrote:
The usual scheme to initialize a cpuidle driver on a SMP is:
cpuidle_register_driver(drv); for_each_possible_cpu(cpu) { device = &per_cpu(cpuidle_dev, cpu); cpuidle_register_device(device); }This code is duplicated in each cpuidle driver.
On UP systems, it is done this way:
cpuidle_register_driver(drv); device = &per_cpu(cpuidle_dev, cpu); cpuidle_register_device(device);On UP, the macro 'for_each_cpu' does one iteration:
#define for_each_cpu(cpu, mask) \ for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask)
Hence, the initialization loop is the same for UP than SMP.
Beside, we saw different bugs / mis-initialization / return code unchecked in the different drivers, the code is duplicated including bugs. After fixing all these ones, it appears the initialization pattern is the same for everyone.
Let's add a wrapper function doing this initialization with a cpumask parameter for the coupled idle states and use it for all the drivers.
Isn't this going to cause problems for big.LITTLE, which has two independent sets of coupled cpus, and may even have different drivers for the two types of cpu?
AFAICT no, because the "low levels" API are still there, I mean cpuidle_register_driver and cpuidle_register_device. That will be up to the b.L driver to use the API which fits its need.
Remove the duplicate code and use the cpuidle 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 5657d4a..488e074 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) @@ -112,40 +111,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_possible(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_possible_cpu(cpu) { - device = &per_cpu(ux500_cpuidle_device, cpu); - cpuidle_unregister_device(device); - } - - cpuidle_unregister_driver(&ux500_idle_driver); - goto out; + return cpuidle_register(&ux500_idle_driver, NULL); }
device_initcall(ux500_idle_init);
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 0130df7..48f1228 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, @@ -60,20 +58,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 cpuidle_register(&at91_idle_driver, NULL); }
device_initcall(at91_init_cpuidle);
On 04/10/2013 04:22 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
Seems okay:
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 0130df7..48f1228 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, @@ -60,20 +58,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 cpuidle_register(&at91_idle_driver, NULL);
} device_initcall(at91_init_cpuidle);
Remove the duplicated code and use the cpuidle 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 027a787..cca045c 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" @@ -259,8 +260,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, @@ -336,8 +335,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"); @@ -346,16 +343,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 cpuidle_register(&omap3_idle_driver, NULL); }
Remove the duplicated code and use the cpuidle common code for initialization.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-s3c64xx/cpuidle.c | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-)
diff --git a/arch/arm/mach-s3c64xx/cpuidle.c b/arch/arm/mach-s3c64xx/cpuidle.c index 852ff16..3c8ab07 100644 --- a/arch/arm/mach-s3c64xx/cpuidle.c +++ b/arch/arm/mach-s3c64xx/cpuidle.c @@ -40,8 +40,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, @@ -60,16 +58,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 cpuidle_register(&s3c64xx_cpuidle_driver, NULL); } device_initcall(s3c64xx_init_cpuidle);
Remove the duplicated code and use the cpuidle common code for initialization.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-tegra/cpuidle-tegra114.c | 26 +------------------------- 1 file changed, 1 insertion(+), 25 deletions(-)
diff --git a/arch/arm/mach-tegra/cpuidle-tegra114.c b/arch/arm/mach-tegra/cpuidle-tegra114.c index c5fadf9..1d1c602 100644 --- a/arch/arm/mach-tegra/cpuidle-tegra114.c +++ b/arch/arm/mach-tegra/cpuidle-tegra114.c @@ -29,31 +29,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; - - ret = cpuidle_register_device(dev); - if (ret) { - pr_err("CPU%u: CPUidle device registration failed\n", - cpu); - return ret; - } - } - return 0; + return cpuidle_register(&tegra_idle_driver, NULL); }
On 04/10/2013 08:22 AM, Daniel Lezcano wrote:
Subject: ARM: tegra1: cpuidle: use init/exit common routine
The tag there should be just ARM: tegra: - Tegra1 isn't something that's supported upstream. Same comment for the other two Tegra patches.
Remove the duplicated code and use the cpuidle common code for initialization.
Other than that, the 3 Tegra patches look reasonable assuming the patches that implement the new common APIs are accepted, so the 3 Tegra patches,
Acked-by: Stephen Warren swarren@nvidia.com
On 04/10/2013 07:29 PM, Stephen Warren wrote:
On 04/10/2013 08:22 AM, Daniel Lezcano wrote:
Subject: ARM: tegra1: cpuidle: use init/exit common routine
The tag there should be just ARM: tegra: - Tegra1 isn't something that's supported upstream. Same comment for the other two Tegra patches.
Ok.
Remove the duplicated code and use the cpuidle common code for initialization.
Other than that, the 3 Tegra patches look reasonable assuming the patches that implement the new common APIs are accepted, so the 3 Tegra patches,
Acked-by: Stephen Warren swarren@nvidia.com
Thanks for review
-- Daniel
Remove the duplicated code and use the cpuidle 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 d671ae9..0afeb5c 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, @@ -34,12 +33,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 cpuidle_register(cpuidle_drv, NULL); }
On Wed, Apr 10, 2013 at 04:22:16PM +0200, Daniel Lezcano wrote:
Remove the duplicated code and use the cpuidle 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 d671ae9..0afeb5c 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, @@ -34,12 +33,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 cpuidle_register(cpuidle_drv, NULL);
}
1.7.9.5
Remove the duplicated code and use the cpuidle common code for initialization.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-omap2/cpuidle44xx.c | 22 ++-------------------- 1 file changed, 2 insertions(+), 20 deletions(-)
diff --git a/arch/arm/mach-omap2/cpuidle44xx.c b/arch/arm/mach-omap2/cpuidle44xx.c index cd188de..861dd90 100644 --- a/arch/arm/mach-omap2/cpuidle44xx.c +++ b/arch/arm/mach-omap2/cpuidle44xx.c @@ -15,6 +15,7 @@ #include <linux/cpu_pm.h> #include <linux/export.h>
+#include <asm/cpuidle.h> #include <asm/proc-fns.h>
#include "common.h" @@ -157,8 +158,6 @@ fail: return index; }
-static DEFINE_PER_CPU(struct cpuidle_device, omap4_idle_dev); - static struct cpuidle_driver omap4_idle_driver = { .name = "omap4_idle", .owner = THIS_MODULE, @@ -207,9 +206,6 @@ static struct cpuidle_driver omap4_idle_driver = { */ int __init omap4_idle_init(void) { - struct cpuidle_device *dev; - unsigned int cpu_id = 0; - mpu_pd = pwrdm_lookup("mpu_pwrdm"); cpu_pd[0] = pwrdm_lookup("cpu0_pwrdm"); cpu_pd[1] = pwrdm_lookup("cpu1_pwrdm"); @@ -221,19 +217,5 @@ int __init omap4_idle_init(void) if (!cpu_clkdm[0] || !cpu_clkdm[1]) return -ENODEV;
- for_each_cpu(cpu_id, cpu_online_mask) { - dev = &per_cpu(omap4_idle_dev, cpu_id); - dev->cpu = cpu_id; -#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED - dev->coupled_cpus = *cpu_online_mask; -#endif - cpuidle_register_driver(&omap4_idle_driver); - - if (cpuidle_register_device(dev)) { - pr_err("%s: CPUidle register failed\n", __func__); - return -EIO; - } - } - - return 0; + return cpuidle_register(&omap4_idle_driver, cpu_online_mask); }
Remove the duplicated code and use the cpuidle common code for initialization.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-tegra/cpuidle-tegra20.c | 33 +-------------------------------- 1 file changed, 1 insertion(+), 32 deletions(-)
diff --git a/arch/arm/mach-tegra/cpuidle-tegra20.c b/arch/arm/mach-tegra/cpuidle-tegra20.c index f1f6ac4..d6c2ba6 100644 --- a/arch/arm/mach-tegra/cpuidle-tegra20.c +++ b/arch/arm/mach-tegra/cpuidle-tegra20.c @@ -70,8 +70,6 @@ static struct cpuidle_driver tegra_idle_driver = { .safe_state_index = 0, };
-static DEFINE_PER_CPU(struct cpuidle_device, tegra_idle_device); - #ifdef CONFIG_PM_SLEEP #ifdef CONFIG_SMP static void __iomem *pmc = IO_ADDRESS(TEGRA_PMC_BASE); @@ -220,34 +218,5 @@ static int tegra20_idle_lp2_coupled(struct cpuidle_device *dev,
int __init tegra20_cpuidle_init(void) { - int ret; - unsigned int cpu; - struct cpuidle_device *dev; - struct cpuidle_driver *drv = &tegra_idle_driver; - -#ifdef CONFIG_PM_SLEEP - tegra_tear_down_cpu = tegra20_tear_down_cpu; -#endif - - 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; -#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED - dev->coupled_cpus = *cpu_possible_mask; -#endif - - ret = cpuidle_register_device(dev); - if (ret) { - pr_err("CPU%u: CPUidle device registration failed\n", - cpu); - return ret; - } - } - return 0; + return cpuidle_register(&tegra_idle_driver, cpu_possible_mask); }
On Wed, 2013-04-10 at 22:22 +0800, Daniel Lezcano wrote:
Remove the duplicated code and use the cpuidle common code for initialization.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
arch/arm/mach-tegra/cpuidle-tegra20.c | 33 +-------------------------------- 1 file changed, 1 insertion(+), 32 deletions(-)
diff --git a/arch/arm/mach-tegra/cpuidle-tegra20.c b/arch/arm/mach-tegra/cpuidle-tegra20.c index f1f6ac4..d6c2ba6 100644 --- a/arch/arm/mach-tegra/cpuidle-tegra20.c +++ b/arch/arm/mach-tegra/cpuidle-tegra20.c @@ -70,8 +70,6 @@ static struct cpuidle_driver tegra_idle_driver = { .safe_state_index = 0, }; -static DEFINE_PER_CPU(struct cpuidle_device, tegra_idle_device);
#ifdef CONFIG_PM_SLEEP #ifdef CONFIG_SMP static void __iomem *pmc = IO_ADDRESS(TEGRA_PMC_BASE); @@ -220,34 +218,5 @@ static int tegra20_idle_lp2_coupled(struct cpuidle_device *dev, int __init tegra20_cpuidle_init(void) {
- int ret;
- unsigned int cpu;
- struct cpuidle_device *dev;
- struct cpuidle_driver *drv = &tegra_idle_driver;
-#ifdef CONFIG_PM_SLEEP
- tegra_tear_down_cpu = tegra20_tear_down_cpu;
-#endif
Hi Daniel,
Please keep these 3 lines above, just like you did for Tegra30.
Thanks, Joseph
- return cpuidle_register(&tegra_idle_driver, cpu_possible_mask);
}
On 04/11/2013 03:01 AM, Joseph Lo wrote:
On Wed, 2013-04-10 at 22:22 +0800, Daniel Lezcano wrote:
Remove the duplicated code and use the cpuidle common code for initialization.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
arch/arm/mach-tegra/cpuidle-tegra20.c | 33 +-------------------------------- 1 file changed, 1 insertion(+), 32 deletions(-)
diff --git a/arch/arm/mach-tegra/cpuidle-tegra20.c b/arch/arm/mach-tegra/cpuidle-tegra20.c index f1f6ac4..d6c2ba6 100644 --- a/arch/arm/mach-tegra/cpuidle-tegra20.c +++ b/arch/arm/mach-tegra/cpuidle-tegra20.c @@ -70,8 +70,6 @@ static struct cpuidle_driver tegra_idle_driver = { .safe_state_index = 0, }; -static DEFINE_PER_CPU(struct cpuidle_device, tegra_idle_device);
#ifdef CONFIG_PM_SLEEP #ifdef CONFIG_SMP static void __iomem *pmc = IO_ADDRESS(TEGRA_PMC_BASE); @@ -220,34 +218,5 @@ static int tegra20_idle_lp2_coupled(struct cpuidle_device *dev, int __init tegra20_cpuidle_init(void) {
- int ret;
- unsigned int cpu;
- struct cpuidle_device *dev;
- struct cpuidle_driver *drv = &tegra_idle_driver;
-#ifdef CONFIG_PM_SLEEP
- tegra_tear_down_cpu = tegra20_tear_down_cpu;
-#endif
Hi Daniel,
Please keep these 3 lines above, just like you did for Tegra30.
Thanks, Joseph
Oups, thanks !
-- Daniel
Remove the duplicated code and use the cpuidle common code for initialization.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-tegra/cpuidle-tegra30.c | 27 +-------------------------- 1 file changed, 1 insertion(+), 26 deletions(-)
diff --git a/arch/arm/mach-tegra/cpuidle-tegra30.c b/arch/arm/mach-tegra/cpuidle-tegra30.c index f6a0c72..36dc2be 100644 --- a/arch/arm/mach-tegra/cpuidle-tegra30.c +++ b/arch/arm/mach-tegra/cpuidle-tegra30.c @@ -64,8 +64,6 @@ static struct cpuidle_driver tegra_idle_driver = { }, };
-static DEFINE_PER_CPU(struct cpuidle_device, tegra_idle_device); - #ifdef CONFIG_PM_SLEEP static bool tegra30_cpu_cluster_power_down(struct cpuidle_device *dev, struct cpuidle_driver *drv, @@ -156,31 +154,8 @@ static int tegra30_idle_lp2(struct cpuidle_device *dev,
int __init tegra30_cpuidle_init(void) { - int ret; - unsigned int cpu; - struct cpuidle_device *dev; - struct cpuidle_driver *drv = &tegra_idle_driver; - #ifdef CONFIG_PM_SLEEP tegra_tear_down_cpu = tegra30_tear_down_cpu; #endif - - 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; - - ret = cpuidle_register_device(dev); - if (ret) { - pr_err("CPU%u: CPUidle device registration failed\n", - cpu); - return ret; - } - } - return 0; + return cpuidle_register(&tegra_idle_driver, NULL); }
Remove the duplicated code and use the cpuidle common code for initialization.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- drivers/cpuidle/cpuidle-calxeda.c | 52 +------------------------------------ 1 file changed, 1 insertion(+), 51 deletions(-)
diff --git a/drivers/cpuidle/cpuidle-calxeda.c b/drivers/cpuidle/cpuidle-calxeda.c index a3b56f5..e344b56 100644 --- a/drivers/cpuidle/cpuidle-calxeda.c +++ b/drivers/cpuidle/cpuidle-calxeda.c @@ -35,8 +35,6 @@ extern void highbank_set_cpu_jump(int cpu, void *jump_addr); extern void *scu_base_addr;
-static struct cpuidle_device __percpu *calxeda_idle_cpuidle_devices; - static inline unsigned int get_auxcr(void) { unsigned int val; @@ -85,19 +83,6 @@ static int calxeda_pwrdown_idle(struct cpuidle_device *dev, return index; }
-static void calxeda_idle_cpuidle_devices_uninit(void) -{ - int i; - struct cpuidle_device *dev; - - for_each_possible_cpu(i) { - dev = per_cpu_ptr(calxeda_idle_cpuidle_devices, i); - cpuidle_unregister_device(dev); - } - - free_percpu(calxeda_idle_cpuidle_devices); -} - static struct cpuidle_driver calxeda_idle_driver = { .name = "calxeda_idle", .states = { @@ -117,44 +102,9 @@ static struct cpuidle_driver calxeda_idle_driver = {
static int __init calxeda_cpuidle_init(void) { - int cpu_id; - int ret; - struct cpuidle_device *dev; - struct cpuidle_driver *drv = &calxeda_idle_driver; - if (!of_machine_is_compatible("calxeda,highbank")) return -ENODEV;
- ret = cpuidle_register_driver(drv); - if (ret) - return ret; - - calxeda_idle_cpuidle_devices = alloc_percpu(struct cpuidle_device); - if (calxeda_idle_cpuidle_devices == NULL) { - ret = -ENOMEM; - goto unregister_drv; - } - - /* initialize state data for each cpuidle_device */ - for_each_possible_cpu(cpu_id) { - dev = per_cpu_ptr(calxeda_idle_cpuidle_devices, cpu_id); - dev->cpu = cpu_id; - dev->state_count = drv->state_count; - - ret = cpuidle_register_device(dev); - if (ret) { - pr_err("Failed to register cpu %u, error: %d\n", - cpu_id, ret); - goto uninit; - } - } - - return 0; - -uninit: - calxeda_idle_cpuidle_devices_uninit(); -unregister_drv: - cpuidle_unregister_driver(drv); - return ret; + return cpuidle_register(&calxeda_idle_driver, NULL); } module_init(calxeda_cpuidle_init);
On 04/10/2013 09:22 AM, Daniel Lezcano wrote:
Remove the duplicated code and use the cpuidle common code for initialization.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
drivers/cpuidle/cpuidle-calxeda.c | 52 +------------------------------------ 1 file changed, 1 insertion(+), 51 deletions(-)
For this and patch #5:
Acked-by: Rob Herring rob.herring@calxeda.com
Rob
diff --git a/drivers/cpuidle/cpuidle-calxeda.c b/drivers/cpuidle/cpuidle-calxeda.c index a3b56f5..e344b56 100644 --- a/drivers/cpuidle/cpuidle-calxeda.c +++ b/drivers/cpuidle/cpuidle-calxeda.c @@ -35,8 +35,6 @@ extern void highbank_set_cpu_jump(int cpu, void *jump_addr); extern void *scu_base_addr; -static struct cpuidle_device __percpu *calxeda_idle_cpuidle_devices;
static inline unsigned int get_auxcr(void) { unsigned int val; @@ -85,19 +83,6 @@ static int calxeda_pwrdown_idle(struct cpuidle_device *dev, return index; } -static void calxeda_idle_cpuidle_devices_uninit(void) -{
- int i;
- struct cpuidle_device *dev;
- for_each_possible_cpu(i) {
dev = per_cpu_ptr(calxeda_idle_cpuidle_devices, i);cpuidle_unregister_device(dev);- }
- free_percpu(calxeda_idle_cpuidle_devices);
-}
static struct cpuidle_driver calxeda_idle_driver = { .name = "calxeda_idle", .states = { @@ -117,44 +102,9 @@ static struct cpuidle_driver calxeda_idle_driver = { static int __init calxeda_cpuidle_init(void) {
- int cpu_id;
- int ret;
- struct cpuidle_device *dev;
- struct cpuidle_driver *drv = &calxeda_idle_driver;
- if (!of_machine_is_compatible("calxeda,highbank")) return -ENODEV;
- ret = cpuidle_register_driver(drv);
- if (ret)
return ret;- calxeda_idle_cpuidle_devices = alloc_percpu(struct cpuidle_device);
- if (calxeda_idle_cpuidle_devices == NULL) {
ret = -ENOMEM;goto unregister_drv;- }
- /* initialize state data for each cpuidle_device */
- for_each_possible_cpu(cpu_id) {
dev = per_cpu_ptr(calxeda_idle_cpuidle_devices, cpu_id);dev->cpu = cpu_id;dev->state_count = drv->state_count;ret = cpuidle_register_device(dev);if (ret) {pr_err("Failed to register cpu %u, error: %d\n",cpu_id, ret);goto uninit;}- }
- return 0;
-uninit:
- calxeda_idle_cpuidle_devices_uninit();
-unregister_drv:
- cpuidle_unregister_driver(drv);
- return ret;
- return cpuidle_register(&calxeda_idle_driver, NULL);
} module_init(calxeda_cpuidle_init);
Remove the duplicated code and use the cpuidle common code for initialization.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- drivers/cpuidle/cpuidle-kirkwood.c | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-)
diff --git a/drivers/cpuidle/cpuidle-kirkwood.c b/drivers/cpuidle/cpuidle-kirkwood.c index 6f31524..53290e1 100644 --- a/drivers/cpuidle/cpuidle-kirkwood.c +++ b/drivers/cpuidle/cpuidle-kirkwood.c @@ -52,9 +52,6 @@ static struct cpuidle_driver kirkwood_idle_driver = { }, .state_count = KIRKWOOD_MAX_STATES, }; -static struct cpuidle_device *device; - -static DEFINE_PER_CPU(struct cpuidle_device, kirkwood_cpuidle_device);
/* Initialize CPU idle by registering the idle states */ static int kirkwood_cpuidle_probe(struct platform_device *pdev) @@ -69,22 +66,12 @@ static int kirkwood_cpuidle_probe(struct platform_device *pdev) if (IS_ERR(ddr_operation_base)) return PTR_ERR(ddr_operation_base);
- device = &per_cpu(kirkwood_cpuidle_device, smp_processor_id()); - device->state_count = KIRKWOOD_MAX_STATES; - - cpuidle_register_driver(&kirkwood_idle_driver); - if (cpuidle_register_device(device)) { - pr_err("kirkwood_init_cpuidle: Failed registering\n"); - return -EIO; - } - return 0; + return cpuidle_register(&kirkwood_idle_driver, NULL); }
int kirkwood_cpuidle_remove(struct platform_device *pdev) { - cpuidle_unregister_device(device); - cpuidle_unregister_driver(&kirkwood_idle_driver); - + cpuidle_unregister(&kirkwood_idle_driver); return 0; }
Remove the duplicated code and use the cpuidle common code for initialization.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-davinci/cpuidle.c | 20 +------------------- 1 file changed, 1 insertion(+), 19 deletions(-)
diff --git a/arch/arm/mach-davinci/cpuidle.c b/arch/arm/mach-davinci/cpuidle.c index c2887c5..36aef3a 100644 --- a/arch/arm/mach-davinci/cpuidle.c +++ b/arch/arm/mach-davinci/cpuidle.c @@ -25,7 +25,6 @@
#define DAVINCI_CPUIDLE_MAX_STATES 2
-static DEFINE_PER_CPU(struct cpuidle_device, davinci_cpuidle_device); static void __iomem *ddr2_reg_base; static bool ddr2_pdown;
@@ -76,12 +75,8 @@ static struct cpuidle_driver davinci_idle_driver = {
static int __init davinci_cpuidle_probe(struct platform_device *pdev) { - int ret; - struct cpuidle_device *device; struct davinci_cpuidle_config *pdata = pdev->dev.platform_data;
- device = &per_cpu(davinci_cpuidle_device, smp_processor_id()); - if (!pdata) { dev_err(&pdev->dev, "cannot get platform data\n"); return -ENOENT; @@ -91,20 +86,7 @@ static int __init davinci_cpuidle_probe(struct platform_device *pdev)
ddr2_pdown = pdata->ddr2_pdown;
- ret = cpuidle_register_driver(&davinci_idle_driver); - if (ret) { - dev_err(&pdev->dev, "failed to register driver\n"); - return ret; - } - - ret = cpuidle_register_device(device); - if (ret) { - dev_err(&pdev->dev, "failed to register device\n"); - cpuidle_unregister_driver(&davinci_idle_driver); - return ret; - } - - return 0; + return cpuidle_register(&davinci_idle_driver, NULL); }
static struct platform_driver davinci_cpuidle_driver = {
Remove the duplicated code and use the cpuidle common code for initialization.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-imx/cpuidle-imx5.c | 36 +----------------------------------- arch/arm/mach-imx/cpuidle-imx6q.c | 36 +----------------------------------- 2 files changed, 2 insertions(+), 70 deletions(-)
diff --git a/arch/arm/mach-imx/cpuidle-imx5.c b/arch/arm/mach-imx/cpuidle-imx5.c index 3d9f0c9..ba4565f 100644 --- a/arch/arm/mach-imx/cpuidle-imx5.c +++ b/arch/arm/mach-imx/cpuidle-imx5.c @@ -13,8 +13,6 @@
#include "hardware.h"
-static DEFINE_PER_CPU(struct cpuidle_device, devices); - static int imx5_cpuidle_enter(struct cpuidle_device *dev, struct cpuidle_driver *drv, int index) { @@ -38,37 +36,5 @@ static struct cpuidle_driver imx5_cpuidle_driver = {
int __init imx5_cpuidle_init(void) { - struct cpuidle_device *dev; - int cpu, ret; - - ret = cpuidle_register_driver(&imx5_cpuidle_driver); - if (ret) { - pr_err("%s: Failed to register cpuidle driver with error: %d\n", - __func__, ret); - return ret; - } - - /* initialize state data for each cpuidle_device */ - for_each_possible_cpu(cpu) { - dev = &per_cpu(devices, cpu); - dev->cpu = cpu; - - ret = cpuidle_register_device(dev); - if (ret) { - pr_err("%s: Failed to register cpu %u, error: %d\n", - __func__, cpu, ret); - goto out_unregister; - } - } - - return 0; - -out_unregister: - for_each_possible_cpu(cpu) { - dev = &per_cpu(devices, cpu); - cpuidle_unregister_device(dev); - } - - cpuidle_unregister_driver(&imx5_cpuidle_driver); - return ret; + return cpuidle_register(&imx5_cpuidle_driver, NULL); } diff --git a/arch/arm/mach-imx/cpuidle-imx6q.c b/arch/arm/mach-imx/cpuidle-imx6q.c index c066b74faa..23ddfb6 100644 --- a/arch/arm/mach-imx/cpuidle-imx6q.c +++ b/arch/arm/mach-imx/cpuidle-imx6q.c @@ -14,8 +14,6 @@ #include "common.h" #include "cpuidle.h"
-static DEFINE_PER_CPU(struct cpuidle_device, devices); - static atomic_t master = ATOMIC_INIT(0); static DEFINE_SPINLOCK(master_lock);
@@ -67,43 +65,11 @@ static struct cpuidle_driver imx6q_cpuidle_driver = {
int __init imx6q_cpuidle_init(void) { - struct cpuidle_device *dev; - int cpu, ret; - /* Need to enable SCU standby for entering WAIT modes */ imx_scu_standby_enable();
/* Set chicken bit to get a reliable WAIT mode support */ imx6q_set_chicken_bit();
- ret = cpuidle_register_driver(&imx6q_cpuidle_driver); - if (ret) { - pr_err("%s: Failed to register cpuidle driver with error: %d\n", - __func__, ret); - return ret; - } - - /* initialize state data for each cpuidle_device */ - for_each_possible_cpu(cpu) { - dev = &per_cpu(devices, cpu); - dev->cpu = cpu; - - ret = cpuidle_register_device(dev); - if (ret) { - pr_err("%s: Failed to register cpu %u, error: %d\n", - __func__, cpu, ret); - goto out_unregister; - } - } - - return 0; - -out_unregister: - for_each_possible_cpu(cpu) { - dev = &per_cpu(devices, cpu); - cpuidle_unregister_device(dev); - } - - cpuidle_unregister_driver(&imx6q_cpuidle_driver); - return ret; + return cpuidle_register(&imx6q_cpuidle_driver, NULL); }
On Wednesday, April 10, 2013 04:22:05 PM Daniel Lezcano wrote:
This patchset series provide some code consolidation across the different cpuidle drivers. It contains two parts, the first one is the removal of the time keeping flag and the second one, is a common initialization routine.
All the drivers use the en_core_tk_irqen flag, which means it is not necessary to make the time computation optional. We can remove this flag and assume the cpuidle framework always manage this operation.
The cpuidle code initialization is duplicated across the different drivers in the same manner.
The repeating pattern is:
SMP:
cpuidle_register_driver(drv); for_each_possible_cpu(cpu) { dev = per_cpu(cpuidle_device, cpu); cpuidle_register_device(dev); }
UP:
cpuidle_register_driver(drv); cpuidle_register_device(dev);
As on a UP machine the macro 'for_each_cpu' is a one iteration loop, using the initialization loop from SMP to UP works.
The patchset does some cleanup for different drivers in order to make the init code the same. Then it introduces a generic function:
cpuidle_register(struct cpuidle_driver *drv, struct cpumask *cpumask)
The cpumask is for the coupled idle states.
The drivers are then modified to take into account this new function and to remove the duplicated code.
The benefit is observable in the diffstat: 332 lines of code removed.
Tested-on: u8500 Tested-on: at91 Tested-on: intel i5 Tested-on: OMAP4
Compiled with and without CPU_IDLE for: u8500, at91, davinci, exynos, imx5, imx6, kirkwood, multi_v7 (for calxeda), omap2plus, s3c64, tegra1, tegra2, tegra3
Daniel Lezcano (18): ARM: OMAP3: remove cpuidle_wrap_enter cpuidle: remove en_core_tk_irqen flag ARM: ux500: cpuidle: replace for_each_online_cpu by for_each_possible_cpu ARM: imx: cpuidle: create separate drivers for imx5/imx6 cpuidle: make a single register function for all 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: cpuidle: use init/exit common routine ARM: OMAP4: cpuidle: use init/exit common routine ARM: tegra2: cpuidle: use init/exit common routine ARM: tegra3: cpuidle: use init/exit common routine ARM: calxeda: cpuidle: use init/exit common routine ARM: kirkwood: cpuidle: use init/exit common routine ARM: davinci: cpuidle: use init/exit common routine ARM: imx: cpuidle: use init/exit common routine
arch/arm/mach-at91/cpuidle.c | 18 +-- arch/arm/mach-davinci/cpuidle.c | 21 +--- arch/arm/mach-exynos/cpuidle.c | 1 - arch/arm/mach-imx/Makefile | 1 + arch/arm/mach-imx/cpuidle-imx5.c | 40 +++++++ arch/arm/mach-imx/cpuidle-imx6q.c | 3 +- arch/arm/mach-imx/cpuidle.c | 80 ------------- arch/arm/mach-imx/cpuidle.h | 10 +- arch/arm/mach-imx/pm-imx5.c | 30 +---- arch/arm/mach-omap2/cpuidle34xx.c | 49 ++------ arch/arm/mach-omap2/cpuidle44xx.c | 23 +--- arch/arm/mach-s3c64xx/cpuidle.c | 15 +-- arch/arm/mach-shmobile/cpuidle.c | 11 +- arch/arm/mach-shmobile/pm-sh7372.c | 1 - arch/arm/mach-tegra/cpuidle-tegra114.c | 27 +---- arch/arm/mach-tegra/cpuidle-tegra20.c | 34 +----- arch/arm/mach-tegra/cpuidle-tegra30.c | 28 +---- arch/arm/mach-ux500/cpuidle.c | 33 +----- arch/powerpc/platforms/pseries/processor_idle.c | 1 - arch/sh/kernel/cpu/shmobile/cpuidle.c | 1 - arch/x86/kernel/apm_32.c | 1 - drivers/acpi/processor_idle.c | 1 - drivers/cpuidle/cpuidle-calxeda.c | 53 +-------- drivers/cpuidle/cpuidle-kirkwood.c | 18 +-- drivers/cpuidle/cpuidle.c | 137 ++++++++++++++--------- drivers/idle/intel_idle.c | 1 - include/linux/cpuidle.h | 20 ++-- 27 files changed, 162 insertions(+), 496 deletions(-) create mode 100644 arch/arm/mach-imx/cpuidle-imx5.c delete mode 100644 arch/arm/mach-imx/cpuidle.c
I will be traveling from Saturday onwards pretty much until the beginning of the 3.10 merge window, so I'm afraid this will have to wait for the next release cycle. At least I'm not going to take patches that haven't been ACKed before tomorrow evening (CEST).
Thanks, Rafael
On 04/10/2013 09:12 PM, Rafael J. Wysocki wrote:
On Wednesday, April 10, 2013 04:22:05 PM Daniel Lezcano wrote:
This patchset series provide some code consolidation across the different cpuidle drivers. It contains two parts, the first one is the removal of the time keeping flag and the second one, is a common initialization routine.
All the drivers use the en_core_tk_irqen flag, which means it is not necessary to make the time computation optional. We can remove this flag and assume the cpuidle framework always manage this operation.
The cpuidle code initialization is duplicated across the different drivers in the same manner.
The repeating pattern is:
SMP:
cpuidle_register_driver(drv); for_each_possible_cpu(cpu) { dev = per_cpu(cpuidle_device, cpu); cpuidle_register_device(dev); }
UP:
cpuidle_register_driver(drv); cpuidle_register_device(dev);
As on a UP machine the macro 'for_each_cpu' is a one iteration loop, using the initialization loop from SMP to UP works.
The patchset does some cleanup for different drivers in order to make the init code the same. Then it introduces a generic function:
cpuidle_register(struct cpuidle_driver *drv, struct cpumask *cpumask)
The cpumask is for the coupled idle states.
The drivers are then modified to take into account this new function and to remove the duplicated code.
The benefit is observable in the diffstat: 332 lines of code removed.
Tested-on: u8500 Tested-on: at91 Tested-on: intel i5 Tested-on: OMAP4
Compiled with and without CPU_IDLE for: u8500, at91, davinci, exynos, imx5, imx6, kirkwood, multi_v7 (for calxeda), omap2plus, s3c64, tegra1, tegra2, tegra3
Daniel Lezcano (18): ARM: OMAP3: remove cpuidle_wrap_enter cpuidle: remove en_core_tk_irqen flag ARM: ux500: cpuidle: replace for_each_online_cpu by for_each_possible_cpu ARM: imx: cpuidle: create separate drivers for imx5/imx6 cpuidle: make a single register function for all 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: cpuidle: use init/exit common routine ARM: OMAP4: cpuidle: use init/exit common routine ARM: tegra2: cpuidle: use init/exit common routine ARM: tegra3: cpuidle: use init/exit common routine ARM: calxeda: cpuidle: use init/exit common routine ARM: kirkwood: cpuidle: use init/exit common routine ARM: davinci: cpuidle: use init/exit common routine ARM: imx: cpuidle: use init/exit common routine
arch/arm/mach-at91/cpuidle.c | 18 +-- arch/arm/mach-davinci/cpuidle.c | 21 +--- arch/arm/mach-exynos/cpuidle.c | 1 - arch/arm/mach-imx/Makefile | 1 + arch/arm/mach-imx/cpuidle-imx5.c | 40 +++++++ arch/arm/mach-imx/cpuidle-imx6q.c | 3 +- arch/arm/mach-imx/cpuidle.c | 80 ------------- arch/arm/mach-imx/cpuidle.h | 10 +- arch/arm/mach-imx/pm-imx5.c | 30 +---- arch/arm/mach-omap2/cpuidle34xx.c | 49 ++------ arch/arm/mach-omap2/cpuidle44xx.c | 23 +--- arch/arm/mach-s3c64xx/cpuidle.c | 15 +-- arch/arm/mach-shmobile/cpuidle.c | 11 +- arch/arm/mach-shmobile/pm-sh7372.c | 1 - arch/arm/mach-tegra/cpuidle-tegra114.c | 27 +---- arch/arm/mach-tegra/cpuidle-tegra20.c | 34 +----- arch/arm/mach-tegra/cpuidle-tegra30.c | 28 +---- arch/arm/mach-ux500/cpuidle.c | 33 +----- arch/powerpc/platforms/pseries/processor_idle.c | 1 - arch/sh/kernel/cpu/shmobile/cpuidle.c | 1 - arch/x86/kernel/apm_32.c | 1 - drivers/acpi/processor_idle.c | 1 - drivers/cpuidle/cpuidle-calxeda.c | 53 +-------- drivers/cpuidle/cpuidle-kirkwood.c | 18 +-- drivers/cpuidle/cpuidle.c | 137 ++++++++++++++--------- drivers/idle/intel_idle.c | 1 - include/linux/cpuidle.h | 20 ++-- 27 files changed, 162 insertions(+), 496 deletions(-) create mode 100644 arch/arm/mach-imx/cpuidle-imx5.c delete mode 100644 arch/arm/mach-imx/cpuidle.c
I will be traveling from Saturday onwards pretty much until the beginning of the 3.10 merge window, so I'm afraid this will have to wait for the next release cycle. At least I'm not going to take patches that haven't been ACKed before tomorrow evening (CEST).
Ok, thanks for the heads-up.
-- Daniel
linaro-kernel@lists.linaro.org
-
Andrew Lunn -
Colin Cross -
Daniel Lezcano -
Daniel Lezcano -
Joseph Lo -
Nicolas Ferre -
Rafael J. Wysocki -
Rob Herring -
Shawn Guo -
Simon Horman -
Stephen Warren