[PATCH V5 1/6] drivers: cpuidle: Read CPU's idle state from PM domain

[Brendan Jackman]

Hi Lina,

On Fri, Mar 03 2017 at 21:48, Lina Iyer wrote:
> Currently CPUs idle states and idle states of its parent are represented
> in a flattened model by the cpu-dile-states property of the CPU node.
> The CPUs idle states are followed by its cluster idle states. With the
> introduction of CPU PM domains, the CPUs and domain idle states may be
> represented hierarchically as part of the domain DT definition. This
> would mean presenting idle state information in 2 places - CPU nodes for
> the CPU and the cluster's with the PM domains.
>
> Also, it makes sense to define domains around each individual CPU since
> each of them is a power domain in its own right. The CPU idle states can
> now be represented as its domain's idle state, defined by the
> domain-idle-states property. This avoids presenting idle states in
> multiple places in the DT.
>
> Modify the DT-based cpuidle driver to check for the presence of a CPU's
> domain and if present read the domain-idle-states of the PM domain and
> if the CPU's domain is absent, revert to reading in the cpu-idle-states
> property of the CPU DT node.
>
> Suggested-by: Sudeep Holla <sudeep.holla at qrm.com>
s/qrm/arm/

> Signed-off-by: Lina Iyer <lina.iyer at linaro.org>
> ---
>  drivers/cpuidle/dt_idle_states.c | 38 +++++++++++++++++++++++++++++++++++---
>  1 file changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/cpuidle/dt_idle_states.c b/drivers/cpuidle/dt_idle_states.c
> index ffca4fc..4df7d45 100644
> --- a/drivers/cpuidle/dt_idle_states.c
> +++ b/drivers/cpuidle/dt_idle_states.c
> @@ -98,6 +98,39 @@ static int init_state_node(struct cpuidle_state *idle_state,
>  }
>
>  /*
> + * Get the state node at @idx. State node may be defined as domain's idle state
> + * if the CPU has its own domain or defined as CPU's idle state if it doesn't
> + * have a domain provider.
> + */
> +static struct device_node *get_state_node(struct device_node *cpu_node,
> +				unsigned int idx)
> +{
> +	struct device_node *dn;
> +	bool cpu_has_domain = false;
> +	struct of_phandle_args args;
> +	const char *property;
> +	int err;
> +
> +	err = of_parse_phandle_with_args(cpu_node, "power-domains",
> +					"#power-domain-cells", 0, &args);
Should probably have an of_node_put to match this.
> +	if (!err) {
> +		dn = args.np;
> +		err = of_count_phandle_with_args(dn, "domain-idle-states",
> +								NULL);
> +		cpu_has_domain = (err > 0);

So if a CPU has a power domain but that domain doesn't have any idle
states, then we fall back to cpu-idle-states?

I think the presence of a power domain for a CPU should mean
cpu-idle-states is totally ignored, and this should be made clear in the
power_domain.txt binding doc.

(I have had this conversation with someone before, I think it was
 internal at ARM but if I'm wrong and we've already discussed it then
 I'm sorry!)
> +	}
> +
> +	if (cpu_has_domain) {
> +		property = "domain-idle-states";
> +	} else {
> +		property = "cpu-idle-states";
> +		dn = cpu_node;
> +	}
> +
> +	return of_parse_phandle(dn, property, idx);
> +}
> +
> +/*
>   * Check that the idle state is uniform across all CPUs in the CPUidle driver
>   * cpumask
>   */
> @@ -118,8 +151,7 @@ static bool idle_state_valid(struct device_node *state_node, unsigned int idx,
>  	for (cpu = cpumask_next(cpumask_first(cpumask), cpumask);
>  	     cpu < nr_cpu_ids; cpu = cpumask_next(cpu, cpumask)) {
>  		cpu_node = of_cpu_device_node_get(cpu);
> -		curr_state_node = of_parse_phandle(cpu_node, "cpu-idle-states",
> -						   idx);
> +		curr_state_node = get_state_node(cpu_node, idx);
>  		if (state_node != curr_state_node)
>  			valid = false;
>
> @@ -176,7 +208,7 @@ int dt_init_idle_driver(struct cpuidle_driver *drv,
>  	cpu_node = of_cpu_device_node_get(cpumask_first(cpumask));
>
>  	for (i = 0; ; i++) {
> -		state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
> +		state_node = get_state_node(cpu_node, i);
>  		if (!state_node)
>  			break;

This patch only supports using domain-idle-states for the CPU-level idle
states, i.e. it only looks at one level of the power-domains graph
rather than walking it and "linearising"/"flattening" the discovered
states into a cpuidle-friendly list.

That's not a reason against merging this patch but we should note the
limitation and maybe even print a warning in if we find a parent for the
CPU's power domain but we're using cpuidle rather than runtime PM.

Cheers,
Brendan