[RFC 1/4] cpuidle: define the enter function in the driver structure

[RFC 1/4] cpuidle: define the enter function in the driver structure

[Daniel Lezcano]

We have the state index passed as parameter to the 'enter' function.
Most of the drivers assign their 'enter' functions several times in
the cpuidle_state structure, as we have the index, we can delegate
to the driver to handle their own callback array.

That will have the benefit of removing multiple lines of code in the
different drivers.

In order to smoothly modify the driver, the 'enter' function are in
the driver structure and in the cpuidle state structure. That will
let the time to modify the different drivers one by one.
So the 'cpuidle_enter' function checks if the 'enter' callback is
assigned in the driver structure and use it, otherwise it invokes
the 'enter' assigned to the cpuidle_state.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/cpuidle/cpuidle.c |    4 +++-
 include/linux/cpuidle.h   |    1 +
 2 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index d90519c..155dee7 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -46,7 +46,9 @@ static inline int cpuidle_enter(struct cpuidle_device *dev,
 				struct cpuidle_driver *drv, int index)
 {
 	struct cpuidle_state *target_state = &drv->states[index];
-	return target_state->enter(dev, drv, index);
+
+	return drv->enter(dev, drv, index) ? drv->enter(dev, drv, index) :
+		target_state->enter(dev, drv, index);
 }
 
 static inline int cpuidle_enter_tk(struct cpuidle_device *dev,
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 6c26a3d..d82e169 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -130,6 +130,7 @@ struct cpuidle_driver {
 	struct cpuidle_state	states[CPUIDLE_STATE_MAX];
 	int			state_count;
 	int			safe_state_index;
+	int (*enter)(struct cpuidle_device *, struct cpuidle_driver *, int);
 };
 
 #ifdef CONFIG_CPU_IDLE
-- 
1.7.5.4

[RFC 2/4] cpuidle: move enter_dead to the driver structure

[Daniel Lezcano]

The 'enter_dead' function is only used for processor_idle.c
and the same function is used several times. We fall into the
same abuse with the multiple callbacks for the same function.

This patch fixes that by moving the 'enter_dead' function to the
driver structure. A flag CPUIDLE_FLAG_DEAD_VALID has been added
to handle the callback conditional invokation.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/acpi/processor_idle.c |    7 +++----
 drivers/cpuidle/cpuidle.c     |    4 ++--
 include/linux/cpuidle.h       |   25 +++++++++++++++++--------
 3 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index f3decb3..5590ab4 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -996,6 +996,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
 struct cpuidle_driver acpi_idle_driver = {
 	.name =		"acpi_idle",
 	.owner =	THIS_MODULE,
+	.enter_dead = acpi_idle_play_dead,
 };
 
 /**
@@ -1104,16 +1105,14 @@ static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
 			case ACPI_STATE_C1:
 			if (cx->entry_method == ACPI_CSTATE_FFH)
 				state->flags |= CPUIDLE_FLAG_TIME_VALID;
-
+			state->flags |= CPUIDLE_FLAG_DEAD_VALID;
 			state->enter = acpi_idle_enter_c1;
-			state->enter_dead = acpi_idle_play_dead;
 			drv->safe_state_index = count;
 			break;
 
 			case ACPI_STATE_C2:
-			state->flags |= CPUIDLE_FLAG_TIME_VALID;
+			state->flags |= CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_DEAD_VALID;
 			state->enter = acpi_idle_enter_simple;
-			state->enter_dead = acpi_idle_play_dead;
 			drv->safe_state_index = count;
 			break;
 
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 155dee7..68bf115 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -81,14 +81,14 @@ int cpuidle_play_dead(void)
 	for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) {
 		struct cpuidle_state *s = &drv->states[i];
 
-		if (s->power_usage < power_usage && s->enter_dead) {
+		if (s->power_usage < power_usage && (s->flags & CPUIDLE_FLAG_DEAD_VALID)) {
 			power_usage = s->power_usage;
 			dead_state = i;
 		}
 	}
 
 	if (dead_state != -1)
-		return drv->states[dead_state].enter_dead(dev, dead_state);
+		return drv->enter_dead(dev, dead_state);
 
 	return -ENODEV;
 }
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index d82e169..f5915e9 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -49,16 +49,24 @@ struct cpuidle_state {
 	unsigned int    disable;
 
 	int (*enter)	(struct cpuidle_device *dev,
-			struct cpuidle_driver *drv,
-			int index);
-
-	int (*enter_dead) (struct cpuidle_device *dev, int index);
+			 struct cpuidle_driver *drv,
+			 int index);
 };
 
-/* Idle State Flags */
-#define CPUIDLE_FLAG_TIME_VALID	(0x01) /* is residency time measurable? */
-
-#define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000)
+/********************************************************************************
+ * Idle State Flags:                                                            *
+ * CPUIDLE_FLAG_TIME_VALID  : the drivers could measure the state residency     *
+ *                            time and store it in the 'target_residency'field. *
+ *                            This flag will tell the cpuidle core to use this  *
+ *                            value to compute an accurate prediction.          *
+ *                                                                              *
+ * CPUIDLE_FLAG_DEAD_VALID  : the driver may want to deal with dead cpus, tell  *
+ *                            the cpuidle core the specified state can use the  *
+ *                            enter_dead function.                              *
+ *                                                                              *
+ *******************************************************************************/
+#define CPUIDLE_FLAG_TIME_VALID  0x01
+#define CPUIDLE_FLAG_DEAD_VALID  0x02
 
 /**
  * cpuidle_get_statedata - retrieves private driver state data
@@ -131,6 +139,7 @@ struct cpuidle_driver {
 	int			state_count;
 	int			safe_state_index;
 	int (*enter)(struct cpuidle_device *, struct cpuidle_driver *, int);
+	int (*enter_dead)(struct cpuidle_device *, int);
 };
 
 #ifdef CONFIG_CPU_IDLE
-- 
1.7.5.4

[RFC 3/4] cpuidle : move tlb flag to the cpuidle header

[Daniel Lezcano]

Move this specific flag to the header file.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/idle/intel_idle.c |    8 --------
 include/linux/cpuidle.h   |    7 +++++++
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index d0f59c3..3456fc9 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -100,14 +100,6 @@ static int intel_idle(struct cpuidle_device *dev,
 static struct cpuidle_state *cpuidle_state_table;
 
 /*
- * Set this flag for states where the HW flushes the TLB for us
- * and so we don't need cross-calls to keep it consistent.
- * If this flag is set, SW flushes the TLB, so even if the
- * HW doesn't do the flushing, this flag is safe to use.
- */
-#define CPUIDLE_FLAG_TLB_FLUSHED	0x10000
-
-/*
  * States are indexed by the cstate number,
  * which is also the index into the MWAIT hint array.
  * Thus C0 is a dummy.
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index f5915e9..aafacd4 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -64,9 +64,16 @@ struct cpuidle_state {
  *                            the cpuidle core the specified state can use the  *
  *                            enter_dead function.                              *
  *                                                                              *
+ * CPUIDLE_FLAG_TLB_FLUSHED : Set this flag for states where the HW flushes the *
+ *                            TLB for us and so we don't need cross-calls to    *
+ *                            keep it consistent. If this flag is set, SW       *
+ *                            flushes the TLB, so even if the HW doesn't do the *
+ *                            flushing, this flag is safe to use.               *
+ *                                                                              *
  *******************************************************************************/
 #define CPUIDLE_FLAG_TIME_VALID  0x01
 #define CPUIDLE_FLAG_DEAD_VALID  0x02
+#define CPUIDLE_FLAG_TLB_FLUSHED 0x04
 
 /**
  * cpuidle_get_statedata - retrieves private driver state data
-- 
1.7.5.4

[RFC 4/4] cpuidle: replace the 'disable' field by a flag

[Daniel Lezcano]

We have a flag field for each cpuidle state but we don't use it
for the 'disabled' states. Let's remove the integer field and use
the flag field.

Signed-off-by: Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/cpuidle/cpuidle.c        |    1 -
 drivers/cpuidle/governors/menu.c |    5 +++--
 drivers/cpuidle/sysfs.c          |    4 ++--
 include/linux/cpuidle.h          |    4 +++-
 4 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 68bf115..3068b65 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -267,7 +267,6 @@ static void poll_idle_init(struct cpuidle_driver *drv)
 	state->power_usage = -1;
 	state->flags = 0;
 	state->enter = poll_idle;
-	state->disable = 0;
 }
 #else
 static void poll_idle_init(struct cpuidle_driver *drv) {}
diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
index 0633575..c1fe87b 100644
--- a/drivers/cpuidle/governors/menu.c
+++ b/drivers/cpuidle/governors/menu.c
@@ -281,7 +281,8 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 	 * unless the timer is happening really really soon.
 	 */
 	if (data->expected_us > 5 &&
-		drv->states[CPUIDLE_DRIVER_STATE_START].disable == 0)
+	    !(drv->states[CPUIDLE_DRIVER_STATE_START].flags &
+	      CPUIDLE_FLAG_DISABLED))
 		data->last_state_idx = CPUIDLE_DRIVER_STATE_START;
 
 	/*
@@ -291,7 +292,7 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 	for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) {
 		struct cpuidle_state *s = &drv->states[i];
 
-		if (s->disable)
+		if (s->flags & CPUIDLE_FLAG_DISABLED)
 			continue;
 		if (s->target_residency > data->predicted_us)
 			continue;
diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
index 88032b4..a9a5f05 100644
--- a/drivers/cpuidle/sysfs.c
+++ b/drivers/cpuidle/sysfs.c
@@ -245,9 +245,9 @@ static ssize_t store_state_##_name(struct cpuidle_state *state, \
 	if (err) \
 		return err; \
 	if (value) \
-		state->disable = 1; \
+		state->flags |= CPUIDLE_FLAG_DISABLED; \
 	else \
-		state->disable = 0; \
+		state->flags &= ~CPUIDLE_FLAG_DISABLED; \
 	return size; \
 }
 
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index aafacd4..65f4c52 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -46,7 +46,6 @@ struct cpuidle_state {
 	unsigned int	exit_latency; /* in US */
 	int		power_usage; /* in mW */
 	unsigned int	target_residency; /* in US */
-	unsigned int    disable;
 
 	int (*enter)	(struct cpuidle_device *dev,
 			 struct cpuidle_driver *drv,
@@ -70,10 +69,13 @@ struct cpuidle_state {
  *                            flushes the TLB, so even if the HW doesn't do the *
  *                            flushing, this flag is safe to use.               *
  *                                                                              *
+ * CPUIDLE_FLAG_DISABLE     : the state is disabled if this flag is set         *
+ *                                                                              *
  *******************************************************************************/
 #define CPUIDLE_FLAG_TIME_VALID  0x01
 #define CPUIDLE_FLAG_DEAD_VALID  0x02
 #define CPUIDLE_FLAG_TLB_FLUSHED 0x04
+#define CPUIDLE_FLAG_DISABLED    0x08
 
 /**
  * cpuidle_get_statedata - retrieves private driver state data
-- 
1.7.5.4

Re: [linux-pm] [RFC 1/4] cpuidle: define the enter function in the driver structure

[Deepthi Dharwar]

Hi Daniel,

On 06/08/2012 09:32 PM, Daniel Lezcano wrote:

> We have the state index passed as parameter to the 'enter' function.
> Most of the drivers assign their 'enter' functions several times in
> the cpuidle_state structure, as we have the index, we can delegate
> to the driver to handle their own callback array.
> 
> That will have the benefit of removing multiple lines of code in the
> different drivers.
> 
> In order to smoothly modify the driver, the 'enter' function are in
> the driver structure and in the cpuidle state structure. That will
> let the time to modify the different drivers one by one.
> So the 'cpuidle_enter' function checks if the 'enter' callback is
> assigned in the driver structure and use it, otherwise it invokes
> the 'enter' assigned to the cpuidle_state.


Currently, the backend driver initializes
all the cpuidle states supported on the platform,
and each state can have its own enter routine
which can be unique  This is a clean approach.

By moving the enter routine into the driver,
we are enforcing in having only one enter state.
There is unnecessary overhead involved
in calling a wrapper routine just to
index into the right idle state routine
for many platforms at runtime.


> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/cpuidle/cpuidle.c |    4 +++-
>  include/linux/cpuidle.h   |    1 +
>  2 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index d90519c..155dee7 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -46,7 +46,9 @@ static inline int cpuidle_enter(struct cpuidle_device *dev,
>  				struct cpuidle_driver *drv, int index)
>  {
>  	struct cpuidle_state *target_state = &drv->states[index];
> -	return target_state->enter(dev, drv, index);
> +
> +	return drv->enter(dev, drv, index) ? drv->enter(dev, drv, index) :
> +		target_state->enter(dev, drv, index);
>  }
> 
>  static inline int cpuidle_enter_tk(struct cpuidle_device *dev,
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index 6c26a3d..d82e169 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -130,6 +130,7 @@ struct cpuidle_driver {
>  	struct cpuidle_state	states[CPUIDLE_STATE_MAX];
>  	int			state_count;
>  	int			safe_state_index;
> +	int (*enter)(struct cpuidle_device *, struct cpuidle_driver *, int);
>  };
> 
>  #ifdef CONFIG_CPU_IDLE


Cheers,
Deepthi

Re: [RFC 1/4] cpuidle: define the enter function in the driver structure

[Daniel Lezcano]

On 06/08/2012 07:33 PM, Deepthi Dharwar wrote:
> Hi Daniel,

Hi Deepthi,

> On 06/08/2012 09:32 PM, Daniel Lezcano wrote:
> 
>> We have the state index passed as parameter to the 'enter' function.
>> Most of the drivers assign their 'enter' functions several times in
>> the cpuidle_state structure, as we have the index, we can delegate
>> to the driver to handle their own callback array.
>>
>> That will have the benefit of removing multiple lines of code in the
>> different drivers.
>>
>> In order to smoothly modify the driver, the 'enter' function are in
>> the driver structure and in the cpuidle state structure. That will
>> let the time to modify the different drivers one by one.
>> So the 'cpuidle_enter' function checks if the 'enter' callback is
>> assigned in the driver structure and use it, otherwise it invokes
>> the 'enter' assigned to the cpuidle_state.
> 
> 
> Currently, the backend driver initializes
> all the cpuidle states supported on the platform,
> and each state can have its own enter routine
> which can be unique This is a clean approach.

Yes, I perfectly understood the purpose of this field but as clean it is
it does not make sense as it is not used in this way. If it is supposed
to be done in the way you are describing here, we should have the same
number of states and enter functions. Here it is how it is used:

 --------------------------------------------------
| Arch             | nr states | nr enter function |
 --------------------------------------------------
| x86 (nehalem)    |    3      |         1         |
 --------------------------------------------------
| x86 (snb)        |    4      |         1         |
 --------------------------------------------------
| x86 (atom)       |    4      |         1         |
 --------------------------------------------------
| ARM tegra        |    1      |         1         |
 --------------------------------------------------
| ARM omap3        |    7      |         2         |
 --------------------------------------------------
| ARM omap4        |    3      |         1         |
 --------------------------------------------------
| ARM ux500        |    2      |         1         |
 --------------------------------------------------
| ARM shmobile     |    1      |         1         |
 --------------------------------------------------
| ARM davinci      |    2      |         1         |
 --------------------------------------------------
| ARM at91         |    2      |         1         |
 --------------------------------------------------
| ARM s3c64xx      |    1      |         1         |
 --------------------------------------------------
| ARM exynos       |    2      |         1         |
 --------------------------------------------------
| ARM kirkwood     |    2      |         1         |
 --------------------------------------------------
| SH               |    3      |         1         |
 --------------------------------------------------
| PPC              |    2      |         2         |
 --------------------------------------------------
|                  |           |                   |
| TOTAL            |    39     |        17         |
|                  |           |                   |
 --------------------------------------------------


As you can see most of the enter functions are only used as one.
The Omap3 cpuidle driver enter function for C2 calls the enter function
of C1. Other arch, already use a table of callbacks or the index.

> By moving the enter routine into the driver,
> we are enforcing in having only one enter state.
> There is unnecessary overhead involved
> in calling a wrapper routine just to
> index into the right idle state routine
> for many platforms at runtime.

I don't agree. For the sake of encapsulated code, we duplicate n-times a
field and that is not used in this way. It is quite easy to have in the
driver specific code a common enter function to ventilate to the right
routine without adding extra overhead and let the common code use a
single enter routine (which is already the case today).

Re: [linux-pm] [RFC 1/4] cpuidle: define the enter function in the driver structure

[Jean Pihet]

Hi Daniel,

On Fri, Jun 8, 2012 at 11:34 PM, Daniel Lezcano
<daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On 06/08/2012 07:33 PM, Deepthi Dharwar wrote:
>> Hi Daniel,
>
> Hi Deepthi,
>
>> On 06/08/2012 09:32 PM, Daniel Lezcano wrote:
>>
>>> We have the state index passed as parameter to the 'enter' function.
>>> Most of the drivers assign their 'enter' functions several times in
>>> the cpuidle_state structure, as we have the index, we can delegate
>>> to the driver to handle their own callback array.
>>>
>>> That will have the benefit of removing multiple lines of code in the
>>> different drivers.
>>>
>>> In order to smoothly modify the driver, the 'enter' function are in
>>> the driver structure and in the cpuidle state structure. That will
>>> let the time to modify the different drivers one by one.
>>> So the 'cpuidle_enter' function checks if the 'enter' callback is
>>> assigned in the driver structure and use it, otherwise it invokes
>>> the 'enter' assigned to the cpuidle_state.
>>
>>
>> Currently, the backend driver initializes
>> all the cpuidle states supported on the platform,
>> and each state can have its own enter routine
>> which can be unique This is a clean approach.
>
> Yes, I perfectly understood the purpose of this field but as clean it is
> it does not make sense as it is not used in this way. If it is supposed
> to be done in the way you are describing here, we should have the same
> number of states and enter functions. Here it is how it is used:
>
>  --------------------------------------------------
> | Arch             | nr states | nr enter function |
>  --------------------------------------------------
> | x86 (nehalem)    |    3      |         1         |
>  --------------------------------------------------
> | x86 (snb)        |    4      |         1         |
>  --------------------------------------------------
> | x86 (atom)       |    4      |         1         |
>  --------------------------------------------------
> | ARM tegra        |    1      |         1         |
>  --------------------------------------------------
> | ARM omap3        |    7      |         2         |
>  --------------------------------------------------
> | ARM omap4        |    3      |         1         |
>  --------------------------------------------------
> | ARM ux500        |    2      |         1         |
>  --------------------------------------------------
> | ARM shmobile     |    1      |         1         |
>  --------------------------------------------------
> | ARM davinci      |    2      |         1         |
>  --------------------------------------------------
> | ARM at91         |    2      |         1         |
>  --------------------------------------------------
> | ARM s3c64xx      |    1      |         1         |
>  --------------------------------------------------
> | ARM exynos       |    2      |         1         |
>  --------------------------------------------------
> | ARM kirkwood     |    2      |         1         |
>  --------------------------------------------------
> | SH               |    3      |         1         |
>  --------------------------------------------------
> | PPC              |    2      |         2         |
>  --------------------------------------------------
> |                  |           |                   |
> | TOTAL            |    39     |        17         |
> |                  |           |                   |
>  --------------------------------------------------
>
>
> As you can see most of the enter functions are only used as one.
> The Omap3 cpuidle driver enter function for C2 calls the enter function
> of C1. Other arch, already use a table of callbacks or the index.
There is a plan to remove the extra enter function as part of an
optimization, cf. [1]. The fix is planned to reach the 3.6 mainline
kernel via Kevin's tree [2].

[1] http://marc.info/?l=linux-omap&m=133856365818099&w=2
[2] http://git.kernel.org/?p=linux/kernel/git/khilman/linux-omap-pm.git;a=shortlog;h=refs/heads/for_3.6/pm/performance

The result is that there will be only one enter function for OMAP3.

Regards,
Jean

>> By moving the enter routine into the driver,
>> we are enforcing in having only one enter state.
>> There is unnecessary overhead involved
>> in calling a wrapper routine just to
>> index into the right idle state routine
>> for many platforms at runtime.
>
> I don't agree. For the sake of encapsulated code, we duplicate n-times a
> field and that is not used in this way. It is quite easy to have in the
> driver specific code a common enter function to ventilate to the right
> routine without adding extra overhead and let the common code use a
> single enter routine (which is already the case today).
>
>
> _______________________________________________
> linux-pm mailing list
> linux-pm-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linuxfoundation.org/mailman/listinfo/linux-pm

Re: [RFC 1/4] cpuidle: define the enter function in the driver structure

[Daniel Lezcano]

On 06/13/2012 02:44 PM, Jean Pihet wrote:
> Hi Daniel,
> 
> On Fri, Jun 8, 2012 at 11:34 PM, Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>> On 06/08/2012 07:33 PM, Deepthi Dharwar wrote:
>>> Hi Daniel,
>>
>> Hi Deepthi,
>>
>>> On 06/08/2012 09:32 PM, Daniel Lezcano wrote:
>>>
>>>> We have the state index passed as parameter to the 'enter' function.
>>>> Most of the drivers assign their 'enter' functions several times in
>>>> the cpuidle_state structure, as we have the index, we can delegate
>>>> to the driver to handle their own callback array.
>>>>
>>>> That will have the benefit of removing multiple lines of code in the
>>>> different drivers.
>>>>
>>>> In order to smoothly modify the driver, the 'enter' function are in
>>>> the driver structure and in the cpuidle state structure. That will
>>>> let the time to modify the different drivers one by one.
>>>> So the 'cpuidle_enter' function checks if the 'enter' callback is
>>>> assigned in the driver structure and use it, otherwise it invokes
>>>> the 'enter' assigned to the cpuidle_state.
>>>
>>>
>>> Currently, the backend driver initializes
>>> all the cpuidle states supported on the platform,
>>> and each state can have its own enter routine
>>> which can be unique This is a clean approach.
>>
>> Yes, I perfectly understood the purpose of this field but as clean it is
>> it does not make sense as it is not used in this way. If it is supposed
>> to be done in the way you are describing here, we should have the same
>> number of states and enter functions. Here it is how it is used:
>>
>>  --------------------------------------------------
>> | Arch             | nr states | nr enter function |
>>  --------------------------------------------------
>> | x86 (nehalem)    |    3      |         1         |
>>  --------------------------------------------------
>> | x86 (snb)        |    4      |         1         |
>>  --------------------------------------------------
>> | x86 (atom)       |    4      |         1         |
>>  --------------------------------------------------
>> | ARM tegra        |    1      |         1         |
>>  --------------------------------------------------
>> | ARM omap3        |    7      |         2         |
>>  --------------------------------------------------
>> | ARM omap4        |    3      |         1         |
>>  --------------------------------------------------
>> | ARM ux500        |    2      |         1         |
>>  --------------------------------------------------
>> | ARM shmobile     |    1      |         1         |
>>  --------------------------------------------------
>> | ARM davinci      |    2      |         1         |
>>  --------------------------------------------------
>> | ARM at91         |    2      |         1         |
>>  --------------------------------------------------
>> | ARM s3c64xx      |    1      |         1         |
>>  --------------------------------------------------
>> | ARM exynos       |    2      |         1         |
>>  --------------------------------------------------
>> | ARM kirkwood     |    2      |         1         |
>>  --------------------------------------------------
>> | SH               |    3      |         1         |
>>  --------------------------------------------------
>> | PPC              |    2      |         2         |
>>  --------------------------------------------------
>> |                  |           |                   |
>> | TOTAL            |    39     |        17         |
>> |                  |           |                   |
>>  --------------------------------------------------
>>
>>
>> As you can see most of the enter functions are only used as one.
>> The Omap3 cpuidle driver enter function for C2 calls the enter function
>> of C1. Other arch, already use a table of callbacks or the index.
> There is a plan to remove the extra enter function as part of an
> optimization, cf. [1]. The fix is planned to reach the 3.6 mainline
> kernel via Kevin's tree [2].
> 
> [1] http://marc.info/?l=linux-omap&m=133856365818099&w=2
> [2] http://git.kernel.org/?p=linux/kernel/git/khilman/linux-omap-pm.git;a=shortlog;h=refs/heads/for_3.6/pm/performance
> 
> The result is that there will be only one enter function for OMAP3.

Another argument in favor of moving this field to the driver structure.

Thanks Jean for the information.

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

_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-pm

Re: [RFC 1/4] cpuidle: define the enter function in the driver structure

[Namhyung Kim]

Hi,

On Fri,  8 Jun 2012 18:02:42 +0200, Daniel Lezcano wrote:
> We have the state index passed as parameter to the 'enter' function.
> Most of the drivers assign their 'enter' functions several times in
> the cpuidle_state structure, as we have the index, we can delegate
> to the driver to handle their own callback array.
>
> That will have the benefit of removing multiple lines of code in the
> different drivers.
>
> In order to smoothly modify the driver, the 'enter' function are in
> the driver structure and in the cpuidle state structure. That will
> let the time to modify the different drivers one by one.
> So the 'cpuidle_enter' function checks if the 'enter' callback is
> assigned in the driver structure and use it, otherwise it invokes
> the 'enter' assigned to the cpuidle_state.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/cpuidle/cpuidle.c |    4 +++-
>  include/linux/cpuidle.h   |    1 +
>  2 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index d90519c..155dee7 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -46,7 +46,9 @@ static inline int cpuidle_enter(struct cpuidle_device *dev,
>  				struct cpuidle_driver *drv, int index)
>  {
>  	struct cpuidle_state *target_state = &drv->states[index];
> -	return target_state->enter(dev, drv, index);
> +
> +	return drv->enter(dev, drv, index) ? drv->enter(dev, drv, index) :

Do you mean:
	       drv->enter ? drv->enter(dev, drv, index) :
?

Thanks,
Namhyung


> +		target_state->enter(dev, drv, index);
>  }
>  
>  static inline int cpuidle_enter_tk(struct cpuidle_device *dev,
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index 6c26a3d..d82e169 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -130,6 +130,7 @@ struct cpuidle_driver {
>  	struct cpuidle_state	states[CPUIDLE_STATE_MAX];
>  	int			state_count;
>  	int			safe_state_index;
> +	int (*enter)(struct cpuidle_device *, struct cpuidle_driver *, int);
>  };
>  
>  #ifdef CONFIG_CPU_IDLE

Re: [RFC 1/4] cpuidle: define the enter function in the driver structure

[Daniel Lezcano]

On 06/14/2012 09:49 AM, Namhyung Kim wrote:
> Hi,
> 
> On Fri,  8 Jun 2012 18:02:42 +0200, Daniel Lezcano wrote:
>> We have the state index passed as parameter to the 'enter' function.
>> Most of the drivers assign their 'enter' functions several times in
>> the cpuidle_state structure, as we have the index, we can delegate
>> to the driver to handle their own callback array.
>>
>> That will have the benefit of removing multiple lines of code in the
>> different drivers.
>>
>> In order to smoothly modify the driver, the 'enter' function are in
>> the driver structure and in the cpuidle state structure. That will
>> let the time to modify the different drivers one by one.
>> So the 'cpuidle_enter' function checks if the 'enter' callback is
>> assigned in the driver structure and use it, otherwise it invokes
>> the 'enter' assigned to the cpuidle_state.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>>  drivers/cpuidle/cpuidle.c |    4 +++-
>>  include/linux/cpuidle.h   |    1 +
>>  2 files changed, 4 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
>> index d90519c..155dee7 100644
>> --- a/drivers/cpuidle/cpuidle.c
>> +++ b/drivers/cpuidle/cpuidle.c
>> @@ -46,7 +46,9 @@ static inline int cpuidle_enter(struct cpuidle_device *dev,
>>  				struct cpuidle_driver *drv, int index)
>>  {
>>  	struct cpuidle_state *target_state = &drv->states[index];
>> -	return target_state->enter(dev, drv, index);
>> +
>> +	return drv->enter(dev, drv, index) ? drv->enter(dev, drv, index) :
> 
> Do you mean:
> 	       drv->enter ? drv->enter(dev, drv, index) :
> ?
> 
> Thanks,
> Namhyung

Right :)

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