Message-ID: <a2448df9-9b8b-4b7e-ada5-6f26d7e7da97@quicinc.com>
Date: Wed, 23 Oct 2024 21:05:09 +0800
From: Jie Luo <quic_luoj@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
CC: Bjorn Andersson <andersson@...nel.org>,
        Michael Turquette
	<mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>, Rob Herring
	<robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley
	<conor+dt@...nel.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon
	<will@...nel.org>, Konrad Dybcio <konradybcio@...nel.org>,
        <linux-arm-msm@...r.kernel.org>, <linux-clk@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>, <quic_kkumarcs@...cinc.com>,
        <quic_suruchia@...cinc.com>, <quic_pavir@...cinc.com>,
        <quic_linchen@...cinc.com>, <quic_leiwei@...cinc.com>,
        <bartosz.golaszewski@...aro.org>, <srinivas.kandagatla@...aro.org>
Subject: Re: [PATCH v4 4/4] arm64: dts: qcom: Add CMN PLL node for IPQ9574 SoC



On 10/18/2024 11:38 PM, Dmitry Baryshkov wrote:
> On Fri, Oct 18, 2024 at 10:03:08PM +0800, Jie Luo wrote:
>>
>>
>> On 10/18/2024 4:11 PM, Dmitry Baryshkov wrote:
>>> On Fri, 18 Oct 2024 at 09:55, Jie Luo <quic_luoj@...cinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 10/18/2024 6:32 AM, Dmitry Baryshkov wrote:
>>>>> On Tue, Oct 15, 2024 at 10:16:54PM +0800, Luo Jie wrote:
>>>>>> The CMN PLL clock controller allows selection of an input
>>>>>> clock rate from a defined set of input clock rates. It in-turn
>>>>>> supplies fixed rate output clocks to the hardware blocks that
>>>>>> provide ethernet functions such as PPE (Packet Process Engine)
>>>>>> and connected switch or PHY, and to GCC.
>>>>>>
>>>>>> Signed-off-by: Luo Jie <quic_luoj@...cinc.com>
>>>>>> ---
>>>>>>     arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi |  6 +++++-
>>>>>>     arch/arm64/boot/dts/qcom/ipq9574.dtsi            | 20 +++++++++++++++++++-
>>>>>>     2 files changed, 24 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi b/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi
>>>>>> index 91e104b0f865..77e1e42083f3 100644
>>>>>> --- a/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi
>>>>>> +++ b/arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi
>>>>>> @@ -3,7 +3,7 @@
>>>>>>      * IPQ9574 RDP board common device tree source
>>>>>>      *
>>>>>>      * Copyright (c) 2020-2021 The Linux Foundation. All rights reserved.
>>>>>> - * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
>>>>>> + * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All rights reserved.
>>>>>>      */
>>>>>>
>>>>>>     /dts-v1/;
>>>>>> @@ -164,6 +164,10 @@ &usb3 {
>>>>>>        status = "okay";
>>>>>>     };
>>>>>>
>>>>>> +&cmn_pll_ref_clk {
>>>>>> +    clock-frequency = <48000000>;
>>>>>> +};
>>>>>> +
>>>>>>     &xo_board_clk {
>>>>>>        clock-frequency = <24000000>;
>>>>>>     };
>>>>>> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>>>>> index 14c7b3a78442..93f66bb83c5a 100644
>>>>>> --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>>>>> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>>>>> @@ -3,10 +3,11 @@
>>>>>>      * IPQ9574 SoC device tree source
>>>>>>      *
>>>>>>      * Copyright (c) 2020-2021 The Linux Foundation. All rights reserved.
>>>>>> - * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
>>>>>> + * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All rights reserved.
>>>>>>      */
>>>>>>
>>>>>>     #include <dt-bindings/clock/qcom,apss-ipq.h>
>>>>>> +#include <dt-bindings/clock/qcom,ipq-cmn-pll.h>
>>>>>>     #include <dt-bindings/clock/qcom,ipq9574-gcc.h>
>>>>>>     #include <dt-bindings/interconnect/qcom,ipq9574.h>
>>>>>>     #include <dt-bindings/interrupt-controller/arm-gic.h>
>>>>>> @@ -19,6 +20,11 @@ / {
>>>>>>        #size-cells = <2>;
>>>>>>
>>>>>>        clocks {
>>>>>> +            cmn_pll_ref_clk: cmn-pll-ref-clk {
>>>>>> +                    compatible = "fixed-clock";
>>>>>> +                    #clock-cells = <0>;
>>>>>> +            };
>>>>>
>>>>> Which block provides this clock? If it is provided by the external XO
>>>>> then it should not be a part of the SoC dtsi.
>>>>
>>>> The on-chip WiFi block supplies this reference clock. So keeping it in
>>>> the SoC DTSI is perhaps appropriate.
>>>
>>> Then maybe it should be provided by the WiFi device node? At least you
>>> should document your design decisions in the commit message.
>>
>> This CMN PLL reference clock is fixed rate and is automatically
>> generated by the SoC's internal Wi-Fi hardware block with no software
>> configuration required from the Wi-Fi side.
>>
>> Sure, I will enhance the commit message to add the information on the
>> fixed reference clock from Wi-Fi block. Hope this is ok.
> 
> We have other fixed clocks which are provided by hardware blocks.
> Without additional details it is impossible to answer whether it is fine
> or not.

There is an XO on the board which supplies reference clock (48Mhz or
96Mhz) to the Wi-Fi block on the SoC. There is a multiplier/divider in
the Wi-Fi block, which ensures the output reference clock of 48Mhz is
supplied to CMN PLL block.

In summary, below is the path to receive the reference clock at CMN PLL:
The clock path is .XO (48 MHZ/96 MHZ) --> WiFi (multiplier/divider) 
-->(48 MHZ) --> CMN PLL.

There is no software configuration required for the entire path, as it
is fully controlled by bootstrap pins on the board. There are bootstrap
pins for selecting the selecting the XO frequency (48Mhz or 96Mhz) and
based on this, the divider is automatically selected by HW (1 for 48Mhz,
2 for 96Mhz), to ensure output clock to CMN PLL is 48Mhz.

> 
>>
>>>
>>> Also, I don't think this node passes DT schema validation. Did you check it?
>>
>> Yes, the DT is validated against the schema, I have shared the logs
>> below. Could you please let me know If anything needs rectification?
> 
> I see, you are setting the cmn_pll_ref_clk frequency in the
> ipq9574-rdp-common.dtsi file. If the PLL is internal to the SoC, why is
> the frequency set outside of it? Is it generated by multiplying the XO
> clk? Should you be using fixed-factor clock instead?
> 

Since the reference clock is controlled by bootstrap pins on the board,
it may be appropriate to define the frequency for this reference clock
in the board DTS. Given the clock tree described above, I will update
the cmn_pll_ref_clk to define it as a fixed-factor clock as per your
suggestion, with its frequency and factors configured in board DTSI.
These values defined in rdp-common.dtsi will be default values that can
be overridden if necessary by different boards. Hope this approach is
fine.

In ipq9574.dtsi:
cmn_pll_ref_clk: cmn-pll-ref-clk { 
  
  

         compatible = "fixed-factor-clock"; 
  
  

         clocks = <&xo_clk>; 
  
  
  
  
  

	#clock-cells = <0>;
};

xo_clk: xo {
	compatible = "fixed-clock";
	#clock-cells = <0>;
};

In ipq9574-rdp-common.dtsi.
&cmn_pll_ref_clk {
	clock-div = <1>;
	clock-mult = <1>;
};

&xo_clk {
	clock-frequency = <48000000>;
}

Powered by blists - more mailing lists