All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ulf Hansson <ulf.hansson@linaro.org>
To: Sudeep Holla <sudeep.holla@arm.com>,
	Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>,
	linux-pm@vger.kernel.org
Cc: "Rafael J . Wysocki" <rjw@rjwysocki.net>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Lina Iyer <ilina@codeaurora.org>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Stephen Boyd <sboyd@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Benjamin Gaignard <benjamin.gaignard@st.com>,
	Ulf Hansson <ulf.hansson@linaro.org>,
	linux-arm-kernel@lists.infradead.org
Subject: [PATCH v2 4/4] cpuidle: psci: Allow WFI to be the only state for the hierarchical topology
Date: Tue,  3 Mar 2020 21:35:59 +0100	[thread overview]
Message-ID: <20200303203559.23995-5-ulf.hansson@linaro.org> (raw)
In-Reply-To: <20200303203559.23995-1-ulf.hansson@linaro.org>

It's possible that only the WFI state is supported for the CPU, while also
a shared idle state exists for a group of CPUs.

When the hierarchical topology is used, the shared idle state may not be
compatible with arm,idle-state, rather with "domain-idle-state", which
makes dt_init_idle_driver() to return zero. This leads to that the
cpuidle-psci driver bails out during initialization, avoiding to register a
cpuidle driver and instead relies on the default architectural back-end
(called via cpu_do_idle()). In other words, the shared idle state becomes
unused.

Let's fix this behaviour, by allowing the dt_init_idle_driver() to return 0
and then continue with the initialization. If it turns out that the
hierarchical topology is used and we have some additional states to manage,
then continue with the cpuidle driver registration, otherwise bail out as
before.

Reported-by: Benjamin Gaignard <benjamin.gaignard@st.com>
Fixes: a65a397f2451 ("cpuidle: psci: Add support for PM domains by using genpd")
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v2:
	- Convert the error code returned from psci_cpu_suspend_enter() into an
	expected error code by cpuidle core.

---
 drivers/cpuidle/cpuidle-psci.c | 48 +++++++++++++++++++++-------------
 1 file changed, 30 insertions(+), 18 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
index bae9140a65a5..ae0fabec2742 100644
--- a/drivers/cpuidle/cpuidle-psci.c
+++ b/drivers/cpuidle/cpuidle-psci.c
@@ -56,16 +56,19 @@ static int psci_enter_domain_idle_state(struct cpuidle_device *dev,
 	u32 *states = data->psci_states;
 	struct device *pd_dev = data->dev;
 	u32 state;
-	int ret;
+	int ret = 0;
 
 	/* Do runtime PM to manage a hierarchical CPU toplogy. */
 	pm_runtime_put_sync_suspend(pd_dev);
 
 	state = psci_get_domain_state();
-	if (!state)
+	if (!state && states)
 		state = states[idx];
 
-	ret = psci_enter_state(idx, state);
+	if (state)
+		ret = psci_cpu_suspend_enter(state) ? -1 : idx;
+	else
+		cpu_do_idle();
 
 	pm_runtime_get_sync(pd_dev);
 
@@ -180,7 +183,7 @@ static int __init psci_dt_cpu_init_topology(struct cpuidle_driver *drv,
 	drv->states[state_count - 1].enter = psci_enter_domain_idle_state;
 	psci_cpuidle_use_cpuhp = true;
 
-	return 0;
+	return 1;
 }
 
 static int __init psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
@@ -192,6 +195,13 @@ static int __init psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
 	struct device_node *state_node;
 	struct psci_cpuidle_data *data = per_cpu_ptr(&psci_cpuidle_data, cpu);
 
+	/*
+	 * Special case when WFI is the only state, as we may still need to
+	 * initialize data, if the hierarchical topology is used.
+	 */
+	if (!state_count)
+		return psci_dt_cpu_init_topology(drv, data, 1, cpu);
+
 	state_count++; /* Add WFI state too */
 	psci_states = kcalloc(state_count, sizeof(*psci_states), GFP_KERNEL);
 	if (!psci_states)
@@ -223,7 +233,7 @@ static int __init psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
 
 	/* Idle states parsed correctly, store them in the per-cpu struct. */
 	data->psci_states = psci_states;
-	return 0;
+	return state_count;
 
 free_mem:
 	kfree(psci_states);
@@ -282,33 +292,35 @@ static int __init psci_idle_init_cpu(int cpu)
 		return -ENOMEM;
 
 	drv->cpumask = (struct cpumask *)cpumask_of(cpu);
+	drv->state_count = 1;
 
 	/*
-	 * Initialize idle states data, starting at index 1, since
-	 * by default idle state 0 is the quiescent state reached
-	 * by the cpu by executing the wfi instruction.
-	 *
-	 * If no DT idle states are detected (ret == 0) let the driver
-	 * initialization fail accordingly since there is no reason to
-	 * initialize the idle driver if only wfi is supported, the
-	 * default archictectural back-end already executes wfi
-	 * on idle entry.
+	 * Initialize idle states data, starting at index 1, since by default
+	 * idle state 0 is the quiescent state reached by the cpu by executing
+	 * the wfi instruction. If no DT idle states are detected (ret == 0),
+	 * we may still use the hierarchical topology.
 	 */
 	ret = dt_init_idle_driver(drv, psci_idle_state_match, 1);
-	if (ret <= 0) {
-		ret = ret ? : -ENODEV;
+	if (ret < 0)
 		goto out_kfree_drv;
-	}
 
 	/*
 	 * Initialize PSCI idle states.
 	 */
 	ret = psci_cpu_init_idle(drv, cpu, ret);
-	if (ret) {
+	if (ret < 0) {
 		pr_err("CPU %d failed to PSCI idle\n", cpu);
 		goto out_kfree_drv;
 	}
 
+	/* If there are no idle states to manage, but the wfi state and we also
+	 * don't use the hierarchical topology, let the driver initialization
+	 * fail. Instead, let's rely on the default architectural back-end to
+	 * execute wfi on idle entry.
+	 */
+	if (!ret)
+		goto out_kfree_drv;
+
 	ret = cpuidle_register(drv, NULL);
 	if (ret)
 		goto out_kfree_drv;
-- 
2.20.1


WARNING: multiple messages have this Message-ID (diff)
From: Ulf Hansson <ulf.hansson@linaro.org>
To: Sudeep Holla <sudeep.holla@arm.com>,
	Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>,
	linux-pm@vger.kernel.org
Cc: Ulf Hansson <ulf.hansson@linaro.org>,
	Benjamin Gaignard <benjamin.gaignard@st.com>,
	Stephen Boyd <sboyd@kernel.org>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	"Rafael J . Wysocki" <rjw@rjwysocki.net>,
	Lina Iyer <ilina@codeaurora.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	linux-arm-kernel@lists.infradead.org
Subject: [PATCH v2 4/4] cpuidle: psci: Allow WFI to be the only state for the hierarchical topology
Date: Tue,  3 Mar 2020 21:35:59 +0100	[thread overview]
Message-ID: <20200303203559.23995-5-ulf.hansson@linaro.org> (raw)
In-Reply-To: <20200303203559.23995-1-ulf.hansson@linaro.org>

It's possible that only the WFI state is supported for the CPU, while also
a shared idle state exists for a group of CPUs.

When the hierarchical topology is used, the shared idle state may not be
compatible with arm,idle-state, rather with "domain-idle-state", which
makes dt_init_idle_driver() to return zero. This leads to that the
cpuidle-psci driver bails out during initialization, avoiding to register a
cpuidle driver and instead relies on the default architectural back-end
(called via cpu_do_idle()). In other words, the shared idle state becomes
unused.

Let's fix this behaviour, by allowing the dt_init_idle_driver() to return 0
and then continue with the initialization. If it turns out that the
hierarchical topology is used and we have some additional states to manage,
then continue with the cpuidle driver registration, otherwise bail out as
before.

Reported-by: Benjamin Gaignard <benjamin.gaignard@st.com>
Fixes: a65a397f2451 ("cpuidle: psci: Add support for PM domains by using genpd")
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v2:
	- Convert the error code returned from psci_cpu_suspend_enter() into an
	expected error code by cpuidle core.

---
 drivers/cpuidle/cpuidle-psci.c | 48 +++++++++++++++++++++-------------
 1 file changed, 30 insertions(+), 18 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
index bae9140a65a5..ae0fabec2742 100644
--- a/drivers/cpuidle/cpuidle-psci.c
+++ b/drivers/cpuidle/cpuidle-psci.c
@@ -56,16 +56,19 @@ static int psci_enter_domain_idle_state(struct cpuidle_device *dev,
 	u32 *states = data->psci_states;
 	struct device *pd_dev = data->dev;
 	u32 state;
-	int ret;
+	int ret = 0;
 
 	/* Do runtime PM to manage a hierarchical CPU toplogy. */
 	pm_runtime_put_sync_suspend(pd_dev);
 
 	state = psci_get_domain_state();
-	if (!state)
+	if (!state && states)
 		state = states[idx];
 
-	ret = psci_enter_state(idx, state);
+	if (state)
+		ret = psci_cpu_suspend_enter(state) ? -1 : idx;
+	else
+		cpu_do_idle();
 
 	pm_runtime_get_sync(pd_dev);
 
@@ -180,7 +183,7 @@ static int __init psci_dt_cpu_init_topology(struct cpuidle_driver *drv,
 	drv->states[state_count - 1].enter = psci_enter_domain_idle_state;
 	psci_cpuidle_use_cpuhp = true;
 
-	return 0;
+	return 1;
 }
 
 static int __init psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
@@ -192,6 +195,13 @@ static int __init psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
 	struct device_node *state_node;
 	struct psci_cpuidle_data *data = per_cpu_ptr(&psci_cpuidle_data, cpu);
 
+	/*
+	 * Special case when WFI is the only state, as we may still need to
+	 * initialize data, if the hierarchical topology is used.
+	 */
+	if (!state_count)
+		return psci_dt_cpu_init_topology(drv, data, 1, cpu);
+
 	state_count++; /* Add WFI state too */
 	psci_states = kcalloc(state_count, sizeof(*psci_states), GFP_KERNEL);
 	if (!psci_states)
@@ -223,7 +233,7 @@ static int __init psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
 
 	/* Idle states parsed correctly, store them in the per-cpu struct. */
 	data->psci_states = psci_states;
-	return 0;
+	return state_count;
 
 free_mem:
 	kfree(psci_states);
@@ -282,33 +292,35 @@ static int __init psci_idle_init_cpu(int cpu)
 		return -ENOMEM;
 
 	drv->cpumask = (struct cpumask *)cpumask_of(cpu);
+	drv->state_count = 1;
 
 	/*
-	 * Initialize idle states data, starting at index 1, since
-	 * by default idle state 0 is the quiescent state reached
-	 * by the cpu by executing the wfi instruction.
-	 *
-	 * If no DT idle states are detected (ret == 0) let the driver
-	 * initialization fail accordingly since there is no reason to
-	 * initialize the idle driver if only wfi is supported, the
-	 * default archictectural back-end already executes wfi
-	 * on idle entry.
+	 * Initialize idle states data, starting at index 1, since by default
+	 * idle state 0 is the quiescent state reached by the cpu by executing
+	 * the wfi instruction. If no DT idle states are detected (ret == 0),
+	 * we may still use the hierarchical topology.
 	 */
 	ret = dt_init_idle_driver(drv, psci_idle_state_match, 1);
-	if (ret <= 0) {
-		ret = ret ? : -ENODEV;
+	if (ret < 0)
 		goto out_kfree_drv;
-	}
 
 	/*
 	 * Initialize PSCI idle states.
 	 */
 	ret = psci_cpu_init_idle(drv, cpu, ret);
-	if (ret) {
+	if (ret < 0) {
 		pr_err("CPU %d failed to PSCI idle\n", cpu);
 		goto out_kfree_drv;
 	}
 
+	/* If there are no idle states to manage, but the wfi state and we also
+	 * don't use the hierarchical topology, let the driver initialization
+	 * fail. Instead, let's rely on the default architectural back-end to
+	 * execute wfi on idle entry.
+	 */
+	if (!ret)
+		goto out_kfree_drv;
+
 	ret = cpuidle_register(drv, NULL);
 	if (ret)
 		goto out_kfree_drv;
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  parent reply	other threads:[~2020-03-03 20:36 UTC|newest]

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-03 20:35 [PATCH v2 0/4] cpuidle: psci: Some fixes when using the hierarchical layout Ulf Hansson
2020-03-03 20:35 ` Ulf Hansson
2020-03-03 20:35 ` [PATCH v2 1/4] PM / Domains: Allow no domain-idle-states DT property in genpd when parsing Ulf Hansson
2020-03-03 20:35   ` Ulf Hansson
2020-03-04 10:48   ` Sudeep Holla
2020-03-04 10:48     ` Sudeep Holla
2020-03-03 20:35 ` [PATCH v2 2/4] cpuidle: psci: Fixup support for domain idle states being zero Ulf Hansson
2020-03-03 20:35   ` Ulf Hansson
2020-03-04 10:50   ` Sudeep Holla
2020-03-04 10:50     ` Sudeep Holla
2020-03-04 12:17     ` Ulf Hansson
2020-03-04 12:17       ` Ulf Hansson
2020-03-03 20:35 ` [PATCH v2 3/4] cpuidle: psci: Split psci_dt_cpu_init_idle() Ulf Hansson
2020-03-03 20:35   ` Ulf Hansson
2020-03-04 12:12   ` Sudeep Holla
2020-03-04 12:12     ` Sudeep Holla
2020-03-04 12:20     ` Ulf Hansson
2020-03-04 12:20       ` Ulf Hansson
2020-03-03 20:35 ` Ulf Hansson [this message]
2020-03-03 20:35   ` [PATCH v2 4/4] cpuidle: psci: Allow WFI to be the only state for the hierarchical topology Ulf Hansson
2020-03-04 12:23   ` Sudeep Holla
2020-03-04 12:23     ` Sudeep Holla
2020-03-05 14:17     ` Ulf Hansson
2020-03-05 14:17       ` Ulf Hansson
2020-03-05 16:23       ` Sudeep Holla
2020-03-05 16:23         ` Sudeep Holla
2020-03-06  9:28         ` Ulf Hansson
2020-03-06  9:28           ` Ulf Hansson
2020-03-06 10:04           ` Sudeep Holla
2020-03-06 10:04             ` Sudeep Holla
2020-03-06 10:47             ` Benjamin Gaignard
2020-03-06 10:47               ` Benjamin Gaignard
2020-03-06 12:06               ` Sudeep Holla
2020-03-06 12:06                 ` Sudeep Holla
2020-03-06 12:32                 ` Benjamin Gaignard
2020-03-06 12:32                   ` Benjamin Gaignard
2020-03-06 14:23                   ` Sudeep Holla
2020-03-06 14:23                     ` Sudeep Holla
2020-03-06 14:44                     ` Benjamin Gaignard
2020-03-06 14:44                       ` Benjamin Gaignard
2020-03-06 14:50                       ` Sudeep Holla
2020-03-06 14:50                         ` Sudeep Holla
2020-03-06 15:35                         ` Benjamin Gaignard
2020-03-06 15:35                           ` Benjamin Gaignard
2020-03-06 15:55                           ` Sudeep Holla
2020-03-06 15:55                             ` Sudeep Holla
2020-03-03 22:27 ` [PATCH v2 0/4] cpuidle: psci: Some fixes when using the hierarchical layout Rafael J. Wysocki
2020-03-03 22:27   ` Rafael J. Wysocki
2020-03-09  7:20   ` Ulf Hansson
2020-03-09  7:20     ` Ulf Hansson
2020-03-10  8:37     ` Rafael J. Wysocki
2020-03-10  8:37       ` Rafael J. Wysocki

Reply instructions:

You may reply publicly 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=20200303203559.23995-5-ulf.hansson@linaro.org \
    --to=ulf.hansson@linaro.org \
    --cc=Lorenzo.Pieralisi@arm.com \
    --cc=benjamin.gaignard@st.com \
    --cc=bjorn.andersson@linaro.org \
    --cc=daniel.lezcano@linaro.org \
    --cc=ilina@codeaurora.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=rjw@rjwysocki.net \
    --cc=sboyd@kernel.org \
    --cc=sudeep.holla@arm.com \
    --cc=vincent.guittot@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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.