[RFC PATCH v2 0/1] ARM: cpuidle: heterogeneous systems extension

[RFC PATCH v2 0/1] ARM: cpuidle: heterogeneous systems extension

[Lorenzo Pieralisi]

This patch is a v2 of a previous posting:

http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/328192.html

v1 -> v2:

- Rebased against 4.0
- Removed DT affinity probe interface
- Improved and optimized idle states comparison and cpumask creation
- Merged DT idle states vetting code in the ARM CPUidle driver

This patch aims at enabling the generic CPUidle driver on ARM systems
with heterogeneous idle states. In the CPUidle context the kernel
considers cpus equal (and group them together through a CPUidle driver)
if they share the same set of idle states.
Since the idle states represent the differentiation point when it comes
to creating CPUidle drivers (their states and related cpu logical mask),
the idle states can be used as a means to create logical masks of cpus
for which the CPUidle driver applies.

This patch sets the maximum number of static CPUidle drivers allowed to
two, since it is hard to foresee systems with more than two sets of CPUs
having different idle states; it is meant to keep allocation simple
while keeping functionality.

The code carrying out idle states comparison is completely generic
and borrowed from OF unit tests; it overrides the OF API entirely
though to carry out properties (ie phandles list) comparison in the
fastest possible way. If deemed worthwhile it can be moved to generic OF
layer.

Tested through several dts idle states permutations on Juno board with
CPUidle and related idle states enabled.

Cc: Howard Chen <howard.chen-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Kevin Hilman <khilman-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
Cc: Lina Iyer <lina.iyer-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Mathieu Poirier <mathieu.poirier-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>

Lorenzo Pieralisi (1):
  drivers: cpuidle: cpuidle-arm: heterogeneous systems extension

 drivers/cpuidle/Kconfig.arm   |   1 +
 drivers/cpuidle/cpuidle-arm.c | 176 ++++++++++++++++++++++++++++++++++++------
 2 files changed, 152 insertions(+), 25 deletions(-)

-- 
2.2.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH v2 1/1] drivers: cpuidle: cpuidle-arm: heterogeneous systems extension

[Lorenzo Pieralisi]

Some ARM systems (eg big.LITTLE ones) can be composed of CPUs having
different hardware power management configurations and in the context
of CPUidle, different idle states. The generic ARM CPUidle driver
treats all possible CPUs as equal and initializes a common idle driver
through DT idle states for all possible CPUs.

Current driver cannot manage systems where CPUs are heterogeneous
and therefore can have different idle states.

This patch augments the generic ARM CPUidle driver, by adding code that
at boot initializes CPUidle drivers by going through the
cpu_possible_mask and through DT parsing detects the cpus sharing the
same idle states, thus creating the CPUidle drivers cpumasks.

The drivers are then initialized through the DT idle states interface,
that parses and initializes the DT idle states for the cpus set in the
drivers cpumasks.

This patch instantiates a static array of idle drivers, some of which
can turn out to be unused (eg platforms with uniform idle states
on all possible CPUs), and relies on the config option
CPU_IDLE_MULTIPLE_DRIVERS to be selected by default; this can cause a
little memory overhead, but at the same time allows supporting most of
the current and future ARM platforms through a single generic CPUidle
driver.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Howard Chen <howard.chen@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Kevin Hilman <khilman@linaro.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Lina Iyer <lina.iyer@linaro.org>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Mark Rutland <mark.rutland@arm.com>
---
 drivers/cpuidle/Kconfig.arm   |   1 +
 drivers/cpuidle/cpuidle-arm.c | 176 ++++++++++++++++++++++++++++++++++++------
 2 files changed, 152 insertions(+), 25 deletions(-)

diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
index 21340e0..90c6553 100644
--- a/drivers/cpuidle/Kconfig.arm
+++ b/drivers/cpuidle/Kconfig.arm
@@ -3,6 +3,7 @@
 #
 config ARM_CPUIDLE
         bool "Generic ARM/ARM64 CPU idle Driver"
+        select CPU_IDLE_MULTIPLE_DRIVERS
         select DT_IDLE_STATES
         help
           Select this to enable generic cpuidle driver for ARM.
diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
index 545069d..251fa2a 100644
--- a/drivers/cpuidle/cpuidle-arm.c
+++ b/drivers/cpuidle/cpuidle-arm.c
@@ -14,6 +14,7 @@
 #include <linux/cpuidle.h>
 #include <linux/cpumask.h>
 #include <linux/cpu_pm.h>
+#include <linux/of_device.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of.h>
@@ -58,23 +59,27 @@ static int arm_enter_idle_state(struct cpuidle_device *dev,
 	return ret ? -1 : idx;
 }
 
-static struct cpuidle_driver arm_idle_driver = {
-	.name = "arm_idle",
-	.owner = THIS_MODULE,
-	/*
-	 * State at index 0 is standby wfi and considered standard
-	 * on all ARM platforms. If in some platforms simple wfi
-	 * can't be used as "state 0", DT bindings must be implemented
-	 * to work around this issue and allow installing a special
-	 * handler for idle state index 0.
-	 */
-	.states[0] = {
-		.enter                  = arm_enter_idle_state,
-		.exit_latency           = 1,
-		.target_residency       = 1,
-		.power_usage		= UINT_MAX,
-		.name                   = "WFI",
-		.desc                   = "ARM WFI",
+#define ARM_CPUIDLE_MAX_DRIVERS	2
+
+static struct cpuidle_driver arm_idle_drivers[ARM_CPUIDLE_MAX_DRIVERS] = {
+	[0 ... ARM_CPUIDLE_MAX_DRIVERS - 1] = {
+		.name = "arm_idle",
+		.owner = THIS_MODULE,
+		/*
+		 * State at index 0 is standby wfi and considered standard
+		 * on all ARM platforms. If in some platforms simple wfi
+		 * can't be used as "state 0", DT bindings must be implemented
+		 * to work around this issue and allow installing a special
+		 * handler for idle state index 0.
+		 */
+		.states[0] = {
+			.enter                  = arm_enter_idle_state,
+			.exit_latency           = 1,
+			.target_residency       = 1,
+			.power_usage		= UINT_MAX,
+			.name                   = "WFI",
+			.desc                   = "ARM WFI",
+		}
 	}
 };
 
@@ -85,17 +90,68 @@ static const struct of_device_id arm_idle_state_match[] __initconst = {
 };
 
 /*
- * arm_idle_init
+ * Compare idle states phandle properties
  *
- * Registers the arm specific cpuidle driver with the cpuidle
- * framework. It relies on core code to parse the idle states
- * and initialize them using driver data structures accordingly.
+ * Return true if properties are valid and equal, false otherwise
  */
-static int __init arm_idle_init(void)
+static bool __init idle_states_cmp(struct property *states1,
+				   struct property *states2)
+{
+	/*
+	 * NB: Implemented through code from drivers/of/unittest.c
+	 *     Function is generic and can be moved to generic OF code
+	 *     if needed
+	 */
+	return states1 && states2 &&
+	       (states1->length == states2->length) &&
+	       states1->value && states2->value &&
+	       !memcmp(states1->value, states2->value, states1->length);
+}
+
+static int __init arm_idle_init_driver(struct cpuidle_driver *drv)
 {
-	int cpu, ret;
-	struct cpuidle_driver *drv = &arm_idle_driver;
+	int ret, cpu;
 	struct cpuidle_device *dev;
+	struct property *curr_idle_states, *idle_states = NULL;
+	struct device_node *cpu_node;
+
+	for_each_cpu(cpu, drv->cpumask) {
+		cpu_node = of_cpu_device_node_get(cpu);
+		curr_idle_states = of_find_property(cpu_node,
+						    "cpu-idle-states", NULL);
+		of_node_put(cpu_node);
+
+		/*
+		 * Stash the first valid idle states phandle in the cpumask.
+		 * If curr_idle_states is NULL assigning it to idle_states
+		 * is harmless and it is managed by idle states comparison
+		 * code. Keep track of first valid phandle so that
+		 * subsequent cpus can compare against it.
+		 */
+		if (!idle_states)
+			idle_states = curr_idle_states;
+
+		/*
+		 * If idle states phandles are not equal, remove the
+		 * cpu from the driver mask since a CPUidle driver
+		 * is only capable of managing uniform idle states.
+		 *
+		 * Comparison works also when idle_states and
+		 * curr_idle_states are the same property, since
+		 * they can be == NULL so the cpu must be removed from
+		 * the driver mask in that case too (ie cpu has no idle
+		 * states).
+		 */
+		if (!idle_states_cmp(idle_states, curr_idle_states))
+			cpumask_clear_cpu(cpu, drv->cpumask);
+	}
+
+	/*
+	 *  If there are no valid states for this driver we rely on arch
+	 *  default idle behaviour, bail out
+	 */
+	if (!idle_states)
+		return -ENODEV;
 
 	/*
 	 * Initialize idle states data, starting at index 1.
@@ -117,7 +173,7 @@ static int __init arm_idle_init(void)
 	 * Call arch CPU operations in order to initialize
 	 * idle states suspend back-end specific data
 	 */
-	for_each_possible_cpu(cpu) {
+	for_each_cpu(cpu, drv->cpumask) {
 		ret = arm_cpuidle_init(cpu);
 
 		/*
@@ -157,7 +213,77 @@ out_fail:
 	}
 
 	cpuidle_unregister_driver(drv);
+	return ret;
+}
+
+/*
+ * arm_idle_init
+ *
+ * Registers the arm specific cpuidle driver(s) with the cpuidle
+ * framework. It relies on core code to parse the idle states
+ * and initialize them using driver data structures accordingly.
+ */
+static int __init arm_idle_init(void)
+{
+	int i, ret = -ENODEV;
+	struct cpuidle_driver *drv;
+	cpumask_var_t tmpmask;
+
+	/*
+	 * These drivers require DT idle states to be present.
+	 * If no idle states are detected there is no reason to
+	 * proceed any further hence we return early.
+	 */
+	if (!of_find_node_by_name(NULL, "idle-states"))
+		return -ENODEV;
+
+	if (!alloc_cpumask_var(&tmpmask, GFP_KERNEL))
+		return -ENOMEM;
+
+	/*
+	 * We need to vet idle states to create CPUidle drivers
+	 * that share a common set of them. Create a tmp mask
+	 * that we use to keep track of initialized cpus.
+	 * Start off by initializing the mask with all possible
+	 * cpus, we clear it as we go, till either all cpus
+	 * have a CPUidle driver initialized or there are some
+	 * CPUs that have no idle states or a parsing error
+	 * occurs.
+	 */
+	cpumask_copy(tmpmask, cpu_possible_mask);
+
+	for (i = 0; !cpumask_empty(tmpmask); i++) {
+		if (i == ARM_CPUIDLE_MAX_DRIVERS) {
+			pr_warn("number of drivers exceeding static allocation\n");
+			break;
+		}
+
+		drv = &arm_idle_drivers[i];
+		drv->cpumask = kzalloc(cpumask_size(), GFP_KERNEL);
+		if (!drv->cpumask) {
+			ret = -ENOMEM;
+			break;
+		}
+		/*
+		 * Force driver mask, arm_idle_init_driver()
+		 * will tweak it by vetting idle states.
+		 */
+		cpumask_copy(drv->cpumask, tmpmask);
+
+		ret = arm_idle_init_driver(drv);
+		if (ret) {
+			kfree(drv->cpumask);
+			break;
+		}
+		/*
+		 * Remove the cpus that were part of the registered
+		 * driver from the mask of cpus to be initialized
+		 * and restart.
+		 */
+		cpumask_andnot(tmpmask, tmpmask, drv->cpumask);
+	}
 
+	free_cpumask_var(tmpmask);
 	return ret;
 }
 device_initcall(arm_idle_init);
-- 
2.2.1

Re: [RFC PATCH v2 1/1] drivers: cpuidle: cpuidle-arm: heterogeneous systems extension

[Daniel Lezcano]

On 04/16/2015 06:10 PM, Lorenzo Pieralisi wrote:
> Some ARM systems (eg big.LITTLE ones) can be composed of CPUs having
> different hardware power management configurations and in the context
> of CPUidle, different idle states. The generic ARM CPUidle driver
> treats all possible CPUs as equal and initializes a common idle driver
> through DT idle states for all possible CPUs.
>
> Current driver cannot manage systems where CPUs are heterogeneous
> and therefore can have different idle states.
>
> This patch augments the generic ARM CPUidle driver, by adding code that
> at boot initializes CPUidle drivers by going through the
> cpu_possible_mask and through DT parsing detects the cpus sharing the
> same idle states, thus creating the CPUidle drivers cpumasks.
>
> The drivers are then initialized through the DT idle states interface,
> that parses and initializes the DT idle states for the cpus set in the
> drivers cpumasks.
>
> This patch instantiates a static array of idle drivers, some of which
> can turn out to be unused (eg platforms with uniform idle states
> on all possible CPUs), and relies on the config option
> CPU_IDLE_MULTIPLE_DRIVERS to be selected by default; this can cause a
> little memory overhead, but at the same time allows supporting most of
> the current and future ARM platforms through a single generic CPUidle
> driver.
>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Howard Chen <howard.chen@linaro.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Kevin Hilman <khilman@linaro.org>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Lina Iyer <lina.iyer@linaro.org>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> ---
>   drivers/cpuidle/Kconfig.arm   |   1 +
>   drivers/cpuidle/cpuidle-arm.c | 176 ++++++++++++++++++++++++++++++++++++------
>   2 files changed, 152 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
> index 21340e0..90c6553 100644
> --- a/drivers/cpuidle/Kconfig.arm
> +++ b/drivers/cpuidle/Kconfig.arm
> @@ -3,6 +3,7 @@
>   #
>   config ARM_CPUIDLE
>           bool "Generic ARM/ARM64 CPU idle Driver"
> +        select CPU_IDLE_MULTIPLE_DRIVERS
>           select DT_IDLE_STATES
>           help
>             Select this to enable generic cpuidle driver for ARM.
> diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
> index 545069d..251fa2a 100644
> --- a/drivers/cpuidle/cpuidle-arm.c
> +++ b/drivers/cpuidle/cpuidle-arm.c
> @@ -14,6 +14,7 @@
>   #include <linux/cpuidle.h>
>   #include <linux/cpumask.h>
>   #include <linux/cpu_pm.h>
> +#include <linux/of_device.h>
>   #include <linux/kernel.h>
>   #include <linux/module.h>
>   #include <linux/of.h>
> @@ -58,23 +59,27 @@ static int arm_enter_idle_state(struct cpuidle_device *dev,
>   	return ret ? -1 : idx;
>   }
>
> -static struct cpuidle_driver arm_idle_driver = {
> -	.name = "arm_idle",
> -	.owner = THIS_MODULE,
> -	/*
> -	 * State at index 0 is standby wfi and considered standard
> -	 * on all ARM platforms. If in some platforms simple wfi
> -	 * can't be used as "state 0", DT bindings must be implemented
> -	 * to work around this issue and allow installing a special
> -	 * handler for idle state index 0.
> -	 */
> -	.states[0] = {
> -		.enter                  = arm_enter_idle_state,
> -		.exit_latency           = 1,
> -		.target_residency       = 1,
> -		.power_usage		= UINT_MAX,
> -		.name                   = "WFI",
> -		.desc                   = "ARM WFI",
> +#define ARM_CPUIDLE_MAX_DRIVERS	2
> +
> +static struct cpuidle_driver arm_idle_drivers[ARM_CPUIDLE_MAX_DRIVERS] = {
> +	[0 ... ARM_CPUIDLE_MAX_DRIVERS - 1] = {
> +		.name = "arm_idle",
> +		.owner = THIS_MODULE,
> +		/*
> +		 * State at index 0 is standby wfi and considered standard
> +		 * on all ARM platforms. If in some platforms simple wfi
> +		 * can't be used as "state 0", DT bindings must be implemented
> +		 * to work around this issue and allow installing a special
> +		 * handler for idle state index 0.
> +		 */
> +		.states[0] = {
> +			.enter                  = arm_enter_idle_state,
> +			.exit_latency           = 1,
> +			.target_residency       = 1,
> +			.power_usage		= UINT_MAX,
> +			.name                   = "WFI",
> +			.desc                   = "ARM WFI",
> +		}
>   	}
>   };
>
> @@ -85,17 +90,68 @@ static const struct of_device_id arm_idle_state_match[] __initconst = {
>   };
>
>   /*
> - * arm_idle_init
> + * Compare idle states phandle properties
>    *
> - * Registers the arm specific cpuidle driver with the cpuidle
> - * framework. It relies on core code to parse the idle states
> - * and initialize them using driver data structures accordingly.
> + * Return true if properties are valid and equal, false otherwise

Just a detail. Would be more consistent to return zero when valid and 
equal ?

>    */
> -static int __init arm_idle_init(void)
> +static bool __init idle_states_cmp(struct property *states1,
> +				   struct property *states2)
> +{
> +	/*
> +	 * NB: Implemented through code from drivers/of/unittest.c
> +	 *     Function is generic and can be moved to generic OF code
> +	 *     if needed
> +	 */
> +	return states1 && states2 &&
> +	       (states1->length == states2->length) &&
> +	       states1->value && states2->value &&
> +	       !memcmp(states1->value, states2->value, states1->length);
> +}
> +
> +static int __init arm_idle_init_driver(struct cpuidle_driver *drv)
>   {
> -	int cpu, ret;
> -	struct cpuidle_driver *drv = &arm_idle_driver;
> +	int ret, cpu;
>   	struct cpuidle_device *dev;
> +	struct property *curr_idle_states, *idle_states = NULL;
> +	struct device_node *cpu_node;
> +
> +	for_each_cpu(cpu, drv->cpumask) {
> +		cpu_node = of_cpu_device_node_get(cpu);
> +		curr_idle_states = of_find_property(cpu_node,
> +						    "cpu-idle-states", NULL);
> +		of_node_put(cpu_node);
> +
> +		/*
> +		 * Stash the first valid idle states phandle in the cpumask.
> +		 * If curr_idle_states is NULL assigning it to idle_states
> +		 * is harmless and it is managed by idle states comparison
> +		 * code. Keep track of first valid phandle so that
> +		 * subsequent cpus can compare against it.
> +		 */
> +		if (!idle_states)
> +			idle_states = curr_idle_states;
> +
> +		/*
> +		 * If idle states phandles are not equal, remove the
> +		 * cpu from the driver mask since a CPUidle driver
> +		 * is only capable of managing uniform idle states.
> +		 *
> +		 * Comparison works also when idle_states and
> +		 * curr_idle_states are the same property, since
> +		 * they can be == NULL so the cpu must be removed from
> +		 * the driver mask in that case too (ie cpu has no idle
> +		 * states).
> +		 */
> +		if (!idle_states_cmp(idle_states, curr_idle_states))
> +			cpumask_clear_cpu(cpu, drv->cpumask);
> +	}
> +
> +	/*
> +	 *  If there are no valid states for this driver we rely on arch
> +	 *  default idle behaviour, bail out
> +	 */
> +	if (!idle_states)
> +		return -ENODEV;
>
>   	/*
>   	 * Initialize idle states data, starting at index 1.
> @@ -117,7 +173,7 @@ static int __init arm_idle_init(void)
>   	 * Call arch CPU operations in order to initialize
>   	 * idle states suspend back-end specific data
>   	 */
> -	for_each_possible_cpu(cpu) {
> +	for_each_cpu(cpu, drv->cpumask) {
>   		ret = arm_cpuidle_init(cpu);
>
>   		/*
> @@ -157,7 +213,77 @@ out_fail:
>   	}
>
>   	cpuidle_unregister_driver(drv);
> +	return ret;
> +}
> +
> +/*
> + * arm_idle_init
> + *
> + * Registers the arm specific cpuidle driver(s) with the cpuidle
> + * framework. It relies on core code to parse the idle states
> + * and initialize them using driver data structures accordingly.
> + */
> +static int __init arm_idle_init(void)
> +{
> +	int i, ret = -ENODEV;
> +	struct cpuidle_driver *drv;
> +	cpumask_var_t tmpmask;
> +
> +	/*
> +	 * These drivers require DT idle states to be present.
> +	 * If no idle states are detected there is no reason to
> +	 * proceed any further hence we return early.
> +	 */
> +	if (!of_find_node_by_name(NULL, "idle-states"))
> +		return -ENODEV;
> +
> +	if (!alloc_cpumask_var(&tmpmask, GFP_KERNEL))
> +		return -ENOMEM;
> +
> +	/*
> +	 * We need to vet idle states to create CPUidle drivers
> +	 * that share a common set of them. Create a tmp mask
> +	 * that we use to keep track of initialized cpus.
> +	 * Start off by initializing the mask with all possible
> +	 * cpus, we clear it as we go, till either all cpus
> +	 * have a CPUidle driver initialized or there are some
> +	 * CPUs that have no idle states or a parsing error
> +	 * occurs.
> +	 */
> +	cpumask_copy(tmpmask, cpu_possible_mask);
> +
> +	for (i = 0; !cpumask_empty(tmpmask); i++) {
> +		if (i == ARM_CPUIDLE_MAX_DRIVERS) {
> +			pr_warn("number of drivers exceeding static allocation\n");
> +			break;
> +		}
> +
> +		drv = &arm_idle_drivers[i];
> +		drv->cpumask = kzalloc(cpumask_size(), GFP_KERNEL);
> +		if (!drv->cpumask) {
> +			ret = -ENOMEM;
> +			break;
> +		}
> +		/*
> +		 * Force driver mask, arm_idle_init_driver()
> +		 * will tweak it by vetting idle states.
> +		 */
> +		cpumask_copy(drv->cpumask, tmpmask);

Why do you need to copy tmpmask ? Isn't simpler to have a zero-ed 
cpumask and let the arm_idle_init_driver function to set a bit instead 
of clearing it ?

> +		ret = arm_idle_init_driver(drv);
> +		if (ret) {
> +			kfree(drv->cpumask);
> +			break;
> +		}
> +		/*
> +		 * Remove the cpus that were part of the registered
> +		 * driver from the mask of cpus to be initialized
> +		 * and restart.
> +		 */
> +		cpumask_andnot(tmpmask, tmpmask, drv->cpumask);

If there is a DT definition with just a cluster with the cpuidle driver 
set and another one without any idle state, we will have always a 
pr_warn because i == ARM_CPUIDLE_MAX_DRIVERS due to tmpmask being never 
equal to a zero mask. Is it the purpose to detect such cases ?

> +	}
>
> +	free_cpumask_var(tmpmask);
>   	return ret;
>   }
>   device_initcall(arm_idle_init);
>


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

Re: [RFC PATCH v2 1/1] drivers: cpuidle: cpuidle-arm: heterogeneous systems extension

[Lorenzo Pieralisi]

On Mon, May 04, 2015 at 02:19:15PM +0100, Daniel Lezcano wrote:
> On 04/16/2015 06:10 PM, Lorenzo Pieralisi wrote:

<snip>

> >   /*
> > - * arm_idle_init
> > + * Compare idle states phandle properties
> >    *
> > - * Registers the arm specific cpuidle driver with the cpuidle
> > - * framework. It relies on core code to parse the idle states
> > - * and initialize them using driver data structures accordingly.
> > + * Return true if properties are valid and equal, false otherwise
> 
> Just a detail. Would be more consistent to return zero when valid and
> equal ?

Consistent with memcmp you mean ? Yes, I can change it.

> >    */
> > -static int __init arm_idle_init(void)
> > +static bool __init idle_states_cmp(struct property *states1,
> > +                                struct property *states2)
> > +{
> > +     /*
> > +      * NB: Implemented through code from drivers/of/unittest.c
> > +      *     Function is generic and can be moved to generic OF code
> > +      *     if needed
> > +      */
> > +     return states1 && states2 &&
> > +            (states1->length == states2->length) &&
> > +            states1->value && states2->value &&
> > +            !memcmp(states1->value, states2->value, states1->length);
> > +}
> > +
> > +static int __init arm_idle_init_driver(struct cpuidle_driver *drv)
> >   {
> > -     int cpu, ret;
> > -     struct cpuidle_driver *drv = &arm_idle_driver;
> > +     int ret, cpu;
> >       struct cpuidle_device *dev;
> > +     struct property *curr_idle_states, *idle_states = NULL;
> > +     struct device_node *cpu_node;
> > +
> > +     for_each_cpu(cpu, drv->cpumask) {
> > +             cpu_node = of_cpu_device_node_get(cpu);
> > +             curr_idle_states = of_find_property(cpu_node,
> > +                                                 "cpu-idle-states", NULL);
> > +             of_node_put(cpu_node);
> > +
> > +             /*
> > +              * Stash the first valid idle states phandle in the cpumask.
> > +              * If curr_idle_states is NULL assigning it to idle_states
> > +              * is harmless and it is managed by idle states comparison
> > +              * code. Keep track of first valid phandle so that
> > +              * subsequent cpus can compare against it.
> > +              */
> > +             if (!idle_states)
> > +                     idle_states = curr_idle_states;
> > +
> > +             /*
> > +              * If idle states phandles are not equal, remove the
> > +              * cpu from the driver mask since a CPUidle driver
> > +              * is only capable of managing uniform idle states.
> > +              *
> > +              * Comparison works also when idle_states and
> > +              * curr_idle_states are the same property, since
> > +              * they can be == NULL so the cpu must be removed from
> > +              * the driver mask in that case too (ie cpu has no idle
> > +              * states).
> > +              */
> > +             if (!idle_states_cmp(idle_states, curr_idle_states))
> > +                     cpumask_clear_cpu(cpu, drv->cpumask);
> > +     }
> > +
> > +     /*
> > +      *  If there are no valid states for this driver we rely on arch
> > +      *  default idle behaviour, bail out
> > +      */
> > +     if (!idle_states)
> > +             return -ENODEV;
> >
> >       /*
> >        * Initialize idle states data, starting at index 1.
> > @@ -117,7 +173,7 @@ static int __init arm_idle_init(void)
> >        * Call arch CPU operations in order to initialize
> >        * idle states suspend back-end specific data
> >        */
> > -     for_each_possible_cpu(cpu) {
> > +     for_each_cpu(cpu, drv->cpumask) {
> >               ret = arm_cpuidle_init(cpu);
> >
> >               /*
> > @@ -157,7 +213,77 @@ out_fail:
> >       }
> >
> >       cpuidle_unregister_driver(drv);
> > +     return ret;
> > +}
> > +
> > +/*
> > + * arm_idle_init
> > + *
> > + * Registers the arm specific cpuidle driver(s) with the cpuidle
> > + * framework. It relies on core code to parse the idle states
> > + * and initialize them using driver data structures accordingly.
> > + */
> > +static int __init arm_idle_init(void)
> > +{
> > +     int i, ret = -ENODEV;
> > +     struct cpuidle_driver *drv;
> > +     cpumask_var_t tmpmask;
> > +
> > +     /*
> > +      * These drivers require DT idle states to be present.
> > +      * If no idle states are detected there is no reason to
> > +      * proceed any further hence we return early.
> > +      */
> > +     if (!of_find_node_by_name(NULL, "idle-states"))
> > +             return -ENODEV;
> > +
> > +     if (!alloc_cpumask_var(&tmpmask, GFP_KERNEL))
> > +             return -ENOMEM;
> > +
> > +     /*
> > +      * We need to vet idle states to create CPUidle drivers
> > +      * that share a common set of them. Create a tmp mask
> > +      * that we use to keep track of initialized cpus.
> > +      * Start off by initializing the mask with all possible
> > +      * cpus, we clear it as we go, till either all cpus
> > +      * have a CPUidle driver initialized or there are some
> > +      * CPUs that have no idle states or a parsing error
> > +      * occurs.
> > +      */
> > +     cpumask_copy(tmpmask, cpu_possible_mask);
> > +
> > +     for (i = 0; !cpumask_empty(tmpmask); i++) {
> > +             if (i == ARM_CPUIDLE_MAX_DRIVERS) {
> > +                     pr_warn("number of drivers exceeding static allocation\n");
> > +                     break;
> > +             }
> > +
> > +             drv = &arm_idle_drivers[i];
> > +             drv->cpumask = kzalloc(cpumask_size(), GFP_KERNEL);
> > +             if (!drv->cpumask) {
> > +                     ret = -ENOMEM;
> > +                     break;
> > +             }
> > +             /*
> > +              * Force driver mask, arm_idle_init_driver()
> > +              * will tweak it by vetting idle states.
> > +              */
> > +             cpumask_copy(drv->cpumask, tmpmask);
> 
> Why do you need to copy tmpmask ? Isn't simpler to have a zero-ed
> cpumask and let the arm_idle_init_driver function to set a bit instead
> of clearing it ?

No, because we need to keep track of logical cpus that have been
already parsed, we need a tmpmask to keep track of that, how could
we do it otherwise ? We can have more than two cpumask sets.

> 
> > +             ret = arm_idle_init_driver(drv);
> > +             if (ret) {
> > +                     kfree(drv->cpumask);
> > +                     break;
> > +             }
> > +             /*
> > +              * Remove the cpus that were part of the registered
> > +              * driver from the mask of cpus to be initialized
> > +              * and restart.
> > +              */
> > +             cpumask_andnot(tmpmask, tmpmask, drv->cpumask);
> 
> If there is a DT definition with just a cluster with the cpuidle driver
> set and another one without any idle state, we will have always a
> pr_warn because i == ARM_CPUIDLE_MAX_DRIVERS due to tmpmask being never
> equal to a zero mask. Is it the purpose to detect such cases ?

Not really, because arm_idle_init_driver() would return -ENODEV when
it detects cpus with no idle states and the loop will break before warning.

If we had two cluster of cpus with an idle-states set per cluster plus
some cpus with no idle states the warning would be hit, because
in actual facts we have more cpuidle sets than drivers (I know, a cpu
set with no idle states falls back to default idle, but I think
that's a detail that is easy to sort out).

I can create idle drivers dynamically so that the ARM_CPUIDLE_MAX_DRIVERS
check can be removed or relaxed (but the code becomes slightly more complex).

Thanks for having a look, apart from these comments do we think it is
a reasonable approach ?

Lorenzo

> 
> > +     }
> >
> > +     free_cpumask_var(tmpmask);
> >       return ret;
> >   }
> >   device_initcall(arm_idle_init);
> >
> 
> 
> --
>   <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
> 

Re: [RFC PATCH v2 1/1] drivers: cpuidle: cpuidle-arm: heterogeneous systems extension

[Lorenzo Pieralisi]

On Tue, May 05, 2015 at 04:56:15PM +0100, Lorenzo Pieralisi wrote:
> On Mon, May 04, 2015 at 02:19:15PM +0100, Daniel Lezcano wrote:
> > On 04/16/2015 06:10 PM, Lorenzo Pieralisi wrote:
> 
> <snip>
> 
> > >   /*
> > > - * arm_idle_init
> > > + * Compare idle states phandle properties
> > >    *
> > > - * Registers the arm specific cpuidle driver with the cpuidle
> > > - * framework. It relies on core code to parse the idle states
> > > - * and initialize them using driver data structures accordingly.
> > > + * Return true if properties are valid and equal, false otherwise
> >
> > Just a detail. Would be more consistent to return zero when valid and
> > equal ?
> 
> Consistent with memcmp you mean ? Yes, I can change it.
> 
> > >    */
> > > -static int __init arm_idle_init(void)
> > > +static bool __init idle_states_cmp(struct property *states1,
> > > +                                struct property *states2)
> > > +{
> > > +     /*
> > > +      * NB: Implemented through code from drivers/of/unittest.c
> > > +      *     Function is generic and can be moved to generic OF code
> > > +      *     if needed
> > > +      */
> > > +     return states1 && states2 &&
> > > +            (states1->length == states2->length) &&
> > > +            states1->value && states2->value &&
> > > +            !memcmp(states1->value, states2->value, states1->length);
> > > +}
> > > +
> > > +static int __init arm_idle_init_driver(struct cpuidle_driver *drv)
> > >   {
> > > -     int cpu, ret;
> > > -     struct cpuidle_driver *drv = &arm_idle_driver;
> > > +     int ret, cpu;
> > >       struct cpuidle_device *dev;
> > > +     struct property *curr_idle_states, *idle_states = NULL;
> > > +     struct device_node *cpu_node;
> > > +
> > > +     for_each_cpu(cpu, drv->cpumask) {
> > > +             cpu_node = of_cpu_device_node_get(cpu);
> > > +             curr_idle_states = of_find_property(cpu_node,
> > > +                                                 "cpu-idle-states", NULL);
> > > +             of_node_put(cpu_node);
> > > +
> > > +             /*
> > > +              * Stash the first valid idle states phandle in the cpumask.
> > > +              * If curr_idle_states is NULL assigning it to idle_states
> > > +              * is harmless and it is managed by idle states comparison
> > > +              * code. Keep track of first valid phandle so that
> > > +              * subsequent cpus can compare against it.
> > > +              */
> > > +             if (!idle_states)
> > > +                     idle_states = curr_idle_states;
> > > +
> > > +             /*
> > > +              * If idle states phandles are not equal, remove the
> > > +              * cpu from the driver mask since a CPUidle driver
> > > +              * is only capable of managing uniform idle states.
> > > +              *
> > > +              * Comparison works also when idle_states and
> > > +              * curr_idle_states are the same property, since
> > > +              * they can be == NULL so the cpu must be removed from
> > > +              * the driver mask in that case too (ie cpu has no idle
> > > +              * states).
> > > +              */
> > > +             if (!idle_states_cmp(idle_states, curr_idle_states))
> > > +                     cpumask_clear_cpu(cpu, drv->cpumask);
> > > +     }
> > > +
> > > +     /*
> > > +      *  If there are no valid states for this driver we rely on arch
> > > +      *  default idle behaviour, bail out
> > > +      */
> > > +     if (!idle_states)
> > > +             return -ENODEV;
> > >
> > >       /*
> > >        * Initialize idle states data, starting at index 1.
> > > @@ -117,7 +173,7 @@ static int __init arm_idle_init(void)
> > >        * Call arch CPU operations in order to initialize
> > >        * idle states suspend back-end specific data
> > >        */
> > > -     for_each_possible_cpu(cpu) {
> > > +     for_each_cpu(cpu, drv->cpumask) {
> > >               ret = arm_cpuidle_init(cpu);
> > >
> > >               /*
> > > @@ -157,7 +213,77 @@ out_fail:
> > >       }
> > >
> > >       cpuidle_unregister_driver(drv);
> > > +     return ret;
> > > +}
> > > +
> > > +/*
> > > + * arm_idle_init
> > > + *
> > > + * Registers the arm specific cpuidle driver(s) with the cpuidle
> > > + * framework. It relies on core code to parse the idle states
> > > + * and initialize them using driver data structures accordingly.
> > > + */
> > > +static int __init arm_idle_init(void)
> > > +{
> > > +     int i, ret = -ENODEV;
> > > +     struct cpuidle_driver *drv;
> > > +     cpumask_var_t tmpmask;
> > > +
> > > +     /*
> > > +      * These drivers require DT idle states to be present.
> > > +      * If no idle states are detected there is no reason to
> > > +      * proceed any further hence we return early.
> > > +      */
> > > +     if (!of_find_node_by_name(NULL, "idle-states"))
> > > +             return -ENODEV;
> > > +
> > > +     if (!alloc_cpumask_var(&tmpmask, GFP_KERNEL))
> > > +             return -ENOMEM;
> > > +
> > > +     /*
> > > +      * We need to vet idle states to create CPUidle drivers
> > > +      * that share a common set of them. Create a tmp mask
> > > +      * that we use to keep track of initialized cpus.
> > > +      * Start off by initializing the mask with all possible
> > > +      * cpus, we clear it as we go, till either all cpus
> > > +      * have a CPUidle driver initialized or there are some
> > > +      * CPUs that have no idle states or a parsing error
> > > +      * occurs.
> > > +      */
> > > +     cpumask_copy(tmpmask, cpu_possible_mask);
> > > +
> > > +     for (i = 0; !cpumask_empty(tmpmask); i++) {
> > > +             if (i == ARM_CPUIDLE_MAX_DRIVERS) {
> > > +                     pr_warn("number of drivers exceeding static allocation\n");
> > > +                     break;
> > > +             }
> > > +
> > > +             drv = &arm_idle_drivers[i];
> > > +             drv->cpumask = kzalloc(cpumask_size(), GFP_KERNEL);
> > > +             if (!drv->cpumask) {
> > > +                     ret = -ENOMEM;
> > > +                     break;
> > > +             }
> > > +             /*
> > > +              * Force driver mask, arm_idle_init_driver()
> > > +              * will tweak it by vetting idle states.
> > > +              */
> > > +             cpumask_copy(drv->cpumask, tmpmask);
> >
> > Why do you need to copy tmpmask ? Isn't simpler to have a zero-ed
> > cpumask and let the arm_idle_init_driver function to set a bit instead
> > of clearing it ?
> 
> No, because we need to keep track of logical cpus that have been
> already parsed, we need a tmpmask to keep track of that, how could
> we do it otherwise ? We can have more than two cpumask sets.
> 
> >
> > > +             ret = arm_idle_init_driver(drv);
> > > +             if (ret) {
> > > +                     kfree(drv->cpumask);
> > > +                     break;
> > > +             }
> > > +             /*
> > > +              * Remove the cpus that were part of the registered
> > > +              * driver from the mask of cpus to be initialized
> > > +              * and restart.
> > > +              */
> > > +             cpumask_andnot(tmpmask, tmpmask, drv->cpumask);
> >
> > If there is a DT definition with just a cluster with the cpuidle driver
> > set and another one without any idle state, we will have always a
> > pr_warn because i == ARM_CPUIDLE_MAX_DRIVERS due to tmpmask being never
> > equal to a zero mask. Is it the purpose to detect such cases ?
> 
> Not really, because arm_idle_init_driver() would return -ENODEV when
> it detects cpus with no idle states and the loop will break before warning.
> 
> If we had two cluster of cpus with an idle-states set per cluster plus
> some cpus with no idle states the warning would be hit, because
> in actual facts we have more cpuidle sets than drivers (I know, a cpu
> set with no idle states falls back to default idle, but I think
> that's a detail that is easy to sort out).
> 
> I can create idle drivers dynamically so that the ARM_CPUIDLE_MAX_DRIVERS
> check can be removed or relaxed (but the code becomes slightly more complex).
> 
> Thanks for having a look, apart from these comments do we think it is
> a reasonable approach ?

Any further comments ? I can post a v2 with an updated idle_states_cmp()
return value, if we think the multiple drivers approach is valid.

Thanks a lot,
Lorenzo

Re: [RFC PATCH v2 1/1] drivers: cpuidle: cpuidle-arm: heterogeneous systems extension

[Daniel Kurtz]

Hi Lorenzo,

On Tue, May 12, 2015 at 9:03 PM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Tue, May 05, 2015 at 04:56:15PM +0100, Lorenzo Pieralisi wrote:
>> On Mon, May 04, 2015 at 02:19:15PM +0100, Daniel Lezcano wrote:
>> > On 04/16/2015 06:10 PM, Lorenzo Pieralisi wrote:
>>
>> <snip>
>>
>> > >   /*
>> > > - * arm_idle_init
>> > > + * Compare idle states phandle properties
>> > >    *
>> > > - * Registers the arm specific cpuidle driver with the cpuidle
>> > > - * framework. It relies on core code to parse the idle states
>> > > - * and initialize them using driver data structures accordingly.
>> > > + * Return true if properties are valid and equal, false otherwise
>> >
>> > Just a detail. Would be more consistent to return zero when valid and
>> > equal ?
>>
>> Consistent with memcmp you mean ? Yes, I can change it.
>>
>> > >    */
>> > > -static int __init arm_idle_init(void)
>> > > +static bool __init idle_states_cmp(struct property *states1,
>> > > +                                struct property *states2)
>> > > +{
>> > > +     /*
>> > > +      * NB: Implemented through code from drivers/of/unittest.c
>> > > +      *     Function is generic and can be moved to generic OF code
>> > > +      *     if needed
>> > > +      */
>> > > +     return states1 && states2 &&
>> > > +            (states1->length == states2->length) &&
>> > > +            states1->value && states2->value &&
>> > > +            !memcmp(states1->value, states2->value, states1->length);
>> > > +}
>> > > +
>> > > +static int __init arm_idle_init_driver(struct cpuidle_driver *drv)
>> > >   {
>> > > -     int cpu, ret;
>> > > -     struct cpuidle_driver *drv = &arm_idle_driver;
>> > > +     int ret, cpu;
>> > >       struct cpuidle_device *dev;
>> > > +     struct property *curr_idle_states, *idle_states = NULL;
>> > > +     struct device_node *cpu_node;
>> > > +
>> > > +     for_each_cpu(cpu, drv->cpumask) {
>> > > +             cpu_node = of_cpu_device_node_get(cpu);
>> > > +             curr_idle_states = of_find_property(cpu_node,
>> > > +                                                 "cpu-idle-states", NULL);
>> > > +             of_node_put(cpu_node);
>> > > +
>> > > +             /*
>> > > +              * Stash the first valid idle states phandle in the cpumask.
>> > > +              * If curr_idle_states is NULL assigning it to idle_states
>> > > +              * is harmless and it is managed by idle states comparison
>> > > +              * code. Keep track of first valid phandle so that
>> > > +              * subsequent cpus can compare against it.
>> > > +              */
>> > > +             if (!idle_states)
>> > > +                     idle_states = curr_idle_states;
>> > > +
>> > > +             /*
>> > > +              * If idle states phandles are not equal, remove the
>> > > +              * cpu from the driver mask since a CPUidle driver
>> > > +              * is only capable of managing uniform idle states.
>> > > +              *
>> > > +              * Comparison works also when idle_states and
>> > > +              * curr_idle_states are the same property, since
>> > > +              * they can be == NULL so the cpu must be removed from
>> > > +              * the driver mask in that case too (ie cpu has no idle
>> > > +              * states).
>> > > +              */
>> > > +             if (!idle_states_cmp(idle_states, curr_idle_states))
>> > > +                     cpumask_clear_cpu(cpu, drv->cpumask);
>> > > +     }
>> > > +
>> > > +     /*
>> > > +      *  If there are no valid states for this driver we rely on arch
>> > > +      *  default idle behaviour, bail out
>> > > +      */
>> > > +     if (!idle_states)
>> > > +             return -ENODEV;
>> > >
>> > >       /*
>> > >        * Initialize idle states data, starting at index 1.
>> > > @@ -117,7 +173,7 @@ static int __init arm_idle_init(void)
>> > >        * Call arch CPU operations in order to initialize
>> > >        * idle states suspend back-end specific data
>> > >        */
>> > > -     for_each_possible_cpu(cpu) {
>> > > +     for_each_cpu(cpu, drv->cpumask) {
>> > >               ret = arm_cpuidle_init(cpu);
>> > >
>> > >               /*
>> > > @@ -157,7 +213,77 @@ out_fail:
>> > >       }
>> > >
>> > >       cpuidle_unregister_driver(drv);
>> > > +     return ret;
>> > > +}
>> > > +
>> > > +/*
>> > > + * arm_idle_init
>> > > + *
>> > > + * Registers the arm specific cpuidle driver(s) with the cpuidle
>> > > + * framework. It relies on core code to parse the idle states
>> > > + * and initialize them using driver data structures accordingly.
>> > > + */
>> > > +static int __init arm_idle_init(void)
>> > > +{
>> > > +     int i, ret = -ENODEV;
>> > > +     struct cpuidle_driver *drv;
>> > > +     cpumask_var_t tmpmask;
>> > > +
>> > > +     /*
>> > > +      * These drivers require DT idle states to be present.
>> > > +      * If no idle states are detected there is no reason to
>> > > +      * proceed any further hence we return early.
>> > > +      */
>> > > +     if (!of_find_node_by_name(NULL, "idle-states"))
>> > > +             return -ENODEV;
>> > > +
>> > > +     if (!alloc_cpumask_var(&tmpmask, GFP_KERNEL))
>> > > +             return -ENOMEM;
>> > > +
>> > > +     /*
>> > > +      * We need to vet idle states to create CPUidle drivers
>> > > +      * that share a common set of them. Create a tmp mask
>> > > +      * that we use to keep track of initialized cpus.
>> > > +      * Start off by initializing the mask with all possible
>> > > +      * cpus, we clear it as we go, till either all cpus
>> > > +      * have a CPUidle driver initialized or there are some
>> > > +      * CPUs that have no idle states or a parsing error
>> > > +      * occurs.
>> > > +      */
>> > > +     cpumask_copy(tmpmask, cpu_possible_mask);
>> > > +
>> > > +     for (i = 0; !cpumask_empty(tmpmask); i++) {
>> > > +             if (i == ARM_CPUIDLE_MAX_DRIVERS) {
>> > > +                     pr_warn("number of drivers exceeding static allocation\n");
>> > > +                     break;
>> > > +             }
>> > > +
>> > > +             drv = &arm_idle_drivers[i];
>> > > +             drv->cpumask = kzalloc(cpumask_size(), GFP_KERNEL);
>> > > +             if (!drv->cpumask) {
>> > > +                     ret = -ENOMEM;
>> > > +                     break;
>> > > +             }
>> > > +             /*
>> > > +              * Force driver mask, arm_idle_init_driver()
>> > > +              * will tweak it by vetting idle states.
>> > > +              */
>> > > +             cpumask_copy(drv->cpumask, tmpmask);
>> >
>> > Why do you need to copy tmpmask ? Isn't simpler to have a zero-ed
>> > cpumask and let the arm_idle_init_driver function to set a bit instead
>> > of clearing it ?
>>
>> No, because we need to keep track of logical cpus that have been
>> already parsed, we need a tmpmask to keep track of that, how could
>> we do it otherwise ? We can have more than two cpumask sets.
>>
>> >
>> > > +             ret = arm_idle_init_driver(drv);
>> > > +             if (ret) {
>> > > +                     kfree(drv->cpumask);
>> > > +                     break;
>> > > +             }
>> > > +             /*
>> > > +              * Remove the cpus that were part of the registered
>> > > +              * driver from the mask of cpus to be initialized
>> > > +              * and restart.
>> > > +              */
>> > > +             cpumask_andnot(tmpmask, tmpmask, drv->cpumask);
>> >
>> > If there is a DT definition with just a cluster with the cpuidle driver
>> > set and another one without any idle state, we will have always a
>> > pr_warn because i == ARM_CPUIDLE_MAX_DRIVERS due to tmpmask being never
>> > equal to a zero mask. Is it the purpose to detect such cases ?
>>
>> Not really, because arm_idle_init_driver() would return -ENODEV when
>> it detects cpus with no idle states and the loop will break before warning.
>>
>> If we had two cluster of cpus with an idle-states set per cluster plus
>> some cpus with no idle states the warning would be hit, because
>> in actual facts we have more cpuidle sets than drivers (I know, a cpu
>> set with no idle states falls back to default idle, but I think
>> that's a detail that is easy to sort out).
>>
>> I can create idle drivers dynamically so that the ARM_CPUIDLE_MAX_DRIVERS
>> check can be removed or relaxed (but the code becomes slightly more complex).
>>
>> Thanks for having a look, apart from these comments do we think it is
>> a reasonable approach ?
>
> Any further comments ? I can post a v2 with an updated idle_states_cmp()
> return value, if we think the multiple drivers approach is valid.

Can you post v2?
>From the silence on the list, I guess there is no strong objection to
your approach.
So, perhaps it is time to remove the "RFC" as well so this can get on
track to be merged.

-Dan


>
> Thanks a lot,
> Lorenzo
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v2 1/1] drivers: cpuidle: cpuidle-arm: heterogeneous systems extension

[Lorenzo Pieralisi]

Hi Daniel,

On Tue, Jul 14, 2015 at 05:52:48AM +0100, Daniel Kurtz wrote:

[...]

> > Any further comments ? I can post a v2 with an updated idle_states_cmp()
> > return value, if we think the multiple drivers approach is valid.
> 
> Can you post v2?
> From the silence on the list, I guess there is no strong objection to
> your approach.

I just wanted to make sure this patch is strictly required before asking
for a merge in the mainline, it adds to the code complexity so it should
not be taken lightly.

> So, perhaps it is time to remove the "RFC" as well so this can get on
> track to be merged.

I will drop the RFC status and add a simple configuration to define
a config entry to configure the max number of drivers statically so
that we do not waste memory for nothing.

Thanks,
Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 1/1] drivers: cpuidle: cpuidle-arm: heterogeneous systems extension

[Lorenzo Pieralisi]

Hi Daniel,

On Mon, May 04, 2015 at 02:19:15PM +0100, Daniel Lezcano wrote:
> On 04/16/2015 06:10 PM, Lorenzo Pieralisi wrote:

[...]

> >   /*
> > - * arm_idle_init
> > + * Compare idle states phandle properties
> >    *
> > - * Registers the arm specific cpuidle driver with the cpuidle
> > - * framework. It relies on core code to parse the idle states
> > - * and initialize them using driver data structures accordingly.
> > + * Return true if properties are valid and equal, false otherwise
> 
> Just a detail. Would be more consistent to return zero when valid and
> equal ?

It is a detail but I think it is clearer as it is (ie what would it
return on failure ? After all it is just used as a predicate, there
is no use for a return value other than a boolean), so unless you
have a strong feeling against it I would leave it as it is.

Thanks,
Lorenzo

> >    */
> > -static int __init arm_idle_init(void)
> > +static bool __init idle_states_cmp(struct property *states1,
> > +                                struct property *states2)
> > +{
> > +     /*
> > +      * NB: Implemented through code from drivers/of/unittest.c
> > +      *     Function is generic and can be moved to generic OF code
> > +      *     if needed
> > +      */
> > +     return states1 && states2 &&
> > +            (states1->length == states2->length) &&
> > +            states1->value && states2->value &&
> > +            !memcmp(states1->value, states2->value, states1->length);
> > +}
> > +
> > +static int __init arm_idle_init_driver(struct cpuidle_driver *drv)
> >   {
> > -     int cpu, ret;
> > -     struct cpuidle_driver *drv = &arm_idle_driver;
> > +     int ret, cpu;
> >       struct cpuidle_device *dev;
> > +     struct property *curr_idle_states, *idle_states = NULL;
> > +     struct device_node *cpu_node;
> > +
> > +     for_each_cpu(cpu, drv->cpumask) {
> > +             cpu_node = of_cpu_device_node_get(cpu);
> > +             curr_idle_states = of_find_property(cpu_node,
> > +                                                 "cpu-idle-states", NULL);
> > +             of_node_put(cpu_node);
> > +
> > +             /*
> > +              * Stash the first valid idle states phandle in the cpumask.
> > +              * If curr_idle_states is NULL assigning it to idle_states
> > +              * is harmless and it is managed by idle states comparison
> > +              * code. Keep track of first valid phandle so that
> > +              * subsequent cpus can compare against it.
> > +              */
> > +             if (!idle_states)
> > +                     idle_states = curr_idle_states;
> > +
> > +             /*
> > +              * If idle states phandles are not equal, remove the
> > +              * cpu from the driver mask since a CPUidle driver
> > +              * is only capable of managing uniform idle states.
> > +              *
> > +              * Comparison works also when idle_states and
> > +              * curr_idle_states are the same property, since
> > +              * they can be == NULL so the cpu must be removed from
> > +              * the driver mask in that case too (ie cpu has no idle
> > +              * states).
> > +              */
> > +             if (!idle_states_cmp(idle_states, curr_idle_states))
> > +                     cpumask_clear_cpu(cpu, drv->cpumask);
> > +     }
> > +
> > +     /*
> > +      *  If there are no valid states for this driver we rely on arch
> > +      *  default idle behaviour, bail out
> > +      */
> > +     if (!idle_states)
> > +             return -ENODEV;
> >
> >       /*
> >        * Initialize idle states data, starting at index 1.
> > @@ -117,7 +173,7 @@ static int __init arm_idle_init(void)
> >        * Call arch CPU operations in order to initialize
> >        * idle states suspend back-end specific data
> >        */
> > -     for_each_possible_cpu(cpu) {
> > +     for_each_cpu(cpu, drv->cpumask) {
> >               ret = arm_cpuidle_init(cpu);
> >
> >               /*
> > @@ -157,7 +213,77 @@ out_fail:
> >       }
> >
> >       cpuidle_unregister_driver(drv);
> > +     return ret;
> > +}
> > +
> > +/*
> > + * arm_idle_init
> > + *
> > + * Registers the arm specific cpuidle driver(s) with the cpuidle
> > + * framework. It relies on core code to parse the idle states
> > + * and initialize them using driver data structures accordingly.
> > + */
> > +static int __init arm_idle_init(void)
> > +{
> > +     int i, ret = -ENODEV;
> > +     struct cpuidle_driver *drv;
> > +     cpumask_var_t tmpmask;
> > +
> > +     /*
> > +      * These drivers require DT idle states to be present.
> > +      * If no idle states are detected there is no reason to
> > +      * proceed any further hence we return early.
> > +      */
> > +     if (!of_find_node_by_name(NULL, "idle-states"))
> > +             return -ENODEV;
> > +
> > +     if (!alloc_cpumask_var(&tmpmask, GFP_KERNEL))
> > +             return -ENOMEM;
> > +
> > +     /*
> > +      * We need to vet idle states to create CPUidle drivers
> > +      * that share a common set of them. Create a tmp mask
> > +      * that we use to keep track of initialized cpus.
> > +      * Start off by initializing the mask with all possible
> > +      * cpus, we clear it as we go, till either all cpus
> > +      * have a CPUidle driver initialized or there are some
> > +      * CPUs that have no idle states or a parsing error
> > +      * occurs.
> > +      */
> > +     cpumask_copy(tmpmask, cpu_possible_mask);
> > +
> > +     for (i = 0; !cpumask_empty(tmpmask); i++) {
> > +             if (i == ARM_CPUIDLE_MAX_DRIVERS) {
> > +                     pr_warn("number of drivers exceeding static allocation\n");
> > +                     break;
> > +             }
> > +
> > +             drv = &arm_idle_drivers[i];
> > +             drv->cpumask = kzalloc(cpumask_size(), GFP_KERNEL);
> > +             if (!drv->cpumask) {
> > +                     ret = -ENOMEM;
> > +                     break;
> > +             }
> > +             /*
> > +              * Force driver mask, arm_idle_init_driver()
> > +              * will tweak it by vetting idle states.
> > +              */
> > +             cpumask_copy(drv->cpumask, tmpmask);
> 
> Why do you need to copy tmpmask ? Isn't simpler to have a zero-ed
> cpumask and let the arm_idle_init_driver function to set a bit instead
> of clearing it ?
> 
> > +             ret = arm_idle_init_driver(drv);
> > +             if (ret) {
> > +                     kfree(drv->cpumask);
> > +                     break;
> > +             }
> > +             /*
> > +              * Remove the cpus that were part of the registered
> > +              * driver from the mask of cpus to be initialized
> > +              * and restart.
> > +              */
> > +             cpumask_andnot(tmpmask, tmpmask, drv->cpumask);
> 
> If there is a DT definition with just a cluster with the cpuidle driver
> set and another one without any idle state, we will have always a
> pr_warn because i == ARM_CPUIDLE_MAX_DRIVERS due to tmpmask being never
> equal to a zero mask. Is it the purpose to detect such cases ?
> 
> > +     }
> >
> > +     free_cpumask_var(tmpmask);
> >       return ret;
> >   }
> >   device_initcall(arm_idle_init);
> >
> 
> 
> --
>   <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
> 

Re: [RFC PATCH v2 0/1] ARM: cpuidle: heterogeneous systems extension

[Kevin Hilman]

On Thu, Apr 16, 2015 at 9:10 AM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:

[...]

> This patch sets the maximum number of static CPUidle drivers allowed to
> two, since it is hard to foresee systems with more than two sets of CPUs
> having different idle states

It's not hard to forsee anymore:

   http://www.cnx-software.com/2015/04/21/mediatek-helio-x20-deca-core-processor/
   https://liliputing.com/2015/04/closer-look-at-mediateks-upcoming-10-core-helio-x20-processor.html

Kevin

Re: [RFC PATCH v2 0/1] ARM: cpuidle: heterogeneous systems extension

[Lorenzo Pieralisi]

On Tue, Apr 21, 2015 at 07:24:54PM +0100, Kevin Hilman wrote:
> On Thu, Apr 16, 2015 at 9:10 AM, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> 
> [...]
> 
> > This patch sets the maximum number of static CPUidle drivers allowed to
> > two, since it is hard to foresee systems with more than two sets of CPUs
> > having different idle states
> 
> It's not hard to forsee anymore:
> 
>    http://www.cnx-software.com/2015/04/21/mediatek-helio-x20-deca-core-processor/
>    https://liliputing.com/2015/04/closer-look-at-mediateks-upcoming-10-core-helio-x20-processor.html

Eheh I stand corrected :). Not a big deal, drivers can be allocated
dynamically; it seems that the designs above leave us no choice, the
idle states are the only differentiating factor when it comes
to CPUidle driver logical cpumask creation, happy to hear more
opinions on this, but I do not really see what we can use instead.

Thanks,
Lorenzo

Re: [RFC PATCH v2 0/1] ARM: cpuidle: heterogeneous systems extension

[Lina Iyer]

On Wed, Apr 22 2015 at 02:55 -0600, Lorenzo Pieralisi wrote:
>On Tue, Apr 21, 2015 at 07:24:54PM +0100, Kevin Hilman wrote:
>> On Thu, Apr 16, 2015 at 9:10 AM, Lorenzo Pieralisi
>> <lorenzo.pieralisi@arm.com> wrote:
>>
>> [...]
>>
>> > This patch sets the maximum number of static CPUidle drivers allowed to
>> > two, since it is hard to foresee systems with more than two sets of CPUs
>> > having different idle states
>>
>> It's not hard to forsee anymore:
>>
>>    http://www.cnx-software.com/2015/04/21/mediatek-helio-x20-deca-core-processor/
>>    https://liliputing.com/2015/04/closer-look-at-mediateks-upcoming-10-core-helio-x20-processor.html
>
>Eheh I stand corrected :). Not a big deal, drivers can be allocated
>dynamically; it seems that the designs above leave us no choice, the
>idle states are the only differentiating factor when it comes
>to CPUidle driver logical cpumask creation, happy to hear more
>opinions on this, but I do not really see what we can use instead.


Dynamic idle driver would be nice. Otherwise, the patch looks good to
me.

Thanks,
Lina

Re: [RFC PATCH v2 0/1] ARM: cpuidle: heterogeneous systems extension

[Lorenzo Pieralisi]

On Thu, Apr 16, 2015 at 05:10:16PM +0100, Lorenzo Pieralisi wrote:
> This patch is a v2 of a previous posting:
> 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/328192.html
> 
> v1 -> v2:
> 
> - Rebased against 4.0
> - Removed DT affinity probe interface
> - Improved and optimized idle states comparison and cpumask creation
> - Merged DT idle states vetting code in the ARM CPUidle driver
> 
> This patch aims at enabling the generic CPUidle driver on ARM systems
> with heterogeneous idle states. In the CPUidle context the kernel
> considers cpus equal (and group them together through a CPUidle driver)
> if they share the same set of idle states.
> Since the idle states represent the differentiation point when it comes
> to creating CPUidle drivers (their states and related cpu logical mask),
> the idle states can be used as a means to create logical masks of cpus
> for which the CPUidle driver applies.
> 
> This patch sets the maximum number of static CPUidle drivers allowed to
> two, since it is hard to foresee systems with more than two sets of CPUs
> having different idle states; it is meant to keep allocation simple
> while keeping functionality.
> 
> The code carrying out idle states comparison is completely generic
> and borrowed from OF unit tests; it overrides the OF API entirely
> though to carry out properties (ie phandles list) comparison in the
> fastest possible way. If deemed worthwhile it can be moved to generic OF
> layer.
> 
> Tested through several dts idle states permutations on Juno board with
> CPUidle and related idle states enabled.

Are there any comments/opinions on this patch ?

Thanks,
Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html