[PATCH v7 0/5] Add interconnect driver for IPQ9574 SoC

[PATCH v7 0/5] Add interconnect driver for IPQ9574 SoC

[Varadarajan Narayanan]

MSM platforms manage NoC related clocks and scaling from RPM.
However, in IPQ SoCs, RPM is not involved in managing NoC
related clocks and there is no NoC scaling.

However, there is a requirement to enable some NoC interface
clocks for the accessing the peripherals present in the
system. Hence add a minimalistic interconnect driver that
establishes a path from the processor/memory to those peripherals
and vice versa.

---
v7:	Fix macro names in dt-bindings header
	Do clock get in icc driver

v6:	Removed 'Reviewed-by: Krzysztof' from dt-bindings patch
	Remove clock get from ICC driver as suggested by Stephen Boyd
	so that the actual peripheral can do the clock get
	first_id -> icc_first_node_id
	Remove tristate from INTERCONNECT_CLK
v5:
	Split gcc-ipq9574.c and common.c changes into separate patches
	Introduce devm_icc_clk_register
	Fix error handling
v4:
gcc-ipq9574.c
	Use clk_hw instead of indices
common.c
	Do icc register in qcom_cc_probe() call stream
common.h
	Add icc clock info to qcom_cc_desc structure

v3:
qcom,ipq9574.h
	Move 'first id' define to clock driver
gcc-ipq9574.c:
	Use indexed identifiers here to avoid confusion
	Fix error messages and move code to common.c as it can be
	shared with future SoCs

v2:
qcom,ipq9574.h
	Fix license identifier
	Rename macros
qcom,ipq9574-gcc.yaml
	Include interconnect-cells
gcc-ipq9574.c
	Update commit log
	Remove IS_ENABLED(CONFIG_INTERCONNECT) and auto select it from Kconfig
ipq9574.dtsi
	Moved to separate patch
	Include interconnect-cells to clock controller node
drivers/clk/qcom/Kconfig:
	Auto select CONFIG_INTERCONNECT & CONFIG_INTERCONNECT_CLK

Varadarajan Narayanan (5):
  dt-bindings: interconnect: Add Qualcomm IPQ9574 support
  interconnect: icc-clk: Add devm_icc_clk_register
  clk: qcom: common: Add interconnect clocks support
  clk: qcom: ipq9574: Use icc-clk for enabling NoC related clocks
  arm64: dts: qcom: ipq9574: Add icc provider ability to gcc

 .../bindings/clock/qcom,ipq9574-gcc.yaml      |  3 +
 arch/arm64/boot/dts/qcom/ipq9574.dtsi         |  2 +
 drivers/clk/qcom/Kconfig                      |  2 +
 drivers/clk/qcom/common.c                     | 31 ++++++-
 drivers/clk/qcom/common.h                     |  3 +
 drivers/clk/qcom/gcc-ipq9574.c                | 30 +++++++
 drivers/interconnect/icc-clk.c                | 18 ++++
 .../dt-bindings/interconnect/qcom,ipq9574.h   | 87 +++++++++++++++++++
 include/linux/interconnect-clk.h              |  2 +
 9 files changed, 177 insertions(+), 1 deletion(-)
 create mode 100644 include/dt-bindings/interconnect/qcom,ipq9574.h

-- 
2.34.1

[PATCH v7 1/5] dt-bindings: interconnect: Add Qualcomm IPQ9574 support

[Varadarajan Narayanan]

Add interconnect-cells to clock provider so that it can be
used as icc provider.

Add master/slave ids for Qualcomm IPQ9574 Network-On-Chip
interfaces. This will be used by the gcc-ipq9574 driver
that will for providing interconnect services using the
icc-clk framework.

Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
v7:
Fix macro names to be consistent with other bindings
v6:
Removed Reviewed-by: Krzysztof Kozlowski
Redefine the bindings such that driver and DT can share them

v3:
Squash Documentation/ and include/ changes into same patch

qcom,ipq9574.h
	Move 'first id' to clock driver

---
 .../bindings/clock/qcom,ipq9574-gcc.yaml      |  3 +
 .../dt-bindings/interconnect/qcom,ipq9574.h   | 87 +++++++++++++++++++
 2 files changed, 90 insertions(+)
 create mode 100644 include/dt-bindings/interconnect/qcom,ipq9574.h

diff --git a/Documentation/devicetree/bindings/clock/qcom,ipq9574-gcc.yaml b/Documentation/devicetree/bindings/clock/qcom,ipq9574-gcc.yaml
index 944a0ea79cd6..824781cbdf34 100644
--- a/Documentation/devicetree/bindings/clock/qcom,ipq9574-gcc.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,ipq9574-gcc.yaml
@@ -33,6 +33,9 @@ properties:
       - description: PCIE30 PHY3 pipe clock source
       - description: USB3 PHY pipe clock source
 
+  '#interconnect-cells':
+    const: 1
+
 required:
   - compatible
   - clocks
diff --git a/include/dt-bindings/interconnect/qcom,ipq9574.h b/include/dt-bindings/interconnect/qcom,ipq9574.h
new file mode 100644
index 000000000000..0b076b0cf880
--- /dev/null
+++ b/include/dt-bindings/interconnect/qcom,ipq9574.h
@@ -0,0 +1,87 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+#ifndef INTERCONNECT_QCOM_IPQ9574_H
+#define INTERCONNECT_QCOM_IPQ9574_H
+
+#define ICC_ANOC_PCIE0		0
+#define ICC_SNOC_PCIE0		1
+#define ICC_ANOC_PCIE1		2
+#define ICC_SNOC_PCIE1		3
+#define ICC_ANOC_PCIE2		4
+#define ICC_SNOC_PCIE2		5
+#define ICC_ANOC_PCIE3		6
+#define ICC_SNOC_PCIE3		7
+#define ICC_SNOC_USB		8
+#define ICC_ANOC_USB_AXI	9
+#define ICC_NSSNOC_NSSCC	10
+#define ICC_NSSNOC_SNOC_0	11
+#define ICC_NSSNOC_SNOC_1	12
+#define ICC_NSSNOC_PCNOC_1	13
+#define ICC_NSSNOC_QOSGEN_REF	14
+#define ICC_NSSNOC_TIMEOUT_REF	15
+#define ICC_NSSNOC_XO_DCD	16
+#define ICC_NSSNOC_ATB		17
+#define ICC_MEM_NOC_NSSNOC	18
+#define ICC_NSSNOC_MEMNOC	19
+#define ICC_NSSNOC_MEM_NOC_1	20
+
+#define ICC_NSSNOC_PPE		0
+#define ICC_NSSNOC_PPE_CFG	1
+#define ICC_NSSNOC_NSS_CSR	2
+#define ICC_NSSNOC_IMEM_QSB	3
+#define ICC_NSSNOC_IMEM_AHB	4
+
+#define MASTER_ANOC_PCIE0		(ICC_ANOC_PCIE0 * 2)
+#define SLAVE_ANOC_PCIE0		((ICC_ANOC_PCIE0 * 2) + 1)
+#define MASTER_SNOC_PCIE0		(ICC_SNOC_PCIE0 * 2)
+#define SLAVE_SNOC_PCIE0		((ICC_SNOC_PCIE0 * 2) + 1)
+#define MASTER_ANOC_PCIE1		(ICC_ANOC_PCIE1 * 2)
+#define SLAVE_ANOC_PCIE1		((ICC_ANOC_PCIE1 * 2) + 1)
+#define MASTER_SNOC_PCIE1		(ICC_SNOC_PCIE1 * 2)
+#define SLAVE_SNOC_PCIE1		((ICC_SNOC_PCIE1 * 2) + 1)
+#define MASTER_ANOC_PCIE2		(ICC_ANOC_PCIE2 * 2)
+#define SLAVE_ANOC_PCIE2		((ICC_ANOC_PCIE2 * 2) + 1)
+#define MASTER_SNOC_PCIE2		(ICC_SNOC_PCIE2 * 2)
+#define SLAVE_SNOC_PCIE2		((ICC_SNOC_PCIE2 * 2) + 1)
+#define MASTER_ANOC_PCIE3		(ICC_ANOC_PCIE3 * 2)
+#define SLAVE_ANOC_PCIE3		((ICC_ANOC_PCIE3 * 2) + 1)
+#define MASTER_SNOC_PCIE3		(ICC_SNOC_PCIE3 * 2)
+#define SLAVE_SNOC_PCIE3		((ICC_SNOC_PCIE3 * 2) + 1)
+#define MASTER_USB			(ICC_USB * 2)
+#define SLAVE_USB			((ICC_USB * 2) + 1)
+#define MASTER_USB_AXI			(ICC_USB_AXI * 2)
+#define SLAVE_USB_AXI			((ICC_USB_AXI * 2) + 1)
+#define MASTER_NSSNOC_NSSCC		(ICC_NSSNOC_NSSCC * 2)
+#define SLAVE_NSSNOC_NSSCC		((ICC_NSSNOC_NSSCC * 2) + 1)
+#define MASTER_NSSNOC_SNOC_0		(ICC_NSSNOC_SNOC_0 * 2)
+#define SLAVE_NSSNOC_SNOC_0		((ICC_NSSNOC_SNOC_0 * 2) + 1)
+#define MASTER_NSSNOC_SNOC_1		(ICC_NSSNOC_SNOC_1 * 2)
+#define SLAVE_NSSNOC_SNOC_1		((ICC_NSSNOC_SNOC_1 * 2) + 1)
+#define MASTER_NSSNOC_PCNOC_1		(ICC_NSSNOC_PCNOC_1 * 2)
+#define SLAVE_NSSNOC_PCNOC_1		((ICC_NSSNOC_PCNOC_1 * 2) + 1)
+#define MASTER_NSSNOC_QOSGEN_REF	(ICC_NSSNOC_QOSGEN_REF * 2)
+#define SLAVE_NSSNOC_QOSGEN_REF		((ICC_NSSNOC_QOSGEN_REF * 2) + 1)
+#define MASTER_NSSNOC_TIMEOUT_REF	(ICC_NSSNOC_TIMEOUT_REF * 2)
+#define SLAVE_NSSNOC_TIMEOUT_REF	((ICC_NSSNOC_TIMEOUT_REF * 2) + 1)
+#define MASTER_NSSNOC_XO_DCD		(ICC_NSSNOC_XO_DCD * 2)
+#define SLAVE_NSSNOC_XO_DCD		((ICC_NSSNOC_XO_DCD * 2) + 1)
+#define MASTER_NSSNOC_ATB		(ICC_NSSNOC_ATB * 2)
+#define SLAVE_NSSNOC_ATB		((ICC_NSSNOC_ATB * 2) + 1)
+#define MASTER_MEM_NOC_NSSNOC		(ICC_MEM_NOC_NSSNOC * 2)
+#define SLAVE_MEM_NOC_NSSNOC		((ICC_MEM_NOC_NSSNOC * 2) + 1)
+#define MASTER_NSSNOC_MEMNOC		(ICC_NSSNOC_MEMNOC * 2)
+#define SLAVE_NSSNOC_MEMNOC		((ICC_NSSNOC_MEMNOC * 2) + 1)
+#define MASTER_NSSNOC_MEM_NOC_1		(ICC_NSSNOC_MEM_NOC_1 * 2)
+#define SLAVE_NSSNOC_MEM_NOC_1		((ICC_NSSNOC_MEM_NOC_1 * 2) + 1)
+
+#define MASTER_NSSNOC_PPE		(ICC_NSSNOC_PPE * 2)
+#define SLAVE_NSSNOC_PPE		((ICC_NSSNOC_PPE * 2) + 1)
+#define MASTER_NSSNOC_PPE_CFG		(ICC_NSSNOC_PPE_CFG * 2)
+#define SLAVE_NSSNOC_PPE_CFG		((ICC_NSSNOC_PPE_CFG * 2) + 1)
+#define MASTER_NSSNOC_NSS_CSR		(ICC_NSSNOC_NSS_CSR * 2)
+#define SLAVE_NSSNOC_NSS_CSR		((ICC_NSSNOC_NSS_CSR * 2) + 1)
+#define MASTER_NSSNOC_IMEM_QSB		(ICC_NSSNOC_IMEM_QSB * 2)
+#define SLAVE_NSSNOC_IMEM_QSB		((ICC_NSSNOC_IMEM_QSB * 2) + 1)
+#define MASTER_NSSNOC_IMEM_AHB		(ICC_NSSNOC_IMEM_AHB * 2)
+#define SLAVE_NSSNOC_IMEM_AHB		((ICC_NSSNOC_IMEM_AHB * 2) + 1)
+
+#endif /* INTERCONNECT_QCOM_IPQ9574_H */
-- 
2.34.1

[PATCH v7 2/5] interconnect: icc-clk: Add devm_icc_clk_register

[Varadarajan Narayanan]

Wrap icc_clk_register to create devm_icc_clk_register to be
able to release the resources properly.

Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
v7: Simplify devm_icc_clk_register implementation as suggested in review
v5: Introduced devm_icc_clk_register
---
 drivers/interconnect/icc-clk.c   | 18 ++++++++++++++++++
 include/linux/interconnect-clk.h |  2 ++
 2 files changed, 20 insertions(+)

diff --git a/drivers/interconnect/icc-clk.c b/drivers/interconnect/icc-clk.c
index d787f2ea36d9..bce946592c98 100644
--- a/drivers/interconnect/icc-clk.c
+++ b/drivers/interconnect/icc-clk.c
@@ -148,6 +148,24 @@ struct icc_provider *icc_clk_register(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(icc_clk_register);
 
+static void devm_icc_release(void *res)
+{
+	icc_clk_unregister(res);
+}
+
+int devm_icc_clk_register(struct device *dev, unsigned int first_id,
+			  unsigned int num_clocks, const struct icc_clk_data *data)
+{
+	struct icc_provider *prov;
+
+	prov = icc_clk_register(dev, first_id, num_clocks, data);
+	if (IS_ERR(prov))
+		return PTR_ERR(prov);
+
+	return devm_add_action_or_reset(dev, devm_icc_release, prov);
+}
+EXPORT_SYMBOL_GPL(devm_icc_clk_register);
+
 /**
  * icc_clk_unregister() - unregister a previously registered clk interconnect provider
  * @provider: provider returned by icc_clk_register()
diff --git a/include/linux/interconnect-clk.h b/include/linux/interconnect-clk.h
index 0cd80112bea5..5c611a8b0892 100644
--- a/include/linux/interconnect-clk.h
+++ b/include/linux/interconnect-clk.h
@@ -17,6 +17,8 @@ struct icc_provider *icc_clk_register(struct device *dev,
 				      unsigned int first_id,
 				      unsigned int num_clocks,
 				      const struct icc_clk_data *data);
+int devm_icc_clk_register(struct device *dev, unsigned int first_id,
+			  unsigned int num_clocks, const struct icc_clk_data *data);
 void icc_clk_unregister(struct icc_provider *provider);
 
 #endif
-- 
2.34.1

[PATCH v7 3/5] clk: qcom: common: Add interconnect clocks support

[Varadarajan Narayanan]

Unlike MSM platforms that manage NoC related clocks and scaling
from RPM, IPQ SoCs dont involve RPM in managing NoC related
clocks and there is no NoC scaling.

However, there is a requirement to enable some NoC interface
clocks for accessing the peripheral controllers present on
these NoCs. Though exposing these as normal clocks would work,
having a minimalistic interconnect driver to handle these clocks
would make it consistent with other Qualcomm platforms resulting
in common code paths. This is similar to msm8996-cbf's usage of
icc-clk framework.

Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
v7: Restore clk_get
v6: first_id -> icc_first_node_id
    Remove clock get so that the peripheral that uses the clock
    can do the clock get
v5: Split changes in common.c to separate patch
    Fix error handling
    Use devm_icc_clk_register instead of icc_clk_register
v4: Use clk_hw instead of indices
    Do icc register in qcom_cc_probe() call stream
    Add icc clock info to qcom_cc_desc structure
v3: Use indexed identifiers here to avoid confusion
    Fix error messages and move to common.c
v2: Move DTS to separate patch
    Update commit log
    Auto select CONFIG_INTERCONNECT & CONFIG_INTERCONNECT_CLK to fix build error
---
 drivers/clk/qcom/common.c | 31 ++++++++++++++++++++++++++++++-
 drivers/clk/qcom/common.h |  3 +++
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
index 8b6080eb43a7..fa4ec89c04c4 100644
--- a/drivers/clk/qcom/common.c
+++ b/drivers/clk/qcom/common.c
@@ -8,6 +8,7 @@
 #include <linux/regmap.h>
 #include <linux/platform_device.h>
 #include <linux/clk-provider.h>
+#include <linux/interconnect-clk.h>
 #include <linux/reset-controller.h>
 #include <linux/of.h>
 
@@ -252,6 +253,34 @@ static struct clk_hw *qcom_cc_clk_hw_get(struct of_phandle_args *clkspec,
 	return cc->rclks[idx] ? &cc->rclks[idx]->hw : NULL;
 }
 
+static int qcom_cc_icc_register(struct device *dev,
+				const struct qcom_cc_desc *desc)
+{
+	struct icc_clk_data *icd;
+	int i;
+
+	if (!IS_ENABLED(CONFIG_INTERCONNECT_CLK))
+		return 0;
+
+	if (!desc->icc_hws)
+		return 0;
+
+	icd = devm_kcalloc(dev, desc->num_icc_hws, sizeof(*icd), GFP_KERNEL);
+	if (!icd)
+		return -ENOMEM;
+
+	for (i = 0; i < desc->num_icc_hws; i++) {
+		icd[i].clk = devm_clk_hw_get_clk(dev, desc->icc_hws[i], "icc");
+		if (!icd[i].clk)
+			return dev_err_probe(dev, -ENOENT,
+					     "(%d) clock entry is null\n", i);
+		icd[i].name = clk_hw_get_name(desc->icc_hws[i]);
+	}
+
+	return devm_icc_clk_register(dev, desc->icc_first_node_id,
+						     desc->num_icc_hws, icd);
+}
+
 int qcom_cc_really_probe(struct platform_device *pdev,
 			 const struct qcom_cc_desc *desc, struct regmap *regmap)
 {
@@ -327,7 +356,7 @@ int _qcom_cc_really_probe(struct device *dev,
 	if (ret)
 		return ret;
 
-	return 0;
+	return qcom_cc_icc_register(dev, desc);
 }
 EXPORT_SYMBOL_GPL(_qcom_cc_really_probe);
 
diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
index 8657257d56d3..43073d2ef32a 100644
--- a/drivers/clk/qcom/common.h
+++ b/drivers/clk/qcom/common.h
@@ -29,6 +29,9 @@ struct qcom_cc_desc {
 	size_t num_gdscs;
 	struct clk_hw **clk_hws;
 	size_t num_clk_hws;
+	struct clk_hw **icc_hws;
+	size_t num_icc_hws;
+	unsigned int icc_first_node_id;
 };
 
 /**
-- 
2.34.1

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

[Varadarajan Narayanan]

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>
---
v7: Auto select INTERCONNECT & INTERCONNECT_CLK in COMMON_CLK_QCOM
    to address build break with random config build test, with the
    following combination

	CONFIG_COMMON_CLK_QCOM=y
		and
	CONFIG_INTERCONNECT_CLK=m

    the following error is seen as devm_icc_clk_register is in a
    module and being referenced from vmlinux.

	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'

v6: Move enum to dt-bindings and share between here and DT
    first_id -> icc_first_node_id
v5: Split from common.c changes into separate patch
    No functional changes
---
 drivers/clk/qcom/Kconfig       |  2 ++
 drivers/clk/qcom/gcc-ipq9574.c | 30 ++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index b0e0eeb1604e..e51328bbd146 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -17,6 +17,8 @@ menuconfig COMMON_CLK_QCOM
 	select RATIONAL
 	select REGMAP_MMIO
 	select RESET_CONTROLLER
+	select INTERCONNECT
+	select INTERCONNECT_CLK
 
 if COMMON_CLK_QCOM
 
diff --git a/drivers/clk/qcom/gcc-ipq9574.c b/drivers/clk/qcom/gcc-ipq9574.c
index 43da03e4c2dd..0ede625777c9 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"
@@ -4067,6 +4068,32 @@ static const struct qcom_reset_map gcc_ipq9574_resets[] = {
 	[GCC_WCSS_Q6_TBU_BCR] = { 0x12054, 0 },
 };
 
+#define IPQ_APPS_ID			9574	/* some unique value */
+
+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" },
 	{ }
@@ -4089,6 +4116,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),
+	.icc_first_node_id = IPQ_APPS_ID,
 };
 
 static int gcc_ipq9574_probe(struct platform_device *pdev)
-- 
2.34.1

[PATCH v7 5/5] arm64: dts: qcom: ipq9574: Add icc provider ability to gcc

[Varadarajan Narayanan]

IPQ SoCs dont involve RPM in managing NoC related clocks and
there is no NoC scaling. Linux itself handles these clocks.
However, these should not be exposed as just clocks and align
with other Qualcomm SoCs that handle these clocks from a
interconnect provider.

Hence include icc provider capability to the gcc node so that
peripherals can use the interconnect facility to enable these
clocks.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
 arch/arm64/boot/dts/qcom/ipq9574.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
index c5abadf94975..0aba4c60e850 100644
--- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
@@ -8,6 +8,7 @@
 
 #include <dt-bindings/clock/qcom,apss-ipq.h>
 #include <dt-bindings/clock/qcom,ipq9574-gcc.h>
+#include <dt-bindings/interconnect/qcom,ipq9574.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>
@@ -457,6 +458,7 @@ gcc: clock-controller@1800000 {
 			#clock-cells = <1>;
 			#reset-cells = <1>;
 			#power-domain-cells = <1>;
+			#interconnect-cells = <1>;
 		};
 
 		tcsr_mutex: hwlock@1905000 {
-- 
2.34.1

Re: [PATCH v7 2/5] interconnect: icc-clk: Add devm_icc_clk_register

[Dmitry Baryshkov]

On Wed, 3 Apr 2024 at 13:42, Varadarajan Narayanan
<quic_varada@quicinc.com> wrote:
>
> Wrap icc_clk_register to create devm_icc_clk_register to be
> able to release the resources properly.
>
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> ---
> v7: Simplify devm_icc_clk_register implementation as suggested in review
> v5: Introduced devm_icc_clk_register
> ---
>  drivers/interconnect/icc-clk.c   | 18 ++++++++++++++++++
>  include/linux/interconnect-clk.h |  2 ++
>  2 files changed, 20 insertions(+)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

Re: [PATCH v7 3/5] clk: qcom: common: Add interconnect clocks support

[Dmitry Baryshkov]

On Wed, 3 Apr 2024 at 13:42, Varadarajan Narayanan
<quic_varada@quicinc.com> wrote:
>
> Unlike MSM platforms that manage NoC related clocks and scaling
> from RPM, IPQ SoCs dont involve RPM in managing NoC related
> clocks and there is no NoC scaling.
>
> However, there is a requirement to enable some NoC interface
> clocks for accessing the peripheral controllers present on
> these NoCs. Though exposing these as normal clocks would work,
> having a minimalistic interconnect driver to handle these clocks
> would make it consistent with other Qualcomm platforms resulting
> in common code paths. This is similar to msm8996-cbf's usage of
> icc-clk framework.
>
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> ---
> v7: Restore clk_get
> v6: first_id -> icc_first_node_id
>     Remove clock get so that the peripheral that uses the clock
>     can do the clock get
> v5: Split changes in common.c to separate patch
>     Fix error handling
>     Use devm_icc_clk_register instead of icc_clk_register
> v4: Use clk_hw instead of indices
>     Do icc register in qcom_cc_probe() call stream
>     Add icc clock info to qcom_cc_desc structure
> v3: Use indexed identifiers here to avoid confusion
>     Fix error messages and move to common.c
> v2: Move DTS to separate patch
>     Update commit log
>     Auto select CONFIG_INTERCONNECT & CONFIG_INTERCONNECT_CLK to fix build error
> ---
>  drivers/clk/qcom/common.c | 31 ++++++++++++++++++++++++++++++-
>  drivers/clk/qcom/common.h |  3 +++
>  2 files changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> index 8b6080eb43a7..fa4ec89c04c4 100644
> --- a/drivers/clk/qcom/common.c
> +++ b/drivers/clk/qcom/common.c
> @@ -8,6 +8,7 @@
>  #include <linux/regmap.h>
>  #include <linux/platform_device.h>
>  #include <linux/clk-provider.h>
> +#include <linux/interconnect-clk.h>
>  #include <linux/reset-controller.h>
>  #include <linux/of.h>
>
> @@ -252,6 +253,34 @@ static struct clk_hw *qcom_cc_clk_hw_get(struct of_phandle_args *clkspec,
>         return cc->rclks[idx] ? &cc->rclks[idx]->hw : NULL;
>  }
>
> +static int qcom_cc_icc_register(struct device *dev,
> +                               const struct qcom_cc_desc *desc)
> +{
> +       struct icc_clk_data *icd;
> +       int i;
> +
> +       if (!IS_ENABLED(CONFIG_INTERCONNECT_CLK))
> +               return 0;
> +
> +       if (!desc->icc_hws)
> +               return 0;
> +
> +       icd = devm_kcalloc(dev, desc->num_icc_hws, sizeof(*icd), GFP_KERNEL);
> +       if (!icd)
> +               return -ENOMEM;
> +
> +       for (i = 0; i < desc->num_icc_hws; i++) {
> +               icd[i].clk = devm_clk_hw_get_clk(dev, desc->icc_hws[i], "icc");
> +               if (!icd[i].clk)
> +                       return dev_err_probe(dev, -ENOENT,
> +                                            "(%d) clock entry is null\n", i);
> +               icd[i].name = clk_hw_get_name(desc->icc_hws[i]);
> +       }
> +
> +       return devm_icc_clk_register(dev, desc->icc_first_node_id,
> +                                                    desc->num_icc_hws, icd);
> +}
> +
>  int qcom_cc_really_probe(struct platform_device *pdev,
>                          const struct qcom_cc_desc *desc, struct regmap *regmap)
>  {
> @@ -327,7 +356,7 @@ int _qcom_cc_really_probe(struct device *dev,
>         if (ret)
>                 return ret;
>
> -       return 0;
> +       return qcom_cc_icc_register(dev, desc);
>  }
>  EXPORT_SYMBOL_GPL(_qcom_cc_really_probe);
>
> diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
> index 8657257d56d3..43073d2ef32a 100644
> --- a/drivers/clk/qcom/common.h
> +++ b/drivers/clk/qcom/common.h
> @@ -29,6 +29,9 @@ struct qcom_cc_desc {
>         size_t num_gdscs;
>         struct clk_hw **clk_hws;
>         size_t num_clk_hws;
> +       struct clk_hw **icc_hws;

Still we are passing hws here. We already have all the hws in a
different array. Can we just pass the indices?

> +       size_t num_icc_hws;
> +       unsigned int icc_first_node_id;
>  };
>
>  /**
> --
> 2.34.1
>


-- 
With best wishes
Dmitry

Re: [PATCH v7 1/5] dt-bindings: interconnect: Add Qualcomm IPQ9574 support

[Krzysztof Kozlowski]

On 03/04/2024 12:42, Varadarajan Narayanan wrote:
> Add interconnect-cells to clock provider so that it can be
> used as icc provider.
> 
> Add master/slave ids for Qualcomm IPQ9574 Network-On-Chip
> interfaces. This will be used by the gcc-ipq9574 driver
> that will for providing interconnect services using the
> icc-clk framework.
> 
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> ---
> v7:
> Fix macro names to be consistent with other bindings
> v6:
> Removed Reviewed-by: Krzysztof Kozlowski
> Redefine the bindings such that driver and DT can share them
> 
> v3:
> Squash Documentation/ and include/ changes into same patch
> 
> qcom,ipq9574.h
> 	Move 'first id' to clock driver
> 
> ---
>  .../bindings/clock/qcom,ipq9574-gcc.yaml      |  3 +
>  .../dt-bindings/interconnect/qcom,ipq9574.h   | 87 +++++++++++++++++++
>  2 files changed, 90 insertions(+)
>  create mode 100644 include/dt-bindings/interconnect/qcom,ipq9574.h
> 
> diff --git a/Documentation/devicetree/bindings/clock/qcom,ipq9574-gcc.yaml b/Documentation/devicetree/bindings/clock/qcom,ipq9574-gcc.yaml
> index 944a0ea79cd6..824781cbdf34 100644
> --- a/Documentation/devicetree/bindings/clock/qcom,ipq9574-gcc.yaml
> +++ b/Documentation/devicetree/bindings/clock/qcom,ipq9574-gcc.yaml
> @@ -33,6 +33,9 @@ properties:
>        - description: PCIE30 PHY3 pipe clock source
>        - description: USB3 PHY pipe clock source
>  
> +  '#interconnect-cells':
> +    const: 1
> +
>  required:
>    - compatible
>    - clocks
> diff --git a/include/dt-bindings/interconnect/qcom,ipq9574.h b/include/dt-bindings/interconnect/qcom,ipq9574.h
> new file mode 100644
> index 000000000000..0b076b0cf880
> --- /dev/null
> +++ b/include/dt-bindings/interconnect/qcom,ipq9574.h
> @@ -0,0 +1,87 @@
> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> +#ifndef INTERCONNECT_QCOM_IPQ9574_H
> +#define INTERCONNECT_QCOM_IPQ9574_H
> +
> +#define ICC_ANOC_PCIE0		0
> +#define ICC_SNOC_PCIE0		1
> +#define ICC_ANOC_PCIE1		2
> +#define ICC_SNOC_PCIE1		3
> +#define ICC_ANOC_PCIE2		4
> +#define ICC_SNOC_PCIE2		5
> +#define ICC_ANOC_PCIE3		6
> +#define ICC_SNOC_PCIE3		7
> +#define ICC_SNOC_USB		8
> +#define ICC_ANOC_USB_AXI	9
> +#define ICC_NSSNOC_NSSCC	10
> +#define ICC_NSSNOC_SNOC_0	11
> +#define ICC_NSSNOC_SNOC_1	12
> +#define ICC_NSSNOC_PCNOC_1	13
> +#define ICC_NSSNOC_QOSGEN_REF	14
> +#define ICC_NSSNOC_TIMEOUT_REF	15
> +#define ICC_NSSNOC_XO_DCD	16
> +#define ICC_NSSNOC_ATB		17
> +#define ICC_MEM_NOC_NSSNOC	18
> +#define ICC_NSSNOC_MEMNOC	19
> +#define ICC_NSSNOC_MEM_NOC_1	20
> +
> +#define ICC_NSSNOC_PPE		0
> +#define ICC_NSSNOC_PPE_CFG	1
> +#define ICC_NSSNOC_NSS_CSR	2
> +#define ICC_NSSNOC_IMEM_QSB	3
> +#define ICC_NSSNOC_IMEM_AHB	4
> +
> +#define MASTER_ANOC_PCIE0		(ICC_ANOC_PCIE0 * 2)
> +#define SLAVE_ANOC_PCIE0		((ICC_ANOC_PCIE0 * 2) + 1)

Which existing Qualcomm platform has such code?

This is the third time I am asking for consistent headers. Open
existing, recently added headers and look how it is done there. Why?
Because I am against such calculations and see no reason for them.

Best regards,
Krzysztof

Re: [PATCH v7 1/5] dt-bindings: interconnect: Add Qualcomm IPQ9574 support

[Varadarajan Narayanan]

On Wed, Apr 03, 2024 at 04:59:40PM +0200, Krzysztof Kozlowski wrote:
> On 03/04/2024 12:42, Varadarajan Narayanan wrote:
> > Add interconnect-cells to clock provider so that it can be
> > used as icc provider.
> >
> > Add master/slave ids for Qualcomm IPQ9574 Network-On-Chip
> > interfaces. This will be used by the gcc-ipq9574 driver
> > that will for providing interconnect services using the
> > icc-clk framework.
> >
> > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> > ---
> > v7:
> > Fix macro names to be consistent with other bindings
> > v6:
> > Removed Reviewed-by: Krzysztof Kozlowski
> > Redefine the bindings such that driver and DT can share them
> >
> > v3:
> > Squash Documentation/ and include/ changes into same patch
> >
> > qcom,ipq9574.h
> > 	Move 'first id' to clock driver
> >
> > ---
> >  .../bindings/clock/qcom,ipq9574-gcc.yaml      |  3 +
> >  .../dt-bindings/interconnect/qcom,ipq9574.h   | 87 +++++++++++++++++++
> >  2 files changed, 90 insertions(+)
> >  create mode 100644 include/dt-bindings/interconnect/qcom,ipq9574.h
> >
> > diff --git a/Documentation/devicetree/bindings/clock/qcom,ipq9574-gcc.yaml b/Documentation/devicetree/bindings/clock/qcom,ipq9574-gcc.yaml
> > index 944a0ea79cd6..824781cbdf34 100644
> > --- a/Documentation/devicetree/bindings/clock/qcom,ipq9574-gcc.yaml
> > +++ b/Documentation/devicetree/bindings/clock/qcom,ipq9574-gcc.yaml
> > @@ -33,6 +33,9 @@ properties:
> >        - description: PCIE30 PHY3 pipe clock source
> >        - description: USB3 PHY pipe clock source
> >
> > +  '#interconnect-cells':
> > +    const: 1
> > +
> >  required:
> >    - compatible
> >    - clocks
> > diff --git a/include/dt-bindings/interconnect/qcom,ipq9574.h b/include/dt-bindings/interconnect/qcom,ipq9574.h
> > new file mode 100644
> > index 000000000000..0b076b0cf880
> > --- /dev/null
> > +++ b/include/dt-bindings/interconnect/qcom,ipq9574.h
> > @@ -0,0 +1,87 @@
> > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> > +#ifndef INTERCONNECT_QCOM_IPQ9574_H
> > +#define INTERCONNECT_QCOM_IPQ9574_H
> > +
> > +#define ICC_ANOC_PCIE0		0
> > +#define ICC_SNOC_PCIE0		1
> > +#define ICC_ANOC_PCIE1		2
> > +#define ICC_SNOC_PCIE1		3
> > +#define ICC_ANOC_PCIE2		4
> > +#define ICC_SNOC_PCIE2		5
> > +#define ICC_ANOC_PCIE3		6
> > +#define ICC_SNOC_PCIE3		7
> > +#define ICC_SNOC_USB		8
> > +#define ICC_ANOC_USB_AXI	9
> > +#define ICC_NSSNOC_NSSCC	10
> > +#define ICC_NSSNOC_SNOC_0	11
> > +#define ICC_NSSNOC_SNOC_1	12
> > +#define ICC_NSSNOC_PCNOC_1	13
> > +#define ICC_NSSNOC_QOSGEN_REF	14
> > +#define ICC_NSSNOC_TIMEOUT_REF	15
> > +#define ICC_NSSNOC_XO_DCD	16
> > +#define ICC_NSSNOC_ATB		17
> > +#define ICC_MEM_NOC_NSSNOC	18
> > +#define ICC_NSSNOC_MEMNOC	19
> > +#define ICC_NSSNOC_MEM_NOC_1	20
> > +
> > +#define ICC_NSSNOC_PPE		0
> > +#define ICC_NSSNOC_PPE_CFG	1
> > +#define ICC_NSSNOC_NSS_CSR	2
> > +#define ICC_NSSNOC_IMEM_QSB	3
> > +#define ICC_NSSNOC_IMEM_AHB	4
> > +
> > +#define MASTER_ANOC_PCIE0		(ICC_ANOC_PCIE0 * 2)
> > +#define SLAVE_ANOC_PCIE0		((ICC_ANOC_PCIE0 * 2) + 1)
>
> Which existing Qualcomm platform has such code?

Existing Qualcomm platforms don't use icc-clk. They use icc-rpm
or icc-rpmh. clk-cbf-msm8996.c is the only driver that uses icc-clk.

The icc_clk_register automatically creates master & slave nodes
for each clk entry provided as input with the node-ids 'n' and
'n+1'. Since clk-cbf-msm8996.c has only one entry, it could just
define MASTER_CBF_M4M and SLAVE_CBF_M4M with 0 and 1 and avoid these
calculations.

However, ipq9574 gives an array of clock entries as input to
icc_clk_register. To tie the order/sequence of these clock
entries correctly with the node-ids, this calculation is needed.

> This is the third time I am asking for consistent headers. Open
> existing, recently added headers and look how it is done there. Why?
> Because I am against such calculations and see no reason for them.

Apologies. Regret that I have to trouble you.

In this ipq9574 case, have to reconcile between the following
feedbacks.

1. https://lore.kernel.org/linux-arm-msm/fe40b307-26d0-4b2a-869b-5d093415b9d1@linaro.org/
   We could probably use indexed identifiers here to avoid confusion:
   [ICC_BINDING_NAME] = CLK_BINDING_NAME

2. https://lore.kernel.org/linux-arm-msm/95f4e99a60cc97770fc3cee850b62faf.sboyd@kernel.org/
   Are these supposed to be in a dt-binding header?

3. https://lore.kernel.org/linux-arm-msm/031d0a35-b192-4161-beef-97b89d5d1da6@linaro.org/
   Do you use them as well in the DTS?

Having the defines (with the calculations) seemed to to comply
with the above three feedbacks.

Please let me know if this can be handled in a different way that
would be consistent with other Qualcomm platforms.

Thanks
Varada

Re: [PATCH v7 1/5] dt-bindings: interconnect: Add Qualcomm IPQ9574 support

[Varadarajan Narayanan]

On Thu, Apr 04, 2024 at 02:25:06PM +0530, Varadarajan Narayanan wrote:
> On Wed, Apr 03, 2024 at 04:59:40PM +0200, Krzysztof Kozlowski wrote:
> > On 03/04/2024 12:42, Varadarajan Narayanan wrote:
> > > Add interconnect-cells to clock provider so that it can be
> > > used as icc provider.
> > >
> > > Add master/slave ids for Qualcomm IPQ9574 Network-On-Chip
> > > interfaces. This will be used by the gcc-ipq9574 driver
> > > that will for providing interconnect services using the
> > > icc-clk framework.
> > >
> > > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> > > ---
> > > v7:
> > > Fix macro names to be consistent with other bindings
> > > v6:
> > > Removed Reviewed-by: Krzysztof Kozlowski
> > > Redefine the bindings such that driver and DT can share them
> > >
> > > v3:
> > > Squash Documentation/ and include/ changes into same patch
> > >
> > > qcom,ipq9574.h
> > > 	Move 'first id' to clock driver
> > >
> > > ---
> > >  .../bindings/clock/qcom,ipq9574-gcc.yaml      |  3 +
> > >  .../dt-bindings/interconnect/qcom,ipq9574.h   | 87 +++++++++++++++++++
> > >  2 files changed, 90 insertions(+)
> > >  create mode 100644 include/dt-bindings/interconnect/qcom,ipq9574.h
> > >
> > > diff --git a/Documentation/devicetree/bindings/clock/qcom,ipq9574-gcc.yaml b/Documentation/devicetree/bindings/clock/qcom,ipq9574-gcc.yaml
> > > index 944a0ea79cd6..824781cbdf34 100644
> > > --- a/Documentation/devicetree/bindings/clock/qcom,ipq9574-gcc.yaml
> > > +++ b/Documentation/devicetree/bindings/clock/qcom,ipq9574-gcc.yaml
> > > @@ -33,6 +33,9 @@ properties:
> > >        - description: PCIE30 PHY3 pipe clock source
> > >        - description: USB3 PHY pipe clock source
> > >
> > > +  '#interconnect-cells':
> > > +    const: 1
> > > +
> > >  required:
> > >    - compatible
> > >    - clocks
> > > diff --git a/include/dt-bindings/interconnect/qcom,ipq9574.h b/include/dt-bindings/interconnect/qcom,ipq9574.h
> > > new file mode 100644
> > > index 000000000000..0b076b0cf880
> > > --- /dev/null
> > > +++ b/include/dt-bindings/interconnect/qcom,ipq9574.h
> > > @@ -0,0 +1,87 @@
> > > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> > > +#ifndef INTERCONNECT_QCOM_IPQ9574_H
> > > +#define INTERCONNECT_QCOM_IPQ9574_H
> > > +
> > > +#define ICC_ANOC_PCIE0		0
> > > +#define ICC_SNOC_PCIE0		1
> > > +#define ICC_ANOC_PCIE1		2
> > > +#define ICC_SNOC_PCIE1		3
> > > +#define ICC_ANOC_PCIE2		4
> > > +#define ICC_SNOC_PCIE2		5
> > > +#define ICC_ANOC_PCIE3		6
> > > +#define ICC_SNOC_PCIE3		7
> > > +#define ICC_SNOC_USB		8
> > > +#define ICC_ANOC_USB_AXI	9
> > > +#define ICC_NSSNOC_NSSCC	10
> > > +#define ICC_NSSNOC_SNOC_0	11
> > > +#define ICC_NSSNOC_SNOC_1	12
> > > +#define ICC_NSSNOC_PCNOC_1	13
> > > +#define ICC_NSSNOC_QOSGEN_REF	14
> > > +#define ICC_NSSNOC_TIMEOUT_REF	15
> > > +#define ICC_NSSNOC_XO_DCD	16
> > > +#define ICC_NSSNOC_ATB		17
> > > +#define ICC_MEM_NOC_NSSNOC	18
> > > +#define ICC_NSSNOC_MEMNOC	19
> > > +#define ICC_NSSNOC_MEM_NOC_1	20
> > > +
> > > +#define ICC_NSSNOC_PPE		0
> > > +#define ICC_NSSNOC_PPE_CFG	1
> > > +#define ICC_NSSNOC_NSS_CSR	2
> > > +#define ICC_NSSNOC_IMEM_QSB	3
> > > +#define ICC_NSSNOC_IMEM_AHB	4
> > > +
> > > +#define MASTER_ANOC_PCIE0		(ICC_ANOC_PCIE0 * 2)
> > > +#define SLAVE_ANOC_PCIE0		((ICC_ANOC_PCIE0 * 2) + 1)
> >
> > Which existing Qualcomm platform has such code?
>
> Existing Qualcomm platforms don't use icc-clk. They use icc-rpm
> or icc-rpmh. clk-cbf-msm8996.c is the only driver that uses icc-clk.
>
> The icc_clk_register automatically creates master & slave nodes
> for each clk entry provided as input with the node-ids 'n' and
> 'n+1'. Since clk-cbf-msm8996.c has only one entry, it could just
> define MASTER_CBF_M4M and SLAVE_CBF_M4M with 0 and 1 and avoid these
> calculations.
>
> However, ipq9574 gives an array of clock entries as input to
> icc_clk_register. To tie the order/sequence of these clock
> entries correctly with the node-ids, this calculation is needed.
>
> > This is the third time I am asking for consistent headers. Open
> > existing, recently added headers and look how it is done there. Why?
> > Because I am against such calculations and see no reason for them.
>
> Apologies. Regret that I have to trouble you.
>
> In this ipq9574 case, have to reconcile between the following
> feedbacks.
>
> 1. https://lore.kernel.org/linux-arm-msm/fe40b307-26d0-4b2a-869b-5d093415b9d1@linaro.org/
>    We could probably use indexed identifiers here to avoid confusion:
>    [ICC_BINDING_NAME] = CLK_BINDING_NAME
>
> 2. https://lore.kernel.org/linux-arm-msm/95f4e99a60cc97770fc3cee850b62faf.sboyd@kernel.org/
>    Are these supposed to be in a dt-binding header?
>
> 3. https://lore.kernel.org/linux-arm-msm/031d0a35-b192-4161-beef-97b89d5d1da6@linaro.org/
>    Do you use them as well in the DTS?
>
> Having the defines (with the calculations) seemed to to comply
> with the above three feedbacks.
>
> Please let me know if this can be handled in a different way that
> would be consistent with other Qualcomm platforms.

Krzysztof,

Is this ok? Can I post a new version addressing other review comments?

Thanks
Varada

Re: [PATCH v7 1/5] dt-bindings: interconnect: Add Qualcomm IPQ9574 support

[Krzysztof Kozlowski]

On 09/04/2024 09:41, Varadarajan Narayanan wrote:
> On Thu, Apr 04, 2024 at 02:25:06PM +0530, Varadarajan Narayanan wrote:
>> On Wed, Apr 03, 2024 at 04:59:40PM +0200, Krzysztof Kozlowski wrote:
>>> On 03/04/2024 12:42, Varadarajan Narayanan wrote:
>>>> Add interconnect-cells to clock provider so that it can be
>>>> used as icc provider.
>>>>
>>>> Add master/slave ids for Qualcomm IPQ9574 Network-On-Chip
>>>> interfaces. This will be used by the gcc-ipq9574 driver
>>>> that will for providing interconnect services using the
>>>> icc-clk framework.
>>>>
>>>> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
>>>> ---
>>>> v7:
>>>> Fix macro names to be consistent with other bindings
>>>> v6:
>>>> Removed Reviewed-by: Krzysztof Kozlowski
>>>> Redefine the bindings such that driver and DT can share them
>>>>
>>>> v3:
>>>> Squash Documentation/ and include/ changes into same patch
>>>>
>>>> qcom,ipq9574.h
>>>> 	Move 'first id' to clock driver
>>>>
>>>> ---
>>>>  .../bindings/clock/qcom,ipq9574-gcc.yaml      |  3 +
>>>>  .../dt-bindings/interconnect/qcom,ipq9574.h   | 87 +++++++++++++++++++
>>>>  2 files changed, 90 insertions(+)
>>>>  create mode 100644 include/dt-bindings/interconnect/qcom,ipq9574.h
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/clock/qcom,ipq9574-gcc.yaml b/Documentation/devicetree/bindings/clock/qcom,ipq9574-gcc.yaml
>>>> index 944a0ea79cd6..824781cbdf34 100644
>>>> --- a/Documentation/devicetree/bindings/clock/qcom,ipq9574-gcc.yaml
>>>> +++ b/Documentation/devicetree/bindings/clock/qcom,ipq9574-gcc.yaml
>>>> @@ -33,6 +33,9 @@ properties:
>>>>        - description: PCIE30 PHY3 pipe clock source
>>>>        - description: USB3 PHY pipe clock source
>>>>
>>>> +  '#interconnect-cells':
>>>> +    const: 1
>>>> +
>>>>  required:
>>>>    - compatible
>>>>    - clocks
>>>> diff --git a/include/dt-bindings/interconnect/qcom,ipq9574.h b/include/dt-bindings/interconnect/qcom,ipq9574.h
>>>> new file mode 100644
>>>> index 000000000000..0b076b0cf880
>>>> --- /dev/null
>>>> +++ b/include/dt-bindings/interconnect/qcom,ipq9574.h
>>>> @@ -0,0 +1,87 @@
>>>> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
>>>> +#ifndef INTERCONNECT_QCOM_IPQ9574_H
>>>> +#define INTERCONNECT_QCOM_IPQ9574_H
>>>> +
>>>> +#define ICC_ANOC_PCIE0		0
>>>> +#define ICC_SNOC_PCIE0		1
>>>> +#define ICC_ANOC_PCIE1		2
>>>> +#define ICC_SNOC_PCIE1		3
>>>> +#define ICC_ANOC_PCIE2		4
>>>> +#define ICC_SNOC_PCIE2		5
>>>> +#define ICC_ANOC_PCIE3		6
>>>> +#define ICC_SNOC_PCIE3		7
>>>> +#define ICC_SNOC_USB		8
>>>> +#define ICC_ANOC_USB_AXI	9
>>>> +#define ICC_NSSNOC_NSSCC	10
>>>> +#define ICC_NSSNOC_SNOC_0	11
>>>> +#define ICC_NSSNOC_SNOC_1	12
>>>> +#define ICC_NSSNOC_PCNOC_1	13
>>>> +#define ICC_NSSNOC_QOSGEN_REF	14
>>>> +#define ICC_NSSNOC_TIMEOUT_REF	15
>>>> +#define ICC_NSSNOC_XO_DCD	16
>>>> +#define ICC_NSSNOC_ATB		17
>>>> +#define ICC_MEM_NOC_NSSNOC	18
>>>> +#define ICC_NSSNOC_MEMNOC	19
>>>> +#define ICC_NSSNOC_MEM_NOC_1	20
>>>> +
>>>> +#define ICC_NSSNOC_PPE		0
>>>> +#define ICC_NSSNOC_PPE_CFG	1
>>>> +#define ICC_NSSNOC_NSS_CSR	2
>>>> +#define ICC_NSSNOC_IMEM_QSB	3
>>>> +#define ICC_NSSNOC_IMEM_AHB	4
>>>> +
>>>> +#define MASTER_ANOC_PCIE0		(ICC_ANOC_PCIE0 * 2)
>>>> +#define SLAVE_ANOC_PCIE0		((ICC_ANOC_PCIE0 * 2) + 1)
>>>
>>> Which existing Qualcomm platform has such code?
>>
>> Existing Qualcomm platforms don't use icc-clk. They use icc-rpm
>> or icc-rpmh. clk-cbf-msm8996.c is the only driver that uses icc-clk.
>>
>> The icc_clk_register automatically creates master & slave nodes
>> for each clk entry provided as input with the node-ids 'n' and
>> 'n+1'. Since clk-cbf-msm8996.c has only one entry, it could just
>> define MASTER_CBF_M4M and SLAVE_CBF_M4M with 0 and 1 and avoid these
>> calculations.
>>
>> However, ipq9574 gives an array of clock entries as input to
>> icc_clk_register. To tie the order/sequence of these clock
>> entries correctly with the node-ids, this calculation is needed.
>>
>>> This is the third time I am asking for consistent headers. Open
>>> existing, recently added headers and look how it is done there. Why?
>>> Because I am against such calculations and see no reason for them.
>>
>> Apologies. Regret that I have to trouble you.
>>
>> In this ipq9574 case, have to reconcile between the following
>> feedbacks.
>>
>> 1. https://lore.kernel.org/linux-arm-msm/fe40b307-26d0-4b2a-869b-5d093415b9d1@linaro.org/
>>    We could probably use indexed identifiers here to avoid confusion:
>>    [ICC_BINDING_NAME] = CLK_BINDING_NAME
>>
>> 2. https://lore.kernel.org/linux-arm-msm/95f4e99a60cc97770fc3cee850b62faf.sboyd@kernel.org/
>>    Are these supposed to be in a dt-binding header?
>>
>> 3. https://lore.kernel.org/linux-arm-msm/031d0a35-b192-4161-beef-97b89d5d1da6@linaro.org/
>>    Do you use them as well in the DTS?
>>
>> Having the defines (with the calculations) seemed to to comply
>> with the above three feedbacks.
>>
>> Please let me know if this can be handled in a different way that
>> would be consistent with other Qualcomm platforms.
> 
> Krzysztof,
> 
> Is this ok? Can I post a new version addressing other review comments?

I don't understand and you did not answered before, why you have to do
it differently than all other Qualcomm interconnect providers. Maybe the
code here needs it, maybe not, but I don't see any argument proving this.

Best regards,
Krzysztof

Re: [PATCH v7 1/5] dt-bindings: interconnect: Add Qualcomm IPQ9574 support

[Varadarajan Narayanan]

On Tue, Apr 09, 2024 at 11:45:51AM +0200, Krzysztof Kozlowski wrote:
> On 09/04/2024 09:41, Varadarajan Narayanan wrote:
> > On Thu, Apr 04, 2024 at 02:25:06PM +0530, Varadarajan Narayanan wrote:
> >> On Wed, Apr 03, 2024 at 04:59:40PM +0200, Krzysztof Kozlowski wrote:
> >>> On 03/04/2024 12:42, Varadarajan Narayanan wrote:
> >>>> Add interconnect-cells to clock provider so that it can be
> >>>> used as icc provider.
> >>>>
> >>>> Add master/slave ids for Qualcomm IPQ9574 Network-On-Chip
> >>>> interfaces. This will be used by the gcc-ipq9574 driver
> >>>> that will for providing interconnect services using the
> >>>> icc-clk framework.
> >>>>
> >>>> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> >>>> ---
> >>>> v7:
> >>>> Fix macro names to be consistent with other bindings
> >>>> v6:
> >>>> Removed Reviewed-by: Krzysztof Kozlowski
> >>>> Redefine the bindings such that driver and DT can share them
> >>>>
> >>>> v3:
> >>>> Squash Documentation/ and include/ changes into same patch
> >>>>
> >>>> qcom,ipq9574.h
> >>>> 	Move 'first id' to clock driver
> >>>>
> >>>> ---
> >>>>  .../bindings/clock/qcom,ipq9574-gcc.yaml      |  3 +
> >>>>  .../dt-bindings/interconnect/qcom,ipq9574.h   | 87 +++++++++++++++++++
> >>>>  2 files changed, 90 insertions(+)
> >>>>  create mode 100644 include/dt-bindings/interconnect/qcom,ipq9574.h
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/clock/qcom,ipq9574-gcc.yaml b/Documentation/devicetree/bindings/clock/qcom,ipq9574-gcc.yaml
> >>>> index 944a0ea79cd6..824781cbdf34 100644
> >>>> --- a/Documentation/devicetree/bindings/clock/qcom,ipq9574-gcc.yaml
> >>>> +++ b/Documentation/devicetree/bindings/clock/qcom,ipq9574-gcc.yaml
> >>>> @@ -33,6 +33,9 @@ properties:
> >>>>        - description: PCIE30 PHY3 pipe clock source
> >>>>        - description: USB3 PHY pipe clock source
> >>>>
> >>>> +  '#interconnect-cells':
> >>>> +    const: 1
> >>>> +
> >>>>  required:
> >>>>    - compatible
> >>>>    - clocks
> >>>> diff --git a/include/dt-bindings/interconnect/qcom,ipq9574.h b/include/dt-bindings/interconnect/qcom,ipq9574.h
> >>>> new file mode 100644
> >>>> index 000000000000..0b076b0cf880
> >>>> --- /dev/null
> >>>> +++ b/include/dt-bindings/interconnect/qcom,ipq9574.h
> >>>> @@ -0,0 +1,87 @@
> >>>> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> >>>> +#ifndef INTERCONNECT_QCOM_IPQ9574_H
> >>>> +#define INTERCONNECT_QCOM_IPQ9574_H
> >>>> +
> >>>> +#define ICC_ANOC_PCIE0		0
> >>>> +#define ICC_SNOC_PCIE0		1
> >>>> +#define ICC_ANOC_PCIE1		2
> >>>> +#define ICC_SNOC_PCIE1		3
> >>>> +#define ICC_ANOC_PCIE2		4
> >>>> +#define ICC_SNOC_PCIE2		5
> >>>> +#define ICC_ANOC_PCIE3		6
> >>>> +#define ICC_SNOC_PCIE3		7
> >>>> +#define ICC_SNOC_USB		8
> >>>> +#define ICC_ANOC_USB_AXI	9
> >>>> +#define ICC_NSSNOC_NSSCC	10
> >>>> +#define ICC_NSSNOC_SNOC_0	11
> >>>> +#define ICC_NSSNOC_SNOC_1	12
> >>>> +#define ICC_NSSNOC_PCNOC_1	13
> >>>> +#define ICC_NSSNOC_QOSGEN_REF	14
> >>>> +#define ICC_NSSNOC_TIMEOUT_REF	15
> >>>> +#define ICC_NSSNOC_XO_DCD	16
> >>>> +#define ICC_NSSNOC_ATB		17
> >>>> +#define ICC_MEM_NOC_NSSNOC	18
> >>>> +#define ICC_NSSNOC_MEMNOC	19
> >>>> +#define ICC_NSSNOC_MEM_NOC_1	20
> >>>> +
> >>>> +#define ICC_NSSNOC_PPE		0
> >>>> +#define ICC_NSSNOC_PPE_CFG	1
> >>>> +#define ICC_NSSNOC_NSS_CSR	2
> >>>> +#define ICC_NSSNOC_IMEM_QSB	3
> >>>> +#define ICC_NSSNOC_IMEM_AHB	4
> >>>> +
> >>>> +#define MASTER_ANOC_PCIE0		(ICC_ANOC_PCIE0 * 2)
> >>>> +#define SLAVE_ANOC_PCIE0		((ICC_ANOC_PCIE0 * 2) + 1)
> >>>
> >>> Which existing Qualcomm platform has such code?
> >>
> >> Existing Qualcomm platforms don't use icc-clk. They use icc-rpm
> >> or icc-rpmh. clk-cbf-msm8996.c is the only driver that uses icc-clk.
> >>
> >> The icc_clk_register automatically creates master & slave nodes
> >> for each clk entry provided as input with the node-ids 'n' and
> >> 'n+1'. Since clk-cbf-msm8996.c has only one entry, it could just
> >> define MASTER_CBF_M4M and SLAVE_CBF_M4M with 0 and 1 and avoid these
> >> calculations.
> >>
> >> However, ipq9574 gives an array of clock entries as input to
> >> icc_clk_register. To tie the order/sequence of these clock
> >> entries correctly with the node-ids, this calculation is needed.
> >>
> >>> This is the third time I am asking for consistent headers. Open
> >>> existing, recently added headers and look how it is done there. Why?
> >>> Because I am against such calculations and see no reason for them.
> >>
> >> Apologies. Regret that I have to trouble you.
> >>
> >> In this ipq9574 case, have to reconcile between the following
> >> feedbacks.
> >>
> >> 1. https://lore.kernel.org/linux-arm-msm/fe40b307-26d0-4b2a-869b-5d093415b9d1@linaro.org/
> >>    We could probably use indexed identifiers here to avoid confusion:
> >>    [ICC_BINDING_NAME] = CLK_BINDING_NAME
> >>
> >> 2. https://lore.kernel.org/linux-arm-msm/95f4e99a60cc97770fc3cee850b62faf.sboyd@kernel.org/
> >>    Are these supposed to be in a dt-binding header?
> >>
> >> 3. https://lore.kernel.org/linux-arm-msm/031d0a35-b192-4161-beef-97b89d5d1da6@linaro.org/
> >>    Do you use them as well in the DTS?
> >>
> >> Having the defines (with the calculations) seemed to to comply
> >> with the above three feedbacks.
> >>
> >> Please let me know if this can be handled in a different way that
> >> would be consistent with other Qualcomm platforms.
> >
> > Krzysztof,
> >
> > Is this ok? Can I post a new version addressing other review comments?
>
> I don't understand and you did not answered before, why you have to do
> it differently than all other Qualcomm interconnect providers. Maybe the
> code here needs it, maybe not, but I don't see any argument proving this.

Other Qualcomm interconnect providers use the icc-rpm.

	1. The SoC specific interconnect providers have control
	   over the master/slave id-numbers and is hard coded.

	2. These id-numbers are used by the RPM firmware.

IPQ9574 uses icc-clk.

	1. The ipq9574 specific interconnect provider doesn't
	   have control over the master/slave id-numbers. The
	   icc-clk framework auto generates it in the order of
	   the clock entries given as input.

	2. These auto-generated id-numbers have to be correctly
	   tied to the DT nodes. Else, the relevant clocks may
	   not get enabled.

Since ICC-CLK creates two ids per clock entry (one MASTER_xxx and
one SLAVE_xxx), using those MASTER/SLAVE_xxx macros as indices in
the below array would create holes.

	static int icc_ipq9574_hws[] = {
		[MASTER_ANOC_PCIE0] = GCC_ANOC_PCIE0_1LANE_M_CLK,
		[MASTER_SNOC_PCIE0] = GCC_SNOC_PCIE0_1LANE_S_CLK,
		[MASTER_ANOC_PCIE1] = GCC_ANOC_PCIE1_1LANE_M_CLK,
		[MASTER_SNOC_PCIE1] = GCC_SNOC_PCIE1_1LANE_S_CLK,
		. . .
	};

Other Qualcomm drivers don't have this issue and they can
directly use the MASTER/SLAVE_xxx macros.

As the MASTER_xxx macros cannot be used, have to define a new set
of macros that can be used for indices in the above array. This
is the reason for the ICC_BINDING_NAME macros.

Does this answer your concern? Please let me know.

Thanks
Varada

Re: [PATCH v7 1/5] dt-bindings: interconnect: Add Qualcomm IPQ9574 support

[Krzysztof Kozlowski]

On 09/04/2024 13:03, Varadarajan Narayanan wrote:
> On Tue, Apr 09, 2024 at 11:45:51AM +0200, Krzysztof Kozlowski wrote:
>> On 09/04/2024 09:41, Varadarajan Narayanan wrote:
>>> On Thu, Apr 04, 2024 at 02:25:06PM +0530, Varadarajan Narayanan wrote:
>>>> On Wed, Apr 03, 2024 at 04:59:40PM +0200, Krzysztof Kozlowski wrote:
>>>>> On 03/04/2024 12:42, Varadarajan Narayanan wrote:
>>>>>> Add interconnect-cells to clock provider so that it can be
>>>>>> used as icc provider.
>>>>>>
>>>>>> Add master/slave ids for Qualcomm IPQ9574 Network-On-Chip
>>>>>> interfaces. This will be used by the gcc-ipq9574 driver
>>>>>> that will for providing interconnect services using the
>>>>>> icc-clk framework.
>>>>>>
>>>>>> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
>>>>>> ---
>>>>>> v7:
>>>>>> Fix macro names to be consistent with other bindings
>>>>>> v6:
>>>>>> Removed Reviewed-by: Krzysztof Kozlowski
>>>>>> Redefine the bindings such that driver and DT can share them
>>>>>>
>>>>>> v3:
>>>>>> Squash Documentation/ and include/ changes into same patch
>>>>>>
>>>>>> qcom,ipq9574.h
>>>>>> 	Move 'first id' to clock driver
>>>>>>
>>>>>> ---
>>>>>>  .../bindings/clock/qcom,ipq9574-gcc.yaml      |  3 +
>>>>>>  .../dt-bindings/interconnect/qcom,ipq9574.h   | 87 +++++++++++++++++++
>>>>>>  2 files changed, 90 insertions(+)
>>>>>>  create mode 100644 include/dt-bindings/interconnect/qcom,ipq9574.h
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/clock/qcom,ipq9574-gcc.yaml b/Documentation/devicetree/bindings/clock/qcom,ipq9574-gcc.yaml
>>>>>> index 944a0ea79cd6..824781cbdf34 100644
>>>>>> --- a/Documentation/devicetree/bindings/clock/qcom,ipq9574-gcc.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/clock/qcom,ipq9574-gcc.yaml
>>>>>> @@ -33,6 +33,9 @@ properties:
>>>>>>        - description: PCIE30 PHY3 pipe clock source
>>>>>>        - description: USB3 PHY pipe clock source
>>>>>>
>>>>>> +  '#interconnect-cells':
>>>>>> +    const: 1
>>>>>> +
>>>>>>  required:
>>>>>>    - compatible
>>>>>>    - clocks
>>>>>> diff --git a/include/dt-bindings/interconnect/qcom,ipq9574.h b/include/dt-bindings/interconnect/qcom,ipq9574.h
>>>>>> new file mode 100644
>>>>>> index 000000000000..0b076b0cf880
>>>>>> --- /dev/null
>>>>>> +++ b/include/dt-bindings/interconnect/qcom,ipq9574.h
>>>>>> @@ -0,0 +1,87 @@
>>>>>> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
>>>>>> +#ifndef INTERCONNECT_QCOM_IPQ9574_H
>>>>>> +#define INTERCONNECT_QCOM_IPQ9574_H
>>>>>> +
>>>>>> +#define ICC_ANOC_PCIE0		0
>>>>>> +#define ICC_SNOC_PCIE0		1
>>>>>> +#define ICC_ANOC_PCIE1		2
>>>>>> +#define ICC_SNOC_PCIE1		3
>>>>>> +#define ICC_ANOC_PCIE2		4
>>>>>> +#define ICC_SNOC_PCIE2		5
>>>>>> +#define ICC_ANOC_PCIE3		6
>>>>>> +#define ICC_SNOC_PCIE3		7
>>>>>> +#define ICC_SNOC_USB		8
>>>>>> +#define ICC_ANOC_USB_AXI	9
>>>>>> +#define ICC_NSSNOC_NSSCC	10
>>>>>> +#define ICC_NSSNOC_SNOC_0	11
>>>>>> +#define ICC_NSSNOC_SNOC_1	12
>>>>>> +#define ICC_NSSNOC_PCNOC_1	13
>>>>>> +#define ICC_NSSNOC_QOSGEN_REF	14
>>>>>> +#define ICC_NSSNOC_TIMEOUT_REF	15
>>>>>> +#define ICC_NSSNOC_XO_DCD	16
>>>>>> +#define ICC_NSSNOC_ATB		17
>>>>>> +#define ICC_MEM_NOC_NSSNOC	18
>>>>>> +#define ICC_NSSNOC_MEMNOC	19
>>>>>> +#define ICC_NSSNOC_MEM_NOC_1	20
>>>>>> +
>>>>>> +#define ICC_NSSNOC_PPE		0
>>>>>> +#define ICC_NSSNOC_PPE_CFG	1
>>>>>> +#define ICC_NSSNOC_NSS_CSR	2
>>>>>> +#define ICC_NSSNOC_IMEM_QSB	3
>>>>>> +#define ICC_NSSNOC_IMEM_AHB	4
>>>>>> +
>>>>>> +#define MASTER_ANOC_PCIE0		(ICC_ANOC_PCIE0 * 2)
>>>>>> +#define SLAVE_ANOC_PCIE0		((ICC_ANOC_PCIE0 * 2) + 1)
>>>>>
>>>>> Which existing Qualcomm platform has such code?
>>>>
>>>> Existing Qualcomm platforms don't use icc-clk. They use icc-rpm
>>>> or icc-rpmh. clk-cbf-msm8996.c is the only driver that uses icc-clk.
>>>>
>>>> The icc_clk_register automatically creates master & slave nodes
>>>> for each clk entry provided as input with the node-ids 'n' and
>>>> 'n+1'. Since clk-cbf-msm8996.c has only one entry, it could just
>>>> define MASTER_CBF_M4M and SLAVE_CBF_M4M with 0 and 1 and avoid these
>>>> calculations.
>>>>
>>>> However, ipq9574 gives an array of clock entries as input to
>>>> icc_clk_register. To tie the order/sequence of these clock
>>>> entries correctly with the node-ids, this calculation is needed.
>>>>
>>>>> This is the third time I am asking for consistent headers. Open
>>>>> existing, recently added headers and look how it is done there. Why?
>>>>> Because I am against such calculations and see no reason for them.
>>>>
>>>> Apologies. Regret that I have to trouble you.
>>>>
>>>> In this ipq9574 case, have to reconcile between the following
>>>> feedbacks.
>>>>
>>>> 1. https://lore.kernel.org/linux-arm-msm/fe40b307-26d0-4b2a-869b-5d093415b9d1@linaro.org/
>>>>    We could probably use indexed identifiers here to avoid confusion:
>>>>    [ICC_BINDING_NAME] = CLK_BINDING_NAME
>>>>
>>>> 2. https://lore.kernel.org/linux-arm-msm/95f4e99a60cc97770fc3cee850b62faf.sboyd@kernel.org/
>>>>    Are these supposed to be in a dt-binding header?
>>>>
>>>> 3. https://lore.kernel.org/linux-arm-msm/031d0a35-b192-4161-beef-97b89d5d1da6@linaro.org/
>>>>    Do you use them as well in the DTS?
>>>>
>>>> Having the defines (with the calculations) seemed to to comply
>>>> with the above three feedbacks.
>>>>
>>>> Please let me know if this can be handled in a different way that
>>>> would be consistent with other Qualcomm platforms.
>>>
>>> Krzysztof,
>>>
>>> Is this ok? Can I post a new version addressing other review comments?
>>
>> I don't understand and you did not answered before, why you have to do
>> it differently than all other Qualcomm interconnect providers. Maybe the
>> code here needs it, maybe not, but I don't see any argument proving this.
> 
> Other Qualcomm interconnect providers use the icc-rpm.
> 
> 	1. The SoC specific interconnect providers have control
> 	   over the master/slave id-numbers and is hard coded.
> 
> 	2. These id-numbers are used by the RPM firmware.
> 
> IPQ9574 uses icc-clk.
> 
> 	1. The ipq9574 specific interconnect provider doesn't
> 	   have control over the master/slave id-numbers. The
> 	   icc-clk framework auto generates it in the order of
> 	   the clock entries given as input.

Okay, so what happens if icc-clk way of generating them changes a bit?
It can change, why not, driver implementation is not an ABI.

> 
> 	2. These auto-generated id-numbers have to be correctly
> 	   tied to the DT nodes. Else, the relevant clocks may
> 	   not get enabled.

Sorry, I don't get, how auto generated ID number is tied to DT node.
What DT node?

> 
> Since ICC-CLK creates two ids per clock entry (one MASTER_xxx and
> one SLAVE_xxx), using those MASTER/SLAVE_xxx macros as indices in
> the below array would create holes.
> 
> 	static int icc_ipq9574_hws[] = {
> 		[MASTER_ANOC_PCIE0] = GCC_ANOC_PCIE0_1LANE_M_CLK,
> 		[MASTER_SNOC_PCIE0] = GCC_SNOC_PCIE0_1LANE_S_CLK,
> 		[MASTER_ANOC_PCIE1] = GCC_ANOC_PCIE1_1LANE_M_CLK,
> 		[MASTER_SNOC_PCIE1] = GCC_SNOC_PCIE1_1LANE_S_CLK,
> 		. . .
> 	};
> 
> Other Qualcomm drivers don't have this issue and they can
> directly use the MASTER/SLAVE_xxx macros.

I understand, thanks, yet your last patch keeps adding fake IDs, means
IDs which are not part of ABI.

> 
> As the MASTER_xxx macros cannot be used, have to define a new set
> of macros that can be used for indices in the above array. This
> is the reason for the ICC_BINDING_NAME macros.

Then maybe fix the driver, instead of adding something which is not an
ABI to bindings and completely skipping the actual ABI.

Best regards,
Krzysztof

Re: [PATCH v7 1/5] dt-bindings: interconnect: Add Qualcomm IPQ9574 support

[Varadarajan Narayanan]

On Tue, Apr 09, 2024 at 02:20:12PM +0200, Krzysztof Kozlowski wrote:
> On 09/04/2024 13:03, Varadarajan Narayanan wrote:
> > On Tue, Apr 09, 2024 at 11:45:51AM +0200, Krzysztof Kozlowski wrote:
> >> On 09/04/2024 09:41, Varadarajan Narayanan wrote:
> >>> On Thu, Apr 04, 2024 at 02:25:06PM +0530, Varadarajan Narayanan wrote:
> >>>> On Wed, Apr 03, 2024 at 04:59:40PM +0200, Krzysztof Kozlowski wrote:
> >>>>> On 03/04/2024 12:42, Varadarajan Narayanan wrote:
> >>>>>> Add interconnect-cells to clock provider so that it can be
> >>>>>> used as icc provider.
> >>>>>>
> >>>>>> Add master/slave ids for Qualcomm IPQ9574 Network-On-Chip
> >>>>>> interfaces. This will be used by the gcc-ipq9574 driver
> >>>>>> that will for providing interconnect services using the
> >>>>>> icc-clk framework.
> >>>>>>
> >>>>>> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> >>>>>> ---
> >>>>>> v7:
> >>>>>> Fix macro names to be consistent with other bindings
> >>>>>> v6:
> >>>>>> Removed Reviewed-by: Krzysztof Kozlowski
> >>>>>> Redefine the bindings such that driver and DT can share them
> >>>>>>
> >>>>>> v3:
> >>>>>> Squash Documentation/ and include/ changes into same patch
> >>>>>>
> >>>>>> qcom,ipq9574.h
> >>>>>> 	Move 'first id' to clock driver
> >>>>>>
> >>>>>> ---
> >>>>>>  .../bindings/clock/qcom,ipq9574-gcc.yaml      |  3 +
> >>>>>>  .../dt-bindings/interconnect/qcom,ipq9574.h   | 87 +++++++++++++++++++
> >>>>>>  2 files changed, 90 insertions(+)
> >>>>>>  create mode 100644 include/dt-bindings/interconnect/qcom,ipq9574.h
> >>>>>>
> >>>>>> diff --git a/Documentation/devicetree/bindings/clock/qcom,ipq9574-gcc.yaml b/Documentation/devicetree/bindings/clock/qcom,ipq9574-gcc.yaml
> >>>>>> index 944a0ea79cd6..824781cbdf34 100644
> >>>>>> --- a/Documentation/devicetree/bindings/clock/qcom,ipq9574-gcc.yaml
> >>>>>> +++ b/Documentation/devicetree/bindings/clock/qcom,ipq9574-gcc.yaml
> >>>>>> @@ -33,6 +33,9 @@ properties:
> >>>>>>        - description: PCIE30 PHY3 pipe clock source
> >>>>>>        - description: USB3 PHY pipe clock source
> >>>>>>
> >>>>>> +  '#interconnect-cells':
> >>>>>> +    const: 1
> >>>>>> +
> >>>>>>  required:
> >>>>>>    - compatible
> >>>>>>    - clocks
> >>>>>> diff --git a/include/dt-bindings/interconnect/qcom,ipq9574.h b/include/dt-bindings/interconnect/qcom,ipq9574.h
> >>>>>> new file mode 100644
> >>>>>> index 000000000000..0b076b0cf880
> >>>>>> --- /dev/null
> >>>>>> +++ b/include/dt-bindings/interconnect/qcom,ipq9574.h
> >>>>>> @@ -0,0 +1,87 @@
> >>>>>> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> >>>>>> +#ifndef INTERCONNECT_QCOM_IPQ9574_H
> >>>>>> +#define INTERCONNECT_QCOM_IPQ9574_H
> >>>>>> +
> >>>>>> +#define ICC_ANOC_PCIE0		0
> >>>>>> +#define ICC_SNOC_PCIE0		1
> >>>>>> +#define ICC_ANOC_PCIE1		2
> >>>>>> +#define ICC_SNOC_PCIE1		3
> >>>>>> +#define ICC_ANOC_PCIE2		4
> >>>>>> +#define ICC_SNOC_PCIE2		5
> >>>>>> +#define ICC_ANOC_PCIE3		6
> >>>>>> +#define ICC_SNOC_PCIE3		7
> >>>>>> +#define ICC_SNOC_USB		8
> >>>>>> +#define ICC_ANOC_USB_AXI	9
> >>>>>> +#define ICC_NSSNOC_NSSCC	10
> >>>>>> +#define ICC_NSSNOC_SNOC_0	11
> >>>>>> +#define ICC_NSSNOC_SNOC_1	12
> >>>>>> +#define ICC_NSSNOC_PCNOC_1	13
> >>>>>> +#define ICC_NSSNOC_QOSGEN_REF	14
> >>>>>> +#define ICC_NSSNOC_TIMEOUT_REF	15
> >>>>>> +#define ICC_NSSNOC_XO_DCD	16
> >>>>>> +#define ICC_NSSNOC_ATB		17
> >>>>>> +#define ICC_MEM_NOC_NSSNOC	18
> >>>>>> +#define ICC_NSSNOC_MEMNOC	19
> >>>>>> +#define ICC_NSSNOC_MEM_NOC_1	20
> >>>>>> +
> >>>>>> +#define ICC_NSSNOC_PPE		0
> >>>>>> +#define ICC_NSSNOC_PPE_CFG	1
> >>>>>> +#define ICC_NSSNOC_NSS_CSR	2
> >>>>>> +#define ICC_NSSNOC_IMEM_QSB	3
> >>>>>> +#define ICC_NSSNOC_IMEM_AHB	4
> >>>>>> +
> >>>>>> +#define MASTER_ANOC_PCIE0		(ICC_ANOC_PCIE0 * 2)
> >>>>>> +#define SLAVE_ANOC_PCIE0		((ICC_ANOC_PCIE0 * 2) + 1)
> >>>>>
> >>>>> Which existing Qualcomm platform has such code?
> >>>>
> >>>> Existing Qualcomm platforms don't use icc-clk. They use icc-rpm
> >>>> or icc-rpmh. clk-cbf-msm8996.c is the only driver that uses icc-clk.
> >>>>
> >>>> The icc_clk_register automatically creates master & slave nodes
> >>>> for each clk entry provided as input with the node-ids 'n' and
> >>>> 'n+1'. Since clk-cbf-msm8996.c has only one entry, it could just
> >>>> define MASTER_CBF_M4M and SLAVE_CBF_M4M with 0 and 1 and avoid these
> >>>> calculations.
> >>>>
> >>>> However, ipq9574 gives an array of clock entries as input to
> >>>> icc_clk_register. To tie the order/sequence of these clock
> >>>> entries correctly with the node-ids, this calculation is needed.
> >>>>
> >>>>> This is the third time I am asking for consistent headers. Open
> >>>>> existing, recently added headers and look how it is done there. Why?
> >>>>> Because I am against such calculations and see no reason for them.
> >>>>
> >>>> Apologies. Regret that I have to trouble you.
> >>>>
> >>>> In this ipq9574 case, have to reconcile between the following
> >>>> feedbacks.
> >>>>
> >>>> 1. https://lore.kernel.org/linux-arm-msm/fe40b307-26d0-4b2a-869b-5d093415b9d1@linaro.org/
> >>>>    We could probably use indexed identifiers here to avoid confusion:
> >>>>    [ICC_BINDING_NAME] = CLK_BINDING_NAME
> >>>>
> >>>> 2. https://lore.kernel.org/linux-arm-msm/95f4e99a60cc97770fc3cee850b62faf.sboyd@kernel.org/
> >>>>    Are these supposed to be in a dt-binding header?
> >>>>
> >>>> 3. https://lore.kernel.org/linux-arm-msm/031d0a35-b192-4161-beef-97b89d5d1da6@linaro.org/
> >>>>    Do you use them as well in the DTS?
> >>>>
> >>>> Having the defines (with the calculations) seemed to to comply
> >>>> with the above three feedbacks.
> >>>>
> >>>> Please let me know if this can be handled in a different way that
> >>>> would be consistent with other Qualcomm platforms.
> >>>
> >>> Krzysztof,
> >>>
> >>> Is this ok? Can I post a new version addressing other review comments?
> >>
> >> I don't understand and you did not answered before, why you have to do
> >> it differently than all other Qualcomm interconnect providers. Maybe the
> >> code here needs it, maybe not, but I don't see any argument proving this.
> >
> > Other Qualcomm interconnect providers use the icc-rpm.
> >
> > 	1. The SoC specific interconnect providers have control
> > 	   over the master/slave id-numbers and is hard coded.
> >
> > 	2. These id-numbers are used by the RPM firmware.
> >
> > IPQ9574 uses icc-clk.
> >
> > 	1. The ipq9574 specific interconnect provider doesn't
> > 	   have control over the master/slave id-numbers. The
> > 	   icc-clk framework auto generates it in the order of
> > 	   the clock entries given as input.
>
> Okay, so what happens if icc-clk way of generating them changes a bit?
> It can change, why not, driver implementation is not an ABI.
>
> >
> > 	2. These auto-generated id-numbers have to be correctly
> > 	   tied to the DT nodes. Else, the relevant clocks may
> > 	   not get enabled.
>
> Sorry, I don't get, how auto generated ID number is tied to DT node.
> What DT node?

I meant the following usage for the 'interconnects' entry of the
consumer peripheral's node.

	interconnects = <&gcc MASTER_ANOC_PCIE0 &gcc SLAVE_ANOC_PCIE0>,
			      ^^^^^^^^^^^^^^^^^      ^^^^^^^^^^^^^^^^
			<&gcc MASTER_SNOC_PCIE0 &gcc SLAVE_SNOC_PCIE0>;
			      ^^^^^^^^^^^^^^^^^      ^^^^^^^^^^^^^^^^

> > Since ICC-CLK creates two ids per clock entry (one MASTER_xxx and
> > one SLAVE_xxx), using those MASTER/SLAVE_xxx macros as indices in
> > the below array would create holes.
> >
> > 	static int icc_ipq9574_hws[] = {
> > 		[MASTER_ANOC_PCIE0] = GCC_ANOC_PCIE0_1LANE_M_CLK,
> > 		[MASTER_SNOC_PCIE0] = GCC_SNOC_PCIE0_1LANE_S_CLK,
> > 		[MASTER_ANOC_PCIE1] = GCC_ANOC_PCIE1_1LANE_M_CLK,
> > 		[MASTER_SNOC_PCIE1] = GCC_SNOC_PCIE1_1LANE_S_CLK,
> > 		. . .
> > 	};
> >
> > Other Qualcomm drivers don't have this issue and they can
> > directly use the MASTER/SLAVE_xxx macros.
>
> I understand, thanks, yet your last patch keeps adding fake IDs, means
> IDs which are not part of ABI.
>
> >
> > As the MASTER_xxx macros cannot be used, have to define a new set
> > of macros that can be used for indices in the above array. This
> > is the reason for the ICC_BINDING_NAME macros.
>
> Then maybe fix the driver, instead of adding something which is not an
> ABI to bindings and completely skipping the actual ABI.

Will remove the ICC_xxx defines from the header. And in the
driver will change the declaration as follows. Will that be
acceptable?

	static int icc_ipq9574_hws[] = {
		[MASTER_ANOC_PCIE0 / 2] = GCC_ANOC_PCIE0_1LANE_M_CLK,
		[MASTER_SNOC_PCIE0 / 2] = GCC_SNOC_PCIE0_1LANE_S_CLK,
		[MASTER_ANOC_PCIE1 / 2] = GCC_ANOC_PCIE1_1LANE_M_CLK,
		[MASTER_SNOC_PCIE1 / 2] = GCC_SNOC_PCIE1_1LANE_S_CLK,
		[MASTER_ANOC_PCIE2 / 2] = GCC_ANOC_PCIE2_2LANE_M_CLK,
		[MASTER_SNOC_PCIE2 / 2] = GCC_SNOC_PCIE2_2LANE_S_CLK,
		[MASTER_ANOC_PCIE3 / 2] = GCC_ANOC_PCIE3_2LANE_M_CLK,
		[MASTER_SNOC_PCIE3 / 2] = GCC_SNOC_PCIE3_2LANE_S_CLK,
		[MASTER_SNOC_USB / 2] = GCC_SNOC_USB_CLK,
		[MASTER_ANOC_USB_AXI / 2] = GCC_ANOC_USB_AXI_CLK,
		[MASTER_NSSNOC_NSSCC / 2] = GCC_NSSNOC_NSSCC_CLK,
		[MASTER_NSSNOC_SNOC_0 / 2] = GCC_NSSNOC_SNOC_CLK,
		[MASTER_NSSNOC_SNOC_1 / 2] = GCC_NSSNOC_SNOC_1_CLK,
		[MASTER_NSSNOC_PCNOC_1 / 2] = GCC_NSSNOC_PCNOC_1_CLK,
		[MASTER_NSSNOC_QOSGEN_REF / 2] = GCC_NSSNOC_QOSGEN_REF_CLK,
		[MASTER_NSSNOC_TIMEOUT_REF / 2] = GCC_NSSNOC_TIMEOUT_REF_CLK,
		[MASTER_NSSNOC_XO_DCD / 2] = GCC_NSSNOC_XO_DCD_CLK,
		[MASTER_NSSNOC_ATB / 2] = GCC_NSSNOC_ATB_CLK,
		[MASTER_MEM_NOC_NSSNOC / 2] = GCC_MEM_NOC_NSSNOC_CLK,
		[MASTER_NSSNOC_MEMNOC / 2] = GCC_NSSNOC_MEMNOC_CLK,
		[MASTER_NSSNOC_MEM_NOC_1 / 2] = GCC_NSSNOC_MEM_NOC_1_CLK,
	};

Thanks
Varada

Re: [PATCH v7 1/5] dt-bindings: interconnect: Add Qualcomm IPQ9574 support

[Krzysztof Kozlowski]

On 10/04/2024 12:02, Varadarajan Narayanan wrote:
>> Okay, so what happens if icc-clk way of generating them changes a bit?
>> It can change, why not, driver implementation is not an ABI.
>>
>>>
>>> 	2. These auto-generated id-numbers have to be correctly
>>> 	   tied to the DT nodes. Else, the relevant clocks may
>>> 	   not get enabled.
>>
>> Sorry, I don't get, how auto generated ID number is tied to DT node.
>> What DT node?
> 
> I meant the following usage for the 'interconnects' entry of the
> consumer peripheral's node.
> 
> 	interconnects = <&gcc MASTER_ANOC_PCIE0 &gcc SLAVE_ANOC_PCIE0>,
> 			      ^^^^^^^^^^^^^^^^^      ^^^^^^^^^^^^^^^^
> 			<&gcc MASTER_SNOC_PCIE0 &gcc SLAVE_SNOC_PCIE0>;
> 			      ^^^^^^^^^^^^^^^^^      ^^^^^^^^^^^^^^^^
> 
>>> Since ICC-CLK creates two ids per clock entry (one MASTER_xxx and
>>> one SLAVE_xxx), using those MASTER/SLAVE_xxx macros as indices in
>>> the below array would create holes.
>>>
>>> 	static int icc_ipq9574_hws[] = {
>>> 		[MASTER_ANOC_PCIE0] = GCC_ANOC_PCIE0_1LANE_M_CLK,
>>> 		[MASTER_SNOC_PCIE0] = GCC_SNOC_PCIE0_1LANE_S_CLK,
>>> 		[MASTER_ANOC_PCIE1] = GCC_ANOC_PCIE1_1LANE_M_CLK,
>>> 		[MASTER_SNOC_PCIE1] = GCC_SNOC_PCIE1_1LANE_S_CLK,
>>> 		. . .
>>> 	};
>>>
>>> Other Qualcomm drivers don't have this issue and they can
>>> directly use the MASTER/SLAVE_xxx macros.
>>
>> I understand, thanks, yet your last patch keeps adding fake IDs, means
>> IDs which are not part of ABI.
>>
>>>
>>> As the MASTER_xxx macros cannot be used, have to define a new set
>>> of macros that can be used for indices in the above array. This
>>> is the reason for the ICC_BINDING_NAME macros.
>>
>> Then maybe fix the driver, instead of adding something which is not an
>> ABI to bindings and completely skipping the actual ABI.
> 
> Will remove the ICC_xxx defines from the header. And in the
> driver will change the declaration as follows. Will that be
> acceptable?
> 
> 	static int icc_ipq9574_hws[] = {
> 		[MASTER_ANOC_PCIE0 / 2] = GCC_ANOC_PCIE0_1LANE_M_CLK,

What is the binding in such case? What exactly do you bind between
driver and DTS?

Best regards,
Krzysztof

Re: [PATCH v7 1/5] dt-bindings: interconnect: Add Qualcomm IPQ9574 support

[Konrad Dybcio]

On 4/10/24 13:15, Krzysztof Kozlowski wrote:
> On 10/04/2024 12:02, Varadarajan Narayanan wrote:
>>> Okay, so what happens if icc-clk way of generating them changes a bit?
>>> It can change, why not, driver implementation is not an ABI.
>>>
>>>>
>>>> 	2. These auto-generated id-numbers have to be correctly
>>>> 	   tied to the DT nodes. Else, the relevant clocks may
>>>> 	   not get enabled.
>>>
>>> Sorry, I don't get, how auto generated ID number is tied to DT node.
>>> What DT node?
>>
>> I meant the following usage for the 'interconnects' entry of the
>> consumer peripheral's node.
>>
>> 	interconnects = <&gcc MASTER_ANOC_PCIE0 &gcc SLAVE_ANOC_PCIE0>,
>> 			      ^^^^^^^^^^^^^^^^^      ^^^^^^^^^^^^^^^^
>> 			<&gcc MASTER_SNOC_PCIE0 &gcc SLAVE_SNOC_PCIE0>;
>> 			      ^^^^^^^^^^^^^^^^^      ^^^^^^^^^^^^^^^^
>>
>>>> Since ICC-CLK creates two ids per clock entry (one MASTER_xxx and
>>>> one SLAVE_xxx), using those MASTER/SLAVE_xxx macros as indices in
>>>> the below array would create holes.
>>>>
>>>> 	static int icc_ipq9574_hws[] = {
>>>> 		[MASTER_ANOC_PCIE0] = GCC_ANOC_PCIE0_1LANE_M_CLK,
>>>> 		[MASTER_SNOC_PCIE0] = GCC_SNOC_PCIE0_1LANE_S_CLK,
>>>> 		[MASTER_ANOC_PCIE1] = GCC_ANOC_PCIE1_1LANE_M_CLK,
>>>> 		[MASTER_SNOC_PCIE1] = GCC_SNOC_PCIE1_1LANE_S_CLK,
>>>> 		. . .
>>>> 	};
>>>>
>>>> Other Qualcomm drivers don't have this issue and they can
>>>> directly use the MASTER/SLAVE_xxx macros.
>>>
>>> I understand, thanks, yet your last patch keeps adding fake IDs, means
>>> IDs which are not part of ABI.
>>>
>>>>
>>>> As the MASTER_xxx macros cannot be used, have to define a new set
>>>> of macros that can be used for indices in the above array. This
>>>> is the reason for the ICC_BINDING_NAME macros.
>>>
>>> Then maybe fix the driver, instead of adding something which is not an
>>> ABI to bindings and completely skipping the actual ABI.
>>
>> Will remove the ICC_xxx defines from the header. And in the
>> driver will change the declaration as follows. Will that be
>> acceptable?
>>
>> 	static int icc_ipq9574_hws[] = {
>> 		[MASTER_ANOC_PCIE0 / 2] = GCC_ANOC_PCIE0_1LANE_M_CLK,
> 
> What is the binding in such case? What exactly do you bind between
> driver and DTS?

I think what Krzysztof is trying to say here is "the icc-clk API is tragic"
and the best solution would be to make it such that the interconnect indices
are set explicitly, instead of (master, slave), (master, slave) etc.

Does that sound good, Krzysztof?

Konrad

Re: [PATCH v7 1/5] dt-bindings: interconnect: Add Qualcomm IPQ9574 support

[Krzysztof Kozlowski]

On 10/04/2024 13:48, Konrad Dybcio wrote:
> 
> 
> On 4/10/24 13:15, Krzysztof Kozlowski wrote:
>> On 10/04/2024 12:02, Varadarajan Narayanan wrote:
>>>> Okay, so what happens if icc-clk way of generating them changes a bit?
>>>> It can change, why not, driver implementation is not an ABI.
>>>>
>>>>>
>>>>> 	2. These auto-generated id-numbers have to be correctly
>>>>> 	   tied to the DT nodes. Else, the relevant clocks may
>>>>> 	   not get enabled.
>>>>
>>>> Sorry, I don't get, how auto generated ID number is tied to DT node.
>>>> What DT node?
>>>
>>> I meant the following usage for the 'interconnects' entry of the
>>> consumer peripheral's node.
>>>
>>> 	interconnects = <&gcc MASTER_ANOC_PCIE0 &gcc SLAVE_ANOC_PCIE0>,
>>> 			      ^^^^^^^^^^^^^^^^^      ^^^^^^^^^^^^^^^^
>>> 			<&gcc MASTER_SNOC_PCIE0 &gcc SLAVE_SNOC_PCIE0>;
>>> 			      ^^^^^^^^^^^^^^^^^      ^^^^^^^^^^^^^^^^
>>>
>>>>> Since ICC-CLK creates two ids per clock entry (one MASTER_xxx and
>>>>> one SLAVE_xxx), using those MASTER/SLAVE_xxx macros as indices in
>>>>> the below array would create holes.
>>>>>
>>>>> 	static int icc_ipq9574_hws[] = {
>>>>> 		[MASTER_ANOC_PCIE0] = GCC_ANOC_PCIE0_1LANE_M_CLK,
>>>>> 		[MASTER_SNOC_PCIE0] = GCC_SNOC_PCIE0_1LANE_S_CLK,
>>>>> 		[MASTER_ANOC_PCIE1] = GCC_ANOC_PCIE1_1LANE_M_CLK,
>>>>> 		[MASTER_SNOC_PCIE1] = GCC_SNOC_PCIE1_1LANE_S_CLK,
>>>>> 		. . .
>>>>> 	};
>>>>>
>>>>> Other Qualcomm drivers don't have this issue and they can
>>>>> directly use the MASTER/SLAVE_xxx macros.
>>>>
>>>> I understand, thanks, yet your last patch keeps adding fake IDs, means
>>>> IDs which are not part of ABI.
>>>>
>>>>>
>>>>> As the MASTER_xxx macros cannot be used, have to define a new set
>>>>> of macros that can be used for indices in the above array. This
>>>>> is the reason for the ICC_BINDING_NAME macros.
>>>>
>>>> Then maybe fix the driver, instead of adding something which is not an
>>>> ABI to bindings and completely skipping the actual ABI.
>>>
>>> Will remove the ICC_xxx defines from the header. And in the
>>> driver will change the declaration as follows. Will that be
>>> acceptable?
>>>
>>> 	static int icc_ipq9574_hws[] = {
>>> 		[MASTER_ANOC_PCIE0 / 2] = GCC_ANOC_PCIE0_1LANE_M_CLK,
>>
>> What is the binding in such case? What exactly do you bind between
>> driver and DTS?
> 
> I think what Krzysztof is trying to say here is "the icc-clk API is tragic"
> and the best solution would be to make it such that the interconnect indices
> are set explicitly, instead of (master, slave), (master, slave) etc.
> 
> Does that sound good, Krzysztof?

Yes, I think earlier I expressed that icc-clk might needs fixes. The
indices you define in the binding must be used by DTS and by the driver.
Directly, otherwise it is error-prone and not really an ABI...

Best regards,
Krzysztof

Re: [PATCH v7 1/5] dt-bindings: interconnect: Add Qualcomm IPQ9574 support

[Varadarajan Narayanan]

On Wed, Apr 10, 2024 at 02:01:00PM +0200, Krzysztof Kozlowski wrote:
> On 10/04/2024 13:48, Konrad Dybcio wrote:
> >
> >
> > On 4/10/24 13:15, Krzysztof Kozlowski wrote:
> >> On 10/04/2024 12:02, Varadarajan Narayanan wrote:
> >>>> Okay, so what happens if icc-clk way of generating them changes a bit?
> >>>> It can change, why not, driver implementation is not an ABI.
> >>>>
> >>>>>
> >>>>> 	2. These auto-generated id-numbers have to be correctly
> >>>>> 	   tied to the DT nodes. Else, the relevant clocks may
> >>>>> 	   not get enabled.
> >>>>
> >>>> Sorry, I don't get, how auto generated ID number is tied to DT node.
> >>>> What DT node?
> >>>
> >>> I meant the following usage for the 'interconnects' entry of the
> >>> consumer peripheral's node.
> >>>
> >>> 	interconnects = <&gcc MASTER_ANOC_PCIE0 &gcc SLAVE_ANOC_PCIE0>,
> >>> 			      ^^^^^^^^^^^^^^^^^      ^^^^^^^^^^^^^^^^
> >>> 			<&gcc MASTER_SNOC_PCIE0 &gcc SLAVE_SNOC_PCIE0>;
> >>> 			      ^^^^^^^^^^^^^^^^^      ^^^^^^^^^^^^^^^^
> >>>
> >>>>> Since ICC-CLK creates two ids per clock entry (one MASTER_xxx and
> >>>>> one SLAVE_xxx), using those MASTER/SLAVE_xxx macros as indices in
> >>>>> the below array would create holes.
> >>>>>
> >>>>> 	static int icc_ipq9574_hws[] = {
> >>>>> 		[MASTER_ANOC_PCIE0] = GCC_ANOC_PCIE0_1LANE_M_CLK,
> >>>>> 		[MASTER_SNOC_PCIE0] = GCC_SNOC_PCIE0_1LANE_S_CLK,
> >>>>> 		[MASTER_ANOC_PCIE1] = GCC_ANOC_PCIE1_1LANE_M_CLK,
> >>>>> 		[MASTER_SNOC_PCIE1] = GCC_SNOC_PCIE1_1LANE_S_CLK,
> >>>>> 		. . .
> >>>>> 	};
> >>>>>
> >>>>> Other Qualcomm drivers don't have this issue and they can
> >>>>> directly use the MASTER/SLAVE_xxx macros.
> >>>>
> >>>> I understand, thanks, yet your last patch keeps adding fake IDs, means
> >>>> IDs which are not part of ABI.
> >>>>
> >>>>>
> >>>>> As the MASTER_xxx macros cannot be used, have to define a new set
> >>>>> of macros that can be used for indices in the above array. This
> >>>>> is the reason for the ICC_BINDING_NAME macros.
> >>>>
> >>>> Then maybe fix the driver, instead of adding something which is not an
> >>>> ABI to bindings and completely skipping the actual ABI.
> >>>
> >>> Will remove the ICC_xxx defines from the header. And in the
> >>> driver will change the declaration as follows. Will that be
> >>> acceptable?
> >>>
> >>> 	static int icc_ipq9574_hws[] = {
> >>> 		[MASTER_ANOC_PCIE0 / 2] = GCC_ANOC_PCIE0_1LANE_M_CLK,
> >>
> >> What is the binding in such case? What exactly do you bind between
> >> driver and DTS?
> >
> > I think what Krzysztof is trying to say here is "the icc-clk API is tragic"
> > and the best solution would be to make it such that the interconnect indices
> > are set explicitly, instead of (master, slave), (master, slave) etc.
> >
> > Does that sound good, Krzysztof?
>
> Yes, I think earlier I expressed that icc-clk might needs fixes.

Ok

> The indices you define in the binding must be used by DTS and by the driver.

There are 3 drivers in play here.
	1. The icc-clk driver
	2. The gcc (i.e. the interconnect driver)
	3. The consumer peripheral's driver

By 'driver' I assume, you mean the icc-clk driver.

> Directly, otherwise it is error-prone and not really an ABI...

To address this, will modify the icc-clk driver as follows.

	==========================================
	diff --git a/include/linux/interconnect-clk.h b/include/linux/interconnect-clk.h
	index 5c611a8b0892..9bcee3e9c56c 100644
	--- a/include/linux/interconnect-clk.h
	+++ b/include/linux/interconnect-clk.h
	@@ -11,6 +11,8 @@ struct device;
	 struct icc_clk_data {
		struct clk *clk;
		const char *name;
	+	unsigned int master_id;
	+	unsigned int slave_id;
	 };


	diff --git a/drivers/interconnect/icc-clk.c b/drivers/interconnect/icc-clk.c
	index bce946592c98..f788db15cd76 100644
	--- a/drivers/interconnect/icc-clk.c
	+++ b/drivers/interconnect/icc-clk.c
	@@ -108,7 +108,7 @@ struct icc_provider *icc_clk_register(struct device *dev,
		for (i = 0, j = 0; i < num_clocks; i++) {
			qp->clocks[i].clk = data[i].clk;

	-		node = icc_node_create(first_id + j);
	+		node = icc_node_create(first_id + data[i].master_id);
			if (IS_ERR(node)) {
				ret = PTR_ERR(node);
				goto err;
	@@ -118,10 +118,10 @@ struct icc_provider *icc_clk_register(struct device *dev,
			node->data = &qp->clocks[i];
			icc_node_add(node, provider);
			/* link to the next node, slave */
	-		icc_link_create(node, first_id + j + 1);
	+		icc_link_create(node, first_id + data[i].slave_id);
			onecell->nodes[j++] = node;

	-		node = icc_node_create(first_id + j);
	+		node = icc_node_create(first_id + data[i].slave_id);
			if (IS_ERR(node)) {
				ret = PTR_ERR(node);
				goto err;
	==========================================

And update the inputs going from gcc-ipq9574.c accordingly
to use the MASTER_xxx and SLAVE_xxx defines. Will this be ok?

Konrad & Krzysztof kindly let me know.

Thanks
Varada

Re: [PATCH v7 1/5] dt-bindings: interconnect: Add Qualcomm IPQ9574 support

[Varadarajan Narayanan]

On Fri, Apr 12, 2024 at 03:02:47PM +0530, Varadarajan Narayanan wrote:
> On Wed, Apr 10, 2024 at 02:01:00PM +0200, Krzysztof Kozlowski wrote:
> > On 10/04/2024 13:48, Konrad Dybcio wrote:
> > >
> > >
> > > On 4/10/24 13:15, Krzysztof Kozlowski wrote:
> > >> On 10/04/2024 12:02, Varadarajan Narayanan wrote:
> > >>>> Okay, so what happens if icc-clk way of generating them changes a bit?
> > >>>> It can change, why not, driver implementation is not an ABI.
> > >>>>
> > >>>>>
> > >>>>> 	2. These auto-generated id-numbers have to be correctly
> > >>>>> 	   tied to the DT nodes. Else, the relevant clocks may
> > >>>>> 	   not get enabled.
> > >>>>
> > >>>> Sorry, I don't get, how auto generated ID number is tied to DT node.
> > >>>> What DT node?
> > >>>
> > >>> I meant the following usage for the 'interconnects' entry of the
> > >>> consumer peripheral's node.
> > >>>
> > >>> 	interconnects = <&gcc MASTER_ANOC_PCIE0 &gcc SLAVE_ANOC_PCIE0>,
> > >>> 			      ^^^^^^^^^^^^^^^^^      ^^^^^^^^^^^^^^^^
> > >>> 			<&gcc MASTER_SNOC_PCIE0 &gcc SLAVE_SNOC_PCIE0>;
> > >>> 			      ^^^^^^^^^^^^^^^^^      ^^^^^^^^^^^^^^^^
> > >>>
> > >>>>> Since ICC-CLK creates two ids per clock entry (one MASTER_xxx and
> > >>>>> one SLAVE_xxx), using those MASTER/SLAVE_xxx macros as indices in
> > >>>>> the below array would create holes.
> > >>>>>
> > >>>>> 	static int icc_ipq9574_hws[] = {
> > >>>>> 		[MASTER_ANOC_PCIE0] = GCC_ANOC_PCIE0_1LANE_M_CLK,
> > >>>>> 		[MASTER_SNOC_PCIE0] = GCC_SNOC_PCIE0_1LANE_S_CLK,
> > >>>>> 		[MASTER_ANOC_PCIE1] = GCC_ANOC_PCIE1_1LANE_M_CLK,
> > >>>>> 		[MASTER_SNOC_PCIE1] = GCC_SNOC_PCIE1_1LANE_S_CLK,
> > >>>>> 		. . .
> > >>>>> 	};
> > >>>>>
> > >>>>> Other Qualcomm drivers don't have this issue and they can
> > >>>>> directly use the MASTER/SLAVE_xxx macros.
> > >>>>
> > >>>> I understand, thanks, yet your last patch keeps adding fake IDs, means
> > >>>> IDs which are not part of ABI.
> > >>>>
> > >>>>>
> > >>>>> As the MASTER_xxx macros cannot be used, have to define a new set
> > >>>>> of macros that can be used for indices in the above array. This
> > >>>>> is the reason for the ICC_BINDING_NAME macros.
> > >>>>
> > >>>> Then maybe fix the driver, instead of adding something which is not an
> > >>>> ABI to bindings and completely skipping the actual ABI.
> > >>>
> > >>> Will remove the ICC_xxx defines from the header. And in the
> > >>> driver will change the declaration as follows. Will that be
> > >>> acceptable?
> > >>>
> > >>> 	static int icc_ipq9574_hws[] = {
> > >>> 		[MASTER_ANOC_PCIE0 / 2] = GCC_ANOC_PCIE0_1LANE_M_CLK,
> > >>
> > >> What is the binding in such case? What exactly do you bind between
> > >> driver and DTS?
> > >
> > > I think what Krzysztof is trying to say here is "the icc-clk API is tragic"
> > > and the best solution would be to make it such that the interconnect indices
> > > are set explicitly, instead of (master, slave), (master, slave) etc.
> > >
> > > Does that sound good, Krzysztof?
> >
> > Yes, I think earlier I expressed that icc-clk might needs fixes.
>
> Ok
>
> > The indices you define in the binding must be used by DTS and by the driver.
>
> There are 3 drivers in play here.
> 	1. The icc-clk driver
> 	2. The gcc (i.e. the interconnect driver)
> 	3. The consumer peripheral's driver
>
> By 'driver' I assume, you mean the icc-clk driver.
>
> > Directly, otherwise it is error-prone and not really an ABI...
>
> To address this, will modify the icc-clk driver as follows.
>
> 	==========================================
> 	diff --git a/include/linux/interconnect-clk.h b/include/linux/interconnect-clk.h
> 	index 5c611a8b0892..9bcee3e9c56c 100644
> 	--- a/include/linux/interconnect-clk.h
> 	+++ b/include/linux/interconnect-clk.h
> 	@@ -11,6 +11,8 @@ struct device;
> 	 struct icc_clk_data {
> 		struct clk *clk;
> 		const char *name;
> 	+	unsigned int master_id;
> 	+	unsigned int slave_id;
> 	 };
>
>
> 	diff --git a/drivers/interconnect/icc-clk.c b/drivers/interconnect/icc-clk.c
> 	index bce946592c98..f788db15cd76 100644
> 	--- a/drivers/interconnect/icc-clk.c
> 	+++ b/drivers/interconnect/icc-clk.c
> 	@@ -108,7 +108,7 @@ struct icc_provider *icc_clk_register(struct device *dev,
> 		for (i = 0, j = 0; i < num_clocks; i++) {
> 			qp->clocks[i].clk = data[i].clk;
>
> 	-		node = icc_node_create(first_id + j);
> 	+		node = icc_node_create(first_id + data[i].master_id);
> 			if (IS_ERR(node)) {
> 				ret = PTR_ERR(node);
> 				goto err;
> 	@@ -118,10 +118,10 @@ struct icc_provider *icc_clk_register(struct device *dev,
> 			node->data = &qp->clocks[i];
> 			icc_node_add(node, provider);
> 			/* link to the next node, slave */
> 	-		icc_link_create(node, first_id + j + 1);
> 	+		icc_link_create(node, first_id + data[i].slave_id);
> 			onecell->nodes[j++] = node;
>
> 	-		node = icc_node_create(first_id + j);
> 	+		node = icc_node_create(first_id + data[i].slave_id);
> 			if (IS_ERR(node)) {
> 				ret = PTR_ERR(node);
> 				goto err;
> 	==========================================
>
> And update the inputs going from gcc-ipq9574.c accordingly
> to use the MASTER_xxx and SLAVE_xxx defines. Will this be ok?
>
> Konrad & Krzysztof kindly let me know.

Have addressed these and other comments and posted v8.
Please review.

Thanks
Varada