| Message ID | 20230825091234.32713-7-quic_devipriy@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add NSS clock controller support for IPQ9574 | expand |
On Fri, 25 Aug 2023 at 12:15, Devi Priya <quic_devipriy@quicinc.com> wrote: > > Add a node for the nss clock controller found on ipq9574 based devices. > > Signed-off-by: Devi Priya <quic_devipriy@quicinc.com> > --- > Changes in V2: > - Dropped the fixed clock node gcc_gpll0_out_aux and added > support for the same in gcc driver > - Updated the node name to clock-controller@39b00000 > - Added clock-names to retrieve the nssnoc clocks and add them > to the list of pm clocks in nss driver > > arch/arm64/boot/dts/qcom/ipq9574.dtsi | 48 +++++++++++++++++++++++++++ > 1 file changed, 48 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi > index 51aba071c1eb..903311547e96 100644 > --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi > +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi > @@ -10,6 +10,8 @@ > #include <dt-bindings/clock/qcom,ipq9574-gcc.h> > #include <dt-bindings/interrupt-controller/arm-gic.h> > #include <dt-bindings/reset/qcom,ipq9574-gcc.h> > +#include <dt-bindings/clock/qcom,ipq9574-nsscc.h> > +#include <dt-bindings/reset/qcom,ipq9574-nsscc.h> > #include <dt-bindings/thermal/thermal.h> > > / { > @@ -18,6 +20,24 @@ / { > #size-cells = <2>; > > clocks { > + bias_pll_cc_clk: bias-pll-cc-clk { > + compatible = "fixed-clock"; > + clock-frequency = <1200000000>; > + #clock-cells = <0>; > + }; > + > + bias_pll_nss_noc_clk: bias-pll-nss-noc-clk { > + compatible = "fixed-clock"; > + clock-frequency = <461500000>; > + #clock-cells = <0>; > + }; > + > + bias_pll_ubi_nc_clk: bias-pll-ubi-nc-clk { > + compatible = "fixed-clock"; > + clock-frequency = <353000000>; > + #clock-cells = <0>; > + }; Which part provides these clocks? > + > sleep_clk: sleep-clk { > compatible = "fixed-clock"; > #clock-cells = <0>; > @@ -722,6 +742,34 @@ frame@b128000 { > status = "disabled"; > }; > }; > + > + nsscc: clock-controller@39b00000 { > + compatible = "qcom,ipq9574-nsscc"; > + reg = <0x39b00000 0x80000>; > + clocks = <&gcc GCC_NSSNOC_NSSCC_CLK>, > + <&gcc GCC_NSSNOC_SNOC_CLK>, > + <&gcc GCC_NSSNOC_SNOC_1_CLK>, > + <&bias_pll_cc_clk>, > + <&bias_pll_nss_noc_clk>, > + <&bias_pll_ubi_nc_clk>, > + <&gcc GPLL0_OUT_AUX>, > + <0>, > + <0>, > + <0>, > + <0>, > + <0>, > + <0>, > + <&xo_board_clk>; If you move xo_board closer to the start of the list, it will be slightly easier to review. > + clock-names = "nssnoc_nsscc", "nssnoc_snoc", "nssnoc_snoc_1", > + "bias_pll_cc_clk", "bias_pll_nss_noc_clk", > + "bias_pll_ubi_nc_clk", "gpll0_out_aux", "uniphy0_nss_rx_clk", > + "uniphy0_nss_tx_clk", "uniphy1_nss_rx_clk", > + "uniphy1_nss_tx_clk", "uniphy2_nss_rx_clk", > + "uniphy2_nss_tx_clk", "xo_board_clk"; You are using clock indices. Please drop clock-names. > + #clock-cells = <1>; > + #reset-cells = <1>; > + #power-domain-cells = <1>; > + }; > }; > > thermal-zones { > -- > 2.34.1 >
On 8/25/2023 4:58 PM, Dmitry Baryshkov wrote: > On Fri, 25 Aug 2023 at 12:15, Devi Priya <quic_devipriy@quicinc.com> wrote: >> >> Add a node for the nss clock controller found on ipq9574 based devices. >> >> Signed-off-by: Devi Priya <quic_devipriy@quicinc.com> >> --- >> Changes in V2: >> - Dropped the fixed clock node gcc_gpll0_out_aux and added >> support for the same in gcc driver >> - Updated the node name to clock-controller@39b00000 >> - Added clock-names to retrieve the nssnoc clocks and add them >> to the list of pm clocks in nss driver >> >> arch/arm64/boot/dts/qcom/ipq9574.dtsi | 48 +++++++++++++++++++++++++++ >> 1 file changed, 48 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi >> index 51aba071c1eb..903311547e96 100644 >> --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi >> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi >> @@ -10,6 +10,8 @@ >> #include <dt-bindings/clock/qcom,ipq9574-gcc.h> >> #include <dt-bindings/interrupt-controller/arm-gic.h> >> #include <dt-bindings/reset/qcom,ipq9574-gcc.h> >> +#include <dt-bindings/clock/qcom,ipq9574-nsscc.h> >> +#include <dt-bindings/reset/qcom,ipq9574-nsscc.h> >> #include <dt-bindings/thermal/thermal.h> >> >> / { >> @@ -18,6 +20,24 @@ / { >> #size-cells = <2>; >> >> clocks { >> + bias_pll_cc_clk: bias-pll-cc-clk { >> + compatible = "fixed-clock"; >> + clock-frequency = <1200000000>; >> + #clock-cells = <0>; >> + }; >> + >> + bias_pll_nss_noc_clk: bias-pll-nss-noc-clk { >> + compatible = "fixed-clock"; >> + clock-frequency = <461500000>; >> + #clock-cells = <0>; >> + }; >> + >> + bias_pll_ubi_nc_clk: bias-pll-ubi-nc-clk { >> + compatible = "fixed-clock"; >> + clock-frequency = <353000000>; >> + #clock-cells = <0>; >> + }; > > Which part provides these clocks? The Bias PLL generates these clocks based on the reference clock. > >> + >> sleep_clk: sleep-clk { >> compatible = "fixed-clock"; >> #clock-cells = <0>; >> @@ -722,6 +742,34 @@ frame@b128000 { >> status = "disabled"; >> }; >> }; >> + >> + nsscc: clock-controller@39b00000 { >> + compatible = "qcom,ipq9574-nsscc"; >> + reg = <0x39b00000 0x80000>; >> + clocks = <&gcc GCC_NSSNOC_NSSCC_CLK>, >> + <&gcc GCC_NSSNOC_SNOC_CLK>, >> + <&gcc GCC_NSSNOC_SNOC_1_CLK>, >> + <&bias_pll_cc_clk>, >> + <&bias_pll_nss_noc_clk>, >> + <&bias_pll_ubi_nc_clk>, >> + <&gcc GPLL0_OUT_AUX>, >> + <0>, >> + <0>, >> + <0>, >> + <0>, >> + <0>, >> + <0>, >> + <&xo_board_clk>; > > If you move xo_board closer to the start of the list, it will be > slightly easier to review. Sure okay > >> + clock-names = "nssnoc_nsscc", "nssnoc_snoc", "nssnoc_snoc_1", >> + "bias_pll_cc_clk", "bias_pll_nss_noc_clk", >> + "bias_pll_ubi_nc_clk", "gpll0_out_aux", "uniphy0_nss_rx_clk", >> + "uniphy0_nss_tx_clk", "uniphy1_nss_rx_clk", >> + "uniphy1_nss_tx_clk", "uniphy2_nss_rx_clk", >> + "uniphy2_nss_tx_clk", "xo_board_clk"; > > You are using clock indices. Please drop clock-names. Sure okay Thanks, Devi Priya > >> + #clock-cells = <1>; >> + #reset-cells = <1>; >> + #power-domain-cells = <1>; >> + }; >> }; >> >> thermal-zones { >> -- >> 2.34.1 >> > >
Hi Dmitry, On Fri, Aug 25, 2023 at 1:28 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > On Fri, 25 Aug 2023 at 12:15, Devi Priya <quic_devipriy@quicinc.com> wrote: > > Add a node for the nss clock controller found on ipq9574 based devices. > > > > Signed-off-by: Devi Priya <quic_devipriy@quicinc.com> > > --- > > Changes in V2: > > - Dropped the fixed clock node gcc_gpll0_out_aux and added > > support for the same in gcc driver > > - Updated the node name to clock-controller@39b00000 > > - Added clock-names to retrieve the nssnoc clocks and add them > > to the list of pm clocks in nss driver > > > > arch/arm64/boot/dts/qcom/ipq9574.dtsi | 48 +++++++++++++++++++++++++++ > > 1 file changed, 48 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi > > index 51aba071c1eb..903311547e96 100644 > > --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi > > +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi > > @@ -722,6 +742,34 @@ frame@b128000 { > > status = "disabled"; > > }; > > }; > > + > > + nsscc: clock-controller@39b00000 { > > + compatible = "qcom,ipq9574-nsscc"; > > + reg = <0x39b00000 0x80000>; > > + clocks = <&gcc GCC_NSSNOC_NSSCC_CLK>, > > + <&gcc GCC_NSSNOC_SNOC_CLK>, > > + <&gcc GCC_NSSNOC_SNOC_1_CLK>, > > + <&bias_pll_cc_clk>, > > + <&bias_pll_nss_noc_clk>, > > + <&bias_pll_ubi_nc_clk>, > > + <&gcc GPLL0_OUT_AUX>, > > + <0>, > > + <0>, > > + <0>, > > + <0>, > > + <0>, > > + <0>, > > + <&xo_board_clk>; > > If you move xo_board closer to the start of the list, it will be > slightly easier to review. > > > + clock-names = "nssnoc_nsscc", "nssnoc_snoc", "nssnoc_snoc_1", > > + "bias_pll_cc_clk", "bias_pll_nss_noc_clk", > > + "bias_pll_ubi_nc_clk", "gpll0_out_aux", "uniphy0_nss_rx_clk", > > + "uniphy0_nss_tx_clk", "uniphy1_nss_rx_clk", > > + "uniphy1_nss_tx_clk", "uniphy2_nss_rx_clk", > > + "uniphy2_nss_tx_clk", "xo_board_clk"; > > You are using clock indices. Please drop clock-names. What do you mean by "using clock indices"? Note that the "clock-names" property is required according to the DT bindings. Gr{oetje,eeting}s, Geert
On 13/09/2023 10:23, Geert Uytterhoeven wrote: >> >>> + clock-names = "nssnoc_nsscc", "nssnoc_snoc", "nssnoc_snoc_1", >>> + "bias_pll_cc_clk", "bias_pll_nss_noc_clk", >>> + "bias_pll_ubi_nc_clk", "gpll0_out_aux", "uniphy0_nss_rx_clk", >>> + "uniphy0_nss_tx_clk", "uniphy1_nss_rx_clk", >>> + "uniphy1_nss_tx_clk", "uniphy2_nss_rx_clk", >>> + "uniphy2_nss_tx_clk", "xo_board_clk"; >> >> You are using clock indices. Please drop clock-names. > > What do you mean by "using clock indices"? > Note that the "clock-names" property is required according to the DT bindings. Indeed, thanks for pointing this out. Probably bindings should be changed. Best regards, Krzysztof
Hi Krzysztof, On Wed, Sep 13, 2023 at 10:26 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 13/09/2023 10:23, Geert Uytterhoeven wrote: > >> > >>> + clock-names = "nssnoc_nsscc", "nssnoc_snoc", "nssnoc_snoc_1", > >>> + "bias_pll_cc_clk", "bias_pll_nss_noc_clk", > >>> + "bias_pll_ubi_nc_clk", "gpll0_out_aux", "uniphy0_nss_rx_clk", > >>> + "uniphy0_nss_tx_clk", "uniphy1_nss_rx_clk", > >>> + "uniphy1_nss_tx_clk", "uniphy2_nss_rx_clk", > >>> + "uniphy2_nss_tx_clk", "xo_board_clk"; > >> > >> You are using clock indices. Please drop clock-names. > > > > What do you mean by "using clock indices"? > > Note that the "clock-names" property is required according to the DT bindings. > > Indeed, thanks for pointing this out. Probably bindings should be changed. But what's so great about not having "clock-names"? There are _14_ input clocks. Gr{oetje,eeting}s, Geert
On 13.09.2023 10:38, Geert Uytterhoeven wrote: > Hi Krzysztof, > > On Wed, Sep 13, 2023 at 10:26 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> On 13/09/2023 10:23, Geert Uytterhoeven wrote: >>>> >>>>> + clock-names = "nssnoc_nsscc", "nssnoc_snoc", "nssnoc_snoc_1", >>>>> + "bias_pll_cc_clk", "bias_pll_nss_noc_clk", >>>>> + "bias_pll_ubi_nc_clk", "gpll0_out_aux", "uniphy0_nss_rx_clk", >>>>> + "uniphy0_nss_tx_clk", "uniphy1_nss_rx_clk", >>>>> + "uniphy1_nss_tx_clk", "uniphy2_nss_rx_clk", >>>>> + "uniphy2_nss_tx_clk", "xo_board_clk"; >>>> >>>> You are using clock indices. Please drop clock-names. >>> >>> What do you mean by "using clock indices"? >>> Note that the "clock-names" property is required according to the DT bindings. >> >> Indeed, thanks for pointing this out. Probably bindings should be changed. > > But what's so great about not having "clock-names"? > There are _14_ input clocks. clk_parent_data has an "index" member, which lets us bind clocks[n] to parent[n]. With the DT properties being ABI, including the order of entries within, that lets us get rid of clock-names and the matching is marginally faster. Konrad
On 13/09/2023 10:38, Geert Uytterhoeven wrote: > Hi Krzysztof, > > On Wed, Sep 13, 2023 at 10:26 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> On 13/09/2023 10:23, Geert Uytterhoeven wrote: >>>> >>>>> + clock-names = "nssnoc_nsscc", "nssnoc_snoc", "nssnoc_snoc_1", >>>>> + "bias_pll_cc_clk", "bias_pll_nss_noc_clk", >>>>> + "bias_pll_ubi_nc_clk", "gpll0_out_aux", "uniphy0_nss_rx_clk", >>>>> + "uniphy0_nss_tx_clk", "uniphy1_nss_rx_clk", >>>>> + "uniphy1_nss_tx_clk", "uniphy2_nss_rx_clk", >>>>> + "uniphy2_nss_tx_clk", "xo_board_clk"; >>>> >>>> You are using clock indices. Please drop clock-names. >>> >>> What do you mean by "using clock indices"? >>> Note that the "clock-names" property is required according to the DT bindings. >> >> Indeed, thanks for pointing this out. Probably bindings should be changed. > > But what's so great about not having "clock-names"? > There are _14_ input clocks. There is nothing particularly wrong. They are just not used by Linux implementation and they confuse people into thinking items are not strictly ordered. Thus agreement long time ago for Qualcomm clock controllers was to drop the clock-names to avoid that confusion and make it explicit. Best regards, Krzysztof
diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi index 51aba071c1eb..903311547e96 100644 --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi @@ -10,6 +10,8 @@ #include <dt-bindings/clock/qcom,ipq9574-gcc.h> #include <dt-bindings/interrupt-controller/arm-gic.h> #include <dt-bindings/reset/qcom,ipq9574-gcc.h> +#include <dt-bindings/clock/qcom,ipq9574-nsscc.h> +#include <dt-bindings/reset/qcom,ipq9574-nsscc.h> #include <dt-bindings/thermal/thermal.h> / { @@ -18,6 +20,24 @@ / { #size-cells = <2>; clocks { + bias_pll_cc_clk: bias-pll-cc-clk { + compatible = "fixed-clock"; + clock-frequency = <1200000000>; + #clock-cells = <0>; + }; + + bias_pll_nss_noc_clk: bias-pll-nss-noc-clk { + compatible = "fixed-clock"; + clock-frequency = <461500000>; + #clock-cells = <0>; + }; + + bias_pll_ubi_nc_clk: bias-pll-ubi-nc-clk { + compatible = "fixed-clock"; + clock-frequency = <353000000>; + #clock-cells = <0>; + }; + sleep_clk: sleep-clk { compatible = "fixed-clock"; #clock-cells = <0>; @@ -722,6 +742,34 @@ frame@b128000 { status = "disabled"; }; }; + + nsscc: clock-controller@39b00000 { + compatible = "qcom,ipq9574-nsscc"; + reg = <0x39b00000 0x80000>; + clocks = <&gcc GCC_NSSNOC_NSSCC_CLK>, + <&gcc GCC_NSSNOC_SNOC_CLK>, + <&gcc GCC_NSSNOC_SNOC_1_CLK>, + <&bias_pll_cc_clk>, + <&bias_pll_nss_noc_clk>, + <&bias_pll_ubi_nc_clk>, + <&gcc GPLL0_OUT_AUX>, + <0>, + <0>, + <0>, + <0>, + <0>, + <0>, + <&xo_board_clk>; + clock-names = "nssnoc_nsscc", "nssnoc_snoc", "nssnoc_snoc_1", + "bias_pll_cc_clk", "bias_pll_nss_noc_clk", + "bias_pll_ubi_nc_clk", "gpll0_out_aux", "uniphy0_nss_rx_clk", + "uniphy0_nss_tx_clk", "uniphy1_nss_rx_clk", + "uniphy1_nss_tx_clk", "uniphy2_nss_rx_clk", + "uniphy2_nss_tx_clk", "xo_board_clk"; + #clock-cells = <1>; + #reset-cells = <1>; + #power-domain-cells = <1>; + }; }; thermal-zones {
Add a node for the nss clock controller found on ipq9574 based devices. Signed-off-by: Devi Priya <quic_devipriy@quicinc.com> --- Changes in V2: - Dropped the fixed clock node gcc_gpll0_out_aux and added support for the same in gcc driver - Updated the node name to clock-controller@39b00000 - Added clock-names to retrieve the nssnoc clocks and add them to the list of pm clocks in nss driver arch/arm64/boot/dts/qcom/ipq9574.dtsi | 48 +++++++++++++++++++++++++++ 1 file changed, 48 insertions(+)