Linux-PM Archive on lore.kernel.org
 help / color / Atom feed
From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
To: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>,
	Mark Rutland <mark.rutland@arm.com>,
	linux-arm-kernel@lists.infradead.org,
	"Rafael J . Wysocki" <rjw@rjwysocki.net>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	"Raju P . L . S . S . S . N" <rplsssn@codeaurora.org>,
	Amit Kucheria <amit.kucheria@linaro.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Stephen Boyd <sboyd@kernel.org>,
	Niklas Cassel <niklas.cassel@linaro.org>,
	Tony Lindgren <tony@atomide.com>,
	Kevin Hilman <khilman@kernel.org>,
	Lina Iyer <ilina@codeaurora.org>,
	Viresh Kumar <viresh.kumar@linaro.org>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Souvik Chakravarty <souvik.chakravarty@arm.com>,
	linux-pm@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 10/18] drivers: firmware: psci: Add hierarchical domain idle states converter
Date: Tue, 9 Jul 2019 16:31:38 +0100
Message-ID: <20190709153138.GA22871@e121166-lin.cambridge.arm.com> (raw)
In-Reply-To: <20190513192300.653-11-ulf.hansson@linaro.org>

On Mon, May 13, 2019 at 09:22:52PM +0200, Ulf Hansson wrote:
> If the hierarchical CPU topology is used, but the OS initiated mode isn't
> supported, we need to rely solely on the regular cpuidle framework to
> manage the idle state selection, rather than using genpd and its governor.
> 
> For this reason, introduce a new PSCI DT helper function,
> psci_dt_pm_domains_parse_states(), which parses and converts the
> hierarchically described domain idle states from DT, into regular flattened
> cpuidle states. The converted states are added to the existing cpuidle
> driver's array of idle states, which make them available for cpuidle.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> 
> Changes:
> 	- Some simplification of the code.
> 
> ---
>  drivers/firmware/psci/psci.h           |   5 ++
>  drivers/firmware/psci/psci_pm_domain.c | 118 +++++++++++++++++++++++++
>  2 files changed, 123 insertions(+)
> 
> diff --git a/drivers/firmware/psci/psci.h b/drivers/firmware/psci/psci.h
> index 00d2e3dcef49..c36e0e6649e9 100644
> --- a/drivers/firmware/psci/psci.h
> +++ b/drivers/firmware/psci/psci.h
> @@ -3,6 +3,7 @@
>  #ifndef __PSCI_H
>  #define __PSCI_H
>  
> +struct cpuidle_driver;
>  struct device_node;
>  
>  int psci_set_osi_mode(void);
> @@ -13,8 +14,12 @@ void psci_set_domain_state(u32 state);
>  int psci_dt_parse_state_node(struct device_node *np, u32 *state);
>  #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
>  int psci_dt_init_pm_domains(struct device_node *np);
> +int psci_dt_pm_domains_parse_states(struct cpuidle_driver *drv,
> +		struct device_node *cpu_node, u32 *psci_states);
>  #else
>  static inline int psci_dt_init_pm_domains(struct device_node *np) { return 0; }
> +static inline int psci_dt_pm_domains_parse_states(struct cpuidle_driver *drv,
> +		struct device_node *cpu_node, u32 *psci_states) { return 0; }
>  #endif
>  #endif
>  
> diff --git a/drivers/firmware/psci/psci_pm_domain.c b/drivers/firmware/psci/psci_pm_domain.c
> index 3c6ca846caf4..3aa645dba81b 100644
> --- a/drivers/firmware/psci/psci_pm_domain.c
> +++ b/drivers/firmware/psci/psci_pm_domain.c
> @@ -14,6 +14,10 @@
>  #include <linux/pm_domain.h>
>  #include <linux/slab.h>
>  #include <linux/string.h>
> +#include <linux/cpuidle.h>
> +#include <linux/cpu_pm.h>
> +
> +#include <asm/cpuidle.h>
>  
>  #include "psci.h"
>  
> @@ -104,6 +108,53 @@ static void psci_pd_free_states(struct genpd_power_state *states,
>  	kfree(states);
>  }
>  
> +static int psci_pd_enter_pc(struct cpuidle_device *dev,
> +			struct cpuidle_driver *drv, int idx)
> +{
> +	return CPU_PM_CPU_IDLE_ENTER(arm_cpuidle_suspend, idx);
> +}
> +
> +static void psci_pd_enter_s2idle_pc(struct cpuidle_device *dev,
> +			struct cpuidle_driver *drv, int idx)
> +{
> +	psci_pd_enter_pc(dev, drv, idx);
> +}
> +
> +static void psci_pd_convert_states(struct cpuidle_state *idle_state,
> +			u32 *psci_state, struct genpd_power_state *state)
> +{
> +	u32 *state_data = state->data;
> +	u64 target_residency_us = state->residency_ns;
> +	u64 exit_latency_us = state->power_on_latency_ns +
> +			state->power_off_latency_ns;
> +
> +	*psci_state = *state_data;
> +	do_div(target_residency_us, 1000);
> +	idle_state->target_residency = target_residency_us;
> +	do_div(exit_latency_us, 1000);
> +	idle_state->exit_latency = exit_latency_us;
> +	idle_state->enter = &psci_pd_enter_pc;
> +	idle_state->enter_s2idle = &psci_pd_enter_s2idle_pc;
> +	idle_state->flags |= CPUIDLE_FLAG_TIMER_STOP;

This is arbitrary and not necessarily true.

I think that this patch is useful to represent my reservations about the
current approach. As a matter of fact, idle state entry will always be a
CPUidle decision.

You only need PM domain information to understand when all CPUs
in a power domain are actually idle but that's all genPD can do
in this respect.

I think this patchset would be much simpler if both CPUidle and
genPD governor would work on *one* set of idle states, globally
indexed (and that would be true for PSCI suspend parameters too).

To work with a unified set of idle states between CPUidle and genPD
(tossing some ideas around):

- We can implement a genPD CPUidle governor that in its select method
  takes into account genPD information (for instance by avoiding
  selection of idle states that require multiple cpus to be in idle
  to be effectively active)
- We can use genPD to enable/disable CPUidle states through runtime
  PM information

There may be other ways. My point is that current code, with two (or
more if the hierarchy grows) sets of idle states across two subsystems
(CPUidle and genPD) is not very well defined and honestly very hard to
grasp and prone to errors.

> +
> +	strncpy(idle_state->name, to_of_node(state->fwnode)->name,
> +		CPUIDLE_NAME_LEN - 1);
> +	strncpy(idle_state->desc, to_of_node(state->fwnode)->name,
> +		CPUIDLE_NAME_LEN - 1);
> +}
> +
> +static bool psci_pd_is_provider(struct device_node *np)
> +{
> +	struct psci_pd_provider *pd_prov, *it;
> +
> +	list_for_each_entry_safe(pd_prov, it, &psci_pd_providers, link) {
> +		if (pd_prov->node == np)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
>  static int psci_pd_init(struct device_node *np)
>  {
>  	struct generic_pm_domain *pd;
> @@ -265,4 +316,71 @@ int psci_dt_init_pm_domains(struct device_node *np)
>  	pr_err("failed to create CPU PM domains ret=%d\n", ret);
>  	return ret;
>  }
> +
> +int psci_dt_pm_domains_parse_states(struct cpuidle_driver *drv,
> +			struct device_node *cpu_node, u32 *psci_states)
> +{
> +	struct genpd_power_state *pd_states;
> +	struct of_phandle_args args;
> +	int ret, pd_state_count, i, state_idx, psci_idx;
> +	u32 cpu_psci_state = psci_states[drv->state_count - 2];

This (-2) is very dodgy and I doubt it would work on hierarchies going
above "cluster" level.

As I say above, I think we should work towards a single array of
idle states to be selected by a CPUidle governor using genPD
runtime information to bias the results according to the number
of CPUs in a genPD that entered/exit idle.

To be more precise, all idles states should be "domain-idle-state"
compatible, even the CPU ones, the distinction between what CPUidle
and genPD manage is a bit stretched IMO in this patchset.

We will have a chance to talk about this but I thought I would
comment publically if anyone else is willing to chime in, this
is not a PSCI problem at all, it is a CPUidle/genPD coexistence
design problem which is much broader.

Lorenzo

> +	struct device_node *np = of_node_get(cpu_node);
> +
> +
> +	/* Walk the CPU topology to find compatible domain idle states. */
> +	while (np) {
> +		ret = of_parse_phandle_with_args(np, "power-domains",
> +					"#power-domain-cells", 0, &args);
> +		of_node_put(np);
> +		if (ret)
> +			return 0;
> +
> +		np = args.np;
> +
> +		/* Verify that the node represents a psci pd provider. */
> +		if (!psci_pd_is_provider(np)) {
> +			of_node_put(np);
> +			return 0;
> +		}
> +
> +		/* Parse for compatible domain idle states. */
> +		ret = psci_pd_parse_states(np, &pd_states, &pd_state_count);
> +		if (ret) {
> +			of_node_put(np);
> +			return ret;
> +		}
> +
> +		i = 0;
> +		while (i < pd_state_count) {
> +
> +			state_idx = drv->state_count;
> +			if (state_idx >= CPUIDLE_STATE_MAX) {
> +				pr_warn("exceeding max cpuidle states\n");
> +				of_node_put(np);
> +				return 0;
> +			}
> +
> +			/* WFI state is not part of psci_states. */
> +			psci_idx = state_idx - 1 + i;
> +			psci_pd_convert_states(&drv->states[state_idx + i],
> +					&psci_states[psci_idx], &pd_states[i]);
> +
> +			/*
> +			 * In the hierarchical CPU topology the master PM domain
> +			 * idle state's DT property, "arm,psci-suspend-param",
> +			 * don't contain the bits for the idle state of the CPU,
> +			 * let's add those here.
> +			 */
> +			psci_states[psci_idx] |= cpu_psci_state;
> +			pr_debug("psci-power-state %#x index %d\n",
> +				psci_states[psci_idx], psci_idx);
> +
> +			drv->state_count++;
> +			i++;
> +		}
> +		psci_pd_free_states(pd_states, pd_state_count);
> +	}
> +
> +	return 0;
> +}
>  #endif
> -- 
> 2.17.1
> 

  parent reply index

Thread overview: 89+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-13 19:22 [PATCH 00/18] ARM/ARM64: Support hierarchical CPU arrangement for PSCI Ulf Hansson
2019-05-13 19:22 ` Ulf Hansson
2019-05-13 19:22 ` [PATCH 01/18] dt: psci: Update DT bindings to support hierarchical PSCI states Ulf Hansson
2019-05-13 19:22   ` Ulf Hansson
2019-07-19 11:29   ` Lorenzo Pieralisi
2019-05-13 19:22 ` [PATCH 02/18] of: base: Add of_get_cpu_state_node() to get idle states for a CPU node Ulf Hansson
2019-05-13 19:22   ` Ulf Hansson
2019-05-13 19:22 ` [PATCH 03/18] cpuidle: dt: Support hierarchical CPU idle states Ulf Hansson
2019-05-13 19:22   ` Ulf Hansson
2019-05-13 19:22 ` [PATCH 04/18] ARM/ARM64: cpuidle: Let back-end init ops take the driver as input Ulf Hansson
2019-05-13 19:22   ` Ulf Hansson
2019-06-07 15:00   ` Sudeep Holla
2019-06-10 10:20     ` Ulf Hansson
2019-05-13 19:22 ` [PATCH 05/18] drivers: firmware: psci: Simplify state node parsing Ulf Hansson
2019-05-13 19:22   ` Ulf Hansson
2019-06-07 15:01   ` Sudeep Holla
2019-05-13 19:22 ` [PATCH 06/18] drivers: firmware: psci: Support hierarchical CPU idle states Ulf Hansson
2019-05-13 19:22   ` Ulf Hansson
2019-06-07 15:03   ` Sudeep Holla
2019-05-13 19:22 ` [PATCH 07/18] drivers: firmware: psci: Prepare to use OS initiated suspend mode Ulf Hansson
2019-05-13 19:22   ` Ulf Hansson
2019-06-07 15:17   ` Sudeep Holla
2019-06-10 10:21     ` Ulf Hansson
2019-06-10 10:42       ` Sudeep Holla
2019-07-16 14:53   ` Sudeep Holla
2019-05-13 19:22 ` [PATCH 08/18] drivers: firmware: psci: Prepare to support PM domains Ulf Hansson
2019-05-13 19:22   ` Ulf Hansson
2019-06-07 15:21   ` Sudeep Holla
2019-05-13 19:22 ` [PATCH 09/18] drivers: firmware: psci: Add support for PM domains using genpd Ulf Hansson
2019-05-13 19:22   ` Ulf Hansson
2019-06-07 15:27   ` Sudeep Holla
2019-06-10 10:21     ` Ulf Hansson
2019-06-10 10:59       ` Sudeep Holla
2019-07-16 15:05   ` Sudeep Holla
2019-07-18 11:04     ` Ulf Hansson
2019-07-18 13:19       ` Sudeep Holla
2019-07-18 17:57         ` Lina Iyer
2019-07-19  9:45           ` Sudeep Holla
2019-05-13 19:22 ` [PATCH 10/18] drivers: firmware: psci: Add hierarchical domain idle states converter Ulf Hansson
2019-05-13 19:22   ` Ulf Hansson
2019-07-09 15:31   ` Lorenzo Pieralisi [this message]
2019-07-16  8:45     ` Ulf Hansson
2019-07-16 14:51       ` Lorenzo Pieralisi
2019-07-18 11:43         ` Ulf Hansson
2019-07-18 13:36           ` Lorenzo Pieralisi
2019-05-13 19:22 ` [PATCH 11/18] drivers: firmware: psci: Introduce psci_dt_topology_init() Ulf Hansson
2019-05-13 19:22   ` Ulf Hansson
2019-05-13 19:22 ` [PATCH 12/18] drivers: firmware: psci: Add a helper to attach a CPU to its PM domain Ulf Hansson
2019-05-13 19:22   ` Ulf Hansson
2019-05-13 19:22 ` [PATCH 13/18] drivers: firmware: psci: Attach the CPU's device " Ulf Hansson
2019-05-13 19:22   ` Ulf Hansson
2019-05-13 19:22 ` [PATCH 14/18] drivers: firmware: psci: Manage runtime PM in the idle path for CPUs Ulf Hansson
2019-05-13 19:22   ` Ulf Hansson
2019-07-16 15:53   ` Lorenzo Pieralisi
2019-07-18 10:35     ` Ulf Hansson
2019-07-18 13:30       ` Lorenzo Pieralisi
2019-07-18 16:54         ` Ulf Hansson
2019-07-18 17:41           ` Lina Iyer
2019-07-18 21:49             ` Ulf Hansson
2019-07-19 10:02               ` Lorenzo Pieralisi
2019-05-13 19:22 ` [PATCH 15/18] drivers: firmware: psci: Support CPU hotplug for the hierarchical model Ulf Hansson
2019-05-13 19:22   ` Ulf Hansson
2019-06-07 15:31   ` Sudeep Holla
2019-06-10 10:21     ` Ulf Hansson
2019-06-10 11:02       ` Sudeep Holla
2019-05-13 19:22 ` [PATCH 16/18] arm64: kernel: Respect the hierarchical CPU topology in DT for PSCI Ulf Hansson
2019-05-13 19:22   ` Ulf Hansson
2019-05-13 19:22 ` [PATCH 17/18] arm64: dts: Convert to the hierarchical CPU topology layout for MSM8916 Ulf Hansson
2019-05-13 19:22   ` Ulf Hansson
2019-07-16 14:47   ` Sudeep Holla
2019-07-16 20:36     ` Lina Iyer
2019-07-17 17:18       ` Sudeep Holla
2019-05-13 19:23 ` [PATCH 18/18] arm64: dts: hikey: Convert to the hierarchical CPU topology layout Ulf Hansson
2019-05-13 19:23   ` Ulf Hansson
2019-07-16 14:47   ` Sudeep Holla
2019-07-18 10:48     ` Ulf Hansson
2019-07-18 13:11       ` Sudeep Holla
2019-05-14  8:08 ` [PATCH 00/18] ARM/ARM64: Support hierarchical CPU arrangement for PSCI Rafael J. Wysocki
2019-05-14  8:08   ` Rafael J. Wysocki
2019-05-14  8:58   ` Ulf Hansson
2019-05-14  8:58     ` Ulf Hansson
2019-06-07 15:42     ` Sudeep Holla
2019-06-07 19:34       ` Bjorn Andersson
2019-06-10 10:32         ` Sudeep Holla
2019-06-10 15:54           ` Ulf Hansson
2019-06-10 17:16             ` Lorenzo Pieralisi
2019-06-10 18:57               ` Ulf Hansson
2019-06-18 11:56                 ` Ulf Hansson
2019-06-07 11:19 ` Ulf Hansson

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190709153138.GA22871@e121166-lin.cambridge.arm.com \
    --to=lorenzo.pieralisi@arm.com \
    --cc=amit.kucheria@linaro.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=daniel.lezcano@linaro.org \
    --cc=geert+renesas@glider.be \
    --cc=ilina@codeaurora.org \
    --cc=khilman@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=niklas.cassel@linaro.org \
    --cc=rjw@rjwysocki.net \
    --cc=rplsssn@codeaurora.org \
    --cc=sboyd@kernel.org \
    --cc=souvik.chakravarty@arm.com \
    --cc=sudeep.holla@arm.com \
    --cc=tony@atomide.com \
    --cc=ulf.hansson@linaro.org \
    --cc=vincent.guittot@linaro.org \
    --cc=viresh.kumar@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-PM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pm/0 linux-pm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pm linux-pm/ https://lore.kernel.org/linux-pm \
		linux-pm@vger.kernel.org linux-pm@archiver.kernel.org
	public-inbox-index linux-pm


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pm


AGPL code for this site: git clone https://public-inbox.org/ public-inbox