dt-bindings: interconnect: Add OSM L3 DT bindings
Subsystems (3)
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH v3 1/2 dt-bindings: interconnect: Add OSM L3 DT bindings
Add bindings for Operating State Manager (OSM) L3 interconnect provider on SDM845 SoCs.
--- .../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
Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml new file +56 -0
@@ -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 <[email protected]> + +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>; + };
include/dt-bindings/interconnect/qcom,osm-l3.h new file +12 -0
@@ -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
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Quoting Sibi Sankar (2019-11-18 07:45:21)
Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml line 7
Quoted 13 lines
Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml new file +7
-0@@ -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.
Quoted 4 lines
Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml new file +4
-0+ clock-names = "xo", "alternate"; + + #interconnect-cells = <1>; + };
Hey Stephen,
Thanks for taking time to
review the series.
Quoted 17 lines · Stephen Boyd
Quoting Sibi Sankar (2019-11-18 07:45:21)
Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml line 7
Quoted 13 lines
Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml new file +7
-0@@ -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.
Quoted 5 lines
Quoted 5 lines
Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml new file +4
-0+ clock-names = "xo", "alternate"; + + #interconnect-cells = <1>; + };
PATCH v3 2/2 interconnect: qcom: Add OSM L3 interconnect provider support
In reply to a message not in the archiveOn 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.
--- 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
drivers/interconnect/qcom/Kconfig +7 -0
@@ -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
drivers/interconnect/qcom/Makefile +2 -0
@@ -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
drivers/interconnect/qcom/osm-l3.c new file +284 -0
284 lines not on this page · raw message
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
A message that is not in the archive (sent off-list or not yet received): 0101016e7f30ad15-18908ef0-a2b9-4a2a-bf32-6cb3aa447b01-000000@us-west-2.amazonses.com
Re: [PATCH v3 2/2] interconnect: qcom: Add OSM L3 interconnect provider support
Hi Sibi,
Quoted 18 lines · 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 <[email protected]>--- 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.cdrivers/interconnect/qcom/Kconfig +0
-0
drivers/interconnect/qcom/Kconfig config INTERCONNECT_QCOM · line 10
Quoted 6 lines
drivers/interconnect/qcom/Kconfig +3
-0@@ -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?
Quoted 58 lines
drivers/interconnect/qcom/Kconfig +4
-0+ 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_QCOMdrivers/interconnect/qcom/Makefile +2
-0@@ -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.odrivers/interconnect/qcom/osm-l3.c new file +28
-0@@ -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
drivers/interconnect/qcom/osm-l3.c line 38
Quoted 10 lines
drivers/interconnect/qcom/osm-l3.c new file +10
-0+ +#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.
Quoted 32 lines
drivers/interconnect/qcom/osm-l3.c new file +32
-0+ +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, \
drivers/interconnect/qcom/osm-l3.c line 80
Quoted 10 lines
drivers/interconnect/qcom/osm-l3.c new file +10
-0+ .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?
Quoted 7 lines
drivers/interconnect/qcom/osm-l3.c new file +7
-0+ [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),
drivers/interconnect/qcom/osm-l3.c line 97
Quoted 10 lines
drivers/interconnect/qcom/osm-l3.c new file +10
-0+}; + +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.
Quoted 15 lines
drivers/interconnect/qcom/osm-l3.c new file +15
-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);
drivers/interconnect/qcom/osm-l3.c line 122
Quoted 10 lines
drivers/interconnect/qcom/osm-l3.c new file +10
-0+ +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.
Quoted 6 lines
drivers/interconnect/qcom/osm-l3.c new file +6
-0+if (qp->lut_tables[index] >= rate) +break ; + } + +writel_relaxed (index, qp->base +REG_PERF_STATE ); +
drivers/interconnect/qcom/osm-l3.c line 138
Quoted 10 lines
drivers/interconnect/qcom/osm-l3.c new file +10
-0+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?
Quoted 23 lines
drivers/interconnect/qcom/osm-l3.c new file +23
-0+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))
drivers/interconnect/qcom/osm-l3.c line 171
Quoted 10 lines
drivers/interconnect/qcom/osm-l3.c new file +10
-0+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.
Quoted 18 lines
drivers/interconnect/qcom/osm-l3.c new file +18
-0+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 +
drivers/interconnect/qcom/osm-l3.c line 199
Quoted 10 lines
drivers/interconnect/qcom/osm-l3.c new file +10
-0+ 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?
drivers/interconnect/qcom/osm-l3.c line 208
Quoted 9 lines
drivers/interconnect/qcom/osm-l3.c new file +9
-0+* 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?
Quoted 33 lines
drivers/interconnect/qcom/osm-l3.c new file +33
-0+ + 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;
drivers/interconnect/qcom/osm-l3.c line 251
Quoted 10 lines
drivers/interconnect/qcom/osm-l3.c new file +10
-0+ } + + 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.
Quoted 37 lines
drivers/interconnect/qcom/osm-l3.c new file +33
-1+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
Hey Evan,
Thanks for taking time to review the series.
Quoted 26 lines · Evan Green
Hi Sibi,
Quoted 23 lines · 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 <[email protected]>--- 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.cdrivers/interconnect/qcom/Kconfig +0
-0@@ -5,6 +5,13 @@ config INTERCONNECT_QCOM help Support for Qualcomm's Network-on-Chip interconnecthardware.
Quoted 5 lines
drivers/interconnect/qcom/Kconfig
Quoted 3 lines
drivers/interconnect/qcom/Kconfig +3
-0+config INTERCONNECT_QCOM_OSM_L3 + tristate "Qualcomm OSM L3 interconnect driver" + depends on INTERCONNECT_QCOM || COMPILE_TESTShould 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.
Quoted 67 lines
Quoted 67 lines
drivers/interconnect/qcom/Kconfig +2
-0+ help + Say y here to support the Operating State Manager (OSM)interconnect
+ driver which controls the scaling of L3 caches on Qualcomm
SoCs.drivers/interconnect/qcom/Kconfig +1
-0+ config INTERCONNECT_QCOM_QCS404 tristate "Qualcomm QCS404 interconnect driver" depends on INTERCONNECT_QCOMdrivers/interconnect/qcom/Makefile +2
-0@@ -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.odrivers/interconnect/qcom/osm-l3.c new file +33
-0@@ -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)
Quoted 8 lines
drivers/interconnect/qcom/osm-l3.c
Quoted 5 lines
drivers/interconnect/qcom/osm-l3.c new file +5
-0+ +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 :)
Quoted 45 lines
Quoted 45 lines
drivers/interconnect/qcom/osm-l3.c new file +37
-0+ +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__ }, \ + }
Quoted 8 lines
drivers/interconnect/qcom/osm-l3.c
Quoted 6 lines
drivers/interconnect/qcom/osm-l3.c new file +2
-0+ +DEFINE_QNODE(osm_apps_l3, SDM845_MASTER_OSM_L3_APPS, 16,
SDM845_SLAVE_OSM_L3);drivers/interconnect/qcom/osm-l3.c new file +3
-0+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.
Quoted 51 lines
Quoted 14 lines
drivers/interconnect/qcom/osm-l3.c new file +10
-0+ [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,
+ u32peak_bw, u32*agg_avg, u32*agg_peak)drivers/interconnect/qcom/osm-l3.c
Quoted 6 lines
drivers/interconnect/qcom/osm-l3.c new file +6
-0+{ + *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.Quoted 23 lines
drivers/interconnect/qcom/osm-l3.c new file +23
-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);
Quoted 8 lines
drivers/interconnect/qcom/osm-l3.c
Quoted 2 lines
drivers/interconnect/qcom/osm-l3.c new file +2
-0+ +for (index =0 ; index < qp->max_state; index++) {If the rate is too high, you'll end up setting
max_stateinto 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.
Quoted 57 lines
Quoted 14 lines
drivers/interconnect/qcom/osm-l3.c new file +12
-0+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);drivers/interconnect/qcom/osm-l3.c
Quoted 4 lines
drivers/interconnect/qcom/osm-l3.c new file +4
-0+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
_safevariant 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
_safeversion?Quoted 30 lines
drivers/interconnect/qcom/osm-l3.c new file +30
-0+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))
Quoted 8 lines
drivers/interconnect/qcom/osm-l3.c
Quoted 3 lines
drivers/interconnect/qcom/osm-l3.c new file +3
-0+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.
Quoted 24 lines
Quoted 24 lines
drivers/interconnect/qcom/osm-l3.c new file +23
-0+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;
Quoted 8 lines
Quoted 6 lines
drivers/interconnect/qcom/osm-l3.c new file +5
-0+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.
Quoted 4 lines
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.
Quoted 11 lines
drivers/interconnect/qcom/osm-l3.c
Quoted 10 lines
drivers/interconnect/qcom/osm-l3.c new file +9
-0+* end of table +*/ +if (i >0 && prev_freq == freq) +break ; + + qp->lut_tables[i] = freq; + prev_freq = freq; + } + qp->max_state = i;
Quoted 2 lines
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.
Quoted 41 lines
Quoted 41 lines
drivers/interconnect/qcom/osm-l3.c new file +9
-0+ + 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);drivers/interconnect/qcom/osm-l3.c new file +14
-0+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 interconnectprovider\n");
drivers/interconnect/qcom/osm-l3.c new file +15
-0+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);
Quoted 8 lines
drivers/interconnect/qcom/osm-l3.c
Quoted 6 lines
drivers/interconnect/qcom/osm-l3.c new file +2
-0+ + dev_dbg(&pdev->dev, "registered node %p %s %d\n",node,
drivers/interconnect/qcom/osm-l3.c new file +3
-0+ qnodes[i]->name, node->id); + +/* populate links */ Not a super useful comment.
lol will remove it
Quoted 40 lines
Quoted 40 lines
drivers/interconnect/qcom/osm-l3.c new file +17
-0+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},
drivers/interconnect/qcom/osm-l3.c new file +16
-1+ { }, +}; +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
Hey Evan/Georgi,
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.
Quoted 478 lines · On 2019-11-19 17:30, [email protected] wrote:
Hey Evan,
Thanks for taking time to review the series.Quoted 26 lines · Evan Green
Hi Sibi,
Quoted 23 lines · 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 <[email protected]>--- 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.cdrivers/interconnect/qcom/Kconfig +0
-0@@ -5,6 +5,13 @@ config INTERCONNECT_QCOM help Support for Qualcomm's Network-on-Chip interconnecthardware.
Quoted 5 lines
drivers/interconnect/qcom/Kconfig
Quoted 3 lines
drivers/interconnect/qcom/Kconfig +3
-0+config INTERCONNECT_QCOM_OSM_L3 + tristate "Qualcomm OSM L3 interconnect driver" + depends on INTERCONNECT_QCOM || COMPILE_TESTShould 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.Quoted 67 lines
Quoted 67 lines
drivers/interconnect/qcom/Kconfig +2
-0+ help + Say y here to support the Operating State Manager (OSM)interconnect
+ driver which controls the scaling of L3 caches on Qualcomm
SoCs.drivers/interconnect/qcom/Kconfig +1
-0+ config INTERCONNECT_QCOM_QCS404 tristate "Qualcomm QCS404 interconnect driver" depends on INTERCONNECT_QCOMdrivers/interconnect/qcom/Makefile +2
-0@@ -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.odrivers/interconnect/qcom/osm-l3.c new file +33
-0@@ -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)
Quoted 8 lines
drivers/interconnect/qcom/osm-l3.c
Quoted 5 lines
drivers/interconnect/qcom/osm-l3.c new file +5
-0+ +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 :)Quoted 45 lines
Quoted 45 lines
drivers/interconnect/qcom/osm-l3.c new file +37
-0+ +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__ }, \ + }Quoted 8 lines
drivers/interconnect/qcom/osm-l3.c
Quoted 6 lines
drivers/interconnect/qcom/osm-l3.c new file +2
-0+ +DEFINE_QNODE(osm_apps_l3, SDM845_MASTER_OSM_L3_APPS, 16,
SDM845_SLAVE_OSM_L3);drivers/interconnect/qcom/osm-l3.c new file +3
-0+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.Quoted 51 lines
Quoted 14 lines
drivers/interconnect/qcom/osm-l3.c new file +10
-0+ [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,
+ u32peak_bw, u32*agg_avg, u32*agg_peak)drivers/interconnect/qcom/osm-l3.c
Quoted 6 lines
drivers/interconnect/qcom/osm-l3.c new file +6
-0+{ + *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.Quoted 23 lines
drivers/interconnect/qcom/osm-l3.c new file +23
-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);Quoted 8 lines
drivers/interconnect/qcom/osm-l3.c
Quoted 2 lines
drivers/interconnect/qcom/osm-l3.c new file +2
-0+ +for (index =0 ; index < qp->max_state; index++) {If the rate is too high, you'll end up setting
max_stateinto 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.Quoted 57 lines
Quoted 14 lines
drivers/interconnect/qcom/osm-l3.c new file +12
-0+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);drivers/interconnect/qcom/osm-l3.c
Quoted 4 lines
drivers/interconnect/qcom/osm-l3.c new file +4
-0+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
_safevariant 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
_safeversion?Quoted 30 lines
drivers/interconnect/qcom/osm-l3.c new file +30
-0+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))Quoted 8 lines
drivers/interconnect/qcom/osm-l3.c
Quoted 3 lines
drivers/interconnect/qcom/osm-l3.c new file +3
-0+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.
Quoted 24 lines
Quoted 24 lines
drivers/interconnect/qcom/osm-l3.c new file +23
-0+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;Quoted 8 lines
Quoted 6 lines
drivers/interconnect/qcom/osm-l3.c new file +5
-0+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_cntfield so will update the comment.Quoted 4 lines
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.Quoted 11 lines
drivers/interconnect/qcom/osm-l3.c
Quoted 10 lines
drivers/interconnect/qcom/osm-l3.c new file +9
-0+* end of table +*/ +if (i >0 && prev_freq == freq) +break ; + + qp->lut_tables[i] = freq; + prev_freq = freq; + } + qp->max_state = i;Quoted 2 lines
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_ENABLEis 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.Quoted 41 lines
Quoted 41 lines
drivers/interconnect/qcom/osm-l3.c new file +9
-0+ + 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);drivers/interconnect/qcom/osm-l3.c new file +14
-0+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 interconnectprovider\n");
drivers/interconnect/qcom/osm-l3.c new file +15
-0+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);Quoted 8 lines
drivers/interconnect/qcom/osm-l3.c
Quoted 6 lines
drivers/interconnect/qcom/osm-l3.c new file +2
-0+ + dev_dbg(&pdev->dev, "registered node %p %s %d\n",node,
drivers/interconnect/qcom/osm-l3.c new file +3
-0+ qnodes[i]->name, node->id); + +/* populate links */ Not a super useful comment.
lol will remove it
Quoted 40 lines
Quoted 40 lines
drivers/interconnect/qcom/osm-l3.c new file +17
-0+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},
drivers/interconnect/qcom/osm-l3.c new file +16
-1+ { }, +}; +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
Signature · 2 lines
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
Quoted 16 lines · Sibi Sankar
Hey Evan/Georgi,
With the "Redefine interconnect provider DT nodes for SDM845" series, wouldn't it make more sense to define the
OSM_L3icc nodes in thesdm845.cicc driver and have the common helpers inosm_l3driver? 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.
Quoted 2 lines
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
Hey Evan,
Quoted 17 lines · Evan Green
Quoted 16 lines · Sibi Sankar
Hey Evan/Georgi,
With the "Redefine interconnect provider DT nodes for SDM845" series, wouldn't it make more sense to define the
OSM_L3icc nodes in thesdm845.cicc driver and have the common helpers inosm_l3driver? 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.
Quoted 7 lines
Quoted 2 lines
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.cfor the new nodes. But then you'd need to do something inicc_set()of sdm845. Is that when you'd call out to theosm_l3driver?
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.
Quoted 1 line
Signature · 3 lines
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
Quoted 29 lines · Sibi Sankar
Hey Evan,
Quoted 17 lines · Evan Green
Quoted 16 lines · Sibi Sankar
Hey Evan/Georgi,
With the "Redefine interconnect provider DT nodes for SDM845" series, wouldn't it make more sense to define the
OSM_L3icc nodes in thesdm845.cicc driver and have the common helpers inosm_l3driver? 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.Quoted 7 lines
Quoted 2 lines
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.cfor the new nodes. But then you'd need to do something inicc_set()of sdm845. Is that when you'd call out to theosm_l3driver?
Quoted 8 lines
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_setas helpers and have it called from the sdm845/sc7180 icc driver so that we would be able to linkosm_l3with 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
Quoted 39 lines · Evan Green
Quoted 29 lines · Sibi Sankar
Hey Evan,
Quoted 17 lines · Evan Green
Quoted 16 lines · Sibi Sankar
Hey Evan/Georgi,
With the "Redefine interconnect provider DT nodes for SDM845" series, wouldn't it make more sense to define the
OSM_L3icc nodes in thesdm845.cicc driver and have the common helpers inosm_l3driver? 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.Quoted 7 lines
Quoted 2 lines
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.cfor the new nodes. But then you'd need to do something inicc_set()of sdm845. Is that when you'd call out to theosm_l3driver?Quoted 8 lines
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_setas helpers and have it called from the sdm845/sc7180 icc driver so that we would be able to linkosm_l3with rsc nodes on future qcom SoCs.
Quoted 6 lines
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.
Signature · 2 lines
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
In reply to Evan GreenHi Evan,
Quoted 16 lines · Evan Green
Hi Sibi,
Quoted 13 lines · 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 <[email protected]>--- 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
[..]
Quoted 7 lines
Quoted 7 lines
(quoted diff) +7
-0+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;
Quoted 8 lines
Quoted 1 line
+}
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.
Quoted 4 lines
Quoted 4 lines
(quoted diff) +3
-0+ +static int qcom_icc_set(struct icc_node *src, struct icc_node *dst) +{
[..]
Quoted 14 lines
Review
Quoted 8 lines
(quoted diff) +8
-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
_safevariant of the macro. So we end up destroying the list we're iterating over.
Quoted 2 lines
Georgi, did you have a plan to refactor this, or should we just change this to be the
_safeversion?
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
Quoted 7 lines
Quoted 7 lines
(quoted diff) +6
-0+ icc_node_del(n); + icc_node_destroy(n->id); + } + + return icc_provider_del(provider); +}