[RFC][PATCH 0/7] OMAP4 cpuidle cleanup

[RFC][PATCH 0/7] OMAP4 cpuidle cleanup

[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 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(-)

-- 
1.7.5.4

[RFC][PATCH 1/7] ARM: OMAP4: cpuidle - Remove unused valid field

[Daniel Lezcano]

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;
-- 
1.7.5.4

[RFC][PATCH 1/7] ARM: OMAP4: cpuidle - Remove unused valid field

[Shilimkar, Santosh]

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

[RFC][PATCH 1/7] ARM: OMAP4: cpuidle - Remove unused valid field

[Daniel Lezcano]

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 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.

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.

-- 
  <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

[Santosh Shilimkar]

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 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.
> 
> 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

[RFC][PATCH 1/7] ARM: OMAP4: cpuidle - Remove unused valid field

[Jean Pihet]

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 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.
>>
>> 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 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

[Daniel Lezcano]

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;
-- 
1.7.5.4

[RFC][PATCH 2/7] ARM: OMAP4: cpuidle - Declare the states with the driver declaration

[Santosh Shilimkar]

+ 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

[RFC][PATCH 2/7] ARM: OMAP4: cpuidle - Declare the states with the driver declaration

[Jean Pihet]

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 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

[Daniel Lezcano]

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 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.

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 at vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
  <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

[Daniel Lezcano]

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;
-- 
1.7.5.4

[RFC][PATCH 4/7] ARM: OMAP4: cpuidle - fix static omap4_idle_data declaration

[Daniel Lezcano]

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;
 
 /**
-- 
1.7.5.4

[RFC][PATCH 4/7] ARM: OMAP4: cpuidle - fix static omap4_idle_data declaration

[Santosh Shilimkar]

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

[RFC][PATCH 5/7] ARM: OMAP4: cpuidle - Initialize omap4_idle_data at compile time

[Daniel Lezcano]

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;
 
 /**
-- 
1.7.5.4

[RFC][PATCH 6/7] ARM: OMAP4: cpuidle - use the omap4_idle_data variable directly

[Daniel Lezcano]

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");
-- 
1.7.5.4

[RFC][PATCH 7/7] ARM: OMAP4: cpuidle - remove omap4_idle_data initialization at boot time

[Daniel Lezcano]

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;
-- 
1.7.5.4

[RFC][PATCH 0/7] OMAP4 cpuidle cleanup

[Shilimkar, Santosh]

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

[RFC][PATCH 0/7] OMAP4 cpuidle cleanup

[Daniel Lezcano]

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 at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
  <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 0/7] OMAP4 cpuidle cleanup

[Santosh Shilimkar]

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

[RFC][PATCH 0/7] OMAP4 cpuidle cleanup

[Daniel Lezcano]

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



-- 
  <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 0/7] OMAP4 cpuidle cleanup

[Shilimkar, Santosh]

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.

[RFC][PATCH 0/7] OMAP4 cpuidle cleanup

[Daniel Lezcano]

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

-- 
  <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 0/7] OMAP4 cpuidle cleanup

[Santosh Shilimkar]

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

[RFC][PATCH 0/7] OMAP4 cpuidle cleanup

[Daniel Lezcano]

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
>
>


-- 
  <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 0/7] OMAP4 cpuidle cleanup

[Jean Pihet]

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

[RFC][PATCH 0/7] OMAP4 cpuidle cleanup

[Daniel Lezcano]

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 Pihet<j-pihet@ti.com>

Thanks for the review Jean.

>>
>>
>
> Thanks!
> Jean


-- 
  <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 0/7] OMAP4 cpuidle cleanup

[Shilimkar, Santosh]

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>

[RFC][PATCH 0/7] OMAP4 cpuidle cleanup

[Jean Pihet]

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

[RFC][PATCH 0/7] OMAP4 cpuidle cleanup

[Daniel Lezcano]

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


-- 
  <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 0/7] OMAP4 cpuidle cleanup

[Daniel Lezcano]

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


-- 
  <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 0/7] OMAP4 cpuidle cleanup

[Kevin Hilman]

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

[RFC][PATCH 0/7] OMAP4 cpuidle cleanup

[Daniel Lezcano]

On 03/21/2012 10:54 PM, Kevin Hilman wrote:
> 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.

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.

-- 
  <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 0/7] OMAP4 cpuidle cleanup

[Kevin Hilman]

Daniel Lezcano <daniel.lezcano@linaro.org> writes:

> On 03/21/2012 10:54 PM, Kevin Hilman wrote:
>> 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.
>
> 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

[RFC][PATCH 0/7] OMAP4 cpuidle cleanup

[Daniel Lezcano]

On 03/22/2012 07:36 PM, Kevin Hilman wrote:
> Daniel Lezcano<daniel.lezcano@linaro.org>  writes:
>
>> On 03/21/2012 10:54 PM, Kevin Hilman wrote:
>>> 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.
>>
>> 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


-- 
  <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