Message-ID: <cqgkc3qpupbv47rqxiyhe2m466zxcxepyfcgyaieo2sggffprx@mstqi4pqoiqc>
Date: Fri, 18 Oct 2024 18:38:48 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Jie Luo <quic_luoj@...cinc.com>
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 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.

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

-- 
With best wishes
Dmitry

Powered by blists - more mailing lists