Message-ID: <CAL_Jsq+aVEOPzqddu-X8GLJPex+J6V+_T1qaGHAXgp94+_-ptg@mail.gmail.com>
Date: Tue, 9 Apr 2024 09:09:46 -0500
From: Rob Herring <robh@...nel.org>
To: Cristian Marussi <cristian.marussi@....com>
Cc: Peng Fan <peng.fan@....com>, Krzysztof Kozlowski <krzk@...nel.org>, 
	"Peng Fan (OSS)" <peng.fan@....nxp.com>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, Shawn Guo <shawnguo@...nel.org>, 
	Sascha Hauer <s.hauer@...gutronix.de>, Pengutronix Kernel Team <kernel@...gutronix.de>, 
	Fabio Estevam <festevam@...il.com>, Sudeep Holla <sudeep.holla@....com>, 
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>, "imx@...ts.linux.dev" <imx@...ts.linux.dev>, 
	"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
 additionalProperties to true

On Tue, Apr 9, 2024 at 7:01 AM Cristian Marussi
<cristian.marussi@....com> wrote:
>
> On Tue, Apr 09, 2024 at 09:25:10AM +0000, Peng Fan wrote:
> > Hi Krzysztof,
> >
> > > Subject: RE: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> > > additionalProperties to true
> > >
> > > > Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> > > > additionalProperties to true
> > > >
> > > > On 08/04/2024 08:08, Peng Fan wrote:
> > > > >> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> > > > >> additionalProperties to true
> > > > >>
> > > > >> On 08/04/2024 01:50, Peng Fan wrote:
> > > > >>>> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> > > > >>>> additionalProperties to true
> > > > >>>>
> > > > >>>> On 07/04/2024 12:04, Peng Fan wrote:
> > > > >>>>>> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi:
> > > > >>>>>> set additionalProperties to true
> > > > >>>>>>
> > > > >>>>>> On 07/04/2024 02:37, Peng Fan wrote:
> > > > >>>>>>>> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi:
> > > > >>>>>>>> set additionalProperties to true
> > > > >>>>>>>>
> > > > >>>>>>>> On 05/04/2024 14:39, Peng Fan (OSS) wrote:
> > > > >>>>>>>>> From: Peng Fan <peng.fan@....com>
> > > > >>>>>>>>>
> > > > >>>>>>>>> When adding vendor extension protocols, there is dt-schema
> > > > >> warning:
> > > > >>>>>>>>> "
> > > > >>>>>>>>> imx,scmi.example.dtb: scmi: 'protocol@81', 'protocol@84' do
> > > > >>>>>>>>> not match any of the regexes: 'pinctrl-[0-9]+'
> > > > >>>>>>>>> "
> > > > >>>>>>>>>
> > > > >>>>>>>>> Set additionalProperties to true to address the issue.
> > > > >>>>>>>>
> > > > >>>>>>>> I do not see anything addressed here, except making the
> > > > >>>>>>>> binding accepting anything anywhere...
> > > > >>>>>>>
> > > > >>>>>>> I not wanna add vendor protocols in arm,scmi.yaml, so will
> > > > >>>>>>> introduce a new yaml imx.scmi.yaml which add i.MX SCMI
> > > > >>>>>>> protocol
> > > > >> extension.
> > > > >>>>>>>
> > > > >>>>>>> With additionalProperties set to false, I not know how, please
> > > > suggest.
> > > > >>>>>>
> > > > >>>>>> First of all, you cannot affect negatively existing devices
> > > > >>>>>> (their
> > > > >>>>>> bindings) and your patch does exactly that. This should make
> > > > >>>>>> you thing what is the correct approach...
> > > > >>>>>>
> > > > >>>>>> Rob gave you the comment about missing compatible - you still
> > > > >>>>>> did not address that.
> > > > >>>>>
> > > > >>>>> I added the compatible in patch 2/6 in the examples "compatible
> > > > >>>>> =
> > > > >>>> "arm,scmi";"
> > > > >>>>
> > > > >>>> So you claim that your vendor extensions are the same or fully
> > > > >>>> compatible with arm,scmi and you add nothing... Are your
> > > > >>>> extensions/protocol valid for arm,scmi?
> > > > >>>
> > > > >>> Yes. They are valid for arm,scmi.
> > > > >>>
> > > > >>>  If yes, why is this in separate binding. If no, why you use
> > > > >>> someone
> > > > >>>> else's compatible?
> > > > >>>
> > > > >>> Per SCMI Spec
> > > > >>> 0x80-0xFF: Reserved for vendor or platform-specific extensions to
> > > > >>> this interface
> > > > >>>
> > > > >>> i.MX use 0x81 for BBM, 0x84 for MISC. But other vendors will use
> > > > >>> the id for their own protocol.
> > > > >>
> > > > >> So how are they valid for arm,scmi? I don't understand.
> > > > >
> > > > > arm,scmi is a firmware compatible string. The protocol node is a sub-node.
> > > > > I think the arm,scmi is that saying the firmware following SCMI spec
> > > > > to implement the protocols.
> > > > >
> > > > > For vendor reserved ID, firmware also follow the SCMI spec to
> > > > > implement their own usage, so from firmware level, it is ARM SCMI
> > > > > spec
> > > > compatible.
> > > >
> > > > That's not the point. It is obvious that your firmware is compatible
> > > > with arm,scmi, but what you try to say in this and revised patch is
> > > > that every arm,scmi is compatible with your implementation. What you
> > > > are saying is that 0x84 is MISC protocol for every firmware, Qualcomm,
> > > > NXP, Samsung, TI, Mediatek etc.
> > > >
> > > > I claim it is not true. 0x84 is not MISC for Qualcomm, for example.
> > >
> > > You are right. I am lost now on how to add vendor ID support, using
> > > arm,scmi.yaml or adding a new imx,scmi.yaml or else.
> >
>
> Hi Peng,
>
> I dont think in the following you will find the solution to the problem,
> it is just to recap the situation and constraints around vendor protocol
> bindings.
>
> Describing SCMI vendors protocols is tricky because while on one side
> the protocol node has to be rooted under the main scmi fw DT node (like
> all the standard protocols) and be 'derived' from the arm,scmi.yaml
> protocol-node definition, the optional additional properties of the a specific
> vendor protocol nodes can be customized by each single vendor, and since,
> as you said, you can have multiple protocols from different vendors sharing the
> same protocol number, you could have multiple disjoint sets of valid properties
> allowed under that same protocol node number; so on one side you have to stick
> to some basic protocol-node defs and be rooted under the SCMI node, while on
> the other side you will have multiple possibly allowed sets of additional
> properties to check against, so IOW you cannot anyway just set
> additionalProperties to false since that will result in no checks at all.
>
> As a consequence, at runtime, in the final DTB shipped with a specific
> platform you should have only one of the possible vendor nodes for each
> of these overlapping protocols, and the SCMI core at probe time will
> pick the proper protocol implementation based on the vendor/sub_vendor
> IDs gathered from the running SCMI fw platform at init: this way you
> can just build the usual "all-inclusive" defconfig without worrying
> about vendor protocol clashes since the SCMI core can pick the right
> protocol implementation, you should just had taken care to provide the
> proper DTB for your protocol; BUT this also means that it is not possible
> to add multiple DT bindings based on a 'if vendor' condition since the
> vendor itself is NOT defined and not needed in the bindings since it is
> discoverable at runtime.
>
> So, after all of this blabbing of mine about this, I am wondering if it
> is not possible that the solution is to handle each and every vendor
> protocol node that appears with a block of addtional properties that
> are picked via a oneOf statement from some external vendor specific
> yaml.
> (...in a similar way to how pinctrl additional properties are added...)
>
>
> NOTE THAT the following is just an example of what I mean, it is certainly
> wrong, incomplete annd maybe just not acceptable (and could cause DT
> maintainers eyes to bleed :P)...
>
> ...so it is just fr the sake of explaining what I mean...
>
> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> index e9d3f043c4ed..3c38a1e3ffed 100644
> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> @@ -278,6 +278,22 @@ properties:
>      required:
>        - reg
>
> +  protocol@81:
> +    $ref: '#/$defs/protocol-node'
> +    unevaluatedProperties: false
> +
> +    properties:
> +      reg:
> +        const: 0x81
> +
> +    patternProperties:
> +      '$':
> +        type: object

Did you mean to have child nodes under the protocol node rather than in it?

> +        oneOf:
> +          - $ref: /schemas/vendor-A/scmi-protos.yaml#
> +          - $ref: /schemas/vendor-B/protos.yaml#

Moved up one level, this would work, but it would have to be an
'anyOf' because it is possible that 2 vendors have the exact same set
of properties.

I can think of 2 other ways to structure this.

First, is a specific vendor protocol discoverable? Not that is 0x81
protocol present, but that 0x81 is vendor Foo's extra special
value-add protocol? If not, I think we should require a compatible
string on vendor protocols. Then the base SCMI schema can require just
that, and each vendor protocol defines its node with a $ref to
'#/$defs/protocol-node'.

The 2nd way is just a variation of the oneOf above, but do we do 1
file per vendor protocol or 1 file per vendor. Either should be
doable, just a matter of where 'protocol@81', etc. are defined.

Rob

Powered by blists - more mailing lists