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

Krzysztof Kozlowski
2022-05-12T08:14:27Z

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