Re: [PATCH v3 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

[Jie Luo]

On 12/15/2023 4:39 PM, Krzysztof Kozlowski wrote:
> On 15/12/2023 09:28, Jie Luo wrote:
> > On 12/15/2023 3:27 PM, Krzysztof Kozlowski wrote:
> > > On 14/12/2023 10:03, Luo Jie wrote:
> > > > Update the yaml file for the new DTS properties.
> > > >
> > > > 1. cmn-reference-clock for the CMN PLL source clock select.
> > > > 2. clock-frequency for MDIO clock frequency config.
> > > > 3. add uniphy AHB & SYS GCC clocks.
> > > > 4. add reset-gpios for MDIO bus level reset.
> > > >
> > > > Signed-off-by: Luo Jie <quic_luoj@xxxxxxxxxxx>
> > > > ---
> > > >    .../bindings/net/qcom,ipq4019-mdio.yaml       | 143 +++++++++++++++++-
> > > >    1 file changed, 139 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
> > > > index 3407e909e8a7..79f8513739e7 100644
> > > > --- a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
> > > > +++ b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
> > > > @@ -20,6 +20,8 @@ properties:
> > > >              - enum:
> > > >                  - qcom,ipq6018-mdio
> > > >                  - qcom,ipq8074-mdio
> > > > +              - qcom,ipq9574-mdio
> > > > +              - qcom,ipq5332-mdio
>
> Why do you add entries to the end of the list? In reversed order?

Thanks for pointing it out, i will move "- qcom,ipq5332-mdio" before
"- qcom,ipq6018-mdio".

> > > >              - const: qcom,ipq4019-mdio
> > > >    
> > > >      "#address-cells":
> > > > @@ -30,19 +32,77 @@ properties:
> > > >    
> > > >      reg:
> > > >        minItems: 1
> > > > -    maxItems: 2
> > > > +    maxItems: 5
> > > >        description:
> > > > -      the first Address and length of the register set for the MDIO controller.
> > > > -      the second Address and length of the register for ethernet LDO, this second
> > > > -      address range is only required by the platform IPQ50xx.
> > > > +      the first Address and length of the register set for the MDIO controller,
> > > > +      the optional second, third and fourth address and length of the register
> > > > +      for ethernet LDO, these three address range are required by the platform
> > > > +      IPQ50xx/IPQ5332, the last address and length is for the CMN clock to
> > > > +      select the reference clock.
> > > > +
> > > > +  reg-names:
> > > > +    minItems: 1
> > > > +    maxItems: 5
> > > >    
> > > >      clocks:
> > > > +    minItems: 1
> > > >        items:
> > > >          - description: MDIO clock source frequency fixed to 100MHZ
> > > > +      - description: UNIPHY0 AHB clock source frequency fixed to 100MHZ
> > > > +      - description: UNIPHY1 AHB clock source frequency fixed to 100MHZ
> > > > +      - description: UNIPHY0 SYS clock source frequency fixed to 24MHZ
> > > > +      - description: UNIPHY1 SYS clock source frequency fixed to 24MHZ
> > > >    
> > > >      clock-names:
> > > > +    minItems: 1
> > > >        items:
> > > >          - const: gcc_mdio_ahb_clk
> > > > +      - const: uniphy0_ahb
> > > > +      - const: uniphy1_ahb
> > > > +      - const: uniphy0_sys
> > > > +      - const: uniphy1_sys
> > > > +
> > > > +  cmn-reference-clock:
> > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > >
> > > Nothing improved here
> >
> > With this change, the warning is not reported when i run
> > dt_binding_check, looks the new added property needs
> > the type ref to avoid the warning reported.
>
> Nothing improved in the property name, nor its style, nor in the actual
> contents/values.

This property is for CMN PLL block reference clock configuration,
so i use this property name.

it will be appreciated if you can suggest a suitable name, thanks.

> ...
>
> > > > +  reset-gpios:
> > > > +    maxItems: 1
> > > > +
> > > > +  reset-assert-us:
> > > > +    maxItems: 1
> > >
> > > This does not look related to ipq5332.
> >
> > The reset gpio properties are needed on ipq5332, since qca8084 phy is
> > connected, which uses the MDIO bus level gpio reset.
>
> I am talking about this property, not these properties.

ok.

> > Without declaring these gpio properties, the warning will be reported
> > by dt_binding_check.
>
> How is it even possible to have warnings if there is no such node in
> DTS? We do not care about warnings in your downstream code.
>
>
> Best regards,
> Krzysztof

If i do not declare the property "reset-assert-us" and 
"reset-deassert-us", the warning will be reported by "make 
dt_binding_check" since i
add a example in this file.