[PATCH 09/16] dt-bindings: firmware: arm,scmi: Extend bindings for protocol@13

[PATCH 09/16] dt-bindings: firmware: arm,scmi: Extend bindings for protocol@13

[Ulf Hansson]

The protocol@13 node is describing the performance scaling option for the
ARM SCMI interface, as a clock provider. This is unnecessary limiting, as
performance scaling is in many cases not limited to switching a clock's
frequency.

Therefore, let's extend the binding so the interface can be modelled as a
generic "performance domain" too. The common way to describe this, is to
use the "power-domain" bindings, so let's use that.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: devicetree@vger.kernel.org
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
index 5824c43e9893..cff9d1e4cea1 100644
--- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
+++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
@@ -145,8 +145,8 @@ properties:
       '#clock-cells':
         const: 1
 
-    required:
-      - '#clock-cells'
+      '#power-domain-cells':
+        const: 1
 
   protocol@14:
     $ref: '#/$defs/protocol-node'
-- 
2.34.1

Re: [PATCH 09/16] dt-bindings: firmware: arm,scmi: Extend bindings for protocol@13

[Rob Herring]

On Wed, Jun 07, 2023 at 02:46:21PM +0200, Ulf Hansson wrote:
> The protocol@13 node is describing the performance scaling option for the
> ARM SCMI interface, as a clock provider. This is unnecessary limiting, as
> performance scaling is in many cases not limited to switching a clock's
> frequency.
> 
> Therefore, let's extend the binding so the interface can be modelled as a
> generic "performance domain" too. The common way to describe this, is to
> use the "power-domain" bindings, so let's use that.

What's wrong with the performance-domain binding?

> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> index 5824c43e9893..cff9d1e4cea1 100644
> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> @@ -145,8 +145,8 @@ properties:
>        '#clock-cells':
>          const: 1
>  
> -    required:
> -      - '#clock-cells'
> +      '#power-domain-cells':
> +        const: 1
>  
>    protocol@14:
>      $ref: '#/$defs/protocol-node'
> -- 
> 2.34.1
> 

Re: [PATCH 09/16] dt-bindings: firmware: arm,scmi: Extend bindings for protocol@13

[Sudeep Holla]

On Wed, Jun 07, 2023 at 02:46:21PM +0200, Ulf Hansson wrote:
> The protocol@13 node is describing the performance scaling option for the
> ARM SCMI interface, as a clock provider. This is unnecessary limiting, as
> performance scaling is in many cases not limited to switching a clock's
> frequency.
> 
> Therefore, let's extend the binding so the interface can be modelled as a
> generic "performance domain" too. The common way to describe this, is to
> use the "power-domain" bindings, so let's use that.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> index 5824c43e9893..cff9d1e4cea1 100644
> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> @@ -145,8 +145,8 @@ properties:
>        '#clock-cells':
>          const: 1
>  
> -    required:
> -      - '#clock-cells'

I am yet to look at the patches, just looked at this binding changes for now.

Won't this break compatibility with existing DTBs. IMO, this is strict
no no, you can't drop #clock-cells. I wanted to add performance-domains
here as alternative but decided to not as I knew you were working on this.

-- 
Regards,
Sudeep

Re: [PATCH 09/16] dt-bindings: firmware: arm,scmi: Extend bindings for protocol@13

[Ulf Hansson]

On Thu, 15 Jun 2023 at 01:00, Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Jun 07, 2023 at 02:46:21PM +0200, Ulf Hansson wrote:
> > The protocol@13 node is describing the performance scaling option for the
> > ARM SCMI interface, as a clock provider. This is unnecessary limiting, as
> > performance scaling is in many cases not limited to switching a clock's
> > frequency.
> >
> > Therefore, let's extend the binding so the interface can be modelled as a
> > generic "performance domain" too. The common way to describe this, is to
> > use the "power-domain" bindings, so let's use that.
>
> What's wrong with the performance-domain binding?

In my opinion I think the performance-domain binding is superfluous.
We already have plenty of power-domains that do performance scaling
too - and they stick with the power-domain binding, as it's
sufficient.

That said, I would rather follow the defacto standard that has been
used for many years in the kernel. Do you have a preference that we
should stick to?

Kind regards
Uffe

>
> >
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> > Cc: Conor Dooley <conor+dt@kernel.org>
> > Cc: devicetree@vger.kernel.org
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >  Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > index 5824c43e9893..cff9d1e4cea1 100644
> > --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > @@ -145,8 +145,8 @@ properties:
> >        '#clock-cells':
> >          const: 1
> >
> > -    required:
> > -      - '#clock-cells'
> > +      '#power-domain-cells':
> > +        const: 1
> >
> >    protocol@14:
> >      $ref: '#/$defs/protocol-node'
> > --
> > 2.34.1
> >

Re: [PATCH 09/16] dt-bindings: firmware: arm,scmi: Extend bindings for protocol@13

[Ulf Hansson]

On Thu, 15 Jun 2023 at 10:44, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Wed, Jun 07, 2023 at 02:46:21PM +0200, Ulf Hansson wrote:
> > The protocol@13 node is describing the performance scaling option for the
> > ARM SCMI interface, as a clock provider. This is unnecessary limiting, as
> > performance scaling is in many cases not limited to switching a clock's
> > frequency.
> >
> > Therefore, let's extend the binding so the interface can be modelled as a
> > generic "performance domain" too. The common way to describe this, is to
> > use the "power-domain" bindings, so let's use that.
> >
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> > Cc: Conor Dooley <conor+dt@kernel.org>
> > Cc: devicetree@vger.kernel.org
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >  Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > index 5824c43e9893..cff9d1e4cea1 100644
> > --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > @@ -145,8 +145,8 @@ properties:
> >        '#clock-cells':
> >          const: 1
> >
> > -    required:
> > -      - '#clock-cells'
>
> I am yet to look at the patches, just looked at this binding changes for now.
>
> Won't this break compatibility with existing DTBs. IMO, this is strict
> no no, you can't drop #clock-cells. I wanted to add performance-domains
> here as alternative but decided to not as I knew you were working on this.

Thanks for reviewing!

The point with the suggested change was to allow any kind of
combination of using #clock-cells and/or #power-domain-cells. Honestly
I didn't really know how to best express that in the binding, but
maybe someone can help me out here?

I think enforcing #clock-cells to be used is unnecessary. Making it
optional should not break existing DTBs, right?

Moreover, currently it seems to be only Juno that uses "protocol@13"
and the "#clock-cells" (at least by looking at the DTSes in-kernel).
So, I wonder if it's really such a big deal to update the DT bindings
for "protocol@13" at this point, but I may not have the complete
picture.

Kind regards
Uffe

Re: [PATCH 09/16] dt-bindings: firmware: arm,scmi: Extend bindings for protocol@13

[Sudeep Holla]

On Thu, Jun 15, 2023 at 11:39:06AM +0200, Ulf Hansson wrote:
> On Thu, 15 Jun 2023 at 10:44, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Wed, Jun 07, 2023 at 02:46:21PM +0200, Ulf Hansson wrote:
> > > The protocol@13 node is describing the performance scaling option for the
> > > ARM SCMI interface, as a clock provider. This is unnecessary limiting, as
> > > performance scaling is in many cases not limited to switching a clock's
> > > frequency.
> > >
> > > Therefore, let's extend the binding so the interface can be modelled as a
> > > generic "performance domain" too. The common way to describe this, is to
> > > use the "power-domain" bindings, so let's use that.
> > >
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> > > Cc: Conor Dooley <conor+dt@kernel.org>
> > > Cc: devicetree@vger.kernel.org
> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > ---
> > >  Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > index 5824c43e9893..cff9d1e4cea1 100644
> > > --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > @@ -145,8 +145,8 @@ properties:
> > >        '#clock-cells':
> > >          const: 1
> > >
> > > -    required:
> > > -      - '#clock-cells'
> >
> > I am yet to look at the patches, just looked at this binding changes for now.
> >
> > Won't this break compatibility with existing DTBs. IMO, this is strict
> > no no, you can't drop #clock-cells. I wanted to add performance-domains
> > here as alternative but decided to not as I knew you were working on this.
> 
> Thanks for reviewing!
> 
> The point with the suggested change was to allow any kind of
> combination of using #clock-cells and/or #power-domain-cells. Honestly
> I didn't really know how to best express that in the binding, but
> maybe someone can help me out here?
>

Even I don't know exact details, but there are rules like if this
property is present, some other property can't be there or something
on the similar lines. I have vague idea/recollection from my previous
experiments which probably was not needed then and hence I can't just
point to any examples unless I go and search myself.

> I think enforcing #clock-cells to be used is unnecessary. Making it
> optional should not break existing DTBs, right?

Correct. That is what I meant, it is either #clock-cells or
#power-domain-cells

>
> Moreover, currently it seems to be only Juno that uses "protocol@13"
> and the "#clock-cells" (at least by looking at the DTSes in-kernel).

Yes only one that has upstream DTS changes, but for sure it is used on
couple of other platforms. So for we are still far away from deprecate it
but we can eventually once users of it are ready to use new binding.

> So, I wonder if it's really such a big deal to update the DT bindings
> for "protocol@13" at this point, but I may not have the complete
> picture.
>

Yes it does break compatibility. Yes I know Juno is not a production
platform, but associating DT with kernel change makes is hard to switch
to older stable kernel versions without DT change which I really hate as
I will be wondering which SCMI perf is not working with stable kernel few
months down the line. So yes, we are not dropping the support for old
bindings even if it just for Juno(though I am sure it is not the only one).
I have spent time on such silly things when we were in the process of
pushing these bindings initially upstream. I prefer not to repeat that.

--
Regards,
Sudeep

Re: [PATCH 09/16] dt-bindings: firmware: arm,scmi: Extend bindings for protocol@13

[Ulf Hansson]

On Thu, 15 Jun 2023 at 15:30, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Jun 15, 2023 at 11:39:06AM +0200, Ulf Hansson wrote:
> > On Thu, 15 Jun 2023 at 10:44, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > On Wed, Jun 07, 2023 at 02:46:21PM +0200, Ulf Hansson wrote:
> > > > The protocol@13 node is describing the performance scaling option for the
> > > > ARM SCMI interface, as a clock provider. This is unnecessary limiting, as
> > > > performance scaling is in many cases not limited to switching a clock's
> > > > frequency.
> > > >
> > > > Therefore, let's extend the binding so the interface can be modelled as a
> > > > generic "performance domain" too. The common way to describe this, is to
> > > > use the "power-domain" bindings, so let's use that.
> > > >
> > > > Cc: Rob Herring <robh+dt@kernel.org>
> > > > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> > > > Cc: Conor Dooley <conor+dt@kernel.org>
> > > > Cc: devicetree@vger.kernel.org
> > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > > ---
> > > >  Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > > index 5824c43e9893..cff9d1e4cea1 100644
> > > > --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > > +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> > > > @@ -145,8 +145,8 @@ properties:
> > > >        '#clock-cells':
> > > >          const: 1
> > > >
> > > > -    required:
> > > > -      - '#clock-cells'
> > >
> > > I am yet to look at the patches, just looked at this binding changes for now.
> > >
> > > Won't this break compatibility with existing DTBs. IMO, this is strict
> > > no no, you can't drop #clock-cells. I wanted to add performance-domains
> > > here as alternative but decided to not as I knew you were working on this.
> >
> > Thanks for reviewing!
> >
> > The point with the suggested change was to allow any kind of
> > combination of using #clock-cells and/or #power-domain-cells. Honestly
> > I didn't really know how to best express that in the binding, but
> > maybe someone can help me out here?
> >
>
> Even I don't know exact details, but there are rules like if this
> property is present, some other property can't be there or something
> on the similar lines. I have vague idea/recollection from my previous
> experiments which probably was not needed then and hence I can't just
> point to any examples unless I go and search myself.

I will figure it out, np!

>
> > I think enforcing #clock-cells to be used is unnecessary. Making it
> > optional should not break existing DTBs, right?
>
> Correct. That is what I meant, it is either #clock-cells or
> #power-domain-cells

Should we allow both? Or maybe that is just confusing?

In either case, I am converting the scmi cpufreq driver to cope with
using #power-domain-cells too, as that is useful regardless I think.
However, that's a separate series on top of $subject series.

>
> >
> > Moreover, currently it seems to be only Juno that uses "protocol@13"
> > and the "#clock-cells" (at least by looking at the DTSes in-kernel).
>
> Yes only one that has upstream DTS changes, but for sure it is used on
> couple of other platforms. So for we are still far away from deprecate it
> but we can eventually once users of it are ready to use new binding.

Okay, let's discuss when to deprecate it, but let's do that later on.

>
> > So, I wonder if it's really such a big deal to update the DT bindings
> > for "protocol@13" at this point, but I may not have the complete
> > picture.
> >
>
> Yes it does break compatibility. Yes I know Juno is not a production
> platform, but associating DT with kernel change makes is hard to switch
> to older stable kernel versions without DT change which I really hate as
> I will be wondering which SCMI perf is not working with stable kernel few
> months down the line. So yes, we are not dropping the support for old
> bindings even if it just for Juno(though I am sure it is not the only one).
> I have spent time on such silly things when we were in the process of
> pushing these bindings initially upstream. I prefer not to repeat that.

Okay, thanks for sharing the information. Let's simply follow the
regular path of how we deal with deprecating DT bindings then.

Kind regards
Uffe

Re: [PATCH 09/16] dt-bindings: firmware: arm,scmi: Extend bindings for protocol@13

[Rob Herring]

On Thu, Jun 15, 2023 at 3:11 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Thu, 15 Jun 2023 at 01:00, Rob Herring <robh@kernel.org> wrote:
> >
> > On Wed, Jun 07, 2023 at 02:46:21PM +0200, Ulf Hansson wrote:
> > > The protocol@13 node is describing the performance scaling option for the
> > > ARM SCMI interface, as a clock provider. This is unnecessary limiting, as
> > > performance scaling is in many cases not limited to switching a clock's
> > > frequency.
> > >
> > > Therefore, let's extend the binding so the interface can be modelled as a
> > > generic "performance domain" too. The common way to describe this, is to
> > > use the "power-domain" bindings, so let's use that.
> >
> > What's wrong with the performance-domain binding?
>
> In my opinion I think the performance-domain binding is superfluous.
> We already have plenty of power-domains that do performance scaling
> too - and they stick with the power-domain binding, as it's
> sufficient.

IMO, power-domains should be for power islands which can be power
gated. I know they are abused though. Of course, when things are
hidden in firmware, you don't really know. A power-domain could be
just a clock or a clock could be multiple physical clocks.

> That said, I would rather follow the defacto standard that has been
> used for many years in the kernel. Do you have a preference that we
> should stick to?

If power-domains are sufficient, then why do we have
performance-domains? We need clear rules for when to use what.

Rob

Re: [PATCH 09/16] dt-bindings: firmware: arm,scmi: Extend bindings for protocol@13

[Ulf Hansson]

On Fri, 14 Jul 2023 at 19:15, Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Jun 15, 2023 at 3:11 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Thu, 15 Jun 2023 at 01:00, Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Wed, Jun 07, 2023 at 02:46:21PM +0200, Ulf Hansson wrote:
> > > > The protocol@13 node is describing the performance scaling option for the
> > > > ARM SCMI interface, as a clock provider. This is unnecessary limiting, as
> > > > performance scaling is in many cases not limited to switching a clock's
> > > > frequency.
> > > >
> > > > Therefore, let's extend the binding so the interface can be modelled as a
> > > > generic "performance domain" too. The common way to describe this, is to
> > > > use the "power-domain" bindings, so let's use that.
> > >
> > > What's wrong with the performance-domain binding?
> >
> > In my opinion I think the performance-domain binding is superfluous.
> > We already have plenty of power-domains that do performance scaling
> > too - and they stick with the power-domain binding, as it's
> > sufficient.
>
> IMO, power-domains should be for power islands which can be power
> gated. I know they are abused though. Of course, when things are
> hidden in firmware, you don't really know. A power-domain could be
> just a clock or a clock could be multiple physical clocks.

I would also like to point out that it's perfectly possible that a
power-domain can be a combination of a "power-island" and a
performance-domain. In fact we have those cases already (Qcom, Tegra).

>
> > That said, I would rather follow the defacto standard that has been
> > used for many years in the kernel. Do you have a preference that we
> > should stick to?
>
> If power-domains are sufficient, then why do we have
> performance-domains? We need clear rules for when to use what.

Well, I think we invented the performance-domains bindings, especially
with CPUs in mind. So far, that's the only use-case too (Mediatek,
Apple). Even if I think the power-domains bindings would have worked
fine for these cases too, maybe we should limit the
performance-domains binding to be used for CPUs?

Anyway, for the more generic use-case, I think the power-domains DT
binding is better to stick with (it's what we have used for many years
by now), as it provides us with the flexibility of hooking up an
opp-table to the power-domain, allowing us to make the domain
"performance-capable" too.

Kind regards
Uffe