Message-ID: <d8e723f0-2c51-402b-9464-7e4a928cf748@quicinc.com>
Date:   Wed, 13 Dec 2023 16:42:22 +0800
From:   Jie Luo <quic_luoj@...cinc.com>
To:     Rob Herring <robh@...nel.org>
CC:     <agross@...nel.org>, <andersson@...nel.org>,
        <konrad.dybcio@...aro.org>, <davem@...emloft.net>,
        <edumazet@...gle.com>, <kuba@...nel.org>, <pabeni@...hat.com>,
        <krzysztof.kozlowski+dt@...aro.org>, <conor+dt@...nel.org>,
        <andrew@...n.ch>, <hkallweit1@...il.com>, <linux@...linux.org.uk>,
        <robert.marko@...tura.hr>, <linux-arm-msm@...r.kernel.org>,
        <netdev@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <quic_srichara@...cinc.com>
Subject: Re: [PATCH v2 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332
 platform



On 12/13/2023 4:06 AM, Rob Herring wrote:
> On Tue, Dec 12, 2023 at 07:51:50PM +0800, 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@...cinc.com>
>> ---
>>   .../bindings/net/qcom,ipq4019-mdio.yaml       | 157 +++++++++++++++++-
>>   1 file changed, 153 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..9546a6ad7841 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
> 
> A driver can function without knowing about all these new registers and
> clocks? If not, then it can't be compatible with "qcom,ipq4019-mdio".

Yes, the driver can work without knowing the compatible string.
the configuration is decided by the DT property defined or not.

> 
>>   
>>     "#address-cells":
>> @@ -30,19 +32,71 @@ 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/IPQ9574, 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
> 
> These are all clock inputs to this h/w block and not some other clocks
> you want to manage?

Yes, for ipq5332, these 5 clocks are need to be managed, for the legacy 
platform such as ipq8074, only MDIO clock is needed.

No other more clock needs to be managed for the current IPQ platforms.

> 
>>   
>>     clock-names:
>> +    minItems: 1
>>       items:
>>         - const: gcc_mdio_ahb_clk
>> +      - const: gcc_uniphy0_ahb_clk
>> +      - const: gcc_uniphy1_ahb_clk
>> +      - const: gcc_uniphy0_sys_clk
>> +      - const: gcc_uniphy1_sys_clk
> 
> "gcc" is presumably the name of the clock controller in QCom chips.
> Well, the clock source should not be part of the binding. The names
> should be local for what they are for. So drop 'gcc_'. And '_clk' is
> also redundant, so drop it too. Unfortunately you are stuck with the
> name of the 1st entry.

Yes, gcc is the name of QCOM SOC clock controller.
will remove the "gcc_" and "_clk" for the new added clocks.

we should keep the existed DT gcc_mdio_ahb_clk unmodified, right?
since it has been used in the current device tree.

> 
>> +
>> +  cmn-reference-clock:
>> +    oneOf:
>> +      - items:
>> +          - 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:
>> +          - enum:
>> +              - 12500000
>> +              - 6250000
>> +              - 3125000
>> +              - 1562500
>> +              - 781250
>> +              - 390625
>> +    description:
>> +      The MDIO bus clock that must be output by the MDIO bus hardware,
>> +      only the listed frequecies above can be configured, other frequency
>> +      will cause malfunction. If absent, the default hardware value is used.
>> +
>> +  reset-gpios:
>> +    maxItems: 1
>> +
>> +  reset-assert-us:
>> +    maxItems: 1
>> +
>> +  reset-deassert-us:
>> +    maxItems: 1
>>   
>>   required:
>>     - compatible
>> @@ -61,6 +115,8 @@ allOf:
>>                 - qcom,ipq5018-mdio
>>                 - qcom,ipq6018-mdio
>>                 - qcom,ipq8074-mdio
>> +              - qcom,ipq5332-mdio
>> +              - qcom,ipq9574-mdio
>>       then:
>>         required:
>>           - clocks
>> @@ -70,6 +126,40 @@ allOf:
>>           clocks: false
>>           clock-names: false
>>   
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - qcom,ipq5332-mdio
>> +    then:
>> +      properties:
>> +        clocks:
>> +          minItems: 5
>> +          maxItems: 5
>> +        reg-names:
>> +          items:
>> +            - const: mdio
>> +            - const: eth_ldo1
>> +            - const: eth_ldo2
>> +            - const: cmn_blk
> 
> Perhaps cmn_blk should come 2nd, so all the variants have the same entry
> indices. Then you can move this to the top level and just say 'minItems:
> 4' here.

Thanks Rob for the suggestion, i will update to move cmn_blk to the 2nd
location.

> 
> 
>> +
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - qcom,ipq9574-mdio
>> +    then:
>> +      properties:
>> +        reg-names:
>> +          items:
>> +            - const: mdio
>> +            - const: eth_ldo1
>> +            - const: eth_ldo2
>> +            - const: eth_ldo3
>> +            - const: cmn_blk
> 
> And 'minItems: 5' here.
> 
> The ipq9574 adds the CMN block, but none of the clocks? Weird.
> 
> Rob

For ipq9574, only mdio clock is needed, the uniphy ahb and sys clock is
not needed to configure.

Yes, there is some Ethernet design delta between ipq9574 and ipq5332.

Powered by blists - more mailing lists