lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <g3wh3yjkzgdfwbj4hnabtmziwsmqn3cloffblhshrkgj3tx4h7@2w2ho2mvs5c6>
Date: Fri, 25 Oct 2024 17:05:31 +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 Wed, Oct 23, 2024 at 09:05:09PM +0800, Jie Luo wrote:
> 
> 
> 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.

If the clock is always fixed to this frequency, then it's ok, thank you.

> 
> > 
> > > 
> > > > 
> > > > 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>;
> }

Sounds perfect, thank you!


-- 
With best wishes
Dmitry

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ