diff mbox series

[5/5] cpuidle: psci: Prevent domain idlestates until consumers are ready

Message ID 20200615152054.6819-6-ulf.hansson@linaro.org
State Superseded
Headers show
Series cpuidle: psci: Various improvements for PSCI PM domains | expand

Commit Message

Ulf Hansson June 15, 2020, 3:20 p.m. UTC 
Depending on the SoC/platform, additional devices may be part of the PSCI
PM domain topology. This is the case with 'qcom,rpmh-rsc' device, for
example, even if this is not yet visible in the corresponding DTS-files.

Without going into too much details, a device like the 'qcom,rpmh-rsc' may
have HW constraints that needs to be obeyed to, before a domain idlestate
can be picked.

Therefore, let's implement the ->sync_state() callback to receive a
notification when all consumers of the PSCI PM domain providers have been
attached/probed to it. In this way, we can make sure all constraints from
all relevant devices, are taken into account before allowing a domain
idlestate to be picked.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

---
 drivers/cpuidle/cpuidle-psci-domain.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

-- 
2.20.1

Comments

Saravana Kannan June 15, 2020, 6:05 p.m. UTC | #1 
On Mon, Jun 15, 2020 at 8:21 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>

> Depending on the SoC/platform, additional devices may be part of the PSCI

> PM domain topology. This is the case with 'qcom,rpmh-rsc' device, for

> example, even if this is not yet visible in the corresponding DTS-files.

>

> Without going into too much details, a device like the 'qcom,rpmh-rsc' may

> have HW constraints that needs to be obeyed to, before a domain idlestate

> can be picked.

>

> Therefore, let's implement the ->sync_state() callback to receive a

> notification when all consumers of the PSCI PM domain providers have been

> attached/probed to it. In this way, we can make sure all constraints from

> all relevant devices, are taken into account before allowing a domain

> idlestate to be picked.

>

> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

> ---

>  drivers/cpuidle/cpuidle-psci-domain.c | 14 ++++++++++++++

>  1 file changed, 14 insertions(+)

>

> diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c

> index bf527d2bb4b6..b6e9649ab0da 100644

> --- a/drivers/cpuidle/cpuidle-psci-domain.c

> +++ b/drivers/cpuidle/cpuidle-psci-domain.c

> @@ -27,6 +27,7 @@ struct psci_pd_provider {

>  };

>

>  static LIST_HEAD(psci_pd_providers);

> +static bool psci_pd_allow_domain_state;


Is there ever only 1 device that's probed by this driver? If yes, this
is okay. Otherwise, you'll need to handle this on a per device basis.

-Saravana
Ulf Hansson June 16, 2020, 6:49 a.m. UTC | #2 
On Mon, 15 Jun 2020 at 20:06, Saravana Kannan <saravanak@google.com> wrote:
>

> On Mon, Jun 15, 2020 at 8:21 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:

> >

> > Depending on the SoC/platform, additional devices may be part of the PSCI

> > PM domain topology. This is the case with 'qcom,rpmh-rsc' device, for

> > example, even if this is not yet visible in the corresponding DTS-files.

> >

> > Without going into too much details, a device like the 'qcom,rpmh-rsc' may

> > have HW constraints that needs to be obeyed to, before a domain idlestate

> > can be picked.

> >

> > Therefore, let's implement the ->sync_state() callback to receive a

> > notification when all consumers of the PSCI PM domain providers have been

> > attached/probed to it. In this way, we can make sure all constraints from

> > all relevant devices, are taken into account before allowing a domain

> > idlestate to be picked.

> >

> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

> > ---

> >  drivers/cpuidle/cpuidle-psci-domain.c | 14 ++++++++++++++

> >  1 file changed, 14 insertions(+)

> >

> > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c

> > index bf527d2bb4b6..b6e9649ab0da 100644

> > --- a/drivers/cpuidle/cpuidle-psci-domain.c

> > +++ b/drivers/cpuidle/cpuidle-psci-domain.c

> > @@ -27,6 +27,7 @@ struct psci_pd_provider {

> >  };

> >

> >  static LIST_HEAD(psci_pd_providers);

> > +static bool psci_pd_allow_domain_state;

>

> Is there ever only 1 device that's probed by this driver? If yes, this

> is okay. Otherwise, you'll need to handle this on a per device basis.


There is only one device. Subnodes, may exist to describe a
hierarchical description of the topology of the power-domains [1].

Kind regards
Uffe

[1] Documentation/devicetree/bindings/arm/psci.yaml
Saravana Kannan June 16, 2020, 7:05 a.m. UTC | #3 
On Mon, Jun 15, 2020 at 11:50 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>

> On Mon, 15 Jun 2020 at 20:06, Saravana Kannan <saravanak@google.com> wrote:

> >

> > On Mon, Jun 15, 2020 at 8:21 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:

> > >

> > > Depending on the SoC/platform, additional devices may be part of the PSCI

> > > PM domain topology. This is the case with 'qcom,rpmh-rsc' device, for

> > > example, even if this is not yet visible in the corresponding DTS-files.

> > >

> > > Without going into too much details, a device like the 'qcom,rpmh-rsc' may

> > > have HW constraints that needs to be obeyed to, before a domain idlestate

> > > can be picked.

> > >

> > > Therefore, let's implement the ->sync_state() callback to receive a

> > > notification when all consumers of the PSCI PM domain providers have been

> > > attached/probed to it. In this way, we can make sure all constraints from

> > > all relevant devices, are taken into account before allowing a domain

> > > idlestate to be picked.

> > >

> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

> > > ---

> > >  drivers/cpuidle/cpuidle-psci-domain.c | 14 ++++++++++++++

> > >  1 file changed, 14 insertions(+)

> > >

> > > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c

> > > index bf527d2bb4b6..b6e9649ab0da 100644

> > > --- a/drivers/cpuidle/cpuidle-psci-domain.c

> > > +++ b/drivers/cpuidle/cpuidle-psci-domain.c

> > > @@ -27,6 +27,7 @@ struct psci_pd_provider {

> > >  };

> > >

> > >  static LIST_HEAD(psci_pd_providers);

> > > +static bool psci_pd_allow_domain_state;

> >

> > Is there ever only 1 device that's probed by this driver? If yes, this

> > is okay. Otherwise, you'll need to handle this on a per device basis.

>

> There is only one device. Subnodes, may exist to describe a

> hierarchical description of the topology of the power-domains [1].


Thanks. In that case:

Acked-by: Saravana Kannan <saravanak@google.com>


-Saravana
diff mbox series

Patch

diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
index bf527d2bb4b6..b6e9649ab0da 100644
--- a/drivers/cpuidle/cpuidle-psci-domain.c
+++ b/drivers/cpuidle/cpuidle-psci-domain.c
@@ -27,6 +27,7 @@  struct psci_pd_provider {
 };
 
 static LIST_HEAD(psci_pd_providers);
+static bool psci_pd_allow_domain_state;
 
 static int psci_pd_power_off(struct generic_pm_domain *pd)
 {
@@ -36,6 +37,9 @@  static int psci_pd_power_off(struct generic_pm_domain *pd)
 	if (!state->data)
 		return 0;
 
+	if (!psci_pd_allow_domain_state)
+		return -EBUSY;
+
 	/* OSI mode is enabled, set the corresponding domain state. */
 	pd_state = state->data;
 	psci_set_domain_state(*pd_state);
@@ -222,6 +226,15 @@  static void psci_pd_remove_topology(struct device_node *np)
 	psci_pd_init_topology(np, false);
 }
 
+static void psci_cpuidle_domain_sync_state(struct device *dev)
+{
+	/*
+	 * All devices have now been attached/probed to the PM domain topology,
+	 * hence it's fine to allow domain states to be picked.
+	 */
+	psci_pd_allow_domain_state = true;
+}
+
 static const struct of_device_id psci_of_match[] = {
 	{ .compatible = "arm,psci-1.0" },
 	{}
@@ -289,6 +302,7 @@  static struct platform_driver psci_cpuidle_domain_driver = {
 	.driver = {
 		.name = "psci-cpuidle-domain",
 		.of_match_table = psci_of_match,
+		.sync_state = psci_cpuidle_domain_sync_state,
 	},
 };