On Tue, Apr 9, 2024 at 7:01 AM Cristian Marussi <cristian.marussi@xxxxxxx> 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@xxxxxxx> > > > > >>>>>>>>> > > > > >>>>>>>>> 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
