OMAP4 cpuidle cleanup
Subsystems (3)
cover OMAP4 cpuidle cleanup
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(-)
Signature · 1 line
1.7.5.4
RFC PATCH 1/7 ARM: OMAP4: cpuidle - Remove unused valid field
The 'valid' field is never used in the code, let's remove it.
--- arch/arm/mach-omap2/cpuidle44xx.c | 9 +++------ 1 files changed, 3 insertions(+), 6 deletions(-)
arch/arm/mach-omap2/cpuidle44xx.c +3 -6
@@ -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 ;
Signature · 1 line
1.7.5.4
RFC PATCH 1/7 ARM: OMAP4: cpuidle - Remove unused valid field
Quoted 4 lines · Daniel Lezcano
The 'valid' field is never used in the code, let's remove it.
Signed-off-by Daniel Lezcano <[email protected]>---
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
RFC PATCH 1/7 ARM: OMAP4: cpuidle - Remove unused valid field
Quoted 9 lines · Santosh
Quoted 4 lines · Daniel Lezcano
The 'valid' field is never used in the code, let's remove it.
Signed-off-by Daniel Lezcano<[email protected]>---
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.
Signature · 5 lines
<http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
RFC PATCH 1/7 ARM: OMAP4: cpuidle - Remove unused valid field
Quoted 7 lines · Daniel Lezcano
Quoted 6 lines · Santosh
Quoted 4 lines · Daniel Lezcano
The 'valid' field is never used in the code, let's remove it.
Signed-off-by Daniel Lezcano<[email protected]>---
Quoted 8 lines
Quoted 3 lines
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.
Quoted 3 lines
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
RFC PATCH 1/7 ARM: OMAP4: cpuidle - Remove unused valid field
Quoted 16 lines · Santosh Shilimkar
Quoted 7 lines · Daniel Lezcano
Quoted 6 lines · Santosh
Quoted 4 lines · On Wed, Mar 21, 2012 at 2:57 PM, Daniel Lezcano <[email protected]>…
The 'valid' field is never used in the code, let's remove it.
Signed-off-by Daniel Lezcano<[email protected]>---
Quoted 8 lines
Quoted 3 lines
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.
Quoted 7 lines
When I said debug, I mean CPUIDLE debug and not any special debug code.
Quoted 3 lines
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.
Quoted 1 line
From the kernel space, for debug, you can set the .disable value to 1
in the cpuidle_driver->states struct.
Quoted 8 lines
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
Quoted 5 lines
_______________________________________________
linaro-dev mailing list
linaro-dev at lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
RFC PATCH 2/7 ARM: OMAP4: cpuidle - Declare the states with the driver declaration
In reply to Daniel LezcanoThe 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.
--- arch/arm/mach-omap2/cpuidle44xx.c | 56 +++++++++++++++++++++---------------- 1 files changed, 32 insertions(+), 24 deletions(-)
arch/arm/mach-omap2/cpuidle44xx.c +32 -24
@@ -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 ;
Signature · 1 line
1.7.5.4
RFC PATCH 2/7 ARM: OMAP4: cpuidle - Declare the states with the driver declaration
+ Jean,
Quoted 7 lines · Daniel Lezcano
The cpuidle API allows to declare statically the states in the driver structure. Let's use it.
We do no longer need thefill_cstatefunction called at runtime and by the way adding more instructions at boot time.Signed-off-by Daniel Lezcano <[email protected]>---
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
RFC PATCH 2/7 ARM: OMAP4: cpuidle - Declare the states with the driver declaration
In reply to Daniel LezcanoQuoted 28 lines · Daniel Lezcano
The cpuidle API allows to declare statically the states in the driver structure. Let's use it.
We do no longer need thefill_cstatefunction called at runtime and by the way adding more instructions at boot time.Signed-off-by Daniel Lezcano <[email protected]>---
?arch/arm/mach-omap2/cpuidle44xx.c | ? 56 +++++++++++++++++++++---------------- ?1 files changed, 32 insertions(+), 24 deletions(-)arch/arm/mach-omap2/cpuidle44xx.c +10
-0@@ -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" + ? ? ? ? ? ? ? },
...
Quoted 6 lines
+ ? ? ? },
+ ? ? ? .state_count =OMAP4_NUM_STATES,
?};arch/arm/mach-omap2/cpuidle44xx.c +0
-2-static inline void _fill_cstate (struct cpuidle_driver *drv, - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? int idx, const char *descr)
arch/arm/mach-omap2/cpuidle44xx.c
Quoted 10 lines
arch/arm/mach-omap2/cpuidle44xx.c +0
-10-{ - ? ? ? 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).
Quoted 6 lines
-
?static inline structomap4_idle_statedata*_fill_cstate_usage( ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?structcpuidle_device*dev, ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?int idx)arch/arm/mach-omap2/cpuidle44xx.c +0
-0@@ -196,37 +213,28 @@ int __init omap4_idle_init(void) ? ? ? ?if ((!mpu_pd) || (!cpu0_pd) || (!cpu1_pd))
arch/arm/mach-omap2/cpuidle44xx.c int __init omap4_idle_init(void) · line 206
Quoted 10 lines
arch/arm/mach-omap2/cpuidle44xx.c +0
-5? ? ? ? ? ? ? ?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
Quoted 7 lines
arch/arm/mach-omap2/cpuidle44xx.c +0
-1--1.7.5.4
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo at vger.kernel.org
More majordomo info at ?http://vger.kernel.org/majordomo-info.html
RFC PATCH 2/7 ARM: OMAP4: cpuidle - Declare the states with the driver declaration
Quoted 63 lines · Jean Pihet
Quoted 28 lines · Daniel Lezcano
The cpuidle API allows to declare statically the states in the driver structure. Let's use it.
We do no longer need thefill_cstatefunction called at runtime and by the way adding more instructions at boot time.Signed-off-by Daniel Lezcano<[email protected]>--- arch/arm/mach-omap2/cpuidle44xx.c | 56 +++++++++++++++++++++---------------- 1 files changed, 32 insertions(+), 24 deletions(-)arch/arm/mach-omap2/cpuidle44xx.c +10
-0@@ -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" + },...
Quoted 6 lines
arch/arm/mach-omap2/cpuidle44xx.c +2
-2+ }, + .state_count = OMAP4_NUM_STATES, }; -static inline void _fill_cstate (struct cpuidle_driver *drv, -int idx,const char *descr)arch/arm/mach-omap2/cpuidle44xx.c DEFINE_PER_CPU(struct cpuidle_device, omap4_idle_dev); · line 164
Quoted 10 lines
arch/arm/mach-omap2/cpuidle44xx.c +0
-10-{ -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).
Quoted 13 lines
arch/arm/mach-omap2/cpuidle44xx.c +0
-3-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;
Quoted 4 lines
arch/arm/mach-omap2/cpuidle44xx.c int __init omap4_idle_init(void) · line 206
Quoted 3 lines
arch/arm/mach-omap2/cpuidle44xx.c +0
-3-/* 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.
Quoted 12 lines
...
Thanks,
JeanQuoted 7 lines
arch/arm/mach-omap2/cpuidle44xx.c +0
-1--1.7.5.4
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 5 lines
<http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
RFC PATCH 3/7 ARM: OMAP4: cpuidle - Remove the cpuidle_params_table table
In reply to Daniel LezcanoWe do not longer need this table as we defined the values in the driver states.
--- arch/arm/mach-omap2/cpuidle44xx.c | 11 +---------- 1 files changed, 1 insertions(+), 10 deletions(-)
arch/arm/mach-omap2/cpuidle44xx.c +1 -10
@@ -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 3struct omap4_idle_statedata omap4_idle_data[OMAP4_NUM_STATES ];static struct powerdomain *mpu_pd, *cpu0_pd, *cpu1_pd;
Signature · 1 line
1.7.5.4
RFC PATCH 4/7 ARM: OMAP4: cpuidle - fix static omap4_idle_data declaration
In reply to Daniel Lezcano--- arch/arm/mach-omap2/cpuidle44xx.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
arch/arm/mach-omap2/cpuidle44xx.c +1 -1
@@ -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;/**
Signature · 1 line
1.7.5.4
RFC PATCH 4/7 ARM: OMAP4: cpuidle - fix static omap4_idle_data declaration
Quoted 10 lines · Daniel Lezcano
Signed-off-by Daniel Lezcano <[email protected]>--- arch/arm/mach-omap2/cpuidle44xx.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)arch/arm/mach-omap2/cpuidle44xx.c +0
-0
arch/arm/mach-omap2/cpuidle44xx.c struct omap4_idle_statedata { · line 36
Quoted 5 lines
arch/arm/mach-omap2/cpuidle44xx.c +1
-1@@ -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
RFC PATCH 5/7 ARM: OMAP4: cpuidle - Initialize omap4_idle_data at compile time
In reply to Daniel Lezcano--- arch/arm/mach-omap2/cpuidle44xx.c | 19 ++++++++++++++++++- 1 files changed, 18 insertions(+), 1 deletions(-)
arch/arm/mach-omap2/cpuidle44xx.c +18 -1
@@ -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;/**
Signature · 1 line
1.7.5.4
RFC PATCH 6/7 ARM: OMAP4: cpuidle - use the omap4_idle_data variable directly
In reply to Daniel LezcanoWe 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.
--- arch/arm/mach-omap2/cpuidle44xx.c | 17 +++++------------ 1 files changed, 5 insertions(+), 12 deletions(-)
arch/arm/mach-omap2/cpuidle44xx.c +5 -12
@@ -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" );
Signature · 1 line
1.7.5.4
RFC PATCH 7/7 ARM: OMAP4: cpuidle - remove omap4_idle_data initialization at boot time
In reply to Daniel LezcanoWe initialized it at compile time, no need to do that at boot time.
--- arch/arm/mach-omap2/cpuidle44xx.c | 26 +------------------------- 1 files changed, 1 insertions(+), 25 deletions(-)
arch/arm/mach-omap2/cpuidle44xx.c +1 -25
@@ -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;
Signature · 1 line
1.7.5.4
cover OMAP4 cpuidle cleanup
In reply to Daniel LezcanoQuoted 4 lines · Daniel Lezcano
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.
Quoted 1 line
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.
Quoted 2 lines
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
cover OMAP4 cpuidle cleanup
Quoted 7 lines · Santosh
Quoted 4 lines · Daniel Lezcano
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.
Quoted 4 lines
Quoted 1 line
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 ?
Quoted 3 lines
Quoted 2 lines
and why the WFI is not used ?
I didn't get this question. Do you mean the generic WFI?
yes.
Quoted 2 lines
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.
Quoted 4 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 5 lines
<http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
cover OMAP4 cpuidle cleanup
Quoted 11 lines · Daniel Lezcano
Quoted 7 lines · Santosh
Quoted 4 lines · Daniel Lezcano
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.
Quoted 13 lines
Quoted 5 lines
Quoted 2 lines
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 ?
Quoted 3 lines
Quoted 2 lines
and why the WFI is not used ?
I didn't get this question. Do you mean the generic WFI?
I execute default idle loop.
Quoted 8 lines
yes.
Quoted 2 lines
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_idleinstead 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
cover OMAP4 cpuidle cleanup
Quoted 18 lines · Santosh Shilimkar
Quoted 17 lines · Daniel Lezcano
Quoted 7 lines · Santosh
Quoted 4 lines · Daniel Lezcano
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.
Quoted 5 lines
Quoted 2 lines
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.
Quoted 8 lines
Quoted 7 lines
Ok, thanks for the information. I will look deeper. What happens to cpu1 when it is online and has nothing to do ?
Quoted 3 lines
Quoted 2 lines
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' ?
Quoted 10 lines
Quoted 8 lines
yes.
Quoted 2 lines
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_idleinstead 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
Signature · 5 lines
<http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
cover OMAP4 cpuidle cleanup
Quoted 31 lines · Daniel Lezcano
Quoted 30 lines · Santosh Shilimkar
Quoted 28 lines · Daniel Lezcano
Quoted 9 lines · Santosh
Quoted 5 lines · On Wed, Mar 21, 2012 at 2:57 PM, Daniel Lezcano <[email protected]> ?…
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.
Quoted 6 lines
Quoted 2 lines
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 ?
Quoted 2 lines
Quoted 2 lines
and why the WFI is not used ?
Quoted 9 lines
Quoted 4 lines
Quoted 3 lines
Quoted 1 line
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.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
yes.
Quoted 2 lines
If yes, then, it's mainly because OMAP need additional custom barriers.
Quoted 9 lines
Quoted 5 lines
Quoted 3 lines
Ah, ok. I am not sure if it is possible but that may be cool if we can call
cpu_do_idleinstead 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.
cover OMAP4 cpuidle cleanup
Quoted 36 lines · Santosh
Quoted 34 lines · Daniel Lezcano
Quoted 33 lines · Santosh Shilimkar
Quoted 31 lines · Daniel Lezcano
Quoted 9 lines · Santosh
Quoted 5 lines · Daniel Lezcano
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.
Quoted 6 lines
Quoted 2 lines
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 ?
Quoted 3 lines
Quoted 2 lines
and why the WFI is not used ?
I didn't get this question. Do you mean the generic WFI?
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
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.
Quoted 13 lines
Quoted 13 lines
Quoted 11 lines
Quoted 9 lines
yes.
Quoted 2 lines
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_idleinstead 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.
Quoted 7 lines
Quoted 2 lines
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
Signature · 5 lines
<http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
cover OMAP4 cpuidle cleanup
In reply to Daniel LezcanoDaniel,
Quoted 7 lines · Daniel Lezcano
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 ?
Quoted 10 lines
Daniel Lezcano (7):
ARM: OMAP4: cpuidle - Remove unused valid field
ARM: OMAP4: cpuidle - Declare the states with the driver declaration
ARM: OMAP4: cpuidle - Remove thecpuidle_params_tabletable
ARM: OMAP4: cpuidle - fix staticomap4_idle_datadeclaration
ARM: OMAP4: cpuidle - Initializeomap4_idle_dataat compile time
ARM: OMAP4: cpuidle - use theomap4_idle_datavariable directly
ARM: OMAP4: cpuidle - removeomap4_idle_datainitialization 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
cover OMAP4 cpuidle cleanup
Quoted 20 lines · Santosh Shilimkar
Daniel,
Quoted 7 lines · Daniel Lezcano
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 ?
Quoted 10 lines
Daniel Lezcano (7):
ARM: OMAP4: cpuidle - Remove unused valid field
ARM: OMAP4: cpuidle - Declare the states with the driver declaration
ARM: OMAP4: cpuidle - Remove thecpuidle_params_tabletable
ARM: OMAP4: cpuidle - fix staticomap4_idle_datadeclaration
ARM: OMAP4: cpuidle - Initializeomap4_idle_dataat compile time
ARM: OMAP4: cpuidle - use theomap4_idle_datavariable directly
ARM: OMAP4: cpuidle - removeomap4_idle_datainitialization at boot
time
Quoted 2 lines
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
Quoted 8 lines
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
Signature · 5 lines
<http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
cover OMAP4 cpuidle cleanup
In reply to Santosh ShilimkarHi Santosh, Daniel,
Quoted 20 lines · Santosh Shilimkar
Daniel,
Quoted 7 lines · Daniel Lezcano
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 ?
Quoted 10 lines
Daniel Lezcano (7):
? ARM: OMAP4: cpuidle - Remove unused valid field
? ARM: OMAP4: cpuidle - Declare the states with the driver declaration ? ARM: OMAP4: cpuidle - Remove thecpuidle_params_tabletable ? ARM: OMAP4: cpuidle - fix staticomap4_idle_datadeclaration ? ARM: OMAP4: cpuidle - Initializeomap4_idle_dataat compile time ? ARM: OMAP4: cpuidle - use theomap4_idle_datavariable directly ? ARM: OMAP4: cpuidle - removeomap4_idle_datainitialization at boot ? ? time
Quoted 6 lines
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.
Quoted 5 lines
Regards
santosh
Thanks!
Jean
cover OMAP4 cpuidle cleanup
Quoted 27 lines · Jean Pihet
Hi Santosh, Daniel,
Quoted 23 lines · Santosh Shilimkar
Daniel,
Quoted 7 lines · Daniel Lezcano
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 ?
Quoted 10 lines
Daniel Lezcano (7):
ARM: OMAP4: cpuidle - Remove unused valid field
ARM: OMAP4: cpuidle - Declare the states with the driver declaration
ARM: OMAP4: cpuidle - Remove thecpuidle_params_tabletable
ARM: OMAP4: cpuidle - fix staticomap4_idle_datadeclaration
ARM: OMAP4: cpuidle - Initializeomap4_idle_dataat compile time
ARM: OMAP4: cpuidle - use theomap4_idle_datavariable directly
ARM: OMAP4: cpuidle - removeomap4_idle_datainitialization at boot
timeThe series looks fine to me in general. This clean-up is applicable for OMAP3 cpuidle code as well.
Quoted 6 lines
Quoted 3 lines
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<[email protected]>
Thanks for the review Jean.
Quoted 5 lines
Quoted 2 lines
Thanks!
Jean
Signature · 5 lines
<http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
cover OMAP4 cpuidle cleanup
In reply to Jean PihetQuoted 27 lines · Jean Pihet
Hi Santosh, Daniel,
Quoted 23 lines · Santosh Shilimkar
Daniel,
Quoted 7 lines · Daniel Lezcano
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 ?
Quoted 10 lines
Daniel Lezcano (7):
? ARM: OMAP4: cpuidle - Remove unused valid field
? ARM: OMAP4: cpuidle - Declare the states with the driver declaration ? ARM: OMAP4: cpuidle - Remove thecpuidle_params_tabletable ? ARM: OMAP4: cpuidle - fix staticomap4_idle_datadeclaration ? ARM: OMAP4: cpuidle - Initializeomap4_idle_dataat compile time ? ARM: OMAP4: cpuidle - use theomap4_idle_datavariable directly ? ARM: OMAP4: cpuidle - removeomap4_idle_datainitialization at boot ? ? timeThe series looks fine to me in general. This clean-up is applicable for OMAP3 cpuidle code as well.
Quoted 7 lines
Quoted 3 lines
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 <[email protected]>
Thanks Jean for looking at it.
Daniel,
Feel free to add.
cover OMAP4 cpuidle cleanup
In reply to Santosh ShilimkarQuoted 20 lines · Santosh Shilimkar
Daniel,
Quoted 7 lines · Daniel Lezcano
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 ?
Quoted 10 lines
Daniel Lezcano (7):
? ARM: OMAP4: cpuidle - Remove unused valid field
? ARM: OMAP4: cpuidle - Declare the states with the driver declaration ? ARM: OMAP4: cpuidle - Remove thecpuidle_params_tabletable ? ARM: OMAP4: cpuidle - fix staticomap4_idle_datadeclaration ? ARM: OMAP4: cpuidle - Initializeomap4_idle_dataat compile time ? ARM: OMAP4: cpuidle - use theomap4_idle_datavariable directly ? ARM: OMAP4: cpuidle - removeomap4_idle_datainitialization at boot ? ? time
Quoted 2 lines
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.
Quoted 8 lines
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
cover OMAP4 cpuidle cleanup
Quoted 22 lines · Jean Pihet
Quoted 20 lines · Santosh Shilimkar
Daniel,
Quoted 17 lines · Daniel Lezcano
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 thecpuidle_params_tabletable
ARM: OMAP4: cpuidle - fix staticomap4_idle_datadeclaration
ARM: OMAP4: cpuidle - Initializeomap4_idle_dataat compile time
ARM: OMAP4: cpuidle - use theomap4_idle_datavariable directly
ARM: OMAP4: cpuidle - removeomap4_idle_datainitialization at boot
time
Quoted 8 lines
Quoted 2 lines
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 thecpuidle_paramsstruct 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
Quoted 11 lines
Quoted 8 lines
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
santoshThanks,
Jean
Signature · 5 lines
<http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
cover OMAP4 cpuidle cleanup
In reply to Jean PihetQuoted 22 lines · Jean Pihet
Quoted 20 lines · Santosh Shilimkar
Daniel,
Quoted 17 lines · Daniel Lezcano
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 thecpuidle_params_tabletable
ARM: OMAP4: cpuidle - fix staticomap4_idle_datadeclaration
ARM: OMAP4: cpuidle - Initializeomap4_idle_dataat compile time
ARM: OMAP4: cpuidle - use theomap4_idle_datavariable directly
ARM: OMAP4: cpuidle - removeomap4_idle_datainitialization at boot
time
Quoted 6 lines
Quoted 2 lines
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 ?
Quoted 14 lines
- the latency settings can be overriden by the board code, so the
cpuidle_paramsstruct is needed.Quoted 8 lines
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
santoshThanks,
Jean
Signature · 5 lines
<http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
cover OMAP4 cpuidle cleanup
Quoted 49 lines · Daniel Lezcano
Quoted 22 lines · Jean Pihet
Quoted 20 lines · Santosh Shilimkar
Daniel,
Quoted 17 lines · Daniel Lezcano
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 thecpuidle_params_tabletable
ARM: OMAP4: cpuidle - fix staticomap4_idle_datadeclaration
ARM: OMAP4: cpuidle - Initializeomap4_idle_dataat compile time
ARM: OMAP4: cpuidle - use theomap4_idle_datavariable directly
ARM: OMAP4: cpuidle - removeomap4_idle_datainitialization at boot
timeQuoted 6 lines
Quoted 2 lines
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}, };
Quoted 3 lines
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.
Quoted 4 lines
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
cover OMAP4 cpuidle cleanup
Quoted 51 lines · Kevin Hilman
Quoted 49 lines · Daniel Lezcano
Quoted 22 lines · Jean Pihet
Quoted 20 lines · Santosh Shilimkar
Daniel,
Quoted 17 lines · Daniel Lezcano
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 thecpuidle_params_tabletable
ARM: OMAP4: cpuidle - fix staticomap4_idle_datadeclaration
ARM: OMAP4: cpuidle - Initializeomap4_idle_dataat compile time
ARM: OMAP4: cpuidle - use theomap4_idle_datavariable directly
ARM: OMAP4: cpuidle - removeomap4_idle_datainitialization at boot
timeQuoted 6 lines
Quoted 2 lines
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}, };
Quoted 7 lines
Quoted 3 lines
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 ?
Quoted 8 lines
Quoted 4 lines
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.
Quoted 5 lines
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.
Signature · 5 lines
<http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
cover OMAP4 cpuidle cleanup
Quoted 56 lines · Daniel Lezcano
Quoted 55 lines · Kevin Hilman
Quoted 52 lines · Daniel Lezcano
Quoted 22 lines · Jean Pihet
Quoted 20 lines · Santosh Shilimkar
Daniel,
Quoted 17 lines · Daniel Lezcano
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 thecpuidle_params_tabletable
ARM: OMAP4: cpuidle - fix staticomap4_idle_datadeclaration
ARM: OMAP4: cpuidle - Initializeomap4_idle_dataat compile time
ARM: OMAP4: cpuidle - use theomap4_idle_datavariable directly
ARM: OMAP4: cpuidle - removeomap4_idle_datainitialization at boot
timeQuoted 6 lines
Quoted 2 lines
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 ?
Quoted 5 lines
Quoted 3 lines
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.
Quoted 21 lines
Quoted 8 lines
Quoted 4 lines
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_driverrx51_idle_driver= {
.name = "rx51_idle",.owner = THIS_MODULE, .states = { { /* What is in rx51_cpuidle_params */ } };
Quoted 2 lines
We put in there only the valid fields and we keep in a comment the table with the latency numbers.
Ah, OK. I misunderstood.
Quoted 3 lines
Assuming the valid field is for handling the table overwritting, we can then remove it. By this way, we simplify the
next_valid_statefunction.
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.
Quoted 3 lines
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?
Quoted 10 lines
Quoted 5 lines
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
cover OMAP4 cpuidle cleanup
Quoted 90 lines · Kevin Hilman
Quoted 56 lines · Daniel Lezcano
Quoted 55 lines · Kevin Hilman
Quoted 52 lines · Daniel Lezcano
Quoted 22 lines · Jean Pihet
Quoted 20 lines · Santosh Shilimkar
Daniel,
Quoted 17 lines · Daniel Lezcano
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 thecpuidle_params_tabletable
ARM: OMAP4: cpuidle - fix staticomap4_idle_datadeclaration
ARM: OMAP4: cpuidle - Initializeomap4_idle_dataat compile time
ARM: OMAP4: cpuidle - use theomap4_idle_datavariable directly
ARM: OMAP4: cpuidle - removeomap4_idle_datainitialization at boot
timeQuoted 6 lines
Quoted 2 lines
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 ?Quoted 5 lines
Quoted 3 lines
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.
Quoted 21 lines
Quoted 8 lines
Quoted 4 lines
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_driverrx51_idle_driver= {
.name = "rx51_idle",.owner = THIS_MODULE, .states = { { /* What is in rx51_cpuidle_params */ } };Quoted 2 lines
We put in there only the valid fields and we keep in a comment the table with the latency numbers.
Quoted 8 lines
Ah, OK. I misunderstood.
Quoted 3 lines
Assuming the valid field is for handling the table overwritting, we can then remove it. By this way, we simplify the
next_valid_statefunction.probably we can remove
next_valid_stateall 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.
Quoted 7 lines
Quoted 3 lines
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.
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
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.
Quoted 8 lines
Quoted 4 lines
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.
Quoted 3 lines
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 ?
Quoted 10 lines
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.
Quoted 8 lines
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
Signature · 5 lines
<http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog