[PATCH v3 1/2] dt-bindings: interconnect: Add OSM L3 DT bindings

[PATCH v3 1/2] dt-bindings: interconnect: Add OSM L3 DT bindings

[Sibi Sankar]

Add bindings for Operating State Manager (OSM) L3 interconnect provider
on SDM845 SoCs.

Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---
 .../bindings/interconnect/qcom,osm-l3.yaml    | 56 +++++++++++++++++++
 .../dt-bindings/interconnect/qcom,osm-l3.h    | 12 ++++
 2 files changed, 68 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
 create mode 100644 include/dt-bindings/interconnect/qcom,osm-l3.h

diff --git a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
new file mode 100644
index 0000000000000..fec8289ceeeed
--- /dev/null
+++ b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
@@ -0,0 +1,56 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interconnect/qcom,osm-l3.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Operating State Manager (OSM) L3 Interconnect Provider
+
+maintainers:
+  - Sibi Sankar <sibis@codeaurora.org>
+
+description:
+  L3 cache bandwidth requirements on Qualcomm SoCs is serviced by the OSM.
+  The OSM L3 interconnect provider aggregates the L3 bandwidth requests
+  from CPU/GPU and relays it to the OSM.
+
+properties:
+  compatible:
+    const: "qcom,sdm845-osm-l3"
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: xo clock
+      - description: alternate clock
+
+  clock-names:
+    items:
+      - const: xo
+      - const: alternate
+
+  '#interconnect-cells':
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - '#interconnect-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    osm_l3: interconnect@17d41000 {
+      compatible = "qcom,sdm845-osm-l3";
+      reg = <0x17d41000 0x1400>;
+
+      clocks = <&rpmhcc 0>, <&gcc 165>;
+      clock-names = "xo", "alternate";
+
+      #interconnect-cells = <1>;
+    };
diff --git a/include/dt-bindings/interconnect/qcom,osm-l3.h b/include/dt-bindings/interconnect/qcom,osm-l3.h
new file mode 100644
index 0000000000000..54858ff7674d7
--- /dev/null
+++ b/include/dt-bindings/interconnect/qcom,osm-l3.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2019 The Linux Foundation. All rights reserved.
+ */
+
+#ifndef __DT_BINDINGS_INTERCONNECT_QCOM_OSM_L3_H
+#define __DT_BINDINGS_INTERCONNECT_QCOM_OSM_L3_H
+
+#define MASTER_OSM_L3_APPS	0
+#define SLAVE_OSM_L3		1
+
+#endif
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 1/2] dt-bindings: interconnect: Add OSM L3 DT bindings

[Stephen Boyd]

Quoting Sibi Sankar (2019-11-18 07:45:21)
> diff --git a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
> new file mode 100644
> index 0000000000000..fec8289ceeeed
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
> @@ -0,0 +1,56 @@
> +examples:
> +  - |
> +    osm_l3: interconnect@17d41000 {
> +      compatible = "qcom,sdm845-osm-l3";
> +      reg = <0x17d41000 0x1400>;
> +
> +      clocks = <&rpmhcc 0>, <&gcc 165>;

Can you use #define names here? That would make it clearer what sort of
clk is expected here.

> +      clock-names = "xo", "alternate";
> +
> +      #interconnect-cells = <1>;
> +    };

Re: [PATCH v3 1/2] dt-bindings: interconnect: Add OSM L3 DT bindings

[sibis]

Hey Stephen,

Thanks for taking time to
review the series.

On 2019-11-19 06:01, Stephen Boyd wrote:
> Quoting Sibi Sankar (2019-11-18 07:45:21)
>> diff --git 
>> a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml 
>> b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
>> new file mode 100644
>> index 0000000000000..fec8289ceeeed
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
>> @@ -0,0 +1,56 @@
>> +examples:
>> +  - |
>> +    osm_l3: interconnect@17d41000 {
>> +      compatible = "qcom,sdm845-osm-l3";
>> +      reg = <0x17d41000 0x1400>;
>> +
>> +      clocks = <&rpmhcc 0>, <&gcc 165>;
> 
> Can you use #define names here? That would make it clearer what sort of
> clk is expected here.

yes will do that.

> 
>> +      clock-names = "xo", "alternate";
>> +
>> +      #interconnect-cells = <1>;
>> +    };

[PATCH v3 2/2] interconnect: qcom: Add OSM L3 interconnect provider support

[Sibi Sankar]

On some Qualcomm SoCs, Operating State Manager (OSM) controls the
resources of scaling L3 caches. Add a driver to handle bandwidth
requests to OSM L3 from CPU/GPU.

Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---
 drivers/interconnect/qcom/Kconfig  |   7 +
 drivers/interconnect/qcom/Makefile |   2 +
 drivers/interconnect/qcom/osm-l3.c | 284 +++++++++++++++++++++++++++++
 3 files changed, 293 insertions(+)
 create mode 100644 drivers/interconnect/qcom/osm-l3.c

diff --git a/drivers/interconnect/qcom/Kconfig b/drivers/interconnect/qcom/Kconfig
index ecf057d7e2409..17aee5b0f15b7 100644
--- a/drivers/interconnect/qcom/Kconfig
+++ b/drivers/interconnect/qcom/Kconfig
@@ -5,6 +5,13 @@ config INTERCONNECT_QCOM
 	help
 	  Support for Qualcomm's Network-on-Chip interconnect hardware.
 
+config INTERCONNECT_QCOM_OSM_L3
+	tristate "Qualcomm OSM L3 interconnect driver"
+	depends on INTERCONNECT_QCOM || COMPILE_TEST
+	help
+	  Say y here to support the Operating State Manager (OSM) interconnect
+	  driver which controls the scaling of L3 caches on Qualcomm SoCs.
+
 config INTERCONNECT_QCOM_QCS404
 	tristate "Qualcomm QCS404 interconnect driver"
 	depends on INTERCONNECT_QCOM
diff --git a/drivers/interconnect/qcom/Makefile b/drivers/interconnect/qcom/Makefile
index 9adf9e380545e..8d86d6515ffc9 100644
--- a/drivers/interconnect/qcom/Makefile
+++ b/drivers/interconnect/qcom/Makefile
@@ -1,10 +1,12 @@
 # SPDX-License-Identifier: GPL-2.0
 
+icc-osm-l3-objs				:= osm-l3.o
 qnoc-msm8974-objs			:= msm8974.o
 qnoc-qcs404-objs			:= qcs404.o
 qnoc-sdm845-objs			:= sdm845.o
 icc-smd-rpm-objs			:= smd-rpm.o
 
+obj-$(CONFIG_INTERCONNECT_QCOM_OSM_L3) += icc-osm-l3.o
 obj-$(CONFIG_INTERCONNECT_QCOM_MSM8974) += qnoc-msm8974.o
 obj-$(CONFIG_INTERCONNECT_QCOM_QCS404) += qnoc-qcs404.o
 obj-$(CONFIG_INTERCONNECT_QCOM_SDM845) += qnoc-sdm845.o
diff --git a/drivers/interconnect/qcom/osm-l3.c b/drivers/interconnect/qcom/osm-l3.c
new file mode 100644
index 0000000000000..5e9f9ce02863b
--- /dev/null
+++ b/drivers/interconnect/qcom/osm-l3.c
@@ -0,0 +1,284 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2019, The Linux Foundation. All rights reserved.
+ *
+ */
+
+#include <dt-bindings/interconnect/qcom,osm-l3.h>
+#include <dt-bindings/interconnect/qcom,sdm845.h>
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/interconnect-provider.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+
+#define LUT_MAX_ENTRIES			40U
+#define LUT_SRC				GENMASK(31, 30)
+#define LUT_L_VAL			GENMASK(7, 0)
+#define LUT_ROW_SIZE			32
+#define CLK_HW_DIV			2
+
+/* Register offsets */
+#define REG_ENABLE			0x0
+#define REG_FREQ_LUT			0x110
+#define REG_PERF_STATE			0x920
+
+#define OSM_L3_MAX_LINKS		1
+
+#define to_qcom_provider(_provider) \
+	container_of(_provider, struct qcom_osm_l3_icc_provider, provider)
+
+enum {
+	SDM845_MASTER_OSM_L3_APPS = SLAVE_TCU + 1,
+	SDM845_SLAVE_OSM_L3,
+};
+
+struct qcom_osm_l3_icc_provider {
+	void __iomem *base;
+	unsigned int max_state;
+	unsigned long lut_tables[LUT_MAX_ENTRIES];
+	struct icc_provider provider;
+};
+
+/**
+ * struct qcom_icc_node - Qualcomm specific interconnect nodes
+ * @name: the node name used in debugfs
+ * @links: an array of nodes where we can go next while traversing
+ * @id: a unique node identifier
+ * @num_links: the total number of @links
+ * @buswidth: width of the interconnect between a node and the bus
+ */
+struct qcom_icc_node {
+	const char *name;
+	u16 links[OSM_L3_MAX_LINKS];
+	u16 id;
+	u16 num_links;
+	u16 buswidth;
+};
+
+struct qcom_icc_desc {
+	struct qcom_icc_node **nodes;
+	size_t num_nodes;
+};
+
+#define DEFINE_QNODE(_name, _id, _buswidth, ...)			\
+		static struct qcom_icc_node _name = {			\
+		.name = #_name,						\
+		.id = _id,						\
+		.buswidth = _buswidth,					\
+		.num_links = ARRAY_SIZE(((int[]){ __VA_ARGS__ })),	\
+		.links = { __VA_ARGS__ },				\
+	}
+
+DEFINE_QNODE(osm_apps_l3, SDM845_MASTER_OSM_L3_APPS, 16, SDM845_SLAVE_OSM_L3);
+DEFINE_QNODE(osm_l3, SDM845_SLAVE_OSM_L3, 16);
+
+static struct qcom_icc_node *sdm845_osm_l3_nodes[] = {
+	[MASTER_OSM_L3_APPS] = &osm_apps_l3,
+	[SLAVE_OSM_L3] = &osm_l3,
+};
+
+static struct qcom_icc_desc sdm845_osm_l3 = {
+	.nodes = sdm845_osm_l3_nodes,
+	.num_nodes = ARRAY_SIZE(sdm845_osm_l3_nodes),
+};
+
+static int qcom_icc_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
+			      u32 peak_bw, u32 *agg_avg, u32 *agg_peak)
+{
+	*agg_avg += avg_bw;
+	*agg_peak = max_t(u32, *agg_peak, peak_bw);
+
+	return 0;
+}
+
+static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
+{
+	struct qcom_osm_l3_icc_provider *qp;
+	struct icc_provider *provider;
+	struct qcom_icc_node *qn;
+	struct icc_node *n;
+	unsigned int index;
+	u32 agg_peak = 0;
+	u32 agg_avg = 0;
+	u64 rate;
+
+	qn = src->data;
+	provider = src->provider;
+	qp = to_qcom_provider(provider);
+
+	list_for_each_entry(n, &provider->nodes, node_list)
+		qcom_icc_aggregate(n, 0, n->avg_bw, n->peak_bw,
+				   &agg_avg, &agg_peak);
+
+	rate = max(agg_avg, agg_peak);
+	rate = icc_units_to_bps(rate);
+	do_div(rate, qn->buswidth);
+
+	for (index = 0; index < qp->max_state; index++) {
+		if (qp->lut_tables[index] >= rate)
+			break;
+	}
+
+	writel_relaxed(index, qp->base + REG_PERF_STATE);
+
+	return 0;
+}
+
+static int qcom_osm_l3_remove(struct platform_device *pdev)
+{
+	struct qcom_osm_l3_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);
+	}
+
+	return icc_provider_del(provider);
+}
+
+static int qcom_osm_l3_probe(struct platform_device *pdev)
+{
+	u32 info, src, lval, i, prev_freq = 0, freq;
+	static unsigned long hw_rate, xo_rate;
+	struct qcom_osm_l3_icc_provider *qp;
+	const struct qcom_icc_desc *desc;
+	struct icc_onecell_data *data;
+	struct icc_provider *provider;
+	struct qcom_icc_node **qnodes;
+	struct icc_node *node;
+	size_t num_nodes;
+	struct clk *clk;
+	int ret;
+
+	clk = clk_get(&pdev->dev, "xo");
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	xo_rate = clk_get_rate(clk);
+	clk_put(clk);
+
+	clk = clk_get(&pdev->dev, "alternate");
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	hw_rate = clk_get_rate(clk) / CLK_HW_DIV;
+	clk_put(clk);
+
+	qp = devm_kzalloc(&pdev->dev, sizeof(*qp), GFP_KERNEL);
+	if (!qp)
+		return -ENOMEM;
+
+	qp->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(qp->base))
+		return PTR_ERR(qp->base);
+
+	/* HW should be in enabled state to proceed */
+	if (!(readl_relaxed(qp->base + REG_ENABLE) & 0x1)) {
+		dev_err(&pdev->dev, "error hardware not enabled\n");
+		return -ENODEV;
+	}
+
+	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
+		info = readl_relaxed(qp->base + REG_FREQ_LUT +
+				     i * LUT_ROW_SIZE);
+		src = FIELD_GET(LUT_SRC, info);
+		lval = FIELD_GET(LUT_L_VAL, info);
+		if (src)
+			freq = xo_rate * lval;
+		else
+			freq = hw_rate;
+
+		/*
+		 * Two of the same frequencies with the same core counts means
+		 * end of table
+		 */
+		if (i > 0 && prev_freq == freq)
+			break;
+
+		qp->lut_tables[i] = freq;
+		prev_freq = freq;
+	}
+	qp->max_state = i;
+
+	desc = of_device_get_match_data(&pdev->dev);
+	if (!desc)
+		return -EINVAL;
+
+	qnodes = desc->nodes;
+	num_nodes = desc->num_nodes;
+
+	data = devm_kcalloc(&pdev->dev, num_nodes, sizeof(*node), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	provider = &qp->provider;
+	provider->dev = &pdev->dev;
+	provider->set = qcom_icc_set;
+	provider->aggregate = qcom_icc_aggregate;
+	provider->xlate = of_icc_xlate_onecell;
+	INIT_LIST_HEAD(&provider->nodes);
+	provider->data = data;
+
+	ret = icc_provider_add(provider);
+	if (ret) {
+		dev_err(&pdev->dev, "error adding interconnect provider\n");
+		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 %p %s %d\n", node,
+			qnodes[i]->name, node->id);
+
+		/* 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:
+	qcom_osm_l3_remove(pdev);
+	return ret;
+}
+
+static const struct of_device_id osm_l3_of_match[] = {
+	{ .compatible = "qcom,sdm845-osm-l3", .data = &sdm845_osm_l3 },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, osm_l3_of_match);
+
+static struct platform_driver osm_l3_driver = {
+	.probe = qcom_osm_l3_probe,
+	.remove = qcom_osm_l3_remove,
+	.driver = {
+		.name = "osm-l3",
+		.of_match_table = osm_l3_of_match,
+	},
+};
+module_platform_driver(osm_l3_driver);
+
+MODULE_DESCRIPTION("Qualcomm OSM L3 interconnect driver");
+MODULE_LICENSE("GPL v2");
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 2/2] interconnect: qcom: Add OSM L3 interconnect provider support

[Evan Green]

Hi Sibi,

On Mon, Nov 18, 2019 at 7:45 AM Sibi Sankar <sibis@codeaurora.org> wrote:
>
> On some Qualcomm SoCs, Operating State Manager (OSM) controls the
> resources of scaling L3 caches. Add a driver to handle bandwidth
> requests to OSM L3 from CPU/GPU.
>
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  drivers/interconnect/qcom/Kconfig  |   7 +
>  drivers/interconnect/qcom/Makefile |   2 +
>  drivers/interconnect/qcom/osm-l3.c | 284 +++++++++++++++++++++++++++++
>  3 files changed, 293 insertions(+)
>  create mode 100644 drivers/interconnect/qcom/osm-l3.c
>
> diff --git a/drivers/interconnect/qcom/Kconfig b/drivers/interconnect/qcom/Kconfig
> index ecf057d7e2409..17aee5b0f15b7 100644
> --- a/drivers/interconnect/qcom/Kconfig
> +++ b/drivers/interconnect/qcom/Kconfig
> @@ -5,6 +5,13 @@ config INTERCONNECT_QCOM
>         help
>           Support for Qualcomm's Network-on-Chip interconnect hardware.
>
> +config INTERCONNECT_QCOM_OSM_L3
> +       tristate "Qualcomm OSM L3 interconnect driver"
> +       depends on INTERCONNECT_QCOM || COMPILE_TEST

Should we depend on something sdm845 here?

> +       help
> +         Say y here to support the Operating State Manager (OSM) interconnect
> +         driver which controls the scaling of L3 caches on Qualcomm SoCs.
> +
>  config INTERCONNECT_QCOM_QCS404
>         tristate "Qualcomm QCS404 interconnect driver"
>         depends on INTERCONNECT_QCOM
> diff --git a/drivers/interconnect/qcom/Makefile b/drivers/interconnect/qcom/Makefile
> index 9adf9e380545e..8d86d6515ffc9 100644
> --- a/drivers/interconnect/qcom/Makefile
> +++ b/drivers/interconnect/qcom/Makefile
> @@ -1,10 +1,12 @@
>  # SPDX-License-Identifier: GPL-2.0
>
> +icc-osm-l3-objs                                := osm-l3.o
>  qnoc-msm8974-objs                      := msm8974.o
>  qnoc-qcs404-objs                       := qcs404.o
>  qnoc-sdm845-objs                       := sdm845.o
>  icc-smd-rpm-objs                       := smd-rpm.o
>
> +obj-$(CONFIG_INTERCONNECT_QCOM_OSM_L3) += icc-osm-l3.o
>  obj-$(CONFIG_INTERCONNECT_QCOM_MSM8974) += qnoc-msm8974.o
>  obj-$(CONFIG_INTERCONNECT_QCOM_QCS404) += qnoc-qcs404.o
>  obj-$(CONFIG_INTERCONNECT_QCOM_SDM845) += qnoc-sdm845.o
> diff --git a/drivers/interconnect/qcom/osm-l3.c b/drivers/interconnect/qcom/osm-l3.c
> new file mode 100644
> index 0000000000000..5e9f9ce02863b
> --- /dev/null
> +++ b/drivers/interconnect/qcom/osm-l3.c
> @@ -0,0 +1,284 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019, The Linux Foundation. All rights reserved.
> + *
> + */
> +
> +#include <dt-bindings/interconnect/qcom,osm-l3.h>
> +#include <dt-bindings/interconnect/qcom,sdm845.h>
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/interconnect-provider.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +
> +#define LUT_MAX_ENTRIES                        40U
> +#define LUT_SRC                                GENMASK(31, 30)
> +#define LUT_L_VAL                      GENMASK(7, 0)
> +#define LUT_ROW_SIZE                   32
> +#define CLK_HW_DIV                     2
> +
> +/* Register offsets */
> +#define REG_ENABLE                     0x0
> +#define REG_FREQ_LUT                   0x110
> +#define REG_PERF_STATE                 0x920
> +
> +#define OSM_L3_MAX_LINKS               1
> +
> +#define to_qcom_provider(_provider) \
> +       container_of(_provider, struct qcom_osm_l3_icc_provider, provider)
> +
> +enum {
> +       SDM845_MASTER_OSM_L3_APPS = SLAVE_TCU + 1,
> +       SDM845_SLAVE_OSM_L3,
> +};

Should these just go in qcom,sdm845.h? Seems nice to have them all in
one place, and then they can be accessed in the DT if needed.

> +
> +struct qcom_osm_l3_icc_provider {
> +       void __iomem *base;
> +       unsigned int max_state;
> +       unsigned long lut_tables[LUT_MAX_ENTRIES];
> +       struct icc_provider provider;
> +};
> +
> +/**
> + * struct qcom_icc_node - Qualcomm specific interconnect nodes
> + * @name: the node name used in debugfs
> + * @links: an array of nodes where we can go next while traversing
> + * @id: a unique node identifier
> + * @num_links: the total number of @links
> + * @buswidth: width of the interconnect between a node and the bus
> + */
> +struct qcom_icc_node {
> +       const char *name;
> +       u16 links[OSM_L3_MAX_LINKS];
> +       u16 id;
> +       u16 num_links;
> +       u16 buswidth;
> +};
> +
> +struct qcom_icc_desc {
> +       struct qcom_icc_node **nodes;
> +       size_t num_nodes;
> +};
> +
> +#define DEFINE_QNODE(_name, _id, _buswidth, ...)                       \
> +               static struct qcom_icc_node _name = {                   \
> +               .name = #_name,                                         \
> +               .id = _id,                                              \
> +               .buswidth = _buswidth,                                  \
> +               .num_links = ARRAY_SIZE(((int[]){ __VA_ARGS__ })),      \
> +               .links = { __VA_ARGS__ },                               \
> +       }
> +
> +DEFINE_QNODE(osm_apps_l3, SDM845_MASTER_OSM_L3_APPS, 16, SDM845_SLAVE_OSM_L3);
> +DEFINE_QNODE(osm_l3, SDM845_SLAVE_OSM_L3, 16);
> +
> +static struct qcom_icc_node *sdm845_osm_l3_nodes[] = {

const?

> +       [MASTER_OSM_L3_APPS] = &osm_apps_l3,
> +       [SLAVE_OSM_L3] = &osm_l3,
> +};
> +
> +static struct qcom_icc_desc sdm845_osm_l3 = {
> +       .nodes = sdm845_osm_l3_nodes,
> +       .num_nodes = ARRAY_SIZE(sdm845_osm_l3_nodes),
> +};
> +
> +static int qcom_icc_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
> +                             u32 peak_bw, u32 *agg_avg, u32 *agg_peak)
> +{
> +       *agg_avg += avg_bw;
> +       *agg_peak = max_t(u32, *agg_peak, peak_bw);
> +
> +       return 0;
> +}

Georgi, I wonder if it's a good idea to make a small collection of
"std" aggregate functions in the interconnect core that a driver can
just point to if it's doing something super standard like this (ie
driver->aggregate = icc_std_aggregate;). This is probably fine as-is
for now, but if we see a lot more copy/pastes of this function we
should think about it.

> +
> +static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
> +{
> +       struct qcom_osm_l3_icc_provider *qp;
> +       struct icc_provider *provider;
> +       struct qcom_icc_node *qn;
> +       struct icc_node *n;
> +       unsigned int index;
> +       u32 agg_peak = 0;
> +       u32 agg_avg = 0;
> +       u64 rate;
> +
> +       qn = src->data;
> +       provider = src->provider;
> +       qp = to_qcom_provider(provider);
> +
> +       list_for_each_entry(n, &provider->nodes, node_list)
> +               qcom_icc_aggregate(n, 0, n->avg_bw, n->peak_bw,
> +                                  &agg_avg, &agg_peak);
> +
> +       rate = max(agg_avg, agg_peak);
> +       rate = icc_units_to_bps(rate);
> +       do_div(rate, qn->buswidth);
> +
> +       for (index = 0; index < qp->max_state; index++) {

If the rate is too high, you'll end up setting max_state into the
register. That's probably bad, right? (Or maybe it's not because the
table ends with the same value twice, but it seems like relying on an
implementation detail). We could guard against that by only iterating
to index < qp->max_state - 1.

> +               if (qp->lut_tables[index] >= rate)
> +                       break;
> +       }
> +
> +       writel_relaxed(index, qp->base + REG_PERF_STATE);
> +
> +       return 0;
> +}
> +
> +static int qcom_osm_l3_remove(struct platform_device *pdev)
> +{
> +       struct qcom_osm_l3_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) {

There was a comment on one of the other threads that we've been
copy/pasting this snippet around and it's wrong because it doesn't use
the _safe variant of the macro. So we end up destroying the list we're
iterating over.

Georgi, did you have a plan to refactor this, or should we just change
this to be the _safe version?

> +               icc_node_del(n);
> +               icc_node_destroy(n->id);
> +       }
> +
> +       return icc_provider_del(provider);
> +}
> +
> +static int qcom_osm_l3_probe(struct platform_device *pdev)
> +{
> +       u32 info, src, lval, i, prev_freq = 0, freq;
> +       static unsigned long hw_rate, xo_rate;
> +       struct qcom_osm_l3_icc_provider *qp;
> +       const struct qcom_icc_desc *desc;
> +       struct icc_onecell_data *data;
> +       struct icc_provider *provider;
> +       struct qcom_icc_node **qnodes;
> +       struct icc_node *node;
> +       size_t num_nodes;
> +       struct clk *clk;
> +       int ret;
> +
> +       clk = clk_get(&pdev->dev, "xo");
> +       if (IS_ERR(clk))
> +               return PTR_ERR(clk);
> +
> +       xo_rate = clk_get_rate(clk);
> +       clk_put(clk);
> +
> +       clk = clk_get(&pdev->dev, "alternate");
> +       if (IS_ERR(clk))
> +               return PTR_ERR(clk);
> +
> +       hw_rate = clk_get_rate(clk) / CLK_HW_DIV;

It's a little weird there's a constant divide in there, though I guess
it's in the cpufreq driver as well. I guess this is fine if it's
likely to stay there (and the same) when this driver is generalized
for other SoCs.

> +       clk_put(clk);
> +
> +       qp = devm_kzalloc(&pdev->dev, sizeof(*qp), GFP_KERNEL);
> +       if (!qp)
> +               return -ENOMEM;
> +
> +       qp->base = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(qp->base))
> +               return PTR_ERR(qp->base);
> +
> +       /* HW should be in enabled state to proceed */
> +       if (!(readl_relaxed(qp->base + REG_ENABLE) & 0x1)) {
> +               dev_err(&pdev->dev, "error hardware not enabled\n");
> +               return -ENODEV;
> +       }
> +
> +       for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> +               info = readl_relaxed(qp->base + REG_FREQ_LUT +
> +                                    i * LUT_ROW_SIZE);
> +               src = FIELD_GET(LUT_SRC, info);
> +               lval = FIELD_GET(LUT_L_VAL, info);
> +               if (src)
> +                       freq = xo_rate * lval;
> +               else
> +                       freq = hw_rate;
> +
> +               /*
> +                * Two of the same frequencies with the same core counts means

"core counts" seems like a copied comment that doesn't apply.

But you only look at freq and not core count, is that really
equivalent to the table's boundary condition? Or do you need to be
comparing info == info_prev?

> +                * end of table
> +                */
> +               if (i > 0 && prev_freq == freq)
> +                       break;
> +
> +               qp->lut_tables[i] = freq;
> +               prev_freq = freq;
> +       }
> +       qp->max_state = i;

Should we error out or complain if there are too few entries, or if
the table is not in increasing order?

> +
> +       desc = of_device_get_match_data(&pdev->dev);
> +       if (!desc)
> +               return -EINVAL;
> +
> +       qnodes = desc->nodes;
> +       num_nodes = desc->num_nodes;
> +
> +       data = devm_kcalloc(&pdev->dev, num_nodes, sizeof(*node), GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       provider = &qp->provider;
> +       provider->dev = &pdev->dev;
> +       provider->set = qcom_icc_set;
> +       provider->aggregate = qcom_icc_aggregate;
> +       provider->xlate = of_icc_xlate_onecell;
> +       INIT_LIST_HEAD(&provider->nodes);
> +       provider->data = data;
> +
> +       ret = icc_provider_add(provider);
> +       if (ret) {
> +               dev_err(&pdev->dev, "error adding interconnect provider\n");
> +               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 %p %s %d\n", node,
> +                       qnodes[i]->name, node->id);
> +
> +               /* populate links */

Not a super useful comment.


> +               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:
> +       qcom_osm_l3_remove(pdev);
> +       return ret;
> +}
> +
> +static const struct of_device_id osm_l3_of_match[] = {
> +       { .compatible = "qcom,sdm845-osm-l3", .data = &sdm845_osm_l3 },
> +       { },
> +};
> +MODULE_DEVICE_TABLE(of, osm_l3_of_match);
> +
> +static struct platform_driver osm_l3_driver = {
> +       .probe = qcom_osm_l3_probe,
> +       .remove = qcom_osm_l3_remove,
> +       .driver = {
> +               .name = "osm-l3",
> +               .of_match_table = osm_l3_of_match,
> +       },
> +};
> +module_platform_driver(osm_l3_driver);
> +
> +MODULE_DESCRIPTION("Qualcomm OSM L3 interconnect driver");
> +MODULE_LICENSE("GPL v2");
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>

Re: [PATCH v3 2/2] interconnect: qcom: Add OSM L3 interconnect provider support

[sibis]

Hey Evan,
Thanks for taking time to review
the series.

On 2019-11-19 04:12, Evan Green wrote:
> Hi Sibi,
> 
> On Mon, Nov 18, 2019 at 7:45 AM Sibi Sankar <sibis@codeaurora.org> 
> wrote:
>> 
>> On some Qualcomm SoCs, Operating State Manager (OSM) controls the
>> resources of scaling L3 caches. Add a driver to handle bandwidth
>> requests to OSM L3 from CPU/GPU.
>> 
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> ---
>>  drivers/interconnect/qcom/Kconfig  |   7 +
>>  drivers/interconnect/qcom/Makefile |   2 +
>>  drivers/interconnect/qcom/osm-l3.c | 284 
>> +++++++++++++++++++++++++++++
>>  3 files changed, 293 insertions(+)
>>  create mode 100644 drivers/interconnect/qcom/osm-l3.c
>> 
>> diff --git a/drivers/interconnect/qcom/Kconfig 
>> b/drivers/interconnect/qcom/Kconfig
>> index ecf057d7e2409..17aee5b0f15b7 100644
>> --- a/drivers/interconnect/qcom/Kconfig
>> +++ b/drivers/interconnect/qcom/Kconfig
>> @@ -5,6 +5,13 @@ config INTERCONNECT_QCOM
>>         help
>>           Support for Qualcomm's Network-on-Chip interconnect 
>> hardware.
>> 
>> +config INTERCONNECT_QCOM_OSM_L3
>> +       tristate "Qualcomm OSM L3 interconnect driver"
>> +       depends on INTERCONNECT_QCOM || COMPILE_TEST
> 
> Should we depend on something sdm845 here?

not really...

maybe I can have 845 interconnect
selecting osm_l3? since sc7180 also
would be doing the same.

> 
>> +       help
>> +         Say y here to support the Operating State Manager (OSM) 
>> interconnect
>> +         driver which controls the scaling of L3 caches on Qualcomm 
>> SoCs.
>> +
>>  config INTERCONNECT_QCOM_QCS404
>>         tristate "Qualcomm QCS404 interconnect driver"
>>         depends on INTERCONNECT_QCOM
>> diff --git a/drivers/interconnect/qcom/Makefile 
>> b/drivers/interconnect/qcom/Makefile
>> index 9adf9e380545e..8d86d6515ffc9 100644
>> --- a/drivers/interconnect/qcom/Makefile
>> +++ b/drivers/interconnect/qcom/Makefile
>> @@ -1,10 +1,12 @@
>>  # SPDX-License-Identifier: GPL-2.0
>> 
>> +icc-osm-l3-objs                                := osm-l3.o
>>  qnoc-msm8974-objs                      := msm8974.o
>>  qnoc-qcs404-objs                       := qcs404.o
>>  qnoc-sdm845-objs                       := sdm845.o
>>  icc-smd-rpm-objs                       := smd-rpm.o
>> 
>> +obj-$(CONFIG_INTERCONNECT_QCOM_OSM_L3) += icc-osm-l3.o
>>  obj-$(CONFIG_INTERCONNECT_QCOM_MSM8974) += qnoc-msm8974.o
>>  obj-$(CONFIG_INTERCONNECT_QCOM_QCS404) += qnoc-qcs404.o
>>  obj-$(CONFIG_INTERCONNECT_QCOM_SDM845) += qnoc-sdm845.o
>> diff --git a/drivers/interconnect/qcom/osm-l3.c 
>> b/drivers/interconnect/qcom/osm-l3.c
>> new file mode 100644
>> index 0000000000000..5e9f9ce02863b
>> --- /dev/null
>> +++ b/drivers/interconnect/qcom/osm-l3.c
>> @@ -0,0 +1,284 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2019, The Linux Foundation. All rights reserved.
>> + *
>> + */
>> +
>> +#include <dt-bindings/interconnect/qcom,osm-l3.h>
>> +#include <dt-bindings/interconnect/qcom,sdm845.h>
>> +#include <linux/bitfield.h>
>> +#include <linux/clk.h>
>> +#include <linux/interconnect-provider.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/platform_device.h>
>> +
>> +#define LUT_MAX_ENTRIES                        40U
>> +#define LUT_SRC                                GENMASK(31, 30)
>> +#define LUT_L_VAL                      GENMASK(7, 0)
>> +#define LUT_ROW_SIZE                   32
>> +#define CLK_HW_DIV                     2
>> +
>> +/* Register offsets */
>> +#define REG_ENABLE                     0x0
>> +#define REG_FREQ_LUT                   0x110
>> +#define REG_PERF_STATE                 0x920
>> +
>> +#define OSM_L3_MAX_LINKS               1
>> +
>> +#define to_qcom_provider(_provider) \
>> +       container_of(_provider, struct qcom_osm_l3_icc_provider, 
>> provider)
>> +
>> +enum {
>> +       SDM845_MASTER_OSM_L3_APPS = SLAVE_TCU + 1,
>> +       SDM845_SLAVE_OSM_L3,
>> +};
> 
> Should these just go in qcom,sdm845.h? Seems nice to have them all in
> one place, and then they can be accessed in the DT if needed.

yeah I can do that, by doing
this I can also get rid of the
SLAVE_TCU + 1 :)

> 
>> +
>> +struct qcom_osm_l3_icc_provider {
>> +       void __iomem *base;
>> +       unsigned int max_state;
>> +       unsigned long lut_tables[LUT_MAX_ENTRIES];
>> +       struct icc_provider provider;
>> +};
>> +
>> +/**
>> + * struct qcom_icc_node - Qualcomm specific interconnect nodes
>> + * @name: the node name used in debugfs
>> + * @links: an array of nodes where we can go next while traversing
>> + * @id: a unique node identifier
>> + * @num_links: the total number of @links
>> + * @buswidth: width of the interconnect between a node and the bus
>> + */
>> +struct qcom_icc_node {
>> +       const char *name;
>> +       u16 links[OSM_L3_MAX_LINKS];
>> +       u16 id;
>> +       u16 num_links;
>> +       u16 buswidth;
>> +};
>> +
>> +struct qcom_icc_desc {
>> +       struct qcom_icc_node **nodes;
>> +       size_t num_nodes;
>> +};
>> +
>> +#define DEFINE_QNODE(_name, _id, _buswidth, ...)                      
>>  \
>> +               static struct qcom_icc_node _name = {                  
>>  \
>> +               .name = #_name,                                        
>>  \
>> +               .id = _id,                                             
>>  \
>> +               .buswidth = _buswidth,                                 
>>  \
>> +               .num_links = ARRAY_SIZE(((int[]){ __VA_ARGS__ })),     
>>  \
>> +               .links = { __VA_ARGS__ },                              
>>  \
>> +       }
>> +
>> +DEFINE_QNODE(osm_apps_l3, SDM845_MASTER_OSM_L3_APPS, 16, 
>> SDM845_SLAVE_OSM_L3);
>> +DEFINE_QNODE(osm_l3, SDM845_SLAVE_OSM_L3, 16);
>> +
>> +static struct qcom_icc_node *sdm845_osm_l3_nodes[] = {
> 
> const?

unfortunately we can't ...

data->nodes[i] = node;
we setup links later ^^ with
the pointer.

> 
>> +       [MASTER_OSM_L3_APPS] = &osm_apps_l3,
>> +       [SLAVE_OSM_L3] = &osm_l3,
>> +};
>> +
>> +static struct qcom_icc_desc sdm845_osm_l3 = {
>> +       .nodes = sdm845_osm_l3_nodes,
>> +       .num_nodes = ARRAY_SIZE(sdm845_osm_l3_nodes),
>> +};
>> +
>> +static int qcom_icc_aggregate(struct icc_node *node, u32 tag, u32 
>> avg_bw,
>> +                             u32 peak_bw, u32 *agg_avg, u32 
>> *agg_peak)
>> +{
>> +       *agg_avg += avg_bw;
>> +       *agg_peak = max_t(u32, *agg_peak, peak_bw);
>> +
>> +       return 0;
>> +}
> 
> Georgi, I wonder if it's a good idea to make a small collection of
> "std" aggregate functions in the interconnect core that a driver can
> just point to if it's doing something super standard like this (ie
> driver->aggregate = icc_std_aggregate;). This is probably fine as-is
> for now, but if we see a lot more copy/pastes of this function we
> should think about it.
> 
>> +
>> +static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
>> +{
>> +       struct qcom_osm_l3_icc_provider *qp;
>> +       struct icc_provider *provider;
>> +       struct qcom_icc_node *qn;
>> +       struct icc_node *n;
>> +       unsigned int index;
>> +       u32 agg_peak = 0;
>> +       u32 agg_avg = 0;
>> +       u64 rate;
>> +
>> +       qn = src->data;
>> +       provider = src->provider;
>> +       qp = to_qcom_provider(provider);
>> +
>> +       list_for_each_entry(n, &provider->nodes, node_list)
>> +               qcom_icc_aggregate(n, 0, n->avg_bw, n->peak_bw,
>> +                                  &agg_avg, &agg_peak);
>> +
>> +       rate = max(agg_avg, agg_peak);
>> +       rate = icc_units_to_bps(rate);
>> +       do_div(rate, qn->buswidth);
>> +
>> +       for (index = 0; index < qp->max_state; index++) {
> 
> If the rate is too high, you'll end up setting max_state into the
> register. That's probably bad, right? (Or maybe it's not because the
> table ends with the same value twice, but it seems like relying on an
> implementation detail). We could guard against that by only iterating
> to index < qp->max_state - 1.

yes, using max_state - 1 makes sense
here. Will change this in the next
re-spin.

> 
>> +               if (qp->lut_tables[index] >= rate)
>> +                       break;
>> +       }
>> +
>> +       writel_relaxed(index, qp->base + REG_PERF_STATE);
>> +
>> +       return 0;
>> +}
>> +
>> +static int qcom_osm_l3_remove(struct platform_device *pdev)
>> +{
>> +       struct qcom_osm_l3_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) {
> 
> There was a comment on one of the other threads that we've been
> copy/pasting this snippet around and it's wrong because it doesn't use
> the _safe variant of the macro. So we end up destroying the list we're
> iterating over.
> 
> Georgi, did you have a plan to refactor this, or should we just change
> this to be the _safe version?
> 
>> +               icc_node_del(n);
>> +               icc_node_destroy(n->id);
>> +       }
>> +
>> +       return icc_provider_del(provider);
>> +}
>> +
>> +static int qcom_osm_l3_probe(struct platform_device *pdev)
>> +{
>> +       u32 info, src, lval, i, prev_freq = 0, freq;
>> +       static unsigned long hw_rate, xo_rate;
>> +       struct qcom_osm_l3_icc_provider *qp;
>> +       const struct qcom_icc_desc *desc;
>> +       struct icc_onecell_data *data;
>> +       struct icc_provider *provider;
>> +       struct qcom_icc_node **qnodes;
>> +       struct icc_node *node;
>> +       size_t num_nodes;
>> +       struct clk *clk;
>> +       int ret;
>> +
>> +       clk = clk_get(&pdev->dev, "xo");
>> +       if (IS_ERR(clk))
>> +               return PTR_ERR(clk);
>> +
>> +       xo_rate = clk_get_rate(clk);
>> +       clk_put(clk);
>> +
>> +       clk = clk_get(&pdev->dev, "alternate");
>> +       if (IS_ERR(clk))
>> +               return PTR_ERR(clk);
>> +
>> +       hw_rate = clk_get_rate(clk) / CLK_HW_DIV;
> 
> It's a little weird there's a constant divide in there, though I guess
> it's in the cpufreq driver as well. I guess this is fine if it's
> likely to stay there (and the same) when this driver is generalized
> for other SoCs.

yeah I don't see this changing
on sc7180 so I'll leave this
as is.

> 
>> +       clk_put(clk);
>> +
>> +       qp = devm_kzalloc(&pdev->dev, sizeof(*qp), GFP_KERNEL);
>> +       if (!qp)
>> +               return -ENOMEM;
>> +
>> +       qp->base = devm_platform_ioremap_resource(pdev, 0);
>> +       if (IS_ERR(qp->base))
>> +               return PTR_ERR(qp->base);
>> +
>> +       /* HW should be in enabled state to proceed */
>> +       if (!(readl_relaxed(qp->base + REG_ENABLE) & 0x1)) {
>> +               dev_err(&pdev->dev, "error hardware not enabled\n");
>> +               return -ENODEV;
>> +       }
>> +
>> +       for (i = 0; i < LUT_MAX_ENTRIES; i++) {
>> +               info = readl_relaxed(qp->base + REG_FREQ_LUT +
>> +                                    i * LUT_ROW_SIZE);
>> +               src = FIELD_GET(LUT_SRC, info);
>> +               lval = FIELD_GET(LUT_L_VAL, info);
>> +               if (src)
>> +                       freq = xo_rate * lval;
>> +               else
>> +                       freq = hw_rate;
>> +
>> +               /*
>> +                * Two of the same frequencies with the same core 
>> counts means
> 
> "core counts" seems like a copied comment that doesn't apply.

yes we don't use the core_cnt
field so will update the comment.

> 
> But you only look at freq and not core count, is that really
> equivalent to the table's boundary condition? Or do you need to be
> comparing info == info_prev?

no we can get by comparing
current freq with prev freq
on OSM L3.

> 
>> +                * end of table
>> +                */
>> +               if (i > 0 && prev_freq == freq)
>> +                       break;
>> +
>> +               qp->lut_tables[i] = freq;
>> +               prev_freq = freq;
>> +       }
>> +       qp->max_state = i;
> 
> Should we error out or complain if there are too few entries, or if
> the table is not in increasing order?

OSM does make sure of the
increasing order and correct
number of freq entries if the
REG_ENABLE is set. However I
don't really mind adding the
increasing order check but we
can't really detect too few
entries since the number can
vary across SKUs.

> 
>> +
>> +       desc = of_device_get_match_data(&pdev->dev);
>> +       if (!desc)
>> +               return -EINVAL;
>> +
>> +       qnodes = desc->nodes;
>> +       num_nodes = desc->num_nodes;
>> +
>> +       data = devm_kcalloc(&pdev->dev, num_nodes, sizeof(*node), 
>> GFP_KERNEL);
>> +       if (!data)
>> +               return -ENOMEM;
>> +
>> +       provider = &qp->provider;
>> +       provider->dev = &pdev->dev;
>> +       provider->set = qcom_icc_set;
>> +       provider->aggregate = qcom_icc_aggregate;
>> +       provider->xlate = of_icc_xlate_onecell;
>> +       INIT_LIST_HEAD(&provider->nodes);
>> +       provider->data = data;
>> +
>> +       ret = icc_provider_add(provider);
>> +       if (ret) {
>> +               dev_err(&pdev->dev, "error adding interconnect 
>> provider\n");
>> +               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 %p %s %d\n", 
>> node,
>> +                       qnodes[i]->name, node->id);
>> +
>> +               /* populate links */
> 
> Not a super useful comment.

lol will remove it

> 
> 
>> +               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:
>> +       qcom_osm_l3_remove(pdev);
>> +       return ret;
>> +}
>> +
>> +static const struct of_device_id osm_l3_of_match[] = {
>> +       { .compatible = "qcom,sdm845-osm-l3", .data = &sdm845_osm_l3 
>> },
>> +       { },
>> +};
>> +MODULE_DEVICE_TABLE(of, osm_l3_of_match);
>> +
>> +static struct platform_driver osm_l3_driver = {
>> +       .probe = qcom_osm_l3_probe,
>> +       .remove = qcom_osm_l3_remove,
>> +       .driver = {
>> +               .name = "osm-l3",
>> +               .of_match_table = osm_l3_of_match,
>> +       },
>> +};
>> +module_platform_driver(osm_l3_driver);
>> +
>> +MODULE_DESCRIPTION("Qualcomm OSM L3 interconnect driver");
>> +MODULE_LICENSE("GPL v2");
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>> Forum,
>> a Linux Foundation Collaborative Project
>> 

Re: [PATCH v3 2/2] interconnect: qcom: Add OSM L3 interconnect provider support

[Sibi Sankar]

Hey Evan/Georgi,

https://git.linaro.org/people/georgi.djakov/linux.git/commit/?h=icc-dev&id=9197da7d06e88666d1588e3c21a743e60381264d

With the "Redefine interconnect provider
DT nodes for SDM845" series, wouldn't it
make more sense to define the OSM_L3 icc
nodes in the sdm845.c icc driver and have
the common helpers in osm_l3 driver? Though
we don't plan on linking the OSM L3 nodes
to the other nodes on SDM845/SC7180, we
might have GPU needing to be linked to the
OSM L3 nodes on future SoCs. Let me know
how you want this done.

Anyway I'll re-spin the series once the
SDM845 icc re-work gets re-posted.

On 2019-11-19 17:30, sibis@codeaurora.org wrote:
> Hey Evan,
> Thanks for taking time to review
> the series.
> 
> On 2019-11-19 04:12, Evan Green wrote:
>> Hi Sibi,
>> 
>> On Mon, Nov 18, 2019 at 7:45 AM Sibi Sankar <sibis@codeaurora.org> 
>> wrote:
>>> 
>>> On some Qualcomm SoCs, Operating State Manager (OSM) controls the
>>> resources of scaling L3 caches. Add a driver to handle bandwidth
>>> requests to OSM L3 from CPU/GPU.
>>> 
>>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>>> ---
>>>  drivers/interconnect/qcom/Kconfig  |   7 +
>>>  drivers/interconnect/qcom/Makefile |   2 +
>>>  drivers/interconnect/qcom/osm-l3.c | 284 
>>> +++++++++++++++++++++++++++++
>>>  3 files changed, 293 insertions(+)
>>>  create mode 100644 drivers/interconnect/qcom/osm-l3.c
>>> 
>>> diff --git a/drivers/interconnect/qcom/Kconfig 
>>> b/drivers/interconnect/qcom/Kconfig
>>> index ecf057d7e2409..17aee5b0f15b7 100644
>>> --- a/drivers/interconnect/qcom/Kconfig
>>> +++ b/drivers/interconnect/qcom/Kconfig
>>> @@ -5,6 +5,13 @@ config INTERCONNECT_QCOM
>>>         help
>>>           Support for Qualcomm's Network-on-Chip interconnect 
>>> hardware.
>>> 
>>> +config INTERCONNECT_QCOM_OSM_L3
>>> +       tristate "Qualcomm OSM L3 interconnect driver"
>>> +       depends on INTERCONNECT_QCOM || COMPILE_TEST
>> 
>> Should we depend on something sdm845 here?
> 
> not really...
> 
> maybe I can have 845 interconnect
> selecting osm_l3? since sc7180 also
> would be doing the same.
> 
>> 
>>> +       help
>>> +         Say y here to support the Operating State Manager (OSM) 
>>> interconnect
>>> +         driver which controls the scaling of L3 caches on Qualcomm 
>>> SoCs.
>>> +
>>>  config INTERCONNECT_QCOM_QCS404
>>>         tristate "Qualcomm QCS404 interconnect driver"
>>>         depends on INTERCONNECT_QCOM
>>> diff --git a/drivers/interconnect/qcom/Makefile 
>>> b/drivers/interconnect/qcom/Makefile
>>> index 9adf9e380545e..8d86d6515ffc9 100644
>>> --- a/drivers/interconnect/qcom/Makefile
>>> +++ b/drivers/interconnect/qcom/Makefile
>>> @@ -1,10 +1,12 @@
>>>  # SPDX-License-Identifier: GPL-2.0
>>> 
>>> +icc-osm-l3-objs                                := osm-l3.o
>>>  qnoc-msm8974-objs                      := msm8974.o
>>>  qnoc-qcs404-objs                       := qcs404.o
>>>  qnoc-sdm845-objs                       := sdm845.o
>>>  icc-smd-rpm-objs                       := smd-rpm.o
>>> 
>>> +obj-$(CONFIG_INTERCONNECT_QCOM_OSM_L3) += icc-osm-l3.o
>>>  obj-$(CONFIG_INTERCONNECT_QCOM_MSM8974) += qnoc-msm8974.o
>>>  obj-$(CONFIG_INTERCONNECT_QCOM_QCS404) += qnoc-qcs404.o
>>>  obj-$(CONFIG_INTERCONNECT_QCOM_SDM845) += qnoc-sdm845.o
>>> diff --git a/drivers/interconnect/qcom/osm-l3.c 
>>> b/drivers/interconnect/qcom/osm-l3.c
>>> new file mode 100644
>>> index 0000000000000..5e9f9ce02863b
>>> --- /dev/null
>>> +++ b/drivers/interconnect/qcom/osm-l3.c
>>> @@ -0,0 +1,284 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Copyright (c) 2019, The Linux Foundation. All rights reserved.
>>> + *
>>> + */
>>> +
>>> +#include <dt-bindings/interconnect/qcom,osm-l3.h>
>>> +#include <dt-bindings/interconnect/qcom,sdm845.h>
>>> +#include <linux/bitfield.h>
>>> +#include <linux/clk.h>
>>> +#include <linux/interconnect-provider.h>
>>> +#include <linux/io.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/of_platform.h>
>>> +#include <linux/platform_device.h>
>>> +
>>> +#define LUT_MAX_ENTRIES                        40U
>>> +#define LUT_SRC                                GENMASK(31, 30)
>>> +#define LUT_L_VAL                      GENMASK(7, 0)
>>> +#define LUT_ROW_SIZE                   32
>>> +#define CLK_HW_DIV                     2
>>> +
>>> +/* Register offsets */
>>> +#define REG_ENABLE                     0x0
>>> +#define REG_FREQ_LUT                   0x110
>>> +#define REG_PERF_STATE                 0x920
>>> +
>>> +#define OSM_L3_MAX_LINKS               1
>>> +
>>> +#define to_qcom_provider(_provider) \
>>> +       container_of(_provider, struct qcom_osm_l3_icc_provider, 
>>> provider)
>>> +
>>> +enum {
>>> +       SDM845_MASTER_OSM_L3_APPS = SLAVE_TCU + 1,
>>> +       SDM845_SLAVE_OSM_L3,
>>> +};
>> 
>> Should these just go in qcom,sdm845.h? Seems nice to have them all in
>> one place, and then they can be accessed in the DT if needed.
> 
> yeah I can do that, by doing
> this I can also get rid of the
> SLAVE_TCU + 1 :)
> 
>> 
>>> +
>>> +struct qcom_osm_l3_icc_provider {
>>> +       void __iomem *base;
>>> +       unsigned int max_state;
>>> +       unsigned long lut_tables[LUT_MAX_ENTRIES];
>>> +       struct icc_provider provider;
>>> +};
>>> +
>>> +/**
>>> + * struct qcom_icc_node - Qualcomm specific interconnect nodes
>>> + * @name: the node name used in debugfs
>>> + * @links: an array of nodes where we can go next while traversing
>>> + * @id: a unique node identifier
>>> + * @num_links: the total number of @links
>>> + * @buswidth: width of the interconnect between a node and the bus
>>> + */
>>> +struct qcom_icc_node {
>>> +       const char *name;
>>> +       u16 links[OSM_L3_MAX_LINKS];
>>> +       u16 id;
>>> +       u16 num_links;
>>> +       u16 buswidth;
>>> +};
>>> +
>>> +struct qcom_icc_desc {
>>> +       struct qcom_icc_node **nodes;
>>> +       size_t num_nodes;
>>> +};
>>> +
>>> +#define DEFINE_QNODE(_name, _id, _buswidth, ...)                     
>>>   \
>>> +               static struct qcom_icc_node _name = {                 
>>>   \
>>> +               .name = #_name,                                       
>>>   \
>>> +               .id = _id,                                            
>>>   \
>>> +               .buswidth = _buswidth,                                
>>>   \
>>> +               .num_links = ARRAY_SIZE(((int[]){ __VA_ARGS__ })),    
>>>   \
>>> +               .links = { __VA_ARGS__ },                             
>>>   \
>>> +       }
>>> +
>>> +DEFINE_QNODE(osm_apps_l3, SDM845_MASTER_OSM_L3_APPS, 16, 
>>> SDM845_SLAVE_OSM_L3);
>>> +DEFINE_QNODE(osm_l3, SDM845_SLAVE_OSM_L3, 16);
>>> +
>>> +static struct qcom_icc_node *sdm845_osm_l3_nodes[] = {
>> 
>> const?
> 
> unfortunately we can't ...
> 
> data->nodes[i] = node;
> we setup links later ^^ with
> the pointer.
> 
>> 
>>> +       [MASTER_OSM_L3_APPS] = &osm_apps_l3,
>>> +       [SLAVE_OSM_L3] = &osm_l3,
>>> +};
>>> +
>>> +static struct qcom_icc_desc sdm845_osm_l3 = {
>>> +       .nodes = sdm845_osm_l3_nodes,
>>> +       .num_nodes = ARRAY_SIZE(sdm845_osm_l3_nodes),
>>> +};
>>> +
>>> +static int qcom_icc_aggregate(struct icc_node *node, u32 tag, u32 
>>> avg_bw,
>>> +                             u32 peak_bw, u32 *agg_avg, u32 
>>> *agg_peak)
>>> +{
>>> +       *agg_avg += avg_bw;
>>> +       *agg_peak = max_t(u32, *agg_peak, peak_bw);
>>> +
>>> +       return 0;
>>> +}
>> 
>> Georgi, I wonder if it's a good idea to make a small collection of
>> "std" aggregate functions in the interconnect core that a driver can
>> just point to if it's doing something super standard like this (ie
>> driver->aggregate = icc_std_aggregate;). This is probably fine as-is
>> for now, but if we see a lot more copy/pastes of this function we
>> should think about it.
>> 
>>> +
>>> +static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
>>> +{
>>> +       struct qcom_osm_l3_icc_provider *qp;
>>> +       struct icc_provider *provider;
>>> +       struct qcom_icc_node *qn;
>>> +       struct icc_node *n;
>>> +       unsigned int index;
>>> +       u32 agg_peak = 0;
>>> +       u32 agg_avg = 0;
>>> +       u64 rate;
>>> +
>>> +       qn = src->data;
>>> +       provider = src->provider;
>>> +       qp = to_qcom_provider(provider);
>>> +
>>> +       list_for_each_entry(n, &provider->nodes, node_list)
>>> +               qcom_icc_aggregate(n, 0, n->avg_bw, n->peak_bw,
>>> +                                  &agg_avg, &agg_peak);
>>> +
>>> +       rate = max(agg_avg, agg_peak);
>>> +       rate = icc_units_to_bps(rate);
>>> +       do_div(rate, qn->buswidth);
>>> +
>>> +       for (index = 0; index < qp->max_state; index++) {
>> 
>> If the rate is too high, you'll end up setting max_state into the
>> register. That's probably bad, right? (Or maybe it's not because the
>> table ends with the same value twice, but it seems like relying on an
>> implementation detail). We could guard against that by only iterating
>> to index < qp->max_state - 1.
> 
> yes, using max_state - 1 makes sense
> here. Will change this in the next
> re-spin.
> 
>> 
>>> +               if (qp->lut_tables[index] >= rate)
>>> +                       break;
>>> +       }
>>> +
>>> +       writel_relaxed(index, qp->base + REG_PERF_STATE);
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static int qcom_osm_l3_remove(struct platform_device *pdev)
>>> +{
>>> +       struct qcom_osm_l3_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) {
>> 
>> There was a comment on one of the other threads that we've been
>> copy/pasting this snippet around and it's wrong because it doesn't use
>> the _safe variant of the macro. So we end up destroying the list we're
>> iterating over.
>> 
>> Georgi, did you have a plan to refactor this, or should we just change
>> this to be the _safe version?
>> 
>>> +               icc_node_del(n);
>>> +               icc_node_destroy(n->id);
>>> +       }
>>> +
>>> +       return icc_provider_del(provider);
>>> +}
>>> +
>>> +static int qcom_osm_l3_probe(struct platform_device *pdev)
>>> +{
>>> +       u32 info, src, lval, i, prev_freq = 0, freq;
>>> +       static unsigned long hw_rate, xo_rate;
>>> +       struct qcom_osm_l3_icc_provider *qp;
>>> +       const struct qcom_icc_desc *desc;
>>> +       struct icc_onecell_data *data;
>>> +       struct icc_provider *provider;
>>> +       struct qcom_icc_node **qnodes;
>>> +       struct icc_node *node;
>>> +       size_t num_nodes;
>>> +       struct clk *clk;
>>> +       int ret;
>>> +
>>> +       clk = clk_get(&pdev->dev, "xo");
>>> +       if (IS_ERR(clk))
>>> +               return PTR_ERR(clk);
>>> +
>>> +       xo_rate = clk_get_rate(clk);
>>> +       clk_put(clk);
>>> +
>>> +       clk = clk_get(&pdev->dev, "alternate");
>>> +       if (IS_ERR(clk))
>>> +               return PTR_ERR(clk);
>>> +
>>> +       hw_rate = clk_get_rate(clk) / CLK_HW_DIV;
>> 
>> It's a little weird there's a constant divide in there, though I guess
>> it's in the cpufreq driver as well. I guess this is fine if it's
>> likely to stay there (and the same) when this driver is generalized
>> for other SoCs.
> 
> yeah I don't see this changing
> on sc7180 so I'll leave this
> as is.
> 
>> 
>>> +       clk_put(clk);
>>> +
>>> +       qp = devm_kzalloc(&pdev->dev, sizeof(*qp), GFP_KERNEL);
>>> +       if (!qp)
>>> +               return -ENOMEM;
>>> +
>>> +       qp->base = devm_platform_ioremap_resource(pdev, 0);
>>> +       if (IS_ERR(qp->base))
>>> +               return PTR_ERR(qp->base);
>>> +
>>> +       /* HW should be in enabled state to proceed */
>>> +       if (!(readl_relaxed(qp->base + REG_ENABLE) & 0x1)) {
>>> +               dev_err(&pdev->dev, "error hardware not enabled\n");
>>> +               return -ENODEV;
>>> +       }
>>> +
>>> +       for (i = 0; i < LUT_MAX_ENTRIES; i++) {
>>> +               info = readl_relaxed(qp->base + REG_FREQ_LUT +
>>> +                                    i * LUT_ROW_SIZE);
>>> +               src = FIELD_GET(LUT_SRC, info);
>>> +               lval = FIELD_GET(LUT_L_VAL, info);
>>> +               if (src)
>>> +                       freq = xo_rate * lval;
>>> +               else
>>> +                       freq = hw_rate;
>>> +
>>> +               /*
>>> +                * Two of the same frequencies with the same core 
>>> counts means
>> 
>> "core counts" seems like a copied comment that doesn't apply.
> 
> yes we don't use the core_cnt
> field so will update the comment.
> 
>> 
>> But you only look at freq and not core count, is that really
>> equivalent to the table's boundary condition? Or do you need to be
>> comparing info == info_prev?
> 
> no we can get by comparing
> current freq with prev freq
> on OSM L3.
> 
>> 
>>> +                * end of table
>>> +                */
>>> +               if (i > 0 && prev_freq == freq)
>>> +                       break;
>>> +
>>> +               qp->lut_tables[i] = freq;
>>> +               prev_freq = freq;
>>> +       }
>>> +       qp->max_state = i;
>> 
>> Should we error out or complain if there are too few entries, or if
>> the table is not in increasing order?
> 
> OSM does make sure of the
> increasing order and correct
> number of freq entries if the
> REG_ENABLE is set. However I
> don't really mind adding the
> increasing order check but we
> can't really detect too few
> entries since the number can
> vary across SKUs.
> 
>> 
>>> +
>>> +       desc = of_device_get_match_data(&pdev->dev);
>>> +       if (!desc)
>>> +               return -EINVAL;
>>> +
>>> +       qnodes = desc->nodes;
>>> +       num_nodes = desc->num_nodes;
>>> +
>>> +       data = devm_kcalloc(&pdev->dev, num_nodes, sizeof(*node), 
>>> GFP_KERNEL);
>>> +       if (!data)
>>> +               return -ENOMEM;
>>> +
>>> +       provider = &qp->provider;
>>> +       provider->dev = &pdev->dev;
>>> +       provider->set = qcom_icc_set;
>>> +       provider->aggregate = qcom_icc_aggregate;
>>> +       provider->xlate = of_icc_xlate_onecell;
>>> +       INIT_LIST_HEAD(&provider->nodes);
>>> +       provider->data = data;
>>> +
>>> +       ret = icc_provider_add(provider);
>>> +       if (ret) {
>>> +               dev_err(&pdev->dev, "error adding interconnect 
>>> provider\n");
>>> +               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 %p %s %d\n", 
>>> node,
>>> +                       qnodes[i]->name, node->id);
>>> +
>>> +               /* populate links */
>> 
>> Not a super useful comment.
> 
> lol will remove it
> 
>> 
>> 
>>> +               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:
>>> +       qcom_osm_l3_remove(pdev);
>>> +       return ret;
>>> +}
>>> +
>>> +static const struct of_device_id osm_l3_of_match[] = {
>>> +       { .compatible = "qcom,sdm845-osm-l3", .data = &sdm845_osm_l3 
>>> },
>>> +       { },
>>> +};
>>> +MODULE_DEVICE_TABLE(of, osm_l3_of_match);
>>> +
>>> +static struct platform_driver osm_l3_driver = {
>>> +       .probe = qcom_osm_l3_probe,
>>> +       .remove = qcom_osm_l3_remove,
>>> +       .driver = {
>>> +               .name = "osm-l3",
>>> +               .of_match_table = osm_l3_of_match,
>>> +       },
>>> +};
>>> +module_platform_driver(osm_l3_driver);
>>> +
>>> +MODULE_DESCRIPTION("Qualcomm OSM L3 interconnect driver");
>>> +MODULE_LICENSE("GPL v2");
>>> --
>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>>> Forum,
>>> a Linux Foundation Collaborative Project
>>> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH v3 2/2] interconnect: qcom: Add OSM L3 interconnect provider support

[Evan Green]

On Wed, Nov 27, 2019 at 12:42 AM Sibi Sankar <sibis@codeaurora.org> wrote:
>
> Hey Evan/Georgi,
>
> https://git.linaro.org/people/georgi.djakov/linux.git/commit/?h=icc-dev&id=9197da7d06e88666d1588e3c21a743e60381264d
>
> With the "Redefine interconnect provider
> DT nodes for SDM845" series, wouldn't it
> make more sense to define the OSM_L3 icc
> nodes in the sdm845.c icc driver and have
> the common helpers in osm_l3 driver? Though
> we don't plan on linking the OSM L3 nodes
> to the other nodes on SDM845/SC7180, we
> might have GPU needing to be linked to the
> OSM L3 nodes on future SoCs. Let me know
> how you want this done.
>
> Anyway I'll re-spin the series once the
> SDM845 icc re-work gets re-posted.

I don't have a clear picture of the proposal. You'd put the couple of
extra defines in sdm845.c for the new nodes. But then you'd need to do
something in icc_set() of sdm845. Is that when you'd call out to the
osm_l3 driver?

Re: [PATCH v3 2/2] interconnect: qcom: Add OSM L3 interconnect provider support

[Sibi Sankar]

Hey Evan,

On 12/7/19 12:46 AM, Evan Green wrote:
> On Wed, Nov 27, 2019 at 12:42 AM Sibi Sankar <sibis@codeaurora.org> wrote:
>>
>> Hey Evan/Georgi,
>>
>> https://git.linaro.org/people/georgi.djakov/linux.git/commit/?h=icc-dev&id=9197da7d06e88666d1588e3c21a743e60381264d
>>
>> With the "Redefine interconnect provider
>> DT nodes for SDM845" series, wouldn't it
>> make more sense to define the OSM_L3 icc
>> nodes in the sdm845.c icc driver and have
>> the common helpers in osm_l3 driver? Though
>> we don't plan on linking the OSM L3 nodes
>> to the other nodes on SDM845/SC7180, we
>> might have GPU needing to be linked to the
>> OSM L3 nodes on future SoCs. Let me know
>> how you want this done.
>>
>> Anyway I'll re-spin the series once the
>> SDM845 icc re-work gets re-posted.
> 
> I don't have a clear picture of the proposal. You'd put the couple of
> extra defines in sdm845.c for the new nodes. But then you'd need to do
> something in icc_set() of sdm845. Is that when you'd call out to the
> osm_l3 driver?

with sdm845 icc rework "https://patchwork.kernel.org/cover/11293399/"
osm l3 icc provider needs to know the total number of rsc icc nodes,
i.e I can define the total number of rsc nodes and continue using the
same design as v3 since on sdm845/sc7180 gpu is not cache coherent.

or have the osm l3 table population logic and osm icc_set as helpers
and have it called from the sdm845/sc7180 icc driver so that we would
be able to link osm_l3 with rsc nodes on future qcom SoCs.

> 

-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 2/2] interconnect: qcom: Add OSM L3 interconnect provider support

[Evan Green]

On Mon, Dec 16, 2019 at 10:30 AM Sibi Sankar <sibis@codeaurora.org> wrote:
>
> Hey Evan,
>
> On 12/7/19 12:46 AM, Evan Green wrote:
> > On Wed, Nov 27, 2019 at 12:42 AM Sibi Sankar <sibis@codeaurora.org> wrote:
> >>
> >> Hey Evan/Georgi,
> >>
> >> https://git.linaro.org/people/georgi.djakov/linux.git/commit/?h=icc-dev&id=9197da7d06e88666d1588e3c21a743e60381264d
> >>
> >> With the "Redefine interconnect provider
> >> DT nodes for SDM845" series, wouldn't it
> >> make more sense to define the OSM_L3 icc
> >> nodes in the sdm845.c icc driver and have
> >> the common helpers in osm_l3 driver? Though
> >> we don't plan on linking the OSM L3 nodes
> >> to the other nodes on SDM845/SC7180, we
> >> might have GPU needing to be linked to the
> >> OSM L3 nodes on future SoCs. Let me know
> >> how you want this done.
> >>
> >> Anyway I'll re-spin the series once the
> >> SDM845 icc re-work gets re-posted.
> >
> > I don't have a clear picture of the proposal. You'd put the couple of
> > extra defines in sdm845.c for the new nodes. But then you'd need to do
> > something in icc_set() of sdm845. Is that when you'd call out to the
> > osm_l3 driver?
>
> with sdm845 icc rework "https://patchwork.kernel.org/cover/11293399/"
> osm l3 icc provider needs to know the total number of rsc icc nodes,
> i.e I can define the total number of rsc nodes and continue using the
> same design as v3 since on sdm845/sc7180 gpu is not cache coherent.
>
> or have the osm l3 table population logic and osm icc_set as helpers
> and have it called from the sdm845/sc7180 icc driver so that we would
> be able to link osm_l3 with rsc nodes on future qcom SoCs.

I see, so if we use the same design as v3, then the number of nodes is
established at compile-time, and ends up being specific to sdm845. I'm
fine with either approach, maybe leaning towards the hardcoded
#defines you have now, and waiting to do the refactoring until you
actually have two SoCs that can use this.
-Evan

Re: [PATCH v3 2/2] interconnect: qcom: Add OSM L3 interconnect provider support

[Sibi Sankar]

On 2020-01-08 00:44, Evan Green wrote:
> On Mon, Dec 16, 2019 at 10:30 AM Sibi Sankar <sibis@codeaurora.org> 
> wrote:
>> 
>> Hey Evan,
>> 
>> On 12/7/19 12:46 AM, Evan Green wrote:
>> > On Wed, Nov 27, 2019 at 12:42 AM Sibi Sankar <sibis@codeaurora.org> wrote:
>> >>
>> >> Hey Evan/Georgi,
>> >>
>> >> https://git.linaro.org/people/georgi.djakov/linux.git/commit/?h=icc-dev&id=9197da7d06e88666d1588e3c21a743e60381264d
>> >>
>> >> With the "Redefine interconnect provider
>> >> DT nodes for SDM845" series, wouldn't it
>> >> make more sense to define the OSM_L3 icc
>> >> nodes in the sdm845.c icc driver and have
>> >> the common helpers in osm_l3 driver? Though
>> >> we don't plan on linking the OSM L3 nodes
>> >> to the other nodes on SDM845/SC7180, we
>> >> might have GPU needing to be linked to the
>> >> OSM L3 nodes on future SoCs. Let me know
>> >> how you want this done.
>> >>
>> >> Anyway I'll re-spin the series once the
>> >> SDM845 icc re-work gets re-posted.
>> >
>> > I don't have a clear picture of the proposal. You'd put the couple of
>> > extra defines in sdm845.c for the new nodes. But then you'd need to do
>> > something in icc_set() of sdm845. Is that when you'd call out to the
>> > osm_l3 driver?
>> 
>> with sdm845 icc rework "https://patchwork.kernel.org/cover/11293399/"
>> osm l3 icc provider needs to know the total number of rsc icc nodes,
>> i.e I can define the total number of rsc nodes and continue using the
>> same design as v3 since on sdm845/sc7180 gpu is not cache coherent.
>> 
>> or have the osm l3 table population logic and osm icc_set as helpers
>> and have it called from the sdm845/sc7180 icc driver so that we would
>> be able to link osm_l3 with rsc nodes on future qcom SoCs.
> 
> I see, so if we use the same design as v3, then the number of nodes is
> established at compile-time, and ends up being specific to sdm845. I'm
> fine with either approach, maybe leaning towards the hardcoded
> #defines you have now, and waiting to do the refactoring until you
> actually have two SoCs that can use this.
> -Evan

Thanks will stick to the #defines
for now.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH v3 2/2] interconnect: qcom: Add OSM L3 interconnect provider support

[Georgi Djakov]

Hi Evan,

On 11/19/19 00:42, Evan Green wrote:
> Hi Sibi,
> 
> On Mon, Nov 18, 2019 at 7:45 AM Sibi Sankar <sibis@codeaurora.org> wrote:
>>
>> On some Qualcomm SoCs, Operating State Manager (OSM) controls the
>> resources of scaling L3 caches. Add a driver to handle bandwidth
>> requests to OSM L3 from CPU/GPU.
>>
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> ---
>>  drivers/interconnect/qcom/Kconfig  |   7 +
>>  drivers/interconnect/qcom/Makefile |   2 +
>>  drivers/interconnect/qcom/osm-l3.c | 284 +++++++++++++++++++++++++++++
>>  3 files changed, 293 insertions(+)
>>  create mode 100644 drivers/interconnect/qcom/osm-l3.c
>>
[..]
>> +static int qcom_icc_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
>> +                             u32 peak_bw, u32 *agg_avg, u32 *agg_peak)
>> +{
>> +       *agg_avg += avg_bw;
>> +       *agg_peak = max_t(u32, *agg_peak, peak_bw);
>> +
>> +       return 0;
>> +}
> 
> Georgi, I wonder if it's a good idea to make a small collection of
> "std" aggregate functions in the interconnect core that a driver can
> just point to if it's doing something super standard like this (ie
> driver->aggregate = icc_std_aggregate;). This is probably fine as-is
> for now, but if we see a lot more copy/pastes of this function we
> should think about it.

Sure, thanks for the suggestion! Will submit a patch for this.

> 
>> +
>> +static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
>> +{
[..]
>> +
>> +static int qcom_osm_l3_remove(struct platform_device *pdev)
>> +{
>> +       struct qcom_osm_l3_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) {
> 
> There was a comment on one of the other threads that we've been
> copy/pasting this snippet around and it's wrong because it doesn't use
> the _safe variant of the macro. So we end up destroying the list we're
> iterating over.
> 
> Georgi, did you have a plan to refactor this, or should we just change
> this to be the _safe version?

I have fixes that convert this to the _safe variant (for stable) and also
factor this out into icc_nodes_remove() function. Will post them.

Thanks,
Georgi

> 
>> +               icc_node_del(n);
>> +               icc_node_destroy(n->id);
>> +       }
>> +
>> +       return icc_provider_del(provider);
>> +}