Since commit 46bcfad7a819bd17ac4e831b04405152d59784ab, cpuidle: Single/Global registration of idle states
we have a single registration for the cpuidle states which makes sense. But now two new architectures are coming: tegra3 and big.LITTLE.
These architectures have different cpus with different caracteristics for power saving. High load => powerfull processors, idle => small processors.
That implies different cpu latencies.
This patchset present a simple way to keep the current behavior as introduced by Deepthi without breaking the drivers and add the possibility to specify a per cpu states.
Daniel Lezcano (5): acpi : move the acpi_idle_driver variable declaration acpi : move cpuidle_device field out of the acpi_processor_power structure cpuidle : add a pointer for cpuidle_state in the cpuidle_device cpuidle : use per cpuidle device cpu states cpuidle : add cpuidle_register_states function
drivers/acpi/processor_driver.c | 2 +- drivers/acpi/processor_idle.c | 25 ++++++++++++++++++------- drivers/cpuidle/cpuidle.c | 24 +++++++++++++++++++++--- drivers/cpuidle/governors/ladder.c | 18 +++++++++--------- drivers/cpuidle/governors/menu.c | 8 ++++---- drivers/cpuidle/sysfs.c | 3 +-- include/acpi/processor.h | 3 --- include/linux/cpuidle.h | 11 ++++++++--- 8 files changed, 62 insertions(+), 32 deletions(-)
This variable is only used in the in processor_driver.c. This patch reduces the scope of the variable by moving it to this file.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- drivers/acpi/processor_driver.c | 2 +- include/acpi/processor.h | 1 - 2 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c index 7048b97..017b39d 100644 --- a/drivers/acpi/processor_driver.c +++ b/drivers/acpi/processor_driver.c @@ -113,7 +113,7 @@ static struct acpi_driver acpi_processor_driver = {
DEFINE_PER_CPU(struct acpi_processor *, processors); EXPORT_PER_CPU_SYMBOL(processors); - +extern struct cpuidle_driver acpi_idle_driver; struct acpi_processor_errata errata __read_mostly;
/* -------------------------------------------------------------------------- diff --git a/include/acpi/processor.h b/include/acpi/processor.h index db427fa..8b2c39a 100644 --- a/include/acpi/processor.h +++ b/include/acpi/processor.h @@ -332,7 +332,6 @@ int acpi_processor_power_exit(struct acpi_processor *pr, struct acpi_device *device); int acpi_processor_suspend(struct device *dev); int acpi_processor_resume(struct device *dev); -extern struct cpuidle_driver acpi_idle_driver;
/* in processor_thermal.c */ int acpi_processor_get_limit_info(struct acpi_processor *pr);
Hi Daniel,
On 07/25/2012 04:15 PM, Daniel Lezcano wrote:
This variable is only used in the in processor_driver.c. This patch reduces the scope of the variable by moving it to this file.
This is true after applying your second patch . Maybe you can check the sequencing of patches in the series. As a thumb rule one should be able to build the kernel by applying each and every patch in the series . There would be a build break after applying this one as acpi_idle_driver is still used in processor_idle.c by applying just this patch.
Cheers, Deepthi
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
drivers/acpi/processor_driver.c | 2 +- include/acpi/processor.h | 1 - 2 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c index 7048b97..017b39d 100644 --- a/drivers/acpi/processor_driver.c +++ b/drivers/acpi/processor_driver.c @@ -113,7 +113,7 @@ static struct acpi_driver acpi_processor_driver = {
DEFINE_PER_CPU(struct acpi_processor *, processors); EXPORT_PER_CPU_SYMBOL(processors);
+extern struct cpuidle_driver acpi_idle_driver; struct acpi_processor_errata errata __read_mostly;
/* -------------------------------------------------------------------------- diff --git a/include/acpi/processor.h b/include/acpi/processor.h index db427fa..8b2c39a 100644 --- a/include/acpi/processor.h +++ b/include/acpi/processor.h @@ -332,7 +332,6 @@ int acpi_processor_power_exit(struct acpi_processor *pr, struct acpi_device *device); int acpi_processor_suspend(struct device *dev); int acpi_processor_resume(struct device *dev); -extern struct cpuidle_driver acpi_idle_driver;
/* in processor_thermal.c */ int acpi_processor_get_limit_info(struct acpi_processor *pr);
On 07/27/2012 07:46 AM, Deepthi Dharwar wrote:
Hi Daniel,
On 07/25/2012 04:15 PM, Daniel Lezcano wrote:
This variable is only used in the in processor_driver.c. This patch reduces the scope of the variable by moving it to this file.
This is true after applying your second patch . Maybe you can check the sequencing of patches in the series. As a thumb rule one should be able to build the kernel by applying each and every patch in the series .
Thanks Deepthi. I think I already git bisect tested the patchset. This patch does not break the build, the variable is defined in the processor_driver.c file.
There would be a build break after applying this one as acpi_idle_driver is still used in processor_idle.c by applying just this patch.
Cheers, Deepthi
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
drivers/acpi/processor_driver.c | 2 +- include/acpi/processor.h | 1 - 2 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c index 7048b97..017b39d 100644 --- a/drivers/acpi/processor_driver.c +++ b/drivers/acpi/processor_driver.c @@ -113,7 +113,7 @@ static struct acpi_driver acpi_processor_driver = {
DEFINE_PER_CPU(struct acpi_processor *, processors); EXPORT_PER_CPU_SYMBOL(processors);
+extern struct cpuidle_driver acpi_idle_driver; struct acpi_processor_errata errata __read_mostly;
/* -------------------------------------------------------------------------- diff --git a/include/acpi/processor.h b/include/acpi/processor.h index db427fa..8b2c39a 100644 --- a/include/acpi/processor.h +++ b/include/acpi/processor.h @@ -332,7 +332,6 @@ int acpi_processor_power_exit(struct acpi_processor *pr, struct acpi_device *device); int acpi_processor_suspend(struct device *dev); int acpi_processor_resume(struct device *dev); -extern struct cpuidle_driver acpi_idle_driver;
/* in processor_thermal.c */ int acpi_processor_get_limit_info(struct acpi_processor *pr);
Currently we have the cpuidle_device field in the acpi_processor_power structure. This adds a dependency in processor.h for cpuidle.h.
In order to be consistent with the rest of the drivers and for the per cpu states coming right after this patch, this one move out of the acpi_processor_power structure the cpuidle_device field.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- drivers/acpi/processor_idle.c | 25 ++++++++++++++++++------- include/acpi/processor.h | 2 -- 2 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c index 90582fb..62cc80f 100644 --- a/drivers/acpi/processor_idle.c +++ b/drivers/acpi/processor_idle.c @@ -79,6 +79,8 @@ module_param(bm_check_disable, uint, 0000); static unsigned int latency_factor __read_mostly = 2; module_param(latency_factor, uint, 0644);
+static DEFINE_PER_CPU(struct cpuidle_device, acpi_cpuidle_device); + static int disabled_by_idle_boot_param(void) { return boot_option_idle_override == IDLE_POLL || @@ -998,7 +1000,7 @@ static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr) int i, count = CPUIDLE_DRIVER_STATE_START; struct acpi_processor_cx *cx; struct cpuidle_state_usage *state_usage; - struct cpuidle_device *dev = &pr->power.dev; + struct cpuidle_device *dev = &per_cpu(acpi_cpuidle_device, pr->id);
if (!pr->flags.power_setup_done) return -EINVAL; @@ -1130,6 +1132,7 @@ static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr) int acpi_processor_hotplug(struct acpi_processor *pr) { int ret = 0; + struct cpuidle_device *dev = &per_cpu(acpi_cpuidle_device, pr->id);
if (disabled_by_idle_boot_param()) return 0; @@ -1145,11 +1148,11 @@ int acpi_processor_hotplug(struct acpi_processor *pr) return -ENODEV;
cpuidle_pause_and_lock(); - cpuidle_disable_device(&pr->power.dev); + cpuidle_disable_device(dev); acpi_processor_get_power_info(pr); if (pr->flags.power) { acpi_processor_setup_cpuidle_cx(pr); - ret = cpuidle_enable_device(&pr->power.dev); + ret = cpuidle_enable_device(dev); } cpuidle_resume_and_unlock();
@@ -1160,6 +1163,7 @@ int acpi_processor_cst_has_changed(struct acpi_processor *pr) { int cpu; struct acpi_processor *_pr; + struct cpuidle_device *dev;
if (disabled_by_idle_boot_param()) return 0; @@ -1190,7 +1194,8 @@ int acpi_processor_cst_has_changed(struct acpi_processor *pr) _pr = per_cpu(processors, cpu); if (!_pr || !_pr->flags.power_setup_done) continue; - cpuidle_disable_device(&_pr->power.dev); + dev = &per_cpu(acpi_cpuidle_device, cpu); + cpuidle_disable_device(dev); }
/* Populate Updated C-state information */ @@ -1204,7 +1209,8 @@ int acpi_processor_cst_has_changed(struct acpi_processor *pr) acpi_processor_get_power_info(_pr); if (_pr->flags.power) { acpi_processor_setup_cpuidle_cx(_pr); - cpuidle_enable_device(&_pr->power.dev); + dev = &per_cpu(acpi_cpuidle_device, cpu); + cpuidle_enable_device(dev); } } put_online_cpus(); @@ -1221,6 +1227,7 @@ int __cpuinit acpi_processor_power_init(struct acpi_processor *pr, { acpi_status status = 0; int retval; + struct cpuidle_device *dev; static int first_run;
if (disabled_by_idle_boot_param()) @@ -1270,7 +1277,9 @@ int __cpuinit acpi_processor_power_init(struct acpi_processor *pr, * must already be registered before registering device */ acpi_processor_setup_cpuidle_cx(pr); - retval = cpuidle_register_device(&pr->power.dev); + + dev = &per_cpu(acpi_cpuidle_device, pr->id); + retval = cpuidle_register_device(dev); if (retval) { if (acpi_processor_registered == 0) cpuidle_unregister_driver(&acpi_idle_driver); @@ -1284,11 +1293,13 @@ int __cpuinit acpi_processor_power_init(struct acpi_processor *pr, int acpi_processor_power_exit(struct acpi_processor *pr, struct acpi_device *device) { + struct cpuidle_device *dev = &per_cpu(acpi_cpuidle_device, pr->id); + if (disabled_by_idle_boot_param()) return 0;
if (pr->flags.power) { - cpuidle_unregister_device(&pr->power.dev); + cpuidle_unregister_device(dev); acpi_processor_registered--; if (acpi_processor_registered == 0) cpuidle_unregister_driver(&acpi_idle_driver); diff --git a/include/acpi/processor.h b/include/acpi/processor.h index 8b2c39a..4d98ec8 100644 --- a/include/acpi/processor.h +++ b/include/acpi/processor.h @@ -3,7 +3,6 @@
#include <linux/kernel.h> #include <linux/cpu.h> -#include <linux/cpuidle.h> #include <linux/thermal.h> #include <asm/acpi.h>
@@ -64,7 +63,6 @@ struct acpi_processor_cx { };
struct acpi_processor_power { - struct cpuidle_device dev; struct acpi_processor_cx *state; unsigned long bm_check_timestamp; u32 default_state;
This patch adds a pointer to the cpuidle_state array in the cpuidle_device structure. When the cpuidle_device is initialized, the pointer is assigned from the driver's cpuidle states array.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- drivers/cpuidle/cpuidle.c | 1 + include/linux/cpuidle.h | 1 + 2 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index d6a533e..42b1a8a 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -305,6 +305,7 @@ int cpuidle_enable_device(struct cpuidle_device *dev) return -EIO; if (!dev->state_count) dev->state_count = drv->state_count; + dev->states = drv->states;
if (dev->registered == 0) { ret = __cpuidle_register_device(dev); diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 89dcd30..40a04a1 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -95,6 +95,7 @@ struct cpuidle_device {
int last_residency; int state_count; + struct cpuidle_state *states; struct cpuidle_state_usage states_usage[CPUIDLE_STATE_MAX]; struct cpuidle_state_kobj *kobjs[CPUIDLE_STATE_MAX];
On Wed, Jul 25, 2012 at 12:46 PM, Daniel Lezcano daniel.lezcano@linaro.org wrote:
This patch adds a pointer to the cpuidle_state array in the cpuidle_device structure. When the cpuidle_device is initialized, the pointer is assigned from the driver's cpuidle states array.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
Would be good to also mention the intention behind this change in the change-log. The next patch make it clear but for this change too, it should be added.
Feel free to add my ack if you need one.
Acked-by: Santosh Shilimkar santosh.shilimkar@ti.com
On Wed, Jul 25, 2012 at 12:46:00PM +0200, Daniel Lezcano wrote:
This patch adds a pointer to the cpuidle_state array in the cpuidle_device structure. When the cpuidle_device is initialized, the pointer is assigned from the driver's cpuidle states array.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
drivers/cpuidle/cpuidle.c | 1 + include/linux/cpuidle.h | 1 + 2 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index d6a533e..42b1a8a 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -305,6 +305,7 @@ int cpuidle_enable_device(struct cpuidle_device *dev) return -EIO; if (!dev->state_count) dev->state_count = drv->state_count;
- dev->states = drv->states;
This should only be done when dev->state_count == 0 no?
Cheers,
Peter.
On 09/03/2012 05:16 PM, Peter De Schrijver wrote:
On Wed, Jul 25, 2012 at 12:46:00PM +0200, Daniel Lezcano wrote:
This patch adds a pointer to the cpuidle_state array in the cpuidle_device structure. When the cpuidle_device is initialized, the pointer is assigned from the driver's cpuidle states array.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
drivers/cpuidle/cpuidle.c | 1 + include/linux/cpuidle.h | 1 + 2 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index d6a533e..42b1a8a 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -305,6 +305,7 @@ int cpuidle_enable_device(struct cpuidle_device *dev) return -EIO; if (!dev->state_count) dev->state_count = drv->state_count;
- dev->states = drv->states;
This should only be done when dev->state_count == 0 no?
Right. In acpi/processor_idle.c, the state count for the device is initialized and I overwritten this value in all the cases, but this not right. I removed the initialization in acpi because it is pointless and moved this line to do the 'if' block as you mentioned.
Thanks -- Daniel
We have the cpuidle states pointer stored in the cpuidle device structure. This patch use this pointer instead of the driver's one.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- drivers/cpuidle/cpuidle.c | 6 +++--- drivers/cpuidle/governors/ladder.c | 18 +++++++++--------- drivers/cpuidle/governors/menu.c | 8 ++++---- drivers/cpuidle/sysfs.c | 3 +-- 4 files changed, 17 insertions(+), 18 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index 42b1a8a..199878a 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -45,7 +45,7 @@ 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]; + struct cpuidle_state *target_state = &dev->states[index]; return target_state->enter(dev, drv, index); }
@@ -77,7 +77,7 @@ int cpuidle_play_dead(void)
/* Find lowest-power state that supports long-term idle */ for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) { - struct cpuidle_state *s = &drv->states[i]; + struct cpuidle_state *s = &dev->states[i];
if (s->power_usage < power_usage && s->enter_dead) { power_usage = s->power_usage; @@ -86,7 +86,7 @@ int cpuidle_play_dead(void) }
if (dead_state != -1) - return drv->states[dead_state].enter_dead(dev, dead_state); + return dev->states[dead_state].enter_dead(dev, dead_state);
return -ENODEV; } diff --git a/drivers/cpuidle/governors/ladder.c b/drivers/cpuidle/governors/ladder.c index b6a09ea..b50cae3 100644 --- a/drivers/cpuidle/governors/ladder.c +++ b/drivers/cpuidle/governors/ladder.c @@ -79,17 +79,17 @@ static int ladder_select_state(struct cpuidle_driver *drv,
last_state = &ldev->states[last_idx];
- if (drv->states[last_idx].flags & CPUIDLE_FLAG_TIME_VALID) { + if (dev->states[last_idx].flags & CPUIDLE_FLAG_TIME_VALID) { last_residency = cpuidle_get_last_residency(dev) - \ - drv->states[last_idx].exit_latency; + dev->states[last_idx].exit_latency; } else last_residency = last_state->threshold.promotion_time + 1;
/* consider promotion */ - if (last_idx < drv->state_count - 1 && + if (last_idx < dev->state_count - 1 && last_residency > last_state->threshold.promotion_time && - drv->states[last_idx + 1].exit_latency <= latency_req) { + dev->states[last_idx + 1].exit_latency <= latency_req) { last_state->stats.promotion_count++; last_state->stats.demotion_count = 0; if (last_state->stats.promotion_count >= last_state->threshold.promotion_count) { @@ -100,11 +100,11 @@ static int ladder_select_state(struct cpuidle_driver *drv,
/* consider demotion */ if (last_idx > CPUIDLE_DRIVER_STATE_START && - drv->states[last_idx].exit_latency > latency_req) { + dev->states[last_idx].exit_latency > latency_req) { int i;
for (i = last_idx - 1; i > CPUIDLE_DRIVER_STATE_START; i--) { - if (drv->states[i].exit_latency <= latency_req) + if (dev->states[i].exit_latency <= latency_req) break; } ladder_do_selection(ldev, last_idx, i); @@ -140,8 +140,8 @@ static int ladder_enable_device(struct cpuidle_driver *drv,
ldev->last_state_idx = CPUIDLE_DRIVER_STATE_START;
- for (i = 0; i < drv->state_count; i++) { - state = &drv->states[i]; + for (i = 0; i < dev->state_count; i++) { + state = &dev->states[i]; lstate = &ldev->states[i];
lstate->stats.promotion_count = 0; @@ -150,7 +150,7 @@ static int ladder_enable_device(struct cpuidle_driver *drv, lstate->threshold.promotion_count = PROMOTION_COUNT; lstate->threshold.demotion_count = DEMOTION_COUNT;
- if (i < drv->state_count - 1) + if (i < dev->state_count - 1) lstate->threshold.promotion_time = state->exit_latency; if (i > 0) lstate->threshold.demotion_time = state->exit_latency; diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c index 5b1f2c3..3a9a9bd 100644 --- a/drivers/cpuidle/governors/menu.c +++ b/drivers/cpuidle/governors/menu.c @@ -281,7 +281,7 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev) * unless the timer is happening really really soon. */ if (data->expected_us > 5 && - !drv->states[CPUIDLE_DRIVER_STATE_START].disabled && + !dev->states[CPUIDLE_DRIVER_STATE_START].disabled && dev->states_usage[CPUIDLE_DRIVER_STATE_START].disable == 0) data->last_state_idx = CPUIDLE_DRIVER_STATE_START;
@@ -289,8 +289,8 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev) * Find the idle state with the lowest power while satisfying * our constraints. */ - for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) { - struct cpuidle_state *s = &drv->states[i]; + for (i = CPUIDLE_DRIVER_STATE_START; i < dev->state_count; i++) { + struct cpuidle_state *s = &dev->states[i]; struct cpuidle_state_usage *su = &dev->states_usage[i];
if (s->disabled || su->disable) @@ -338,7 +338,7 @@ static void menu_update(struct cpuidle_driver *drv, struct cpuidle_device *dev) struct menu_device *data = &__get_cpu_var(menu_devices); int last_idx = data->last_state_idx; unsigned int last_idle_us = cpuidle_get_last_residency(dev); - struct cpuidle_state *target = &drv->states[last_idx]; + struct cpuidle_state *target = &dev->states[last_idx]; unsigned int measured_us; u64 new_factor;
diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c index 5f809e3..6b20929 100644 --- a/drivers/cpuidle/sysfs.c +++ b/drivers/cpuidle/sysfs.c @@ -363,14 +363,13 @@ int cpuidle_add_state_sysfs(struct cpuidle_device *device) { int i, ret = -ENOMEM; struct cpuidle_state_kobj *kobj; - struct cpuidle_driver *drv = cpuidle_get_driver();
/* state statistics */ for (i = 0; i < device->state_count; i++) { kobj = kzalloc(sizeof(struct cpuidle_state_kobj), GFP_KERNEL); if (!kobj) goto error_state; - kobj->state = &drv->states[i]; + kobj->state = &device->states[i]; kobj->state_usage = &device->states_usage[i]; init_completion(&kobj->kobj_unregister);
On Wed, Jul 25, 2012 at 12:46 PM, Daniel Lezcano daniel.lezcano@linaro.org wrote:
We have the cpuidle states pointer stored in the cpuidle device structure. This patch use this pointer instead of the driver's one.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
Sorry for another comment on change log. Would be give a reference about the commit which removed the feature(was present before), and also some examples like say big.LITTLE, Tegra arch which needs the per CPU latency tables.
Regards Santosh
The tegra3 and big.LITTLE architecture have different cpu latencies. This API allows to specify a different cpu latency for a specific cpu.
With the previous patches, we use the per cpuidle device states pointer, this function overrides this pointer.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- drivers/cpuidle/cpuidle.c | 17 +++++++++++++++++ include/linux/cpuidle.h | 10 +++++++--- 2 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index 199878a..3b21b68 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -456,6 +456,23 @@ void cpuidle_unregister_device(struct cpuidle_device *dev)
EXPORT_SYMBOL_GPL(cpuidle_unregister_device);
+int cpuidle_register_states(struct cpuidle_device *dev, + struct cpuidle_state *states, + int state_count) +{ + if (!dev || !states) + return -EINVAL; + + if (state_count <= 0) + return -EINVAL; + + dev->states = states; + dev->state_count = state_count; + + return 0; +} +EXPORT_SYMBOL_GPL(cpuidle_register_state); + #ifdef CONFIG_SMP
static void smp_callback(void *v) diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 40a04a1..425200d 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -144,7 +144,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_states(struct cpuidle_device *dev, + struct cpuidle_state *states, + int state_count); extern void cpuidle_pause_and_lock(void); extern void cpuidle_resume_and_unlock(void); extern void cpuidle_pause(void); @@ -156,7 +158,6 @@ extern int cpuidle_wrap_enter(struct cpuidle_device *dev, int (*enter)(struct cpuidle_device *dev, struct cpuidle_driver *drv, int index)); extern int cpuidle_play_dead(void); - #else static inline void disable_cpuidle(void) { } static inline int cpuidle_idle_call(void) { return -ENODEV; } @@ -169,7 +170,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_states(struct cpuidle_device *dev, + struct cpuidle_state *states, + int state_count) +{ return -ENODEV; } static inline void cpuidle_pause_and_lock(void) { } static inline void cpuidle_resume_and_unlock(void) { } static inline void cpuidle_pause(void) { }
On 07/25/2012 04:16 PM, Daniel Lezcano wrote:
The tegra3 and big.LITTLE architecture have different cpu latencies. This API allows to specify a different cpu latency for a specific cpu.
With the previous patches, we use the per cpuidle device states pointer, this function overrides this pointer.
May be add some more explanation and pointers to previous discussions, as stated on the cover in the patch series.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
Acked-by: Deepthi Dharwar deepthi@linux.vnet.ibm.com
drivers/cpuidle/cpuidle.c | 17 +++++++++++++++++ include/linux/cpuidle.h | 10 +++++++--- 2 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index 199878a..3b21b68 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -456,6 +456,23 @@ void cpuidle_unregister_device(struct cpuidle_device *dev)
EXPORT_SYMBOL_GPL(cpuidle_unregister_device);
+int cpuidle_register_states(struct cpuidle_device *dev,
struct cpuidle_state *states,int state_count)+{
- if (!dev || !states)
return -EINVAL;- if (state_count <= 0)
return -EINVAL;- dev->states = states;
- dev->state_count = state_count;
- return 0;
+} +EXPORT_SYMBOL_GPL(cpuidle_register_state);
#ifdef CONFIG_SMP
static void smp_callback(void *v) diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 40a04a1..425200d 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -144,7 +144,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_states(struct cpuidle_device *dev,
struct cpuidle_state *states,int state_count);extern void cpuidle_pause_and_lock(void); extern void cpuidle_resume_and_unlock(void); extern void cpuidle_pause(void); @@ -156,7 +158,6 @@ extern int cpuidle_wrap_enter(struct cpuidle_device *dev, int (*enter)(struct cpuidle_device *dev, struct cpuidle_driver *drv, int index)); extern int cpuidle_play_dead(void);
#else static inline void disable_cpuidle(void) { } static inline int cpuidle_idle_call(void) { return -ENODEV; } @@ -169,7 +170,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_states(struct cpuidle_device *dev,
struct cpuidle_state *states,int state_count)+{ return -ENODEV; } static inline void cpuidle_pause_and_lock(void) { } static inline void cpuidle_resume_and_unlock(void) { } static inline void cpuidle_pause(void) { }
Hi Daniel,
thanks for this patchset.
On Wed, Jul 25, 2012 at 11:46:02AM +0100, Daniel Lezcano wrote:
The tegra3 and big.LITTLE architecture have different cpu latencies. This API allows to specify a different cpu latency for a specific cpu.
With the previous patches, we use the per cpuidle device states pointer, this function overrides this pointer.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
drivers/cpuidle/cpuidle.c | 17 +++++++++++++++++ include/linux/cpuidle.h | 10 +++++++--- 2 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index 199878a..3b21b68 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -456,6 +456,23 @@ void cpuidle_unregister_device(struct cpuidle_device *dev) EXPORT_SYMBOL_GPL(cpuidle_unregister_device); +int cpuidle_register_states(struct cpuidle_device *dev,
struct cpuidle_state *states,int state_count)+{
- if (!dev || !states)
return -EINVAL;- if (state_count <= 0)
return -EINVAL;- dev->states = states;
- dev->state_count = state_count;
Is this function supposed to be called after cpuidle_device registration ? I think so since at registration time the dev->states pointers are all initialized to point to the driver state array, which is global and not really what we want.
Unless this function is called on the cpu that requires swapping the state pointer, I think it is unsafe to register a different state pointer without a minimal level of locking (or disabling idle and renabling idle) since the update of dev->states and dev->state_count is not atomic. Maybe it is implicit but it should be documented somehow to define cpuidle_register_states(...) proper usage.
Thanks, Lorenzo
On 08/10/2012 07:17 PM, Lorenzo Pieralisi wrote:
Hi Daniel,
thanks for this patchset.
On Wed, Jul 25, 2012 at 11:46:02AM +0100, Daniel Lezcano wrote:
The tegra3 and big.LITTLE architecture have different cpu latencies. This API allows to specify a different cpu latency for a specific cpu.
With the previous patches, we use the per cpuidle device states pointer, this function overrides this pointer.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
drivers/cpuidle/cpuidle.c | 17 +++++++++++++++++ include/linux/cpuidle.h | 10 +++++++--- 2 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index 199878a..3b21b68 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -456,6 +456,23 @@ void cpuidle_unregister_device(struct cpuidle_device *dev) EXPORT_SYMBOL_GPL(cpuidle_unregister_device); +int cpuidle_register_states(struct cpuidle_device *dev,
struct cpuidle_state *states,int state_count)+{
- if (!dev || !states)
return -EINVAL;- if (state_count <= 0)
return -EINVAL;- dev->states = states;
- dev->state_count = state_count;
Is this function supposed to be called after cpuidle_device registration ? I think so since at registration time the dev->states pointers are all initialized to point to the driver state array, which is global and not really what we want.
Unless this function is called on the cpu that requires swapping the state pointer, I think it is unsafe to register a different state pointer without a minimal level of locking (or disabling idle and renabling idle) since the update of dev->states and dev->state_count is not atomic. Maybe it is implicit but it should be documented somehow to define cpuidle_register_states(...) proper usage.
Hi Lorenzo,
Yes, you are right. I will add the cpuidle lock.
Thanks !
-- Daniel
On Wed, Jul 25, 2012 at 12:46:02PM +0200, Daniel Lezcano wrote:
The tegra3 and big.LITTLE architecture have different cpu latencies. This API allows to specify a different cpu latency for a specific cpu.
With the previous patches, we use the per cpuidle device states pointer, this function overrides this pointer.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
drivers/cpuidle/cpuidle.c | 17 +++++++++++++++++ include/linux/cpuidle.h | 10 +++++++--- 2 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index 199878a..3b21b68 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -456,6 +456,23 @@ void cpuidle_unregister_device(struct cpuidle_device *dev) EXPORT_SYMBOL_GPL(cpuidle_unregister_device); +int cpuidle_register_states(struct cpuidle_device *dev,
struct cpuidle_state *states,int state_count)+{
- if (!dev || !states)
return -EINVAL;- if (state_count <= 0)
return -EINVAL;- dev->states = states;
- dev->state_count = state_count;
- return 0;
+} +EXPORT_SYMBOL_GPL(cpuidle_register_state);
#ifdef CONFIG_SMP
Looks good... apart from the fact that the function definition says cpuidle_register_stateS and the exported symbol is cpuidle_register_state...
Cheers,
Peter.
On 09/03/2012 03:22 PM, Peter De Schrijver wrote:
On Wed, Jul 25, 2012 at 12:46:02PM +0200, Daniel Lezcano wrote:
The tegra3 and big.LITTLE architecture have different cpu latencies. This API allows to specify a different cpu latency for a specific cpu.
With the previous patches, we use the per cpuidle device states pointer, this function overrides this pointer.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
drivers/cpuidle/cpuidle.c | 17 +++++++++++++++++ include/linux/cpuidle.h | 10 +++++++--- 2 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index 199878a..3b21b68 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -456,6 +456,23 @@ void cpuidle_unregister_device(struct cpuidle_device *dev) EXPORT_SYMBOL_GPL(cpuidle_unregister_device); +int cpuidle_register_states(struct cpuidle_device *dev,
struct cpuidle_state *states,int state_count)+{
- if (!dev || !states)
return -EINVAL;- if (state_count <= 0)
return -EINVAL;- dev->states = states;
- dev->state_count = state_count;
- return 0;
+} +EXPORT_SYMBOL_GPL(cpuidle_register_state);
#ifdef CONFIG_SMP
Looks good... apart from the fact that the function definition says cpuidle_register_stateS and the exported symbol is cpuidle_register_state...
Ok, fixed. Thanks !
-- Daniel
On Wed, Jul 25, 2012 at 12:45:57PM +0200, Daniel Lezcano wrote:
Since commit 46bcfad7a819bd17ac4e831b04405152d59784ab, cpuidle: Single/Global registration of idle states
we have a single registration for the cpuidle states which makes sense. But now two new architectures are coming: tegra3 and big.LITTLE.
These architectures have different cpus with different caracteristics for power saving. High load => powerfull processors, idle => small processors.
That implies different cpu latencies.
This patchset present a simple way to keep the current behavior as introduced by Deepthi without breaking the drivers and add the possibility to specify a per cpu states.
Looks good! I will do some tests with this next week.
Cheers,
Peter.
On 07/27/2012 01:28 PM, Peter De Schrijver wrote:
On Wed, Jul 25, 2012 at 12:45:57PM +0200, Daniel Lezcano wrote:
Since commit 46bcfad7a819bd17ac4e831b04405152d59784ab, cpuidle: Single/Global registration of idle states
we have a single registration for the cpuidle states which makes sense. But now two new architectures are coming: tegra3 and big.LITTLE.
These architectures have different cpus with different caracteristics for power saving. High load => powerfull processors, idle => small processors.
That implies different cpu latencies.
This patchset present a simple way to keep the current behavior as introduced by Deepthi without breaking the drivers and add the possibility to specify a per cpu states.
Looks good! I will do some tests with this next week.
Hi Peter,
shall I consider adding your acked-by ?
Did you have time to test the patchset ?
Thanks -- Daniel
On Fri, Aug 31, 2012 at 11:19:15PM +0200, Daniel Lezcano wrote:
On 07/27/2012 01:28 PM, Peter De Schrijver wrote:
On Wed, Jul 25, 2012 at 12:45:57PM +0200, Daniel Lezcano wrote:
Since commit 46bcfad7a819bd17ac4e831b04405152d59784ab, cpuidle: Single/Global registration of idle states
we have a single registration for the cpuidle states which makes sense. But now two new architectures are coming: tegra3 and big.LITTLE.
These architectures have different cpus with different caracteristics for power saving. High load => powerfull processors, idle => small processors.
That implies different cpu latencies.
This patchset present a simple way to keep the current behavior as introduced by Deepthi without breaking the drivers and add the possibility to specify a per cpu states.
Looks good! I will do some tests with this next week.
Hi Peter,
shall I consider adding your acked-by ?
You can. I don't see any problem with the code as such (besides the race).
Did you have time to test the patchset ?
I haven't had time no :( I was on holiday most of august and had some other things to take care of last week :(. Working on it now.
Cheers,
Peter.
On Mon, Sep 03, 2012 at 01:05:11PM +0200, Peter De Schrijver wrote:
On Fri, Aug 31, 2012 at 11:19:15PM +0200, Daniel Lezcano wrote:
On 07/27/2012 01:28 PM, Peter De Schrijver wrote:
On Wed, Jul 25, 2012 at 12:45:57PM +0200, Daniel Lezcano wrote:
Since commit 46bcfad7a819bd17ac4e831b04405152d59784ab, cpuidle: Single/Global registration of idle states
we have a single registration for the cpuidle states which makes sense. But now two new architectures are coming: tegra3 and big.LITTLE.
These architectures have different cpus with different caracteristics for power saving. High load => powerfull processors, idle => small processors.
That implies different cpu latencies.
This patchset present a simple way to keep the current behavior as introduced by Deepthi without breaking the drivers and add the possibility to specify a per cpu states.
Looks good! I will do some tests with this next week.
Hi Peter,
shall I consider adding your acked-by ?
You can. I don't see any problem with the code as such (besides the race).
Did you have time to test the patchset ?
I haven't had time no :( I was on holiday most of august and had some other things to take care of last week :(. Working on it now.
I did some preliminary testing, besides the issues I posted earlier. It looks all good here.
Cheers,
Peter.
-
Daniel Lezcano -
Deepthi Dharwar -
Lorenzo Pieralisi -
Peter De Schrijver -
Shilimkar, Santosh