2024-04-02 10:36:15

by Varadarajan Narayanan

[permalink] [raw]
Subject: [PATCH v6 0/6] Add interconnect driver for IPQ9574 SoC

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.

---
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 (6):
dt-bindings: interconnect: Add Qualcomm IPQ9574 support
interconnect: icc-clk: Remove tristate from INTERCONNECT_CLK
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 | 38 ++++++++++++++++++-
drivers/clk/qcom/common.h | 3 ++
drivers/clk/qcom/gcc-ipq9574.c | 30 +++++++++++++++
drivers/interconnect/Kconfig | 1 -
drivers/interconnect/icc-clk.c | 29 ++++++++++++++
.../dt-bindings/interconnect/qcom,ipq9574.h | 36 ++++++++++++++++++
include/linux/interconnect-clk.h | 4 ++
10 files changed, 146 insertions(+), 2 deletions(-)
create mode 100644 include/dt-bindings/interconnect/qcom,ipq9574.h

--
2.34.1



2024-04-02 10:36:37

by Varadarajan Narayanan

[permalink] [raw]
Subject: [PATCH v6 1/6] dt-bindings: interconnect: Add Qualcomm IPQ9574 support

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]>
---
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 | 36 +++++++++++++++++++
2 files changed, 39 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..988124c39810
--- /dev/null
+++ b/include/dt-bindings/interconnect/qcom,ipq9574.h
@@ -0,0 +1,36 @@
+/* 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(x) ((ICC_ ## x) * 2)
+#define SLAVE(x) (MASTER(x) + 1)
+
+#endif /* INTERCONNECT_QCOM_IPQ9574_H */
--
2.34.1


2024-04-02 10:36:54

by Varadarajan Narayanan

[permalink] [raw]
Subject: [PATCH v6 2/6] interconnect: icc-clk: Remove tristate from INTERCONNECT_CLK

drivers/clk/qcom/common.c uses devm_icc_clk_register under
IS_ENABLED(CONFIG_INTERCONNECT_CLK). However, in kernel bot
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'

Hence, ensure INTERCONNECT_CLK is not selected as a module.

Reported-by: kernel test robot <[email protected]>
Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
Fixes: 0ac2a08f42ce ("interconnect: add clk-based icc provider support")
Signed-off-by: Varadarajan Narayanan <[email protected]>
---
drivers/interconnect/Kconfig | 1 -
1 file changed, 1 deletion(-)

diff --git a/drivers/interconnect/Kconfig b/drivers/interconnect/Kconfig
index 5faa8d2aecff..f44be5469382 100644
--- a/drivers/interconnect/Kconfig
+++ b/drivers/interconnect/Kconfig
@@ -16,7 +16,6 @@ source "drivers/interconnect/qcom/Kconfig"
source "drivers/interconnect/samsung/Kconfig"

config INTERCONNECT_CLK
- tristate
depends on COMMON_CLK
help
Support for wrapping clocks into the interconnect nodes.
--
2.34.1


2024-04-02 10:37:11

by Varadarajan Narayanan

[permalink] [raw]
Subject: [PATCH v6 3/6] interconnect: icc-clk: Add devm_icc_clk_register

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


2024-04-02 10:37:53

by Varadarajan Narayanan

[permalink] [raw]
Subject: [PATCH v6 4/6] clk: qcom: common: Add interconnect clocks support

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]>
---
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 | 38 +++++++++++++++++++++++++++++++++++++-
drivers/clk/qcom/common.h | 3 +++
2 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
index 75f09e6e057e..d5c008048994 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,41 @@ 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++) {
+ /*
+ * get_clk will be done by the peripheral device using this
+ * clock with devm_clk_hw_get_clk() so that we can 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.
+ */
+ icd[i].clk = desc->icc_hws[i]->clk;
+ 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 PTR_ERR_OR_ZERO(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 +339,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..9058ffd46260 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


2024-04-02 10:37:59

by Varadarajan Narayanan

[permalink] [raw]
Subject: [PATCH v6 5/6] clk: qcom: ipq9574: Use icc-clk for enabling NoC related clocks

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]>
---
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 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..646cafcf8f0b 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,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" },
{ }
@@ -4323,6 +4350,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


2024-04-02 10:38:24

by Varadarajan Narayanan

[permalink] [raw]
Subject: [PATCH v6 6/6] arm64: dts: qcom: ipq9574: Add icc provider ability to gcc

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]>
---
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


2024-04-02 10:41:17

by Dmitry Baryshkov

[permalink] [raw]
Subject: Re: [PATCH v6 2/6] interconnect: icc-clk: Remove tristate from INTERCONNECT_CLK

On Tue, 2 Apr 2024 at 13:34, Varadarajan Narayanan
<[email protected]> wrote:
>
> drivers/clk/qcom/common.c uses devm_icc_clk_register under
> IS_ENABLED(CONFIG_INTERCONNECT_CLK). However, in kernel bot
> 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'
>
> Hence, ensure INTERCONNECT_CLK is not selected as a module.

NAK. Please use `depends on INTERCONNECT_CLK || !INTERCONNECT_CLK` in
your Kconfig dependencies.


>
> Reported-by: kernel test robot <[email protected]>
> Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> Fixes: 0ac2a08f42ce ("interconnect: add clk-based icc provider support")
> Signed-off-by: Varadarajan Narayanan <[email protected]>
> ---
> drivers/interconnect/Kconfig | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/interconnect/Kconfig b/drivers/interconnect/Kconfig
> index 5faa8d2aecff..f44be5469382 100644
> --- a/drivers/interconnect/Kconfig
> +++ b/drivers/interconnect/Kconfig
> @@ -16,7 +16,6 @@ source "drivers/interconnect/qcom/Kconfig"
> source "drivers/interconnect/samsung/Kconfig"
>
> config INTERCONNECT_CLK
> - tristate
> depends on COMMON_CLK
> help
> Support for wrapping clocks into the interconnect nodes.
> --
> 2.34.1
>


--
With best wishes

Dmitry

2024-04-02 10:41:39

by Dmitry Baryshkov

[permalink] [raw]
Subject: Re: [PATCH v6 3/6] interconnect: icc-clk: Add devm_icc_clk_register

On Tue, 2 Apr 2024 at 13:34, Varadarajan Narayanan
<[email protected]> wrote:
>
> 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(+)

Reviewed-by: Dmitry Baryshkov <[email protected]>

--
With best wishes
Dmitry

2024-04-02 10:48:48

by Dmitry Baryshkov

[permalink] [raw]
Subject: Re: [PATCH v6 4/6] clk: qcom: common: Add interconnect clocks support

On Tue, 2 Apr 2024 at 13:34, 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]>
> ---
> 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 | 38 +++++++++++++++++++++++++++++++++++++-
> drivers/clk/qcom/common.h | 3 +++
> 2 files changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> index 75f09e6e057e..d5c008048994 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,41 @@ 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++) {
> + /*
> + * get_clk will be done by the peripheral device using this
> + * clock with devm_clk_hw_get_clk() so that we can 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.

How the clock instance returned to the peripheral driver is supposed
to correspond to the clock instance used by the icc-clk?

> + */
> + icd[i].clk = desc->icc_hws[i]->clk;

You again are abusing clk_hw->clk. Please don't do that.

> + 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 PTR_ERR_OR_ZERO(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 +339,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..9058ffd46260 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
>


--
With best wishes

Dmitry

2024-04-02 10:49:57

by Dmitry Baryshkov

[permalink] [raw]
Subject: Re: [PATCH v6 6/6] arm64: dts: qcom: ipq9574: Add icc provider ability to gcc

On Tue, 2 Apr 2024 at 13:34, Varadarajan Narayanan
<[email protected]> wrote:
>
> 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]>
> ---
> arch/arm64/boot/dts/qcom/ipq9574.dtsi | 2 ++
> 1 file changed, 2 insertions(+)

Reviewed-by: Dmitry Baryshkov <[email protected]>


--
With best wishes
Dmitry

2024-04-02 11:03:32

by Dmitry Baryshkov

[permalink] [raw]
Subject: Re: [PATCH v6 3/6] interconnect: icc-clk: Add devm_icc_clk_register

On Tue, 2 Apr 2024 at 13:40, Dmitry Baryshkov
<[email protected]> wrote:
>
> On Tue, 2 Apr 2024 at 13:34, Varadarajan Narayanan
> <[email protected]> wrote:
> >
> > 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(+)
>
> Reviewed-by: Dmitry Baryshkov <[email protected]>

Wait. Actually,

Unreviewed-by: me

Please return int from devm_icc_clk_register instead of returning the pointer.

--
With best wishes
Dmitry

2024-04-02 11:08:13

by Varadarajan Narayanan

[permalink] [raw]
Subject: Re: [PATCH v6 4/6] clk: qcom: common: Add interconnect clocks support

On Tue, Apr 02, 2024 at 01:48:14PM +0300, Dmitry Baryshkov wrote:
> On Tue, 2 Apr 2024 at 13:34, 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]>
> > ---
> > 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 | 38 +++++++++++++++++++++++++++++++++++++-
> > drivers/clk/qcom/common.h | 3 +++
> > 2 files changed, 40 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> > index 75f09e6e057e..d5c008048994 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,41 @@ 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++) {
> > + /*
> > + * get_clk will be done by the peripheral device using this
> > + * clock with devm_clk_hw_get_clk() so that we can 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.
>
> How the clock instance returned to the peripheral driver is supposed
> to correspond to the clock instance used by the icc-clk?
> > + */
> > + icd[i].clk = desc->icc_hws[i]->clk;
>
> You again are abusing clk_hw->clk. Please don't do that.

Ok, will clk_get in both the places.

Thanks
Varada

> > + 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 PTR_ERR_OR_ZERO(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 +339,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..9058ffd46260 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
> >
>
>
> --
> With best wishes
>
> Dmitry

2024-04-02 11:17:41

by Dmitry Baryshkov

[permalink] [raw]
Subject: Re: [PATCH v6 3/6] interconnect: icc-clk: Add devm_icc_clk_register

On Tue, 2 Apr 2024 at 14:02, Varadarajan Narayanan
<[email protected]> wrote:
>
> On Tue, Apr 02, 2024 at 01:48:08PM +0300, Dmitry Baryshkov wrote:
> > On Tue, 2 Apr 2024 at 13:40, Dmitry Baryshkov
> > <[email protected]> wrote:
> > >
> > > On Tue, 2 Apr 2024 at 13:34, Varadarajan Narayanan
> > > <[email protected]> wrote:
> > > >
> > > > 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(+)
> > >
> > > Reviewed-by: Dmitry Baryshkov <[email protected]>
> >
> > Wait. Actually,
> >
> > Unreviewed-by: me
> >
> > Please return int from devm_icc_clk_register instead of returning the pointer.
>
> Wouldn't returning int break the general assumption that
> devm_foo(), returns the same type as foo(). For example
> devm_clk_hw_get_clk and clk_hw_get_clk return struct clk *?

Not always. The only reason to return icc_provider was to make it
possible to destroy it. With devres-managed function you don't have to
do anything.


--
With best wishes
Dmitry

2024-04-02 11:21:25

by Varadarajan Narayanan

[permalink] [raw]
Subject: Re: [PATCH v6 3/6] interconnect: icc-clk: Add devm_icc_clk_register

On Tue, Apr 02, 2024 at 01:48:08PM +0300, Dmitry Baryshkov wrote:
> On Tue, 2 Apr 2024 at 13:40, Dmitry Baryshkov
> <[email protected]> wrote:
> >
> > On Tue, 2 Apr 2024 at 13:34, Varadarajan Narayanan
> > <[email protected]> wrote:
> > >
> > > 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(+)
> >
> > Reviewed-by: Dmitry Baryshkov <[email protected]>
>
> Wait. Actually,
>
> Unreviewed-by: me
>
> Please return int from devm_icc_clk_register instead of returning the pointer.

Wouldn't returning int break the general assumption that
devm_foo(), returns the same type as foo(). For example
devm_clk_hw_get_clk and clk_hw_get_clk return struct clk *?

Thanks
Varada

2024-04-02 11:24:03

by Varadarajan Narayanan

[permalink] [raw]
Subject: Re: [PATCH v6 3/6] interconnect: icc-clk: Add devm_icc_clk_register

On Tue, Apr 02, 2024 at 02:16:56PM +0300, Dmitry Baryshkov wrote:
> On Tue, 2 Apr 2024 at 14:02, Varadarajan Narayanan
> <[email protected]> wrote:
> >
> > On Tue, Apr 02, 2024 at 01:48:08PM +0300, Dmitry Baryshkov wrote:
> > > On Tue, 2 Apr 2024 at 13:40, Dmitry Baryshkov
> > > <[email protected]> wrote:
> > > >
> > > > On Tue, 2 Apr 2024 at 13:34, Varadarajan Narayanan
> > > > <[email protected]> wrote:
> > > > >
> > > > > 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(+)
> > > >
> > > > Reviewed-by: Dmitry Baryshkov <[email protected]>
> > >
> > > Wait. Actually,
> > >
> > > Unreviewed-by: me
> > >
> > > Please return int from devm_icc_clk_register instead of returning the pointer.
> >
> > Wouldn't returning int break the general assumption that
> > devm_foo(), returns the same type as foo(). For example
> > devm_clk_hw_get_clk and clk_hw_get_clk return struct clk *?
>
> Not always. The only reason to return icc_provider was to make it
> possible to destroy it. With devres-managed function you don't have to
> do anything.

Ok. Will change as follows

return prov; -> return PTR_ERR_OR_ZERO(prov);

Thanks
Varada

2024-04-02 12:12:46

by Dmitry Baryshkov

[permalink] [raw]
Subject: Re: [PATCH v6 3/6] interconnect: icc-clk: Add devm_icc_clk_register

On Tue, 2 Apr 2024 at 14:23, Varadarajan Narayanan
<[email protected]> wrote:
>
> On Tue, Apr 02, 2024 at 02:16:56PM +0300, Dmitry Baryshkov wrote:
> > On Tue, 2 Apr 2024 at 14:02, Varadarajan Narayanan
> > <[email protected]> wrote:
> > >
> > > On Tue, Apr 02, 2024 at 01:48:08PM +0300, Dmitry Baryshkov wrote:
> > > > On Tue, 2 Apr 2024 at 13:40, Dmitry Baryshkov
> > > > <[email protected]> wrote:
> > > > >
> > > > > On Tue, 2 Apr 2024 at 13:34, Varadarajan Narayanan
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > 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(+)
> > > > >
> > > > > Reviewed-by: Dmitry Baryshkov <[email protected]>
> > > >
> > > > Wait. Actually,
> > > >
> > > > Unreviewed-by: me
> > > >
> > > > Please return int from devm_icc_clk_register instead of returning the pointer.
> > >
> > > Wouldn't returning int break the general assumption that
> > > devm_foo(), returns the same type as foo(). For example
> > > devm_clk_hw_get_clk and clk_hw_get_clk return struct clk *?
> >
> > Not always. The only reason to return icc_provider was to make it
> > possible to destroy it. With devres-managed function you don't have to
> > do anything.
>
> Ok. Will change as follows
>
> return prov; -> return PTR_ERR_OR_ZERO(prov);
>

I think the code might become simpler if you first allocate the ICC
provider and then just 'return devm_add_action_or_reset(dev,
your_icc_clk_release, provider)'


--
With best wishes
Dmitry

2024-04-02 14:52:00

by Konrad Dybcio

[permalink] [raw]
Subject: Re: [PATCH v6 2/6] interconnect: icc-clk: Remove tristate from INTERCONNECT_CLK

On 2.04.2024 12:39 PM, Dmitry Baryshkov wrote:
> On Tue, 2 Apr 2024 at 13:34, Varadarajan Narayanan
> <[email protected]> wrote:
>>
>> drivers/clk/qcom/common.c uses devm_icc_clk_register under
>> IS_ENABLED(CONFIG_INTERCONNECT_CLK). However, in kernel bot
>> 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'
>>
>> Hence, ensure INTERCONNECT_CLK is not selected as a module.
>
> NAK. Please use `depends on INTERCONNECT_CLK || !INTERCONNECT_CLK` in
> your Kconfig dependencies.

Should icc-clk ever be built as a module? It really seems like it should be
a part of the core framework.. And dependency management would be easier

Konrad

2024-04-03 07:11:32

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v6 1/6] dt-bindings: interconnect: Add Qualcomm IPQ9574 support

On 02/04/2024 12:34, Varadarajan Narayanan wrote:
> +#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(x) ((ICC_ ## x) * 2)
> +#define SLAVE(x) (MASTER(x) + 1)

You already received comment to make your bindings consistent with other
Qualcomm bindings. Now you repeat the same mistake.

No, that is neither consistent nor greppble.


Best regards,
Krzysztof


2024-04-03 10:46:51

by Varadarajan Narayanan

[permalink] [raw]
Subject: Re: [PATCH v6 1/6] dt-bindings: interconnect: Add Qualcomm IPQ9574 support

On Wed, Apr 03, 2024 at 09:09:15AM +0200, Krzysztof Kozlowski wrote:
> On 02/04/2024 12:34, Varadarajan Narayanan wrote:
> > +#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(x) ((ICC_ ## x) * 2)
> > +#define SLAVE(x) (MASTER(x) + 1)
>
> You already received comment to make your bindings consistent with other
> Qualcomm bindings. Now you repeat the same mistake.
>
> No, that is neither consistent nor greppble.

Sorry. Have restored the naming and posted v7.
Kindly take a look.

Thanks
Varada

2024-04-03 10:55:56

by Varadarajan Narayanan

[permalink] [raw]
Subject: Re: [PATCH v6 3/6] interconnect: icc-clk: Add devm_icc_clk_register

On Tue, Apr 02, 2024 at 03:12:04PM +0300, Dmitry Baryshkov wrote:
> On Tue, 2 Apr 2024 at 14:23, Varadarajan Narayanan
> <[email protected]> wrote:
> >
> > On Tue, Apr 02, 2024 at 02:16:56PM +0300, Dmitry Baryshkov wrote:
> > > On Tue, 2 Apr 2024 at 14:02, Varadarajan Narayanan
> > > <[email protected]> wrote:
> > > >
> > > > On Tue, Apr 02, 2024 at 01:48:08PM +0300, Dmitry Baryshkov wrote:
> > > > > On Tue, 2 Apr 2024 at 13:40, Dmitry Baryshkov
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > On Tue, 2 Apr 2024 at 13:34, Varadarajan Narayanan
> > > > > > <[email protected]> wrote:
> > > > > > >
> > > > > > > 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(+)
> > > > > >
> > > > > > Reviewed-by: Dmitry Baryshkov <[email protected]>
> > > > >
> > > > > Wait. Actually,
> > > > >
> > > > > Unreviewed-by: me
> > > > >
> > > > > Please return int from devm_icc_clk_register instead of returning the pointer.
> > > >
> > > > Wouldn't returning int break the general assumption that
> > > > devm_foo(), returns the same type as foo(). For example
> > > > devm_clk_hw_get_clk and clk_hw_get_clk return struct clk *?
> > >
> > > Not always. The only reason to return icc_provider was to make it
> > > possible to destroy it. With devres-managed function you don't have to
> > > do anything.
> >
> > Ok. Will change as follows
> >
> > return prov; -> return PTR_ERR_OR_ZERO(prov);
> >
>
> I think the code might become simpler if you first allocate the ICC
> provider and then just 'return devm_add_action_or_reset(dev,
> your_icc_clk_release, provider)'

Have posted v7 incorporating these and other feedback.
Please review.

Thanks
Varada

2024-04-03 15:48:34

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH v6 2/6] interconnect: icc-clk: Remove tristate from INTERCONNECT_CLK

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-20240403]
[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/20240402-223729
base: https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
patch link: https://lore.kernel.org/r/20240402103406.3638821-3-quic_varada%40quicinc.com
patch subject: [PATCH v6 2/6] interconnect: icc-clk: Remove tristate from INTERCONNECT_CLK
config: arm64-defconfig (https://download.01.org/0day-ci/archive/20240403/[email protected]/config)
compiler: aarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240403/[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 >>):

aarch64-linux-ld: Unexpected GOT/PLT entries detected!
aarch64-linux-ld: Unexpected run-time procedure linkages detected!
aarch64-linux-ld: drivers/clk/qcom/clk-cbf-8996.o: in function `qcom_msm8996_cbf_icc_remove':
>> drivers/clk/qcom/clk-cbf-8996.c:257:(.text+0x10): undefined reference to `icc_clk_unregister'
aarch64-linux-ld: drivers/clk/qcom/clk-cbf-8996.o: in function `qcom_msm8996_cbf_icc_register':
>> drivers/clk/qcom/clk-cbf-8996.c:244:(.text+0x360): undefined reference to `icc_clk_register'


vim +257 drivers/clk/qcom/clk-cbf-8996.c

12dc71953e664f Dmitry Baryshkov 2023-05-12 234
12dc71953e664f Dmitry Baryshkov 2023-05-12 235 static int qcom_msm8996_cbf_icc_register(struct platform_device *pdev, struct clk_hw *cbf_hw)
12dc71953e664f Dmitry Baryshkov 2023-05-12 236 {
12dc71953e664f Dmitry Baryshkov 2023-05-12 237 struct device *dev = &pdev->dev;
12dc71953e664f Dmitry Baryshkov 2023-05-12 238 struct clk *clk = devm_clk_hw_get_clk(dev, cbf_hw, "cbf");
12dc71953e664f Dmitry Baryshkov 2023-05-12 239 const struct icc_clk_data data[] = {
12dc71953e664f Dmitry Baryshkov 2023-05-12 240 { .clk = clk, .name = "cbf", },
12dc71953e664f Dmitry Baryshkov 2023-05-12 241 };
12dc71953e664f Dmitry Baryshkov 2023-05-12 242 struct icc_provider *provider;
12dc71953e664f Dmitry Baryshkov 2023-05-12 243
12dc71953e664f Dmitry Baryshkov 2023-05-12 @244 provider = icc_clk_register(dev, CBF_MASTER_NODE, ARRAY_SIZE(data), data);
12dc71953e664f Dmitry Baryshkov 2023-05-12 245 if (IS_ERR(provider))
12dc71953e664f Dmitry Baryshkov 2023-05-12 246 return PTR_ERR(provider);
12dc71953e664f Dmitry Baryshkov 2023-05-12 247
12dc71953e664f Dmitry Baryshkov 2023-05-12 248 platform_set_drvdata(pdev, provider);
12dc71953e664f Dmitry Baryshkov 2023-05-12 249
12dc71953e664f Dmitry Baryshkov 2023-05-12 250 return 0;
12dc71953e664f Dmitry Baryshkov 2023-05-12 251 }
12dc71953e664f Dmitry Baryshkov 2023-05-12 252
abaf59c470a7c9 Uwe Kleine-K?nig 2023-09-11 253 static void qcom_msm8996_cbf_icc_remove(struct platform_device *pdev)
12dc71953e664f Dmitry Baryshkov 2023-05-12 254 {
12dc71953e664f Dmitry Baryshkov 2023-05-12 255 struct icc_provider *provider = platform_get_drvdata(pdev);
12dc71953e664f Dmitry Baryshkov 2023-05-12 256
12dc71953e664f Dmitry Baryshkov 2023-05-12 @257 icc_clk_unregister(provider);
12dc71953e664f Dmitry Baryshkov 2023-05-12 258 }
12dc71953e664f Dmitry Baryshkov 2023-05-12 259 #define qcom_msm8996_cbf_icc_sync_state icc_sync_state
12dc71953e664f Dmitry Baryshkov 2023-05-12 260 #else
12dc71953e664f Dmitry Baryshkov 2023-05-12 261 static int qcom_msm8996_cbf_icc_register(struct platform_device *pdev, struct clk_hw *cbf_hw)
12dc71953e664f Dmitry Baryshkov 2023-05-12 262 {
12dc71953e664f Dmitry Baryshkov 2023-05-12 263 dev_warn(&pdev->dev, "CONFIG_INTERCONNECT is disabled, CBF clock is fixed\n");
12dc71953e664f Dmitry Baryshkov 2023-05-12 264
12dc71953e664f Dmitry Baryshkov 2023-05-12 265 return 0;
12dc71953e664f Dmitry Baryshkov 2023-05-12 266 }
abaf59c470a7c9 Uwe Kleine-K?nig 2023-09-11 267 #define qcom_msm8996_cbf_icc_remove(pdev) { }
12dc71953e664f Dmitry Baryshkov 2023-05-12 268 #define qcom_msm8996_cbf_icc_sync_state NULL
12dc71953e664f Dmitry Baryshkov 2023-05-12 269 #endif
12dc71953e664f Dmitry Baryshkov 2023-05-12 270

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki