This patchset is a proposition to improve a bit the code. The changes are code cleanup and does not change the behavior of the driver itself.
A couple a things call my intention. Why the cpuidle device is set for cpu0 only and why the WFI is not used ?
Daniel Lezcano (7): ARM: OMAP4: cpuidle - Remove unused valid field ARM: OMAP4: cpuidle - Declare the states with the driver declaration ARM: OMAP4: cpuidle - Remove the cpuidle_params_table table ARM: OMAP4: cpuidle - fix static omap4_idle_data declaration ARM: OMAP4: cpuidle - Initialize omap4_idle_data at compile time ARM: OMAP4: cpuidle - use the omap4_idle_data variable directly ARM: OMAP4: cpuidle - remove omap4_idle_data initialization at boot time
arch/arm/mach-omap2/cpuidle44xx.c | 126 ++++++++++++++++--------------------- 1 files changed, 54 insertions(+), 72 deletions(-)
The 'valid' field is never used in the code, let's remove it.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-omap2/cpuidle44xx.c | 9 +++------ 1 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/arch/arm/mach-omap2/cpuidle44xx.c b/arch/arm/mach-omap2/cpuidle44xx.c index cfdbb86..1210229 100644 --- a/arch/arm/mach-omap2/cpuidle44xx.c +++ b/arch/arm/mach-omap2/cpuidle44xx.c @@ -29,16 +29,15 @@ struct omap4_idle_statedata { u32 cpu_state; u32 mpu_logic_state; u32 mpu_state; - u8 valid; };
static struct cpuidle_params cpuidle_params_table[] = { /* C1 - CPU0 ON + CPU1 ON + MPU ON */ - {.exit_latency = 2 + 2 , .target_residency = 5, .valid = 1}, + {.exit_latency = 2 + 2 , .target_residency = 5 }, /* C2- CPU0 OFF + CPU1 OFF + MPU CSWR */ - {.exit_latency = 328 + 440 , .target_residency = 960, .valid = 1}, + {.exit_latency = 328 + 440 , .target_residency = 960 }, /* C3 - CPU0 OFF + CPU1 OFF + MPU OSWR */ - {.exit_latency = 460 + 518 , .target_residency = 1100, .valid = 1}, + {.exit_latency = 460 + 518 , .target_residency = 1100 }, };
#define OMAP4_NUM_STATES ARRAY_SIZE(cpuidle_params_table) @@ -171,7 +170,6 @@ static inline struct omap4_idle_statedata *_fill_cstate_usage( struct omap4_idle_statedata *cx = &omap4_idle_data[idx]; struct cpuidle_state_usage *state_usage = &dev->states_usage[idx];
- cx->valid = cpuidle_params_table[idx].valid; cpuidle_set_statedata(state_usage, cx);
return cx; @@ -207,7 +205,6 @@ int __init omap4_idle_init(void) _fill_cstate(drv, 0, "MPUSS ON"); drv->safe_state_index = 0; cx = _fill_cstate_usage(dev, 0); - cx->valid = 1; /* C1 is always valid */ cx->cpu_state = PWRDM_POWER_ON; cx->mpu_state = PWRDM_POWER_ON; cx->mpu_logic_state = PWRDM_POWER_RET;
On Wed, Mar 21, 2012 at 2:57 PM, Daniel Lezcano daniel.lezcano@linaro.org wrote:
The 'valid' field is never used in the code, let's remove it.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
It is used during the registration. This field has been very useful for debug when need to disable a C-state etc. So unless and until there is a strong reason, i would like to retain it.
Regards Santosh
On 03/21/2012 10:41 AM, Shilimkar, Santosh wrote:
On Wed, Mar 21, 2012 at 2:57 PM, Daniel Lezcano daniel.lezcano@linaro.org wrote:
The 'valid' field is never used in the code, let's remove it.
Signed-off-by: Daniel Lezcanodaniel.lezcano@linaro.org
It is used during the registration. This field has been very useful for debug when need to disable a C-state etc. So unless and until there is a strong reason, i would like to retain it.
IMO if it used for debug purpose, it should be moved to the debug code and if the debug code is not upstream, then that 'valid' should not be here but in the out-of-tree code.
By the way, this may be a debate for nothing because a patchset is on the way to disable C-states from sysfs.
On Wednesday 21 March 2012 03:16 PM, Daniel Lezcano wrote:
On 03/21/2012 10:41 AM, Shilimkar, Santosh wrote:
On Wed, Mar 21, 2012 at 2:57 PM, Daniel Lezcano daniel.lezcano@linaro.org wrote:
The 'valid' field is never used in the code, let's remove it.
Signed-off-by: Daniel Lezcanodaniel.lezcano@linaro.org
It is used during the registration. This field has been very useful for debug when need to disable a C-state etc. So unless and until there is a strong reason, i would like to retain it.
IMO if it used for debug purpose, it should be moved to the debug code and if the debug code is not upstream, then that 'valid' should not be here but in the out-of-tree code.
When I said debug, I mean CPUIDLE debug and not any special debug code.
By the way, this may be a debate for nothing because a patchset is on the way to disable C-states from sysfs.
I see but sysfs won't solve that because you want to disable certain C-state so that CPUIDLE driver don't use that state.
Let say if the C4 which is OSWR is broken. In such cases, just setting valid flag let you disable it.
Again I don't have strong objection to this change.
Regards santosh
On Wed, Mar 21, 2012 at 11:03 AM, Santosh Shilimkar santosh.shilimkar@ti.com wrote:
On Wednesday 21 March 2012 03:16 PM, Daniel Lezcano wrote:
On 03/21/2012 10:41 AM, Shilimkar, Santosh wrote:
On Wed, Mar 21, 2012 at 2:57 PM, Daniel Lezcano daniel.lezcano@linaro.org wrote:
The 'valid' field is never used in the code, let's remove it.
Signed-off-by: Daniel Lezcanodaniel.lezcano@linaro.org
It is used during the registration. This field has been very useful for debug when need to disable a C-state etc. So unless and until there is a strong reason, i would like to retain it.
IMO if it used for debug purpose, it should be moved to the debug code and if the debug code is not upstream, then that 'valid' should not be here but in the out-of-tree code.
When I said debug, I mean CPUIDLE debug and not any special debug code.
By the way, this may be a debate for nothing because a patchset is on the way to disable C-states from sysfs.
I see but sysfs won't solve that because you want to disable certain C-state so that CPUIDLE driver don't use that state.
This will solve the problem, the only difference is that you need the user space to switch the disable knob from sysfs.
From the kernel space, for debug, you can set the .disable value to 1
in the cpuidle_driver->states struct.
Let say if the C4 which is OSWR is broken. In such cases, just setting valid flag let you disable it.
Again I don't have strong objection to this change.
Regards santosh
Regards, Jean
linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
The cpuidle API allows to declare statically the states in the driver structure. Let's use it. We do no longer need the fill_cstate function called at runtime and by the way adding more instructions at boot time.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-omap2/cpuidle44xx.c | 56 +++++++++++++++++++++---------------- 1 files changed, 32 insertions(+), 24 deletions(-)
diff --git a/arch/arm/mach-omap2/cpuidle44xx.c b/arch/arm/mach-omap2/cpuidle44xx.c index 1210229..cd6bee7 100644 --- a/arch/arm/mach-omap2/cpuidle44xx.c +++ b/arch/arm/mach-omap2/cpuidle44xx.c @@ -148,21 +148,38 @@ DEFINE_PER_CPU(struct cpuidle_device, omap4_idle_dev); struct cpuidle_driver omap4_idle_driver = { .name = "omap4_idle", .owner = THIS_MODULE, + .states = { + { + /* C1 - CPU0 ON + CPU1 ON + MPU ON */ + .exit_latency = 2 + 2, + .target_residency = 5, + .flags = CPUIDLE_FLAG_TIME_VALID, + .enter = omap4_enter_idle, + .name = "C1", + .desc = "MPUSS ON" + }, + { + /* C2 - CPU0 OFF + CPU1 OFF + MPU CSWR */ + .exit_latency = 328 + 440, + .target_residency = 960, + .flags = CPUIDLE_FLAG_TIME_VALID, + .enter = omap4_enter_idle, + .name = "C2", + .desc = "MPUSS CSWR", + }, + { + /* C3 - CPU0 OFF + CPU1 OFF + MPU OSWR */ + .exit_latency = 460 + 518, + .target_residency = 1100, + .flags = CPUIDLE_FLAG_TIME_VALID, + .enter = omap4_enter_idle, + .name = "C3", + .desc = "MPUSS OSWR", + }, + }, + .state_count = OMAP4_NUM_STATES, };
-static inline void _fill_cstate(struct cpuidle_driver *drv, - int idx, const char *descr) -{ - struct cpuidle_state *state = &drv->states[idx]; - - state->exit_latency = cpuidle_params_table[idx].exit_latency; - state->target_residency = cpuidle_params_table[idx].target_residency; - state->flags = CPUIDLE_FLAG_TIME_VALID; - state->enter = omap4_enter_idle; - sprintf(state->name, "C%d", idx + 1); - strncpy(state->desc, descr, CPUIDLE_DESC_LEN); -} - static inline struct omap4_idle_statedata *_fill_cstate_usage( struct cpuidle_device *dev, int idx) @@ -196,37 +213,28 @@ int __init omap4_idle_init(void) if ((!mpu_pd) || (!cpu0_pd) || (!cpu1_pd)) return -ENODEV;
- - drv->safe_state_index = -1; dev = &per_cpu(omap4_idle_dev, cpu_id); dev->cpu = cpu_id;
- /* C1 - CPU0 ON + CPU1 ON + MPU ON */ - _fill_cstate(drv, 0, "MPUSS ON"); - drv->safe_state_index = 0; cx = _fill_cstate_usage(dev, 0); cx->cpu_state = PWRDM_POWER_ON; cx->mpu_state = PWRDM_POWER_ON; cx->mpu_logic_state = PWRDM_POWER_RET;
- /* C2 - CPU0 OFF + CPU1 OFF + MPU CSWR */ - _fill_cstate(drv, 1, "MPUSS CSWR"); cx = _fill_cstate_usage(dev, 1); cx->cpu_state = PWRDM_POWER_OFF; cx->mpu_state = PWRDM_POWER_RET; cx->mpu_logic_state = PWRDM_POWER_RET;
- /* C3 - CPU0 OFF + CPU1 OFF + MPU OSWR */ - _fill_cstate(drv, 2, "MPUSS OSWR"); cx = _fill_cstate_usage(dev, 2); cx->cpu_state = PWRDM_POWER_OFF; cx->mpu_state = PWRDM_POWER_RET; cx->mpu_logic_state = PWRDM_POWER_OFF;
- drv->state_count = OMAP4_NUM_STATES; cpuidle_register_driver(&omap4_idle_driver);
- dev->state_count = OMAP4_NUM_STATES; + dev->state_count = drv->state_count; + if (cpuidle_register_device(dev)) { pr_err("%s: CPUidle register device failed\n", __func__); return -EIO;
+ Jean,
On Wednesday 21 March 2012 02:57 PM, Daniel Lezcano wrote:
The cpuidle API allows to declare statically the states in the driver structure. Let's use it. We do no longer need the fill_cstate function called at runtime and by the way adding more instructions at boot time.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
Jean added the fill_cstate() kind of helpers o.w in the old cpuidle code9OMAP30, static tables were used. Ofcourse those tables were not uinsg the cpuidle driver structure.
Regards santosh
On Wed, Mar 21, 2012 at 10:27 AM, Daniel Lezcano daniel.lezcano@linaro.org wrote:
The cpuidle API allows to declare statically the states in the driver structure. Let's use it. We do no longer need the fill_cstate function called at runtime and by the way adding more instructions at boot time.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
arch/arm/mach-omap2/cpuidle44xx.c | 56 +++++++++++++++++++++---------------- 1 files changed, 32 insertions(+), 24 deletions(-)
diff --git a/arch/arm/mach-omap2/cpuidle44xx.c b/arch/arm/mach-omap2/cpuidle44xx.c index 1210229..cd6bee7 100644 --- a/arch/arm/mach-omap2/cpuidle44xx.c +++ b/arch/arm/mach-omap2/cpuidle44xx.c @@ -148,21 +148,38 @@ DEFINE_PER_CPU(struct cpuidle_device, omap4_idle_dev); struct cpuidle_driver omap4_idle_driver = { .name = "omap4_idle", .owner = THIS_MODULE,
- .states = {
- {
- /* C1 - CPU0 ON + CPU1 ON + MPU ON */
- .exit_latency = 2 + 2,
- .target_residency = 5,
- .flags = CPUIDLE_FLAG_TIME_VALID,
- .enter = omap4_enter_idle,
- .name = "C1",
- .desc = "MPUSS ON"
- },
...
- },
- .state_count = OMAP4_NUM_STATES,
};
-static inline void _fill_cstate(struct cpuidle_driver *drv,
- int idx, const char *descr)
-{
- struct cpuidle_state *state = &drv->states[idx];
- state->exit_latency = cpuidle_params_table[idx].exit_latency;
- state->target_residency = cpuidle_params_table[idx].target_residency;
- state->flags = CPUIDLE_FLAG_TIME_VALID;
- state->enter = omap4_enter_idle;
- sprintf(state->name, "C%d", idx + 1);
- strncpy(state->desc, descr, CPUIDLE_DESC_LEN);
-}
I am OK with this change, which makes the code more readable (and so maintainable).
static inline struct omap4_idle_statedata *_fill_cstate_usage( struct cpuidle_device *dev, int idx) @@ -196,37 +213,28 @@ int __init omap4_idle_init(void) if ((!mpu_pd) || (!cpu0_pd) || (!cpu1_pd)) return -ENODEV;
- drv->safe_state_index = -1;
dev = &per_cpu(omap4_idle_dev, cpu_id); dev->cpu = cpu_id;
- /* C1 - CPU0 ON + CPU1 ON + MPU ON */
- _fill_cstate(drv, 0, "MPUSS ON");
- drv->safe_state_index = 0;
I would keep this or add a clear comment that C1 is the safe state.
...
Thanks, Jean
-- 1.7.5.4
-- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/21/2012 02:31 PM, Jean Pihet wrote:
On Wed, Mar 21, 2012 at 10:27 AM, Daniel Lezcano daniel.lezcano@linaro.org wrote:
The cpuidle API allows to declare statically the states in the driver structure. Let's use it. We do no longer need the fill_cstate function called at runtime and by the way adding more instructions at boot time.
Signed-off-by: Daniel Lezcanodaniel.lezcano@linaro.org
arch/arm/mach-omap2/cpuidle44xx.c | 56 +++++++++++++++++++++---------------- 1 files changed, 32 insertions(+), 24 deletions(-)
diff --git a/arch/arm/mach-omap2/cpuidle44xx.c b/arch/arm/mach-omap2/cpuidle44xx.c index 1210229..cd6bee7 100644 --- a/arch/arm/mach-omap2/cpuidle44xx.c +++ b/arch/arm/mach-omap2/cpuidle44xx.c @@ -148,21 +148,38 @@ DEFINE_PER_CPU(struct cpuidle_device, omap4_idle_dev); struct cpuidle_driver omap4_idle_driver = { .name = "omap4_idle", .owner = THIS_MODULE,
.states = {{/* C1 - CPU0 ON + CPU1 ON + MPU ON */.exit_latency = 2 + 2,.target_residency = 5,.flags = CPUIDLE_FLAG_TIME_VALID,.enter = omap4_enter_idle,.name = "C1",.desc = "MPUSS ON"},...
},.state_count = OMAP4_NUM_STATES,-static inline void _fill_cstate(struct cpuidle_driver *drv,
int idx, const char *descr)-{
struct cpuidle_state *state =&drv->states[idx];state->exit_latency = cpuidle_params_table[idx].exit_latency;state->target_residency = cpuidle_params_table[idx].target_residency;state->flags = CPUIDLE_FLAG_TIME_VALID;state->enter = omap4_enter_idle;sprintf(state->name, "C%d", idx + 1);strncpy(state->desc, descr, CPUIDLE_DESC_LEN);-}
I am OK with this change, which makes the code more readable (and so maintainable).
- static inline struct omap4_idle_statedata *_fill_cstate_usage( struct cpuidle_device *dev, int idx)
@@ -196,37 +213,28 @@ int __init omap4_idle_init(void) if ((!mpu_pd) || (!cpu0_pd) || (!cpu1_pd)) return -ENODEV;
drv->safe_state_index = -1; dev =&per_cpu(omap4_idle_dev, cpu_id); dev->cpu = cpu_id;/* C1 - CPU0 ON + CPU1 ON + MPU ON */_fill_cstate(drv, 0, "MPUSS ON");drv->safe_state_index = 0;I would keep this or add a clear comment that C1 is the safe state.
Actually with the driver's states declaration, the safe_state_index is initialized to zero, which means the default safe_state is always 0 with the new API. But I can add the initialization anyway in the structure declaration if you want.
...
Thanks, Jean
-- 1.7.5.4
-- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
We do not longer need this table as we defined the values in the driver states.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-omap2/cpuidle44xx.c | 11 +---------- 1 files changed, 1 insertions(+), 10 deletions(-)
diff --git a/arch/arm/mach-omap2/cpuidle44xx.c b/arch/arm/mach-omap2/cpuidle44xx.c index cd6bee7..0455858 100644 --- a/arch/arm/mach-omap2/cpuidle44xx.c +++ b/arch/arm/mach-omap2/cpuidle44xx.c @@ -31,16 +31,7 @@ struct omap4_idle_statedata { u32 mpu_state; };
-static struct cpuidle_params cpuidle_params_table[] = { - /* C1 - CPU0 ON + CPU1 ON + MPU ON */ - {.exit_latency = 2 + 2 , .target_residency = 5 }, - /* C2- CPU0 OFF + CPU1 OFF + MPU CSWR */ - {.exit_latency = 328 + 440 , .target_residency = 960 }, - /* C3 - CPU0 OFF + CPU1 OFF + MPU OSWR */ - {.exit_latency = 460 + 518 , .target_residency = 1100 }, -}; - -#define OMAP4_NUM_STATES ARRAY_SIZE(cpuidle_params_table) +#define OMAP4_NUM_STATES 3
struct omap4_idle_statedata omap4_idle_data[OMAP4_NUM_STATES]; static struct powerdomain *mpu_pd, *cpu0_pd, *cpu1_pd;
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-omap2/cpuidle44xx.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/arm/mach-omap2/cpuidle44xx.c b/arch/arm/mach-omap2/cpuidle44xx.c index 0455858..254f97b 100644 --- a/arch/arm/mach-omap2/cpuidle44xx.c +++ b/arch/arm/mach-omap2/cpuidle44xx.c @@ -33,7 +33,7 @@ struct omap4_idle_statedata {
#define OMAP4_NUM_STATES 3
-struct omap4_idle_statedata omap4_idle_data[OMAP4_NUM_STATES]; +static struct omap4_idle_statedata omap4_idle_data[OMAP4_NUM_STATES]; static struct powerdomain *mpu_pd, *cpu0_pd, *cpu1_pd;
/**
On Wednesday 21 March 2012 02:57 PM, Daniel Lezcano wrote:
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
arch/arm/mach-omap2/cpuidle44xx.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/arm/mach-omap2/cpuidle44xx.c b/arch/arm/mach-omap2/cpuidle44xx.c index 0455858..254f97b 100644 --- a/arch/arm/mach-omap2/cpuidle44xx.c +++ b/arch/arm/mach-omap2/cpuidle44xx.c @@ -33,7 +33,7 @@ struct omap4_idle_statedata { #define OMAP4_NUM_STATES 3 -struct omap4_idle_statedata omap4_idle_data[OMAP4_NUM_STATES]; +static struct omap4_idle_statedata omap4_idle_data[OMAP4_NUM_STATES];
OK
Regards santosh
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-omap2/cpuidle44xx.c | 19 ++++++++++++++++++- 1 files changed, 18 insertions(+), 1 deletions(-)
diff --git a/arch/arm/mach-omap2/cpuidle44xx.c b/arch/arm/mach-omap2/cpuidle44xx.c index 254f97b..e14cd56 100644 --- a/arch/arm/mach-omap2/cpuidle44xx.c +++ b/arch/arm/mach-omap2/cpuidle44xx.c @@ -33,7 +33,24 @@ struct omap4_idle_statedata {
#define OMAP4_NUM_STATES 3
-static struct omap4_idle_statedata omap4_idle_data[OMAP4_NUM_STATES]; +static struct omap4_idle_statedata omap4_idle_data[] = { + { + .cpu_state = PWRDM_POWER_ON, + .mpu_state = PWRDM_POWER_ON, + .mpu_logic_state = PWRDM_POWER_RET, + }, + { + .cpu_state = PWRDM_POWER_OFF, + .mpu_state = PWRDM_POWER_RET, + .mpu_logic_state = PWRDM_POWER_RET, + }, + { + .cpu_state = PWRDM_POWER_OFF, + .mpu_state = PWRDM_POWER_RET, + .mpu_logic_state = PWRDM_POWER_OFF, + }, +}; + static struct powerdomain *mpu_pd, *cpu0_pd, *cpu1_pd;
/**
We are storing the 'omap4_idle_data' in the private data field if the cpuidle device. As we are using this variable only in this file, that does not really make sense. Let's use the global variable directly instead dereferencing pointers in an idle critical loop.
Also, that simplfies the code.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-omap2/cpuidle44xx.c | 17 +++++------------ 1 files changed, 5 insertions(+), 12 deletions(-)
diff --git a/arch/arm/mach-omap2/cpuidle44xx.c b/arch/arm/mach-omap2/cpuidle44xx.c index e14cd56..cb91d1f 100644 --- a/arch/arm/mach-omap2/cpuidle44xx.c +++ b/arch/arm/mach-omap2/cpuidle44xx.c @@ -24,7 +24,7 @@
#ifdef CONFIG_CPU_IDLE
-/* Machine specific information to be recorded in the C-state driver_data */ +/* Machine specific information */ struct omap4_idle_statedata { u32 cpu_state; u32 mpu_logic_state; @@ -67,8 +67,7 @@ static int omap4_enter_idle(struct cpuidle_device *dev, struct cpuidle_driver *drv, int index) { - struct omap4_idle_statedata *cx = - cpuidle_get_statedata(&dev->states_usage[index]); + struct omap4_idle_statedata *cx = &omap4_idle_data[index]; struct timespec ts_preidle, ts_postidle, ts_idle; u32 cpu1_state; int idle_time; @@ -92,7 +91,7 @@ static int omap4_enter_idle(struct cpuidle_device *dev, cpu1_state = pwrdm_read_pwrst(cpu1_pd); if (cpu1_state != PWRDM_POWER_OFF) { new_state_idx = drv->safe_state_index; - cx = cpuidle_get_statedata(&dev->states_usage[new_state_idx]); + cx = &omap4_idle_data[new_state_idx] }
if (index > 0) @@ -193,15 +192,9 @@ static inline struct omap4_idle_statedata *_fill_cstate_usage( int idx) { struct omap4_idle_statedata *cx = &omap4_idle_data[idx]; - struct cpuidle_state_usage *state_usage = &dev->states_usage[idx]; - - cpuidle_set_statedata(state_usage, cx); - return cx; }
- - /** * omap4_idle_init - Init routine for OMAP4 idle * @@ -210,9 +203,9 @@ static inline struct omap4_idle_statedata *_fill_cstate_usage( */ int __init omap4_idle_init(void) { - struct omap4_idle_statedata *cx; - struct cpuidle_device *dev; + struct omap4_idle_statedata *cx = &omap4_idle_data[index]; struct cpuidle_driver *drv = &omap4_idle_driver; + struct cpuidle_device *dev; unsigned int cpu_id = 0;
mpu_pd = pwrdm_lookup("mpu_pwrdm");
We initialized it at compile time, no need to do that at boot time.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-omap2/cpuidle44xx.c | 26 +------------------------- 1 files changed, 1 insertions(+), 25 deletions(-)
diff --git a/arch/arm/mach-omap2/cpuidle44xx.c b/arch/arm/mach-omap2/cpuidle44xx.c index cb91d1f..fd220f9 100644 --- a/arch/arm/mach-omap2/cpuidle44xx.c +++ b/arch/arm/mach-omap2/cpuidle44xx.c @@ -33,7 +33,7 @@ struct omap4_idle_statedata {
#define OMAP4_NUM_STATES 3
-static struct omap4_idle_statedata omap4_idle_data[] = { +static struct omap4_idle_statedata omap4_idle_data[OMAP4_NUM_STATES] = { { .cpu_state = PWRDM_POWER_ON, .mpu_state = PWRDM_POWER_ON, @@ -187,14 +187,6 @@ struct cpuidle_driver omap4_idle_driver = { .state_count = OMAP4_NUM_STATES, };
-static inline struct omap4_idle_statedata *_fill_cstate_usage( - struct cpuidle_device *dev, - int idx) -{ - struct omap4_idle_statedata *cx = &omap4_idle_data[idx]; - return cx; -} - /** * omap4_idle_init - Init routine for OMAP4 idle * @@ -203,7 +195,6 @@ static inline struct omap4_idle_statedata *_fill_cstate_usage( */ int __init omap4_idle_init(void) { - struct omap4_idle_statedata *cx = &omap4_idle_data[index]; struct cpuidle_driver *drv = &omap4_idle_driver; struct cpuidle_device *dev; unsigned int cpu_id = 0; @@ -217,21 +208,6 @@ int __init omap4_idle_init(void) dev = &per_cpu(omap4_idle_dev, cpu_id); dev->cpu = cpu_id;
- cx = _fill_cstate_usage(dev, 0); - cx->cpu_state = PWRDM_POWER_ON; - cx->mpu_state = PWRDM_POWER_ON; - cx->mpu_logic_state = PWRDM_POWER_RET; - - cx = _fill_cstate_usage(dev, 1); - cx->cpu_state = PWRDM_POWER_OFF; - cx->mpu_state = PWRDM_POWER_RET; - cx->mpu_logic_state = PWRDM_POWER_RET; - - cx = _fill_cstate_usage(dev, 2); - cx->cpu_state = PWRDM_POWER_OFF; - cx->mpu_state = PWRDM_POWER_RET; - cx->mpu_logic_state = PWRDM_POWER_OFF; - cpuidle_register_driver(&omap4_idle_driver);
dev->state_count = drv->state_count;
On Wed, Mar 21, 2012 at 2:57 PM, Daniel Lezcano daniel.lezcano@linaro.org wrote:
This patchset is a proposition to improve a bit the code. The changes are code cleanup and does not change the behavior of the driver itself.
Thanks. Will have a look at your series.
A couple a things call my intention. Why the cpuidle device is set for cpu0 only
Because the mainline code CPUIDLE is supported along with CPUhotplug. This is going to change though with Couple CPUIDLE and corresponding OMAP updates.
and why the WFI is not used ?
I didn't get this question. Do you mean the generic WFI? If yes, then, it's mainly because OMAP need additional custom barriers.
Regards Santosh
On 03/21/2012 10:36 AM, Shilimkar, Santosh wrote:
On Wed, Mar 21, 2012 at 2:57 PM, Daniel Lezcano daniel.lezcano@linaro.org wrote:
This patchset is a proposition to improve a bit the code. The changes are code cleanup and does not change the behavior of the driver itself.
Thanks. Will have a look at your series.
Cool, thanks.
A couple a things call my intention. Why the cpuidle device is set for cpu0 only
Because the mainline code CPUIDLE is supported along with CPUhotplug. This is going to change though with Couple CPUIDLE and corresponding OMAP updates.
Ok, thanks for the information. I will look deeper. What happens to cpu1 when it is online and has nothing to do ?
and why the WFI is not used ?
I didn't get this question. Do you mean the generic WFI?
yes.
If yes, then, it's mainly because OMAP need additional custom barriers.
Ah, ok. I am not sure if it is possible but that may be cool if we can call cpu_do_idle instead with additional barrier.
-- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 21 March 2012 03:21 PM, Daniel Lezcano wrote:
On 03/21/2012 10:36 AM, Shilimkar, Santosh wrote:
On Wed, Mar 21, 2012 at 2:57 PM, Daniel Lezcano daniel.lezcano@linaro.org wrote:
This patchset is a proposition to improve a bit the code. The changes are code cleanup and does not change the behavior of the driver itself.
Thanks. Will have a look at your series.
Cool, thanks.
A couple a things call my intention. Why the cpuidle device is set for cpu0 only
Because the mainline code CPUIDLE is supported along with CPUhotplug. This is going to change though with Couple CPUIDLE and corresponding OMAP updates.
Ok, thanks for the information. I will look deeper. What happens to cpu1 when it is online and has nothing to do ?
and why the WFI is not used ?
I didn't get this question. Do you mean the generic WFI?
I execute default idle loop.
yes.
If yes, then, it's mainly because OMAP need additional custom barriers.
Ah, ok. I am not sure if it is possible but that may be cool if we can call cpu_do_idle instead with additional barrier.
There is no need. Since code around WFI is customised, it make no sense to call cpu_do_idle(0 ofr only that instruction sake.
Regards Santosh
On 03/21/2012 10:56 AM, Santosh Shilimkar wrote:
On Wednesday 21 March 2012 03:21 PM, Daniel Lezcano wrote:
On 03/21/2012 10:36 AM, Shilimkar, Santosh wrote:
On Wed, Mar 21, 2012 at 2:57 PM, Daniel Lezcano daniel.lezcano@linaro.org wrote:
This patchset is a proposition to improve a bit the code. The changes are code cleanup and does not change the behavior of the driver itself.
Thanks. Will have a look at your series.
Cool, thanks.
A couple a things call my intention. Why the cpuidle device is set for cpu0 only
Because the mainline code CPUIDLE is supported along with CPUhotplug. This is going to change though with Couple CPUIDLE and corresponding OMAP updates.
Ok, thanks for the information. I will look deeper. What happens to cpu1 when it is online and has nothing to do ?
and why the WFI is not used ?
I didn't get this question. Do you mean the generic WFI?
I execute default idle loop.
So is it not possible to add a cpuidle device for cpu1 and define only one state for the 'wfi-for-omap' ?
yes.
If yes, then, it's mainly because OMAP need additional custom barriers.
Ah, ok. I am not sure if it is possible but that may be cool if we can call cpu_do_idle instead with additional barrier.
There is no need. Since code around WFI is customised, it make no sense to call cpu_do_idle(0 ofr only that instruction sake.
For my personal information, why the WFI is customised for omap4 ?
Thanks -- Daniel
On Wed, Mar 21, 2012 at 4:13 PM, Daniel Lezcano daniel.lezcano@linaro.org wrote:
On 03/21/2012 10:56 AM, Santosh Shilimkar wrote:
On Wednesday 21 March 2012 03:21 PM, Daniel Lezcano wrote:
On 03/21/2012 10:36 AM, Shilimkar, Santosh wrote:
On Wed, Mar 21, 2012 at 2:57 PM, Daniel Lezcano daniel.lezcano@linaro.org wrote:
This patchset is a proposition to improve a bit the code. The changes are code cleanup and does not change the behavior of the driver itself.
Thanks. Will have a look at your series.
Cool, thanks.
A couple a things call my intention. Why the cpuidle device is set for cpu0 only
Because the mainline code CPUIDLE is supported along with CPUhotplug. This is going to change though with Couple CPUIDLE and corresponding OMAP updates.
Ok, thanks for the information. I will look deeper. What happens to cpu1 when it is online and has nothing to do ?
and why the WFI is not used ?
I didn't get this question. Do you mean the generic WFI?
I execute default idle loop.
So is it not possible to add a cpuidle device for cpu1 and define only one state for the 'wfi-for-omap' ?
That's how my post was handling it. But after the review Kevin suggested me to drop the CPU1 shallow state since it was same as default idle.
yes.
If yes, then, it's mainly because OMAP need additional custom barriers.
Ah, ok. I am not sure if it is possible but that may be cool if we can call cpu_do_idle instead with additional barrier.
There is no need. Since code around WFI is customised, it make no sense to call cpu_do_idle(0 ofr only that instruction sake.
For my personal information, why the WFI is customised for omap4 ?
OMAP4 silicon has couple of hardware issues around interconnect and needs to drain the axi buffers with strongly order writes to ensure that data reaches to peripherals and not get stuck. That lead to have custom function. Note that, the wfi instruction itself is same.
On 03/21/2012 11:49 AM, Shilimkar, Santosh wrote:
On Wed, Mar 21, 2012 at 4:13 PM, Daniel Lezcano daniel.lezcano@linaro.org wrote:
On 03/21/2012 10:56 AM, Santosh Shilimkar wrote:
On Wednesday 21 March 2012 03:21 PM, Daniel Lezcano wrote:
On 03/21/2012 10:36 AM, Shilimkar, Santosh wrote:
On Wed, Mar 21, 2012 at 2:57 PM, Daniel Lezcano daniel.lezcano@linaro.org wrote:
This patchset is a proposition to improve a bit the code. The changes are code cleanup and does not change the behavior of the driver itself.
Thanks. Will have a look at your series.
Cool, thanks.
A couple a things call my intention. Why the cpuidle device is set for cpu0 only
Because the mainline code CPUIDLE is supported along with CPUhotplug. This is going to change though with Couple CPUIDLE and corresponding OMAP updates.
Ok, thanks for the information. I will look deeper. What happens to cpu1 when it is online and has nothing to do ?
and why the WFI is not used ?
I didn't get this question. Do you mean the generic WFI?
I execute default idle loop.
So is it not possible to add a cpuidle device for cpu1 and define only one state for the 'wfi-for-omap' ?
That's how my post was handling it. But after the review Kevin suggested me to drop the CPU1 shallow state since it was same as default idle.
Ok, thanks. I am asking that because the more I am looking at the different SoCs cpuidle drivers, the more I am convinced we can factor out more code across SoCs.
yes.
If yes, then, it's mainly because OMAP need additional custom barriers.
Ah, ok. I am not sure if it is possible but that may be cool if we can call cpu_do_idle instead with additional barrier.
There is no need. Since code around WFI is customised, it make no sense to call cpu_do_idle(0 ofr only that instruction sake.
For my personal information, why the WFI is customised for omap4 ?
OMAP4 silicon has couple of hardware issues around interconnect and needs to drain the axi buffers with strongly order writes to ensure that data reaches to peripherals and not get stuck. That lead to have custom function. Note that, the wfi instruction itself is same.
Thanks for the explanation.
-- Daniel
Daniel,
On Wednesday 21 March 2012 02:57 PM, Daniel Lezcano wrote:
This patchset is a proposition to improve a bit the code. The changes are code cleanup and does not change the behavior of the driver itself.
A couple a things call my intention. Why the cpuidle device is set for cpu0 only and why the WFI is not used ?
Daniel Lezcano (7): ARM: OMAP4: cpuidle - Remove unused valid field ARM: OMAP4: cpuidle - Declare the states with the driver declaration ARM: OMAP4: cpuidle - Remove the cpuidle_params_table table ARM: OMAP4: cpuidle - fix static omap4_idle_data declaration ARM: OMAP4: cpuidle - Initialize omap4_idle_data at compile time ARM: OMAP4: cpuidle - use the omap4_idle_data variable directly ARM: OMAP4: cpuidle - remove omap4_idle_data initialization at boot time
The series looks fine to me in general. This clean-up is applicable for OMAP3 cpuidle code as well.
I want Jean to look at this series because some of his earlier clean up has introduced those custom functions which are getting removed in this series.
Regards santosh
On 03/21/2012 11:07 AM, Santosh Shilimkar wrote:
Daniel,
On Wednesday 21 March 2012 02:57 PM, Daniel Lezcano wrote:
This patchset is a proposition to improve a bit the code. The changes are code cleanup and does not change the behavior of the driver itself.
A couple a things call my intention. Why the cpuidle device is set for cpu0 only and why the WFI is not used ?
Daniel Lezcano (7): ARM: OMAP4: cpuidle - Remove unused valid field ARM: OMAP4: cpuidle - Declare the states with the driver declaration ARM: OMAP4: cpuidle - Remove the cpuidle_params_table table ARM: OMAP4: cpuidle - fix static omap4_idle_data declaration ARM: OMAP4: cpuidle - Initialize omap4_idle_data at compile time ARM: OMAP4: cpuidle - use the omap4_idle_data variable directly ARM: OMAP4: cpuidle - remove omap4_idle_data initialization at boot time
The series looks fine to me in general. This clean-up is applicable for OMAP3 cpuidle code as well.
Yes, I am planning to do the same for omap3.
Thanks for reviewing.
-- Daniel
I want Jean to look at this series because some of his earlier clean up has introduced those custom functions which are getting removed in this series.
Regards santosh
Hi Santosh, Daniel,
On Wed, Mar 21, 2012 at 11:07 AM, Santosh Shilimkar santosh.shilimkar@ti.com wrote:
Daniel,
On Wednesday 21 March 2012 02:57 PM, Daniel Lezcano wrote:
This patchset is a proposition to improve a bit the code. The changes are code cleanup and does not change the behavior of the driver itself.
A couple a things call my intention. Why the cpuidle device is set for cpu0 only and why the WFI is not used ?
Daniel Lezcano (7): ARM: OMAP4: cpuidle - Remove unused valid field ARM: OMAP4: cpuidle - Declare the states with the driver declaration ARM: OMAP4: cpuidle - Remove the cpuidle_params_table table ARM: OMAP4: cpuidle - fix static omap4_idle_data declaration ARM: OMAP4: cpuidle - Initialize omap4_idle_data at compile time ARM: OMAP4: cpuidle - use the omap4_idle_data variable directly ARM: OMAP4: cpuidle - remove omap4_idle_data initialization at boot time
The series looks fine to me in general. This clean-up is applicable for OMAP3 cpuidle code as well.
I want Jean to look at this series because some of his earlier clean up has introduced those custom functions which are getting removed in this series.
I am OK with the patch set, I only have minor remarks to the individual patches.
Reviewed-by: Jean Pihet j-pihet@ti.com
Regards santosh
Thanks! Jean
On 03/21/2012 02:19 PM, Jean Pihet wrote:
Hi Santosh, Daniel,
On Wed, Mar 21, 2012 at 11:07 AM, Santosh Shilimkar santosh.shilimkar@ti.com wrote:
Daniel,
On Wednesday 21 March 2012 02:57 PM, Daniel Lezcano wrote:
This patchset is a proposition to improve a bit the code. The changes are code cleanup and does not change the behavior of the driver itself.
A couple a things call my intention. Why the cpuidle device is set for cpu0 only and why the WFI is not used ?
Daniel Lezcano (7): ARM: OMAP4: cpuidle - Remove unused valid field ARM: OMAP4: cpuidle - Declare the states with the driver declaration ARM: OMAP4: cpuidle - Remove the cpuidle_params_table table ARM: OMAP4: cpuidle - fix static omap4_idle_data declaration ARM: OMAP4: cpuidle - Initialize omap4_idle_data at compile time ARM: OMAP4: cpuidle - use the omap4_idle_data variable directly ARM: OMAP4: cpuidle - remove omap4_idle_data initialization at boot time
The series looks fine to me in general. This clean-up is applicable for OMAP3 cpuidle code as well.
I want Jean to look at this series because some of his earlier clean up has introduced those custom functions which are getting removed in this series.
I am OK with the patch set, I only have minor remarks to the individual patches.
Reviewed-by: Jean Pihetj-pihet@ti.com
Thanks for the review Jean.
Thanks! Jean
On Wed, Mar 21, 2012 at 6:49 PM, Jean Pihet jean.pihet@newoldbits.com wrote:
Hi Santosh, Daniel,
On Wed, Mar 21, 2012 at 11:07 AM, Santosh Shilimkar santosh.shilimkar@ti.com wrote:
Daniel,
On Wednesday 21 March 2012 02:57 PM, Daniel Lezcano wrote:
This patchset is a proposition to improve a bit the code. The changes are code cleanup and does not change the behavior of the driver itself.
A couple a things call my intention. Why the cpuidle device is set for cpu0 only and why the WFI is not used ?
Daniel Lezcano (7): ARM: OMAP4: cpuidle - Remove unused valid field ARM: OMAP4: cpuidle - Declare the states with the driver declaration ARM: OMAP4: cpuidle - Remove the cpuidle_params_table table ARM: OMAP4: cpuidle - fix static omap4_idle_data declaration ARM: OMAP4: cpuidle - Initialize omap4_idle_data at compile time ARM: OMAP4: cpuidle - use the omap4_idle_data variable directly ARM: OMAP4: cpuidle - remove omap4_idle_data initialization at boot time
The series looks fine to me in general. This clean-up is applicable for OMAP3 cpuidle code as well.
I want Jean to look at this series because some of his earlier clean up has introduced those custom functions which are getting removed in this series.
I am OK with the patch set, I only have minor remarks to the individual patches.
Reviewed-by: Jean Pihet j-pihet@ti.com
Thanks Jean for looking at it.
Daniel, Feel free to add. Reviewed-by: Santosh Shilimkar santosh.shilimkar@ti.com
On Wed, Mar 21, 2012 at 11:07 AM, Santosh Shilimkar santosh.shilimkar@ti.com wrote:
Daniel,
On Wednesday 21 March 2012 02:57 PM, Daniel Lezcano wrote:
This patchset is a proposition to improve a bit the code. The changes are code cleanup and does not change the behavior of the driver itself.
A couple a things call my intention. Why the cpuidle device is set for cpu0 only and why the WFI is not used ?
Daniel Lezcano (7): ARM: OMAP4: cpuidle - Remove unused valid field ARM: OMAP4: cpuidle - Declare the states with the driver declaration ARM: OMAP4: cpuidle - Remove the cpuidle_params_table table ARM: OMAP4: cpuidle - fix static omap4_idle_data declaration ARM: OMAP4: cpuidle - Initialize omap4_idle_data at compile time ARM: OMAP4: cpuidle - use the omap4_idle_data variable directly ARM: OMAP4: cpuidle - remove omap4_idle_data initialization at boot time
The series looks fine to me in general. This clean-up is applicable for OMAP3 cpuidle code as well.
Great! However OMAP3 has a few specific things that cannot be removed as easily: - the 'valid' flag is used because only certain combinations of power domains states are possible, - the latency settings can be overriden by the board code, so the cpuidle_params struct is needed.
I want Jean to look at this series because some of his earlier clean up has introduced those custom functions which are getting removed in this series.
Regards santosh
Thanks, Jean
On 03/21/2012 02:43 PM, Jean Pihet wrote:
On Wed, Mar 21, 2012 at 11:07 AM, Santosh Shilimkar santosh.shilimkar@ti.com wrote:
Daniel,
On Wednesday 21 March 2012 02:57 PM, Daniel Lezcano wrote:
This patchset is a proposition to improve a bit the code. The changes are code cleanup and does not change the behavior of the driver itself.
A couple a things call my intention. Why the cpuidle device is set for cpu0 only and why the WFI is not used ?
Daniel Lezcano (7): ARM: OMAP4: cpuidle - Remove unused valid field ARM: OMAP4: cpuidle - Declare the states with the driver declaration ARM: OMAP4: cpuidle - Remove the cpuidle_params_table table ARM: OMAP4: cpuidle - fix static omap4_idle_data declaration ARM: OMAP4: cpuidle - Initialize omap4_idle_data at compile time ARM: OMAP4: cpuidle - use the omap4_idle_data variable directly ARM: OMAP4: cpuidle - remove omap4_idle_data initialization at boot time
The series looks fine to me in general. This clean-up is applicable for OMAP3 cpuidle code as well.
Great! However OMAP3 has a few specific things that cannot be removed as easily:
- the 'valid' flag is used because only certain combinations of power
domains states are possible,
- the latency settings can be overriden by the board code, so the
cpuidle_params struct is needed.
Right, I noticed that. I am looking for a way to have a similar cleanup for omap3 but without breaking the rx51 board.
Thanks -- Daniel
I want Jean to look at this series because some of his earlier clean up has introduced those custom functions which are getting removed in this series.
Regards santosh
Thanks, Jean
On 03/21/2012 02:43 PM, Jean Pihet wrote:
On Wed, Mar 21, 2012 at 11:07 AM, Santosh Shilimkar santosh.shilimkar@ti.com wrote:
Daniel,
On Wednesday 21 March 2012 02:57 PM, Daniel Lezcano wrote:
This patchset is a proposition to improve a bit the code. The changes are code cleanup and does not change the behavior of the driver itself.
A couple a things call my intention. Why the cpuidle device is set for cpu0 only and why the WFI is not used ?
Daniel Lezcano (7): ARM: OMAP4: cpuidle - Remove unused valid field ARM: OMAP4: cpuidle - Declare the states with the driver declaration ARM: OMAP4: cpuidle - Remove the cpuidle_params_table table ARM: OMAP4: cpuidle - fix static omap4_idle_data declaration ARM: OMAP4: cpuidle - Initialize omap4_idle_data at compile time ARM: OMAP4: cpuidle - use the omap4_idle_data variable directly ARM: OMAP4: cpuidle - remove omap4_idle_data initialization at boot time
The series looks fine to me in general. This clean-up is applicable for OMAP3 cpuidle code as well.
Great! However OMAP3 has a few specific things that cannot be removed as easily:
- the 'valid' flag is used because only certain combinations of power
domains states are possible,
When I look the board-rx51 code I see:
static struct cpuidle_params rx51_cpuidle_params[] = { /* C1 */ {110 + 162, 5 , 1}, /* C2 */ {106 + 180, 309, 1}, /* C3 */ {107 + 410, 46057, 0}, /* C4 */ {121 + 3374, 46057, 0}, /* C5 */ {855 + 1146, 46057, 1}, /* C6 */ {7580 + 4134, 484329, 0}, /* C7 */ {7505 + 15274, 484329, 1}, };
If C3, C4, C6 are not valid, so AFAICS never used in the cpuidle code why the values are 'exit_latency' and 'target_residency' specified ? I mean why don't we have { 0, 0, 0 } ? Is it just for information ?
I understand the purpose of this code but it looks a bit tricky and hard to factor out. Is it acceptable to create a new cpuidle driver for rx51 then we factor out the code between omap3, omap4 and rx51 when all the code consistent ?
- the latency settings can be overriden by the board code, so the
cpuidle_params struct is needed.
I want Jean to look at this series because some of his earlier clean up has introduced those custom functions which are getting removed in this series.
Regards santosh
Thanks, Jean
Daniel Lezcano daniel.lezcano@linaro.org writes:
On 03/21/2012 02:43 PM, Jean Pihet wrote:
On Wed, Mar 21, 2012 at 11:07 AM, Santosh Shilimkar santosh.shilimkar@ti.com wrote:
Daniel,
On Wednesday 21 March 2012 02:57 PM, Daniel Lezcano wrote:
This patchset is a proposition to improve a bit the code. The changes are code cleanup and does not change the behavior of the driver itself.
A couple a things call my intention. Why the cpuidle device is set for cpu0 only and why the WFI is not used ?
Daniel Lezcano (7): ARM: OMAP4: cpuidle - Remove unused valid field ARM: OMAP4: cpuidle - Declare the states with the driver declaration ARM: OMAP4: cpuidle - Remove the cpuidle_params_table table ARM: OMAP4: cpuidle - fix static omap4_idle_data declaration ARM: OMAP4: cpuidle - Initialize omap4_idle_data at compile time ARM: OMAP4: cpuidle - use the omap4_idle_data variable directly ARM: OMAP4: cpuidle - remove omap4_idle_data initialization at boot time
The series looks fine to me in general. This clean-up is applicable for OMAP3 cpuidle code as well.
Great! However OMAP3 has a few specific things that cannot be removed as easily:
- the 'valid' flag is used because only certain combinations of power
domains states are possible,
When I look the board-rx51 code I see:
static struct cpuidle_params rx51_cpuidle_params[] = { /* C1 */ {110 + 162, 5 , 1}, /* C2 */ {106 + 180, 309, 1}, /* C3 */ {107 + 410, 46057, 0}, /* C4 */ {121 + 3374, 46057, 0}, /* C5 */ {855 + 1146, 46057, 1}, /* C6 */ {7580 + 4134, 484329, 0}, /* C7 */ {7505 + 15274, 484329, 1}, };
If C3, C4, C6 are not valid, so AFAICS never used in the cpuidle code why the values are 'exit_latency' and 'target_residency' specified ? I mean why don't we have { 0, 0, 0 } ? Is it just for information ?
Yes, because getting those numbers were based on some board-specific measurements, and we don't want those values to be lost. Also, some usage patterns might want to (re)enable those states.
I understand the purpose of this code but it looks a bit tricky and hard to factor out. Is it acceptable to create a new cpuidle driver for rx51 then we factor out the code between omap3, omap4 and rx51 when all the code consistent ?
I don't like that idea. All the code is the same, the only thing we need is the ability to pass in board-specific latency numbers for the C-states.
These latency numbers are obviously quite significant when it comes to the final power consumption, and these values often depend on board-specific settings. Until there are generic frameworks for defining all the latencies involved, passing these values in from board files is absolutly needed.
Kevin
On 03/21/2012 10:54 PM, Kevin Hilman wrote:
Daniel Lezcanodaniel.lezcano@linaro.org writes:
On 03/21/2012 02:43 PM, Jean Pihet wrote:
On Wed, Mar 21, 2012 at 11:07 AM, Santosh Shilimkar santosh.shilimkar@ti.com wrote:
Daniel,
On Wednesday 21 March 2012 02:57 PM, Daniel Lezcano wrote:
This patchset is a proposition to improve a bit the code. The changes are code cleanup and does not change the behavior of the driver itself.
A couple a things call my intention. Why the cpuidle device is set for cpu0 only and why the WFI is not used ?
Daniel Lezcano (7): ARM: OMAP4: cpuidle - Remove unused valid field ARM: OMAP4: cpuidle - Declare the states with the driver declaration ARM: OMAP4: cpuidle - Remove the cpuidle_params_table table ARM: OMAP4: cpuidle - fix static omap4_idle_data declaration ARM: OMAP4: cpuidle - Initialize omap4_idle_data at compile time ARM: OMAP4: cpuidle - use the omap4_idle_data variable directly ARM: OMAP4: cpuidle - remove omap4_idle_data initialization at boot time
The series looks fine to me in general. This clean-up is applicable for OMAP3 cpuidle code as well.
Great! However OMAP3 has a few specific things that cannot be removed as easily:
- the 'valid' flag is used because only certain combinations of power
domains states are possible,
When I look the board-rx51 code I see:
static struct cpuidle_params rx51_cpuidle_params[] = { /* C1 */ {110 + 162, 5 , 1}, /* C2 */ {106 + 180, 309, 1}, /* C3 */ {107 + 410, 46057, 0}, /* C4 */ {121 + 3374, 46057, 0}, /* C5 */ {855 + 1146, 46057, 1}, /* C6 */ {7580 + 4134, 484329, 0}, /* C7 */ {7505 + 15274, 484329, 1}, };
If C3, C4, C6 are not valid, so AFAICS never used in the cpuidle code why the values are 'exit_latency' and 'target_residency' specified ? I mean why don't we have { 0, 0, 0 } ? Is it just for information ?
Yes, because getting those numbers were based on some board-specific measurements, and we don't want those values to be lost. Also, some usage patterns might want to (re)enable those states.
When you say re-enable you mean for a custom kernel ?
I understand the purpose of this code but it looks a bit tricky and hard to factor out. Is it acceptable to create a new cpuidle driver for rx51 then we factor out the code between omap3, omap4 and rx51 when all the code consistent ?
I don't like that idea. All the code is the same, the only thing we need is the ability to pass in board-specific latency numbers for the C-states.
Sorry I was not clear, I was not saying duplicating the code. What I meant is to create a driver:
struct cpuidle_driver rx51_idle_driver = { .name = "rx51_idle", .owner = THIS_MODULE, .states = { { /* What is in rx51_cpuidle_params */ } };
We put in there only the valid fields and we keep in a comment the table with the latency numbers.
Assuming the valid field is for handling the table overwritting, we can then remove it. By this way, we simplify the next_valid_state function.
Depending if we have rx51 or not, we register the rx51 driver or the omap3 driver in the init function. That has also the benefit to group the cpuidle code in the cpuidle34xx file.
These latency numbers are obviously quite significant when it comes to the final power consumption, and these values often depend on board-specific settings. Until there are generic frameworks for defining all the latencies involved, passing these values in from board files is absolutly needed.
Yes but before creating the generic framework, we have to do a transversal cpuidle consolidation across the SoC, factor out the code when possible, and ensure the consistency between the different platform and see a common pattern to emerge from this work.
Daniel Lezcano daniel.lezcano@linaro.org writes:
On 03/21/2012 10:54 PM, Kevin Hilman wrote:
Daniel Lezcanodaniel.lezcano@linaro.org writes:
On 03/21/2012 02:43 PM, Jean Pihet wrote:
On Wed, Mar 21, 2012 at 11:07 AM, Santosh Shilimkar santosh.shilimkar@ti.com wrote:
Daniel,
On Wednesday 21 March 2012 02:57 PM, Daniel Lezcano wrote:
This patchset is a proposition to improve a bit the code. The changes are code cleanup and does not change the behavior of the driver itself.
A couple a things call my intention. Why the cpuidle device is set for cpu0 only and why the WFI is not used ?
Daniel Lezcano (7): ARM: OMAP4: cpuidle - Remove unused valid field ARM: OMAP4: cpuidle - Declare the states with the driver declaration ARM: OMAP4: cpuidle - Remove the cpuidle_params_table table ARM: OMAP4: cpuidle - fix static omap4_idle_data declaration ARM: OMAP4: cpuidle - Initialize omap4_idle_data at compile time ARM: OMAP4: cpuidle - use the omap4_idle_data variable directly ARM: OMAP4: cpuidle - remove omap4_idle_data initialization at boot time
The series looks fine to me in general. This clean-up is applicable for OMAP3 cpuidle code as well.
Great! However OMAP3 has a few specific things that cannot be removed as easily:
- the 'valid' flag is used because only certain combinations of power
domains states are possible,
When I look the board-rx51 code I see:
static struct cpuidle_params rx51_cpuidle_params[] = { /* C1 */ {110 + 162, 5 , 1}, /* C2 */ {106 + 180, 309, 1}, /* C3 */ {107 + 410, 46057, 0}, /* C4 */ {121 + 3374, 46057, 0}, /* C5 */ {855 + 1146, 46057, 1}, /* C6 */ {7580 + 4134, 484329, 0}, /* C7 */ {7505 + 15274, 484329, 1}, };
If C3, C4, C6 are not valid, so AFAICS never used in the cpuidle code why the values are 'exit_latency' and 'target_residency' specified ? I mean why don't we have { 0, 0, 0 } ? Is it just for information ?
Yes, because getting those numbers were based on some board-specific measurements, and we don't want those values to be lost. Also, some usage patterns might want to (re)enable those states.
When you say re-enable you mean for a custom kernel ?
Yes.
I understand the purpose of this code but it looks a bit tricky and hard to factor out. Is it acceptable to create a new cpuidle driver for rx51 then we factor out the code between omap3, omap4 and rx51 when all the code consistent ?
I don't like that idea. All the code is the same, the only thing we need is the ability to pass in board-specific latency numbers for the C-states.
Sorry I was not clear, I was not saying duplicating the code. What I meant is to create a driver:
struct cpuidle_driver rx51_idle_driver = { .name = "rx51_idle", .owner = THIS_MODULE, .states = { { /* What is in rx51_cpuidle_params */ } };
We put in there only the valid fields and we keep in a comment the table with the latency numbers.
Ah, OK. I misunderstood.
Assuming the valid field is for handling the table overwritting, we can then remove it. By this way, we simplify the next_valid_state function.
probably we can remove next_valid_state all together after 3.4 since the new sysfs entry for that feature looks to be queued in linux-next.
Depending if we have rx51 or not, we register the rx51 driver or the omap3 driver in the init function. That has also the benefit to group the cpuidle code in the cpuidle34xx file.
yes, but with board-specific data in SoC-specific code, which is not a clean separation IMO. How would you plan to pass which board it's running on?
These latency numbers are obviously quite significant when it comes to the final power consumption, and these values often depend on board-specific settings. Until there are generic frameworks for defining all the latencies involved, passing these values in from board files is absolutly needed.
Yes but before creating the generic framework, we have to do a transversal cpuidle consolidation across the SoC, factor out the code when possible, and ensure the consistency between the different platform and see a common pattern to emerge from this work.
Agreed, but if that means ignoring platform-specific customization, and not putting in other mechanisms to configure platform specific details, it is throwing away useful infrastructure.
IMO, any consolidated framework needs some way to customize or pass in latencies from platform-specific code. Long term, I suppose this needs to be DT based.
That being said, I do want to see this consolidation happen, so...
In OMAP land, we are in the middle of putting in place a better framework for defining/tracking the various latencies involved in PM transitions (using per-device PM Qos.) After that, we will likely be reworking and revalidating these latency numbers for all OMAPs
So maybe the best approach to help in consolidation is just to drop the board-rx51 data all together for now, as well as the ability to pass custom C states from board files.
The default, unoptimized OMAP3 numbers will work fine for that board, and anyone wanting to do optimized power work for that platform can still do it with a custom kernel. (There is still lots of out of tree work for the n900 that never made it upstream, so I doubt the mainline users of n900 will be affected by this level of power tweaking.)
If we do that, then as part of the consolidation effort, some DT-based customization should be defined as well to override/customize C states.
Kevin
On 03/22/2012 07:36 PM, Kevin Hilman wrote:
Daniel Lezcanodaniel.lezcano@linaro.org writes:
On 03/21/2012 10:54 PM, Kevin Hilman wrote:
Daniel Lezcanodaniel.lezcano@linaro.org writes:
On 03/21/2012 02:43 PM, Jean Pihet wrote:
On Wed, Mar 21, 2012 at 11:07 AM, Santosh Shilimkar santosh.shilimkar@ti.com wrote:
Daniel,
On Wednesday 21 March 2012 02:57 PM, Daniel Lezcano wrote: > This patchset is a proposition to improve a bit the code. > The changes are code cleanup and does not change the behavior of the > driver itself. > > A couple a things call my intention. Why the cpuidle device is set for cpu0 only > and why the WFI is not used ? > > Daniel Lezcano (7): > ARM: OMAP4: cpuidle - Remove unused valid field > ARM: OMAP4: cpuidle - Declare the states with the driver declaration > ARM: OMAP4: cpuidle - Remove the cpuidle_params_table table > ARM: OMAP4: cpuidle - fix static omap4_idle_data declaration > ARM: OMAP4: cpuidle - Initialize omap4_idle_data at compile time > ARM: OMAP4: cpuidle - use the omap4_idle_data variable directly > ARM: OMAP4: cpuidle - remove omap4_idle_data initialization at boot > time > The series looks fine to me in general. This clean-up is applicable for OMAP3 cpuidle code as well.
Great! However OMAP3 has a few specific things that cannot be removed as easily:
- the 'valid' flag is used because only certain combinations of power
domains states are possible,
When I look the board-rx51 code I see:
static struct cpuidle_params rx51_cpuidle_params[] = { /* C1 */ {110 + 162, 5 , 1}, /* C2 */ {106 + 180, 309, 1}, /* C3 */ {107 + 410, 46057, 0}, /* C4 */ {121 + 3374, 46057, 0}, /* C5 */ {855 + 1146, 46057, 1}, /* C6 */ {7580 + 4134, 484329, 0}, /* C7 */ {7505 + 15274, 484329, 1}, };
If C3, C4, C6 are not valid, so AFAICS never used in the cpuidle code why the values are 'exit_latency' and 'target_residency' specified ? I mean why don't we have { 0, 0, 0 } ? Is it just for information ?
Yes, because getting those numbers were based on some board-specific measurements, and we don't want those values to be lost. Also, some usage patterns might want to (re)enable those states.
When you say re-enable you mean for a custom kernel ?
Yes.
I understand the purpose of this code but it looks a bit tricky and hard to factor out. Is it acceptable to create a new cpuidle driver for rx51 then we factor out the code between omap3, omap4 and rx51 when all the code consistent ?
I don't like that idea. All the code is the same, the only thing we need is the ability to pass in board-specific latency numbers for the C-states.
Sorry I was not clear, I was not saying duplicating the code. What I meant is to create a driver:
struct cpuidle_driver rx51_idle_driver = { .name = "rx51_idle", .owner = THIS_MODULE, .states = { { /* What is in rx51_cpuidle_params */ } };
We put in there only the valid fields and we keep in a comment the table with the latency numbers.
Ah, OK. I misunderstood.
Assuming the valid field is for handling the table overwritting, we can then remove it. By this way, we simplify the next_valid_state function.
probably we can remove next_valid_state all together after 3.4 since the new sysfs entry for that feature looks to be queued in linux-next.
Oh, that could be very cool. That will remove a reasonable chunk of code.
Depending if we have rx51 or not, we register the rx51 driver or the omap3 driver in the init function. That has also the benefit to group the cpuidle code in the cpuidle34xx file.
yes, but with board-specific data in SoC-specific code, which is not a clean separation IMO. How would you plan to pass which board it's running on?
I was thinking using the same code path than the array overriding for cpuidle_data initialization. I did not looked at this initialization part in details neither the order.
These latency numbers are obviously quite significant when it comes to the final power consumption, and these values often depend on board-specific settings. Until there are generic frameworks for defining all the latencies involved, passing these values in from board files is absolutly needed.
Yes but before creating the generic framework, we have to do a transversal cpuidle consolidation across the SoC, factor out the code when possible, and ensure the consistency between the different platform and see a common pattern to emerge from this work.
Agreed, but if that means ignoring platform-specific customization, and not putting in other mechanisms to configure platform specific details, it is throwing away useful infrastructure.
Yes, I agree.
IMO, any consolidated framework needs some way to customize or pass in latencies from platform-specific code. Long term, I suppose this needs to be DT based.
A random thought here. What do you think if we can write the latencies from the userspace via sysfs ? At present, the 'latency' file is read-only, may be we can add the exit_latency and make them both writable. That will have the benefit of letting the userspace to tweak that depending of the board without recompiling a new kernel and also help to debug/improve without rebooting the host, no ?
That being said, I do want to see this consolidation happen, so...
In OMAP land, we are in the middle of putting in place a better framework for defining/tracking the various latencies involved in PM transitions (using per-device PM Qos.) After that, we will likely be reworking and revalidating these latency numbers for all OMAPs
So maybe the best approach to help in consolidation is just to drop the board-rx51 data all together for now, as well as the ability to pass custom C states from board files.
Good. If this is acceptable then that will simplify the consolidation.
The default, unoptimized OMAP3 numbers will work fine for that board, and anyone wanting to do optimized power work for that platform can still do it with a custom kernel. (There is still lots of out of tree work for the n900 that never made it upstream, so I doubt the mainline users of n900 will be affected by this level of power tweaking.)
If we do that, then as part of the consolidation effort, some DT-based customization should be defined as well to override/customize C states.
Ok, I will give a try by removing the rx51 specific cpuidle data. Then I will be able to do similar changes than OMAP4 for OMAP3.
Thanks Kevin
-- Daniel
-
Daniel Lezcano -
Jean Pihet -
Kevin Hilman -
Santosh Shilimkar -
Shilimkar, Santosh