diff mbox series

[V2,6/7] arm64: dts: qcom: ipq9574: Add support for nsscc node

Message ID 20230825091234.32713-7-quic_devipriy@quicinc.com
State New
Headers show
Series Add NSS clock controller support for IPQ9574 | expand

Commit Message

Devi Priya Aug. 25, 2023, 9:12 a.m. UTC
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(+)

Comments

Devi Priya Sept. 13, 2023, 3:32 a.m. UTC | #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
>>
> 
>
Geert Uytterhoeven Sept. 13, 2023, 8:23 a.m. UTC | #2
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
Krzysztof Kozlowski Sept. 13, 2023, 8:26 a.m. UTC | #3
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
Geert Uytterhoeven Sept. 13, 2023, 8:38 a.m. UTC | #4
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
Konrad Dybcio Sept. 13, 2023, 8:43 a.m. UTC | #5
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
Krzysztof Kozlowski Sept. 13, 2023, 8:55 a.m. UTC | #6
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 mbox series

Patch

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 {