* [V7 0/5] Add EPSS L3 provider support on SA8775P SoC
@ 2025-01-11 16:14 Raviteja Laggyshetty
2025-01-11 16:14 ` [PATCH V7 1/5] interconnect: core: Add dynamic id allocation support Raviteja Laggyshetty
` (4 more replies)
0 siblings, 5 replies; 15+ messages in thread
From: Raviteja Laggyshetty @ 2025-01-11 16:14 UTC (permalink / raw)
To: Georgi Djakov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, Odelu Kukatla, Mike Tipton,
Vivek Aknurwar
Cc: Sibi Sankar, linux-arm-msm, linux-pm, devicetree, linux-kernel
Add Epoch Subsystem (EPSS) L3 provider support on SA8775P SoCs.
Current interconnect framework is based on static IDs for creating node
and registering with framework. This becomes a limitation for topologies
where there are multiple instances of same interconnect provider. Add
icc_node_create_alloc_id() API to create icc node with dynamic id, this
will help to overcome the dependency on static IDs.
Change since v6:
- Added icc_node_create_alloc_id() API to dynamically allocate ID while
creating the node. Replaced the IDA (ID allocator) with
icc_node_create_alloc_id() API to allocate node IDs dynamically.
- Removed qcom,epss-l3-perf generic compatible as per the comment.
- Added L3 ICC handles for CPU0 and CPU4 in DT, as per Bjorn comment.
Link to comment:
https://lore.kernel.org/lkml/ww3t3tu7p36qzlhcetaxif2xzrpgslydmuqo3fqvisbuar4bjh@qc2u43dck3qi/
Change since v5:
- Reused qcom,sm8250-epss-l3 compatible for sa8775p SoC.
- Rearranged the patches, moved dt changes to end of series.
- Updated the commit text.
Changes since v4:
- Added generic compatible "qcom,epss-l3-perf" and split the driver
changes accordingly.
Changes since v3:
- Removed epss-l3-perf generic compatible changes. These will be posted
as separate patch until then SoC specific compatible will be used for
probing.
Changes since v2:
- Updated the commit text to reflect the reason for code change.
- Added SoC-specific and generic compatible to driver match table.
Changes since v1:
- Removed the usage of static IDs and implemented dynamic ID assignment
for icc nodes using IDA.
- Removed separate compatibles for cl0 and cl1. Both cl0 and cl1
devices use the same compatible.
- Added new generic compatible for epss-l3-perf.
Jagadeesh Kona (1):
arm64: dts: qcom: sa8775p: Add CPU OPP tables to scale DDR/L3
Raviteja Laggyshetty (4):
interconnect: core: Add dynamic id allocation support
interconnect: qcom: Add multidev EPSS L3 support
dt-bindings: interconnect: Add EPSS L3 compatible for SA8775P
arm64: dts: qcom: sa8775p: add EPSS l3 interconnect provider
.../bindings/interconnect/qcom,osm-l3.yaml | 1 +
arch/arm64/boot/dts/qcom/sa8775p.dtsi | 229 ++++++++++++++++++
drivers/interconnect/core.c | 32 +++
drivers/interconnect/qcom/osm-l3.c | 91 +++++--
include/linux/interconnect-provider.h | 6 +
5 files changed, 335 insertions(+), 24 deletions(-)
--
2.39.2
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH V7 1/5] interconnect: core: Add dynamic id allocation support
2025-01-11 16:14 [V7 0/5] Add EPSS L3 provider support on SA8775P SoC Raviteja Laggyshetty
@ 2025-01-11 16:14 ` Raviteja Laggyshetty
2025-01-11 20:57 ` Bjorn Andersson
` (2 more replies)
2025-01-11 16:14 ` [PATCH V7 2/5] interconnect: qcom: Add multidev EPSS L3 support Raviteja Laggyshetty
` (3 subsequent siblings)
4 siblings, 3 replies; 15+ messages in thread
From: Raviteja Laggyshetty @ 2025-01-11 16:14 UTC (permalink / raw)
To: Georgi Djakov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, Odelu Kukatla, Mike Tipton,
Vivek Aknurwar
Cc: Sibi Sankar, linux-arm-msm, linux-pm, devicetree, linux-kernel
Current interconnect framework is based on static IDs for creating node
and registering with framework. This becomes a limitation for topologies
where there are multiple instances of same interconnect provider. Add
icc_node_create_alloc_id() API to create icc node with dynamic id, this
will help to overcome the dependency on static IDs.
Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
---
drivers/interconnect/core.c | 32 +++++++++++++++++++++++++++
include/linux/interconnect-provider.h | 6 +++++
2 files changed, 38 insertions(+)
diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
index 9d5404a07e8a..0b7093eb51af 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -858,6 +858,38 @@ struct icc_node *icc_node_create(int id)
}
EXPORT_SYMBOL_GPL(icc_node_create);
+/**
+ * icc_node_create_alloc_id() - create node and dynamically allocate id
+ * @start_id: min id to be allocated
+ *
+ * Return: icc_node pointer on success, or ERR_PTR() on error
+ */
+struct icc_node *icc_node_create_alloc_id(int start_id)
+{
+ struct icc_node *node;
+ int id;
+
+ mutex_lock(&icc_lock);
+
+ node = kzalloc(sizeof(*node), GFP_KERNEL);
+ if (!node)
+ return ERR_PTR(-ENOMEM);
+
+ id = idr_alloc(&icc_idr, node, start_id, 0, GFP_KERNEL);
+ if (id < 0) {
+ WARN(1, "%s: couldn't get idr\n", __func__);
+ kfree(node);
+ node = ERR_PTR(id);
+ goto out;
+ }
+ node->id = id;
+out:
+ mutex_unlock(&icc_lock);
+
+ return node;
+}
+EXPORT_SYMBOL_GPL(icc_node_create_alloc_id);
+
/**
* icc_node_destroy() - destroy a node
* @id: node id
diff --git a/include/linux/interconnect-provider.h b/include/linux/interconnect-provider.h
index f5aef8784692..4fc7a5884374 100644
--- a/include/linux/interconnect-provider.h
+++ b/include/linux/interconnect-provider.h
@@ -117,6 +117,7 @@ struct icc_node {
int icc_std_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
u32 peak_bw, u32 *agg_avg, u32 *agg_peak);
struct icc_node *icc_node_create(int id);
+struct icc_node *icc_node_create_alloc_id(int start_id);
void icc_node_destroy(int id);
int icc_link_create(struct icc_node *node, const int dst_id);
void icc_node_add(struct icc_node *node, struct icc_provider *provider);
@@ -141,6 +142,11 @@ static inline struct icc_node *icc_node_create(int id)
return ERR_PTR(-ENOTSUPP);
}
+static inline struct icc_node *icc_node_create_alloc_id(int start_id)
+{
+ return ERR_PTR(-EOPNOTSUPP);
+}
+
static inline void icc_node_destroy(int id)
{
}
--
2.39.2
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH V7 2/5] interconnect: qcom: Add multidev EPSS L3 support
2025-01-11 16:14 [V7 0/5] Add EPSS L3 provider support on SA8775P SoC Raviteja Laggyshetty
2025-01-11 16:14 ` [PATCH V7 1/5] interconnect: core: Add dynamic id allocation support Raviteja Laggyshetty
@ 2025-01-11 16:14 ` Raviteja Laggyshetty
2025-01-11 21:08 ` Bjorn Andersson
2025-01-11 16:14 ` [PATCH V7 3/5] dt-bindings: interconnect: Add EPSS L3 compatible for SA8775P Raviteja Laggyshetty
` (2 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Raviteja Laggyshetty @ 2025-01-11 16:14 UTC (permalink / raw)
To: Georgi Djakov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, Odelu Kukatla, Mike Tipton,
Vivek Aknurwar
Cc: Sibi Sankar, linux-arm-msm, linux-pm, devicetree, linux-kernel
EPSS on SA8775P has two instances which requires creation of two device
nodes with different compatible and device data because of unique
icc node id and name limitation in interconnect framework.
Add multidevice support to osm-l3 code to get unique node id from icc
framework.
Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
---
drivers/interconnect/qcom/osm-l3.c | 91 ++++++++++++++++++++++--------
1 file changed, 67 insertions(+), 24 deletions(-)
diff --git a/drivers/interconnect/qcom/osm-l3.c b/drivers/interconnect/qcom/osm-l3.c
index 6a656ed44d49..8e98d1c9a840 100644
--- a/drivers/interconnect/qcom/osm-l3.c
+++ b/drivers/interconnect/qcom/osm-l3.c
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
/*
* Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
*/
#include <linux/args.h>
@@ -11,6 +12,7 @@
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/of.h>
+#include <linux/of_address.h>
#include <linux/platform_device.h>
#include <dt-bindings/interconnect/qcom,osm-l3.h>
@@ -34,6 +36,9 @@
#define OSM_L3_MAX_LINKS 1
+#define OSM_L3_NODE_ID_START 10000
+#define OSM_NODE_NAME_SUFFIX_SIZE 10
+
#define to_osm_l3_provider(_provider) \
container_of(_provider, struct qcom_osm_l3_icc_provider, provider)
@@ -55,46 +60,40 @@ struct qcom_osm_l3_icc_provider {
*/
struct qcom_osm_l3_node {
const char *name;
- u16 links[OSM_L3_MAX_LINKS];
+ const char *links[OSM_L3_MAX_LINKS];
u16 id;
u16 num_links;
u16 buswidth;
};
struct qcom_osm_l3_desc {
- const struct qcom_osm_l3_node * const *nodes;
+ struct qcom_osm_l3_node * const *nodes;
size_t num_nodes;
unsigned int lut_row_size;
unsigned int reg_freq_lut;
unsigned int reg_perf_state;
};
-enum {
- OSM_L3_MASTER_NODE = 10000,
- OSM_L3_SLAVE_NODE,
-};
-
-#define DEFINE_QNODE(_name, _id, _buswidth, ...) \
- static const struct qcom_osm_l3_node _name = { \
+#define DEFINE_QNODE(_name, _buswidth, ...) \
+ static struct qcom_osm_l3_node _name = { \
.name = #_name, \
- .id = _id, \
.buswidth = _buswidth, \
.num_links = COUNT_ARGS(__VA_ARGS__), \
- .links = { __VA_ARGS__ }, \
+ __VA_OPT__(.links = { #__VA_ARGS__ }) \
}
-DEFINE_QNODE(osm_l3_master, OSM_L3_MASTER_NODE, 16, OSM_L3_SLAVE_NODE);
-DEFINE_QNODE(osm_l3_slave, OSM_L3_SLAVE_NODE, 16);
+DEFINE_QNODE(osm_l3_master, 16, osm_l3_slave);
+DEFINE_QNODE(osm_l3_slave, 16);
-static const struct qcom_osm_l3_node * const osm_l3_nodes[] = {
+static struct qcom_osm_l3_node * const osm_l3_nodes[] = {
[MASTER_OSM_L3_APPS] = &osm_l3_master,
[SLAVE_OSM_L3] = &osm_l3_slave,
};
-DEFINE_QNODE(epss_l3_master, OSM_L3_MASTER_NODE, 32, OSM_L3_SLAVE_NODE);
-DEFINE_QNODE(epss_l3_slave, OSM_L3_SLAVE_NODE, 32);
+DEFINE_QNODE(epss_l3_master, 32, epss_l3_slave);
+DEFINE_QNODE(epss_l3_slave, 32);
-static const struct qcom_osm_l3_node * const epss_l3_nodes[] = {
+static struct qcom_osm_l3_node * const epss_l3_nodes[] = {
[MASTER_EPSS_L3_APPS] = &epss_l3_master,
[SLAVE_EPSS_L3_SHARED] = &epss_l3_slave,
};
@@ -123,6 +122,19 @@ static const struct qcom_osm_l3_desc epss_l3_l3_vote = {
.reg_perf_state = EPSS_REG_L3_VOTE,
};
+static u16 get_node_id_by_name(const char *node_name,
+ const struct qcom_osm_l3_desc *desc)
+{
+ struct qcom_osm_l3_node *const *nodes = desc->nodes;
+ int i;
+
+ for (i = 0; i < desc->num_nodes; i++) {
+ if (!strcmp(nodes[i]->name, node_name))
+ return nodes[i]->id;
+ }
+ return 0;
+}
+
static int qcom_osm_l3_set(struct icc_node *src, struct icc_node *dst)
{
struct qcom_osm_l3_icc_provider *qp;
@@ -164,10 +176,11 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
const struct qcom_osm_l3_desc *desc;
struct icc_onecell_data *data;
struct icc_provider *provider;
- const struct qcom_osm_l3_node * const *qnodes;
+ struct qcom_osm_l3_node * const *qnodes;
struct icc_node *node;
size_t num_nodes;
struct clk *clk;
+ u64 addr;
int ret;
clk = clk_get(&pdev->dev, "xo");
@@ -188,6 +201,10 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
if (!qp)
return -ENOMEM;
+ ret = of_property_read_reg(pdev->dev.of_node, 0, &addr, NULL);
+ if (ret)
+ return ret;
+
qp->base = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(qp->base))
return PTR_ERR(qp->base);
@@ -242,26 +259,51 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
icc_provider_init(provider);
+ /* create icc nodes */
for (i = 0; i < num_nodes; i++) {
- size_t j;
+ char *node_name;
+ size_t len;
- node = icc_node_create(qnodes[i]->id);
+ node = icc_node_create_alloc_id(OSM_L3_NODE_ID_START);
if (IS_ERR(node)) {
ret = PTR_ERR(node);
goto err;
}
+ qnodes[i]->id = node->id;
+
+ /* len = strlen(node->name) + @ + 8 (base-address) + NULL */
+ len = strlen(qnodes[i]->name) + OSM_NODE_NAME_SUFFIX_SIZE;
+ node_name = devm_kzalloc(&pdev->dev, len, GFP_KERNEL);
+ if (!node_name) {
+ ret = -ENOMEM;
+ goto err;
+ }
+
+ snprintf(node_name, len, "%s@%08llx", qnodes[i]->name, addr);
+ node->name = node_name;
- node->name = qnodes[i]->name;
/* Cast away const and add it back in qcom_osm_l3_set() */
node->data = (void *)qnodes[i];
icc_node_add(node, provider);
- for (j = 0; j < qnodes[i]->num_links; j++)
- icc_link_create(node, qnodes[i]->links[j]);
-
data->nodes[i] = node;
}
+ /* create links in topolgy */
+ for (i = 0; i < num_nodes; i++) {
+ size_t j;
+
+ node = data->nodes[i];
+ for (j = 0; j < qnodes[i]->num_links; j++) {
+ u16 link_node_id = get_node_id_by_name(qnodes[i]->links[j], desc);
+
+ if (link_node_id)
+ icc_link_create(node, link_node_id);
+ else
+ goto err;
+ }
+ }
+
ret = icc_provider_register(provider);
if (ret)
goto err;
@@ -284,6 +326,7 @@ static const struct of_device_id osm_l3_of_match[] = {
{ .compatible = "qcom,sm8150-osm-l3", .data = &osm_l3 },
{ .compatible = "qcom,sc8180x-osm-l3", .data = &osm_l3 },
{ .compatible = "qcom,sm8250-epss-l3", .data = &epss_l3_perf_state },
+ { .compatible = "qcom,sa8775p-epss-l3", .data = &epss_l3_perf_state },
{ }
};
MODULE_DEVICE_TABLE(of, osm_l3_of_match);
--
2.39.2
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH V7 3/5] dt-bindings: interconnect: Add EPSS L3 compatible for SA8775P
2025-01-11 16:14 [V7 0/5] Add EPSS L3 provider support on SA8775P SoC Raviteja Laggyshetty
2025-01-11 16:14 ` [PATCH V7 1/5] interconnect: core: Add dynamic id allocation support Raviteja Laggyshetty
2025-01-11 16:14 ` [PATCH V7 2/5] interconnect: qcom: Add multidev EPSS L3 support Raviteja Laggyshetty
@ 2025-01-11 16:14 ` Raviteja Laggyshetty
2025-01-12 9:31 ` Krzysztof Kozlowski
2025-01-11 16:14 ` [PATCH V7 4/5] arm64: dts: qcom: sa8775p: add EPSS l3 interconnect provider Raviteja Laggyshetty
2025-01-11 16:14 ` [PATCH V7 5/5] arm64: dts: qcom: sa8775p: Add CPU OPP tables to scale DDR/L3 Raviteja Laggyshetty
4 siblings, 1 reply; 15+ messages in thread
From: Raviteja Laggyshetty @ 2025-01-11 16:14 UTC (permalink / raw)
To: Georgi Djakov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, Odelu Kukatla, Mike Tipton,
Vivek Aknurwar
Cc: Sibi Sankar, linux-arm-msm, linux-pm, devicetree, linux-kernel
Add Epoch Subsystem (EPSS) L3 interconnect provider binding on
SA8775P SoCs.
Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
---
Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
index 21dae0b92819..94f7f283787a 100644
--- a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
+++ b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
@@ -33,6 +33,7 @@ properties:
- qcom,sm6375-cpucp-l3
- qcom,sm8250-epss-l3
- qcom,sm8350-epss-l3
+ - qcom,sa8775p-epss-l3
- const: qcom,epss-l3
reg:
--
2.39.2
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH V7 4/5] arm64: dts: qcom: sa8775p: add EPSS l3 interconnect provider
2025-01-11 16:14 [V7 0/5] Add EPSS L3 provider support on SA8775P SoC Raviteja Laggyshetty
` (2 preceding siblings ...)
2025-01-11 16:14 ` [PATCH V7 3/5] dt-bindings: interconnect: Add EPSS L3 compatible for SA8775P Raviteja Laggyshetty
@ 2025-01-11 16:14 ` Raviteja Laggyshetty
2025-01-11 16:14 ` [PATCH V7 5/5] arm64: dts: qcom: sa8775p: Add CPU OPP tables to scale DDR/L3 Raviteja Laggyshetty
4 siblings, 0 replies; 15+ messages in thread
From: Raviteja Laggyshetty @ 2025-01-11 16:14 UTC (permalink / raw)
To: Georgi Djakov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, Odelu Kukatla, Mike Tipton,
Vivek Aknurwar
Cc: Sibi Sankar, linux-arm-msm, linux-pm, devicetree, linux-kernel
Add Epoch Subsystem (EPSS) L3 interconnect provider node on SA8775P
SoCs.
Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
---
arch/arm64/boot/dts/qcom/sa8775p.dtsi | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sa8775p.dtsi b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
index 368bcf7c9802..c6a889d4ddaf 100644
--- a/arch/arm64/boot/dts/qcom/sa8775p.dtsi
+++ b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
@@ -10,6 +10,7 @@
#include <dt-bindings/clock/qcom,sa8775p-gcc.h>
#include <dt-bindings/clock/qcom,sa8775p-gpucc.h>
#include <dt-bindings/dma/qcom-gpi.h>
+#include <dt-bindings/interconnect/qcom,osm-l3.h>
#include <dt-bindings/interconnect/qcom,sa8775p-rpmh.h>
#include <dt-bindings/mailbox/qcom-ipcc.h>
#include <dt-bindings/firmware/qcom,scm.h>
@@ -4282,6 +4283,15 @@ rpmhpd_opp_turbo_l1: opp-9 {
};
};
+ epss_l3_cl0: interconnect@18590000 {
+ compatible = "qcom,sa8775p-epss-l3",
+ "qcom,epss-l3";
+ reg = <0x0 0x18590000 0x0 0x1000>;
+ clocks = <&rpmhcc RPMH_CXO_CLK>, <&gcc GCC_GPLL0>;
+ clock-names = "xo", "alternate";
+ #interconnect-cells = <1>;
+ };
+
cpufreq_hw: cpufreq@18591000 {
compatible = "qcom,sa8775p-cpufreq-epss",
"qcom,cpufreq-epss";
@@ -4295,6 +4305,15 @@ cpufreq_hw: cpufreq@18591000 {
#freq-domain-cells = <1>;
};
+ epss_l3_cl1: interconnect@18592000 {
+ compatible = "qcom,sa8775p-epss-l3",
+ "qcom,epss-l3";
+ reg = <0x0 0x18592000 0x0 0x1000>;
+ clocks = <&rpmhcc RPMH_CXO_CLK>, <&gcc GCC_GPLL0>;
+ clock-names = "xo", "alternate";
+ #interconnect-cells = <1>;
+ };
+
remoteproc_gpdsp0: remoteproc@20c00000 {
compatible = "qcom,sa8775p-gpdsp0-pas";
reg = <0x0 0x20c00000 0x0 0x10000>;
--
2.39.2
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH V7 5/5] arm64: dts: qcom: sa8775p: Add CPU OPP tables to scale DDR/L3
2025-01-11 16:14 [V7 0/5] Add EPSS L3 provider support on SA8775P SoC Raviteja Laggyshetty
` (3 preceding siblings ...)
2025-01-11 16:14 ` [PATCH V7 4/5] arm64: dts: qcom: sa8775p: add EPSS l3 interconnect provider Raviteja Laggyshetty
@ 2025-01-11 16:14 ` Raviteja Laggyshetty
4 siblings, 0 replies; 15+ messages in thread
From: Raviteja Laggyshetty @ 2025-01-11 16:14 UTC (permalink / raw)
To: Georgi Djakov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, Odelu Kukatla, Mike Tipton,
Vivek Aknurwar
Cc: Sibi Sankar, linux-arm-msm, linux-pm, devicetree, linux-kernel,
Shivnandan Kumar
From: Jagadeesh Kona <quic_jkona@quicinc.com>
Add OPP tables required to scale DDR and L3 per freq-domain
on SA8775P platform.
If a single OPP table is used for both CPU domains, then
_allocate_opp_table() won't be invoked for CPU4 but instead
CPU4 will be added as device under the CPU0 OPP table. Due
to this, dev_pm_opp_of_find_icc_paths() won't be invoked for
CPU4 device and hence CPU4 won't be able to independently scale
it's interconnects. Both CPU0 and CPU4 devices will scale the
same ICC path which can lead to one device overwriting the BW
vote placed by other device. Hence CPU0 and CPU4 require separate
OPP tables to allow independent scaling of DDR and L3 frequencies
for each CPU domain, with the final DDR and L3 frequencies being
an aggregate of both.
Co-developed-by: Shivnandan Kumar <quic_kshivnan@quicinc.com>
Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com>
Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
---
arch/arm64/boot/dts/qcom/sa8775p.dtsi | 210 ++++++++++++++++++++++++++
1 file changed, 210 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sa8775p.dtsi b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
index c6a889d4ddaf..7c52bca958fe 100644
--- a/arch/arm64/boot/dts/qcom/sa8775p.dtsi
+++ b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
@@ -49,6 +49,11 @@ cpu0: cpu@0 {
next-level-cache = <&l2_0>;
capacity-dmips-mhz = <1024>;
dynamic-power-coefficient = <100>;
+ operating-points-v2 = <&cpu0_opp_table>;
+ interconnects = <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
+ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ACTIVE_ONLY>,
+ <&epss_l3_cl0 MASTER_EPSS_L3_APPS
+ &epss_l3_cl0 SLAVE_EPSS_L3_SHARED>;
l2_0: l2-cache {
compatible = "cache";
cache-level = <2>;
@@ -71,6 +76,11 @@ cpu1: cpu@100 {
next-level-cache = <&l2_1>;
capacity-dmips-mhz = <1024>;
dynamic-power-coefficient = <100>;
+ operating-points-v2 = <&cpu0_opp_table>;
+ interconnects = <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
+ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ACTIVE_ONLY>,
+ <&epss_l3_cl0 MASTER_EPSS_L3_APPS
+ &epss_l3_cl0 SLAVE_EPSS_L3_SHARED>;
l2_1: l2-cache {
compatible = "cache";
cache-level = <2>;
@@ -88,6 +98,11 @@ cpu2: cpu@200 {
next-level-cache = <&l2_2>;
capacity-dmips-mhz = <1024>;
dynamic-power-coefficient = <100>;
+ operating-points-v2 = <&cpu0_opp_table>;
+ interconnects = <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
+ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ACTIVE_ONLY>,
+ <&epss_l3_cl0 MASTER_EPSS_L3_APPS
+ &epss_l3_cl0 SLAVE_EPSS_L3_SHARED>;
l2_2: l2-cache {
compatible = "cache";
cache-level = <2>;
@@ -105,6 +120,11 @@ cpu3: cpu@300 {
next-level-cache = <&l2_3>;
capacity-dmips-mhz = <1024>;
dynamic-power-coefficient = <100>;
+ operating-points-v2 = <&cpu0_opp_table>;
+ interconnects = <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
+ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ACTIVE_ONLY>,
+ <&epss_l3_cl0 MASTER_EPSS_L3_APPS
+ &epss_l3_cl0 SLAVE_EPSS_L3_SHARED>;
l2_3: l2-cache {
compatible = "cache";
cache-level = <2>;
@@ -122,6 +142,11 @@ cpu4: cpu@10000 {
next-level-cache = <&l2_4>;
capacity-dmips-mhz = <1024>;
dynamic-power-coefficient = <100>;
+ operating-points-v2 = <&cpu4_opp_table>;
+ interconnects = <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
+ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ACTIVE_ONLY>,
+ <&epss_l3_cl1 MASTER_EPSS_L3_APPS
+ &epss_l3_cl1 SLAVE_EPSS_L3_SHARED>;
l2_4: l2-cache {
compatible = "cache";
cache-level = <2>;
@@ -145,6 +170,11 @@ cpu5: cpu@10100 {
next-level-cache = <&l2_5>;
capacity-dmips-mhz = <1024>;
dynamic-power-coefficient = <100>;
+ operating-points-v2 = <&cpu4_opp_table>;
+ interconnects = <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
+ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ACTIVE_ONLY>,
+ <&epss_l3_cl1 MASTER_EPSS_L3_APPS
+ &epss_l3_cl1 SLAVE_EPSS_L3_SHARED>;
l2_5: l2-cache {
compatible = "cache";
cache-level = <2>;
@@ -162,6 +192,11 @@ cpu6: cpu@10200 {
next-level-cache = <&l2_6>;
capacity-dmips-mhz = <1024>;
dynamic-power-coefficient = <100>;
+ operating-points-v2 = <&cpu4_opp_table>;
+ interconnects = <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
+ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ACTIVE_ONLY>,
+ <&epss_l3_cl1 MASTER_EPSS_L3_APPS
+ &epss_l3_cl1 SLAVE_EPSS_L3_SHARED>;
l2_6: l2-cache {
compatible = "cache";
cache-level = <2>;
@@ -179,6 +214,11 @@ cpu7: cpu@10300 {
next-level-cache = <&l2_7>;
capacity-dmips-mhz = <1024>;
dynamic-power-coefficient = <100>;
+ operating-points-v2 = <&cpu4_opp_table>;
+ interconnects = <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
+ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ACTIVE_ONLY>,
+ <&epss_l3_cl1 MASTER_EPSS_L3_APPS
+ &epss_l3_cl1 SLAVE_EPSS_L3_SHARED>;
l2_7: l2-cache {
compatible = "cache";
cache-level = <2>;
@@ -268,6 +308,176 @@ cluster_sleep_apss_rsc_pc: cluster-sleep-1 {
};
};
+ cpu0_opp_table: opp-table-cpu0 {
+ compatible = "operating-points-v2";
+ opp-shared;
+
+ cpu0_opp_1267mhz: opp-1267200000 {
+ opp-hz = /bits/ 64 <1267200000>;
+ opp-peak-kBps = <6220800 29491200>;
+ };
+
+ cpu0_opp_1363mhz: opp-1363200000 {
+ opp-hz = /bits/ 64 <1363200000>;
+ opp-peak-kBps = <6220800 29491200>;
+ };
+
+ cpu0_opp_1459mhz: opp-1459200000 {
+ opp-hz = /bits/ 64 <1459200000>;
+ opp-peak-kBps = <6220800 29491200>;
+ };
+
+ cpu0_opp_1536mhz: opp-1536000000 {
+ opp-hz = /bits/ 64 <1536000000>;
+ opp-peak-kBps = <6220800 29491200>;
+ };
+
+ cpu0_opp_1632mhz: opp-1632000000 {
+ opp-hz = /bits/ 64 <1632000000>;
+ opp-peak-kBps = <6835200 39321600>;
+ };
+
+ cpu0_opp_1708mhz: opp-1708800000 {
+ opp-hz = /bits/ 64 <1708800000>;
+ opp-peak-kBps = <6835200 39321600>;
+ };
+
+ cpu0_opp_1785mhz: opp-1785600000 {
+ opp-hz = /bits/ 64 <1785600000>;
+ opp-peak-kBps = <6835200 39321600>;
+ };
+
+ cpu0_opp_1862mhz: opp-1862400000 {
+ opp-hz = /bits/ 64 <1862400000>;
+ opp-peak-kBps = <6835200 39321600>;
+ };
+
+ cpu0_opp_1939mhz: opp-1939200000 {
+ opp-hz = /bits/ 64 <1939200000>;
+ opp-peak-kBps = <6835200 39321600>;
+ };
+
+ cpu0_opp_2016mhz: opp-2016000000 {
+ opp-hz = /bits/ 64 <2016000000>;
+ opp-peak-kBps = <6835200 39321600>;
+ };
+
+ cpu0_opp_2112mhz: opp-2112000000 {
+ opp-hz = /bits/ 64 <2112000000>;
+ opp-peak-kBps = <8371200 49766400>;
+ };
+
+ cpu0_opp_2188mhz: opp-2188800000 {
+ opp-hz = /bits/ 64 <2188800000>;
+ opp-peak-kBps = <8371200 49766400>;
+ };
+
+ cpu0_opp_2265mhz: opp-2265600000 {
+ opp-hz = /bits/ 64 <2265600000>;
+ opp-peak-kBps = <8371200 49766400>;
+ };
+
+ cpu0_opp_2361mhz: opp-2361600000 {
+ opp-hz = /bits/ 64 <2361600000>;
+ opp-peak-kBps = <12787200 51609600>;
+ };
+
+ cpu0_opp_2457mhz: opp-2457600000 {
+ opp-hz = /bits/ 64 <2457600000>;
+ opp-peak-kBps = <12787200 51609600>;
+ };
+
+ cpu0_opp_2553mhz: opp-2553600000 {
+ opp-hz = /bits/ 64 <2553600000>;
+ opp-peak-kBps = <12787200 54681600>;
+ };
+ };
+
+ cpu4_opp_table: opp-table-cpu4 {
+ compatible = "operating-points-v2";
+ opp-shared;
+
+ cpu4_opp_1267mhz: opp-1267200000 {
+ opp-hz = /bits/ 64 <1267200000>;
+ opp-peak-kBps = <6220800 29491200>;
+ };
+
+ cpu4_opp_1363mhz: opp-1363200000 {
+ opp-hz = /bits/ 64 <1363200000>;
+ opp-peak-kBps = <6220800 29491200>;
+ };
+
+ cpu4_opp_1459mhz: opp-1459200000 {
+ opp-hz = /bits/ 64 <1459200000>;
+ opp-peak-kBps = <6220800 29491200>;
+ };
+
+ cpu4_opp_1536mhz: opp-1536000000 {
+ opp-hz = /bits/ 64 <1536000000>;
+ opp-peak-kBps = <6220800 29491200>;
+ };
+
+ cpu4_opp_1632mhz: opp-1632000000 {
+ opp-hz = /bits/ 64 <1632000000>;
+ opp-peak-kBps = <6835200 39321600>;
+ };
+
+ cpu4_opp_1708mhz: opp-1708800000 {
+ opp-hz = /bits/ 64 <1708800000>;
+ opp-peak-kBps = <6835200 39321600>;
+ };
+
+ cpu4_opp_1785mhz: opp-1785600000 {
+ opp-hz = /bits/ 64 <1785600000>;
+ opp-peak-kBps = <6835200 39321600>;
+ };
+
+ cpu4_opp_1862mhz: opp-1862400000 {
+ opp-hz = /bits/ 64 <1862400000>;
+ opp-peak-kBps = <6835200 39321600>;
+ };
+
+ cpu4_opp_1939mhz: opp-1939200000 {
+ opp-hz = /bits/ 64 <1939200000>;
+ opp-peak-kBps = <6835200 39321600>;
+ };
+
+ cpu4_opp_2016mhz: opp-2016000000 {
+ opp-hz = /bits/ 64 <2016000000>;
+ opp-peak-kBps = <6835200 39321600>;
+ };
+
+ cpu4_opp_2112mhz: opp-2112000000 {
+ opp-hz = /bits/ 64 <2112000000>;
+ opp-peak-kBps = <8371200 49766400>;
+ };
+
+ cpu4_opp_2188mhz: opp-2188800000 {
+ opp-hz = /bits/ 64 <2188800000>;
+ opp-peak-kBps = <8371200 49766400>;
+ };
+
+ cpu4_opp_2265mhz: opp-2265600000 {
+ opp-hz = /bits/ 64 <2265600000>;
+ opp-peak-kBps = <8371200 49766400>;
+ };
+
+ cpu4_opp_2361mhz: opp-2361600000 {
+ opp-hz = /bits/ 64 <2361600000>;
+ opp-peak-kBps = <12787200 51609600>;
+ };
+
+ cpu4_opp_2457mhz: opp-2457600000 {
+ opp-hz = /bits/ 64 <2457600000>;
+ opp-peak-kBps = <12787200 51609600>;
+ };
+
+ cpu4_opp_2553mhz: opp-2553600000 {
+ opp-hz = /bits/ 64 <2553600000>;
+ opp-peak-kBps = <12787200 54681600>;
+ };
+ };
+
dummy-sink {
compatible = "arm,coresight-dummy-sink";
--
2.39.2
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V7 1/5] interconnect: core: Add dynamic id allocation support
2025-01-11 16:14 ` [PATCH V7 1/5] interconnect: core: Add dynamic id allocation support Raviteja Laggyshetty
@ 2025-01-11 20:57 ` Bjorn Andersson
2025-01-23 10:35 ` Raviteja Laggyshetty
2025-01-13 8:14 ` Dmitry Baryshkov
2025-01-20 8:15 ` Dan Carpenter
2 siblings, 1 reply; 15+ messages in thread
From: Bjorn Andersson @ 2025-01-11 20:57 UTC (permalink / raw)
To: Raviteja Laggyshetty
Cc: Georgi Djakov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Konrad Dybcio, Odelu Kukatla, Mike Tipton, Vivek Aknurwar,
Sibi Sankar, linux-arm-msm, linux-pm, devicetree, linux-kernel
On Sat, Jan 11, 2025 at 04:14:25PM +0000, Raviteja Laggyshetty wrote:
> Current interconnect framework is based on static IDs for creating node
> and registering with framework. This becomes a limitation for topologies
> where there are multiple instances of same interconnect provider. Add
> icc_node_create_alloc_id() API to create icc node with dynamic id, this
> will help to overcome the dependency on static IDs.
>
> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
> ---
> drivers/interconnect/core.c | 32 +++++++++++++++++++++++++++
> include/linux/interconnect-provider.h | 6 +++++
> 2 files changed, 38 insertions(+)
>
> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
> index 9d5404a07e8a..0b7093eb51af 100644
> --- a/drivers/interconnect/core.c
> +++ b/drivers/interconnect/core.c
> @@ -858,6 +858,38 @@ struct icc_node *icc_node_create(int id)
> }
> EXPORT_SYMBOL_GPL(icc_node_create);
>
> +/**
> + * icc_node_create_alloc_id() - create node and dynamically allocate id
> + * @start_id: min id to be allocated
> + *
> + * Return: icc_node pointer on success, or ERR_PTR() on error
> + */
> +struct icc_node *icc_node_create_alloc_id(int start_id)
By having clients pass in start_id, you distribute the decision of what
a "good number" is across multiple parts of the system (or you have
clients relying on getting [start_id, start_id + N) back).
Wouldn't it be better to hide that choice in one place (inside the icc
framework)?
Regards,
Bjorn
> +{
> + struct icc_node *node;
> + int id;
> +
> + mutex_lock(&icc_lock);
> +
> + node = kzalloc(sizeof(*node), GFP_KERNEL);
> + if (!node)
> + return ERR_PTR(-ENOMEM);
> +
> + id = idr_alloc(&icc_idr, node, start_id, 0, GFP_KERNEL);
> + if (id < 0) {
> + WARN(1, "%s: couldn't get idr\n", __func__);
> + kfree(node);
> + node = ERR_PTR(id);
> + goto out;
> + }
> + node->id = id;
> +out:
> + mutex_unlock(&icc_lock);
> +
> + return node;
> +}
> +EXPORT_SYMBOL_GPL(icc_node_create_alloc_id);
> +
> /**
> * icc_node_destroy() - destroy a node
> * @id: node id
> diff --git a/include/linux/interconnect-provider.h b/include/linux/interconnect-provider.h
> index f5aef8784692..4fc7a5884374 100644
> --- a/include/linux/interconnect-provider.h
> +++ b/include/linux/interconnect-provider.h
> @@ -117,6 +117,7 @@ struct icc_node {
> int icc_std_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
> u32 peak_bw, u32 *agg_avg, u32 *agg_peak);
> struct icc_node *icc_node_create(int id);
> +struct icc_node *icc_node_create_alloc_id(int start_id);
> void icc_node_destroy(int id);
> int icc_link_create(struct icc_node *node, const int dst_id);
> void icc_node_add(struct icc_node *node, struct icc_provider *provider);
> @@ -141,6 +142,11 @@ static inline struct icc_node *icc_node_create(int id)
> return ERR_PTR(-ENOTSUPP);
> }
>
> +static inline struct icc_node *icc_node_create_alloc_id(int start_id)
> +{
> + return ERR_PTR(-EOPNOTSUPP);
> +}
> +
> static inline void icc_node_destroy(int id)
> {
> }
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V7 2/5] interconnect: qcom: Add multidev EPSS L3 support
2025-01-11 16:14 ` [PATCH V7 2/5] interconnect: qcom: Add multidev EPSS L3 support Raviteja Laggyshetty
@ 2025-01-11 21:08 ` Bjorn Andersson
2025-01-23 10:40 ` Raviteja Laggyshetty
0 siblings, 1 reply; 15+ messages in thread
From: Bjorn Andersson @ 2025-01-11 21:08 UTC (permalink / raw)
To: Raviteja Laggyshetty
Cc: Georgi Djakov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Konrad Dybcio, Odelu Kukatla, Mike Tipton, Vivek Aknurwar,
Sibi Sankar, linux-arm-msm, linux-pm, devicetree, linux-kernel
On Sat, Jan 11, 2025 at 04:14:26PM +0000, Raviteja Laggyshetty wrote:
> EPSS on SA8775P has two instances which requires creation of two device
> nodes with different compatible and device data because of unique
> icc node id and name limitation in interconnect framework.
> Add multidevice support to osm-l3 code to get unique node id from icc
> framework.
>
> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
> ---
> drivers/interconnect/qcom/osm-l3.c | 91 ++++++++++++++++++++++--------
> 1 file changed, 67 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/interconnect/qcom/osm-l3.c b/drivers/interconnect/qcom/osm-l3.c
> index 6a656ed44d49..8e98d1c9a840 100644
> --- a/drivers/interconnect/qcom/osm-l3.c
> +++ b/drivers/interconnect/qcom/osm-l3.c
> @@ -1,6 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0
> /*
> * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
> */
>
> #include <linux/args.h>
> @@ -11,6 +12,7 @@
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/of.h>
> +#include <linux/of_address.h>
> #include <linux/platform_device.h>
>
> #include <dt-bindings/interconnect/qcom,osm-l3.h>
> @@ -34,6 +36,9 @@
>
> #define OSM_L3_MAX_LINKS 1
>
> +#define OSM_L3_NODE_ID_START 10000
> +#define OSM_NODE_NAME_SUFFIX_SIZE 10
> +
> #define to_osm_l3_provider(_provider) \
> container_of(_provider, struct qcom_osm_l3_icc_provider, provider)
>
> @@ -55,46 +60,40 @@ struct qcom_osm_l3_icc_provider {
> */
> struct qcom_osm_l3_node {
> const char *name;
> - u16 links[OSM_L3_MAX_LINKS];
> + const char *links[OSM_L3_MAX_LINKS];
> u16 id;
> u16 num_links;
> u16 buswidth;
> };
>
> struct qcom_osm_l3_desc {
> - const struct qcom_osm_l3_node * const *nodes;
> + struct qcom_osm_l3_node * const *nodes;
> size_t num_nodes;
> unsigned int lut_row_size;
> unsigned int reg_freq_lut;
> unsigned int reg_perf_state;
> };
>
> -enum {
> - OSM_L3_MASTER_NODE = 10000,
> - OSM_L3_SLAVE_NODE,
> -};
> -
> -#define DEFINE_QNODE(_name, _id, _buswidth, ...) \
> - static const struct qcom_osm_l3_node _name = { \
> +#define DEFINE_QNODE(_name, _buswidth, ...) \
> + static struct qcom_osm_l3_node _name = { \
> .name = #_name, \
> - .id = _id, \
> .buswidth = _buswidth, \
> .num_links = COUNT_ARGS(__VA_ARGS__), \
> - .links = { __VA_ARGS__ }, \
> + __VA_OPT__(.links = { #__VA_ARGS__ }) \
> }
>
> -DEFINE_QNODE(osm_l3_master, OSM_L3_MASTER_NODE, 16, OSM_L3_SLAVE_NODE);
> -DEFINE_QNODE(osm_l3_slave, OSM_L3_SLAVE_NODE, 16);
> +DEFINE_QNODE(osm_l3_master, 16, osm_l3_slave);
> +DEFINE_QNODE(osm_l3_slave, 16);
>
> -static const struct qcom_osm_l3_node * const osm_l3_nodes[] = {
> +static struct qcom_osm_l3_node * const osm_l3_nodes[] = {
> [MASTER_OSM_L3_APPS] = &osm_l3_master,
> [SLAVE_OSM_L3] = &osm_l3_slave,
> };
>
> -DEFINE_QNODE(epss_l3_master, OSM_L3_MASTER_NODE, 32, OSM_L3_SLAVE_NODE);
> -DEFINE_QNODE(epss_l3_slave, OSM_L3_SLAVE_NODE, 32);
> +DEFINE_QNODE(epss_l3_master, 32, epss_l3_slave);
> +DEFINE_QNODE(epss_l3_slave, 32);
>
> -static const struct qcom_osm_l3_node * const epss_l3_nodes[] = {
> +static struct qcom_osm_l3_node * const epss_l3_nodes[] = {
> [MASTER_EPSS_L3_APPS] = &epss_l3_master,
> [SLAVE_EPSS_L3_SHARED] = &epss_l3_slave,
> };
> @@ -123,6 +122,19 @@ static const struct qcom_osm_l3_desc epss_l3_l3_vote = {
> .reg_perf_state = EPSS_REG_L3_VOTE,
> };
>
> +static u16 get_node_id_by_name(const char *node_name,
> + const struct qcom_osm_l3_desc *desc)
> +{
> + struct qcom_osm_l3_node *const *nodes = desc->nodes;
> + int i;
> +
> + for (i = 0; i < desc->num_nodes; i++) {
> + if (!strcmp(nodes[i]->name, node_name))
> + return nodes[i]->id;
> + }
> + return 0;
> +}
> +
> static int qcom_osm_l3_set(struct icc_node *src, struct icc_node *dst)
> {
> struct qcom_osm_l3_icc_provider *qp;
> @@ -164,10 +176,11 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
> const struct qcom_osm_l3_desc *desc;
> struct icc_onecell_data *data;
> struct icc_provider *provider;
> - const struct qcom_osm_l3_node * const *qnodes;
> + struct qcom_osm_l3_node * const *qnodes;
> struct icc_node *node;
> size_t num_nodes;
> struct clk *clk;
> + u64 addr;
> int ret;
>
> clk = clk_get(&pdev->dev, "xo");
> @@ -188,6 +201,10 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
> if (!qp)
> return -ENOMEM;
>
> + ret = of_property_read_reg(pdev->dev.of_node, 0, &addr, NULL);
> + if (ret)
> + return ret;
> +
> qp->base = devm_platform_ioremap_resource(pdev, 0);
> if (IS_ERR(qp->base))
> return PTR_ERR(qp->base);
> @@ -242,26 +259,51 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
>
> icc_provider_init(provider);
>
> + /* create icc nodes */
> for (i = 0; i < num_nodes; i++) {
> - size_t j;
> + char *node_name;
> + size_t len;
>
> - node = icc_node_create(qnodes[i]->id);
> + node = icc_node_create_alloc_id(OSM_L3_NODE_ID_START);
> if (IS_ERR(node)) {
> ret = PTR_ERR(node);
> goto err;
> }
> + qnodes[i]->id = node->id;
> +
> + /* len = strlen(node->name) + @ + 8 (base-address) + NULL */
> + len = strlen(qnodes[i]->name) + OSM_NODE_NAME_SUFFIX_SIZE;
> + node_name = devm_kzalloc(&pdev->dev, len, GFP_KERNEL);
> + if (!node_name) {
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> + snprintf(node_name, len, "%s@%08llx", qnodes[i]->name, addr);
> + node->name = node_name;
I don't think it's reasonable to duplicate this logic and the decision
of naming convention in each provider driver. Please provide a generic
solution in the framework.
PS. Not that I want you to use it here, but for the next time be aware
of devm_kasprintf().
>
> - node->name = qnodes[i]->name;
> /* Cast away const and add it back in qcom_osm_l3_set() */
> node->data = (void *)qnodes[i];
> icc_node_add(node, provider);
>
> - for (j = 0; j < qnodes[i]->num_links; j++)
> - icc_link_create(node, qnodes[i]->links[j]);
> -
> data->nodes[i] = node;
> }
>
> + /* create links in topolgy */
> + for (i = 0; i < num_nodes; i++) {
> + size_t j;
> +
> + node = data->nodes[i];
> + for (j = 0; j < qnodes[i]->num_links; j++) {
> + u16 link_node_id = get_node_id_by_name(qnodes[i]->links[j], desc);
Isn't that O(i^2*j) string comparisons? I don't find that acceptable.
> +
> + if (link_node_id)
> + icc_link_create(node, link_node_id);
> + else
> + goto err;
> + }
> + }
> +
> ret = icc_provider_register(provider);
> if (ret)
> goto err;
> @@ -284,6 +326,7 @@ static const struct of_device_id osm_l3_of_match[] = {
> { .compatible = "qcom,sm8150-osm-l3", .data = &osm_l3 },
> { .compatible = "qcom,sc8180x-osm-l3", .data = &osm_l3 },
> { .compatible = "qcom,sm8250-epss-l3", .data = &epss_l3_perf_state },
> + { .compatible = "qcom,sa8775p-epss-l3", .data = &epss_l3_perf_state },
With the exception of sc8180x, this list is sorted alphabetically.
Please insert your entry where it makes sense.
Regards,
Bjorn
> { }
> };
> MODULE_DEVICE_TABLE(of, osm_l3_of_match);
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V7 3/5] dt-bindings: interconnect: Add EPSS L3 compatible for SA8775P
2025-01-11 16:14 ` [PATCH V7 3/5] dt-bindings: interconnect: Add EPSS L3 compatible for SA8775P Raviteja Laggyshetty
@ 2025-01-12 9:31 ` Krzysztof Kozlowski
2025-01-23 12:07 ` Raviteja Laggyshetty
0 siblings, 1 reply; 15+ messages in thread
From: Krzysztof Kozlowski @ 2025-01-12 9:31 UTC (permalink / raw)
To: Raviteja Laggyshetty
Cc: Georgi Djakov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, Odelu Kukatla, Mike Tipton,
Vivek Aknurwar, Sibi Sankar, linux-arm-msm, linux-pm, devicetree,
linux-kernel
On Sat, Jan 11, 2025 at 04:14:27PM +0000, Raviteja Laggyshetty wrote:
> Add Epoch Subsystem (EPSS) L3 interconnect provider binding on
> SA8775P SoCs.
1. And why is this not compatible with sm8250? There was lengthy
discussion and no outcome of it managed to get to commit msg. Really, so
we are going to repeat everything again and you will not get any acks.
You have entire commit msg to explain things but instead you repeat what
the patch does. We can read the diff for that.
2. Binding *ALWAYS* comes before the user.
>
> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
> ---
> Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
> index 21dae0b92819..94f7f283787a 100644
> --- a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
> +++ b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
> @@ -33,6 +33,7 @@ properties:
> - qcom,sm6375-cpucp-l3
> - qcom,sm8250-epss-l3
> - qcom,sm8350-epss-l3
> + - qcom,sa8775p-epss-l3
> - const: qcom,epss-l3
Your driver suggests this is not really true - it is not compatible with
qcom,epss-l3. Maybe it is, maybe not, no clue, commit explains nothing.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V7 1/5] interconnect: core: Add dynamic id allocation support
2025-01-11 16:14 ` [PATCH V7 1/5] interconnect: core: Add dynamic id allocation support Raviteja Laggyshetty
2025-01-11 20:57 ` Bjorn Andersson
@ 2025-01-13 8:14 ` Dmitry Baryshkov
2025-01-23 10:47 ` Raviteja Laggyshetty
2025-01-20 8:15 ` Dan Carpenter
2 siblings, 1 reply; 15+ messages in thread
From: Dmitry Baryshkov @ 2025-01-13 8:14 UTC (permalink / raw)
To: Raviteja Laggyshetty
Cc: Georgi Djakov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, Odelu Kukatla, Mike Tipton,
Vivek Aknurwar, Sibi Sankar, linux-arm-msm, linux-pm, devicetree,
linux-kernel
On Sat, Jan 11, 2025 at 04:14:25PM +0000, Raviteja Laggyshetty wrote:
> Current interconnect framework is based on static IDs for creating node
> and registering with framework. This becomes a limitation for topologies
> where there are multiple instances of same interconnect provider. Add
> icc_node_create_alloc_id() API to create icc node with dynamic id, this
> will help to overcome the dependency on static IDs.
This doesn't overcome the dependency on static ID. Drivers still have to
manually lookup the resulting ID and use it to link the nodes. Instead
ICC framework should be providing a completely dynamic solution:
- icc_node_create() should get a completely dynamic counterpart. Use
e.g. 1000000 as a dynamic start ID.
- icc_link_create() shold get a counterpart which can create a link
between two icc_node instances directly, without an additional lookup.
You can check if your implementation is correct if you can refactor
existing ICC drivers (e.g. icc-clk and/or icc-rpm to drop ID arrays
completely).
>
> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
> ---
> drivers/interconnect/core.c | 32 +++++++++++++++++++++++++++
> include/linux/interconnect-provider.h | 6 +++++
> 2 files changed, 38 insertions(+)
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V7 1/5] interconnect: core: Add dynamic id allocation support
2025-01-11 16:14 ` [PATCH V7 1/5] interconnect: core: Add dynamic id allocation support Raviteja Laggyshetty
2025-01-11 20:57 ` Bjorn Andersson
2025-01-13 8:14 ` Dmitry Baryshkov
@ 2025-01-20 8:15 ` Dan Carpenter
2 siblings, 0 replies; 15+ messages in thread
From: Dan Carpenter @ 2025-01-20 8:15 UTC (permalink / raw)
To: oe-kbuild, Raviteja Laggyshetty, Georgi Djakov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, Odelu Kukatla, Mike Tipton, Vivek Aknurwar
Cc: lkp, oe-kbuild-all, Sibi Sankar, linux-arm-msm, linux-pm,
devicetree, linux-kernel
Hi Raviteja,
kernel test robot noticed the following build warnings:
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Raviteja-Laggyshetty/interconnect-core-Add-dynamic-id-allocation-support/20250112-001756
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20250111161429.51-2-quic_rlaggysh%40quicinc.com
patch subject: [PATCH V7 1/5] interconnect: core: Add dynamic id allocation support
config: arm-randconfig-r072-20250118 (https://download.01.org/0day-ci/archive/20250120/202501201530.UTAPd4lC-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project c23f2417dc5f6dc371afb07af5627ec2a9d373a0)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202501201530.UTAPd4lC-lkp@intel.com/
smatch warnings:
drivers/interconnect/core.c:889 icc_node_create_alloc_id() warn: inconsistent returns 'global &icc_lock'.
vim +889 drivers/interconnect/core.c
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 867 struct icc_node *icc_node_create_alloc_id(int start_id)
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 868 {
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 869 struct icc_node *node;
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 870 int id;
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 871
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 872 mutex_lock(&icc_lock);
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 873
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 874 node = kzalloc(sizeof(*node), GFP_KERNEL);
Do this allocation before taking the mutex_lock(&icc_lock). Otherwise
you'd have to unlock before returning.
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 875 if (!node)
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 876 return ERR_PTR(-ENOMEM);
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 877
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 878 id = idr_alloc(&icc_idr, node, start_id, 0, GFP_KERNEL);
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 879 if (id < 0) {
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 880 WARN(1, "%s: couldn't get idr\n", __func__);
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 881 kfree(node);
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 882 node = ERR_PTR(id);
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 883 goto out;
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 884 }
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 885 node->id = id;
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 886 out:
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 887 mutex_unlock(&icc_lock);
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 888
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 @889 return node;
65971f5d716cb8 Raviteja Laggyshetty 2025-01-11 890 }
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V7 1/5] interconnect: core: Add dynamic id allocation support
2025-01-11 20:57 ` Bjorn Andersson
@ 2025-01-23 10:35 ` Raviteja Laggyshetty
0 siblings, 0 replies; 15+ messages in thread
From: Raviteja Laggyshetty @ 2025-01-23 10:35 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Georgi Djakov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Konrad Dybcio, Odelu Kukatla, Mike Tipton, Vivek Aknurwar,
Sibi Sankar, linux-arm-msm, linux-pm, devicetree, linux-kernel
On 1/12/2025 2:27 AM, Bjorn Andersson wrote:
> On Sat, Jan 11, 2025 at 04:14:25PM +0000, Raviteja Laggyshetty wrote:
>> Current interconnect framework is based on static IDs for creating node
>> and registering with framework. This becomes a limitation for topologies
>> where there are multiple instances of same interconnect provider. Add
>> icc_node_create_alloc_id() API to create icc node with dynamic id, this
>> will help to overcome the dependency on static IDs.
>>
>> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
>> ---
>> drivers/interconnect/core.c | 32 +++++++++++++++++++++++++++
>> include/linux/interconnect-provider.h | 6 +++++
>> 2 files changed, 38 insertions(+)
>>
>> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
>> index 9d5404a07e8a..0b7093eb51af 100644
>> --- a/drivers/interconnect/core.c
>> +++ b/drivers/interconnect/core.c
>> @@ -858,6 +858,38 @@ struct icc_node *icc_node_create(int id)
>> }
>> EXPORT_SYMBOL_GPL(icc_node_create);
>>
>> +/**
>> + * icc_node_create_alloc_id() - create node and dynamically allocate id
>> + * @start_id: min id to be allocated
>> + *
>> + * Return: icc_node pointer on success, or ERR_PTR() on error
>> + */
>> +struct icc_node *icc_node_create_alloc_id(int start_id)
>
> By having clients pass in start_id, you distribute the decision of what
> a "good number" is across multiple parts of the system (or you have
> clients relying on getting [start_id, start_id + N) back).
>
> Wouldn't it be better to hide that choice in one place (inside the icc
> framework)?
>
Yes, inline to Dmitry's suggestion I will be moving the start_id to
framework and all dynamic allocations will start from 10000.
> Regards,
> Bjorn
>
>> +{
>> + struct icc_node *node;
>> + int id;
>> +
>> + mutex_lock(&icc_lock);
>> +
>> + node = kzalloc(sizeof(*node), GFP_KERNEL);
>> + if (!node)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + id = idr_alloc(&icc_idr, node, start_id, 0, GFP_KERNEL);
>> + if (id < 0) {
>> + WARN(1, "%s: couldn't get idr\n", __func__);
>> + kfree(node);
>> + node = ERR_PTR(id);
>> + goto out;
>> + }
>> + node->id = id;
>> +out:
>> + mutex_unlock(&icc_lock);
>> +
>> + return node;
>> +}
>> +EXPORT_SYMBOL_GPL(icc_node_create_alloc_id);
>> +
>> /**
>> * icc_node_destroy() - destroy a node
>> * @id: node id
>> diff --git a/include/linux/interconnect-provider.h b/include/linux/interconnect-provider.h
>> index f5aef8784692..4fc7a5884374 100644
>> --- a/include/linux/interconnect-provider.h
>> +++ b/include/linux/interconnect-provider.h
>> @@ -117,6 +117,7 @@ struct icc_node {
>> int icc_std_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
>> u32 peak_bw, u32 *agg_avg, u32 *agg_peak);
>> struct icc_node *icc_node_create(int id);
>> +struct icc_node *icc_node_create_alloc_id(int start_id);
>> void icc_node_destroy(int id);
>> int icc_link_create(struct icc_node *node, const int dst_id);
>> void icc_node_add(struct icc_node *node, struct icc_provider *provider);
>> @@ -141,6 +142,11 @@ static inline struct icc_node *icc_node_create(int id)
>> return ERR_PTR(-ENOTSUPP);
>> }
>>
>> +static inline struct icc_node *icc_node_create_alloc_id(int start_id)
>> +{
>> + return ERR_PTR(-EOPNOTSUPP);
>> +}
>> +
>> static inline void icc_node_destroy(int id)
>> {
>> }
>> --
>> 2.39.2
>>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V7 2/5] interconnect: qcom: Add multidev EPSS L3 support
2025-01-11 21:08 ` Bjorn Andersson
@ 2025-01-23 10:40 ` Raviteja Laggyshetty
0 siblings, 0 replies; 15+ messages in thread
From: Raviteja Laggyshetty @ 2025-01-23 10:40 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Georgi Djakov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Konrad Dybcio, Odelu Kukatla, Mike Tipton, Vivek Aknurwar,
Sibi Sankar, linux-arm-msm, linux-pm, devicetree, linux-kernel
On 1/12/2025 2:38 AM, Bjorn Andersson wrote:
> On Sat, Jan 11, 2025 at 04:14:26PM +0000, Raviteja Laggyshetty wrote:
>> EPSS on SA8775P has two instances which requires creation of two device
>> nodes with different compatible and device data because of unique
>> icc node id and name limitation in interconnect framework.
>> Add multidevice support to osm-l3 code to get unique node id from icc
>> framework.
>>
>> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
>> ---
>> drivers/interconnect/qcom/osm-l3.c | 91 ++++++++++++++++++++++--------
>> 1 file changed, 67 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/interconnect/qcom/osm-l3.c b/drivers/interconnect/qcom/osm-l3.c
>> index 6a656ed44d49..8e98d1c9a840 100644
>> --- a/drivers/interconnect/qcom/osm-l3.c
>> +++ b/drivers/interconnect/qcom/osm-l3.c
>> @@ -1,6 +1,7 @@
>> // SPDX-License-Identifier: GPL-2.0
>> /*
>> * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
>> + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
>> */
>>
>> #include <linux/args.h>
>> @@ -11,6 +12,7 @@
>> #include <linux/kernel.h>
>> #include <linux/module.h>
>> #include <linux/of.h>
>> +#include <linux/of_address.h>
>> #include <linux/platform_device.h>
>>
>> #include <dt-bindings/interconnect/qcom,osm-l3.h>
>> @@ -34,6 +36,9 @@
>>
>> #define OSM_L3_MAX_LINKS 1
>>
>> +#define OSM_L3_NODE_ID_START 10000
>> +#define OSM_NODE_NAME_SUFFIX_SIZE 10
>> +
>> #define to_osm_l3_provider(_provider) \
>> container_of(_provider, struct qcom_osm_l3_icc_provider, provider)
>>
>> @@ -55,46 +60,40 @@ struct qcom_osm_l3_icc_provider {
>> */
>> struct qcom_osm_l3_node {
>> const char *name;
>> - u16 links[OSM_L3_MAX_LINKS];
>> + const char *links[OSM_L3_MAX_LINKS];
>> u16 id;
>> u16 num_links;
>> u16 buswidth;
>> };
>>
>> struct qcom_osm_l3_desc {
>> - const struct qcom_osm_l3_node * const *nodes;
>> + struct qcom_osm_l3_node * const *nodes;
>> size_t num_nodes;
>> unsigned int lut_row_size;
>> unsigned int reg_freq_lut;
>> unsigned int reg_perf_state;
>> };
>>
>> -enum {
>> - OSM_L3_MASTER_NODE = 10000,
>> - OSM_L3_SLAVE_NODE,
>> -};
>> -
>> -#define DEFINE_QNODE(_name, _id, _buswidth, ...) \
>> - static const struct qcom_osm_l3_node _name = { \
>> +#define DEFINE_QNODE(_name, _buswidth, ...) \
>> + static struct qcom_osm_l3_node _name = { \
>> .name = #_name, \
>> - .id = _id, \
>> .buswidth = _buswidth, \
>> .num_links = COUNT_ARGS(__VA_ARGS__), \
>> - .links = { __VA_ARGS__ }, \
>> + __VA_OPT__(.links = { #__VA_ARGS__ }) \
>> }
>>
>> -DEFINE_QNODE(osm_l3_master, OSM_L3_MASTER_NODE, 16, OSM_L3_SLAVE_NODE);
>> -DEFINE_QNODE(osm_l3_slave, OSM_L3_SLAVE_NODE, 16);
>> +DEFINE_QNODE(osm_l3_master, 16, osm_l3_slave);
>> +DEFINE_QNODE(osm_l3_slave, 16);
>>
>> -static const struct qcom_osm_l3_node * const osm_l3_nodes[] = {
>> +static struct qcom_osm_l3_node * const osm_l3_nodes[] = {
>> [MASTER_OSM_L3_APPS] = &osm_l3_master,
>> [SLAVE_OSM_L3] = &osm_l3_slave,
>> };
>>
>> -DEFINE_QNODE(epss_l3_master, OSM_L3_MASTER_NODE, 32, OSM_L3_SLAVE_NODE);
>> -DEFINE_QNODE(epss_l3_slave, OSM_L3_SLAVE_NODE, 32);
>> +DEFINE_QNODE(epss_l3_master, 32, epss_l3_slave);
>> +DEFINE_QNODE(epss_l3_slave, 32);
>>
>> -static const struct qcom_osm_l3_node * const epss_l3_nodes[] = {
>> +static struct qcom_osm_l3_node * const epss_l3_nodes[] = {
>> [MASTER_EPSS_L3_APPS] = &epss_l3_master,
>> [SLAVE_EPSS_L3_SHARED] = &epss_l3_slave,
>> };
>> @@ -123,6 +122,19 @@ static const struct qcom_osm_l3_desc epss_l3_l3_vote = {
>> .reg_perf_state = EPSS_REG_L3_VOTE,
>> };
>>
>> +static u16 get_node_id_by_name(const char *node_name,
>> + const struct qcom_osm_l3_desc *desc)
>> +{
>> + struct qcom_osm_l3_node *const *nodes = desc->nodes;
>> + int i;
>> +
>> + for (i = 0; i < desc->num_nodes; i++) {
>> + if (!strcmp(nodes[i]->name, node_name))
>> + return nodes[i]->id;
>> + }
>> + return 0;
>> +}
>> +
>> static int qcom_osm_l3_set(struct icc_node *src, struct icc_node *dst)
>> {
>> struct qcom_osm_l3_icc_provider *qp;
>> @@ -164,10 +176,11 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
>> const struct qcom_osm_l3_desc *desc;
>> struct icc_onecell_data *data;
>> struct icc_provider *provider;
>> - const struct qcom_osm_l3_node * const *qnodes;
>> + struct qcom_osm_l3_node * const *qnodes;
>> struct icc_node *node;
>> size_t num_nodes;
>> struct clk *clk;
>> + u64 addr;
>> int ret;
>>
>> clk = clk_get(&pdev->dev, "xo");
>> @@ -188,6 +201,10 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
>> if (!qp)
>> return -ENOMEM;
>>
>> + ret = of_property_read_reg(pdev->dev.of_node, 0, &addr, NULL);
>> + if (ret)
>> + return ret;
>> +
>> qp->base = devm_platform_ioremap_resource(pdev, 0);
>> if (IS_ERR(qp->base))
>> return PTR_ERR(qp->base);
>> @@ -242,26 +259,51 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
>>
>> icc_provider_init(provider);
>>
>> + /* create icc nodes */
>> for (i = 0; i < num_nodes; i++) {
>> - size_t j;
>> + char *node_name;
>> + size_t len;
>>
>> - node = icc_node_create(qnodes[i]->id);
>> + node = icc_node_create_alloc_id(OSM_L3_NODE_ID_START);
>> if (IS_ERR(node)) {
>> ret = PTR_ERR(node);
>> goto err;
>> }
>> + qnodes[i]->id = node->id;
>> +
>> + /* len = strlen(node->name) + @ + 8 (base-address) + NULL */
>> + len = strlen(qnodes[i]->name) + OSM_NODE_NAME_SUFFIX_SIZE;
>> + node_name = devm_kzalloc(&pdev->dev, len, GFP_KERNEL);
>> + if (!node_name) {
>> + ret = -ENOMEM;
>> + goto err;
>> + }
>> +
>> + snprintf(node_name, len, "%s@%08llx", qnodes[i]->name, addr);
>> + node->name = node_name;
>
> I don't think it's reasonable to duplicate this logic and the decision
> of naming convention in each provider driver. Please provide a generic
> solution in the framework.
>
I will be moving the logic to framework.
>
> PS. Not that I want you to use it here, but for the next time be aware
> of devm_kasprintf().
>
Will make use of devm_kasprintf in the next patch revision.
>>
>> - node->name = qnodes[i]->name;
>> /* Cast away const and add it back in qcom_osm_l3_set() */
>> node->data = (void *)qnodes[i];
>> icc_node_add(node, provider);
>>
>> - for (j = 0; j < qnodes[i]->num_links; j++)
>> - icc_link_create(node, qnodes[i]->links[j]);
>> -
>> data->nodes[i] = node;
>> }
>>
>> + /* create links in topolgy */
>> + for (i = 0; i < num_nodes; i++) {
>> + size_t j;
>> +
>> + node = data->nodes[i];
>> + for (j = 0; j < qnodes[i]->num_links; j++) {
>> + u16 link_node_id = get_node_id_by_name(qnodes[i]->links[j], desc);
>
> Isn't that O(i^2*j) string comparisons? I don't find that acceptable.
Agreed, I will be linking the nodes using pointers instead of strings,
this will avoid additional loops and lookups while creating the links.
>
>> +
>> + if (link_node_id)
>> + icc_link_create(node, link_node_id);
>> + else
>> + goto err;
>> + }
>> + }
>> +
>> ret = icc_provider_register(provider);
>> if (ret)
>> goto err;
>> @@ -284,6 +326,7 @@ static const struct of_device_id osm_l3_of_match[] = {
>> { .compatible = "qcom,sm8150-osm-l3", .data = &osm_l3 },
>> { .compatible = "qcom,sc8180x-osm-l3", .data = &osm_l3 },
>> { .compatible = "qcom,sm8250-epss-l3", .data = &epss_l3_perf_state },
>> + { .compatible = "qcom,sa8775p-epss-l3", .data = &epss_l3_perf_state },
>
> With the exception of sc8180x, this list is sorted alphabetically.
> Please insert your entry where it makes sense.
>
Sure, I will fix it in next patch series.
> Regards,
> Bjorn
>
>> { }
>> };
>> MODULE_DEVICE_TABLE(of, osm_l3_of_match);
>> --
>> 2.39.2
>>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V7 1/5] interconnect: core: Add dynamic id allocation support
2025-01-13 8:14 ` Dmitry Baryshkov
@ 2025-01-23 10:47 ` Raviteja Laggyshetty
0 siblings, 0 replies; 15+ messages in thread
From: Raviteja Laggyshetty @ 2025-01-23 10:47 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Georgi Djakov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, Odelu Kukatla, Mike Tipton,
Vivek Aknurwar, Sibi Sankar, linux-arm-msm, linux-pm, devicetree,
linux-kernel
On 1/13/2025 1:44 PM, Dmitry Baryshkov wrote:
> On Sat, Jan 11, 2025 at 04:14:25PM +0000, Raviteja Laggyshetty wrote:
>> Current interconnect framework is based on static IDs for creating node
>> and registering with framework. This becomes a limitation for topologies
>> where there are multiple instances of same interconnect provider. Add
>> icc_node_create_alloc_id() API to create icc node with dynamic id, this
>> will help to overcome the dependency on static IDs.
>
> This doesn't overcome the dependency on static ID. Drivers still have to
> manually lookup the resulting ID and use it to link the nodes. Instead
> ICC framework should be providing a completely dynamic solution:
> - icc_node_create() should get a completely dynamic counterpart. Use
> e.g. 1000000 as a dynamic start ID.
> - icc_link_create() shold get a counterpart which can create a link
> between two icc_node instances directly, without an additional lookup.
>
Agreed, with current implementation, still there is dependency on IDs
for linking the nodes.
Instead of relying on node names for the links, array of struct pointers
will be used, this will eliminate the need for ID lookup and avoids
extra loops.
Instead of providing counter part for the ICC framework APIs which
involves duplication of most of the code, I will modify the existing
icc_node_create, icc_link_create and icc_node_add APIs to support both
static and dynamic IDs.
> You can check if your implementation is correct if you can refactor
> existing ICC drivers (e.g. icc-clk and/or icc-rpm to drop ID arrays
> completely).
>
ok, I will check the implementation on icc-rpmh driver for sa8775p SoC.
>>
>> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
>> ---
>> drivers/interconnect/core.c | 32 +++++++++++++++++++++++++++
>> include/linux/interconnect-provider.h | 6 +++++
>> 2 files changed, 38 insertions(+)
>>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V7 3/5] dt-bindings: interconnect: Add EPSS L3 compatible for SA8775P
2025-01-12 9:31 ` Krzysztof Kozlowski
@ 2025-01-23 12:07 ` Raviteja Laggyshetty
0 siblings, 0 replies; 15+ messages in thread
From: Raviteja Laggyshetty @ 2025-01-23 12:07 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Georgi Djakov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, Odelu Kukatla, Mike Tipton,
Vivek Aknurwar, Sibi Sankar, linux-arm-msm, linux-pm, devicetree,
linux-kernel
On 1/12/2025 3:01 PM, Krzysztof Kozlowski wrote:
> On Sat, Jan 11, 2025 at 04:14:27PM +0000, Raviteja Laggyshetty wrote:
>> Add Epoch Subsystem (EPSS) L3 interconnect provider binding on
>> SA8775P SoCs.
>
> 1. And why is this not compatible with sm8250? There was lengthy
> discussion and no outcome of it managed to get to commit msg. Really, so
> we are going to repeat everything again and you will not get any acks.
>
sa8775p is compatible with sm8250, but only difference is sa8775p has
two instances of L3 EPSS. Initial patches were posted with two
compatibles for supporting two instances.
Later there was a comment to reuse the existing and avoid growing the
compatibles in the driver.
Initially we thought to have separate generic compatible
"qcom,epss-l3-perf" for SoCs which support L3 voting through
EPSS_L3_PERF state register. But actually, it is already supported in
the driver for sm8250 and sc7280. We can reuse sm8250 or sc7280
compatible for sa8775p.
I will update the commit text with details in the next patch revision.
> You have entire commit msg to explain things but instead you repeat what
> the patch does. We can read the diff for that.
>
> 2. Binding *ALWAYS* comes before the user.
Sure, I will update the patch order to bindings, driver and dt in next
patch revision and will follow the same for future patchsets.
>
>>
>> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
>> ---
>> Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
>> index 21dae0b92819..94f7f283787a 100644
>> --- a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
>> +++ b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
>> @@ -33,6 +33,7 @@ properties:
>> - qcom,sm6375-cpucp-l3
>> - qcom,sm8250-epss-l3
>> - qcom,sm8350-epss-l3
>> + - qcom,sa8775p-epss-l3
>> - const: qcom,epss-l3
>
> Your driver suggests this is not really true - it is not compatible with
> qcom,epss-l3. Maybe it is, maybe not, no clue, commit explains nothing.
"qcom,epss-l3" is generic compatible introduced for EPSS H/W from sm8250
SoC. sm8250, sa8775p and sc7280 SoCs have same EPSS H/W and use
EPSS_L3_PERF register for configuring the perf level. We thought to add
generic compatible "qcom,epss-l3-perf" for configuring perf level
through EPSS_L3_PERF register.
Later, as suggested by reviewers, looks like using a different register
cannot be a reason to have different generic compatible as the EPSS H/W
is still same on all the SoCs.
>
> Best regards,
> Krzysztof
>
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2025-01-23 12:07 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-01-11 16:14 [V7 0/5] Add EPSS L3 provider support on SA8775P SoC Raviteja Laggyshetty
2025-01-11 16:14 ` [PATCH V7 1/5] interconnect: core: Add dynamic id allocation support Raviteja Laggyshetty
2025-01-11 20:57 ` Bjorn Andersson
2025-01-23 10:35 ` Raviteja Laggyshetty
2025-01-13 8:14 ` Dmitry Baryshkov
2025-01-23 10:47 ` Raviteja Laggyshetty
2025-01-20 8:15 ` Dan Carpenter
2025-01-11 16:14 ` [PATCH V7 2/5] interconnect: qcom: Add multidev EPSS L3 support Raviteja Laggyshetty
2025-01-11 21:08 ` Bjorn Andersson
2025-01-23 10:40 ` Raviteja Laggyshetty
2025-01-11 16:14 ` [PATCH V7 3/5] dt-bindings: interconnect: Add EPSS L3 compatible for SA8775P Raviteja Laggyshetty
2025-01-12 9:31 ` Krzysztof Kozlowski
2025-01-23 12:07 ` Raviteja Laggyshetty
2025-01-11 16:14 ` [PATCH V7 4/5] arm64: dts: qcom: sa8775p: add EPSS l3 interconnect provider Raviteja Laggyshetty
2025-01-11 16:14 ` [PATCH V7 5/5] arm64: dts: qcom: sa8775p: Add CPU OPP tables to scale DDR/L3 Raviteja Laggyshetty
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®