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

[Jie Luo <quic_luoj@quicinc.com>]

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
>>             - 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.

> 
>> +    oneOf:
>> +      - items:
> 
> So it is enum or not?

it's enum, i will remove the "oneOf: - items:" in the next patch set.

> 
>> +          - enum:
>> +              - 0   # CMN PLL reference internal 48MHZ
>> +              - 1   # CMN PLL reference external 25MHZ
>> +              - 2   # CMN PLL reference external 31250KHZ
>> +              - 3   # CMN PLL reference external 40MHZ
>> +              - 4   # CMN PLL reference external 48MHZ
>> +              - 5   # CMN PLL reference external 50MHZ
>> +              - 6   # CMN PLL reference internal 96MHZ
>> +
>> +  clock-frequency:
>> +    oneOf:
>> +      - items:
> 
> Same questions

it's enum.

> 
>> +          - enum:
>> +              - 12500000
>> +              - 6250000
>> +              - 3125000
>> +              - 1562500
>> +              - 781250
>> +              - 390625
>> +    description:
>> +      The MDIO bus clock that must be output by the MDIO bus hardware,
>> +      only the listed frequencies above can be supported, other frequency
>> +      will cause malfunction. If absent, the default hardware value 0xff
>> +      is used, which means the default MDIO clock frequency 390625HZ, The
>> +      MDIO clock frequency is MDIO_SYS_CLK/(MDIO_CLK_DIV + 1), the SoC
>> +      MDIO_SYS_CLK is fixed to 100MHZ, the MDIO_CLK_DIV is from MDIO control
>> +      register, there is higher clock frequency requirement on the normal
>> +      working case where the MDIO slave devices support high clock frequency.
>> +
>> +  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.

Without declaring these gpio properties, the warning will be reported
by dt_binding_check.

> 
>> +
>> +  reset-deassert-us:
>> +    maxItems: 1
> 
> Neither this.

same as above.

> 
>>   
>>   required:
>>     - compatible
>> @@ -61,6 +121,8 @@ allOf:
>>                 - qcom,ipq5018-mdio
>>                 - qcom,ipq6018-mdio
>>                 - qcom,ipq8074-mdio
>> +              - qcom,ipq5332-mdio
>> +              - qcom,ipq9574-mdio
>>       then:
>>         required:
>>           - clocks
>> @@ -70,6 +132,20 @@ allOf:
>>           clocks: false
>>           clock-names: false
>>   
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - qcom,ipq5332-mdio
>> +    then:
>> +      properties:
>> +        clocks:
>> +          minItems: 5
>> +          maxItems: 5
>> +        reg-names:
>> +          minItems: 4
>> +
>>   unevaluatedProperties: false
>>   
>>   examples:
>> @@ -100,3 +176,62 @@ examples:
>>           reg = <4>;
>>         };
>>       };
>> +
>> +  - |
>> +    #include <dt-bindings/clock/qcom,ipq5332-gcc.h>
>> +    #include <dt-bindings/gpio/gpio.h>
>> +
>> +    mdio@90000 {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +      compatible = "qcom,ipq5332-mdio",
>> +                   "qcom,ipq4019-mdio";
>> +      cmn-reference-clock = <0>;
>> +      clock-frequency = <6250000>;
>> +
>> +      reset-gpios = <&tlmm 51 GPIO_ACTIVE_LOW>;
>> +      reset-assert-us = <100000>;
>> +      reset-deassert-us = <100000>;
>> +
>> +      reg = <0x90000 0x64>,
>> +            <0x9B000 0x800>,
>> +            <0x7A00610 0x4>,
>> +            <0x7A10610 0x4>;
> 
> Lowercase hex, wrong order of properties. Align it with coding style.

will correct it in the next patch set, thanks.

> 
> 
> 
> Best regards,
> Krzysztof
>