diff mbox series

[v2,09/11] cpufreq: scmi: Add support to parse domain-id using #power-domain-cells

Message ID 20230713141738.23970-10-ulf.hansson@linaro.org (mailing list archive)
State New, archived
Delegated to: viresh kumar
Headers show
Series arm_scmi/cpufreq: Add generic performance scaling support | expand

Commit Message

Ulf Hansson July 13, 2023, 2:17 p.m. UTC
The performance domain-id can be described in DT using the power-domains
property or the clock property. The latter is already supported, so let's
add support for the power-domains too.

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

Changes in v2:
	- New patch.

---
 drivers/cpufreq/scmi-cpufreq.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

Comments

Sudeep Holla July 19, 2023, 3:24 p.m. UTC | #1
On Thu, Jul 13, 2023 at 04:17:36PM +0200, Ulf Hansson wrote:
> The performance domain-id can be described in DT using the power-domains
> property or the clock property. The latter is already supported, so let's
> add support for the power-domains too.
>

How is this supposed to work for the CPUs ? The CPU power domains are
generally PSCI on most of the platforms and the one using OSI explicitly
need to specify the details while ones using PC will not need to. Also they
can never be performance domains too. So I am not sure if I am following this
correctly.

--
Regards,
Sudeep
Ulf Hansson July 21, 2023, 11:52 a.m. UTC | #2
On Wed, 19 Jul 2023 at 17:24, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Jul 13, 2023 at 04:17:36PM +0200, Ulf Hansson wrote:
> > The performance domain-id can be described in DT using the power-domains
> > property or the clock property. The latter is already supported, so let's
> > add support for the power-domains too.
> >
>
> How is this supposed to work for the CPUs ? The CPU power domains are
> generally PSCI on most of the platforms and the one using OSI explicitly
> need to specify the details while ones using PC will not need to. Also they
> can never be performance domains too. So I am not sure if I am following this
> correctly.

Your concerns are certainly correct, I completely forgot about this.
We need to specify what power-domain index belongs to what, by using
power-domain-names in DT. So a CPU node, that has both psci for power
and scmi for performance would then typically look like this:

power-domains = <&CPU_PD0>, <&scmi_dvfs 4>;
power-domain-names = "psci", "scmi";

I will take care of this in the next version - and thanks a lot for
pointing this out!

Kind regards
Uffe
Sudeep Holla July 21, 2023, 11:59 a.m. UTC | #3
On Fri, Jul 21, 2023 at 01:52:17PM +0200, Ulf Hansson wrote:
> On Wed, 19 Jul 2023 at 17:24, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Thu, Jul 13, 2023 at 04:17:36PM +0200, Ulf Hansson wrote:
> > > The performance domain-id can be described in DT using the power-domains
> > > property or the clock property. The latter is already supported, so let's
> > > add support for the power-domains too.
> > >
> >
> > How is this supposed to work for the CPUs ? The CPU power domains are
> > generally PSCI on most of the platforms and the one using OSI explicitly
> > need to specify the details while ones using PC will not need to. Also they
> > can never be performance domains too. So I am not sure if I am following this
> > correctly.
> 
> Your concerns are certainly correct, I completely forgot about this.
> We need to specify what power-domain index belongs to what, by using
> power-domain-names in DT. So a CPU node, that has both psci for power
> and scmi for performance would then typically look like this:
> 
> power-domains = <&CPU_PD0>, <&scmi_dvfs 4>;
> power-domain-names = "psci", "scmi";
> 
> I will take care of this in the next version - and thanks a lot for
> pointing this out!


Yes something like this will work. Just curious will this impact the idle
paths ? By that I mean will the presence of additional domains add more
work or will they be skipped as early as possible with just one additional
check ?
Rob Herring July 21, 2023, 2:37 p.m. UTC | #4
On Fri, Jul 21, 2023 at 01:52:17PM +0200, Ulf Hansson wrote:
> On Wed, 19 Jul 2023 at 17:24, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Thu, Jul 13, 2023 at 04:17:36PM +0200, Ulf Hansson wrote:
> > > The performance domain-id can be described in DT using the power-domains
> > > property or the clock property. The latter is already supported, so let's
> > > add support for the power-domains too.
> > >
> >
> > How is this supposed to work for the CPUs ? The CPU power domains are
> > generally PSCI on most of the platforms and the one using OSI explicitly
> > need to specify the details while ones using PC will not need to. Also they
> > can never be performance domains too. So I am not sure if I am following this
> > correctly.
> 
> Your concerns are certainly correct, I completely forgot about this.
> We need to specify what power-domain index belongs to what, by using
> power-domain-names in DT. So a CPU node, that has both psci for power
> and scmi for performance would then typically look like this:
> 
> power-domains = <&CPU_PD0>, <&scmi_dvfs 4>;
> power-domain-names = "psci", "scmi";

That is completely backwards. Entries are named based on the consumer 
side. The function of each clock or interrupt for example. Here your 
entries are based on the provider which should be opaque to the 
consumer.

Rob
Ulf Hansson July 26, 2023, 11:20 a.m. UTC | #5
On Fri, 21 Jul 2023 at 16:37, Rob Herring <robh@kernel.org> wrote:
>
> On Fri, Jul 21, 2023 at 01:52:17PM +0200, Ulf Hansson wrote:
> > On Wed, 19 Jul 2023 at 17:24, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > On Thu, Jul 13, 2023 at 04:17:36PM +0200, Ulf Hansson wrote:
> > > > The performance domain-id can be described in DT using the power-domains
> > > > property or the clock property. The latter is already supported, so let's
> > > > add support for the power-domains too.
> > > >
> > >
> > > How is this supposed to work for the CPUs ? The CPU power domains are
> > > generally PSCI on most of the platforms and the one using OSI explicitly
> > > need to specify the details while ones using PC will not need to. Also they
> > > can never be performance domains too. So I am not sure if I am following this
> > > correctly.
> >
> > Your concerns are certainly correct, I completely forgot about this.
> > We need to specify what power-domain index belongs to what, by using
> > power-domain-names in DT. So a CPU node, that has both psci for power
> > and scmi for performance would then typically look like this:
> >
> > power-domains = <&CPU_PD0>, <&scmi_dvfs 4>;
> > power-domain-names = "psci", "scmi";
>
> That is completely backwards. Entries are named based on the consumer
> side. The function of each clock or interrupt for example. Here your
> entries are based on the provider which should be opaque to the
> consumer.

Okay, so you would rather prefer something along the lines of the below?

power-domain-names = "power", "perf";

The "psci" name is already part of the current cpus DT binding
(Documentation/devicetree/bindings/arm/cpus.yaml), so then it looks
like that deserves an update too. Right?

Kind regards
Uffe
Ulf Hansson July 26, 2023, 11:31 a.m. UTC | #6
On Fri, 21 Jul 2023 at 13:59, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Fri, Jul 21, 2023 at 01:52:17PM +0200, Ulf Hansson wrote:
> > On Wed, 19 Jul 2023 at 17:24, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > On Thu, Jul 13, 2023 at 04:17:36PM +0200, Ulf Hansson wrote:
> > > > The performance domain-id can be described in DT using the power-domains
> > > > property or the clock property. The latter is already supported, so let's
> > > > add support for the power-domains too.
> > > >
> > >
> > > How is this supposed to work for the CPUs ? The CPU power domains are
> > > generally PSCI on most of the platforms and the one using OSI explicitly
> > > need to specify the details while ones using PC will not need to. Also they
> > > can never be performance domains too. So I am not sure if I am following this
> > > correctly.
> >
> > Your concerns are certainly correct, I completely forgot about this.
> > We need to specify what power-domain index belongs to what, by using
> > power-domain-names in DT. So a CPU node, that has both psci for power
> > and scmi for performance would then typically look like this:
> >
> > power-domains = <&CPU_PD0>, <&scmi_dvfs 4>;
> > power-domain-names = "psci", "scmi";
> >
> > I will take care of this in the next version - and thanks a lot for
> > pointing this out!
>
>
> Yes something like this will work. Just curious will this impact the idle
> paths ? By that I mean will the presence of additional domains add more
> work or will they be skipped as early as possible with just one additional
> check ?

Unless I misunderstand your concern, I don't think there is any impact
on the idle path whatsoever. This should be entirely orthogonal.

The scmi-cpufreq driver should only have to care about the
scmi-performance domain, while the cpuidle-psci driver cares only
about psci.

Did that make sense?

Kind regards
Uffe
Ulf Hansson Aug. 21, 2023, 2:23 p.m. UTC | #7
On Wed, 26 Jul 2023 at 13:20, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Fri, 21 Jul 2023 at 16:37, Rob Herring <robh@kernel.org> wrote:
> >
> > On Fri, Jul 21, 2023 at 01:52:17PM +0200, Ulf Hansson wrote:
> > > On Wed, 19 Jul 2023 at 17:24, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > >
> > > > On Thu, Jul 13, 2023 at 04:17:36PM +0200, Ulf Hansson wrote:
> > > > > The performance domain-id can be described in DT using the power-domains
> > > > > property or the clock property. The latter is already supported, so let's
> > > > > add support for the power-domains too.
> > > > >
> > > >
> > > > How is this supposed to work for the CPUs ? The CPU power domains are
> > > > generally PSCI on most of the platforms and the one using OSI explicitly
> > > > need to specify the details while ones using PC will not need to. Also they
> > > > can never be performance domains too. So I am not sure if I am following this
> > > > correctly.
> > >
> > > Your concerns are certainly correct, I completely forgot about this.
> > > We need to specify what power-domain index belongs to what, by using
> > > power-domain-names in DT. So a CPU node, that has both psci for power
> > > and scmi for performance would then typically look like this:
> > >
> > > power-domains = <&CPU_PD0>, <&scmi_dvfs 4>;
> > > power-domain-names = "psci", "scmi";
> >
> > That is completely backwards. Entries are named based on the consumer
> > side. The function of each clock or interrupt for example. Here your
> > entries are based on the provider which should be opaque to the
> > consumer.
>
> Okay, so you would rather prefer something along the lines of the below?
>
> power-domain-names = "power", "perf";
>
> The "psci" name is already part of the current cpus DT binding
> (Documentation/devicetree/bindings/arm/cpus.yaml), so then it looks
> like that deserves an update too. Right?

Rob, may I have your thoughts around this? Is this an acceptable way
forward? Please advise me on what power-domain-names I should use
then.

Or, if you are insisting on using the performance-domain bindings?

Kind regards
Uffe
diff mbox series

Patch

diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
index 78f53e388094..b42f43d9bd89 100644
--- a/drivers/cpufreq/scmi-cpufreq.c
+++ b/drivers/cpufreq/scmi-cpufreq.c
@@ -72,13 +72,19 @@  static unsigned int scmi_cpufreq_fast_switch(struct cpufreq_policy *policy,
 
 static int scmi_cpu_domain_id(struct device *cpu_dev)
 {
-	struct of_phandle_args clkspec;
+	struct of_phandle_args domain_id;
 
 	if (of_parse_phandle_with_args(cpu_dev->of_node, "clocks",
-				       "#clock-cells", 0, &clkspec))
-		return -EINVAL;
+				       "#clock-cells", 0, &domain_id)) {
 
-	return clkspec.args[0];
+		if (of_parse_phandle_with_args(cpu_dev->of_node,
+					       "power-domains",
+					       "#power-domain-cells", 0,
+					       &domain_id))
+			return -EINVAL;
+	}
+
+	return domain_id.args[0];
 }
 
 static int