* [RFC PATCH v2 1/1] drivers: cpuidle: cpuidle-arm: heterogeneous systems extension
@ 2015-05-04 13:19 ` Daniel Lezcano
0 siblings, 0 replies; 24+ messages in thread
From: Daniel Lezcano @ 2015-05-04 13:19 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [RFC PATCH v2 1/1] drivers: cpuidle: cpuidle-arm: heterogeneous systems extension
2015-05-04 13:19 ` Daniel Lezcano
@ 2015-05-05 15:56 ` Lorenzo Pieralisi
-1 siblings, 0 replies; 24+ messages in thread
From: Lorenzo Pieralisi @ 2015-05-05 15:56 UTC (permalink / raw)
To: Daniel Lezcano
Cc: Mark Rutland, devicetree@vger.kernel.org, Howard Chen,
Kevin Hilman, Mathieu Poirier, linux-pm@vger.kernel.org,
Rob Herring, Lina Iyer, Sudeep Holla, grant.likely@linaro.org,
linux-arm-kernel@lists.infradead.org
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
>
^ permalink raw reply [flat|nested] 24+ messages in thread* [RFC PATCH v2 1/1] drivers: cpuidle: cpuidle-arm: heterogeneous systems extension
@ 2015-05-05 15:56 ` Lorenzo Pieralisi
0 siblings, 0 replies; 24+ messages in thread
From: Lorenzo Pieralisi @ 2015-05-05 15:56 UTC (permalink / raw)
To: linux-arm-kernel
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
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [RFC PATCH v2 1/1] drivers: cpuidle: cpuidle-arm: heterogeneous systems extension
2015-05-05 15:56 ` Lorenzo Pieralisi
@ 2015-05-12 13:03 ` Lorenzo Pieralisi
-1 siblings, 0 replies; 24+ messages in thread
From: Lorenzo Pieralisi @ 2015-05-12 13:03 UTC (permalink / raw)
To: Daniel Lezcano
Cc: Mark Rutland, devicetree@vger.kernel.org, Howard Chen,
Kevin Hilman, Mathieu Poirier, linux-pm@vger.kernel.org,
Rob Herring, Lina Iyer, Sudeep Holla, grant.likely@linaro.org,
linux-arm-kernel@lists.infradead.org
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
^ permalink raw reply [flat|nested] 24+ messages in thread* [RFC PATCH v2 1/1] drivers: cpuidle: cpuidle-arm: heterogeneous systems extension
@ 2015-05-12 13:03 ` Lorenzo Pieralisi
0 siblings, 0 replies; 24+ messages in thread
From: Lorenzo Pieralisi @ 2015-05-12 13:03 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [RFC PATCH v2 1/1] drivers: cpuidle: cpuidle-arm: heterogeneous systems extension
2015-05-12 13:03 ` Lorenzo Pieralisi
@ 2015-07-14 4:52 ` Daniel Kurtz
-1 siblings, 0 replies; 24+ messages in thread
From: Daniel Kurtz @ 2015-07-14 4:52 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Daniel Lezcano, Mark Rutland, devicetree@vger.kernel.org,
Howard Chen, Kevin Hilman, Mathieu Poirier,
linux-pm@vger.kernel.org, Rob Herring,
linux-arm-kernel@lists.infradead.org, Sudeep Holla,
grant.likely@linaro.org, Lina Iyer, Ricky Liang,
Eddie Huang (黃智傑), Joseph Lo, Benson Leung
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
^ permalink raw reply [flat|nested] 24+ messages in thread* [RFC PATCH v2 1/1] drivers: cpuidle: cpuidle-arm: heterogeneous systems extension
@ 2015-07-14 4:52 ` Daniel Kurtz
0 siblings, 0 replies; 24+ messages in thread
From: Daniel Kurtz @ 2015-07-14 4:52 UTC (permalink / raw)
To: linux-arm-kernel
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 at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread[parent not found: <CAGS+omBtraNBH43qsFn_YgO3ePbFav9VtwgqDUinbOBMPwOGXA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [RFC PATCH v2 1/1] drivers: cpuidle: cpuidle-arm: heterogeneous systems extension
2015-07-14 4:52 ` Daniel Kurtz
@ 2015-07-14 10:04 ` Lorenzo Pieralisi
-1 siblings, 0 replies; 24+ messages in thread
From: Lorenzo Pieralisi @ 2015-07-14 10:04 UTC (permalink / raw)
To: Daniel Kurtz
Cc: Daniel Lezcano, Mark Rutland,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Howard Chen,
Kevin Hilman, Mathieu Poirier,
linux-pm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Rob Herring,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Sudeep Holla,
grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org, Lina Iyer,
Ricky Liang, Eddie Huang (?????????), Joseph Lo, Benson Leung
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* [RFC PATCH v2 1/1] drivers: cpuidle: cpuidle-arm: heterogeneous systems extension
@ 2015-07-14 10:04 ` Lorenzo Pieralisi
0 siblings, 0 replies; 24+ messages in thread
From: Lorenzo Pieralisi @ 2015-07-14 10:04 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH v2 1/1] drivers: cpuidle: cpuidle-arm: heterogeneous systems extension
2015-05-04 13:19 ` Daniel Lezcano
@ 2015-07-14 10:23 ` Lorenzo Pieralisi
-1 siblings, 0 replies; 24+ messages in thread
From: Lorenzo Pieralisi @ 2015-07-14 10:23 UTC (permalink / raw)
To: Daniel Lezcano
Cc: linux-arm-kernel@lists.infradead.org, linux-pm@vger.kernel.org,
devicetree@vger.kernel.org, Howard Chen, Rob Herring,
Kevin Hilman, Sudeep Holla, Lina Iyer, grant.likely@linaro.org,
Mathieu Poirier, Mark Rutland
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
>
^ permalink raw reply [flat|nested] 24+ messages in thread* [RFC PATCH v2 1/1] drivers: cpuidle: cpuidle-arm: heterogeneous systems extension
@ 2015-07-14 10:23 ` Lorenzo Pieralisi
0 siblings, 0 replies; 24+ messages in thread
From: Lorenzo Pieralisi @ 2015-07-14 10:23 UTC (permalink / raw)
To: linux-arm-kernel
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
>
^ permalink raw reply [flat|nested] 24+ messages in thread