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
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
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 ..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
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +#include "qcs404_ids.h"
>> +
>> +#define RPM_BUS_MASTER_REQ 0x73616d62
>> +#define RPM_BUS_SLAVE_REQ 0x766c7362
>> +#define RPM_KEY_BW 0x7762
>> +
>> +#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(>dev, "unable to retrieve handle to RPM\n");
>> +return -ENODEV;
>> +}
>> +
>> +desc = of_device_get_match_data(>dev);
>> +if (!desc)
>> +return -EINVAL;
>> +
>> +qnodes = desc->nodes;
>> +num_nodes = desc->num_nodes;
>> +
>> +qp = devm_kzalloc(>dev, sizeof(*qp), GFP_KERNEL);
>> +if (!qp)
>> +return -ENOMEM;
>> +
>> +data = devm_kcalloc(>dev, num_nodes, sizeof(*node), GFP_KERNEL);
>> +if (!data)
>> +return -ENOMEM;
>> +
>> +qp->bus_clk = devm_clk_get(>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(>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, >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
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 ..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
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "qcs404_ids.h"
> +
> +#define RPM_BUS_MASTER_REQ 0x73616d62
> +#define RPM_BUS_SLAVE_REQ0x766c7362
> +#define RPM_KEY_BW 0x7762
> +
> +#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, >nodes, node_list)
> + qcom_icc_aggregate(n, n->avg_bw, n->peak_bw,
> +_avg, _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(>dev, "unable to retrieve handle to RPM\n");
> + return -ENODEV;
> + }
> +
> + desc = of_device_get_match_data(>dev);
> +
[PATCH 2/3] interconnect: qcom: Add QCS404 interconnect provider driver
From: Bjorn Andersson
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
Signed-off-by: Georgi Djakov
---
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 ..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
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "qcs404_ids.h"
+
+#define RPM_BUS_MASTER_REQ 0x73616d62
+#define RPM_BUS_SLAVE_REQ 0x766c7362
+#define RPM_KEY_BW 0x7762
+
+#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);
