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.
---
v8: Change icc-clk driver to take master and slave ids instead
of auto generating
Remove ICC_xxx defines from dt-bindings header
Define MASTER/SLAVE_xxx macros from 0 .. n
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 (7):
interconnect: icc-clk: Allow user to specify master/slave ids
clk: qcom: cbf-msm8996: Specify master and slave id
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/clk-cbf-8996.c | 7 ++-
drivers/clk/qcom/common.c | 35 ++++++++++-
drivers/clk/qcom/common.h | 16 +++++
drivers/clk/qcom/gcc-ipq9574.c | 31 ++++++++++
drivers/interconnect/icc-clk.c | 24 +++++++-
.../dt-bindings/interconnect/qcom,ipq9574.h | 59 +++++++++++++++++++
include/linux/interconnect-clk.h | 4 ++
10 files changed, 178 insertions(+), 5 deletions(-)
create mode 100644 include/dt-bindings/interconnect/qcom,ipq9574.h
--
2.34.1
Presently, icc-clk driver autogenerates the master and slave ids.
However, devices with multiple nodes on the interconnect could
have other constraints and may not match with the auto generated
node ids. Hence, allow the driver to provide the preferred master
and slave ids.
Signed-off-by: Varadarajan Narayanan <[email protected]>
---
v8: Per review feedback, set master/slave ids explicitly. Dont autogenerate
https://lore.kernel.org/linux-arm-msm/[email protected]/
---
drivers/interconnect/icc-clk.c | 6 +++---
include/linux/interconnect-clk.h | 2 ++
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/interconnect/icc-clk.c b/drivers/interconnect/icc-clk.c
index d787f2ea36d9..2be193fd7d8f 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;
diff --git a/include/linux/interconnect-clk.h b/include/linux/interconnect-clk.h
index 0cd80112bea5..170898faaacb 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;
};
struct icc_provider *icc_clk_register(struct device *dev,
--
2.34.1
The icc-clk driver has been changed to take master and slave id
from the caller instead of auto-generating them. Update
clk-cbf-8996 accordingly.
Signed-off-by: Varadarajan Narayanan <[email protected]>
---
drivers/clk/qcom/clk-cbf-8996.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/qcom/clk-cbf-8996.c b/drivers/clk/qcom/clk-cbf-8996.c
index fe24b4abeab4..a077d4403967 100644
--- a/drivers/clk/qcom/clk-cbf-8996.c
+++ b/drivers/clk/qcom/clk-cbf-8996.c
@@ -237,7 +237,12 @@ static int qcom_msm8996_cbf_icc_register(struct platform_device *pdev, struct cl
struct device *dev = &pdev->dev;
struct clk *clk = devm_clk_hw_get_clk(dev, cbf_hw, "cbf");
const struct icc_clk_data data[] = {
- { .clk = clk, .name = "cbf", },
+ {
+ .clk = clk,
+ .name = "cbf",
+ .master_id = MASTER_CBF_M4M,
+ .slave_id = SLAVE_CBF_M4M,
+ },
};
struct icc_provider *provider;
--
2.34.1
Wrap icc_clk_register to create devm_icc_clk_register to be
able to release the resources properly.
Reviewed-by: Dmitry Baryshkov <[email protected]>
Signed-off-by: Varadarajan Narayanan <[email protected]>
---
v8: Added Reviewed-by: Dmitry Baryshkov
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 2be193fd7d8f..f788db15cd76 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 170898faaacb..9bcee3e9c56c 100644
--- a/include/linux/interconnect-clk.h
+++ b/include/linux/interconnect-clk.h
@@ -19,6 +19,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
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 <[email protected]>
---
v8:
Remove ICC_xxx macros
Fix macro defines to be consistent with other bindings
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 | 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..42019335c7dd
--- /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 0
+#define SLAVE_ANOC_PCIE0 1
+#define MASTER_SNOC_PCIE0 2
+#define SLAVE_SNOC_PCIE0 3
+#define MASTER_ANOC_PCIE1 4
+#define SLAVE_ANOC_PCIE1 5
+#define MASTER_SNOC_PCIE1 6
+#define SLAVE_SNOC_PCIE1 7
+#define MASTER_ANOC_PCIE2 8
+#define SLAVE_ANOC_PCIE2 9
+#define MASTER_SNOC_PCIE2 10
+#define SLAVE_SNOC_PCIE2 11
+#define MASTER_ANOC_PCIE3 12
+#define SLAVE_ANOC_PCIE3 13
+#define MASTER_SNOC_PCIE3 14
+#define SLAVE_SNOC_PCIE3 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_0 22
+#define SLAVE_NSSNOC_SNOC_0 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_NSSNOC_PPE 0
+#define SLAVE_NSSNOC_PPE 1
+#define MASTER_NSSNOC_PPE_CFG 2
+#define SLAVE_NSSNOC_PPE_CFG 3
+#define MASTER_NSSNOC_NSS_CSR 4
+#define SLAVE_NSSNOC_NSS_CSR 5
+#define MASTER_NSSNOC_IMEM_QSB 6
+#define SLAVE_NSSNOC_IMEM_QSB 7
+#define MASTER_NSSNOC_IMEM_AHB 8
+#define SLAVE_NSSNOC_IMEM_AHB 9
+
+#endif /* INTERCONNECT_QCOM_IPQ9574_H */
--
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]>
---
v8: Explicitly set master and slave ids
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 | 35 ++++++++++++++++++++++++++++++++++-
drivers/clk/qcom/common.h | 16 ++++++++++++++++
2 files changed, 50 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
index 75f09e6e057e..a6410b1828ca 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>
@@ -234,6 +235,38 @@ 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;
+ struct clk_hw *hws;
+ 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].master_id = desc->icc_hws[i].master_id;
+ icd[i].slave_id = desc->icc_hws[i].slave_id;
+ hws = &desc->clks[desc->icc_hws[i].clk_id]->hw;
+ icd[i].clk = devm_clk_hw_get_clk(dev, hws, "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(hws);
+ }
+
+ 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)
{
@@ -303,7 +336,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..f6b25df1ca17 100644
--- a/drivers/clk/qcom/common.h
+++ b/drivers/clk/qcom/common.h
@@ -19,6 +19,19 @@ struct clk_hw;
#define PLL_VOTE_FSM_ENA BIT(20)
#define PLL_VOTE_FSM_RESET BIT(21)
+struct qcom_icc_hws_data {
+ int master_id;
+ int slave_id;
+ int clk_id;
+};
+
+#define HWS_DATA(_b, _c) \
+{ \
+ .master_id = MASTER_##_b, \
+ .slave_id = SLAVE_##_b, \
+ .clk_id = _c, \
+}
+
struct qcom_cc_desc {
const struct regmap_config *config;
struct clk_regmap **clks;
@@ -29,6 +42,9 @@ struct qcom_cc_desc {
size_t num_gdscs;
struct clk_hw **clk_hws;
size_t num_clk_hws;
+ struct qcom_icc_hws_data *icc_hws;
+ size_t num_icc_hws;
+ unsigned int icc_first_node_id;
};
/**
--
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]>
---
v8: Bind clock and interconnect using master and slave ids
Use indices instead of clock pointers
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 | 31 +++++++++++++++++++++++++++++++
2 files changed, 33 insertions(+)
diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index 8ab08e7b5b6c..b65a373f2e6b 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 0a3f846695b8..7983e9ba0f35 100644
--- a/drivers/clk/qcom/gcc-ipq9574.c
+++ b/drivers/clk/qcom/gcc-ipq9574.c
@@ -4,6 +4,7 @@
*/
#include <linux/clk-provider.h>
+#include <linux/interconnect-clk.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/of.h>
@@ -12,6 +13,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 +4303,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 qcom_icc_hws_data icc_ipq9574_hws[] = {
+ HWS_DATA(ANOC_PCIE0, GCC_ANOC_PCIE0_1LANE_M_CLK),
+ HWS_DATA(SNOC_PCIE0, GCC_SNOC_PCIE0_1LANE_S_CLK),
+ HWS_DATA(ANOC_PCIE1, GCC_ANOC_PCIE1_1LANE_M_CLK),
+ HWS_DATA(SNOC_PCIE1, GCC_SNOC_PCIE1_1LANE_S_CLK),
+ HWS_DATA(ANOC_PCIE2, GCC_ANOC_PCIE2_2LANE_M_CLK),
+ HWS_DATA(SNOC_PCIE2, GCC_SNOC_PCIE2_2LANE_S_CLK),
+ HWS_DATA(ANOC_PCIE3, GCC_ANOC_PCIE3_2LANE_M_CLK),
+ HWS_DATA(SNOC_PCIE3, GCC_SNOC_PCIE3_2LANE_S_CLK),
+ HWS_DATA(USB, GCC_SNOC_USB_CLK),
+ HWS_DATA(USB_AXI, GCC_ANOC_USB_AXI_CLK),
+ HWS_DATA(NSSNOC_NSSCC, GCC_NSSNOC_NSSCC_CLK),
+ HWS_DATA(NSSNOC_SNOC_0, GCC_NSSNOC_SNOC_CLK),
+ HWS_DATA(NSSNOC_SNOC_1, GCC_NSSNOC_SNOC_1_CLK),
+ HWS_DATA(NSSNOC_PCNOC_1, GCC_NSSNOC_PCNOC_1_CLK),
+ HWS_DATA(NSSNOC_QOSGEN_REF, GCC_NSSNOC_QOSGEN_REF_CLK),
+ HWS_DATA(NSSNOC_TIMEOUT_REF, GCC_NSSNOC_TIMEOUT_REF_CLK),
+ HWS_DATA(NSSNOC_XO_DCD, GCC_NSSNOC_XO_DCD_CLK),
+ HWS_DATA(NSSNOC_ATB, GCC_NSSNOC_ATB_CLK),
+ HWS_DATA(MEM_NOC_NSSNOC, GCC_MEM_NOC_NSSNOC_CLK),
+ HWS_DATA(NSSNOC_MEMNOC, GCC_NSSNOC_MEMNOC_CLK),
+ HWS_DATA(NSSNOC_MEM_NOC_1, GCC_NSSNOC_MEM_NOC_1_CLK),
+};
+
static const struct of_device_id gcc_ipq9574_match_table[] = {
{ .compatible = "qcom,ipq9574-gcc" },
{ }
@@ -4323,6 +4351,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
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 <[email protected]>
Signed-off-by: Varadarajan Narayanan <[email protected]>
---
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
On Wed, 17 Apr 2024 at 13:56, Varadarajan Narayanan
<[email protected]> wrote:
>
> The icc-clk driver has been changed to take master and slave id
> from the caller instead of auto-generating them. Update
> clk-cbf-8996 accordingly.
This should be squashed into the previous patch. Otherwise the driver
is broken between two commits.
>
> Signed-off-by: Varadarajan Narayanan <[email protected]>
> ---
> drivers/clk/qcom/clk-cbf-8996.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/qcom/clk-cbf-8996.c b/drivers/clk/qcom/clk-cbf-8996.c
> index fe24b4abeab4..a077d4403967 100644
> --- a/drivers/clk/qcom/clk-cbf-8996.c
> +++ b/drivers/clk/qcom/clk-cbf-8996.c
> @@ -237,7 +237,12 @@ static int qcom_msm8996_cbf_icc_register(struct platform_device *pdev, struct cl
> struct device *dev = &pdev->dev;
> struct clk *clk = devm_clk_hw_get_clk(dev, cbf_hw, "cbf");
> const struct icc_clk_data data[] = {
> - { .clk = clk, .name = "cbf", },
> + {
> + .clk = clk,
> + .name = "cbf",
> + .master_id = MASTER_CBF_M4M,
> + .slave_id = SLAVE_CBF_M4M,
> + },
> };
> struct icc_provider *provider;
>
> --
> 2.34.1
>
--
With best wishes
Dmitry
On Wed, 17 Apr 2024 at 13:57, Varadarajan Narayanan
<[email protected]> 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 <[email protected]>
> ---
> v8: Explicitly set master and slave ids
> 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 | 35 ++++++++++++++++++++++++++++++++++-
> drivers/clk/qcom/common.h | 16 ++++++++++++++++
> 2 files changed, 50 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> index 75f09e6e057e..a6410b1828ca 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>
>
> @@ -234,6 +235,38 @@ 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;
> + struct clk_hw *hws;
> + 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].master_id = desc->icc_hws[i].master_id;
> + icd[i].slave_id = desc->icc_hws[i].slave_id;
> + hws = &desc->clks[desc->icc_hws[i].clk_id]->hw;
I think I keep on repeating this again and again. Instead of passing
indices please pass clk_hw pointers.
> + icd[i].clk = devm_clk_hw_get_clk(dev, hws, "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(hws);
> + }
> +
> + 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)
> {
> @@ -303,7 +336,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..f6b25df1ca17 100644
> --- a/drivers/clk/qcom/common.h
> +++ b/drivers/clk/qcom/common.h
> @@ -19,6 +19,19 @@ struct clk_hw;
> #define PLL_VOTE_FSM_ENA BIT(20)
> #define PLL_VOTE_FSM_RESET BIT(21)
>
> +struct qcom_icc_hws_data {
> + int master_id;
> + int slave_id;
> + int clk_id;
> +};
> +
> +#define HWS_DATA(_b, _c) \
> +{ \
> + .master_id = MASTER_##_b, \
> + .slave_id = SLAVE_##_b, \
> + .clk_id = _c, \
> +}
This shouldn't be a part of this commit. It is not used in it.
> +
> struct qcom_cc_desc {
> const struct regmap_config *config;
> struct clk_regmap **clks;
> @@ -29,6 +42,9 @@ struct qcom_cc_desc {
> size_t num_gdscs;
> struct clk_hw **clk_hws;
> size_t num_clk_hws;
> + struct qcom_icc_hws_data *icc_hws;
> + size_t num_icc_hws;
> + unsigned int icc_first_node_id;
> };
>
> /**
> --
> 2.34.1
>
--
With best wishes
Dmitry
On Wed, 17 Apr 2024 at 13:57, Varadarajan Narayanan
<[email protected]> wrote:
>
> 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]>
> ---
> v8: Bind clock and interconnect using master and slave ids
> Use indices instead of clock pointers
> 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 | 31 +++++++++++++++++++++++++++++++
> 2 files changed, 33 insertions(+)
>
> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index 8ab08e7b5b6c..b65a373f2e6b 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 0a3f846695b8..7983e9ba0f35 100644
> --- a/drivers/clk/qcom/gcc-ipq9574.c
> +++ b/drivers/clk/qcom/gcc-ipq9574.c
> @@ -4,6 +4,7 @@
> */
>
> #include <linux/clk-provider.h>
> +#include <linux/interconnect-clk.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/of.h>
> @@ -12,6 +13,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 +4303,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 qcom_icc_hws_data icc_ipq9574_hws[] = {
> + HWS_DATA(ANOC_PCIE0, GCC_ANOC_PCIE0_1LANE_M_CLK),
Have you seen other parts of the qcom framework using macros to wrap
around structure initialisation? I don't think so. Please follow the
suit and inline the macro here.
> + HWS_DATA(SNOC_PCIE0, GCC_SNOC_PCIE0_1LANE_S_CLK),
> + HWS_DATA(ANOC_PCIE1, GCC_ANOC_PCIE1_1LANE_M_CLK),
> + HWS_DATA(SNOC_PCIE1, GCC_SNOC_PCIE1_1LANE_S_CLK),
> + HWS_DATA(ANOC_PCIE2, GCC_ANOC_PCIE2_2LANE_M_CLK),
> + HWS_DATA(SNOC_PCIE2, GCC_SNOC_PCIE2_2LANE_S_CLK),
> + HWS_DATA(ANOC_PCIE3, GCC_ANOC_PCIE3_2LANE_M_CLK),
> + HWS_DATA(SNOC_PCIE3, GCC_SNOC_PCIE3_2LANE_S_CLK),
> + HWS_DATA(USB, GCC_SNOC_USB_CLK),
> + HWS_DATA(USB_AXI, GCC_ANOC_USB_AXI_CLK),
> + HWS_DATA(NSSNOC_NSSCC, GCC_NSSNOC_NSSCC_CLK),
> + HWS_DATA(NSSNOC_SNOC_0, GCC_NSSNOC_SNOC_CLK),
> + HWS_DATA(NSSNOC_SNOC_1, GCC_NSSNOC_SNOC_1_CLK),
> + HWS_DATA(NSSNOC_PCNOC_1, GCC_NSSNOC_PCNOC_1_CLK),
> + HWS_DATA(NSSNOC_QOSGEN_REF, GCC_NSSNOC_QOSGEN_REF_CLK),
> + HWS_DATA(NSSNOC_TIMEOUT_REF, GCC_NSSNOC_TIMEOUT_REF_CLK),
> + HWS_DATA(NSSNOC_XO_DCD, GCC_NSSNOC_XO_DCD_CLK),
> + HWS_DATA(NSSNOC_ATB, GCC_NSSNOC_ATB_CLK),
> + HWS_DATA(MEM_NOC_NSSNOC, GCC_MEM_NOC_NSSNOC_CLK),
> + HWS_DATA(NSSNOC_MEMNOC, GCC_NSSNOC_MEMNOC_CLK),
> + HWS_DATA(NSSNOC_MEM_NOC_1, GCC_NSSNOC_MEM_NOC_1_CLK),
> +};
> +
> static const struct of_device_id gcc_ipq9574_match_table[] = {
> { .compatible = "qcom,ipq9574-gcc" },
> { }
> @@ -4323,6 +4351,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
>
--
With best wishes
Dmitry
On 17/04/2024 12:56, 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 <[email protected]>
> ---
> v8:
> Remove ICC_xxx macros
> Fix macro defines to be consistent with other bindings
> 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 | 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..42019335c7dd
> --- /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 0
> +#define SLAVE_ANOC_PCIE0 1
I still do not see any usage of it. At least symbol cannot be resolved.
I assume you use the value, otherwise it would mean our entire feedback
was ignored, but then why this cannot be searchable?
Again, open existing drivers and look how it is there. Not being able to
find the constant is not good.
Best regards,
Krzysztof
On Wed, Apr 17, 2024 at 02:29:03PM +0300, Dmitry Baryshkov wrote:
> On Wed, 17 Apr 2024 at 13:57, Varadarajan Narayanan
> <[email protected]> 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 <[email protected]>
> > ---
> > v8: Explicitly set master and slave ids
> > 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 | 35 ++++++++++++++++++++++++++++++++++-
> > drivers/clk/qcom/common.h | 16 ++++++++++++++++
> > 2 files changed, 50 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> > index 75f09e6e057e..a6410b1828ca 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>
> >
> > @@ -234,6 +235,38 @@ 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;
> > + struct clk_hw *hws;
> > + 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].master_id = desc->icc_hws[i].master_id;
> > + icd[i].slave_id = desc->icc_hws[i].slave_id;
> > + hws = &desc->clks[desc->icc_hws[i].clk_id]->hw;
>
> I think I keep on repeating this again and again. Instead of passing
> indices please pass clk_hw pointers.
I'm sorry. Based on the following feedback for v7 from you I changed it to
use indices instead of clk_hw pointers. Am I missing something?
https://lore.kernel.org/linux-arm-msm/CAA8EJpohAe-aW1QqVkE9NBRU0DpZR7UiwdUKk6rS_YFAhenZZA@mail.gmail.com/
<quote>
> + 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?
</quote>
Please confirm.
> > + icd[i].clk = devm_clk_hw_get_clk(dev, hws, "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(hws);
> > + }
> > +
> > + 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)
> > {
> > @@ -303,7 +336,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..f6b25df1ca17 100644
> > --- a/drivers/clk/qcom/common.h
> > +++ b/drivers/clk/qcom/common.h
> > @@ -19,6 +19,19 @@ struct clk_hw;
> > #define PLL_VOTE_FSM_ENA BIT(20)
> > #define PLL_VOTE_FSM_RESET BIT(21)
> >
> > +struct qcom_icc_hws_data {
> > + int master_id;
> > + int slave_id;
> > + int clk_id;
> > +};
> > +
> > +#define HWS_DATA(_b, _c) \
> > +{ \
> > + .master_id = MASTER_##_b, \
> > + .slave_id = SLAVE_##_b, \
> > + .clk_id = _c, \
> > +}
>
> This shouldn't be a part of this commit. It is not used in it.
Ok.
Thanks
Varada
> > +
> > struct qcom_cc_desc {
> > const struct regmap_config *config;
> > struct clk_regmap **clks;
> > @@ -29,6 +42,9 @@ struct qcom_cc_desc {
> > size_t num_gdscs;
> > struct clk_hw **clk_hws;
> > size_t num_clk_hws;
> > + struct qcom_icc_hws_data *icc_hws;
> > + size_t num_icc_hws;
> > + unsigned int icc_first_node_id;
> > };
> >
> > /**
> > --
> > 2.34.1
> >
>
>
> --
> With best wishes
> Dmitry
On Wed, Apr 17, 2024 at 02:26:36PM +0300, Dmitry Baryshkov wrote:
> On Wed, 17 Apr 2024 at 13:56, Varadarajan Narayanan
> <[email protected]> wrote:
> >
> > The icc-clk driver has been changed to take master and slave id
> > from the caller instead of auto-generating them. Update
> > clk-cbf-8996 accordingly.
>
> This should be squashed into the previous patch. Otherwise the driver
> is broken between two commits.
Ok. Will squash and post a new one.
Thanks
Varada
>
> >
> > Signed-off-by: Varadarajan Narayanan <[email protected]>
> > ---
> > drivers/clk/qcom/clk-cbf-8996.c | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/clk/qcom/clk-cbf-8996.c b/drivers/clk/qcom/clk-cbf-8996.c
> > index fe24b4abeab4..a077d4403967 100644
> > --- a/drivers/clk/qcom/clk-cbf-8996.c
> > +++ b/drivers/clk/qcom/clk-cbf-8996.c
> > @@ -237,7 +237,12 @@ static int qcom_msm8996_cbf_icc_register(struct platform_device *pdev, struct cl
> > struct device *dev = &pdev->dev;
> > struct clk *clk = devm_clk_hw_get_clk(dev, cbf_hw, "cbf");
> > const struct icc_clk_data data[] = {
> > - { .clk = clk, .name = "cbf", },
> > + {
> > + .clk = clk,
> > + .name = "cbf",
> > + .master_id = MASTER_CBF_M4M,
> > + .slave_id = SLAVE_CBF_M4M,
> > + },
> > };
> > struct icc_provider *provider;
> >
> > --
> > 2.34.1
> >
>
>
> --
> With best wishes
> Dmitry
On Wed, Apr 17, 2024 at 04:07:59PM +0200, Krzysztof Kozlowski wrote:
> On 17/04/2024 12:56, 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 <[email protected]>
> > ---
> > v8:
> > Remove ICC_xxx macros
> > Fix macro defines to be consistent with other bindings
> > 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 | 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..42019335c7dd
> > --- /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 0
> > +#define SLAVE_ANOC_PCIE0 1
>
> I still do not see any usage of it. At least symbol cannot be resolved.
> I assume you use the value, otherwise it would mean our entire feedback
> was ignored, but then why this cannot be searchable?
>
> Again, open existing drivers and look how it is there. Not being able to
> find the constant is not good.
It is used in the 6th patch in drivers/clk/qcom/gcc-ipq9574.c via the
HWS_DATA macro. Will remove the macro and use it explicitly (as suggested
by Dmitry also).
Thanks
Varada
On Wed, Apr 17, 2024 at 02:30:02PM +0300, Dmitry Baryshkov wrote:
> On Wed, 17 Apr 2024 at 13:57, Varadarajan Narayanan
> <[email protected]> wrote:
> >
> > 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]>
> > ---
> > v8: Bind clock and interconnect using master and slave ids
> > Use indices instead of clock pointers
> > 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 | 31 +++++++++++++++++++++++++++++++
> > 2 files changed, 33 insertions(+)
> >
> > diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> > index 8ab08e7b5b6c..b65a373f2e6b 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 0a3f846695b8..7983e9ba0f35 100644
> > --- a/drivers/clk/qcom/gcc-ipq9574.c
> > +++ b/drivers/clk/qcom/gcc-ipq9574.c
> > @@ -4,6 +4,7 @@
> > */
> >
> > #include <linux/clk-provider.h>
> > +#include <linux/interconnect-clk.h>
> > #include <linux/kernel.h>
> > #include <linux/module.h>
> > #include <linux/of.h>
> > @@ -12,6 +13,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 +4303,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 qcom_icc_hws_data icc_ipq9574_hws[] = {
> > + HWS_DATA(ANOC_PCIE0, GCC_ANOC_PCIE0_1LANE_M_CLK),
>
> Have you seen other parts of the qcom framework using macros to wrap
> around structure initialisation? I don't think so. Please follow the
> suit and inline the macro here.
Ok, will change and post a new version.
Thanks
Varada
> > + HWS_DATA(SNOC_PCIE0, GCC_SNOC_PCIE0_1LANE_S_CLK),
> > + HWS_DATA(ANOC_PCIE1, GCC_ANOC_PCIE1_1LANE_M_CLK),
> > + HWS_DATA(SNOC_PCIE1, GCC_SNOC_PCIE1_1LANE_S_CLK),
> > + HWS_DATA(ANOC_PCIE2, GCC_ANOC_PCIE2_2LANE_M_CLK),
> > + HWS_DATA(SNOC_PCIE2, GCC_SNOC_PCIE2_2LANE_S_CLK),
> > + HWS_DATA(ANOC_PCIE3, GCC_ANOC_PCIE3_2LANE_M_CLK),
> > + HWS_DATA(SNOC_PCIE3, GCC_SNOC_PCIE3_2LANE_S_CLK),
> > + HWS_DATA(USB, GCC_SNOC_USB_CLK),
> > + HWS_DATA(USB_AXI, GCC_ANOC_USB_AXI_CLK),
> > + HWS_DATA(NSSNOC_NSSCC, GCC_NSSNOC_NSSCC_CLK),
> > + HWS_DATA(NSSNOC_SNOC_0, GCC_NSSNOC_SNOC_CLK),
> > + HWS_DATA(NSSNOC_SNOC_1, GCC_NSSNOC_SNOC_1_CLK),
> > + HWS_DATA(NSSNOC_PCNOC_1, GCC_NSSNOC_PCNOC_1_CLK),
> > + HWS_DATA(NSSNOC_QOSGEN_REF, GCC_NSSNOC_QOSGEN_REF_CLK),
> > + HWS_DATA(NSSNOC_TIMEOUT_REF, GCC_NSSNOC_TIMEOUT_REF_CLK),
> > + HWS_DATA(NSSNOC_XO_DCD, GCC_NSSNOC_XO_DCD_CLK),
> > + HWS_DATA(NSSNOC_ATB, GCC_NSSNOC_ATB_CLK),
> > + HWS_DATA(MEM_NOC_NSSNOC, GCC_MEM_NOC_NSSNOC_CLK),
> > + HWS_DATA(NSSNOC_MEMNOC, GCC_NSSNOC_MEMNOC_CLK),
> > + HWS_DATA(NSSNOC_MEM_NOC_1, GCC_NSSNOC_MEM_NOC_1_CLK),
> > +};
> > +
> > static const struct of_device_id gcc_ipq9574_match_table[] = {
> > { .compatible = "qcom,ipq9574-gcc" },
> > { }
> > @@ -4323,6 +4351,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
> >
>
>
> --
> With best wishes
> Dmitry
On Wed, 17 Apr 2024 at 19:34, Varadarajan Narayanan
<[email protected]> wrote:
>
> On Wed, Apr 17, 2024 at 02:29:03PM +0300, Dmitry Baryshkov wrote:
> > On Wed, 17 Apr 2024 at 13:57, Varadarajan Narayanan
> > <[email protected]> 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 <[email protected]>
> > > ---
> > > v8: Explicitly set master and slave ids
> > > 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 | 35 ++++++++++++++++++++++++++++++++++-
> > > drivers/clk/qcom/common.h | 16 ++++++++++++++++
> > > 2 files changed, 50 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> > > index 75f09e6e057e..a6410b1828ca 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>
> > >
> > > @@ -234,6 +235,38 @@ 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;
> > > + struct clk_hw *hws;
> > > + 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].master_id = desc->icc_hws[i].master_id;
> > > + icd[i].slave_id = desc->icc_hws[i].slave_id;
> > > + hws = &desc->clks[desc->icc_hws[i].clk_id]->hw;
> >
> > I think I keep on repeating this again and again. Instead of passing
> > indices please pass clk_hw pointers.
>
> I'm sorry. Based on the following feedback for v7 from you I changed it to
> use indices instead of clk_hw pointers. Am I missing something?
>
> https://lore.kernel.org/linux-arm-msm/CAA8EJpohAe-aW1QqVkE9NBRU0DpZR7UiwdUKk6rS_YFAhenZZA@mail.gmail.com/
> <quote>
> > + 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?
> </quote>
>
> Please confirm.
Ok, it's fine then.
>
> > > + icd[i].clk = devm_clk_hw_get_clk(dev, hws, "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(hws);
> > > + }
> > > +
> > > + 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)
> > > {
> > > @@ -303,7 +336,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..f6b25df1ca17 100644
> > > --- a/drivers/clk/qcom/common.h
> > > +++ b/drivers/clk/qcom/common.h
> > > @@ -19,6 +19,19 @@ struct clk_hw;
> > > #define PLL_VOTE_FSM_ENA BIT(20)
> > > #define PLL_VOTE_FSM_RESET BIT(21)
> > >
> > > +struct qcom_icc_hws_data {
> > > + int master_id;
> > > + int slave_id;
> > > + int clk_id;
> > > +};
> > > +
> > > +#define HWS_DATA(_b, _c) \
> > > +{ \
> > > + .master_id = MASTER_##_b, \
> > > + .slave_id = SLAVE_##_b, \
> > > + .clk_id = _c, \
> > > +}
> >
> > This shouldn't be a part of this commit. It is not used in it.
>
> Ok.
>
> Thanks
> Varada
>
> > > +
> > > struct qcom_cc_desc {
> > > const struct regmap_config *config;
> > > struct clk_regmap **clks;
> > > @@ -29,6 +42,9 @@ struct qcom_cc_desc {
> > > size_t num_gdscs;
> > > struct clk_hw **clk_hws;
> > > size_t num_clk_hws;
> > > + struct qcom_icc_hws_data *icc_hws;
> > > + size_t num_icc_hws;
> > > + unsigned int icc_first_node_id;
> > > };
> > >
> > > /**
> > > --
> > > 2.34.1
> > >
> >
> >
> > --
> > With best wishes
> > Dmitry
--
With best wishes
Dmitry