Message-ID: <a1e5ffec-a20d-4389-83f9-ee09bd9d733d@linaro.org>
Date: Fri, 15 Dec 2023 08:29:21 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Jie Luo <quic_luoj@...cinc.com>, Conor Dooley <conor@...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, robh+dt@...nel.org, 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 15/12/2023 07:46, Jie Luo wrote:
> 
> 
> On 12/15/2023 1:12 AM, Conor Dooley wrote:
>> On Wed, Dec 13, 2023 at 04:26:56PM +0800, Jie Luo wrote:
>>>
>>>
>>> On 12/13/2023 12:06 AM, Conor Dooley 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
>>>>>      "#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
>>>>>      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
>>>>
>>>>> +  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
>>>>
>>>> Why is this not represented by an element of the clocks property?
>>>
>>> This property is for the reference clock source selection of CMN PLL,
>>> CMN PLL generates the different clock rates for the different Ethernet
>>> blocks, this CMN PLL configuration is not located in the GCC, so the
>>> clock framework can't be used, which is the general hardware register
>>> instead of RCG register for GCC.
>>
>> I don't see how the clock being provided by the "GCC" (whatever that is)
>> or by some other clock controller or fixed clock makes a difference.
>> Why can't the other clock provider be represented in the devicetree?
>>
> 
> cmn-reference-clock is for selecting the reference clock source for the
> whole Ethernet block, which is just the standalone configure register.

Sure, you are aware though that all clocks are just configure registers?

Which clocks are these mentioned in the property? From where do they come?

Anyway, property is in existing form is not correct - this is not a
generic property.


> however the clock provider has the logical register distribution, such
> as for one clock tree, there is RCG, DIVIDER and branch registers in
> the qcom soc chip.
> 
> The clock consumer defines the clock IDs of device tree to reference the
> clocks provided by the clock controller, and these clock IDs are
> provided by the header file of clock provider.
> 
> like this,
> clocks = <&gcc GCC_MDIO_AHB_CLK>, 
> 
>           <&gcc GCC_UNIPHY0_AHB_CLK>, 
> 
>           <&gcc GCC_UNIPHY1_AHB_CLK>, 
> 
>           <&gcc GCC_UNIPHY0_SYS_CLK>, 
> 
>           <&gcc GCC_UNIPHY1_SYS_CLK>;
> 
> gcc is the device node of clock provider, and GCC_MDIO_AHB_CLK is the 
> clock ID.



Best regards,
Krzysztof

Powered by blists - more mailing lists