[PATCH v2 0/4] cpuidle: psci: Some fixes when using the hierarchical layout

[PATCH v2 0/4] cpuidle: psci: Some fixes when using the hierarchical layout

[Ulf Hansson]

Changes in v2:
	- Small changes to patch 3 and 4, see their changelogs.

While collaborating with Benjamin Gaignard to deploy the hierarchical layout
for an ST SoC, it has turned that I have clearly missed to test a couple of
corner cases in recently added support to the cpuidle-psci driver.

This series are fixing the issues we have found.

Kind regards
Ulf Hansson


Ulf Hansson (4):
  PM / Domains: Allow no domain-idle-states DT property in genpd when
    parsing
  cpuidle: psci: Fixup support for domain idle states being zero
  cpuidle: psci: Split psci_dt_cpu_init_idle()
  cpuidle: psci: Allow WFI to be the only state for the hierarchical
    topology

 drivers/base/power/domain.c           |  2 +-
 drivers/cpuidle/cpuidle-psci-domain.c |  6 ++
 drivers/cpuidle/cpuidle-psci.c        | 92 ++++++++++++++++-----------
 3 files changed, 63 insertions(+), 37 deletions(-)

-- 
2.20.1


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

[PATCH v2 1/4] PM / Domains: Allow no domain-idle-states DT property in genpd when parsing

[Ulf Hansson]

Commit 2c361684803e ("PM / Domains: Don't treat zero found compatible idle
states as an error"), moved of_genpd_parse_idle_states() towards allowing
none compatible idle state to be found for the device node, rather than
returning an error code.

However, it didn't consider that the "domain-idle-states" DT property may
be missing as it's optional, which makes of_count_phandle_with_args() to
return -ENOENT. Let's fix this to make the behaviour consistent.

Reported-by: Benjamin Gaignard <benjamin.gaignard@st.com>
Fixes: 2c361684803e ("PM / Domains: Don't treat zero found compatible idle states as an error")
Cc: <stable@vger.kernel.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 959d6d5eb000..0a01df608849 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -2653,7 +2653,7 @@ static int genpd_iterate_idle_states(struct device_node *dn,
 
 	ret = of_count_phandle_with_args(dn, "domain-idle-states", NULL);
 	if (ret <= 0)
-		return ret;
+		return ret == -ENOENT ? 0 : ret;
 
 	/* Loop over the phandles until all the requested entry is found */
 	of_for_each_phandle(&it, ret, dn, "domain-idle-states", NULL, 0) {
-- 
2.20.1


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

Re: [PATCH v2 1/4] PM / Domains: Allow no domain-idle-states DT property in genpd when parsing

[Sudeep Holla]

On Tue, Mar 03, 2020 at 09:35:56PM +0100, Ulf Hansson wrote:
> Commit 2c361684803e ("PM / Domains: Don't treat zero found compatible idle
> states as an error"), moved of_genpd_parse_idle_states() towards allowing
> none compatible idle state to be found for the device node, rather than
> returning an error code.
>
> However, it didn't consider that the "domain-idle-states" DT property may
> be missing as it's optional, which makes of_count_phandle_with_args() to
> return -ENOENT. Let's fix this to make the behaviour consistent.
>

Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

--
Regards,
Sudeep

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

[PATCH v2 2/4] cpuidle: psci: Fixup support for domain idle states being zero

[Ulf Hansson]

The current code intends to allow a PSCI PM domain to have none domain idle
states defined in DT. However, a few minor things needs to be fixed to make
this correctly supported, so let's do that.

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>
---
 drivers/cpuidle/cpuidle-psci-domain.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
index 423f03bbeb74..c34b12c4069a 100644
--- a/drivers/cpuidle/cpuidle-psci-domain.c
+++ b/drivers/cpuidle/cpuidle-psci-domain.c
@@ -49,6 +49,9 @@ static int __init psci_pd_parse_state_nodes(struct genpd_power_state *states,
 	int i, ret;
 	u32 psci_state, *psci_state_buf;
 
+	if (!states)
+		return 0;
+
 	for (i = 0; i < state_count; i++) {
 		ret = psci_dt_parse_state_node(to_of_node(states[i].fwnode),
 					&psci_state);
@@ -96,6 +99,9 @@ static void psci_pd_free_states(struct genpd_power_state *states,
 {
 	int i;
 
+	if (!states)
+		return;
+
 	for (i = 0; i < state_count; i++)
 		kfree(states[i].data);
 	kfree(states);
-- 
2.20.1


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

Re: [PATCH v2 2/4] cpuidle: psci: Fixup support for domain idle states being zero

[Sudeep Holla]

On Tue, Mar 03, 2020 at 09:35:57PM +0100, Ulf Hansson wrote:
> The current code intends to allow a PSCI PM domain to have none domain idle
> states defined in DT. However, a few minor things needs to be fixed to make
> this correctly supported, so let's do that.
>
> 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>
> ---
>  drivers/cpuidle/cpuidle-psci-domain.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
> index 423f03bbeb74..c34b12c4069a 100644
> --- a/drivers/cpuidle/cpuidle-psci-domain.c
> +++ b/drivers/cpuidle/cpuidle-psci-domain.c
> @@ -49,6 +49,9 @@ static int __init psci_pd_parse_state_nodes(struct genpd_power_state *states,
>  	int i, ret;
>  	u32 psci_state, *psci_state_buf;
>
> +	if (!states)
> +		return 0;
> +

Was any issue found ? Or just code inspection ? If states = NULL,
state_count = 0, and I don't see anything blowing up. It may save couple
of extra instruction execution.

>  	for (i = 0; i < state_count; i++) {
>  		ret = psci_dt_parse_state_node(to_of_node(states[i].fwnode),
>  					&psci_state);
> @@ -96,6 +99,9 @@ static void psci_pd_free_states(struct genpd_power_state *states,
>  {
>  	int i;
>
> +	if (!states)
> +		return;
> +

Same here and kfree(NULL) is also valid.

--
Regards,
Sudeep

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

Re: [PATCH v2 2/4] cpuidle: psci: Fixup support for domain idle states being zero

[Ulf Hansson]

On Wed, 4 Mar 2020 at 11:50, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Tue, Mar 03, 2020 at 09:35:57PM +0100, Ulf Hansson wrote:
> > The current code intends to allow a PSCI PM domain to have none domain idle
> > states defined in DT. However, a few minor things needs to be fixed to make
> > this correctly supported, so let's do that.
> >
> > 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>
> > ---
> >  drivers/cpuidle/cpuidle-psci-domain.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
> > index 423f03bbeb74..c34b12c4069a 100644
> > --- a/drivers/cpuidle/cpuidle-psci-domain.c
> > +++ b/drivers/cpuidle/cpuidle-psci-domain.c
> > @@ -49,6 +49,9 @@ static int __init psci_pd_parse_state_nodes(struct genpd_power_state *states,
> >       int i, ret;
> >       u32 psci_state, *psci_state_buf;
> >
> > +     if (!states)
> > +             return 0;
> > +
>
> Was any issue found ? Or just code inspection ? If states = NULL,
> state_count = 0, and I don't see anything blowing up. It may save couple
> of extra instruction execution.

Code inspection, the real problem was fixed in patch 1.

>
> >       for (i = 0; i < state_count; i++) {
> >               ret = psci_dt_parse_state_node(to_of_node(states[i].fwnode),
> >                                       &psci_state);
> > @@ -96,6 +99,9 @@ static void psci_pd_free_states(struct genpd_power_state *states,
> >  {
> >       int i;
> >
> > +     if (!states)
> > +             return;
> > +
>
> Same here and kfree(NULL) is also valid.

Yep, let's drop $subject patch from the series, it's not needed.

Thanks for reviewing!

Kind regards
Uffe

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

[PATCH v2 3/4] cpuidle: psci: Split psci_dt_cpu_init_idle()

[Ulf Hansson]

To make the code a bit more readable, but also to prepare some code to be
re-used, let's move the OSI specific initialization out of the
psci_dt_cpu_init_idle() and into a separate function.

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

Changes in v2:
	- Adopted suggestions from Stephen to use IS_ERR_OR_NULL and
	PTR_ERR_OR_ZERO, which further clarified the code.

---
 drivers/cpuidle/cpuidle-psci.c | 46 ++++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 19 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
index edd7a54ef0d3..bae9140a65a5 100644
--- a/drivers/cpuidle/cpuidle-psci.c
+++ b/drivers/cpuidle/cpuidle-psci.c
@@ -160,6 +160,29 @@ int __init psci_dt_parse_state_node(struct device_node *np, u32 *state)
 	return 0;
 }
 
+static int __init psci_dt_cpu_init_topology(struct cpuidle_driver *drv,
+					    struct psci_cpuidle_data *data,
+					    unsigned int state_count, int cpu)
+{
+	/* Currently limit the hierarchical topology to be used in OSI mode. */
+	if (!psci_has_osi_support())
+		return 0;
+
+	data->dev = psci_dt_attach_cpu(cpu);
+	if (IS_ERR_OR_NULL(data->dev))
+		return PTR_ERR_OR_ZERO(data->dev);
+
+	/*
+	 * Using the deepest state for the CPU to trigger a potential selection
+	 * of a shared state for the domain, assumes the domain states are all
+	 * deeper states.
+	 */
+	drv->states[state_count - 1].enter = psci_enter_domain_idle_state;
+	psci_cpuidle_use_cpuhp = true;
+
+	return 0;
+}
+
 static int __init psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
 					struct device_node *cpu_node,
 					unsigned int state_count, int cpu)
@@ -193,25 +216,10 @@ static int __init psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
 		goto free_mem;
 	}
 
-	/* Currently limit the hierarchical topology to be used in OSI mode. */
-	if (psci_has_osi_support()) {
-		data->dev = psci_dt_attach_cpu(cpu);
-		if (IS_ERR(data->dev)) {
-			ret = PTR_ERR(data->dev);
-			goto free_mem;
-		}
-
-		/*
-		 * Using the deepest state for the CPU to trigger a potential
-		 * selection of a shared state for the domain, assumes the
-		 * domain states are all deeper states.
-		 */
-		if (data->dev) {
-			drv->states[state_count - 1].enter =
-				psci_enter_domain_idle_state;
-			psci_cpuidle_use_cpuhp = true;
-		}
-	}
+	/* Initialize optional data, used for the hierarchical topology. */
+	ret = psci_dt_cpu_init_topology(drv, data, state_count, cpu);
+	if (ret < 0)
+		goto free_mem;
 
 	/* Idle states parsed correctly, store them in the per-cpu struct. */
 	data->psci_states = psci_states;
-- 
2.20.1


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

Re: [PATCH v2 3/4] cpuidle: psci: Split psci_dt_cpu_init_idle()

[Sudeep Holla]

On Tue, Mar 03, 2020 at 09:35:58PM +0100, Ulf Hansson wrote:
> To make the code a bit more readable, but also to prepare some code to be
> re-used, let's move the OSI specific initialization out of the
> psci_dt_cpu_init_idle() and into a separate function.
>
> Fixes: a65a397f2451 ("cpuidle: psci: Add support for PM domains by using genpd")

Not sure if this fixes anything but I am fine to have this if next one is
a real fix.

> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>
> Changes in v2:
> 	- Adopted suggestions from Stephen to use IS_ERR_OR_NULL and
> 	PTR_ERR_OR_ZERO, which further clarified the code.
>
> ---
>  drivers/cpuidle/cpuidle-psci.c | 46 ++++++++++++++++++++--------------
>  1 file changed, 27 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> index edd7a54ef0d3..bae9140a65a5 100644
> --- a/drivers/cpuidle/cpuidle-psci.c
> +++ b/drivers/cpuidle/cpuidle-psci.c
> @@ -160,6 +160,29 @@ int __init psci_dt_parse_state_node(struct device_node *np, u32 *state)
>  	return 0;
>  }
>
> +static int __init psci_dt_cpu_init_topology(struct cpuidle_driver *drv,
> +					    struct psci_cpuidle_data *data,
> +					    unsigned int state_count, int cpu)
> +{
> +	/* Currently limit the hierarchical topology to be used in OSI mode. */
> +	if (!psci_has_osi_support())
> +		return 0;
> +
> +	data->dev = psci_dt_attach_cpu(cpu);
> +	if (IS_ERR_OR_NULL(data->dev))
> +		return PTR_ERR_OR_ZERO(data->dev);
> +

This is what I was asking to do before this was merged when I meant to drop
if(data->dev) check. So happy to see it :)

Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

--
Regards,
Sudeep

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

Re: [PATCH v2 3/4] cpuidle: psci: Split psci_dt_cpu_init_idle()

[Ulf Hansson]

On Wed, 4 Mar 2020 at 13:12, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Tue, Mar 03, 2020 at 09:35:58PM +0100, Ulf Hansson wrote:
> > To make the code a bit more readable, but also to prepare some code to be
> > re-used, let's move the OSI specific initialization out of the
> > psci_dt_cpu_init_idle() and into a separate function.
> >
> > Fixes: a65a397f2451 ("cpuidle: psci: Add support for PM domains by using genpd")
>
> Not sure if this fixes anything but I am fine to have this if next one is
> a real fix.

Yep, that's what I had in mind as well.

>
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >
> > Changes in v2:
> >       - Adopted suggestions from Stephen to use IS_ERR_OR_NULL and
> >       PTR_ERR_OR_ZERO, which further clarified the code.
> >
> > ---
> >  drivers/cpuidle/cpuidle-psci.c | 46 ++++++++++++++++++++--------------
> >  1 file changed, 27 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> > index edd7a54ef0d3..bae9140a65a5 100644
> > --- a/drivers/cpuidle/cpuidle-psci.c
> > +++ b/drivers/cpuidle/cpuidle-psci.c
> > @@ -160,6 +160,29 @@ int __init psci_dt_parse_state_node(struct device_node *np, u32 *state)
> >       return 0;
> >  }
> >
> > +static int __init psci_dt_cpu_init_topology(struct cpuidle_driver *drv,
> > +                                         struct psci_cpuidle_data *data,
> > +                                         unsigned int state_count, int cpu)
> > +{
> > +     /* Currently limit the hierarchical topology to be used in OSI mode. */
> > +     if (!psci_has_osi_support())
> > +             return 0;
> > +
> > +     data->dev = psci_dt_attach_cpu(cpu);
> > +     if (IS_ERR_OR_NULL(data->dev))
> > +             return PTR_ERR_OR_ZERO(data->dev);
> > +
>
> This is what I was asking to do before this was merged when I meant to drop
> if(data->dev) check. So happy to see it :)

I probably didn't get you point well enough. Sorry.

>
> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

Thanks!

Kind regards
Uffe

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

[PATCH v2 4/4] cpuidle: psci: Allow WFI to be the only state for the hierarchical topology

[Ulf Hansson]

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

Re: [PATCH v2 4/4] cpuidle: psci: Allow WFI to be the only state for the hierarchical topology

[Sudeep Holla]

The $subject is bit confusing. IIUC, if there are no idle states to
manage including hierarchical domain states you will not register the driver
right ? If so, you are not allowing WFI to be the only state, hence my
concern with $subject.

On Tue, Mar 03, 2020 at 09:35:59PM +0100, Ulf Hansson wrote:
> 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();

May be, I haven't followed this completely yet, but I don't want to be
in the position to replicated default arch idle hook. Just use the one
that exist by simply not registering the driver.

--
Regards,
Sudeep

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

Re: [PATCH v2 4/4] cpuidle: psci: Allow WFI to be the only state for the hierarchical topology

[Ulf Hansson]

On Wed, 4 Mar 2020 at 13:23, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> The $subject is bit confusing. IIUC, if there are no idle states to
> manage including hierarchical domain states you will not register the driver
> right ? If so, you are not allowing WFI to be the only state, hence my
> concern with $subject.

I agree that's not so clear, but it wasn't easy to fit everything I
wanted to say in one line. :-)

Is this below better and okay for you?

"cpuidle: psci: Update condition when avoiding driver registration".

>
> On Tue, Mar 03, 2020 at 09:35:59PM +0100, Ulf Hansson wrote:
> > 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();
>
> May be, I haven't followed this completely yet, but I don't want to be
> in the position to replicated default arch idle hook. Just use the one
> that exist by simply not registering the driver.

That doesn't work for the configuration I am solving.

Assume this scenario: We have WFI and a domain/cluster idle state.
From the cpuidle governor point of view, it always selects the WFI
state, which means idx is zero.

Then, after we have called pm_runtime_put_sync_suspend() a few lines
above, we may potentially have a "domain state" to use, instead of the
WFI state.

In this case, if we would have called psci_enter_state(), that would
lead us to calling cpu_do_idle() from the __CPU_PM_CPU_IDLE_ENTER()
macro, becuase idx is zero. In other words, the domain state would
become unused.

Hope this clarifies what goes on here?

Kind regards
Uffe

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

Re: [PATCH v2 4/4] cpuidle: psci: Allow WFI to be the only state for the hierarchical topology

[Sudeep Holla]

On Thu, Mar 05, 2020 at 03:17:42PM +0100, Ulf Hansson wrote:
> On Wed, 4 Mar 2020 at 13:23, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > The $subject is bit confusing. IIUC, if there are no idle states to
> > manage including hierarchical domain states you will not register the driver
> > right ? If so, you are not allowing WFI to be the only state, hence my
> > concern with $subject.
>
> I agree that's not so clear, but it wasn't easy to fit everything I
> wanted to say in one line. :-)
>

No worries, just wanted to clarified. Looking at the patch, lot of things
got clarified but thought we can always improve.

> Is this below better and okay for you?
>
> "cpuidle: psci: Update condition when avoiding driver registration".
>

Definitely better than $subject :)

> >
> > On Tue, Mar 03, 2020 at 09:35:59PM +0100, Ulf Hansson wrote:
> > > 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();
> >
> > May be, I haven't followed this completely yet, but I don't want to be
> > in the position to replicated default arch idle hook. Just use the one
> > that exist by simply not registering the driver.
>
> That doesn't work for the configuration I am solving.
>
> Assume this scenario: We have WFI and a domain/cluster idle state.
> From the cpuidle governor point of view, it always selects the WFI
> state, which means idx is zero.
>

OK. The only state that cluster can enter when CPUs are in WFI are
cluster WFI and most hardware can handle it automatically. I don't see
the need to do any extra work for that.

> Then, after we have called pm_runtime_put_sync_suspend() a few lines
> above, we may potentially have a "domain state" to use, instead of the
> WFI state.
>

Are they any platforms with this potential "domain state" to use with
CPU WFI. I want to understand this better.

> In this case, if we would have called psci_enter_state(), that would
> lead us to calling cpu_do_idle() from the __CPU_PM_CPU_IDLE_ENTER()
> macro, becuase idx is zero. In other words, the domain state would
> become unused.
>

For a domain state to become unused with WFI, it needs to be available
and I am not 100% sure of that.

> Hope this clarifies what goes on here?
>

Yes.

--
Regards,
Sudeep

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

Re: [PATCH v2 4/4] cpuidle: psci: Allow WFI to be the only state for the hierarchical topology

[Ulf Hansson]

On Thu, 5 Mar 2020 at 17:23, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Mar 05, 2020 at 03:17:42PM +0100, Ulf Hansson wrote:
> > On Wed, 4 Mar 2020 at 13:23, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > The $subject is bit confusing. IIUC, if there are no idle states to
> > > manage including hierarchical domain states you will not register the driver
> > > right ? If so, you are not allowing WFI to be the only state, hence my
> > > concern with $subject.
> >
> > I agree that's not so clear, but it wasn't easy to fit everything I
> > wanted to say in one line. :-)
> >
>
> No worries, just wanted to clarified. Looking at the patch, lot of things
> got clarified but thought we can always improve.
>
> > Is this below better and okay for you?
> >
> > "cpuidle: psci: Update condition when avoiding driver registration".
> >
>
> Definitely better than $subject :)

Great, then I switch to that.

>
> > >
> > > On Tue, Mar 03, 2020 at 09:35:59PM +0100, Ulf Hansson wrote:
> > > > 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();
> > >
> > > May be, I haven't followed this completely yet, but I don't want to be
> > > in the position to replicated default arch idle hook. Just use the one
> > > that exist by simply not registering the driver.
> >
> > That doesn't work for the configuration I am solving.
> >
> > Assume this scenario: We have WFI and a domain/cluster idle state.
> > From the cpuidle governor point of view, it always selects the WFI
> > state, which means idx is zero.
> >
>
> OK. The only state that cluster can enter when CPUs are in WFI are
> cluster WFI and most hardware can handle it automatically. I don't see
> the need to do any extra work for that.

This isn't about cluster WFI, but about deeper cluster states, such as
a cluster-clock-gated-state and a cluster-power-off-state. It's an ST
platform, which Benjamin is working on.

>
> > Then, after we have called pm_runtime_put_sync_suspend() a few lines
> > above, we may potentially have a "domain state" to use, instead of the
> > WFI state.
> >
>
> Are they any platforms with this potential "domain state" to use with
> CPU WFI. I want to understand this better.
>
> > In this case, if we would have called psci_enter_state(), that would
> > lead us to calling cpu_do_idle() from the __CPU_PM_CPU_IDLE_ENTER()
> > macro, becuase idx is zero. In other words, the domain state would
> > become unused.
> >
>
> For a domain state to become unused with WFI, it needs to be available
> and I am not 100% sure of that.

With these changes from the series, we can fully conform to the
hierarchical DT bindings for PSCI.

I am not sure I understand your concern, is there a cost involved by
applying this?

Kind regards
Uffe

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

Re: [PATCH v2 4/4] cpuidle: psci: Allow WFI to be the only state for the hierarchical topology

[Sudeep Holla]

On Fri, Mar 06, 2020 at 10:28:10AM +0100, Ulf Hansson wrote:
> On Thu, 5 Mar 2020 at 17:23, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >

[...]

> > OK. The only state that cluster can enter when CPUs are in WFI are
> > cluster WFI and most hardware can handle it automatically. I don't see
> > the need to do any extra work for that.
>
> This isn't about cluster WFI, but about deeper cluster states, such as
> a cluster-clock-gated-state and a cluster-power-off-state. It's an ST
> platform, which Benjamin is working on.
>

Then definitely something is completely wrong. You can't enter deeper
cluster states(clock-gated and power-off to be specific) with CPU in
just WFI state. So, if the attempt here is to enter those states, I
disagree with the change.

Benjamin, please share the complete hierarchical topology for your platform.

> >
> > > Then, after we have called pm_runtime_put_sync_suspend() a few lines
> > > above, we may potentially have a "domain state" to use, instead of the
> > > WFI state.
> > >
> >
> > Are they any platforms with this potential "domain state" to use with
> > CPU WFI. I want to understand this better.
> >
> > > In this case, if we would have called psci_enter_state(), that would
> > > lead us to calling cpu_do_idle() from the __CPU_PM_CPU_IDLE_ENTER()
> > > macro, becuase idx is zero. In other words, the domain state would
> > > become unused.
> > >
> >
> > For a domain state to become unused with WFI, it needs to be available
> > and I am not 100% sure of that.
>
> With these changes from the series, we can fully conform to the
> hierarchical DT bindings for PSCI.
>

Theoretically may be, but may not confirm to the hardware states.

> I am not sure I understand your concern, is there a cost involved by
> applying this?
>

Yes as mentioned above.

--
Regards,
Sudeep

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

Re: [PATCH v2 4/4] cpuidle: psci: Allow WFI to be the only state for the hierarchical topology

[Benjamin Gaignard]

Le ven. 6 mars 2020 à 11:04, Sudeep Holla <sudeep.holla@arm.com> a écrit :
>
> On Fri, Mar 06, 2020 at 10:28:10AM +0100, Ulf Hansson wrote:
> > On Thu, 5 Mar 2020 at 17:23, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
>
> [...]
>
> > > OK. The only state that cluster can enter when CPUs are in WFI are
> > > cluster WFI and most hardware can handle it automatically. I don't see
> > > the need to do any extra work for that.
> >
> > This isn't about cluster WFI, but about deeper cluster states, such as
> > a cluster-clock-gated-state and a cluster-power-off-state. It's an ST
> > platform, which Benjamin is working on.
> >
>
> Then definitely something is completely wrong. You can't enter deeper
> cluster states(clock-gated and power-off to be specific) with CPU in
> just WFI state. So, if the attempt here is to enter those states, I
> disagree with the change.
>
> Benjamin, please share the complete hierarchical topology for your platform.

The platform is stm32mp157 SoC which embedded two Cortex A7 in one cluster.
I would like to be able to put the system in a state where clocks of CPUs and
hardware blocks are gated. In this state local timer are off.
The platform should be allowed to go in this state when the devices
within the power
domain are pm_runtime_suspend and the CPUs in WFI.
In DT I have one system power domain where the hardware blocks (i2,
uart; spi, etc..)
are attached + a power per CPU.

Benjamin

>
> > >
> > > > Then, after we have called pm_runtime_put_sync_suspend() a few lines
> > > > above, we may potentially have a "domain state" to use, instead of the
> > > > WFI state.
> > > >
> > >
> > > Are they any platforms with this potential "domain state" to use with
> > > CPU WFI. I want to understand this better.
> > >
> > > > In this case, if we would have called psci_enter_state(), that would
> > > > lead us to calling cpu_do_idle() from the __CPU_PM_CPU_IDLE_ENTER()
> > > > macro, becuase idx is zero. In other words, the domain state would
> > > > become unused.
> > > >
> > >
> > > For a domain state to become unused with WFI, it needs to be available
> > > and I am not 100% sure of that.
> >
> > With these changes from the series, we can fully conform to the
> > hierarchical DT bindings for PSCI.
> >
>
> Theoretically may be, but may not confirm to the hardware states.
>
> > I am not sure I understand your concern, is there a cost involved by
> > applying this?
> >
>
> Yes as mentioned above.
>
> --
> Regards,
> Sudeep
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

Re: [PATCH v2 4/4] cpuidle: psci: Allow WFI to be the only state for the hierarchical topology

[Sudeep Holla]

On Fri, Mar 06, 2020 at 11:47:40AM +0100, Benjamin Gaignard wrote:
> Le ven. 6 mars 2020 à 11:04, Sudeep Holla <sudeep.holla@arm.com> a écrit :
> >
> > On Fri, Mar 06, 2020 at 10:28:10AM +0100, Ulf Hansson wrote:
> > > On Thu, 5 Mar 2020 at 17:23, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > >
> >
> > [...]
> >
> > > > OK. The only state that cluster can enter when CPUs are in WFI are
> > > > cluster WFI and most hardware can handle it automatically. I don't see
> > > > the need to do any extra work for that.
> > >
> > > This isn't about cluster WFI, but about deeper cluster states, such as
> > > a cluster-clock-gated-state and a cluster-power-off-state. It's an ST
> > > platform, which Benjamin is working on.
> > >
> >
> > Then definitely something is completely wrong. You can't enter deeper
> > cluster states(clock-gated and power-off to be specific) with CPU in
> > just WFI state. So, if the attempt here is to enter those states, I
> > disagree with the change.
> >
> > Benjamin, please share the complete hierarchical topology for your platform.
>
> The platform is stm32mp157 SoC which embedded two Cortex A7 in one cluster.

Hang on a minute, is this the same platform where you wanted high
resolution timer and were hacking moving dirty tricks around[1]. Now I think
you have landed here.

> I would like to be able to put the system in a state where clocks of CPUs and
> hardware blocks are gated. In this state local timer are off.

Sure, please create a deeper CPU state than WFI and enter so that the CPU
state is saved and restored correctly. What is the problem doing that ?

> The platform should be allowed to go in this state when the devices
> within the power domain are pm_runtime_suspend and the CPUs in WFI.

Nope, we don't save and restore state when we enter/exit WFI. And hence
we can't allow deeper idle states in the hierarchy. No more discussion
on that.

> In DT I have one system power domain where the hardware blocks (i2,
> uart; spi, etc..) are attached + a power per CPU.

You really need a CPU idle state here.

--
Regards,
Sudeep

[1] https://lore.kernel.org/linux-arm-kernel/a42dd20677cddd8d09ea91a369a4e10b@www.loen.fr/

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

Re: [PATCH v2 4/4] cpuidle: psci: Allow WFI to be the only state for the hierarchical topology

[Benjamin Gaignard]

Le ven. 6 mars 2020 à 13:06, Sudeep Holla <sudeep.holla@arm.com> a écrit :
>
> On Fri, Mar 06, 2020 at 11:47:40AM +0100, Benjamin Gaignard wrote:
> > Le ven. 6 mars 2020 à 11:04, Sudeep Holla <sudeep.holla@arm.com> a écrit :
> > >
> > > On Fri, Mar 06, 2020 at 10:28:10AM +0100, Ulf Hansson wrote:
> > > > On Thu, 5 Mar 2020 at 17:23, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > > >
> > >
> > > [...]
> > >
> > > > > OK. The only state that cluster can enter when CPUs are in WFI are
> > > > > cluster WFI and most hardware can handle it automatically. I don't see
> > > > > the need to do any extra work for that.
> > > >
> > > > This isn't about cluster WFI, but about deeper cluster states, such as
> > > > a cluster-clock-gated-state and a cluster-power-off-state. It's an ST
> > > > platform, which Benjamin is working on.
> > > >
> > >
> > > Then definitely something is completely wrong. You can't enter deeper
> > > cluster states(clock-gated and power-off to be specific) with CPU in
> > > just WFI state. So, if the attempt here is to enter those states, I
> > > disagree with the change.
> > >
> > > Benjamin, please share the complete hierarchical topology for your platform.
> >
> > The platform is stm32mp157 SoC which embedded two Cortex A7 in one cluster.
>
> Hang on a minute, is this the same platform where you wanted high
> resolution timer and were hacking moving dirty tricks around[1]. Now I think
> you have landed here.

yes it has been fixed in this commit:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/arm/kernel/time.c?h=v5.6-rc4&id=022eb8ae8b5ee8c5c813923c69b5ebb1e9612c4c

>
> > I would like to be able to put the system in a state where clocks of CPUs and
> > hardware blocks are gated. In this state local timer are off.
>
> Sure, please create a deeper CPU state than WFI and enter so that the CPU
> state is saved and restored correctly. What is the problem doing that ?

This state stop the clocks for all the hardware blocks and not only the CPUs
so we can't go on it while devices aren't suspended.
I may have missed something but I don't believe that I could add this kind of
conditions in a cpu idle state, right ?
In this state I need to be able to enable the wake up sources because
it is the only
for hardware block used as broadcast timer to wake up the system.

>
> > The platform should be allowed to go in this state when the devices
> > within the power domain are pm_runtime_suspend and the CPUs in WFI.
>
> Nope, we don't save and restore state when we enter/exit WFI. And hence
> we can't allow deeper idle states in the hierarchy. No more discussion
> on that.

>
> > In DT I have one system power domain where the hardware blocks (i2,
> > uart; spi, etc..) are attached + a power per CPU.
>
> You really need a CPU idle state here.
>
> --
> Regards,
> Sudeep
>
> [1] https://lore.kernel.org/linux-arm-kernel/a42dd20677cddd8d09ea91a369a4e10b@www.loen.fr/

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

Re: [PATCH v2 4/4] cpuidle: psci: Allow WFI to be the only state for the hierarchical topology

[Sudeep Holla]

On Fri, Mar 06, 2020 at 01:32:59PM +0100, Benjamin Gaignard wrote:
> Le ven. 6 mars 2020 à 13:06, Sudeep Holla <sudeep.holla@arm.com> a écrit :
> >

[...]

> > Sure, please create a deeper CPU state than WFI and enter so that the CPU
> > state is saved and restored correctly. What is the problem doing that ?
>
> This state stop the clocks for all the hardware blocks and not only the CPUs
> so we can't go on it while devices aren't suspended.
> I may have missed something but I don't believe that I could add this kind of
> conditions in a cpu idle state, right ?
> In this state I need to be able to enable the wake up sources because
> it is the only
> for hardware block used as broadcast timer to wake up the system.
>

We have discussed this in past in the thread I mentioned and may be
others too. It sounds like a broken hardware, sorry if I am wrong.
But this $subject patch is a hack to solve that and I am NACK-ing this
now. Please fix it adding another CPU level idle state, we are not
supporting without that and there is absolutely no need to.

--
Regards,
Sudeep

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

Re: [PATCH v2 4/4] cpuidle: psci: Allow WFI to be the only state for the hierarchical topology

[Benjamin Gaignard]

Le ven. 6 mars 2020 à 15:23, Sudeep Holla <sudeep.holla@arm.com> a écrit :
>
> On Fri, Mar 06, 2020 at 01:32:59PM +0100, Benjamin Gaignard wrote:
> > Le ven. 6 mars 2020 à 13:06, Sudeep Holla <sudeep.holla@arm.com> a écrit :
> > >
>
> [...]
>
> > > Sure, please create a deeper CPU state than WFI and enter so that the CPU
> > > state is saved and restored correctly. What is the problem doing that ?
> >
> > This state stop the clocks for all the hardware blocks and not only the CPUs
> > so we can't go on it while devices aren't suspended.
> > I may have missed something but I don't believe that I could add this kind of
> > conditions in a cpu idle state, right ?
> > In this state I need to be able to enable the wake up sources because
> > it is the only
> > for hardware block used as broadcast timer to wake up the system.
> >
>
> We have discussed this in past in the thread I mentioned and may be
> others too. It sounds like a broken hardware, sorry if I am wrong.
> But this $subject patch is a hack to solve that and I am NACK-ing this
> now. Please fix it adding another CPU level idle state, we are not
> supporting without that and there is absolutely no need to.

A CPU idle state only take care of CPU activities, right ? but before going in
the targeting state I need to be sure that the other hardware blocks
are suspended.
Is it possible to describe that in an idle state ?
What sound broken ? is it because we need to setup the wake up sources ?

>
> --
> Regards,
> Sudeep

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

Re: [PATCH v2 4/4] cpuidle: psci: Allow WFI to be the only state for the hierarchical topology

[Sudeep Holla]

On Fri, Mar 06, 2020 at 03:44:33PM +0100, Benjamin Gaignard wrote:
> Le ven. 6 mars 2020 à 15:23, Sudeep Holla <sudeep.holla@arm.com> a écrit :
> >
> > On Fri, Mar 06, 2020 at 01:32:59PM +0100, Benjamin Gaignard wrote:
> > > Le ven. 6 mars 2020 à 13:06, Sudeep Holla <sudeep.holla@arm.com> a écrit :
> > > >
> >
> > [...]
> >
> > > > Sure, please create a deeper CPU state than WFI and enter so that the CPU
> > > > state is saved and restored correctly. What is the problem doing that ?
> > >
> > > This state stop the clocks for all the hardware blocks and not only the CPUs
> > > so we can't go on it while devices aren't suspended.
> > > I may have missed something but I don't believe that I could add this kind of
> > > conditions in a cpu idle state, right ?
> > > In this state I need to be able to enable the wake up sources because
> > > it is the only
> > > for hardware block used as broadcast timer to wake up the system.
> > >
> >
> > We have discussed this in past in the thread I mentioned and may be
> > others too. It sounds like a broken hardware, sorry if I am wrong.
> > But this $subject patch is a hack to solve that and I am NACK-ing this
> > now. Please fix it adding another CPU level idle state, we are not
> > supporting without that and there is absolutely no need to.
>
> A CPU idle state only take care of CPU activities, right ? but before going in
> the targeting state I need to be sure that the other hardware blocks
> are suspended.
> Is it possible to describe that in an idle state ?
> What sound broken ? is it because we need to setup the wake up sources ?
>

You said: " In DT I have one system power domain where the hardware blocks
(i2c,uart; spi, etc..) are attached + a power per CPU". Now since the CPU
stays in WFI always in this platform, it means it is always ON and you
can't vote to power down the magic "system power domain".

--
Regards,
Sudeep

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

Re: [PATCH v2 4/4] cpuidle: psci: Allow WFI to be the only state for the hierarchical topology

[Benjamin Gaignard]

Le ven. 6 mars 2020 à 15:50, Sudeep Holla <sudeep.holla@arm.com> a écrit :
>
> On Fri, Mar 06, 2020 at 03:44:33PM +0100, Benjamin Gaignard wrote:
> > Le ven. 6 mars 2020 à 15:23, Sudeep Holla <sudeep.holla@arm.com> a écrit :
> > >
> > > On Fri, Mar 06, 2020 at 01:32:59PM +0100, Benjamin Gaignard wrote:
> > > > Le ven. 6 mars 2020 à 13:06, Sudeep Holla <sudeep.holla@arm.com> a écrit :
> > > > >
> > >
> > > [...]
> > >
> > > > > Sure, please create a deeper CPU state than WFI and enter so that the CPU
> > > > > state is saved and restored correctly. What is the problem doing that ?
> > > >
> > > > This state stop the clocks for all the hardware blocks and not only the CPUs
> > > > so we can't go on it while devices aren't suspended.
> > > > I may have missed something but I don't believe that I could add this kind of
> > > > conditions in a cpu idle state, right ?
> > > > In this state I need to be able to enable the wake up sources because
> > > > it is the only
> > > > for hardware block used as broadcast timer to wake up the system.
> > > >
> > >
> > > We have discussed this in past in the thread I mentioned and may be
> > > others too. It sounds like a broken hardware, sorry if I am wrong.
> > > But this $subject patch is a hack to solve that and I am NACK-ing this
> > > now. Please fix it adding another CPU level idle state, we are not
> > > supporting without that and there is absolutely no need to.
> >
> > A CPU idle state only take care of CPU activities, right ? but before going in
> > the targeting state I need to be sure that the other hardware blocks
> > are suspended.
> > Is it possible to describe that in an idle state ?
> > What sound broken ? is it because we need to setup the wake up sources ?
> >
>
> You said: " In DT I have one system power domain where the hardware blocks
> (i2c,uart; spi, etc..) are attached + a power per CPU". Now since the CPU
> stays in WFI always in this platform, it means it is always ON and you
> can't vote to power down the magic "system power domain".

CPU power domains are subdomains of the system power domain so they can vote
for the targeting power domain.

>
> --
> Regards,
> Sudeep

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

Re: [PATCH v2 4/4] cpuidle: psci: Allow WFI to be the only state for the hierarchical topology

[Sudeep Holla]

On Fri, Mar 06, 2020 at 04:35:32PM +0100, Benjamin Gaignard wrote:

[...]

>
> CPU power domains are subdomains of the system power domain

Yes, that is platform specific.

> so they can vote for the targeting power domain.
>

Not when they are in WFI, it can't be powered down. I am going to say one
last time, add a CPU level state to workaround whatever you are trying to
and please stop hacking the psci domain like in the $subject patch.

If it was not any clear before, NACK.

--
Regards,
Sudeep

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

Re: [PATCH v2 0/4] cpuidle: psci: Some fixes when using the hierarchical layout

[Rafael J. Wysocki]

On Tue, Mar 3, 2020 at 9:36 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> Changes in v2:
>         - Small changes to patch 3 and 4, see their changelogs.
>
> While collaborating with Benjamin Gaignard to deploy the hierarchical layout
> for an ST SoC, it has turned that I have clearly missed to test a couple of
> corner cases in recently added support to the cpuidle-psci driver.
>
> This series are fixing the issues we have found.

I can apply the whole series, but I'd need an ACK from the PSCI driver
maintainers for that.

Thanks!

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

Re: [PATCH v2 0/4] cpuidle: psci: Some fixes when using the hierarchical layout

[Ulf Hansson]

On Tue, 3 Mar 2020 at 23:28, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, Mar 3, 2020 at 9:36 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > Changes in v2:
> >         - Small changes to patch 3 and 4, see their changelogs.
> >
> > While collaborating with Benjamin Gaignard to deploy the hierarchical layout
> > for an ST SoC, it has turned that I have clearly missed to test a couple of
> > corner cases in recently added support to the cpuidle-psci driver.
> >
> > This series are fixing the issues we have found.
>
> I can apply the whole series, but I'd need an ACK from the PSCI driver
> maintainers for that.

Patch 1 and patch 3 is ready to go. Although, patch 3 don't need the
fixes tag and can be queued for v5.17 instead.

Do you want me to resend or can you pick them from the series?

Kind regards
Uffe

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

Re: [PATCH v2 0/4] cpuidle: psci: Some fixes when using the hierarchical layout

[Rafael J. Wysocki]

On Mon, Mar 9, 2020 at 8:21 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Tue, 3 Mar 2020 at 23:28, Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Tue, Mar 3, 2020 at 9:36 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > Changes in v2:
> > >         - Small changes to patch 3 and 4, see their changelogs.
> > >
> > > While collaborating with Benjamin Gaignard to deploy the hierarchical layout
> > > for an ST SoC, it has turned that I have clearly missed to test a couple of
> > > corner cases in recently added support to the cpuidle-psci driver.
> > >
> > > This series are fixing the issues we have found.
> >
> > I can apply the whole series, but I'd need an ACK from the PSCI driver
> > maintainers for that.
>
> Patch 1 and patch 3 is ready to go. Although, patch 3 don't need the
> fixes tag and can be queued for v5.17 instead.
>
> Do you want me to resend or can you pick them from the series?

Yes, please!

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