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.
---
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 | 39 +++++++++++-
drivers/clk/qcom/common.h | 3 +
drivers/clk/qcom/gcc-ipq9574.c | 54 +++++++++++++++++
drivers/interconnect/icc-clk.c | 29 +++++++++
.../dt-bindings/interconnect/qcom,ipq9574.h | 59 +++++++++++++++++++
include/linux/interconnect-clk.h | 4 ++
9 files changed, 194 insertions(+), 1 deletion(-)
create mode 100644 include/dt-bindings/interconnect/qcom,ipq9574.h
--
2.34.1
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.
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Signed-off-by: Varadarajan Narayanan <[email protected]>
---
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 | 59 +++++++++++++++++++
2 files changed, 62 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..9c95fbd5dc46
--- /dev/null
+++ b/include/dt-bindings/interconnect/qcom,ipq9574.h
@@ -0,0 +1,59 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+#ifndef INTERCONNECT_QCOM_IPQ9574_H
+#define INTERCONNECT_QCOM_IPQ9574_H
+
+#define MASTER_ANOC_PCIE0_1 0
+#define SLAVE_ANOC_PCIE0_1 1
+#define MASTER_SNOC_PCIE0_1 2
+#define SLAVE_SNOC_PCIE0_1 3
+#define MASTER_ANOC_PCIE1_1 4
+#define SLAVE_ANOC_PCIE1_1 5
+#define MASTER_SNOC_PCIE1_1 6
+#define SLAVE_SNOC_PCIE1_1 7
+#define MASTER_ANOC_PCIE2_2 8
+#define SLAVE_ANOC_PCIE2_2 9
+#define MASTER_SNOC_PCIE2_2 10
+#define SLAVE_SNOC_PCIE2_2 11
+#define MASTER_ANOC_PCIE3_2 12
+#define SLAVE_ANOC_PCIE3_2 13
+#define MASTER_SNOC_PCIE3_2 14
+#define SLAVE_SNOC_PCIE3_2 15
+#define MASTER_USB 16
+#define SLAVE_USB 17
+#define MASTER_USB_AXI 18
+#define SLAVE_USB_AXI 19
+#define MASTER_NSSNOC_NSSCC 20
+#define SLAVE_NSSNOC_NSSCC 21
+#define MASTER_NSSNOC_SNOC 22
+#define SLAVE_NSSNOC_SNOC 23
+#define MASTER_NSSNOC_SNOC_1 24
+#define SLAVE_NSSNOC_SNOC_1 25
+#define MASTER_NSSNOC_PCNOC_1 26
+#define SLAVE_NSSNOC_PCNOC_1 27
+#define MASTER_NSSNOC_QOSGEN_REF 28
+#define SLAVE_NSSNOC_QOSGEN_REF 29
+#define MASTER_NSSNOC_TIMEOUT_REF 30
+#define SLAVE_NSSNOC_TIMEOUT_REF 31
+#define MASTER_NSSNOC_XO_DCD 32
+#define SLAVE_NSSNOC_XO_DCD 33
+#define MASTER_NSSNOC_ATB 34
+#define SLAVE_NSSNOC_ATB 35
+#define MASTER_MEM_NOC_NSSNOC 36
+#define SLAVE_MEM_NOC_NSSNOC 37
+#define MASTER_NSSNOC_MEMNOC 38
+#define SLAVE_NSSNOC_MEMNOC 39
+#define MASTER_NSSNOC_MEM_NOC_1 40
+#define SLAVE_NSSNOC_MEM_NOC_1 41
+
+#define MASTER_NSS_CC_NSSNOC_PPE 0
+#define SLAVE_NSS_CC_NSSNOC_PPE 1
+#define MASTER_NSS_CC_NSSNOC_PPE_CFG 2
+#define SLAVE_NSS_CC_NSSNOC_PPE_CFG 3
+#define MASTER_NSS_CC_NSSNOC_NSS_CSR 4
+#define SLAVE_NSS_CC_NSSNOC_NSS_CSR 5
+#define MASTER_NSS_CC_NSSNOC_IMEM_QSB 6
+#define SLAVE_NSS_CC_NSSNOC_IMEM_QSB 7
+#define MASTER_NSS_CC_NSSNOC_IMEM_AHB 8
+#define SLAVE_NSS_CC_NSSNOC_IMEM_AHB 9
+
+#endif /* INTERCONNECT_QCOM_IPQ9574_H */
--
2.34.1
Wrap icc_clk_register to create devm_icc_clk_register to be
able to release the resources properly.
Signed-off-by: Varadarajan Narayanan <[email protected]>
---
v5: Introduced devm_icc_clk_register
---
drivers/interconnect/icc-clk.c | 29 +++++++++++++++++++++++++++++
include/linux/interconnect-clk.h | 4 ++++
2 files changed, 33 insertions(+)
diff --git a/drivers/interconnect/icc-clk.c b/drivers/interconnect/icc-clk.c
index d787f2ea36d9..89f11fed8820 100644
--- a/drivers/interconnect/icc-clk.c
+++ b/drivers/interconnect/icc-clk.c
@@ -148,6 +148,35 @@ struct icc_provider *icc_clk_register(struct device *dev,
}
EXPORT_SYMBOL_GPL(icc_clk_register);
+static void devm_icc_release(struct device *dev, void *res)
+{
+ icc_clk_unregister(res);
+}
+
+struct icc_provider *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, **provp;
+
+ provp = devres_alloc(devm_icc_release, sizeof(*provp), GFP_KERNEL);
+ if (!provp)
+ return ERR_PTR(-ENOMEM);
+
+ prov = icc_clk_register(dev, first_id, num_clocks, data);
+
+ if (!IS_ERR(prov)) {
+ *provp = prov;
+ devres_add(dev, provp);
+ } else {
+ devres_free(provp);
+ }
+
+ return 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..cb7b648eb1c0 100644
--- a/include/linux/interconnect-clk.h
+++ b/include/linux/interconnect-clk.h
@@ -17,6 +17,10 @@ struct icc_provider *icc_clk_register(struct device *dev,
unsigned int first_id,
unsigned int num_clocks,
const struct icc_clk_data *data);
+struct icc_provider *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
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.
Signed-off-by: Varadarajan Narayanan <[email protected]>
---
v2: Fix include file order
Move to separate patch
---
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 7f2e5cbf3bbb..5b3e69379b1f 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/thermal/thermal.h>
@@ -306,6 +307,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
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 <[email protected]>
---
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(+)
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)
--
2.34.1
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 <[email protected]>
---
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 | 39 ++++++++++++++++++++++++++++++++++++++-
drivers/clk/qcom/common.h | 3 +++
2 files changed, 41 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
index 75f09e6e057e..9fa271812373 100644
--- a/drivers/clk/qcom/common.c
+++ b/drivers/clk/qcom/common.c
@@ -8,6 +8,8 @@
#include <linux/regmap.h>
#include <linux/platform_device.h>
#include <linux/clk-provider.h>
+#include <linux/interconnect-clk.h>
+#include <linux/interconnect-provider.h>
#include <linux/reset-controller.h>
#include <linux/of.h>
@@ -234,6 +236,41 @@ static struct clk_hw *qcom_cc_clk_hw_get(struct of_phandle_args *clkspec,
return cc->rclks[idx] ? &cc->rclks[idx]->hw : NULL;
}
+#if IS_ENABLED(CONFIG_INTERCONNECT_CLK)
+static int qcom_cc_icc_register(struct device *dev,
+ const struct qcom_cc_desc *desc)
+{
+ struct icc_clk_data *icd;
+ int i;
+
+ 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], "qcom");
+ if (IS_ERR(icd[i].clk))
+ return dev_err_probe(dev, PTR_ERR(icd[i].clk),
+ "get clock failed (%ld)\n",
+ PTR_ERR(icd[i].clk));
+
+ icd[i].name = clk_hw_get_name(desc->icc_hws[i]);
+ }
+
+ return PTR_ERR_OR_ZERO(devm_icc_clk_register(dev, desc->first_id,
+ desc->num_icc_hws, icd));
+}
+#else
+static int qcom_cc_icc_register(struct device *dev,
+ const struct qcom_cc_desc *desc)
+{
+ return 0;
+}
+#endif
+
int qcom_cc_really_probe(struct platform_device *pdev,
const struct qcom_cc_desc *desc, struct regmap *regmap)
{
@@ -303,7 +340,7 @@ int qcom_cc_really_probe(struct platform_device *pdev,
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 9c8f7b798d9f..d8ac26d83f3c 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 first_id;
};
/**
--
2.34.1
Quoting Varadarajan Narayanan (2024-03-28 00:59:35)
> 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
> @@ -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 */
How is this supposed to stay unique? I don't understand
icc_node_create() API quite honestly. Why can't icc_clk_register()
maintain some ida of allocated numbers? Or is there some global number
space that we can "reserve" from? I'm quite amazed this is how things
are connected in interconnect framework.
> +
> +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?
> +
> +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,
Quoting Varadarajan Narayanan (2024-03-28 00:59:34)
> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> index 75f09e6e057e..9fa271812373 100644
> --- a/drivers/clk/qcom/common.c
> +++ b/drivers/clk/qcom/common.c
> @@ -8,6 +8,8 @@
> #include <linux/regmap.h>
> #include <linux/platform_device.h>
> #include <linux/clk-provider.h>
> +#include <linux/interconnect-clk.h>
> +#include <linux/interconnect-provider.h>
Do we need the second include?
> #include <linux/reset-controller.h>
> #include <linux/of.h>
>
> @@ -234,6 +236,41 @@ static struct clk_hw *qcom_cc_clk_hw_get(struct of_phandle_args *clkspec,
> return cc->rclks[idx] ? &cc->rclks[idx]->hw : NULL;
> }
>
> +#if IS_ENABLED(CONFIG_INTERCONNECT_CLK)
> +static int qcom_cc_icc_register(struct device *dev,
> + const struct qcom_cc_desc *desc)
> +{
> + struct icc_clk_data *icd;
> + int i;
> +
> + 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], "qcom");
Make the con_id "icc" instead please, so we know the consumer is
icc_clk. Even better would be for the icc_clk device itself to be the
one requesting with devm_clk_hw_get_clk() so that we associate the clk
handle with the consumer device. It would also help us make it so that
drivers defer probe until their clk isn't an orphan.
> + if (IS_ERR(icd[i].clk))
> + return dev_err_probe(dev, PTR_ERR(icd[i].clk),
> + "get clock failed (%ld)\n",
> + PTR_ERR(icd[i].clk));
> +
> + icd[i].name = clk_hw_get_name(desc->icc_hws[i]);
> + }
> +
> + return PTR_ERR_OR_ZERO(devm_icc_clk_register(dev, desc->first_id,
> + desc->num_icc_hws, icd));
> +}
> +#else
> +static int qcom_cc_icc_register(struct device *dev,
> + const struct qcom_cc_desc *desc)
> +{
> + return 0;
> +}
Instead of this please have an
if (!IS_ENABLED(CONFIG_INTERCONNECT_CLK))
return 0;
> +#endif
> +
> int qcom_cc_really_probe(struct platform_device *pdev,
> const struct qcom_cc_desc *desc, struct regmap *regmap)
> {
> @@ -303,7 +340,7 @@ int qcom_cc_really_probe(struct platform_device *pdev,
> 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 9c8f7b798d9f..d8ac26d83f3c 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 first_id;
'first_id' is gross.
On Thu, Mar 28, 2024 at 02:54:52PM -0700, Stephen Boyd wrote:
> Quoting Varadarajan Narayanan (2024-03-28 00:59:34)
> > diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> > index 75f09e6e057e..9fa271812373 100644
> > --- a/drivers/clk/qcom/common.c
> > +++ b/drivers/clk/qcom/common.c
> > @@ -8,6 +8,8 @@
> > #include <linux/regmap.h>
> > #include <linux/platform_device.h>
> > #include <linux/clk-provider.h>
> > +#include <linux/interconnect-clk.h>
> > +#include <linux/interconnect-provider.h>
>
> Do we need the second include?
Will remove.
> > #include <linux/reset-controller.h>
> > #include <linux/of.h>
> >
> > @@ -234,6 +236,41 @@ static struct clk_hw *qcom_cc_clk_hw_get(struct of_phandle_args *clkspec,
> > return cc->rclks[idx] ? &cc->rclks[idx]->hw : NULL;
> > }
> >
> > +#if IS_ENABLED(CONFIG_INTERCONNECT_CLK)
> > +static int qcom_cc_icc_register(struct device *dev,
> > + const struct qcom_cc_desc *desc)
> > +{
> > + struct icc_clk_data *icd;
> > + int i;
> > +
> > + 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], "qcom");
>
> Make the con_id "icc" instead please, so we know the consumer is
> icc_clk.
Ok.
> Even better would be for the icc_clk device itself to be the
> one requesting with devm_clk_hw_get_clk() so that we associate the clk
> handle with the consumer device. It would also help us make it so that
> drivers defer probe until their clk isn't an orphan.
Not sure if I understand the comments correctly.
In one of the previous patches, had
icd[i].clk = clks[noc_clks[i]]->hw.clk;
This was said to be error prone since the clock would not be
ref counted. Hence used devm_clk_hw_get_clk before doing
icc_clk_register.
Now, are you suggesting to use the direct clock pointer
and do a devm_clk_hw_get_clk from the consumer driver?
This will take care of the refcounting. However, we will
have to add these clock entries to the consumer DT node.
Is this ok?
> > + if (IS_ERR(icd[i].clk))
> > + return dev_err_probe(dev, PTR_ERR(icd[i].clk),
> > + "get clock failed (%ld)\n",
> > + PTR_ERR(icd[i].clk));
> > +
> > + icd[i].name = clk_hw_get_name(desc->icc_hws[i]);
> > + }
> > +
> > + return PTR_ERR_OR_ZERO(devm_icc_clk_register(dev, desc->first_id,
> > + desc->num_icc_hws, icd));
> > +}
> > +#else
> > +static int qcom_cc_icc_register(struct device *dev,
> > + const struct qcom_cc_desc *desc)
> > +{
> > + return 0;
> > +}
>
> Instead of this please have an
>
> if (!IS_ENABLED(CONFIG_INTERCONNECT_CLK))
> return 0;
Ok.
> > +#endif
> > +
> > int qcom_cc_really_probe(struct platform_device *pdev,
> > const struct qcom_cc_desc *desc, struct regmap *regmap)
> > {
> > @@ -303,7 +340,7 @@ int qcom_cc_really_probe(struct platform_device *pdev,
> > 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 9c8f7b798d9f..d8ac26d83f3c 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 first_id;
>
> 'first_id' is gross.
will change it to 'icc_id'.
Thanks
Varada
On Thu, Mar 28, 2024 at 02:51:09PM -0700, Stephen Boyd wrote:
> Quoting Varadarajan Narayanan (2024-03-28 00:59:35)
> > 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
> > @@ -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 */
>
> How is this supposed to stay unique?
The icc-clk driver expects some number that wouldn't interfere
with the existing nodes. So just used the SoC id itself.
> I don't understand icc_node_create() API quite honestly. Why
> can't icc_clk_register() maintain some ida of allocated
> numbers? Or is there some global number space that we can
> "reserve" from? I'm quite amazed this is how things are
> connected in interconnect framework.
>
> > +
> > +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.
Thanks
Varada
> > +
> > +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,
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?
It's a pity we see here only parts of DTS, instead of full interconnect
usage.
Best regards,
Krzysztof
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/[email protected]/
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
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/[email protected]/
>
> 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
On 28/03/2024 08:59, 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.
>
> Reviewed-by: Krzysztof Kozlowski <[email protected]>
Please remove my Reviewed tag.
It seems you do not use this binding header: neither in your DTS and nor
in the driver.
Consider the patch NAK-ed and send new version which removes unused header.
Please provide link to upstreamed DTS using this, so we can validate
your usage. Otherwise it looks just wrong and you try to upstream
something which will not pass dtbs checks on the first day, for example.
NAK
Best regards,
Krzysztof
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/[email protected]/
> >
> > 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
>
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/[email protected]/config)
compiler: powerpc64-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20240401/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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'
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Quoting Varadarajan Narayanan (2024-03-29 03:48:24)
> On Thu, Mar 28, 2024 at 02:54:52PM -0700, Stephen Boyd wrote:
> > Quoting Varadarajan Narayanan (2024-03-28 00:59:34)
> > > diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> > > index 75f09e6e057e..9fa271812373 100644
> > > --- a/drivers/clk/qcom/common.c
> > > +++ b/drivers/clk/qcom/common.c
> > > @@ -234,6 +236,41 @@ static struct clk_hw *qcom_cc_clk_hw_get(struct of_phandle_args *clkspec,
> > > return cc->rclks[idx] ? &cc->rclks[idx]->hw : NULL;
> > > }
> > >
> > > +#if IS_ENABLED(CONFIG_INTERCONNECT_CLK)
> > > +static int qcom_cc_icc_register(struct device *dev,
> > > + const struct qcom_cc_desc *desc)
> > > +{
> > > + struct icc_clk_data *icd;
> > > + int i;
> > > +
> > > + 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], "qcom");
> >
> > Make the con_id "icc" instead please, so we know the consumer is
> > icc_clk.
>
> Ok.
>
> > Even better would be for the icc_clk device itself to be the
> > one requesting with devm_clk_hw_get_clk() so that we associate the clk
> > handle with the consumer device. It would also help us make it so that
> > drivers defer probe until their clk isn't an orphan.
>
> Not sure if I understand the comments correctly.
>
> In one of the previous patches, had
> icd[i].clk = clks[noc_clks[i]]->hw.clk;
>
> This was said to be error prone since the clock would not be
> ref counted. Hence used devm_clk_hw_get_clk before doing
> icc_clk_register.
>
> Now, are you suggesting to use the direct clock pointer
> and do a devm_clk_hw_get_clk from the consumer driver?
> This will take care of the refcounting. However, we will
> have to add these clock entries to the consumer DT node.
> Is this ok?
Why do they need to be added to the consumer DT node? Why can't the
icc_clk device driver (icc_clk_driver?) use struct clk_hw instead of
struct clk in struct icc_clk_data? The answer cannot be that the icc_clk
driver cannot be changed.
> > > diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
> > > index 9c8f7b798d9f..d8ac26d83f3c 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 first_id;
> >
> > 'first_id' is gross.
>
> will change it to 'icc_id'.
That's not what I meant :) The whole concept of having to pick some
random number is bad. At the least, hide that in the icc_clk driver so
that we don't have to put this in every clk provider that is also an
interconnect provider.