| Message ID | 20241015-qcom_ipq_cmnpll-v4-4-27817fbe3505@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add CMN PLL clock controller driver for IPQ9574 | expand |
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@quicinc.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. > + > sleep_clk: sleep-clk { > compatible = "fixed-clock"; > #clock-cells = <0>; > @@ -243,6 +249,18 @@ mdio: mdio@90000 { > status = "disabled"; > }; > > + cmn_pll: clock-controller@9b000 { > + compatible = "qcom,ipq9574-cmn-pll"; > + reg = <0x0009b000 0x800>; > + clocks = <&cmn_pll_ref_clk>, > + <&gcc GCC_CMN_12GPLL_AHB_CLK>, > + <&gcc GCC_CMN_12GPLL_SYS_CLK>; > + clock-names = "ref", "ahb", "sys"; > + #clock-cells = <1>; > + assigned-clocks = <&cmn_pll CMN_PLL_CLK>; > + assigned-clock-rates-u64 = /bits/ 64 <12000000000>; > + }; > + > qfprom: efuse@a4000 { > compatible = "qcom,ipq9574-qfprom", "qcom,qfprom"; > reg = <0x000a4000 0x5a1>; > > -- > 2.34.1 >
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@quicinc.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. > >> + >> sleep_clk: sleep-clk { >> compatible = "fixed-clock"; >> #clock-cells = <0>; >> @@ -243,6 +249,18 @@ mdio: mdio@90000 { >> status = "disabled"; >> }; >> >> + cmn_pll: clock-controller@9b000 { >> + compatible = "qcom,ipq9574-cmn-pll"; >> + reg = <0x0009b000 0x800>; >> + clocks = <&cmn_pll_ref_clk>, >> + <&gcc GCC_CMN_12GPLL_AHB_CLK>, >> + <&gcc GCC_CMN_12GPLL_SYS_CLK>; >> + clock-names = "ref", "ahb", "sys"; >> + #clock-cells = <1>; >> + assigned-clocks = <&cmn_pll CMN_PLL_CLK>; >> + assigned-clock-rates-u64 = /bits/ 64 <12000000000>; >> + }; >> + >> qfprom: efuse@a4000 { >> compatible = "qcom,ipq9574-qfprom", "qcom,qfprom"; >> reg = <0x000a4000 0x5a1>; >> >> -- >> 2.34.1 >> >
On Fri, 18 Oct 2024 at 09:55, Jie Luo <quic_luoj@quicinc.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@quicinc.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. Also, I don't think this node passes DT schema validation. Did you check it? > > > > >> + > >> sleep_clk: sleep-clk { > >> compatible = "fixed-clock"; > >> #clock-cells = <0>; > >> @@ -243,6 +249,18 @@ mdio: mdio@90000 { > >> status = "disabled"; > >> }; > >> > >> + cmn_pll: clock-controller@9b000 { > >> + compatible = "qcom,ipq9574-cmn-pll"; > >> + reg = <0x0009b000 0x800>; > >> + clocks = <&cmn_pll_ref_clk>, > >> + <&gcc GCC_CMN_12GPLL_AHB_CLK>, > >> + <&gcc GCC_CMN_12GPLL_SYS_CLK>; > >> + clock-names = "ref", "ahb", "sys"; > >> + #clock-cells = <1>; > >> + assigned-clocks = <&cmn_pll CMN_PLL_CLK>; > >> + assigned-clock-rates-u64 = /bits/ 64 <12000000000>; > >> + }; > >> + > >> qfprom: efuse@a4000 { > >> compatible = "qcom,ipq9574-qfprom", "qcom,qfprom"; > >> reg = <0x000a4000 0x5a1>; > >> > >> -- > >> 2.34.1 > >> > > >
On 10/18/2024 4:11 PM, Dmitry Baryshkov wrote: > On Fri, 18 Oct 2024 at 09:55, Jie Luo <quic_luoj@quicinc.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@quicinc.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. > > 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? dt-doc-validate --version 2024.9 make ARCH=arm64 DT_SCHEMA_FILES=qcom,ipq9574-cmn-pll.yaml CHECK_DTBS=y qcom/ipq9574-rdp433.dtb DTC [C] arch/arm64/boot/dts/qcom/ipq9574-rdp433.dtb make ARCH=arm64 dt_binding_check DT_SCHEMA_FILES=qcom,ipq9574-cmn-pll.yaml SCHEMA Documentation/devicetree/bindings/processed-schema.json CHKDT Documentation/devicetree/bindings LINT Documentation/devicetree/bindings DTEX Documentation/devicetree/bindings/clock/qcom,ipq9574-cmn-pll.example.dts DTC [C] Documentation/devicetree/bindings/clock/qcom,ipq9574-cmn-pll.example.dtb make ARCH=arm64 CHECK_DTBS=y qcom/ipq9574-rdp433.dtb DTC [C] arch/arm64/boot/dts/qcom/ipq9574-rdp433.dtb /local/mnt2/workspace/luoj/projects/opensrc/linux-next-cmnpll-validation/linux-next/arch/arm64/boot/dts/qcom/ipq9574-rdp433.dtb: usb@8af8800: interrupt-names: ['pwr_event'] is too short from schema $id: http://devicetree.org/schemas/usb/qcom,dwc3.yaml# /local/mnt2/workspace/luoj/projects/opensrc/linux-next-cmnpll-validation/linux-next/arch/arm64/boot/dts/qcom/ipq9574-rdp433.dtb: usb@8af8800: interrupts-extended: [[1, 0, 134, 4]] is too short from schema $id: http://devicetree.org/schemas/usb/qcom,dwc3.yaml# > >> >>> >>>> + >>>> sleep_clk: sleep-clk { >>>> compatible = "fixed-clock"; >>>> #clock-cells = <0>; >>>> @@ -243,6 +249,18 @@ mdio: mdio@90000 { >>>> status = "disabled"; >>>> }; >>>> >>>> + cmn_pll: clock-controller@9b000 { >>>> + compatible = "qcom,ipq9574-cmn-pll"; >>>> + reg = <0x0009b000 0x800>; >>>> + clocks = <&cmn_pll_ref_clk>, >>>> + <&gcc GCC_CMN_12GPLL_AHB_CLK>, >>>> + <&gcc GCC_CMN_12GPLL_SYS_CLK>; >>>> + clock-names = "ref", "ahb", "sys"; >>>> + #clock-cells = <1>; >>>> + assigned-clocks = <&cmn_pll CMN_PLL_CLK>; >>>> + assigned-clock-rates-u64 = /bits/ 64 <12000000000>; >>>> + }; >>>> + >>>> qfprom: efuse@a4000 { >>>> compatible = "qcom,ipq9574-qfprom", "qcom,qfprom"; >>>> reg = <0x000a4000 0x5a1>; >>>> >>>> -- >>>> 2.34.1 >>>> >>> >> > >
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@quicinc.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@quicinc.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?
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@quicinc.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@quicinc.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>; }
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@quicinc.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@quicinc.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!
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>; + }; + sleep_clk: sleep-clk { compatible = "fixed-clock"; #clock-cells = <0>; @@ -243,6 +249,18 @@ mdio: mdio@90000 { status = "disabled"; }; + cmn_pll: clock-controller@9b000 { + compatible = "qcom,ipq9574-cmn-pll"; + reg = <0x0009b000 0x800>; + clocks = <&cmn_pll_ref_clk>, + <&gcc GCC_CMN_12GPLL_AHB_CLK>, + <&gcc GCC_CMN_12GPLL_SYS_CLK>; + clock-names = "ref", "ahb", "sys"; + #clock-cells = <1>; + assigned-clocks = <&cmn_pll CMN_PLL_CLK>; + assigned-clock-rates-u64 = /bits/ 64 <12000000000>; + }; + qfprom: efuse@a4000 { compatible = "qcom,ipq9574-qfprom", "qcom,qfprom"; reg = <0x000a4000 0x5a1>;
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@quicinc.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(-)