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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
