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@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> > > > > > > ---
> > > > > > > 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
