diff mbox series

[v5,4/5] clk: qcom: ipq9574: Use icc-clk for enabling NoC related clocks

Message ID 20240328075936.223461-5-quic_varada@quicinc.com
State Superseded
Headers show
Series Add interconnect driver for IPQ9574 SoC | expand

Commit Message

Varadarajan Narayanan March 28, 2024, 7:59 a.m. UTC
Use the icc-clk framework to enable few clocks to be able to
create paths and use the peripherals connected on those NoCs.

Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
v5: Split from common.c changes into separate patch
    No functional changes
---
 drivers/clk/qcom/Kconfig       |  2 ++
 drivers/clk/qcom/gcc-ipq9574.c | 54 ++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+)

Comments

Varadarajan Narayanan March 30, 2024, 9:30 a.m. UTC | #1
On Fri, Mar 29, 2024 at 01:10:03PM +0100, Krzysztof Kozlowski wrote:
> On 29/03/2024 11:55, Varadarajan Narayanan wrote:
> >>> +
> >>> +enum {
> >>> +       ICC_ANOC_PCIE0,
> >>> +       ICC_SNOC_PCIE0,
> >>> +       ICC_ANOC_PCIE1,
> >>> +       ICC_SNOC_PCIE1,
> >>> +       ICC_ANOC_PCIE2,
> >>> +       ICC_SNOC_PCIE2,
> >>> +       ICC_ANOC_PCIE3,
> >>> +       ICC_SNOC_PCIE3,
> >>> +       ICC_SNOC_USB,
> >>> +       ICC_ANOC_USB_AXI,
> >>> +       ICC_NSSNOC_NSSCC,
> >>> +       ICC_NSSNOC_SNOC_0,
> >>> +       ICC_NSSNOC_SNOC_1,
> >>> +       ICC_NSSNOC_PCNOC_1,
> >>> +       ICC_NSSNOC_QOSGEN_REF,
> >>> +       ICC_NSSNOC_TIMEOUT_REF,
> >>> +       ICC_NSSNOC_XO_DCD,
> >>> +       ICC_NSSNOC_ATB,
> >>> +       ICC_MEM_NOC_NSSNOC,
> >>> +       ICC_NSSNOC_MEMNOC,
> >>> +       ICC_NSSNOC_MEM_NOC_1,
> >>> +};
> >>
> >> Are these supposed to be in a dt-binding header?
> >
> > Since these don't directly relate to the ids in the dt-bindings
> > not sure if they will be permitted there. Will move and post a
> > new version and get feedback.
>
> You can answer this by yourself by looking at your DTS. Do you use them
> as well in the DTS?

I can use them in the DTS. The icc-clk framework automatically
creates master and slave nodes as 'n' and 'n+1'. Hence I can have
something like this in the dt-bindings include file

	#define ICC_ANOC_PCIE0		0
	#define ICC_SNOC_PCIE0		1
		.
		.
		.
	#define ICC_NSSNOC_MEM_NOC_1	20

	#define MASTER(x)	((ICC_ ## x) * 2)
	#define SLAVE(x)	(MASTER(x) + 1)

> It's a pity we see here only parts of DTS, instead of full interconnect
> usage.

Unfortunately cannot include the pcie dts changes with this
patch, but you can refer to them at https://lore.kernel.org/linux-arm-msm/20230519090219.15925-5-quic_devipriy@quicinc.com/

The above macros will be used in the pcie node as follows

pcie0: pci@28000000 {
	compatible = "qcom,pcie-ipq9574";
	. . .
	interconnects = <&gcc MASTER(ANOC_PCIE0) &gcc SLAVE(ANOC_PCIE0)>,
			<&gcc MASTER(SNOC_PCIE0) &gcc SLAVE(SNOC_PCIE0)>;
	interconnect-names = "pcie-mem", "cpu-pcie";
	. . .
};

Hope this is acceptable.

Thanks
Varada
Krzysztof Kozlowski March 30, 2024, 10:28 a.m. UTC | #2
On 30/03/2024 10:30, Varadarajan Narayanan wrote:
> On Fri, Mar 29, 2024 at 01:10:03PM +0100, Krzysztof Kozlowski wrote:
>> On 29/03/2024 11:55, Varadarajan Narayanan wrote:
>>>>> +
>>>>> +enum {
>>>>> +       ICC_ANOC_PCIE0,
>>>>> +       ICC_SNOC_PCIE0,
>>>>> +       ICC_ANOC_PCIE1,
>>>>> +       ICC_SNOC_PCIE1,
>>>>> +       ICC_ANOC_PCIE2,
>>>>> +       ICC_SNOC_PCIE2,
>>>>> +       ICC_ANOC_PCIE3,
>>>>> +       ICC_SNOC_PCIE3,
>>>>> +       ICC_SNOC_USB,
>>>>> +       ICC_ANOC_USB_AXI,
>>>>> +       ICC_NSSNOC_NSSCC,
>>>>> +       ICC_NSSNOC_SNOC_0,
>>>>> +       ICC_NSSNOC_SNOC_1,
>>>>> +       ICC_NSSNOC_PCNOC_1,
>>>>> +       ICC_NSSNOC_QOSGEN_REF,
>>>>> +       ICC_NSSNOC_TIMEOUT_REF,
>>>>> +       ICC_NSSNOC_XO_DCD,
>>>>> +       ICC_NSSNOC_ATB,
>>>>> +       ICC_MEM_NOC_NSSNOC,
>>>>> +       ICC_NSSNOC_MEMNOC,
>>>>> +       ICC_NSSNOC_MEM_NOC_1,
>>>>> +};
>>>>
>>>> Are these supposed to be in a dt-binding header?
>>>
>>> Since these don't directly relate to the ids in the dt-bindings
>>> not sure if they will be permitted there. Will move and post a
>>> new version and get feedback.
>>
>> You can answer this by yourself by looking at your DTS. Do you use them
>> as well in the DTS?
> 
> I can use them in the DTS. The icc-clk framework automatically
> creates master and slave nodes as 'n' and 'n+1'. Hence I can have
> something like this in the dt-bindings include file
> 
> 	#define ICC_ANOC_PCIE0		0
> 	#define ICC_SNOC_PCIE0		1
> 		.
> 		.
> 		.
> 	#define ICC_NSSNOC_MEM_NOC_1	20
> 
> 	#define MASTER(x)	((ICC_ ## x) * 2)
> 	#define SLAVE(x)	(MASTER(x) + 1)

I don't understand this or maybe I misunderstood the purpose of this
define. It does not matter if you "can" use something in DT. The
question is: do you use them.

> 
>> It's a pity we see here only parts of DTS, instead of full interconnect
>> usage.
> 
> Unfortunately cannot include the pcie dts changes with this
> patch, but you can refer to them at https://lore.kernel.org/linux-arm-msm/20230519090219.15925-5-quic_devipriy@quicinc.com/
> 
> The above macros will be used in the pcie node as follows
> 
> pcie0: pci@28000000 {
> 	compatible = "qcom,pcie-ipq9574";
> 	. . .
> 	interconnects = <&gcc MASTER(ANOC_PCIE0) &gcc SLAVE(ANOC_PCIE0)>,
> 			<&gcc MASTER(SNOC_PCIE0) &gcc SLAVE(SNOC_PCIE0)>;
> 	interconnect-names = "pcie-mem", "cpu-pcie";

Then why did you add header which is not used?

I will respond there...

Best regards,
Krzysztof
Varadarajan Narayanan March 30, 2024, 11:17 a.m. UTC | #3
On Sat, Mar 30, 2024 at 11:28:09AM +0100, Krzysztof Kozlowski wrote:
> On 30/03/2024 10:30, Varadarajan Narayanan wrote:
> > On Fri, Mar 29, 2024 at 01:10:03PM +0100, Krzysztof Kozlowski wrote:
> >> On 29/03/2024 11:55, Varadarajan Narayanan wrote:
> >>>>> +
> >>>>> +enum {
> >>>>> +       ICC_ANOC_PCIE0,
> >>>>> +       ICC_SNOC_PCIE0,
> >>>>> +       ICC_ANOC_PCIE1,
> >>>>> +       ICC_SNOC_PCIE1,
> >>>>> +       ICC_ANOC_PCIE2,
> >>>>> +       ICC_SNOC_PCIE2,
> >>>>> +       ICC_ANOC_PCIE3,
> >>>>> +       ICC_SNOC_PCIE3,
> >>>>> +       ICC_SNOC_USB,
> >>>>> +       ICC_ANOC_USB_AXI,
> >>>>> +       ICC_NSSNOC_NSSCC,
> >>>>> +       ICC_NSSNOC_SNOC_0,
> >>>>> +       ICC_NSSNOC_SNOC_1,
> >>>>> +       ICC_NSSNOC_PCNOC_1,
> >>>>> +       ICC_NSSNOC_QOSGEN_REF,
> >>>>> +       ICC_NSSNOC_TIMEOUT_REF,
> >>>>> +       ICC_NSSNOC_XO_DCD,
> >>>>> +       ICC_NSSNOC_ATB,
> >>>>> +       ICC_MEM_NOC_NSSNOC,
> >>>>> +       ICC_NSSNOC_MEMNOC,
> >>>>> +       ICC_NSSNOC_MEM_NOC_1,
> >>>>> +};
> >>>>
> >>>> Are these supposed to be in a dt-binding header?
> >>>
> >>> Since these don't directly relate to the ids in the dt-bindings
> >>> not sure if they will be permitted there. Will move and post a
> >>> new version and get feedback.
> >>
> >> You can answer this by yourself by looking at your DTS. Do you use them
> >> as well in the DTS?
> >
> > I can use them in the DTS. The icc-clk framework automatically
> > creates master and slave nodes as 'n' and 'n+1'. Hence I can have
> > something like this in the dt-bindings include file
> >
> > 	#define ICC_ANOC_PCIE0		0
> > 	#define ICC_SNOC_PCIE0		1
> > 		.
> > 		.
> > 		.
> > 	#define ICC_NSSNOC_MEM_NOC_1	20
> >
> > 	#define MASTER(x)	((ICC_ ## x) * 2)
> > 	#define SLAVE(x)	(MASTER(x) + 1)
>
> I don't understand this or maybe I misunderstood the purpose of this
> define. It does not matter if you "can" use something in DT. The
> question is: do you use them.

Yes. It will be used fot the pcie nodes. These defines are for
specifying the endpoints. The icc driver identifies a path that
can connect these endpoints. The peripheral drivers' DT nodes
will make use of these defines.

> >> It's a pity we see here only parts of DTS, instead of full interconnect
> >> usage.
> >
> > Unfortunately cannot include the pcie dts changes with this
> > patch, but you can refer to them at https://lore.kernel.org/linux-arm-msm/20230519090219.15925-5-quic_devipriy@quicinc.com/
> >
> > The above macros will be used in the pcie node as follows
> >
> > pcie0: pci@28000000 {
> > 	compatible = "qcom,pcie-ipq9574";
> > 	. . .
> > 	interconnects = <&gcc MASTER(ANOC_PCIE0) &gcc SLAVE(ANOC_PCIE0)>,
> > 			<&gcc MASTER(SNOC_PCIE0) &gcc SLAVE(SNOC_PCIE0)>;
> > 	interconnect-names = "pcie-mem", "cpu-pcie";
>
> Then why did you add header which is not used?

Since they are a part of the interconnect driver. The peripherals
that use the interconnects will make use of them to specify the
endpoints. After changing per Boyd's comments, the header will
be used from gcc driver. Will post a new version. Kindly review
that.

Thanks
Varada

> I will respond there...
>
> Best regards,
> Krzysztof
>
kernel test robot April 1, 2024, 2:20 p.m. UTC | #4
Hi Varadarajan,

kernel test robot noticed the following build errors:

[auto build test ERROR on clk/clk-next]
[also build test ERROR on robh/for-next linus/master v6.9-rc2 next-20240328]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Varadarajan-Narayanan/dt-bindings-interconnect-Add-Qualcomm-IPQ9574-support/20240328-160522
base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
patch link:    https://lore.kernel.org/r/20240328075936.223461-5-quic_varada%40quicinc.com
patch subject: [PATCH v5 4/5] clk: qcom: ipq9574: Use icc-clk for enabling NoC related clocks
config: powerpc64-randconfig-r132-20240331 (https://download.01.org/0day-ci/archive/20240401/202404012258.MFriF5BV-lkp@intel.com/config)
compiler: powerpc64-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20240401/202404012258.MFriF5BV-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202404012258.MFriF5BV-lkp@intel.com/

All errors (new ones prefixed by >>):

   powerpc64-linux-ld: drivers/clk/qcom/common.o: in function `qcom_cc_really_probe':
>> common.c:(.text+0x980): undefined reference to `devm_icc_clk_register'
diff mbox series

Patch

diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index 8ab08e7b5b6c..af73a0b396eb 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -243,6 +243,8 @@  config IPQ_GCC_8074
 
 config IPQ_GCC_9574
 	tristate "IPQ9574 Global Clock Controller"
+	select INTERCONNECT
+	select INTERCONNECT_CLK
 	help
 	  Support for global clock controller on ipq9574 devices.
 	  Say Y if you want to use peripheral devices such as UART, SPI,
diff --git a/drivers/clk/qcom/gcc-ipq9574.c b/drivers/clk/qcom/gcc-ipq9574.c
index 0a3f846695b8..187fd9dcdf49 100644
--- a/drivers/clk/qcom/gcc-ipq9574.c
+++ b/drivers/clk/qcom/gcc-ipq9574.c
@@ -12,6 +12,7 @@ 
 
 #include <dt-bindings/clock/qcom,ipq9574-gcc.h>
 #include <dt-bindings/reset/qcom,ipq9574-gcc.h>
+#include <dt-bindings/interconnect/qcom,ipq9574.h>
 
 #include "clk-alpha-pll.h"
 #include "clk-branch.h"
@@ -4301,6 +4302,56 @@  static const struct qcom_reset_map gcc_ipq9574_resets[] = {
 	[GCC_WCSS_Q6_TBU_BCR] = { 0x12054, 0 },
 };
 
+#define IPQ_APPS_ID			9574	/* some unique value */
+
+enum {
+	ICC_ANOC_PCIE0,
+	ICC_SNOC_PCIE0,
+	ICC_ANOC_PCIE1,
+	ICC_SNOC_PCIE1,
+	ICC_ANOC_PCIE2,
+	ICC_SNOC_PCIE2,
+	ICC_ANOC_PCIE3,
+	ICC_SNOC_PCIE3,
+	ICC_SNOC_USB,
+	ICC_ANOC_USB_AXI,
+	ICC_NSSNOC_NSSCC,
+	ICC_NSSNOC_SNOC_0,
+	ICC_NSSNOC_SNOC_1,
+	ICC_NSSNOC_PCNOC_1,
+	ICC_NSSNOC_QOSGEN_REF,
+	ICC_NSSNOC_TIMEOUT_REF,
+	ICC_NSSNOC_XO_DCD,
+	ICC_NSSNOC_ATB,
+	ICC_MEM_NOC_NSSNOC,
+	ICC_NSSNOC_MEMNOC,
+	ICC_NSSNOC_MEM_NOC_1,
+};
+
+static struct clk_hw *icc_ipq9574_hws[] = {
+	[ICC_ANOC_PCIE0] = &gcc_anoc_pcie0_1lane_m_clk.clkr.hw,
+	[ICC_SNOC_PCIE0] = &gcc_anoc_pcie1_1lane_m_clk.clkr.hw,
+	[ICC_ANOC_PCIE1] = &gcc_anoc_pcie2_2lane_m_clk.clkr.hw,
+	[ICC_SNOC_PCIE1] = &gcc_anoc_pcie3_2lane_m_clk.clkr.hw,
+	[ICC_ANOC_PCIE2] = &gcc_snoc_pcie0_1lane_s_clk.clkr.hw,
+	[ICC_SNOC_PCIE2] = &gcc_snoc_pcie1_1lane_s_clk.clkr.hw,
+	[ICC_ANOC_PCIE3] = &gcc_snoc_pcie2_2lane_s_clk.clkr.hw,
+	[ICC_SNOC_PCIE3] = &gcc_snoc_pcie3_2lane_s_clk.clkr.hw,
+	[ICC_SNOC_USB] = &gcc_snoc_usb_clk.clkr.hw,
+	[ICC_ANOC_USB_AXI] = &gcc_anoc_usb_axi_clk.clkr.hw,
+	[ICC_NSSNOC_NSSCC] = &gcc_nssnoc_nsscc_clk.clkr.hw,
+	[ICC_NSSNOC_SNOC_0] = &gcc_nssnoc_snoc_clk.clkr.hw,
+	[ICC_NSSNOC_SNOC_1] = &gcc_nssnoc_snoc_1_clk.clkr.hw,
+	[ICC_NSSNOC_PCNOC_1] = &gcc_nssnoc_pcnoc_1_clk.clkr.hw,
+	[ICC_NSSNOC_QOSGEN_REF] = &gcc_nssnoc_qosgen_ref_clk.clkr.hw,
+	[ICC_NSSNOC_TIMEOUT_REF] = &gcc_nssnoc_timeout_ref_clk.clkr.hw,
+	[ICC_NSSNOC_XO_DCD] = &gcc_nssnoc_xo_dcd_clk.clkr.hw,
+	[ICC_NSSNOC_ATB] = &gcc_nssnoc_atb_clk.clkr.hw,
+	[ICC_MEM_NOC_NSSNOC] = &gcc_mem_noc_nssnoc_clk.clkr.hw,
+	[ICC_NSSNOC_MEMNOC] = &gcc_nssnoc_memnoc_clk.clkr.hw,
+	[ICC_NSSNOC_MEM_NOC_1] = &gcc_nssnoc_mem_noc_1_clk.clkr.hw,
+};
+
 static const struct of_device_id gcc_ipq9574_match_table[] = {
 	{ .compatible = "qcom,ipq9574-gcc" },
 	{ }
@@ -4323,6 +4374,9 @@  static const struct qcom_cc_desc gcc_ipq9574_desc = {
 	.num_resets = ARRAY_SIZE(gcc_ipq9574_resets),
 	.clk_hws = gcc_ipq9574_hws,
 	.num_clk_hws = ARRAY_SIZE(gcc_ipq9574_hws),
+	.icc_hws = icc_ipq9574_hws,
+	.num_icc_hws = ARRAY_SIZE(icc_ipq9574_hws),
+	.first_id = IPQ_APPS_ID,
 };
 
 static int gcc_ipq9574_probe(struct platform_device *pdev)