From mboxrd@z Thu Jan 1 00:00:00 1970 From: daniel.lezcano@linaro.org (Daniel Lezcano) Date: Mon, 04 May 2015 15:19:15 +0200 Subject: [RFC PATCH v2 1/1] drivers: cpuidle: cpuidle-arm: heterogeneous systems extension In-Reply-To: <1429200617-9546-2-git-send-email-lorenzo.pieralisi@arm.com> References: <1429200617-9546-1-git-send-email-lorenzo.pieralisi@arm.com> <1429200617-9546-2-git-send-email-lorenzo.pieralisi@arm.com> Message-ID: <554771D3.5010701@linaro.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 > Cc: Howard Chen > Cc: Rob Herring > Cc: Kevin Hilman > Cc: Sudeep Holla > Cc: Lina Iyer > Cc: Daniel Lezcano > Cc: Grant Likely > Cc: Mathieu Poirier > Cc: Mark Rutland > --- > 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 > #include > #include > +#include > #include > #include > #include > @@ -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); > -- Linaro.org ? Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog