Re: [v6 2/3] interconnect: qcom: Add EPSS L3 support on SC7280

From: okukatla
Date: Mon Aug 16 2021 - 13:43:10 EST

On 2021-08-10 18:16, Alex Elder wrote:
On 8/10/21 1:46 AM, Odelu Kukatla wrote:
Add Epoch Subsystem (EPSS) L3 interconnect provider support on
SC7280 SoCs.

Signed-off-by: Odelu Kukatla <okukatla@xxxxxxxxxxxxxx>

I don't have much to say about what this is doing but I
have a few suggestions.

-Alex

Thanks for review Alex!
---
drivers/interconnect/qcom/osm-l3.c | 136 +++++++++++++++++++++++++++++++------
drivers/interconnect/qcom/sc7280.h | 10 +++
2 files changed, 125 insertions(+), 21 deletions(-)

diff --git a/drivers/interconnect/qcom/osm-l3.c b/drivers/interconnect/qcom/osm-l3.c
index c7af143..3b16e73 100644
--- a/drivers/interconnect/qcom/osm-l3.c
+++ b/drivers/interconnect/qcom/osm-l3.c
@@ -9,12 +9,14 @@
#include <linux/io.h>
#include <linux/kernel.h>
#include <linux/module.h>
+#include <linux/of_address.h>
#include <linux/of_device.h>
#include <linux/platform_device.h>
#include <dt-bindings/interconnect/qcom,osm-l3.h>
#include "sc7180.h"
+#include "sc7280.h"
#include "sc8180x.h"
#include "sdm845.h"
#include "sm8150.h"
@@ -33,17 +35,33 @@
/* EPSS Register offsets */
#define EPSS_LUT_ROW_SIZE 4
+#define EPSS_REG_L3_VOTE 0x90
#define EPSS_REG_FREQ_LUT 0x100
#define EPSS_REG_PERF_STATE 0x320
+#define EPSS_CORE_OFFSET 0x4
+#define EPSS_L3_VOTE_REG(base, cpu)\
+ (((base) + EPSS_REG_L3_VOTE) +\
+ ((cpu) * EPSS_CORE_OFFSET))
-#define OSM_L3_MAX_LINKS 1
+#define L3_DOMAIN_CNT 4
+#define L3_MAX_LINKS 9

It may not matter much, but if you specified the
qcom_osm_l3_node->links[] field as the last field
in the structure, I think it could be a flexible
array and you wouldn't have to specify the maximum
number of links. (You are already using the actual
array size to set ->num_links in __DEFINE_QNODE().)

I will address this in next revision, will see if we can move it to flexible array.
#define to_osm_l3_provider(_provider) \
container_of(_provider, struct qcom_osm_l3_icc_provider, provider)
+/**
+ * @domain_base: an array of base address for each clock domain
+ * @max_state: max supported frequency level
+ * @per_core_dcvs: flag used to indicate whether the frequency scaling
+ * for each core is enabled
+ * @reg_perf_state: requested frequency level
+ * @lut_tables: an array of supported frequency levels
+ * @provider: interconnect provider of this node
+ */

Run this to check your kernel doc validity:
scripts/kernel-doc -none <file> [<file>...]

Done!
struct qcom_osm_l3_icc_provider {
- void __iomem *base;
+ void __iomem *domain_base[L3_DOMAIN_CNT];
unsigned int max_state;
+ bool per_core_dcvs;
unsigned int reg_perf_state;
unsigned long lut_tables[LUT_MAX_ENTRIES];
struct icc_provider provider;

. . .

@@ -235,12 +322,17 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
if (!qp)
return -ENOMEM;
- qp->base = devm_platform_ioremap_resource(pdev, 0);
- if (IS_ERR(qp->base))
- return PTR_ERR(qp->base);
+ while (of_get_address(pdev->dev.of_node, i++, NULL, NULL))
+ nr_domain_bases++;

Maybe you could combine these two loops by counting as you go.
I.e.:

i = 0;
while (true) {
void __iomem *base;

if (of_get_address(pdev->dev.of_node, i, NULL, NULL))
break;
base = devm_platform_ioremap_resource(pdev, i);
if (IS_ERR(base))
return PTR_ERR(base);
qp->domain_base[i++] = base
}
nr_domain_bases = i;

Not exactly as above, but will merge these two loops.
+
+ for (i = 0; i < nr_domain_bases ; i++) {
+ qp->domain_base[i] = devm_platform_ioremap_resource(pdev, i);
+ if (IS_ERR(qp->domain_base[i]))
+ return PTR_ERR(qp->domain_base[i]);
+ }
/* HW should be in enabled state to proceed */
- if (!(readl_relaxed(qp->base + REG_ENABLE) & 0x1)) {
+ if (!(readl_relaxed(qp->domain_base[0] + REG_ENABLE) & 0x1)) {
dev_err(&pdev->dev, "error hardware not enabled\n");
return -ENODEV;
}
@@ -252,7 +344,7 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
qp->reg_perf_state = desc->reg_perf_state;
for (i = 0; i < LUT_MAX_ENTRIES; i++) {
- info = readl_relaxed(qp->base + desc->reg_freq_lut +
+ info = readl_relaxed(qp->domain_base[0] + desc->reg_freq_lut +
i * desc->lut_row_size);

Maybe you could define a macro to encapsulate computing this
register offset, along the lines of EPSS_L3_VOTE_REG(). (Here
and elsewhere.)

This register OFFSET calculation is here only, will keep this code as is.
src = FIELD_GET(LUT_SRC, info);
lval = FIELD_GET(LUT_L_VAL, info);
@@ -271,6 +363,7 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
prev_freq = freq;
}
qp->max_state = i;
+ qp->per_core_dcvs = desc->per_core_dcvs;
qnodes = desc->nodes;
num_nodes = desc->num_nodes;
@@ -326,6 +419,7 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
static const struct of_device_id osm_l3_of_match[] = {
{ .compatible = "qcom,sc7180-osm-l3", .data = &sc7180_icc_osm_l3 },
+ { .compatible = "qcom,sc7280-epss-l3", .data = &sc7280_icc_epss_l3 },
{ .compatible = "qcom,sdm845-osm-l3", .data = &sdm845_icc_osm_l3 },
{ .compatible = "qcom,sm8150-osm-l3", .data = &sm8150_icc_osm_l3 },
{ .compatible = "qcom,sc8180x-osm-l3", .data = &sc8180x_icc_osm_l3 },
diff --git a/drivers/interconnect/qcom/sc7280.h b/drivers/interconnect/qcom/sc7280.h
index 175e400..5df7600 100644
--- a/drivers/interconnect/qcom/sc7280.h
+++ b/drivers/interconnect/qcom/sc7280.h
@@ -150,5 +150,15 @@
#define SC7280_SLAVE_PCIE_1 139
#define SC7280_SLAVE_QDSS_STM 140
#define SC7280_SLAVE_TCU 141
+#define SC7280_MASTER_EPSS_L3_APPS 142
+#define SC7280_SLAVE_EPSS_L3_SHARED 143
+#define SC7280_SLAVE_EPSS_L3_CPU0 144
+#define SC7280_SLAVE_EPSS_L3_CPU1 145
+#define SC7280_SLAVE_EPSS_L3_CPU2 146
+#define SC7280_SLAVE_EPSS_L3_CPU3 147
+#define SC7280_SLAVE_EPSS_L3_CPU4 148
+#define SC7280_SLAVE_EPSS_L3_CPU5 149
+#define SC7280_SLAVE_EPSS_L3_CPU6 150
+#define SC7280_SLAVE_EPSS_L3_CPU7 151
#endif