| Message ID | 20240405-imx95-bbm-misc-v2-v2-1-9fc9186856c2@nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | firmware: support i.MX95 SCMI BBM/MISC Extenstion | expand |
On 05/04/2024 14:39, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.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... Best regards, Krzysztof
> 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@nxp.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. Thanks, Peng. > > Best regards, > Krzysztof
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@nxp.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. You need common schema referenced in arm,scmi and your device specific schema, also using it. Best regards, Krzysztof
> 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@nxp.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";" > > You need common schema referenced in arm,scmi and your device specific > schema, also using it. ok, let me try to figure it out. Thanks, Peng. > > > Best regards, > Krzysztof
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@nxp.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? If yes, why is this in separate binding. If no, why you use someone else's compatible? Maybe your binding is correct, feel free to convince me (and read first writing bindings). Best regards, Krzysztof
> 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@nxp.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. I use a separate binding here is to avoid add more vendor stuff in arm,scmi.yaml. Otherwise we will have to add a list as: if nxp xxx else if qcom xxx else if xx yyy. I could add back i.mx extension to arm,scmi.yaml if people agree. Thanks Peng. > > Maybe your binding is correct, feel free to convince me (and read first writing > bindings). > > Best regards, > Krzysztof
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@nxp.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. Best regards, Krzysztof
> 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@nxp.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. firmware { scmi { compatible = "arm,scmi"; #address-cells = <1>; #size-cells = <0>; scmi_devpd: protocol@11 { reg = <0x11>; #power-domain-cells = <1>; }; scmi_sys_power: protocol@12 { reg = <0x12>; }; scmi_perf: protocol@13 { reg = <0x13>; #clock-cells = <1>; #power-domain-cells = <1>; }; Scmi_bbm: protocol@81 { reg = <0x81>; ->vendor id vendor,xyz; ---> vendor properties. } Thanks, Peng. > > > > Best regards, > Krzysztof
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@nxp.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. Best regards, Krzysztof
> 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@nxp.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. Please suggest. Thanks, Peng > > Best regards, > Krzysztof
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@nxp.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. Do you have any suggestions on how to add vendor protocol in dt-schema? I am not sure what to do next, still keep imx,scmi.yaml or add vendor stuff in arm,scmi.yaml? Thanks, Peng. > > Please suggest. > > Thanks, > Peng > > > > Best regards, > > Krzysztof
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@nxp.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 + oneOf: + - $ref: /schemas/vendor-A/scmi-protos.yaml# + - $ref: /schemas/vendor-B/protos.yaml# + unevaluatedProperties: false + additionalProperties: false ...with each new Vendor coming to the party adding a line under oneOf...which would mean probably also having a protocol@NN for each new protocol that appears... Thanks, Cristian
On Tue, Apr 9, 2024 at 7:01 AM Cristian Marussi <cristian.marussi@arm.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@nxp.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
On Tue, Apr 09, 2024 at 09:09:46AM -0500, Rob Herring wrote: > On Tue, Apr 9, 2024 at 7:01 AM Cristian Marussi > <cristian.marussi@arm.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@nxp.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? ... nope ... it is just as bad as my yaml-fu is :P ... but not sure if vendors has also this needs or plain props will suffice... > > > + 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. > ok > 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'. Basically yes it is discoverable, since at runtime the SCMI core, early on, normally discovers the vendor_id/sub_vendor_id by querying the platform via Base protocol and then later only loads/initializes (by closest match) the vendor protocols that are present in the DT AND that has been 'tagged' at compile time with the same vendor_id/sub_vendor_id tuple (in the vendor module code, struct scmi_protocol) Of course you should take care to put the proper protocol@81 node in your vendor_A DTB for the vendor_A SCMI driver to make use of the additional vendor_A properties that you have defined under your node as referred in your vendor-protos.yaml...if you botch that up I will load a protocol and call your vendor_A driver with a vendor_X DT node. DT is currrently vendor-agnostic. > > 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. > Oh, yes mine was just an ill example...one file per vendor will do just fine: the important thing is that the list and the yaml itself can be extended as new vendors appears (in a backward compatble way of course) Thanks, Cristian
Hi Rob, Cristian, > Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set > additionalProperties to true > > On Tue, Apr 09, 2024 at 09:09:46AM -0500, Rob Herring wrote: > > On Tue, Apr 9, 2024 at 7:01 AM Cristian Marussi > > <cristian.marussi@arm.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@nxp.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? > > ... nope ... it is just as bad as my yaml-fu is :P ... but not sure if vendors has > also this needs or plain props will suffice... > > > > > > + 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 try this: protocol@84: anyOf: - $ref: /schemas/firmware/imx,scmi.yaml# - $ref: /schemas/firmware/vendor-B,scmi.yaml# but unevaluatedProperties could not be set to false, otherwise the example check will report reg is unevaluated, fsl,wakeup-sources is unevaluated. The imx,scmi.yaml is as below: properties: protocol@84: $ref: 'arm,scmi.yaml#/$defs/protocol-node' unevaluatedProperties: false description: The MISC Protocol is for managing SoC Misc settings, such as GPR settings properties: reg: const: 0x84 fsl,wakeup-sources: description: Each entry consists of 2 integers, represents the source and electric signal edge items: items: - description: the wakeup source - description: the wakeup electric signal edge minItems: 1 maxItems: 32 $ref: /schemas/types.yaml#/definitions/uint32-matrix required: - reg additionalProperties: true Is the upper ok? Thanks, Peng. > > ok > > > 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'. > > Basically yes it is discoverable, since at runtime the SCMI core, early on, > normally discovers the vendor_id/sub_vendor_id by querying the platform > via Base protocol and then later only loads/initializes (by closest match) the > vendor protocols that are present in the DT AND that has been 'tagged' at > compile time with the same vendor_id/sub_vendor_id tuple (in the vendor > module code, struct scmi_protocol) > > Of course you should take care to put the proper protocol@81 node in your > vendor_A DTB for the vendor_A SCMI driver to make use of the additional > vendor_A properties that you have defined under your node as referred in > your vendor-protos.yaml...if you botch that up I will load a protocol and call > your vendor_A driver with a vendor_X DT node. > > DT is currrently vendor-agnostic. > > > > > 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. > > > > Oh, yes mine was just an ill example...one file per vendor will do just > fine: the important thing is that the list and the yaml itself can be extended as > new vendors appears (in a backward compatble way of course) > > Thanks, > Cristian
diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml index 4591523b51a0..cfc613b65585 100644 --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml @@ -247,7 +247,7 @@ properties: reg: const: 0x18 -additionalProperties: false +additionalProperties: true $defs: protocol-node: