| Message ID | 20230607124628.157465-10-ulf.hansson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | arm_scmi/opp/dvfs: Add generic performance scaling support | expand |
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 >
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.
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 > >
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
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
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
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
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
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'
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(-)