Message-Id: <CJXOGJIR1ONQ.2ZT3JQGVWWHFB@skynet-linux>
Date:   Thu, 12 May 2022 15:02:17 +0530
From:   "Sireesh Kodali" <sireeshkodali1@...il.com>
To:     "Krzysztof Kozlowski" <krzysztof.kozlowski@...aro.org>,
        <linux-remoteproc@...r.kernel.org>
Cc:     <linux-arm-msm@...r.kernel.org>,
        <~postmarketos/upstreaming@...ts.sr.ht>,
        <bjorn.andersson@...aro.org>, <devicetree@...r.kernel.org>,
        <phone-devel@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        "Andy Gross" <agross@...nel.org>,
        "Mathieu Poirier" <mathieu.poirier@...aro.org>,
        "Rob Herring" <robh+dt@...nel.org>,
        "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@...aro.org>
Subject: Re: [PATCH 4/9] dt-bindings: remoteproc: qcom: wcnss: Convert to
 YAML

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@...il.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@...aro.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

Powered by blists - more mailing lists