2019-04-05 03:55:50

by Georgi Djakov

[permalink] [raw]
Subject: [PATCH 0/3] Add QCS404 interconnect provider driver

Add driver to support scaling of the on-chip interconnects on QCS404-based
platforms. Also add the necessary device-tree nodes, so that the driver for
each NoC can probe and register as interconnect-provider.

Bjorn Andersson (1):
interconnect: qcom: Add QCS404 interconnect provider driver

Georgi Djakov (2):
dt-bindings: interconnect: Add Qualcomm QCS404 DT bindings
arm64: dts: qcs404: Add interconnect provider DT nodes

.../bindings/interconnect/qcom,qcs404.txt | 45 ++
arch/arm64/boot/dts/qcom/qcs404.dtsi | 25 +
drivers/interconnect/qcom/Kconfig | 8 +
drivers/interconnect/qcom/Makefile | 2 +
drivers/interconnect/qcom/qcs404.c | 488 ++++++++++++++++++
drivers/interconnect/qcom/qcs404_ids.h | 86 +++
.../dt-bindings/interconnect/qcom,qcs404.h | 88 ++++
7 files changed, 742 insertions(+)
create mode 100644 Documentation/devicetree/bindings/interconnect/qcom,qcs404.txt
create mode 100644 drivers/interconnect/qcom/qcs404.c
create mode 100644 drivers/interconnect/qcom/qcs404_ids.h
create mode 100644 include/dt-bindings/interconnect/qcom,qcs404.h


2019-04-05 03:56:00

by Georgi Djakov

[permalink] [raw]
Subject: [PATCH 1/3] dt-bindings: interconnect: Add Qualcomm QCS404 DT bindings

The Qualcomm QCS404 platform has several buses that could be controlled
and tuned according to the bandwidth demand.

Signed-off-by: Georgi Djakov <[email protected]>
---
.../bindings/interconnect/qcom,qcs404.txt | 45 +++++++++++++++++++
1 file changed, 45 insertions(+)
create mode 100644 Documentation/devicetree/bindings/interconnect/qcom,qcs404.txt

diff --git a/Documentation/devicetree/bindings/interconnect/qcom,qcs404.txt b/Documentation/devicetree/bindings/interconnect/qcom,qcs404.txt
new file mode 100644
index 000000000000..2ea63ea827d7
--- /dev/null
+++ b/Documentation/devicetree/bindings/interconnect/qcom,qcs404.txt
@@ -0,0 +1,45 @@
+Qualcomm QCS404 Network-On-Chip interconnect driver binding
+-----------------------------------------------------------
+
+Required properties :
+- compatible : shall contain only one of the following:
+ "qcom,qcs404-bimc"
+ "qcom,qcs404-pcnoc"
+ "qcom,qcs404-snoc"
+- #interconnect-cells : should contain 1
+
+Optional properties :
+clocks : list of phandles and specifiers to all interconnect bus clocks
+clock-names : clock names should include both "bus_clk" and "bus_a_clk"
+
+Example:
+
+rpm-glink {
+ ...
+ rpm_requests: glink-channel {
+ ...
+ bimc: interconnect@0 {
+ compatible = "qcom,qcs404-bimc";
+ #interconnect-cells = <1>;
+ clock-names = "bus_clk", "bus_a_clk";
+ clocks = <&rpmcc RPM_SMD_BIMC_CLK>,
+ <&rpmcc RPM_SMD_BIMC_A_CLK>;
+ };
+
+ pnoc: interconnect@1 {
+ compatible = "qcom,qcs404-pcnoc";
+ #interconnect-cells = <1>;
+ clock-names = "bus_clk", "bus_a_clk";
+ clocks = <&rpmcc RPM_SMD_PNOC_CLK>,
+ <&rpmcc RPM_SMD_PNOC_A_CLK>;
+ };
+
+ snoc: interconnect@2 {
+ compatible = "qcom,qcs404-snoc";
+ #interconnect-cells = <1>;
+ clock-names = "bus_clk", "bus_a_clk";
+ clocks = <&rpmcc RPM_SMD_SNOC_CLK>,
+ <&rpmcc RPM_SMD_SNOC_A_CLK>;
+ };
+ };
+};

2019-04-05 03:56:28

by Georgi Djakov

[permalink] [raw]
Subject: [PATCH 2/3] interconnect: qcom: Add QCS404 interconnect provider driver

From: Bjorn Andersson <[email protected]>

Add driver for the interconnect buses found in Qualcomm QCS404-based
platforms. The topology consists of three NoCs that are controlled by
a remote processor that collects the aggregated bandwidth for each
master-slave pairs.

Signed-off-by: Bjorn Andersson <[email protected]>
Signed-off-by: Georgi Djakov <[email protected]>
---
drivers/interconnect/qcom/Kconfig | 8 +
drivers/interconnect/qcom/Makefile | 2 +
drivers/interconnect/qcom/qcs404.c | 488 ++++++++++++++++++
drivers/interconnect/qcom/qcs404_ids.h | 86 +++
.../dt-bindings/interconnect/qcom,qcs404.h | 88 ++++
5 files changed, 672 insertions(+)
create mode 100644 drivers/interconnect/qcom/qcs404.c
create mode 100644 drivers/interconnect/qcom/qcs404_ids.h
create mode 100644 include/dt-bindings/interconnect/qcom,qcs404.h

diff --git a/drivers/interconnect/qcom/Kconfig b/drivers/interconnect/qcom/Kconfig
index 290d330abe5a..cf9e6d430994 100644
--- a/drivers/interconnect/qcom/Kconfig
+++ b/drivers/interconnect/qcom/Kconfig
@@ -4,6 +4,14 @@ config INTERCONNECT_QCOM
help
Support for Qualcomm's Network-on-Chip interconnect hardware.

+config INTERCONNECT_QCOM_QCS404
+ tristate "Qualcomm QCS404 interconnect driver"
+ depends on INTERCONNECT_QCOM
+ depends on QCOM_SMD_RPM || COMPILE_TEST
+ help
+ This is a driver for the Qualcomm Network-on-Chip on qcs404-based
+ platforms.
+
config INTERCONNECT_QCOM_SDM845
tristate "Qualcomm SDM845 interconnect driver"
depends on INTERCONNECT_QCOM
diff --git a/drivers/interconnect/qcom/Makefile b/drivers/interconnect/qcom/Makefile
index 1c1cea690f92..059ff325ee6c 100644
--- a/drivers/interconnect/qcom/Makefile
+++ b/drivers/interconnect/qcom/Makefile
@@ -1,5 +1,7 @@
# SPDX-License-Identifier: GPL-2.0

+qnoc-qcs404-objs := qcs404.o
qnoc-sdm845-objs := sdm845.o

+obj-$(CONFIG_INTERCONNECT_QCOM_QCS404) += qnoc-qcs404.o
obj-$(CONFIG_INTERCONNECT_QCOM_SDM845) += qnoc-sdm845.o
diff --git a/drivers/interconnect/qcom/qcs404.c b/drivers/interconnect/qcom/qcs404.c
new file mode 100644
index 000000000000..42d36db13ec0
--- /dev/null
+++ b/drivers/interconnect/qcom/qcs404.c
@@ -0,0 +1,488 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Linaro Ltd
+ */
+
+#include <dt-bindings/interconnect/qcom,qcs404.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/interconnect-provider.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/soc/qcom/smd-rpm.h>
+
+#include "qcs404_ids.h"
+
+#define RPM_BUS_MASTER_REQ 0x73616d62
+#define RPM_BUS_SLAVE_REQ 0x766c7362
+#define RPM_KEY_BW 0x00007762
+
+#define to_qcom_provider(_provider) \
+ container_of(_provider, struct qcom_icc_provider, provider)
+
+struct qcom_smd_rpm *qcs404_rpm;
+
+struct icc_rpm_smd_req {
+ __le32 key;
+ __le32 nbytes;
+ __le32 value;
+};
+
+struct qcom_icc_provider {
+ struct icc_provider provider;
+ struct clk *bus_clk;
+ struct clk *bus_a_clk;
+};
+
+#define QCS404_MAX_LINKS 12
+
+/**
+ * struct qcom_icc_node - Qualcomm specific interconnect nodes
+ * @name: the node name used in debugfs
+ * @id: a unique node identifier
+ * @links: an array of nodes where we can go next while traversing
+ * @num_links: the total number of @links
+ * @buswidth: width of the interconnect between a node and the bus (bytes)
+ * @mas_rpm_id: RPM id for devices that are bus masters
+ * @slv_rpm_id: RPM id for devices that are bus slaves
+ * @rate: current bus clock rate in Hz
+ */
+struct qcom_icc_node {
+ unsigned char *name;
+ u16 id;
+ u16 links[QCS404_MAX_LINKS];
+ u16 num_links;
+ u16 buswidth;
+ int mas_rpm_id;
+ int slv_rpm_id;
+ u64 rate;
+};
+
+struct qcom_icc_desc {
+ struct qcom_icc_node **nodes;
+ size_t num_nodes;
+};
+
+#define DEFINE_QNODE(_name, _id, _buswidth, _mas_rpm_id, _slv_rpm_id, \
+ _numlinks, ...) \
+ static struct qcom_icc_node _name = { \
+ .name = #_name, \
+ .id = _id, \
+ .buswidth = _buswidth, \
+ .mas_rpm_id = _mas_rpm_id, \
+ .slv_rpm_id = _slv_rpm_id, \
+ .num_links = _numlinks, \
+ .links = { __VA_ARGS__ }, \
+ }
+
+DEFINE_QNODE(mas_apps_proc, QCS404_MASTER_AMPSS_M0, 8, 0, -1, 2, QCS404_SLAVE_EBI_CH0, QCS404_BIMC_SNOC_SLV);
+DEFINE_QNODE(mas_oxili, QCS404_MASTER_GRAPHICS_3D, 8, 6, -1, 2, QCS404_SLAVE_EBI_CH0, QCS404_BIMC_SNOC_SLV);
+DEFINE_QNODE(mas_mdp, QCS404_MASTER_MDP_PORT0, 8, 8, -1, 2, QCS404_SLAVE_EBI_CH0, QCS404_BIMC_SNOC_SLV);
+DEFINE_QNODE(mas_snoc_bimc_1, QCS404_SNOC_BIMC_1_MAS, 8, 76, -1, 1, QCS404_SLAVE_EBI_CH0);
+DEFINE_QNODE(mas_tcu_0, QCS404_MASTER_TCU_0, 8, -1, -1, 2, QCS404_SLAVE_EBI_CH0, QCS404_BIMC_SNOC_SLV);
+DEFINE_QNODE(mas_spdm, QCS404_MASTER_SPDM, 4, -1, -1, 1, QCS404_PNOC_INT_3);
+DEFINE_QNODE(mas_blsp_1, QCS404_MASTER_BLSP_1, 4, 41, -1, 1, QCS404_PNOC_INT_3);
+DEFINE_QNODE(mas_blsp_2, QCS404_MASTER_BLSP_2, 4, 39, -1, 1, QCS404_PNOC_INT_3);
+DEFINE_QNODE(mas_xi_usb_hs1, QCS404_MASTER_XM_USB_HS1, 8, 138, -1, 1, QCS404_PNOC_INT_0);
+DEFINE_QNODE(mas_crypto, QCS404_MASTER_CRYPTO_CORE0, 8, 23, -1, 2, QCS404_PNOC_SNOC_SLV, QCS404_PNOC_INT_2);
+DEFINE_QNODE(mas_sdcc_1, QCS404_MASTER_SDCC_1, 8, 33, -1, 1, QCS404_PNOC_INT_0);
+DEFINE_QNODE(mas_sdcc_2, QCS404_MASTER_SDCC_2, 8, 35, -1, 1, QCS404_PNOC_INT_0);
+DEFINE_QNODE(mas_snoc_pcnoc, QCS404_SNOC_PNOC_MAS, 8, 77, -1, 1, QCS404_PNOC_INT_2);
+DEFINE_QNODE(mas_qpic, QCS404_MASTER_QPIC, 4, -1, -1, 1, QCS404_PNOC_INT_0);
+DEFINE_QNODE(mas_qdss_bam, QCS404_MASTER_QDSS_BAM, 4, -1, -1, 1, QCS404_SNOC_QDSS_INT);
+DEFINE_QNODE(mas_bimc_snoc, QCS404_BIMC_SNOC_MAS, 8, 21, -1, 4, QCS404_SLAVE_OCMEM_64, QCS404_SLAVE_CATS_128, QCS404_SNOC_INT_0, QCS404_SNOC_INT_1);
+DEFINE_QNODE(mas_pcnoc_snoc, QCS404_PNOC_SNOC_MAS, 8, 29, -1, 3, QCS404_SNOC_BIMC_1_SLV, QCS404_SNOC_INT_2, QCS404_SNOC_INT_0);
+DEFINE_QNODE(mas_qdss_etr, QCS404_MASTER_QDSS_ETR, 8, -1, -1, 1, QCS404_SNOC_QDSS_INT);
+DEFINE_QNODE(mas_emac, QCS404_MASTER_EMAC, 8, -1, -1, 2, QCS404_SNOC_BIMC_1_SLV, QCS404_SNOC_INT_1);
+DEFINE_QNODE(mas_pcie, QCS404_MASTER_PCIE, 8, -1, -1, 2, QCS404_SNOC_BIMC_1_SLV, QCS404_SNOC_INT_1);
+DEFINE_QNODE(mas_usb3, QCS404_MASTER_USB3, 8, -1, -1, 2, QCS404_SNOC_BIMC_1_SLV, QCS404_SNOC_INT_1);
+DEFINE_QNODE(pcnoc_int_0, QCS404_PNOC_INT_0, 8, 85, 114, 2, QCS404_PNOC_SNOC_SLV, QCS404_PNOC_INT_2);
+DEFINE_QNODE(pcnoc_int_2, QCS404_PNOC_INT_2, 8, 124, 184, 12, QCS404_PNOC_SLV_10, QCS404_SLAVE_TCU, QCS404_PNOC_SLV_11, QCS404_PNOC_SLV_2, QCS404_PNOC_SLV_3, QCS404_PNOC_SLV_0, QCS404_PNOC_SLV_1, QCS404_PNOC_SLV_6, QCS404_PNOC_SLV_7, QCS404_PNOC_SLV_4, QCS404_PNOC_SLV_8, QCS404_PNOC_SLV_9);
+DEFINE_QNODE(pcnoc_int_3, QCS404_PNOC_INT_3, 8, 125, 185, 1, QCS404_PNOC_SNOC_SLV);
+DEFINE_QNODE(pcnoc_s_0, QCS404_PNOC_SLV_0, 4, 89, 118, 3, QCS404_SLAVE_PRNG, QCS404_SLAVE_SPDM_WRAPPER, QCS404_SLAVE_PDM);
+DEFINE_QNODE(pcnoc_s_1, QCS404_PNOC_SLV_1, 4, 90, 119, 1, QCS404_SLAVE_TCSR);
+DEFINE_QNODE(pcnoc_s_2, QCS404_PNOC_SLV_2, 4, -1, -1, 1, QCS404_SLAVE_GRAPHICS_3D_CFG);
+DEFINE_QNODE(pcnoc_s_3, QCS404_PNOC_SLV_3, 4, 92, 121, 1, QCS404_SLAVE_MESSAGE_RAM);
+DEFINE_QNODE(pcnoc_s_4, QCS404_PNOC_SLV_4, 4, 93, 122, 1, QCS404_SLAVE_SNOC_CFG);
+DEFINE_QNODE(pcnoc_s_6, QCS404_PNOC_SLV_6, 4, 94, 123, 3, QCS404_SLAVE_BLSP_1, QCS404_SLAVE_TLMM_NORTH, QCS404_SLAVE_EMAC_CFG);
+DEFINE_QNODE(pcnoc_s_7, QCS404_PNOC_SLV_7, 4, 95, 124, 4, QCS404_SLAVE_TLMM_SOUTH, QCS404_SLAVE_DISPLAY_CFG, QCS404_SLAVE_SDCC_1, QCS404_SLAVE_PCIE_1, QCS404_SLAVE_SDCC_2);
+DEFINE_QNODE(pcnoc_s_8, QCS404_PNOC_SLV_8, 4, 96, 125, 1, QCS404_SLAVE_CRYPTO_0_CFG);
+DEFINE_QNODE(pcnoc_s_9, QCS404_PNOC_SLV_9, 4, 97, 126, 3, QCS404_SLAVE_BLSP_2, QCS404_SLAVE_TLMM_EAST, QCS404_SLAVE_PMIC_ARB);
+DEFINE_QNODE(pcnoc_s_10, QCS404_PNOC_SLV_10, 4, 157, -1, 1, QCS404_SLAVE_USB_HS);
+DEFINE_QNODE(pcnoc_s_11, QCS404_PNOC_SLV_11, 4, 158, 246, 1, QCS404_SLAVE_USB3);
+DEFINE_QNODE(qdss_int, QCS404_SNOC_QDSS_INT, 8, -1, -1, 2, QCS404_SNOC_BIMC_1_SLV, QCS404_SNOC_INT_1);
+DEFINE_QNODE(snoc_int_0, QCS404_SNOC_INT_0, 8, 99, 130, 3, QCS404_SLAVE_LPASS, QCS404_SLAVE_APPSS, QCS404_SLAVE_WCSS);
+DEFINE_QNODE(snoc_int_1, QCS404_SNOC_INT_1, 8, 100, 131, 2, QCS404_SNOC_PNOC_SLV, QCS404_SNOC_INT_2);
+DEFINE_QNODE(snoc_int_2, QCS404_SNOC_INT_2, 8, 134, 197, 2, QCS404_SLAVE_QDSS_STM, QCS404_SLAVE_OCIMEM);
+DEFINE_QNODE(slv_ebi, QCS404_SLAVE_EBI_CH0, 8, -1, 0, 0, 0);
+DEFINE_QNODE(slv_bimc_snoc, QCS404_BIMC_SNOC_SLV, 8, -1, 2, 1, QCS404_BIMC_SNOC_MAS);
+DEFINE_QNODE(slv_spdm, QCS404_SLAVE_SPDM_WRAPPER, 4, -1, -1, 0, 0);
+DEFINE_QNODE(slv_pdm, QCS404_SLAVE_PDM, 4, -1, 41, 0, 0);
+DEFINE_QNODE(slv_prng, QCS404_SLAVE_PRNG, 4, -1, 44, 0, 0);
+DEFINE_QNODE(slv_tcsr, QCS404_SLAVE_TCSR, 4, -1, 50, 0, 0);
+DEFINE_QNODE(slv_snoc_cfg, QCS404_SLAVE_SNOC_CFG, 4, -1, 70, 0, 0);
+DEFINE_QNODE(slv_message_ram, QCS404_SLAVE_MESSAGE_RAM, 4, -1, 55, 0, 0);
+DEFINE_QNODE(slv_disp_ss_cfg, QCS404_SLAVE_DISPLAY_CFG, 4, -1, -1, 0, 0);
+DEFINE_QNODE(slv_gpu_cfg, QCS404_SLAVE_GRAPHICS_3D_CFG, 4, -1, -1, 0, 0);
+DEFINE_QNODE(slv_blsp_1, QCS404_SLAVE_BLSP_1, 4, -1, 39, 0, 0);
+DEFINE_QNODE(slv_tlmm_north, QCS404_SLAVE_TLMM_NORTH, 4, -1, 214, 0, 0);
+DEFINE_QNODE(slv_pcie, QCS404_SLAVE_PCIE_1, 4, -1, -1, 0, 0);
+DEFINE_QNODE(slv_ethernet, QCS404_SLAVE_EMAC_CFG, 4, -1, -1, 0, 0);
+DEFINE_QNODE(slv_blsp_2, QCS404_SLAVE_BLSP_2, 4, -1, 37, 0, 0);
+DEFINE_QNODE(slv_tlmm_east, QCS404_SLAVE_TLMM_EAST, 4, -1, 213, 0, 0);
+DEFINE_QNODE(slv_tcu, QCS404_SLAVE_TCU, 8, -1, -1, 0, 0);
+DEFINE_QNODE(slv_pmic_arb, QCS404_SLAVE_PMIC_ARB, 4, -1, 59, 0, 0);
+DEFINE_QNODE(slv_sdcc_1, QCS404_SLAVE_SDCC_1, 4, -1, 31, 0, 0);
+DEFINE_QNODE(slv_sdcc_2, QCS404_SLAVE_SDCC_2, 4, -1, 33, 0, 0);
+DEFINE_QNODE(slv_tlmm_south, QCS404_SLAVE_TLMM_SOUTH, 4, -1, -1, 0, 0);
+DEFINE_QNODE(slv_usb_hs, QCS404_SLAVE_USB_HS, 4, -1, 40, 0, 0);
+DEFINE_QNODE(slv_usb3, QCS404_SLAVE_USB3, 4, -1, 22, 0, 0);
+DEFINE_QNODE(slv_crypto_0_cfg, QCS404_SLAVE_CRYPTO_0_CFG, 4, -1, 52, 0, 0);
+DEFINE_QNODE(slv_pcnoc_snoc, QCS404_PNOC_SNOC_SLV, 8, -1, 45, 1, QCS404_PNOC_SNOC_MAS);
+DEFINE_QNODE(slv_kpss_ahb, QCS404_SLAVE_APPSS, 4, -1, -1, 0, 0);
+DEFINE_QNODE(slv_wcss, QCS404_SLAVE_WCSS, 4, -1, 23, 0, 0);
+DEFINE_QNODE(slv_snoc_bimc_1, QCS404_SNOC_BIMC_1_SLV, 8, -1, 104, 1, QCS404_SNOC_BIMC_1_MAS);
+DEFINE_QNODE(slv_imem, QCS404_SLAVE_OCIMEM, 8, -1, 26, 0, 0);
+DEFINE_QNODE(slv_snoc_pcnoc, QCS404_SNOC_PNOC_SLV, 8, -1, 28, 1, QCS404_SNOC_PNOC_MAS);
+DEFINE_QNODE(slv_qdss_stm, QCS404_SLAVE_QDSS_STM, 4, -1, 30, 0, 0);
+DEFINE_QNODE(slv_cats_0, QCS404_SLAVE_CATS_128, 16, -1, -1, 0, 0);
+DEFINE_QNODE(slv_cats_1, QCS404_SLAVE_OCMEM_64, 8, -1, -1, 0, 0);
+DEFINE_QNODE(slv_lpass, QCS404_SLAVE_LPASS, 4, -1, -1, 0, 0);
+
+static struct qcom_icc_node *qcs404_bimc_nodes[] = {
+ [MASTER_AMPSS_M0] = &mas_apps_proc,
+ [MASTER_OXILI] = &mas_oxili,
+ [MASTER_MDP_PORT0] = &mas_mdp,
+ [MASTER_SNOC_BIMC_1] = &mas_snoc_bimc_1,
+ [MASTER_TCU_0] = &mas_tcu_0,
+ [SLAVE_EBI_CH0] = &slv_ebi,
+ [SLAVE_BIMC_SNOC] = &slv_bimc_snoc,
+};
+
+static struct qcom_icc_desc qcs404_bimc = {
+ .nodes = qcs404_bimc_nodes,
+ .num_nodes = ARRAY_SIZE(qcs404_bimc_nodes),
+};
+
+static struct qcom_icc_node *qcs404_pcnoc_nodes[] = {
+ [MASTER_SPDM] = &mas_spdm,
+ [MASTER_BLSP_1] = &mas_blsp_1,
+ [MASTER_BLSP_2] = &mas_blsp_2,
+ [MASTER_XI_USB_HS1] = &mas_xi_usb_hs1,
+ [MASTER_CRYPT0] = &mas_crypto,
+ [MASTER_SDCC_1] = &mas_sdcc_1,
+ [MASTER_SDCC_2] = &mas_sdcc_2,
+ [MASTER_SNOC_PCNOC] = &mas_snoc_pcnoc,
+ [MASTER_QPIC] = &mas_qpic,
+ [PCNOC_INT_0] = &pcnoc_int_0,
+ [PCNOC_INT_2] = &pcnoc_int_2,
+ [PCNOC_INT_3] = &pcnoc_int_3,
+ [PCNOC_S_0] = &pcnoc_s_0,
+ [PCNOC_S_1] = &pcnoc_s_1,
+ [PCNOC_S_2] = &pcnoc_s_2,
+ [PCNOC_S_3] = &pcnoc_s_3,
+ [PCNOC_S_4] = &pcnoc_s_4,
+ [PCNOC_S_6] = &pcnoc_s_6,
+ [PCNOC_S_7] = &pcnoc_s_7,
+ [PCNOC_S_8] = &pcnoc_s_8,
+ [PCNOC_S_9] = &pcnoc_s_9,
+ [PCNOC_S_10] = &pcnoc_s_10,
+ [PCNOC_S_11] = &pcnoc_s_11,
+ [SLAVE_SPDM] = &slv_spdm,
+ [SLAVE_PDM] = &slv_pdm,
+ [SLAVE_PRNG] = &slv_prng,
+ [SLAVE_TCSR] = &slv_tcsr,
+ [SLAVE_SNOC_CFG] = &slv_snoc_cfg,
+ [SLAVE_MESSAGE_RAM] = &slv_message_ram,
+ [SLAVE_DISP_SS_CFG] = &slv_disp_ss_cfg,
+ [SLAVE_GPU_CFG] = &slv_gpu_cfg,
+ [SLAVE_BLSP_1] = &slv_blsp_1,
+ [SLAVE_BLSP_2] = &slv_blsp_2,
+ [SLAVE_TLMM_NORTH] = &slv_tlmm_north,
+ [SLAVE_PCIE] = &slv_pcie,
+ [SLAVE_ETHERNET] = &slv_ethernet,
+ [SLAVE_TLMM_EAST] = &slv_tlmm_east,
+ [SLAVE_TCU] = &slv_tcu,
+ [SLAVE_PMIC_ARB] = &slv_pmic_arb,
+ [SLAVE_SDCC_1] = &slv_sdcc_1,
+ [SLAVE_SDCC_2] = &slv_sdcc_2,
+ [SLAVE_TLMM_SOUTH] = &slv_tlmm_south,
+ [SLAVE_USB_HS] = &slv_usb_hs,
+ [SLAVE_USB3] = &slv_usb3,
+ [SLAVE_CRYPTO_0_CFG] = &slv_crypto_0_cfg,
+ [SLAVE_PCNOC_SNOC] = &slv_pcnoc_snoc,
+};
+
+static struct qcom_icc_desc qcs404_pcnoc = {
+ .nodes = qcs404_pcnoc_nodes,
+ .num_nodes = ARRAY_SIZE(qcs404_pcnoc_nodes),
+};
+
+static struct qcom_icc_node *qcs404_snoc_nodes[] = {
+ [MASTER_QDSS_BAM] = &mas_qdss_bam,
+ [MASTER_BIMC_SNOC] = &mas_bimc_snoc,
+ [MASTER_PCNOC_SNOC] = &mas_pcnoc_snoc,
+ [MASTER_QDSS_ETR] = &mas_qdss_etr,
+ [MASTER_EMAC] = &mas_emac,
+ [MASTER_PCIE] = &mas_pcie,
+ [MASTER_USB3] = &mas_usb3,
+ [QDSS_INT] = &qdss_int,
+ [SNOC_INT_0] = &snoc_int_0,
+ [SNOC_INT_1] = &snoc_int_1,
+ [SNOC_INT_2] = &snoc_int_2,
+ [SLAVE_KPSS_AHB] = &slv_kpss_ahb,
+ [SLAVE_WCSS] = &slv_wcss,
+ [SLAVE_SNOC_BIMC_1] = &slv_snoc_bimc_1,
+ [SLAVE_IMEM] = &slv_imem,
+ [SLAVE_SNOC_PCNOC] = &slv_snoc_pcnoc,
+ [SLAVE_QDSS_STM] = &slv_qdss_stm,
+ [SLAVE_CATS_0] = &slv_cats_0,
+ [SLAVE_CATS_1] = &slv_cats_1,
+ [SLAVE_LPASS] = &slv_lpass,
+};
+
+static struct qcom_icc_desc qcs404_snoc = {
+ .nodes = qcs404_snoc_nodes,
+ .num_nodes = ARRAY_SIZE(qcs404_snoc_nodes),
+};
+
+static int qcom_icc_aggregate(struct icc_node *node, u32 avg_bw, u32 peak_bw,
+ u32 *agg_avg, u32 *agg_peak)
+{
+ *agg_avg += avg_bw;
+ *agg_peak = max(*agg_peak, peak_bw);
+
+ return 0;
+}
+
+static int qcom_icc_rpm_smd_send(int ctx, int rsc_type, int id, u32 val)
+{
+ struct icc_rpm_smd_req req = {
+ .key = cpu_to_le32(RPM_KEY_BW),
+ .nbytes = cpu_to_le32(sizeof(u32)),
+ .value = cpu_to_le32(val),
+ };
+
+ return qcom_rpm_smd_write(qcs404_rpm, ctx, rsc_type, id, &req,
+ sizeof(req));
+}
+
+static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
+{
+ struct qcom_icc_provider *qp;
+ struct qcom_icc_node *qn;
+ struct icc_provider *provider;
+ struct icc_node *n;
+ u64 sum_bw;
+ u64 max_peak_bw;
+ u64 rate;
+ u32 agg_avg = 0;
+ u32 agg_peak = 0;
+ int ret = 0;
+
+ qn = src->data;
+ provider = src->provider;
+ qp = to_qcom_provider(provider);
+
+ list_for_each_entry(n, &provider->nodes, node_list)
+ qcom_icc_aggregate(n, n->avg_bw, n->peak_bw,
+ &agg_avg, &agg_peak);
+
+ sum_bw = icc_units_to_bps(agg_avg);
+ max_peak_bw = icc_units_to_bps(agg_peak);
+
+ /* send message to the RPM processor */
+ if (qn->mas_rpm_id != -1) {
+ ret = qcom_icc_rpm_smd_send(QCOM_SMD_RPM_ACTIVE_STATE,
+ RPM_BUS_MASTER_REQ,
+ qn->mas_rpm_id,
+ sum_bw);
+ if (ret) {
+ pr_err("qcom_icc_rpm_smd_send mas %d error %d\n",
+ qn->mas_rpm_id, ret);
+ return ret;
+ }
+ }
+
+ if (qn->slv_rpm_id != -1) {
+ ret = qcom_icc_rpm_smd_send(QCOM_SMD_RPM_ACTIVE_STATE,
+ RPM_BUS_SLAVE_REQ,
+ qn->slv_rpm_id,
+ sum_bw);
+ if (ret) {
+ pr_err("qcom_icc_rpm_smd_send slv error %d\n",
+ ret);
+ return ret;
+ }
+ }
+
+ rate = max(sum_bw, max_peak_bw);
+
+ do_div(rate, qn->buswidth);
+
+ if (qn->rate != rate) {
+ ret = clk_set_rate(qp->bus_clk, rate);
+ if (ret) {
+ pr_err("set clk rate %lld error %d\n", rate, ret);
+ return ret;
+ }
+
+ ret = clk_set_rate(qp->bus_a_clk, rate);
+ if (ret) {
+ pr_err("set clk rate %lld error %d\n", rate, ret);
+ return ret;
+ }
+
+ qn->rate = rate;
+ }
+
+ return ret;
+}
+
+static int qnoc_probe(struct platform_device *pdev)
+{
+ const struct qcom_icc_desc *desc;
+ struct icc_onecell_data *data;
+ struct icc_provider *provider;
+ struct qcom_icc_node **qnodes;
+ struct qcom_icc_provider *qp;
+ struct icc_node *node;
+ size_t num_nodes, i;
+ int ret;
+
+ /* wait for RPM */
+ qcs404_rpm = dev_get_drvdata(pdev->dev.parent);
+ if (!qcs404_rpm) {
+ dev_err(&pdev->dev, "unable to retrieve handle to RPM\n");
+ return -ENODEV;
+ }
+
+ desc = of_device_get_match_data(&pdev->dev);
+ if (!desc)
+ return -EINVAL;
+
+ qnodes = desc->nodes;
+ num_nodes = desc->num_nodes;
+
+ qp = devm_kzalloc(&pdev->dev, sizeof(*qp), GFP_KERNEL);
+ if (!qp)
+ return -ENOMEM;
+
+ data = devm_kcalloc(&pdev->dev, num_nodes, sizeof(*node), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ qp->bus_clk = devm_clk_get(&pdev->dev, "bus_clk");
+ if (IS_ERR(qp->bus_clk))
+ return PTR_ERR(qp->bus_clk);
+
+ ret = clk_prepare_enable(qp->bus_clk);
+ if (ret) {
+ dev_err(&pdev->dev, "error enabling bus_clk: %d\n", ret);
+ return ret;
+ }
+
+ qp->bus_a_clk = devm_clk_get(&pdev->dev, "bus_a_clk");
+ if (IS_ERR(qp->bus_a_clk))
+ return PTR_ERR(qp->bus_a_clk);
+
+ ret = clk_prepare_enable(qp->bus_a_clk);
+ if (ret) {
+ dev_err(&pdev->dev, "error enabling bus_a_clk: %d\n", ret);
+ clk_disable_unprepare(qp->bus_clk);
+ return ret;
+ }
+
+ provider = &qp->provider;
+ INIT_LIST_HEAD(&provider->nodes);
+ provider->dev = &pdev->dev;
+ provider->set = qcom_icc_set;
+ provider->aggregate = qcom_icc_aggregate;
+ provider->xlate = of_icc_xlate_onecell;
+ provider->data = data;
+
+ ret = icc_provider_add(provider);
+ if (ret) {
+ dev_err(&pdev->dev, "error adding interconnect provider\n");
+ clk_disable_unprepare(qp->bus_clk);
+ clk_disable_unprepare(qp->bus_a_clk);
+ return ret;
+ }
+
+ for (i = 0; i < num_nodes; i++) {
+ size_t j;
+
+ node = icc_node_create(qnodes[i]->id);
+ if (IS_ERR(node)) {
+ ret = PTR_ERR(node);
+ goto err;
+ }
+
+ node->name = qnodes[i]->name;
+ node->data = qnodes[i];
+ icc_node_add(node, provider);
+
+ dev_dbg(&pdev->dev, "registered node %s\n", node->name);
+
+ /* populate links */
+ for (j = 0; j < qnodes[i]->num_links; j++)
+ icc_link_create(node, qnodes[i]->links[j]);
+
+ data->nodes[i] = node;
+ }
+ data->num_nodes = num_nodes;
+
+ platform_set_drvdata(pdev, qp);
+
+ return ret;
+err:
+ list_for_each_entry(node, &provider->nodes, node_list) {
+ icc_node_del(node);
+ icc_node_destroy(node->id);
+ }
+ clk_disable_unprepare(qp->bus_clk);
+ clk_disable_unprepare(qp->bus_a_clk);
+ icc_provider_del(provider);
+
+ return ret;
+}
+
+static int qnoc_remove(struct platform_device *pdev)
+{
+ struct qcom_icc_provider *qp = platform_get_drvdata(pdev);
+ struct icc_provider *provider = &qp->provider;
+ struct icc_node *n;
+
+ list_for_each_entry(n, &provider->nodes, node_list) {
+ icc_node_del(n);
+ icc_node_destroy(n->id);
+ }
+ clk_disable_unprepare(qp->bus_clk);
+ clk_disable_unprepare(qp->bus_a_clk);
+
+ return icc_provider_del(provider);
+}
+
+static const struct of_device_id qcs404_noc_of_match[] = {
+ { .compatible = "qcom,qcs404-bimc", .data = &qcs404_bimc },
+ { .compatible = "qcom,qcs404-pcnoc", .data = &qcs404_pcnoc },
+ { .compatible = "qcom,qcs404-snoc", .data = &qcs404_snoc },
+ { },
+};
+MODULE_DEVICE_TABLE(of, qnoc_of_match);
+
+static struct platform_driver qnoc_driver = {
+ .probe = qnoc_probe,
+ .remove = qnoc_remove,
+ .driver = {
+ .name = "qnoc-qcs404",
+ .of_match_table = qcs404_noc_of_match,
+ },
+};
+module_platform_driver(qnoc_driver);
+MODULE_DESCRIPTION("Qualcomm QCS404 NoC driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/interconnect/qcom/qcs404_ids.h b/drivers/interconnect/qcom/qcs404_ids.h
new file mode 100644
index 000000000000..bcf161829241
--- /dev/null
+++ b/drivers/interconnect/qcom/qcs404_ids.h
@@ -0,0 +1,86 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Qualcomm QCS404 interconnect IDs
+ *
+ * Copyright (c) 2019, Linaro Ltd.
+ * Author: Georgi Djakov <[email protected]>
+ */
+
+#ifndef __DRIVERS_INTERCONNECT_QCOM_QCS404_IDS_H
+#define __DRIVERS_INTERCONNECT_QCOM_QCS404_IDS_H
+
+#define QCS404_MASTER_AMPSS_M0 0
+#define QCS404_MASTER_GRAPHICS_3D 1
+#define QCS404_MASTER_MDP_PORT0 2
+#define QCS404_SNOC_BIMC_1_MAS 3
+#define QCS404_MASTER_TCU_0 4
+#define QCS404_MASTER_SPDM 5
+#define QCS404_MASTER_BLSP_1 6
+#define QCS404_MASTER_BLSP_2 7
+#define QCS404_MASTER_XM_USB_HS1 8
+#define QCS404_MASTER_CRYPTO_CORE0 9
+#define QCS404_MASTER_SDCC_1 10
+#define QCS404_MASTER_SDCC_2 11
+#define QCS404_SNOC_PNOC_MAS 12
+#define QCS404_MASTER_QPIC 13
+#define QCS404_MASTER_QDSS_BAM 14
+#define QCS404_BIMC_SNOC_MAS 15
+#define QCS404_PNOC_SNOC_MAS 16
+#define QCS404_MASTER_QDSS_ETR 17
+#define QCS404_MASTER_EMAC 18
+#define QCS404_MASTER_PCIE 19
+#define QCS404_MASTER_USB3 20
+#define QCS404_PNOC_INT_0 21
+#define QCS404_PNOC_INT_2 22
+#define QCS404_PNOC_INT_3 23
+#define QCS404_PNOC_SLV_0 24
+#define QCS404_PNOC_SLV_1 25
+#define QCS404_PNOC_SLV_2 26
+#define QCS404_PNOC_SLV_3 27
+#define QCS404_PNOC_SLV_4 28
+#define QCS404_PNOC_SLV_6 29
+#define QCS404_PNOC_SLV_7 30
+#define QCS404_PNOC_SLV_8 31
+#define QCS404_PNOC_SLV_9 32
+#define QCS404_PNOC_SLV_10 33
+#define QCS404_PNOC_SLV_11 34
+#define QCS404_SNOC_QDSS_INT 35
+#define QCS404_SNOC_INT_0 36
+#define QCS404_SNOC_INT_1 37
+#define QCS404_SNOC_INT_2 38
+#define QCS404_SLAVE_EBI_CH0 39
+#define QCS404_BIMC_SNOC_SLV 40
+#define QCS404_SLAVE_SPDM_WRAPPER 41
+#define QCS404_SLAVE_PDM 42
+#define QCS404_SLAVE_PRNG 43
+#define QCS404_SLAVE_TCSR 44
+#define QCS404_SLAVE_SNOC_CFG 45
+#define QCS404_SLAVE_MESSAGE_RAM 46
+#define QCS404_SLAVE_DISPLAY_CFG 47
+#define QCS404_SLAVE_GRAPHICS_3D_CFG 48
+#define QCS404_SLAVE_BLSP_1 49
+#define QCS404_SLAVE_TLMM_NORTH 50
+#define QCS404_SLAVE_PCIE_1 51
+#define QCS404_SLAVE_EMAC_CFG 52
+#define QCS404_SLAVE_BLSP_2 53
+#define QCS404_SLAVE_TLMM_EAST 54
+#define QCS404_SLAVE_TCU 55
+#define QCS404_SLAVE_PMIC_ARB 56
+#define QCS404_SLAVE_SDCC_1 57
+#define QCS404_SLAVE_SDCC_2 58
+#define QCS404_SLAVE_TLMM_SOUTH 59
+#define QCS404_SLAVE_USB_HS 60
+#define QCS404_SLAVE_USB3 61
+#define QCS404_SLAVE_CRYPTO_0_CFG 62
+#define QCS404_PNOC_SNOC_SLV 63
+#define QCS404_SLAVE_APPSS 64
+#define QCS404_SLAVE_WCSS 65
+#define QCS404_SNOC_BIMC_1_SLV 66
+#define QCS404_SLAVE_OCIMEM 67
+#define QCS404_SNOC_PNOC_SLV 68
+#define QCS404_SLAVE_QDSS_STM 69
+#define QCS404_SLAVE_CATS_128 70
+#define QCS404_SLAVE_OCMEM_64 71
+#define QCS404_SLAVE_LPASS 72
+
+#endif
diff --git a/include/dt-bindings/interconnect/qcom,qcs404.h b/include/dt-bindings/interconnect/qcom,qcs404.h
new file mode 100644
index 000000000000..960f6e39c5f2
--- /dev/null
+++ b/include/dt-bindings/interconnect/qcom,qcs404.h
@@ -0,0 +1,88 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Qualcomm interconnect IDs
+ *
+ * Copyright (c) 2019, Linaro Ltd.
+ * Author: Georgi Djakov <[email protected]>
+ */
+
+#ifndef __DT_BINDINGS_INTERCONNECT_QCOM_QCS404_H
+#define __DT_BINDINGS_INTERCONNECT_QCOM_QCS404_H
+
+#define MASTER_AMPSS_M0 0
+#define MASTER_OXILI 1
+#define MASTER_MDP_PORT0 2
+#define MASTER_SNOC_BIMC_1 3
+#define MASTER_TCU_0 4
+#define SLAVE_EBI_CH0 5
+#define SLAVE_BIMC_SNOC 6
+
+#define MASTER_SPDM 0
+#define MASTER_BLSP_1 1
+#define MASTER_BLSP_2 2
+#define MASTER_XI_USB_HS1 3
+#define MASTER_CRYPT0 4
+#define MASTER_SDCC_1 5
+#define MASTER_SDCC_2 6
+#define MASTER_SNOC_PCNOC 7
+#define MASTER_QPIC 8
+#define PCNOC_INT_0 9
+#define PCNOC_INT_2 10
+#define PCNOC_INT_3 11
+#define PCNOC_S_0 12
+#define PCNOC_S_1 13
+#define PCNOC_S_2 14
+#define PCNOC_S_3 15
+#define PCNOC_S_4 16
+#define PCNOC_S_6 17
+#define PCNOC_S_7 18
+#define PCNOC_S_8 19
+#define PCNOC_S_9 20
+#define PCNOC_S_10 21
+#define PCNOC_S_11 22
+#define SLAVE_SPDM 23
+#define SLAVE_PDM 24
+#define SLAVE_PRNG 25
+#define SLAVE_TCSR 26
+#define SLAVE_SNOC_CFG 27
+#define SLAVE_MESSAGE_RAM 28
+#define SLAVE_DISP_SS_CFG 29
+#define SLAVE_GPU_CFG 30
+#define SLAVE_BLSP_1 31
+#define SLAVE_BLSP_2 32
+#define SLAVE_TLMM_NORTH 33
+#define SLAVE_PCIE 34
+#define SLAVE_ETHERNET 35
+#define SLAVE_TLMM_EAST 36
+#define SLAVE_TCU 37
+#define SLAVE_PMIC_ARB 38
+#define SLAVE_SDCC_1 39
+#define SLAVE_SDCC_2 40
+#define SLAVE_TLMM_SOUTH 41
+#define SLAVE_USB_HS 42
+#define SLAVE_USB3 43
+#define SLAVE_CRYPTO_0_CFG 44
+#define SLAVE_PCNOC_SNOC 45
+
+#define MASTER_QDSS_BAM 0
+#define MASTER_BIMC_SNOC 1
+#define MASTER_PCNOC_SNOC 2
+#define MASTER_QDSS_ETR 3
+#define MASTER_EMAC 4
+#define MASTER_PCIE 5
+#define MASTER_USB3 6
+#define QDSS_INT 7
+#define SNOC_INT_0 8
+#define SNOC_INT_1 9
+#define SNOC_INT_2 10
+#define SLAVE_KPSS_AHB 11
+#define SLAVE_WCSS 12
+#define SLAVE_SNOC_BIMC_1 13
+#define SLAVE_IMEM 14
+#define SLAVE_SNOC_PCNOC 15
+#define SLAVE_QDSS_STM 16
+#define SLAVE_CATS_0 17
+#define SLAVE_CATS_1 18
+#define SLAVE_LPASS 19
+
+#endif

2019-04-05 03:58:17

by Georgi Djakov

[permalink] [raw]
Subject: [PATCH 3/3] arm64: dts: qcs404: Add interconnect provider DT nodes

Add the DT nodes for the network-on-chip interconnect buses found
on qcs404-based platforms.

Signed-off-by: Georgi Djakov <[email protected]>
---
arch/arm64/boot/dts/qcom/qcs404.dtsi | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi
index e8fd26633d57..706e918f79f6 100644
--- a/arch/arm64/boot/dts/qcom/qcs404.dtsi
+++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
// Copyright (c) 2018, Linaro Limited

+#include <dt-bindings/interconnect/qcom,qcs404.h>
#include <dt-bindings/interrupt-controller/arm-gic.h>
#include <dt-bindings/clock/qcom,gcc-qcs404.h>
#include <dt-bindings/clock/qcom,rpmcc.h>
@@ -226,6 +227,30 @@
compatible = "qcom,rpm-qcs404";
qcom,glink-channels = "rpm_requests";

+ bimc: interconnect@0 {
+ compatible = "qcom,qcs404-bimc";
+ #interconnect-cells = <1>;
+ clock-names = "bus_clk", "bus_a_clk";
+ clocks = <&rpmcc RPM_SMD_BIMC_CLK>,
+ <&rpmcc RPM_SMD_BIMC_A_CLK>;
+ };
+
+ pcnoc: interconnect@1 {
+ compatible = "qcom,qcs404-pcnoc";
+ #interconnect-cells = <1>;
+ clock-names = "bus_clk", "bus_a_clk";
+ clocks = <&rpmcc RPM_SMD_PNOC_CLK>,
+ <&rpmcc RPM_SMD_PNOC_A_CLK>;
+ };
+
+ snoc: interconnect@2 {
+ compatible = "qcom,qcs404-snoc";
+ #interconnect-cells = <1>;
+ clock-names = "bus_clk", "bus_a_clk";
+ clocks = <&rpmcc RPM_SMD_SNOC_CLK>,
+ <&rpmcc RPM_SMD_SNOC_A_CLK>;
+ };
+
rpmcc: clock-controller {
compatible = "qcom,rpmcc-qcs404";
#clock-cells = <1>;

2019-04-05 14:35:11

by Bjorn Andersson

[permalink] [raw]
Subject: Re: [PATCH 1/3] dt-bindings: interconnect: Add Qualcomm QCS404 DT bindings

On Fri 05 Apr 10:54 +07 2019, Georgi Djakov wrote:

> The Qualcomm QCS404 platform has several buses that could be controlled
> and tuned according to the bandwidth demand.
>
> Signed-off-by: Georgi Djakov <[email protected]>
> ---
> .../bindings/interconnect/qcom,qcs404.txt | 45 +++++++++++++++++++
> 1 file changed, 45 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/interconnect/qcom,qcs404.txt
>
> diff --git a/Documentation/devicetree/bindings/interconnect/qcom,qcs404.txt b/Documentation/devicetree/bindings/interconnect/qcom,qcs404.txt
> new file mode 100644
> index 000000000000..2ea63ea827d7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interconnect/qcom,qcs404.txt
> @@ -0,0 +1,45 @@
> +Qualcomm QCS404 Network-On-Chip interconnect driver binding
> +-----------------------------------------------------------
> +
> +Required properties :
> +- compatible : shall contain only one of the following:
> + "qcom,qcs404-bimc"

As this is a hardware block available in mmio register space I think you
better represent this on the mmio (soc) bus - and then represent the
link to rpm as a child node of the rpm.

Apart from that this looks good.

Regards,
Bjorn

> + "qcom,qcs404-pcnoc"
> + "qcom,qcs404-snoc"
> +- #interconnect-cells : should contain 1
> +
> +Optional properties :
> +clocks : list of phandles and specifiers to all interconnect bus clocks
> +clock-names : clock names should include both "bus_clk" and "bus_a_clk"
> +
> +Example:
> +
> +rpm-glink {
> + ...
> + rpm_requests: glink-channel {
> + ...
> + bimc: interconnect@0 {
> + compatible = "qcom,qcs404-bimc";
> + #interconnect-cells = <1>;
> + clock-names = "bus_clk", "bus_a_clk";
> + clocks = <&rpmcc RPM_SMD_BIMC_CLK>,
> + <&rpmcc RPM_SMD_BIMC_A_CLK>;
> + };
> +
> + pnoc: interconnect@1 {
> + compatible = "qcom,qcs404-pcnoc";
> + #interconnect-cells = <1>;
> + clock-names = "bus_clk", "bus_a_clk";
> + clocks = <&rpmcc RPM_SMD_PNOC_CLK>,
> + <&rpmcc RPM_SMD_PNOC_A_CLK>;
> + };
> +
> + snoc: interconnect@2 {
> + compatible = "qcom,qcs404-snoc";
> + #interconnect-cells = <1>;
> + clock-names = "bus_clk", "bus_a_clk";
> + clocks = <&rpmcc RPM_SMD_SNOC_CLK>,
> + <&rpmcc RPM_SMD_SNOC_A_CLK>;
> + };
> + };
> +};

2019-04-05 14:59:09

by Bjorn Andersson

[permalink] [raw]
Subject: Re: [PATCH 2/3] interconnect: qcom: Add QCS404 interconnect provider driver

On Fri 05 Apr 10:54 +07 2019, Georgi Djakov wrote:
[..]
> diff --git a/drivers/interconnect/qcom/qcs404.c b/drivers/interconnect/qcom/qcs404.c
> new file mode 100644
> index 000000000000..42d36db13ec0
> --- /dev/null
> +++ b/drivers/interconnect/qcom/qcs404.c
> @@ -0,0 +1,488 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 Linaro Ltd
> + */
> +
> +#include <dt-bindings/interconnect/qcom,qcs404.h>
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/interconnect-provider.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/soc/qcom/smd-rpm.h>
> +
> +#include "qcs404_ids.h"
> +
> +#define RPM_BUS_MASTER_REQ 0x73616d62
> +#define RPM_BUS_SLAVE_REQ 0x766c7362
> +#define RPM_KEY_BW 0x00007762
> +
> +#define to_qcom_provider(_provider) \
> + container_of(_provider, struct qcom_icc_provider, provider)
> +
> +struct qcom_smd_rpm *qcs404_rpm;

static

> +
[..]
> +#define DEFINE_QNODE(_name, _id, _buswidth, _mas_rpm_id, _slv_rpm_id, \
> + _numlinks, ...) \
> + static struct qcom_icc_node _name = { \
> + .name = #_name, \
> + .id = _id, \
> + .buswidth = _buswidth, \
> + .mas_rpm_id = _mas_rpm_id, \
> + .slv_rpm_id = _slv_rpm_id, \
> + .num_links = _numlinks, \

If you write this as ARRAY_SIZE(((int[]){ __VA_ARGS__ })), you don't
need the manually entered _numlinks number.

> + .links = { __VA_ARGS__ }, \
> + }
> +
[..]
> +static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
> +{
> + struct qcom_icc_provider *qp;
> + struct qcom_icc_node *qn;
> + struct icc_provider *provider;
> + struct icc_node *n;
> + u64 sum_bw;
> + u64 max_peak_bw;
> + u64 rate;
> + u32 agg_avg = 0;
> + u32 agg_peak = 0;
> + int ret = 0;
> +
> + qn = src->data;
> + provider = src->provider;
> + qp = to_qcom_provider(provider);
> +
> + list_for_each_entry(n, &provider->nodes, node_list)
> + qcom_icc_aggregate(n, n->avg_bw, n->peak_bw,
> + &agg_avg, &agg_peak);
> +
> + sum_bw = icc_units_to_bps(agg_avg);
> + max_peak_bw = icc_units_to_bps(agg_peak);
> +
> + /* send message to the RPM processor */
> + if (qn->mas_rpm_id != -1) {
> + ret = qcom_icc_rpm_smd_send(QCOM_SMD_RPM_ACTIVE_STATE,
> + RPM_BUS_MASTER_REQ,
> + qn->mas_rpm_id,
> + sum_bw);
> + if (ret) {
> + pr_err("qcom_icc_rpm_smd_send mas %d error %d\n",
> + qn->mas_rpm_id, ret);
> + return ret;
> + }
> + }
> +
> + if (qn->slv_rpm_id != -1) {
> + ret = qcom_icc_rpm_smd_send(QCOM_SMD_RPM_ACTIVE_STATE,
> + RPM_BUS_SLAVE_REQ,
> + qn->slv_rpm_id,
> + sum_bw);
> + if (ret) {
> + pr_err("qcom_icc_rpm_smd_send slv error %d\n",
> + ret);
> + return ret;
> + }
> + }
> +
> + rate = max(sum_bw, max_peak_bw);
> +
> + do_div(rate, qn->buswidth);
> +
> + if (qn->rate != rate) {
> + ret = clk_set_rate(qp->bus_clk, rate);
> + if (ret) {
> + pr_err("set clk rate %lld error %d\n", rate, ret);
> + return ret;
> + }
> +
> + ret = clk_set_rate(qp->bus_a_clk, rate);
> + if (ret) {
> + pr_err("set clk rate %lld error %d\n", rate, ret);
> + return ret;
> + }
> +
> + qn->rate = rate;
> + }
> +
> + return ret;

ret is 0, return 0; and you can skip setting ret = 0 above.

> +}
> +
> +static int qnoc_probe(struct platform_device *pdev)
> +{
> + const struct qcom_icc_desc *desc;
> + struct icc_onecell_data *data;
> + struct icc_provider *provider;
> + struct qcom_icc_node **qnodes;
> + struct qcom_icc_provider *qp;
> + struct icc_node *node;
> + size_t num_nodes, i;
> + int ret;
> +
> + /* wait for RPM */

This isn't waiting, it's getting the reference. That said if you make
these sit on mmio bus you would need to EPROBE_DEFER on the rpm-child
not being probed yet (and by that it would be a wait).

> + qcs404_rpm = dev_get_drvdata(pdev->dev.parent);
> + if (!qcs404_rpm) {
> + dev_err(&pdev->dev, "unable to retrieve handle to RPM\n");
> + return -ENODEV;
> + }
> +
> + desc = of_device_get_match_data(&pdev->dev);
> + if (!desc)
> + return -EINVAL;
> +
> + qnodes = desc->nodes;
> + num_nodes = desc->num_nodes;
> +
> + qp = devm_kzalloc(&pdev->dev, sizeof(*qp), GFP_KERNEL);
> + if (!qp)
> + return -ENOMEM;
> +
> + data = devm_kcalloc(&pdev->dev, num_nodes, sizeof(*node), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + qp->bus_clk = devm_clk_get(&pdev->dev, "bus_clk");

Please use the clk_bulk interface instead.

> + if (IS_ERR(qp->bus_clk))
> + return PTR_ERR(qp->bus_clk);
> +
> + ret = clk_prepare_enable(qp->bus_clk);
> + if (ret) {
> + dev_err(&pdev->dev, "error enabling bus_clk: %d\n", ret);

clk_prepare_enable() will complain if it fails to enable the clock, so
no need to add another print.

> + return ret;
> + }
> +
> + qp->bus_a_clk = devm_clk_get(&pdev->dev, "bus_a_clk");
> + if (IS_ERR(qp->bus_a_clk))
> + return PTR_ERR(qp->bus_a_clk);
> +
> + ret = clk_prepare_enable(qp->bus_a_clk);
> + if (ret) {
> + dev_err(&pdev->dev, "error enabling bus_a_clk: %d\n", ret);
> + clk_disable_unprepare(qp->bus_clk);
> + return ret;
> + }
> +
> + provider = &qp->provider;
> + INIT_LIST_HEAD(&provider->nodes);
> + provider->dev = &pdev->dev;
> + provider->set = qcom_icc_set;
> + provider->aggregate = qcom_icc_aggregate;
> + provider->xlate = of_icc_xlate_onecell;
> + provider->data = data;
> +
> + ret = icc_provider_add(provider);
> + if (ret) {
> + dev_err(&pdev->dev, "error adding interconnect provider\n");
> + clk_disable_unprepare(qp->bus_clk);
> + clk_disable_unprepare(qp->bus_a_clk);
> + return ret;
> + }
> +
> + for (i = 0; i < num_nodes; i++) {
> + size_t j;
> +
> + node = icc_node_create(qnodes[i]->id);
> + if (IS_ERR(node)) {
> + ret = PTR_ERR(node);
> + goto err;
> + }
> +
> + node->name = qnodes[i]->name;
> + node->data = qnodes[i];
> + icc_node_add(node, provider);
> +
> + dev_dbg(&pdev->dev, "registered node %s\n", node->name);
> +
> + /* populate links */
> + for (j = 0; j < qnodes[i]->num_links; j++)
> + icc_link_create(node, qnodes[i]->links[j]);
> +
> + data->nodes[i] = node;
> + }
> + data->num_nodes = num_nodes;
> +
> + platform_set_drvdata(pdev, qp);
> +
> + return ret;

ret is 0 here, so just return 0;

> +err:
> + list_for_each_entry(node, &provider->nodes, node_list) {
> + icc_node_del(node);
> + icc_node_destroy(node->id);
> + }
> + clk_disable_unprepare(qp->bus_clk);
> + clk_disable_unprepare(qp->bus_a_clk);
> + icc_provider_del(provider);
> +
> + return ret;
> +}
[..]
> diff --git a/drivers/interconnect/qcom/qcs404_ids.h b/drivers/interconnect/qcom/qcs404_ids.h

You use these defines in the driver, so I think this file should be the
one in include/dt-bindings...

[..]
> diff --git a/include/dt-bindings/interconnect/qcom,qcs404.h b/include/dt-bindings/interconnect/qcom,qcs404.h

...and that this is an accidental include(?)

Regards,
Bjorn

2019-04-05 15:48:30

by Georgi Djakov

[permalink] [raw]
Subject: Re: [PATCH 1/3] dt-bindings: interconnect: Add Qualcomm QCS404 DT bindings

Hi Bjorn,

On 4/5/19 21:32, Bjorn Andersson wrote:
> On Fri 05 Apr 10:54 +07 2019, Georgi Djakov wrote:
>
>> The Qualcomm QCS404 platform has several buses that could be controlled
>> and tuned according to the bandwidth demand.
>>
>> Signed-off-by: Georgi Djakov <[email protected]>
>> ---
>> .../bindings/interconnect/qcom,qcs404.txt | 45 +++++++++++++++++++
>> 1 file changed, 45 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/interconnect/qcom,qcs404.txt
>>
>> diff --git a/Documentation/devicetree/bindings/interconnect/qcom,qcs404.txt b/Documentation/devicetree/bindings/interconnect/qcom,qcs404.txt
>> new file mode 100644
>> index 000000000000..2ea63ea827d7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/interconnect/qcom,qcs404.txt
>> @@ -0,0 +1,45 @@
>> +Qualcomm QCS404 Network-On-Chip interconnect driver binding
>> +-----------------------------------------------------------
>> +
>> +Required properties :
>> +- compatible : shall contain only one of the following:
>> + "qcom,qcs404-bimc"
>
> As this is a hardware block available in mmio register space I think you
> better represent this on the mmio (soc) bus - and then represent the
> link to rpm as a child node of the rpm.

The mmio register space is not used for expressing bandwidth needs, but
contains mostly QoS related stuff. We do not support QoS for now and
that's why i haven't included it. When we decide to support QoS, we can
add the nodes for the QoS registers and then reference them with some DT
property like qcom,qos = <&bimc_qos>;

Thanks,
Georgi

>
> Apart from that this looks good.
>
> Regards,
> Bjorn
>
>> + "qcom,qcs404-pcnoc"
>> + "qcom,qcs404-snoc"
>> +- #interconnect-cells : should contain 1
>> +
>> +Optional properties :
>> +clocks : list of phandles and specifiers to all interconnect bus clocks
>> +clock-names : clock names should include both "bus_clk" and "bus_a_clk"
>> +
>> +Example:
>> +
>> +rpm-glink {
>> + ...
>> + rpm_requests: glink-channel {
>> + ...
>> + bimc: interconnect@0 {
>> + compatible = "qcom,qcs404-bimc";
>> + #interconnect-cells = <1>;
>> + clock-names = "bus_clk", "bus_a_clk";
>> + clocks = <&rpmcc RPM_SMD_BIMC_CLK>,
>> + <&rpmcc RPM_SMD_BIMC_A_CLK>;
>> + };
>> +
>> + pnoc: interconnect@1 {
>> + compatible = "qcom,qcs404-pcnoc";
>> + #interconnect-cells = <1>;
>> + clock-names = "bus_clk", "bus_a_clk";
>> + clocks = <&rpmcc RPM_SMD_PNOC_CLK>,
>> + <&rpmcc RPM_SMD_PNOC_A_CLK>;
>> + };
>> +
>> + snoc: interconnect@2 {
>> + compatible = "qcom,qcs404-snoc";
>> + #interconnect-cells = <1>;
>> + clock-names = "bus_clk", "bus_a_clk";
>> + clocks = <&rpmcc RPM_SMD_SNOC_CLK>,
>> + <&rpmcc RPM_SMD_SNOC_A_CLK>;
>> + };
>> + };
>> +};

2019-04-08 15:41:55

by Georgi Djakov

[permalink] [raw]
Subject: Re: [PATCH 2/3] interconnect: qcom: Add QCS404 interconnect provider driver

Hi Bjorn,

Thanks for reviewing!

On 4/5/19 17:57, Bjorn Andersson wrote:
> On Fri 05 Apr 10:54 +07 2019, Georgi Djakov wrote:
> [..]
>> diff --git a/drivers/interconnect/qcom/qcs404.c b/drivers/interconnect/qcom/qcs404.c
>> new file mode 100644
>> index 000000000000..42d36db13ec0
>> --- /dev/null
>> +++ b/drivers/interconnect/qcom/qcs404.c
>> @@ -0,0 +1,488 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2019 Linaro Ltd
>> + */
>> +
>> +#include <dt-bindings/interconnect/qcom,qcs404.h>
>> +#include <linux/clk.h>
>> +#include <linux/device.h>
>> +#include <linux/interconnect-provider.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/soc/qcom/smd-rpm.h>
>> +
>> +#include "qcs404_ids.h"
>> +
>> +#define RPM_BUS_MASTER_REQ 0x73616d62
>> +#define RPM_BUS_SLAVE_REQ 0x766c7362
>> +#define RPM_KEY_BW 0x00007762
>> +
>> +#define to_qcom_provider(_provider) \
>> + container_of(_provider, struct qcom_icc_provider, provider)
>> +
>> +struct qcom_smd_rpm *qcs404_rpm;
>
> static

Ok!

>> +
> [..]
>> +#define DEFINE_QNODE(_name, _id, _buswidth, _mas_rpm_id, _slv_rpm_id, \
>> + _numlinks, ...) \
>> + static struct qcom_icc_node _name = { \
>> + .name = #_name, \
>> + .id = _id, \
>> + .buswidth = _buswidth, \
>> + .mas_rpm_id = _mas_rpm_id, \
>> + .slv_rpm_id = _slv_rpm_id, \
>> + .num_links = _numlinks, \
>
> If you write this as ARRAY_SIZE(((int[]){ __VA_ARGS__ })), you don't
> need the manually entered _numlinks number.

This is a really nice idea!

>
>> + .links = { __VA_ARGS__ }, \
>> + }
>> +
> [..]
[..]
>> +
>> + return ret;
>
> ret is 0, return 0; and you can skip setting ret = 0 above.

Ok!

>> +}
>> +
>> +static int qnoc_probe(struct platform_device *pdev)
>> +{
>> + const struct qcom_icc_desc *desc;
>> + struct icc_onecell_data *data;
>> + struct icc_provider *provider;
>> + struct qcom_icc_node **qnodes;
>> + struct qcom_icc_provider *qp;
>> + struct icc_node *node;
>> + size_t num_nodes, i;
>> + int ret;
>> +
>> + /* wait for RPM */
>
> This isn't waiting, it's getting the reference. That said if you make
> these sit on mmio bus you would need to EPROBE_DEFER on the rpm-child
> not being probed yet (and by that it would be a wait).

Agree that it's a reference. The mmio registers are mostly qos related
and seem not required for just requesting bandwidth, so we can add them
later if we want to support priorities and different port modes like
limiter, regulator etc.

>> + qcs404_rpm = dev_get_drvdata(pdev->dev.parent);
>> + if (!qcs404_rpm) {
>> + dev_err(&pdev->dev, "unable to retrieve handle to RPM\n");
>> + return -ENODEV;
>> + }
>> +
>> + desc = of_device_get_match_data(&pdev->dev);
>> + if (!desc)
>> + return -EINVAL;
>> +
>> + qnodes = desc->nodes;
>> + num_nodes = desc->num_nodes;
>> +
>> + qp = devm_kzalloc(&pdev->dev, sizeof(*qp), GFP_KERNEL);
>> + if (!qp)
>> + return -ENOMEM;
>> +
>> + data = devm_kcalloc(&pdev->dev, num_nodes, sizeof(*node), GFP_KERNEL);
>> + if (!data)
>> + return -ENOMEM;
>> +
>> + qp->bus_clk = devm_clk_get(&pdev->dev, "bus_clk");
>
> Please use the clk_bulk interface instead.

Ok, will do.

>> + if (IS_ERR(qp->bus_clk))
>> + return PTR_ERR(qp->bus_clk);
>> +
>> + ret = clk_prepare_enable(qp->bus_clk);
>> + if (ret) {
>> + dev_err(&pdev->dev, "error enabling bus_clk: %d\n", ret);
>
> clk_prepare_enable() will complain if it fails to enable the clock, so
> no need to add another print.

Ok, right!

[..]>> +
>> + platform_set_drvdata(pdev, qp);
>> +
>> + return ret;
>
> ret is 0 here, so just return 0;

Ok!

>> +err:
>> + list_for_each_entry(node, &provider->nodes, node_list) {
>> + icc_node_del(node);
>> + icc_node_destroy(node->id);
>> + }
>> + clk_disable_unprepare(qp->bus_clk);
>> + clk_disable_unprepare(qp->bus_a_clk);
>> + icc_provider_del(provider);
>> +
>> + return ret;
>> +}
> [..]
>> diff --git a/drivers/interconnect/qcom/qcs404_ids.h b/drivers/interconnect/qcom/qcs404_ids.h
>
> You use these defines in the driver, so I think this file should be the
> one in include/dt-bindings...

The ids in this header are in a single global namespace in order to
build the internal topology and could be used for drivers that support
only platform data (although not sure if there would be any).

>
> [..]
>> diff --git a/include/dt-bindings/interconnect/qcom,qcs404.h b/include/dt-bindings/interconnect/qcom,qcs404.h
>

These header is using per NoC local ids and should be used on DT enabled
platforms.

Thanks,
Georgi

2019-04-08 22:56:30

by Bjorn Andersson

[permalink] [raw]
Subject: Re: [PATCH 1/3] dt-bindings: interconnect: Add Qualcomm QCS404 DT bindings

On Fri 05 Apr 08:46 PDT 2019, Georgi Djakov wrote:

> Hi Bjorn,
>
> On 4/5/19 21:32, Bjorn Andersson wrote:
> > On Fri 05 Apr 10:54 +07 2019, Georgi Djakov wrote:
> >
> >> The Qualcomm QCS404 platform has several buses that could be controlled
> >> and tuned according to the bandwidth demand.
> >>
> >> Signed-off-by: Georgi Djakov <[email protected]>
> >> ---
> >> .../bindings/interconnect/qcom,qcs404.txt | 45 +++++++++++++++++++
> >> 1 file changed, 45 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/interconnect/qcom,qcs404.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/interconnect/qcom,qcs404.txt b/Documentation/devicetree/bindings/interconnect/qcom,qcs404.txt
> >> new file mode 100644
> >> index 000000000000..2ea63ea827d7
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/interconnect/qcom,qcs404.txt
> >> @@ -0,0 +1,45 @@
> >> +Qualcomm QCS404 Network-On-Chip interconnect driver binding
> >> +-----------------------------------------------------------
> >> +
> >> +Required properties :
> >> +- compatible : shall contain only one of the following:
> >> + "qcom,qcs404-bimc"
> >
> > As this is a hardware block available in mmio register space I think you
> > better represent this on the mmio (soc) bus - and then represent the
> > link to rpm as a child node of the rpm.
>
> The mmio register space is not used for expressing bandwidth needs, but
> contains mostly QoS related stuff. We do not support QoS for now and
> that's why i haven't included it. When we decide to support QoS, we can
> add the nodes for the QoS registers and then reference them with some DT
> property like qcom,qos = <&bimc_qos>;
>

The fact that QoS support is not implemented today is an implementation
detail, it does not relate to how we're describing the hardware in
DeviceTree. Adding three new nodes under soc{} and referencing these
(and potentially duplicating the clocks properties in the two sets of
nodes?) in the future, rather than just describing the hardware
appropriately today seems odd to me.

Further more, as I shared a while back the aggregation code related to
the RPM would better be shared between 404, 410, 820 and 835. So to me
it makes sense to even on the implementation side split this is NoC part
and RPM part, from the beginning.

Regards,
Bjorn

> Thanks,
> Georgi
>
> >
> > Apart from that this looks good.
> >
> > Regards,
> > Bjorn
> >
> >> + "qcom,qcs404-pcnoc"
> >> + "qcom,qcs404-snoc"
> >> +- #interconnect-cells : should contain 1
> >> +
> >> +Optional properties :
> >> +clocks : list of phandles and specifiers to all interconnect bus clocks
> >> +clock-names : clock names should include both "bus_clk" and "bus_a_clk"
> >> +
> >> +Example:
> >> +
> >> +rpm-glink {
> >> + ...
> >> + rpm_requests: glink-channel {
> >> + ...
> >> + bimc: interconnect@0 {
> >> + compatible = "qcom,qcs404-bimc";
> >> + #interconnect-cells = <1>;
> >> + clock-names = "bus_clk", "bus_a_clk";
> >> + clocks = <&rpmcc RPM_SMD_BIMC_CLK>,
> >> + <&rpmcc RPM_SMD_BIMC_A_CLK>;
> >> + };
> >> +
> >> + pnoc: interconnect@1 {
> >> + compatible = "qcom,qcs404-pcnoc";
> >> + #interconnect-cells = <1>;
> >> + clock-names = "bus_clk", "bus_a_clk";
> >> + clocks = <&rpmcc RPM_SMD_PNOC_CLK>,
> >> + <&rpmcc RPM_SMD_PNOC_A_CLK>;
> >> + };
> >> +
> >> + snoc: interconnect@2 {
> >> + compatible = "qcom,qcs404-snoc";
> >> + #interconnect-cells = <1>;
> >> + clock-names = "bus_clk", "bus_a_clk";
> >> + clocks = <&rpmcc RPM_SMD_SNOC_CLK>,
> >> + <&rpmcc RPM_SMD_SNOC_A_CLK>;
> >> + };
> >> + };
> >> +};

2019-04-09 03:49:58

by Bjorn Andersson

[permalink] [raw]
Subject: Re: [PATCH 2/3] interconnect: qcom: Add QCS404 interconnect provider driver

On Mon 08 Apr 07:33 PDT 2019, Georgi Djakov wrote:
> On 4/5/19 17:57, Bjorn Andersson wrote:
> > On Fri 05 Apr 10:54 +07 2019, Georgi Djakov wrote:
> > [..]
[..]
> >> diff --git a/drivers/interconnect/qcom/qcs404_ids.h b/drivers/interconnect/qcom/qcs404_ids.h
> >
> > You use these defines in the driver, so I think this file should be the
> > one in include/dt-bindings...
>
> The ids in this header are in a single global namespace in order to
> build the internal topology and could be used for drivers that support
> only platform data (although not sure if there would be any).
>

As you say these numbers could be used by drivers on non-DT enabled
platforms, but for that this include file should be in
include/linux/interconnect. That said, there are no such Qualcomm
platforms, so these numbers will only ever be used internally in the
qcs404.c provider, so it would be better to just define them in that
file - to remove the risk of confusion.

> >
> > [..]
> >> diff --git a/include/dt-bindings/interconnect/qcom,qcs404.h b/include/dt-bindings/interconnect/qcom,qcs404.h
> >
>
> These header is using per NoC local ids and should be used on DT enabled
> platforms.
>

I had missed that you implemented support for xlating NoC-specific ids,
so this makes sense now, nice. I presume we won't ever include files in
a way where these defines collide - so this looks good.

Regards,
Bjorn

2019-04-09 10:28:50

by Georgi Djakov

[permalink] [raw]
Subject: Re: [PATCH 2/3] interconnect: qcom: Add QCS404 interconnect provider driver

Hi Bjorn,

On 4/9/19 06:27, Bjorn Andersson wrote:
> On Mon 08 Apr 07:33 PDT 2019, Georgi Djakov wrote:
>> On 4/5/19 17:57, Bjorn Andersson wrote:
>>> On Fri 05 Apr 10:54 +07 2019, Georgi Djakov wrote:
>>> [..]
> [..]
>>>> diff --git a/drivers/interconnect/qcom/qcs404_ids.h b/drivers/interconnect/qcom/qcs404_ids.h
>>>
>>> You use these defines in the driver, so I think this file should be the
>>> one in include/dt-bindings...
>>
>> The ids in this header are in a single global namespace in order to
>> build the internal topology and could be used for drivers that support
>> only platform data (although not sure if there would be any).
>>
>
> As you say these numbers could be used by drivers on non-DT enabled
> platforms, but for that this include file should be in
> include/linux/interconnect. That said, there are no such Qualcomm
> platforms, so these numbers will only ever be used internally in the
> qcs404.c provider, so it would be better to just define them in that
> file - to remove the risk of confusion.

Agreed, will put them in the same file.

>>>
>>> [..]
>>>> diff --git a/include/dt-bindings/interconnect/qcom,qcs404.h b/include/dt-bindings/interconnect/qcom,qcs404.h
>>>
>>
>> These header is using per NoC local ids and should be used on DT enabled
>> platforms.
>>
>
> I had missed that you implemented support for xlating NoC-specific ids,
> so this makes sense now, nice. I presume we won't ever include files in
> a way where these defines collide - so this looks good.
Thanks for the review!
Georgi