Add Epoch Subsystem (EPSS) L3 interconnect provider on
SA8775P SoCs with multiple device support.
Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
---
drivers/interconnect/qcom/osm-l3.c | 85 ++++++++++++++++++++++--------
1 file changed, 63 insertions(+), 22 deletions(-)
diff --git a/drivers/interconnect/qcom/osm-l3.c b/drivers/interconnect/qcom/osm-l3.c
index 6a656ed44d49..a9405b7d251b 100644
--- a/drivers/interconnect/qcom/osm-l3.c
+++ b/drivers/interconnect/qcom/osm-l3.c
@@ -1,16 +1,19 @@
// SPDX-License-Identifier: GPL-2.0
/*
* Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
*/
#include <linux/args.h>
#include <linux/bitfield.h>
#include <linux/clk.h>
+#include <linux/idr.h>
#include <linux/interconnect-provider.h>
#include <linux/io.h>
#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,9 +37,14 @@
#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)
+static DEFINE_IDA(osm_l3_id);
+
struct qcom_osm_l3_icc_provider {
void __iomem *base;
unsigned int max_state;
@@ -55,46 +63,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 +125,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 +179,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 +204,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,8 +262,13 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
icc_provider_init(provider);
+ /* Allocate unique id for qnodes */
+ for (i = 0; i < num_nodes; i++)
+ qnodes[i]->id = ida_alloc_min(&osm_l3_id, OSM_L3_NODE_ID_START, GFP_KERNEL);
+
for (i = 0; i < num_nodes; i++) {
- size_t j;
+ char *node_name;
+ size_t j, len;
node = icc_node_create(qnodes[i]->id);
if (IS_ERR(node)) {
@@ -251,13 +276,29 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
goto err;
}
- node->name = qnodes[i]->name;
+ /* 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;
+
/* 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]);
+ 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;
+ }
data->nodes[i] = node;
}
--
2.39.2On 21/11/2024 12:30, Raviteja Laggyshetty wrote:
> Add Epoch Subsystem (EPSS) L3 interconnect provider on
> SA8775P SoCs with multiple device support.
>
...
> -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[] = {
I think dropping const makes the code worse, not better. Commit msg does
not explain all these changes and I could not figure out the intention
(except modifying but that's just obvious).
> [MASTER_EPSS_L3_APPS] = &epss_l3_master,
> [SLAVE_EPSS_L3_SHARED] = &epss_l3_slave,
> };
> @@ -123,6 +125,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 +179,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 +204,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,8 +262,13 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
>
> icc_provider_init(provider);
>
> + /* Allocate unique id for qnodes */
> + for (i = 0; i < num_nodes; i++)
> + qnodes[i]->id = ida_alloc_min(&osm_l3_id, OSM_L3_NODE_ID_START, GFP_KERNEL);
> +
> for (i = 0; i < num_nodes; i++) {
> - size_t j;
> + char *node_name;
> + size_t j, len;
>
> node = icc_node_create(qnodes[i]->id);
> if (IS_ERR(node)) {
> @@ -251,13 +276,29 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
> goto err;
> }
>
> - node->name = qnodes[i]->name;
> + /* 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;
Why the node name becomes dynamic?
Best regards,
Krzysztof
On 11/21/2024 5:28 PM, Krzysztof Kozlowski wrote:
> On 21/11/2024 12:30, Raviteja Laggyshetty wrote:
>> Add Epoch Subsystem (EPSS) L3 interconnect provider on
>> SA8775P SoCs with multiple device support.
>>
>
>
> ...
>
>> -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[] = {
>
>
> I think dropping const makes the code worse, not better. Commit msg does
> not explain all these changes and I could not figure out the intention
> (except modifying but that's just obvious).
EPSS L3 on SA8775P has two instances and this requires creation of two device nodes in devicetree.
As Interconnect framework requires a unique node id, each device node needs to have different compatible and data.
To overcome the need of having two different compatibles and data, driver code has been modified to acquire unique node id from IDA
and the node name is made dynamic (nodename@address).
Updating node id and node name is not possible with const.
>
>
>
>> [MASTER_EPSS_L3_APPS] = &epss_l3_master,
>> [SLAVE_EPSS_L3_SHARED] = &epss_l3_slave,
>> };
>> @@ -123,6 +125,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 +179,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 +204,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,8 +262,13 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
>>
>> icc_provider_init(provider);
>>
>> + /* Allocate unique id for qnodes */
>> + for (i = 0; i < num_nodes; i++)
>> + qnodes[i]->id = ida_alloc_min(&osm_l3_id, OSM_L3_NODE_ID_START, GFP_KERNEL);
>> +
>> for (i = 0; i < num_nodes; i++) {
>> - size_t j;
>> + char *node_name;
>> + size_t j, len;
>>
>> node = icc_node_create(qnodes[i]->id);
>> if (IS_ERR(node)) {
>> @@ -251,13 +276,29 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
>> goto err;
>> }
>>
>> - node->name = qnodes[i]->name;
>> + /* 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;
>
>
> Why the node name becomes dynamic?
>
> Best regards,
> Krzysztof
On 21.11.2024 7:03 PM, Raviteja Laggyshetty wrote:
>
>
> On 11/21/2024 5:28 PM, Krzysztof Kozlowski wrote:
>> On 21/11/2024 12:30, Raviteja Laggyshetty wrote:
>>> Add Epoch Subsystem (EPSS) L3 interconnect provider on
>>> SA8775P SoCs with multiple device support.
>>>
>>
>>
>> ...
>>
>>> -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[] = {
>>
>>
>> I think dropping const makes the code worse, not better. Commit msg does
>> not explain all these changes and I could not figure out the intention
>> (except modifying but that's just obvious).
>
> EPSS L3 on SA8775P has two instances and this requires creation of two device nodes in devicetree.
> As Interconnect framework requires a unique node id, each device node needs to have different compatible and data.
> To overcome the need of having two different compatibles and data, driver code has been modified to acquire unique node id from IDA
> and the node name is made dynamic (nodename@address).
> Updating node id and node name is not possible with const.
I think it would be better to just fix the interconnect API to do it
internally
Konrad
On Thu, Nov 21, 2024 at 11:33:04PM +0530, Raviteja Laggyshetty wrote:
>
>
> On 11/21/2024 5:28 PM, Krzysztof Kozlowski wrote:
> > On 21/11/2024 12:30, Raviteja Laggyshetty wrote:
> >> Add Epoch Subsystem (EPSS) L3 interconnect provider on
> >> SA8775P SoCs with multiple device support.
> >>
> >
> >
> > ...
> >
> >> -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[] = {
> >
> >
> > I think dropping const makes the code worse, not better. Commit msg does
> > not explain all these changes and I could not figure out the intention
> > (except modifying but that's just obvious).
>
> EPSS L3 on SA8775P has two instances and this requires creation of two device nodes in devicetree.
> As Interconnect framework requires a unique node id, each device node needs to have different compatible and data.
> To overcome the need of having two different compatibles and data, driver code has been modified to acquire unique node id from IDA
> and the node name is made dynamic (nodename@address).
> Updating node id and node name is not possible with const.
Has this been described in the commit message? No. Have you had similar
questions since v1? Yes. What does that tell us?
Also, while we are at it. Please fix your email client settings to wrap
message body text on some sensible (72-75) width.
> >
> >
> >
> >> [MASTER_EPSS_L3_APPS] = &epss_l3_master,
> >> [SLAVE_EPSS_L3_SHARED] = &epss_l3_slave,
> >> };
> >> @@ -123,6 +125,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 +179,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 +204,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,8 +262,13 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
> >>
> >> icc_provider_init(provider);
> >>
> >> + /* Allocate unique id for qnodes */
> >> + for (i = 0; i < num_nodes; i++)
> >> + qnodes[i]->id = ida_alloc_min(&osm_l3_id, OSM_L3_NODE_ID_START, GFP_KERNEL);
> >> +
> >> for (i = 0; i < num_nodes; i++) {
> >> - size_t j;
> >> + char *node_name;
> >> + size_t j, len;
> >>
> >> node = icc_node_create(qnodes[i]->id);
> >> if (IS_ERR(node)) {
> >> @@ -251,13 +276,29 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
> >> goto err;
> >> }
> >>
> >> - node->name = qnodes[i]->name;
> >> + /* 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;
> >
> >
> > Why the node name becomes dynamic?
> >
> > Best regards,
> > Krzysztof
>
--
With best wishes
Dmitry
On 11/22/2024 3:44 AM, Dmitry Baryshkov wrote:
> On Thu, Nov 21, 2024 at 11:33:04PM +0530, Raviteja Laggyshetty wrote:
>>
>>
>> On 11/21/2024 5:28 PM, Krzysztof Kozlowski wrote:
>>> On 21/11/2024 12:30, Raviteja Laggyshetty wrote:
>>>> Add Epoch Subsystem (EPSS) L3 interconnect provider on
>>>> SA8775P SoCs with multiple device support.
>>>>
>>>
>>>
>>> ...
>>>
>>>> -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[] = {
>>>
>>>
>>> I think dropping const makes the code worse, not better. Commit msg does
>>> not explain all these changes and I could not figure out the intention
>>> (except modifying but that's just obvious).
>>
>> EPSS L3 on SA8775P has two instances and this requires creation of two device nodes in devicetree.
>> As Interconnect framework requires a unique node id, each device node needs to have different compatible and data.
>> To overcome the need of having two different compatibles and data, driver code has been modified to acquire unique node id from IDA
>> and the node name is made dynamic (nodename@address).
>> Updating node id and node name is not possible with const.
>
> Has this been described in the commit message? No. Have you had similar
> questions since v1? Yes. What does that tell us?
I will update the commit message in the next patch revision.
>
> Also, while we are at it. Please fix your email client settings to wrap
> message body text on some sensible (72-75) width.
Thanks for suggesting!
>
>>>
>>>
>>>
>>>> [MASTER_EPSS_L3_APPS] = &epss_l3_master,
>>>> [SLAVE_EPSS_L3_SHARED] = &epss_l3_slave,
>>>> };
>>>> @@ -123,6 +125,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 +179,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 +204,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,8 +262,13 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
>>>>
>>>> icc_provider_init(provider);
>>>>
>>>> + /* Allocate unique id for qnodes */
>>>> + for (i = 0; i < num_nodes; i++)
>>>> + qnodes[i]->id = ida_alloc_min(&osm_l3_id, OSM_L3_NODE_ID_START, GFP_KERNEL);
>>>> +
>>>> for (i = 0; i < num_nodes; i++) {
>>>> - size_t j;
>>>> + char *node_name;
>>>> + size_t j, len;
>>>>
>>>> node = icc_node_create(qnodes[i]->id);
>>>> if (IS_ERR(node)) {
>>>> @@ -251,13 +276,29 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
>>>> goto err;
>>>> }
>>>>
>>>> - node->name = qnodes[i]->name;
>>>> + /* 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;
>>>
>>>
>>> Why the node name becomes dynamic?
>>>
>>> Best regards,
>>> Krzysztof
>>
>
© 2016 - 2024 Red Hat, Inc.