diff mbox series

[4/9] dt-bindings: remoteproc: qcom: wcnss: Convert to YAML

Message ID 20220511161602.117772-5-sireeshkodali1@gmail.com (mailing list archive)
State New
Headers show
Series Add support for remoteprocs on the MSM8953 platform | expand

Commit Message

Sireesh Kodali May 11, 2022, 4:15 p.m. UTC
Convert the dt-bindings from txt to YAML. This is in preparation for
including the relevant bindings for the MSM8953 platform's wcnss pil.

Signed-off-by: Sireesh Kodali <sireeshkodali1@gmail.com>
---
 .../bindings/remoteproc/qcom,wcnss-pil.txt    | 177 --------------
 .../bindings/remoteproc/qcom,wcnss-pil.yaml   | 228 ++++++++++++++++++
 2 files changed, 228 insertions(+), 177 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.txt
 create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml

Comments

Krzysztof Kozlowski May 11, 2022, 5:15 p.m. UTC | #1
On 11/05/2022 18:15, Sireesh Kodali wrote:
> Convert the dt-bindings from txt to YAML. This is in preparation for
> including the relevant bindings for the MSM8953 platform's wcnss pil.
> 
> Signed-off-by: Sireesh Kodali <sireeshkodali1@gmail.com>

Thank you for your patch. There is something to discuss/improve.

Please use existing bindings or example-schema as a starting point. Half
of my review could be skipped if you just followed what we already have
in the tree.

Some of these qcom specific properties already exist but you decided to
write them differently... please don't, rather reuse the code.

(...)

> +
> +maintainers:
> +  - Bjorn Andersson <bjorn.andersson@linaro.org>
> +
> +description:
> +  This document defines the binding for a component that loads and boots
> +  firmware on the Qualcomm WCNSS core.
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - items:
> +          - enum:
> +              - qcom,pronto-v2-pil
> +          - enum:
> +              - qcom,pronto

This does not look correct. The fallback compatible should not change.
What is more, it was not documented in original binding, so this should
be done in separate patch.

> +      - items:

No need for items, it's just one item.

> +          - enum:
> +              - qcom,riva-pil
> +              - qcom,pronto-v1-pil
> +              - qcom,pronto-v2-pil
> +
> +  reg:
> +    description: must specify the base address and size of the CCU, DXE and PMU
> +      register blocks

New line after "decription:", drop "must specify" and start with capital
letter.

You need maxItems: 3


> +
> +  reg-names:
> +    items:
> +      - const: ccu
> +      - const: dxe
> +      - const: pmu
> +
> +  interrupts-extended:
> +    description:
> +      Interrupt lines

Skip description, it's obvious.

It should be only "interrupts", not extended.

> +    minItems: 2
> +    maxItems: 5
> +
> +  interrupt-names:
> +    minItems: 2
> +    maxItems: 5

Names should be clearly defined. They were BTW defined in original
bindings, so you should not remove them. This makes me wonder what else
did you remove from original bindings...

Please document all deviations from pure conversion in the commit msg.
It's a second "hidden" difference.

> +
> +  firmware-name:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    description: Relative firmware image path for the WCNSS core. Defaults to
> +      "wcnss.mdt".


Blank line after "description:". This applies to other places as well.

Remove "Defailts to ..." and just add "default" schema.

> +
> +  vddpx-supply:
> +    description: Reference to the PX regulator to be held on behalf of the
> +      booting of the WCNSS core
> +
> +  vddmx-supply:
> +    description: Reference to the MX regulator to be held on behalf of the
> +      booting of the WCNSS core.
> +
> +  vddcx-supply:
> +    description: Reference to the CX regulator to be held on behalf of the
> +      booting of the WCNSS core.

s/Reference to the//

> +
> +  power-domains:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description: References to the power domains that need to be held on
> +      behalf of the booting WCNSS core

1. Ditto.
2. No need for ref
3. maxItems

> +
> +  power-domain-names:
> +    $ref: /schemas/types.yaml#/definitions/string-array

No need for ref, skip description.

> +    description: Names of the power domains
> +    items:
> +      - const: cx
> +      - const: mx
> +
> +  qcom,smem-states:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description: States used by the AP to signal the WCNSS core that it should
> +      shutdown
> +    items:
> +      - description: Stop the modem
> +
> +  qcom,smem-state-names:
> +    $ref: /schemas/types.yaml#/definitions/string-array

No need for ref. Really, it does not appear in any of existing bindings
for smem-state-names, so how did you get it?

> +    description: The names of the state bits used for SMP2P output
> +    items:
> +      - const: stop
> +
> +  memory-region:
> +    maxItems: 1
> +    description: Reference to the reserved-memory for the WCNSS core
> +
> +  smd-edge:
> +    type: object
> +    description:
> +      Qualcomm Shared Memory subnode which represents communication edge,
> +      channels and devices related to the ADSP.

You should reference /schemas/soc/qcom/qcom,smd.yaml

> +
> +  iris:

Generic node name... what is "iris"?

> +    type: object
> +    description:
> +      The iris subnode of the WCNSS PIL is used to describe the attached rf module

s/rf/RF/

> +      and its resource dependencies.
> +
> +    properties:
> +      compatible:
> +        enum:
> +          - qcom,wcn3620
> +          - qcom,wcn3660
> +          - qcom,wcn3660b
> +          - qcom,wcn3680
> +
> +      clocks:
> +        description: XO clock
> +
> +      clock-names:
> +        items:
> +          - const: xo
> +
> +    required:
> +      - compatible

clocks and clock-names were required.
Missing supplies, which were btw as well required.

> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - interrupts-extended
> +  - interrupt-names
> +  - vddpx-supply
> +  - memory-region
> +  - smd-edge
> +  - iris
> +
> +additionalProperties: false
> +
> +if:

Within allOf, please.



Best regards,
Krzysztof
Sireesh Kodali May 12, 2022, 6:50 a.m. UTC | #2
On Wed May 11, 2022 at 10:45 PM IST, Krzysztof Kozlowski wrote:
> On 11/05/2022 18:15, Sireesh Kodali wrote:
> > Convert the dt-bindings from txt to YAML. This is in preparation for
> > including the relevant bindings for the MSM8953 platform's wcnss pil.
> > 
> > Signed-off-by: Sireesh Kodali <sireeshkodali1@gmail.com>
>
> Thank you for your patch. There is something to discuss/improve.
>
> Please use existing bindings or example-schema as a starting point. Half
> of my review could be skipped if you just followed what we already have
> in the tree.
>
> Some of these qcom specific properties already exist but you decided to
> write them differently... please don't, rather reuse the code.
>

Thank you for your review, I will make the chnages as appropriate in v2.
> (...)
>
> > +
> > +maintainers:
> > +  - Bjorn Andersson <bjorn.andersson@linaro.org>
> > +
> > +description:
> > +  This document defines the binding for a component that loads and boots
> > +  firmware on the Qualcomm WCNSS core.
> > +
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - items:
> > +          - enum:
> > +              - qcom,pronto-v2-pil
> > +          - enum:
> > +              - qcom,pronto
>
> This does not look correct. The fallback compatible should not change.
> What is more, it was not documented in original binding, so this should
> be done in separate patch.
>

This was not a change to the fallback compatible. msm8916.dtsi's wcnss
node has "qcom,pronto" as the compatible string, which is why this was
added. It is however not documented in the txt file. Is it sufficient to
add a note in the commit message, or should it be split into a separate
commit?

> > +      - items:
>
> No need for items, it's just one item.
>
> > +          - enum:
> > +              - qcom,riva-pil
> > +              - qcom,pronto-v1-pil
> > +              - qcom,pronto-v2-pil
> > +
> > +  reg:
> > +    description: must specify the base address and size of the CCU, DXE and PMU
> > +      register blocks
>
> New line after "decription:", drop "must specify" and start with capital
> letter.
>
> You need maxItems: 3
>

Will fix in v2
>
> > +
> > +  reg-names:
> > +    items:
> > +      - const: ccu
> > +      - const: dxe
> > +      - const: pmu
> > +
> > +  interrupts-extended:
> > +    description:
> > +      Interrupt lines
>
> Skip description, it's obvious.
>
> It should be only "interrupts", not extended.
>
> > +    minItems: 2
> > +    maxItems: 5
> > +
> > +  interrupt-names:
> > +    minItems: 2
> > +    maxItems: 5
>
> Names should be clearly defined. They were BTW defined in original
> bindings, so you should not remove them. This makes me wonder what else
> did you remove from original bindings...
>
> Please document all deviations from pure conversion in the commit msg.
> It's a second "hidden" difference.
>

Sorry, this was meant to be a pure txt->YAML conversion. The missing
interrupt names was accidental, and will be fixed in v2.
> > +
> > +  firmware-name:
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    description: Relative firmware image path for the WCNSS core. Defaults to
> > +      "wcnss.mdt".
>
>
> Blank line after "description:". This applies to other places as well.
>
> Remove "Defailts to ..." and just add "default" schema.
>

Will be fixed in v2
> > +
> > +  vddpx-supply:
> > +    description: Reference to the PX regulator to be held on behalf of the
> > +      booting of the WCNSS core
> > +
> > +  vddmx-supply:
> > +    description: Reference to the MX regulator to be held on behalf of the
> > +      booting of the WCNSS core.
> > +
> > +  vddcx-supply:
> > +    description: Reference to the CX regulator to be held on behalf of the
> > +      booting of the WCNSS core.
>
> s/Reference to the//
>
> > +
> > +  power-domains:
> > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > +    description: References to the power domains that need to be held on
> > +      behalf of the booting WCNSS core
>
> 1. Ditto.
> 2. No need for ref
> 3. maxItems
>
> > +
> > +  power-domain-names:
> > +    $ref: /schemas/types.yaml#/definitions/string-array
>
> No need for ref, skip description.
>
> > +    description: Names of the power domains
> > +    items:
> > +      - const: cx
> > +      - const: mx
> > +
> > +  qcom,smem-states:
> > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > +    description: States used by the AP to signal the WCNSS core that it should
> > +      shutdown
> > +    items:
> > +      - description: Stop the modem
> > +
> > +  qcom,smem-state-names:
> > +    $ref: /schemas/types.yaml#/definitions/string-array
>
> No need for ref. Really, it does not appear in any of existing bindings
> for smem-state-names, so how did you get it?
>

The smem nodes were copied from /remoteproc/qcom,sdm845-adsp-pil.yaml

> > +    description: The names of the state bits used for SMP2P output
> > +    items:
> > +      - const: stop
> > +
> > +  memory-region:
> > +    maxItems: 1
> > +    description: Reference to the reserved-memory for the WCNSS core
> > +
> > +  smd-edge:
> > +    type: object
> > +    description:
> > +      Qualcomm Shared Memory subnode which represents communication edge,
> > +      channels and devices related to the ADSP.
>
> You should reference /schemas/soc/qcom/qcom,smd.yaml

Will be done in v2
>
> > +
> > +  iris:
>
> Generic node name... what is "iris"?
>
Iris is the RF module, I'll make the description better

> > +    type: object
> > +    description:
> > +      The iris subnode of the WCNSS PIL is used to describe the attached rf module
>
> s/rf/RF/
>
> > +      and its resource dependencies.
> > +
> > +    properties:
> > +      compatible:
> > +        enum:
> > +          - qcom,wcn3620
> > +          - qcom,wcn3660
> > +          - qcom,wcn3660b
> > +          - qcom,wcn3680
> > +
> > +      clocks:
> > +        description: XO clock
> > +
> > +      clock-names:
> > +        items:
> > +          - const: xo
> > +
> > +    required:
> > +      - compatible
>
> clocks and clock-names were required.
> Missing supplies, which were btw as well required.
>
This was unintentional, it will be fixed in v2.

> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reg-names
> > +  - interrupts-extended
> > +  - interrupt-names
> > +  - vddpx-supply
> > +  - memory-region
> > +  - smd-edge
> > +  - iris
> > +
> > +additionalProperties: false
> > +
> > +if:
>
> Within allOf, please.
>

Will be fixed in v2
>
>
> Best regards,
> Krzysztof

Thanks again for the review,
Sireesh
Krzysztof Kozlowski May 12, 2022, 8:14 a.m. UTC | #3
On 12/05/2022 08:50, Sireesh Kodali wrote:
> On Wed May 11, 2022 at 10:45 PM IST, Krzysztof Kozlowski wrote:
>> On 11/05/2022 18:15, Sireesh Kodali wrote:
>>> Convert the dt-bindings from txt to YAML. This is in preparation for
>>> including the relevant bindings for the MSM8953 platform's wcnss pil.
>>>
>>> Signed-off-by: Sireesh Kodali <sireeshkodali1@gmail.com>
>>
>> Thank you for your patch. There is something to discuss/improve.
>>
>> Please use existing bindings or example-schema as a starting point. Half
>> of my review could be skipped if you just followed what we already have
>> in the tree.
>>
>> Some of these qcom specific properties already exist but you decided to
>> write them differently... please don't, rather reuse the code.
>>
> 
> Thank you for your review, I will make the chnages as appropriate in v2.
>> (...)
>>
>>> +
>>> +maintainers:
>>> +  - Bjorn Andersson <bjorn.andersson@linaro.org>
>>> +
>>> +description:
>>> +  This document defines the binding for a component that loads and boots
>>> +  firmware on the Qualcomm WCNSS core.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    oneOf:
>>> +      - items:
>>> +          - enum:
>>> +              - qcom,pronto-v2-pil
>>> +          - enum:
>>> +              - qcom,pronto
>>
>> This does not look correct. The fallback compatible should not change.
>> What is more, it was not documented in original binding, so this should
>> be done in separate patch.
>>
> 
> This was not a change to the fallback compatible. 

You made it an enum, so you expect it to use different fallback for
different cases.

> msm8916.dtsi's wcnss
> node has "qcom,pronto" as the compatible string, which is why this was
> added. It is however not documented in the txt file. Is it sufficient to
> add a note in the commit message, or should it be split into a separate
> commit?

Please split it, assuming that fallback is correct. Maybe the fallback
is wrong?

> 
>>> +      - items:
>>
>> No need for items, it's just one item.
>>
>>> +          - enum:
>>> +              - qcom,riva-pil
>>> +              - qcom,pronto-v1-pil
>>> +              - qcom,pronto-v2-pil
>>> +
>>> +  reg:
>>> +    description: must specify the base address and size of the CCU, DXE and PMU
>>> +      register blocks
>>
>> New line after "decription:", drop "must specify" and start with capital
>> letter.
>>
>> You need maxItems: 3
>>
> 
> Will fix in v2
>>
>>> +
>>> +  reg-names:
>>> +    items:
>>> +      - const: ccu
>>> +      - const: dxe
>>> +      - const: pmu
>>> +
>>> +  interrupts-extended:
>>> +    description:
>>> +      Interrupt lines
>>
>> Skip description, it's obvious.
>>
>> It should be only "interrupts", not extended.
>>
>>> +    minItems: 2
>>> +    maxItems: 5
>>> +
>>> +  interrupt-names:
>>> +    minItems: 2
>>> +    maxItems: 5
>>
>> Names should be clearly defined. They were BTW defined in original
>> bindings, so you should not remove them. This makes me wonder what else
>> did you remove from original bindings...
>>
>> Please document all deviations from pure conversion in the commit msg.
>> It's a second "hidden" difference.
>>
> 
> Sorry, this was meant to be a pure txt->YAML conversion. The missing
> interrupt names was accidental, and will be fixed in v2.
>>> +
>>> +  firmware-name:
>>> +    $ref: /schemas/types.yaml#/definitions/string
>>> +    description: Relative firmware image path for the WCNSS core. Defaults to
>>> +      "wcnss.mdt".
>>
>>
>> Blank line after "description:". This applies to other places as well.
>>
>> Remove "Defailts to ..." and just add "default" schema.
>>
> 
> Will be fixed in v2
>>> +
>>> +  vddpx-supply:
>>> +    description: Reference to the PX regulator to be held on behalf of the
>>> +      booting of the WCNSS core
>>> +
>>> +  vddmx-supply:
>>> +    description: Reference to the MX regulator to be held on behalf of the
>>> +      booting of the WCNSS core.
>>> +
>>> +  vddcx-supply:
>>> +    description: Reference to the CX regulator to be held on behalf of the
>>> +      booting of the WCNSS core.
>>
>> s/Reference to the//
>>
>>> +
>>> +  power-domains:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>>> +    description: References to the power domains that need to be held on
>>> +      behalf of the booting WCNSS core
>>
>> 1. Ditto.
>> 2. No need for ref
>> 3. maxItems
>>
>>> +
>>> +  power-domain-names:
>>> +    $ref: /schemas/types.yaml#/definitions/string-array
>>
>> No need for ref, skip description.
>>
>>> +    description: Names of the power domains
>>> +    items:
>>> +      - const: cx
>>> +      - const: mx
>>> +
>>> +  qcom,smem-states:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>>> +    description: States used by the AP to signal the WCNSS core that it should
>>> +      shutdown
>>> +    items:
>>> +      - description: Stop the modem
>>> +
>>> +  qcom,smem-state-names:
>>> +    $ref: /schemas/types.yaml#/definitions/string-array
>>
>> No need for ref. Really, it does not appear in any of existing bindings
>> for smem-state-names, so how did you get it?
>>
> 
> The smem nodes were copied from /remoteproc/qcom,sdm845-adsp-pil.yaml

Hm, indeed, you're right. There are few files having here ref. I'll fix
these.

> 
>>> +    description: The names of the state bits used for SMP2P output
>>> +    items:
>>> +      - const: stop
>>> +
>>> +  memory-region:
>>> +    maxItems: 1
>>> +    description: Reference to the reserved-memory for the WCNSS core
>>> +
>>> +  smd-edge:
>>> +    type: object
>>> +    description:
>>> +      Qualcomm Shared Memory subnode which represents communication edge,
>>> +      channels and devices related to the ADSP.
>>
>> You should reference /schemas/soc/qcom/qcom,smd.yaml
> 
> Will be done in v2
>>
>>> +
>>> +  iris:
>>
>> Generic node name... what is "iris"?
>>
> Iris is the RF module, I'll make the description better

RF like wifi? Then the property name should be "wifi".



Best regards,
Krzysztof
Krzysztof Kozlowski May 12, 2022, 8:36 a.m. UTC | #4
On 12/05/2022 08:50, Sireesh Kodali wrote:
>>> +    description: The names of the state bits used for SMP2P output
>>> +    items:
>>> +      - const: stop
>>> +
>>> +  memory-region:
>>> +    maxItems: 1
>>> +    description: Reference to the reserved-memory for the WCNSS core
>>> +
>>> +  smd-edge:
>>> +    type: object
>>> +    description:
>>> +      Qualcomm Shared Memory subnode which represents communication edge,
>>> +      channels and devices related to the ADSP.
>>
>> You should reference /schemas/soc/qcom/qcom,smd.yaml

It seems it is not a SMD driver so above reference is not correct. This
should be probably described in its own schema, I just need to
understand what's this...


Best regards,
Krzysztof
Sireesh Kodali May 12, 2022, 9:32 a.m. UTC | #5
On Thu May 12, 2022 at 1:44 PM IST, Krzysztof Kozlowski wrote:
> On 12/05/2022 08:50, Sireesh Kodali wrote:
> > On Wed May 11, 2022 at 10:45 PM IST, Krzysztof Kozlowski wrote:
> >> On 11/05/2022 18:15, Sireesh Kodali wrote:
> >>> Convert the dt-bindings from txt to YAML. This is in preparation for
> >>> including the relevant bindings for the MSM8953 platform's wcnss pil.
> >>>
> >>> Signed-off-by: Sireesh Kodali <sireeshkodali1@gmail.com>
> >>
> >> Thank you for your patch. There is something to discuss/improve.
> >>
> >> Please use existing bindings or example-schema as a starting point. Half
> >> of my review could be skipped if you just followed what we already have
> >> in the tree.
> >>
> >> Some of these qcom specific properties already exist but you decided to
> >> write them differently... please don't, rather reuse the code.
> >>
> > 
> > Thank you for your review, I will make the chnages as appropriate in v2.
> >> (...)
> >>
> >>> +
> >>> +maintainers:
> >>> +  - Bjorn Andersson <bjorn.andersson@linaro.org>
> >>> +
> >>> +description:
> >>> +  This document defines the binding for a component that loads and boots
> >>> +  firmware on the Qualcomm WCNSS core.
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    oneOf:
> >>> +      - items:
> >>> +          - enum:
> >>> +              - qcom,pronto-v2-pil
> >>> +          - enum:
> >>> +              - qcom,pronto
> >>
> >> This does not look correct. The fallback compatible should not change.
> >> What is more, it was not documented in original binding, so this should
> >> be done in separate patch.
> >>
> > 
> > This was not a change to the fallback compatible. 
>
> You made it an enum, so you expect it to use different fallback for
> different cases.
>
> > msm8916.dtsi's wcnss
> > node has "qcom,pronto" as the compatible string, which is why this was
> > added. It is however not documented in the txt file. Is it sufficient to
> > add a note in the commit message, or should it be split into a separate
> > commit?
>
> Please split it, assuming that fallback is correct. Maybe the fallback
> is wrong?

The code doesn't recognize "qcom,pronto", so perhaps the best solution
is to just remove that compatible from msm8916.dtsi?
>
> > 
> >>> +      - items:
> >>
> >> No need for items, it's just one item.
> >>
> >>> +          - enum:
> >>> +              - qcom,riva-pil
> >>> +              - qcom,pronto-v1-pil
> >>> +              - qcom,pronto-v2-pil
> >>> +
> >>> +  reg:
> >>> +    description: must specify the base address and size of the CCU, DXE and PMU
> >>> +      register blocks
> >>
> >> New line after "decription:", drop "must specify" and start with capital
> >> letter.
> >>
> >> You need maxItems: 3
> >>
> > 
> > Will fix in v2
> >>
> >>> +
> >>> +  reg-names:
> >>> +    items:
> >>> +      - const: ccu
> >>> +      - const: dxe
> >>> +      - const: pmu
> >>> +
> >>> +  interrupts-extended:
> >>> +    description:
> >>> +      Interrupt lines
> >>
> >> Skip description, it's obvious.
> >>
> >> It should be only "interrupts", not extended.
> >>
> >>> +    minItems: 2
> >>> +    maxItems: 5
> >>> +
> >>> +  interrupt-names:
> >>> +    minItems: 2
> >>> +    maxItems: 5
> >>
> >> Names should be clearly defined. They were BTW defined in original
> >> bindings, so you should not remove them. This makes me wonder what else
> >> did you remove from original bindings...
> >>
> >> Please document all deviations from pure conversion in the commit msg.
> >> It's a second "hidden" difference.
> >>
> > 
> > Sorry, this was meant to be a pure txt->YAML conversion. The missing
> > interrupt names was accidental, and will be fixed in v2.
> >>> +
> >>> +  firmware-name:
> >>> +    $ref: /schemas/types.yaml#/definitions/string
> >>> +    description: Relative firmware image path for the WCNSS core. Defaults to
> >>> +      "wcnss.mdt".
> >>
> >>
> >> Blank line after "description:". This applies to other places as well.
> >>
> >> Remove "Defailts to ..." and just add "default" schema.
> >>
> > 
> > Will be fixed in v2
> >>> +
> >>> +  vddpx-supply:
> >>> +    description: Reference to the PX regulator to be held on behalf of the
> >>> +      booting of the WCNSS core
> >>> +
> >>> +  vddmx-supply:
> >>> +    description: Reference to the MX regulator to be held on behalf of the
> >>> +      booting of the WCNSS core.
> >>> +
> >>> +  vddcx-supply:
> >>> +    description: Reference to the CX regulator to be held on behalf of the
> >>> +      booting of the WCNSS core.
> >>
> >> s/Reference to the//
> >>
> >>> +
> >>> +  power-domains:
> >>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> >>> +    description: References to the power domains that need to be held on
> >>> +      behalf of the booting WCNSS core
> >>
> >> 1. Ditto.
> >> 2. No need for ref
> >> 3. maxItems
> >>
> >>> +
> >>> +  power-domain-names:
> >>> +    $ref: /schemas/types.yaml#/definitions/string-array
> >>
> >> No need for ref, skip description.
> >>
> >>> +    description: Names of the power domains
> >>> +    items:
> >>> +      - const: cx
> >>> +      - const: mx
> >>> +
> >>> +  qcom,smem-states:
> >>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> >>> +    description: States used by the AP to signal the WCNSS core that it should
> >>> +      shutdown
> >>> +    items:
> >>> +      - description: Stop the modem
> >>> +
> >>> +  qcom,smem-state-names:
> >>> +    $ref: /schemas/types.yaml#/definitions/string-array
> >>
> >> No need for ref. Really, it does not appear in any of existing bindings
> >> for smem-state-names, so how did you get it?
> >>
> > 
> > The smem nodes were copied from /remoteproc/qcom,sdm845-adsp-pil.yaml
>
> Hm, indeed, you're right. There are few files having here ref. I'll fix
> these.
>
> > 
> >>> +    description: The names of the state bits used for SMP2P output
> >>> +    items:
> >>> +      - const: stop
> >>> +
> >>> +  memory-region:
> >>> +    maxItems: 1
> >>> +    description: Reference to the reserved-memory for the WCNSS core
> >>> +
> >>> +  smd-edge:
> >>> +    type: object
> >>> +    description:
> >>> +      Qualcomm Shared Memory subnode which represents communication edge,
> >>> +      channels and devices related to the ADSP.
> >>
> >> You should reference /schemas/soc/qcom/qcom,smd.yaml
> > 
> > Will be done in v2
> >>
> >>> +
> >>> +  iris:
> >>
> >> Generic node name... what is "iris"?
> >>
> > Iris is the RF module, I'll make the description better
>
> RF like wifi? Then the property name should be "wifi".

RF like wifi and bluetooth. However there are wifi and bt subnodes in
the smd-edge subnode. Iris is just the antenna hardware if I understand
correctly. Also this is just a documentation of the existing nodes that
are present in msm8916.dtsi, but for whatever reason their documentation
was missing in the txt file. Without adding this node in the YAML
dtb_check fails.

>
>
> Best regards,
> Krzysztof
Sireesh Kodali May 12, 2022, 10:01 a.m. UTC | #6
On Thu May 12, 2022 at 2:06 PM IST, Krzysztof Kozlowski wrote:
> On 12/05/2022 08:50, Sireesh Kodali wrote:
> >>> +    description: The names of the state bits used for SMP2P output
> >>> +    items:
> >>> +      - const: stop
> >>> +
> >>> +  memory-region:
> >>> +    maxItems: 1
> >>> +    description: Reference to the reserved-memory for the WCNSS core
> >>> +
> >>> +  smd-edge:
> >>> +    type: object
> >>> +    description:
> >>> +      Qualcomm Shared Memory subnode which represents communication edge,
> >>> +      channels and devices related to the ADSP.
> >>
> >> You should reference /schemas/soc/qcom/qcom,smd.yaml
>
> It seems it is not a SMD driver so above reference is not correct. This
> should be probably described in its own schema, I just need to
> understand what's this...
>
The smd-edge node describes the smd channels used to communicate with
the remote processor. For wcnss that would be the remote proc id, and
the channels for bt and wifi (both separate). There's a similar node for
adsp and q6v5.
>
> Best regards,
> Krzysztof
Krzysztof Kozlowski May 12, 2022, 11:02 a.m. UTC | #7
On 12/05/2022 11:32, Sireesh Kodali wrote:
>>>>> +          - enum:
>>>>> +              - qcom,pronto-v2-pil
>>>>> +          - enum:
>>>>> +              - qcom,pronto
>>>>
>>>> This does not look correct. The fallback compatible should not change.
>>>> What is more, it was not documented in original binding, so this should
>>>> be done in separate patch.
>>>>
>>>
>>> This was not a change to the fallback compatible. 
>>
>> You made it an enum, so you expect it to use different fallback for
>> different cases.
>>
>>> msm8916.dtsi's wcnss
>>> node has "qcom,pronto" as the compatible string, which is why this was
>>> added. It is however not documented in the txt file. Is it sufficient to
>>> add a note in the commit message, or should it be split into a separate
>>> commit?
>>
>> Please split it, assuming that fallback is correct. Maybe the fallback
>> is wrong?
> 
> The code doesn't recognize "qcom,pronto", so perhaps the best solution
> is to just remove that compatible from msm8916.dtsi?

Eh, I don't know. You need to check, maybe also in downstream sources.

(...)

>>>>
>>>>> +
>>>>> +  iris:
>>>>
>>>> Generic node name... what is "iris"?
>>>>
>>> Iris is the RF module, I'll make the description better
>>
>> RF like wifi? Then the property name should be "wifi".
> 
> RF like wifi and bluetooth. However there are wifi and bt subnodes in
> the smd-edge subnode. Iris is just the antenna hardware if I understand
> correctly. Also this is just a documentation of the existing nodes that
> are present in msm8916.dtsi, but for whatever reason their documentation
> was missing in the txt file. Without adding this node in the YAML
> dtb_check fails.

It seems commit fd52bdae9ab0 ("wcn36xx: Disable 5GHz for wcn3620")
 added usage of "iris" property but did not document it in the bindings.

You can fix it by documenting (separate patch) existing practice or
document with changing the node name. I am not sure if it is worth the
effort, so just new patch please.

Best regards,
Krzysztof
Sireesh Kodali May 12, 2022, 1:42 p.m. UTC | #8
On Thu May 12, 2022 at 4:32 PM IST, Krzysztof Kozlowski wrote:
> On 12/05/2022 11:32, Sireesh Kodali wrote:
> >>>>> +          - enum:
> >>>>> +              - qcom,pronto-v2-pil
> >>>>> +          - enum:
> >>>>> +              - qcom,pronto
> >>>>
> >>>> This does not look correct. The fallback compatible should not change.
> >>>> What is more, it was not documented in original binding, so this should
> >>>> be done in separate patch.
> >>>>
> >>>
> >>> This was not a change to the fallback compatible. 
> >>
> >> You made it an enum, so you expect it to use different fallback for
> >> different cases.
> >>
> >>> msm8916.dtsi's wcnss
> >>> node has "qcom,pronto" as the compatible string, which is why this was
> >>> added. It is however not documented in the txt file. Is it sufficient to
> >>> add a note in the commit message, or should it be split into a separate
> >>> commit?
> >>
> >> Please split it, assuming that fallback is correct. Maybe the fallback
> >> is wrong?
> > 
> > The code doesn't recognize "qcom,pronto", so perhaps the best solution
> > is to just remove that compatible from msm8916.dtsi?
>
> Eh, I don't know. You need to check, maybe also in downstream sources.
>

I just checked, it seems "qcom,pronto" is used by the wcnss driver in
/net. So both "qcom,pronto-v2-pil" and "qcom,pronto" need to be present,
but the latter wasn't documented.

> (...)
>
> >>>>
> >>>>> +
> >>>>> +  iris:
> >>>>
> >>>> Generic node name... what is "iris"?
> >>>>
> >>> Iris is the RF module, I'll make the description better
> >>
> >> RF like wifi? Then the property name should be "wifi".
> > 
> > RF like wifi and bluetooth. However there are wifi and bt subnodes in
> > the smd-edge subnode. Iris is just the antenna hardware if I understand
> > correctly. Also this is just a documentation of the existing nodes that
> > are present in msm8916.dtsi, but for whatever reason their documentation
> > was missing in the txt file. Without adding this node in the YAML
> > dtb_check fails.
>
> It seems commit fd52bdae9ab0 ("wcn36xx: Disable 5GHz for wcn3620")
>  added usage of "iris" property but did not document it in the bindings.
>
> You can fix it by documenting (separate patch) existing practice or
> document with changing the node name. I am not sure if it is worth the
> effort, so just new patch please.
>

I'll make a 2 separate patches, documenting the extra "qcom,pronto"
compatible, and the iris subnode.

Thanks,
Sireesh

> Best regards,
> Krzysztof
Rob Herring May 12, 2022, 1:49 p.m. UTC | #9
On Wed, 11 May 2022 21:45:57 +0530, Sireesh Kodali wrote:
> Convert the dt-bindings from txt to YAML. This is in preparation for
> including the relevant bindings for the MSM8953 platform's wcnss pil.
> 
> Signed-off-by: Sireesh Kodali <sireeshkodali1@gmail.com>
> ---
>  .../bindings/remoteproc/qcom,wcnss-pil.txt    | 177 --------------
>  .../bindings/remoteproc/qcom,wcnss-pil.yaml   | 228 ++++++++++++++++++
>  2 files changed, 228 insertions(+), 177 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.txt
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml
> 

Running 'make dtbs_check' with the schema in this patch gives the
following warnings. Consider if they are expected or the schema is
incorrect. These may not be new warnings.

Note that it is not yet a requirement to have 0 warnings for dtbs_check.
This will change in the future.

Full log is available here: https://patchwork.ozlabs.org/patch/


remoteproc@a21b000: 'qcom,state', 'qcom,state-names' do not match any of the regexes: 'pinctrl-[0-9]+'
	arch/arm64/boot/dts/qcom/apq8016-sbc.dtb
	arch/arm64/boot/dts/qcom/msm8916-alcatel-idol347.dtb
	arch/arm64/boot/dts/qcom/msm8916-asus-z00l.dtb
	arch/arm64/boot/dts/qcom/msm8916-huawei-g7.dtb
	arch/arm64/boot/dts/qcom/msm8916-longcheer-l8150.dtb
	arch/arm64/boot/dts/qcom/msm8916-longcheer-l8910.dtb
	arch/arm64/boot/dts/qcom/msm8916-mtp.dtb
	arch/arm64/boot/dts/qcom/msm8916-samsung-a3u-eur.dtb
	arch/arm64/boot/dts/qcom/msm8916-samsung-a5u-eur.dtb
	arch/arm64/boot/dts/qcom/msm8916-samsung-j5.dtb
	arch/arm64/boot/dts/qcom/msm8916-samsung-serranove.dtb
	arch/arm64/boot/dts/qcom/msm8916-wingtech-wt88047.dtb
	arch/arm/boot/dts/qcom-apq8016-sbc.dtb
	arch/arm/boot/dts/qcom-msm8916-samsung-serranove.dtb

remoteproc@fb21b000: 'power-domain-names' is a required property
	arch/arm/boot/dts/qcom-msm8974-fairphone-fp2.dtb

remoteproc@fb21b000: 'power-domains' is a required property
	arch/arm/boot/dts/qcom-msm8974-fairphone-fp2.dtb
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.txt b/Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.txt
deleted file mode 100644
index a83080b8905c..000000000000
--- a/Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.txt
+++ /dev/null
@@ -1,177 +0,0 @@ 
-Qualcomm WCNSS Peripheral Image Loader
-
-This document defines the binding for a component that loads and boots firmware
-on the Qualcomm WCNSS core.
-
-- compatible:
-	Usage: required
-	Value type: <string>
-	Definition: must be one of:
-		    "qcom,riva-pil",
-		    "qcom,pronto-v1-pil",
-		    "qcom,pronto-v2-pil"
-
-- reg:
-	Usage: required
-	Value type: <prop-encoded-array>
-	Definition: must specify the base address and size of the CCU, DXE and
-		    PMU register blocks
-
-- reg-names:
-	Usage: required
-	Value type: <stringlist>
-	Definition: must be "ccu", "dxe", "pmu"
-
-- interrupts-extended:
-	Usage: required
-	Value type: <prop-encoded-array>
-	Definition: must list the watchdog and fatal IRQs and may specify the
-		    ready, handover and stop-ack IRQs
-
-- interrupt-names:
-	Usage: required
-	Value type: <stringlist>
-	Definition: should be "wdog", "fatal", optionally followed by "ready",
-		    "handover", "stop-ack"
-
-- firmware-name:
-	Usage: optional
-	Value type: <string>
-	Definition: must list the relative firmware image path for the
-		    WCNSS core. Defaults to "wcnss.mdt".
-
-- vddmx-supply: (deprecated for qcom,pronto-v1/2-pil)
-- vddcx-supply: (deprecated for qcom,pronto-v1/2-pil)
-- vddpx-supply:
-	Usage: required
-	Value type: <phandle>
-	Definition: reference to the regulators to be held on behalf of the
-		    booting of the WCNSS core
-
-- power-domains:
-	Usage: required (for qcom,pronto-v1/2-pil)
-	Value type: <phandle>
-	Definition: reference to the power domains to be held on behalf of the
-		    booting of the WCNSS core
-
-- power-domain-names:
-	Usage: required (for qcom,pronto-v1/2-pil)
-	Value type: <stringlist>
-	Definition: must be "cx", "mx"
-
-- qcom,smem-states:
-	Usage: optional
-	Value type: <prop-encoded-array>
-	Definition: reference to the SMEM state used to indicate to WCNSS that
-		    it should shut down
-
-- qcom,smem-state-names:
-	Usage: optional
-	Value type: <stringlist>
-	Definition: should be "stop"
-
-- memory-region:
-	Usage: required
-	Value type: <prop-encoded-array>
-	Definition: reference to reserved-memory node for the remote processor
-		    see ../reserved-memory/reserved-memory.txt
-
-= SUBNODES
-A required subnode of the WCNSS PIL is used to describe the attached rf module
-and its resource dependencies. It is described by the following properties:
-
-- compatible:
-	Usage: required
-	Value type: <string>
-	Definition: must be one of:
-		    "qcom,wcn3620",
-		    "qcom,wcn3660",
-		    "qcom,wcn3660b",
-		    "qcom,wcn3680"
-
-- clocks:
-	Usage: required
-	Value type: <prop-encoded-array>
-	Definition: should specify the xo clock and optionally the rf clock
-
-- clock-names:
-	Usage: required
-	Value type: <stringlist>
-	Definition: should be "xo", optionally followed by "rf"
-
-- vddxo-supply:
-- vddrfa-supply:
-- vddpa-supply:
-- vdddig-supply:
-	Usage: required
-	Value type: <phandle>
-	Definition: reference to the regulators to be held on behalf of the
-		    booting of the WCNSS core
-
-
-The wcnss node can also have an subnode named "smd-edge" that describes the SMD
-edge, channels and devices related to the WCNSS.
-See ../soc/qcom/qcom,smd.txt for details on how to describe the SMD edge.
-
-= EXAMPLE
-The following example describes the resources needed to boot control the WCNSS,
-with attached WCN3680, as it is commonly found on MSM8974 boards.
-
-pronto@fb204000 {
-	compatible = "qcom,pronto-v2-pil";
-	reg = <0xfb204000 0x2000>, <0xfb202000 0x1000>, <0xfb21b000 0x3000>;
-	reg-names = "ccu", "dxe", "pmu";
-
-	interrupts-extended = <&intc 0 149 1>,
-			      <&wcnss_smp2p_slave 0 0>,
-			      <&wcnss_smp2p_slave 1 0>,
-			      <&wcnss_smp2p_slave 2 0>,
-			      <&wcnss_smp2p_slave 3 0>;
-	interrupt-names = "wdog", "fatal", "ready", "handover", "stop-ack";
-
-	power-domains = <&rpmpd MSM8974_VDDCX>, <&rpmpd MSM8974_VDDMX>;
-	power-domain-names = "cx", "mx";
-
-	vddpx-supply = <&pm8941_s3>;
-
-	qcom,smem-states = <&wcnss_smp2p_out 0>;
-	qcom,smem-state-names = "stop";
-
-	memory-region = <&wcnss_region>;
-
-	pinctrl-names = "default";
-	pinctrl-0 = <&wcnss_pin_a>;
-
-	iris {
-		compatible = "qcom,wcn3680";
-
-		clocks = <&rpmcc RPM_CXO_CLK_SRC>, <&rpmcc RPM_CXO_A2>;
-		clock-names = "xo", "rf";
-
-		vddxo-supply = <&pm8941_l6>;
-		vddrfa-supply = <&pm8941_l11>;
-		vddpa-supply = <&pm8941_l19>;
-		vdddig-supply = <&pm8941_s3>;
-	};
-
-	smd-edge {
-		interrupts = <0 142 1>;
-
-		qcom,ipc = <&apcs 8 17>;
-		qcom,smd-edge = <6>;
-		qcom,remote-pid = <4>;
-
-		label = "pronto";
-
-		wcnss {
-			compatible = "qcom,wcnss";
-			qcom,smd-channels = "WCNSS_CTRL";
-
-			qcom,mmio = <&pronto>;
-
-			bt {
-				compatible = "qcom,wcnss-bt";
-			};
-		};
-	};
-};
diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml
new file mode 100644
index 000000000000..d19f9f87a3e3
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml
@@ -0,0 +1,228 @@ 
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/remoteproc/qcom,wcnss-pil.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm WCNSS Peripheral Image Loader
+
+maintainers:
+  - Bjorn Andersson <bjorn.andersson@linaro.org>
+
+description:
+  This document defines the binding for a component that loads and boots
+  firmware on the Qualcomm WCNSS core.
+
+properties:
+  compatible:
+    oneOf:
+      - items:
+          - enum:
+              - qcom,pronto-v2-pil
+          - enum:
+              - qcom,pronto
+      - items:
+          - enum:
+              - qcom,riva-pil
+              - qcom,pronto-v1-pil
+              - qcom,pronto-v2-pil
+
+  reg:
+    description: must specify the base address and size of the CCU, DXE and PMU
+      register blocks
+
+  reg-names:
+    items:
+      - const: ccu
+      - const: dxe
+      - const: pmu
+
+  interrupts-extended:
+    description:
+      Interrupt lines
+    minItems: 2
+    maxItems: 5
+
+  interrupt-names:
+    minItems: 2
+    maxItems: 5
+
+  firmware-name:
+    $ref: /schemas/types.yaml#/definitions/string
+    description: Relative firmware image path for the WCNSS core. Defaults to
+      "wcnss.mdt".
+
+  vddpx-supply:
+    description: Reference to the PX regulator to be held on behalf of the
+      booting of the WCNSS core
+
+  vddmx-supply:
+    description: Reference to the MX regulator to be held on behalf of the
+      booting of the WCNSS core.
+
+  vddcx-supply:
+    description: Reference to the CX regulator to be held on behalf of the
+      booting of the WCNSS core.
+
+  power-domains:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description: References to the power domains that need to be held on
+      behalf of the booting WCNSS core
+
+  power-domain-names:
+    $ref: /schemas/types.yaml#/definitions/string-array
+    description: Names of the power domains
+    items:
+      - const: cx
+      - const: mx
+
+  qcom,smem-states:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description: States used by the AP to signal the WCNSS core that it should
+      shutdown
+    items:
+      - description: Stop the modem
+
+  qcom,smem-state-names:
+    $ref: /schemas/types.yaml#/definitions/string-array
+    description: The names of the state bits used for SMP2P output
+    items:
+      - const: stop
+
+  memory-region:
+    maxItems: 1
+    description: Reference to the reserved-memory for the WCNSS core
+
+  smd-edge:
+    type: object
+    description:
+      Qualcomm Shared Memory subnode which represents communication edge,
+      channels and devices related to the ADSP.
+
+  iris:
+    type: object
+    description:
+      The iris subnode of the WCNSS PIL is used to describe the attached rf module
+      and its resource dependencies.
+
+    properties:
+      compatible:
+        enum:
+          - qcom,wcn3620
+          - qcom,wcn3660
+          - qcom,wcn3660b
+          - qcom,wcn3680
+
+      clocks:
+        description: XO clock
+
+      clock-names:
+        items:
+          - const: xo
+
+    required:
+      - compatible
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - interrupts-extended
+  - interrupt-names
+  - vddpx-supply
+  - memory-region
+  - smd-edge
+  - iris
+
+additionalProperties: false
+
+if:
+  properties:
+    compatible:
+      contains:
+        enum:
+          - qcom,pronto-v1-pil
+          - qcom,pronto-v2-pil
+then:
+  properties:
+    vddmx-supply:
+      deprecated: true
+      description: Deprecated for qcom,pronto-v1/2-pil
+
+    vddcx-supply:
+      deprecated: true
+      description: Deprecated for qcom,pronto-v1/2-pil
+
+  required:
+    - power-domains
+    - power-domain-names
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/qcom,rpmcc.h>
+    #include <dt-bindings/power/qcom-rpmpd.h>
+    pronto@a21b000 {
+        compatible = "qcom,pronto-v2-pil";
+        reg = <0x0a204000 0x2000>, <0x0a202000 0x1000>, <0x0a21b000 0x3000>;
+        reg-names = "ccu", "dxe", "pmu";
+
+        interrupts-extended = <&intc GIC_SPI 149 IRQ_TYPE_EDGE_RISING>,
+                              <&wcnss_smp2p_in 0 IRQ_TYPE_EDGE_RISING>,
+                              <&wcnss_smp2p_in 1 IRQ_TYPE_EDGE_RISING>,
+                              <&wcnss_smp2p_in 2 IRQ_TYPE_EDGE_RISING>,
+                              <&wcnss_smp2p_in 3 IRQ_TYPE_EDGE_RISING>;
+        interrupt-names = "wdog", "fatal", "ready", "handover", "stop-ack";
+
+        power-domains = <&rpmpd MSM8916_VDDCX>, <&rpmpd MSM8916_VDDMX>;
+        power-domain-names = "cx", "mx";
+
+        vddpx-supply = <&pm8916_l7>;
+
+        qcom,smem-states = <&wcnss_smp2p_out 0>;
+        qcom,smem-state-names = "stop";
+
+        memory-region = <&wcnss_region>;
+
+        pinctrl-names = "default";
+        pinctrl-0 = <&wcnss_pin_a>;
+
+        iris {
+            compatible = "qcom,wcn3620";
+
+            clocks = <&rpmcc RPM_SMD_RF_CLK2>;
+            clock-names = "xo";
+        };
+
+        smd-edge {
+            interrupts = <GIC_SPI 142 IRQ_TYPE_EDGE_RISING>;
+
+            qcom,ipc = <&apcs 8 17>;
+            qcom,smd-edge = <6>;
+            qcom,remote-pid = <4>;
+
+            label = "pronto";
+
+            wcnss {
+                compatible = "qcom,wcnss";
+                qcom,smd-channels = "WCNSS_CTRL";
+
+                qcom,mmio = <&pronto>;
+
+                bt {
+                    compatible = "qcom,wcnss-bt";
+                };
+
+                wifi {
+                    compatible = "qcom,wcnss-wlan";
+
+                    interrupts = <GIC_SPI 145 IRQ_TYPE_LEVEL_HIGH>,
+                                 <GIC_SPI 146 IRQ_TYPE_LEVEL_HIGH>;
+                    interrupt-names = "tx", "rx";
+
+                    qcom,smem-states = <&apps_smsm 10>, <&apps_smsm 9>;
+                    qcom,smem-state-names = "tx-enable", "tx-rings-empty";
+                };
+            };
+        };
+    };