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

From: Ulf Hansson <ulf.hansson@linaro.org>
To: Sudeep Holla <sudeep.holla@arm.com>
Cc: Cristian Marussi <cristian.marussi@arm.com>,
	Viresh Kumar <vireshk@kernel.org>, Nishanth Menon <nm@ti.com>,
	Stephen Boyd <sboyd@kernel.org>, Nikunj Kela <nkela@quicinc.com>,
	Prasad Sodagudi <psodagud@quicinc.com>,
	Alexandre Torgue <alexandre.torgue@foss.st.com>,
	linux-pm@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	devicetree@vger.kernel.org
Subject: Re: [PATCH 09/16] dt-bindings: firmware: arm,scmi: Extend bindings for protocol@13
Date: Fri, 16 Jun 2023 13:48:27 +0200	[thread overview]
Message-ID: <CAPDyKFoWcyznGM7EYkuhSAB+6nNBvsPt77o5B3hyL06pwiFYMA@mail.gmail.com> (raw)
In-Reply-To: <20230615133020.pomw53jrzehbwahd@bogus>

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