diff mbox series

[v2,1/6] dt-bindings: firmware: arm,scmi: set additionalProperties to true

Message ID 20240405-imx95-bbm-misc-v2-v2-1-9fc9186856c2@nxp.com (mailing list archive)
State New, archived
Headers show
Series firmware: support i.MX95 SCMI BBM/MISC Extenstion | expand

Commit Message

Peng Fan (OSS) April 5, 2024, 12:39 p.m. UTC
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.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Krzysztof Kozlowski April 6, 2024, 10:57 a.m. UTC | #1
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
Peng Fan April 7, 2024, 12:37 a.m. UTC | #2
> 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
Krzysztof Kozlowski April 7, 2024, 8:55 a.m. UTC | #3
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
Peng Fan April 7, 2024, 10:04 a.m. UTC | #4
> 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
Krzysztof Kozlowski April 7, 2024, 4:15 p.m. UTC | #5
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
Peng Fan April 7, 2024, 11:50 p.m. UTC | #6
> 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
Krzysztof Kozlowski April 8, 2024, 5:57 a.m. UTC | #7
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
Peng Fan April 8, 2024, 6:08 a.m. UTC | #8
> 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
Krzysztof Kozlowski April 8, 2024, 7:18 a.m. UTC | #9
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
Peng Fan April 8, 2024, 7:23 a.m. UTC | #10
> 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
Peng Fan April 9, 2024, 9:25 a.m. UTC | #11
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
Cristian Marussi April 9, 2024, 12:01 p.m. UTC | #12
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
Rob Herring (Arm) April 9, 2024, 2:09 p.m. UTC | #13
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
Cristian Marussi April 9, 2024, 2:56 p.m. UTC | #14
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
Peng Fan April 11, 2024, 1:50 a.m. UTC | #15
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 mbox series

Patch

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: