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

[Jie Luo <quic_luoj@quicinc.com>]

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@quicinc.com>
>>>> ---
>>>>    .../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.