On 2/17/2025 6:38 AM, Dmitry Baryshkov wrote:
> On Sun, Feb 16, 2025 at 09:58:41PM +0530, Raviteja Laggyshetty wrote:
>>
>>
>> On 2/10/2025 4:27 PM, Dmitry Baryshkov wrote:
>>> On Wed, Feb 05, 2025 at 06:27:39PM +0000, Raviteja Laggyshetty wrote:
>>>> EPSS on SA8775P has two instances, necessitating the creation of two
>>>> device nodes with different compatibles due to the unique ICC node ID
>>>> and name limitations in the interconnect framework. Add multidevice
>>>> support for the OSM-L3 provider to dynamically obtain unique node IDs
>>>> and register with the framework.
>>>>
>>>> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@xxxxxxxxxxx>
>>>> ---
>>>> drivers/interconnect/qcom/osm-l3.c | 46 +++++++++++++++++-------------
>>>> 1 file changed, 26 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/drivers/interconnect/qcom/osm-l3.c b/drivers/interconnect/qcom/osm-l3.c
>>>> index 6a656ed44d49..da2d82700b5a 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>
>>>> @@ -33,6 +34,7 @@
>>>> #define EPSS_REG_PERF_STATE 0x320
>>>>
>>>> #define OSM_L3_MAX_LINKS 1
>>>> +#define ALLOC_DYN_ID -1
>>>
>>> This should be defined by ICC framework.
>>
>> ok, I will move this to framework.
>>>
>>>>
>>>> #define to_osm_l3_provider(_provider) \
>>>> container_of(_provider, struct qcom_osm_l3_icc_provider, provider)
>>>> @@ -55,46 +57,40 @@ struct qcom_osm_l3_icc_provider {
>>>> */
>>>> struct qcom_osm_l3_node {
>>>> const char *name;
>>>> - u16 links[OSM_L3_MAX_LINKS];
>>>> - u16 id;
>>>> + struct qcom_osm_l3_node *links[OSM_L3_MAX_LINKS];
>>>> + int 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 = { \
>
> No. Global data _must_ remain const.
Ok, will make the global struct const.
>
>>>> .name = #_name, \
>>>> - .id = _id, \
>>>> .buswidth = _buswidth, \
>>>> .num_links = COUNT_ARGS(__VA_ARGS__), \
>>>> .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_slave, 16);
>>>> +DEFINE_QNODE(osm_l3_master, 16, &osm_l3_slave);
>>>>
>>>> -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_slave, 32);
>>>> +DEFINE_QNODE(epss_l3_master, 32, &epss_l3_slave);
>>>>
>>>> -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,
>>>> };
>>>> @@ -164,7 +160,7 @@ 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;
>>>> @@ -242,6 +238,10 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
>>>>
>>>> icc_provider_init(provider);
>>>>
>>>> + /*Initialize IDs to ALLOC_DYN_ID to indicate dynamic id allocation*/
>>>> + for (i = 0; i < num_nodes; i++)
>>>> + qnodes[i]->id = ALLOC_DYN_ID;
>>>
>>> This can be initialized statically.
>>
>> There are two instances of EPSS L3 and the target specific compatible
>> data is global which requires resetting the IDs for the second instance
>> probe. If we don't the reset the IDs back to ALLOC_DYN_ID, then ICC
>> framework assumes that ID has been already allocated and doesn't create
>> the new ICC nodes for the second instance.
>
> Well, don't use global data for shared purposes. Consider both your
> instances probing at the same time. So, please drop the
> qcom_osm_l3_node.id, pass ALLOC_DYN_ID directly to the
> icc_node_create(), store returned nodes in a local array and pass node
> pointers to icc_link_create().
>
Will pass ALLOC_DYN_ID as argument to create node instead of
qcom_osm_l3_node.id and avoid its usage.
Instead of creating the local array to store the pointers, will make use
of icc_onecell_data which stores all the nodes present in the provider.
>>
>>>
>>>> +
>>>> for (i = 0; i < num_nodes; i++) {
>>>> size_t j;
>>>>
>>>> @@ -250,14 +250,19 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
>>>> ret = PTR_ERR(node);
>>>> goto err;
>>>> }
>>>> + qnodes[i]->id = node->id;
>>>
>>> Should not be necessary.
>> This is required, each qnode corresponds to a ICC node in framework and
>> some nodes get created in icc_node_create() API and some in
>> icc_link_create() API, to have a track of node creation qnode->id is
>> used, hence initializing qnode->id with id allocated during icc node
>> creation and avoid creation of duplicate nodes.
>
> Basically, no. You cannot do that. Create nodes first, create links
> afterwards.
Sure, Will create nodes first and then create the links.
>
>>>
>>>>
>>>> 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]);
>>>> + for (j = 0; j < qnodes[i]->num_links; j++) {
>>>> + struct qcom_osm_l3_node *link_node = qnodes[i]->links[j];
>>>> +
>>>> + icc_link_create(node, link_node->id);
>>>
>>> Please add icc_link_nodes() (or something like that), taking two struct
>>> icc_node instances. Then you can use it here, instead of reading back
>>> the ID. Ideally the 'ID' should become an internal detail which is of no
>>> concern for the ICC drivers.
>>>
>>
>> Instead of reading back the link node id from the framework, I will call
>> icc_node_create before calling the icc_link_create() API and assign the
>> allocated id to respective qnode in the following way:
>>
>> struct qcom_osm_l3_node *qn_link_node = qnodes[i]->links[j];
>> struct icc_node *link_node = icc_node_create(qnodes[i]->links[j]->id);
>> qn_link_node->id = link_node->id;
>> icc_link_create(node, link_node->id);
>>
>> This looks cleaner than reading back the id.
>
> As you might have guessed from the the earlier comments, no. Don't write
> _anything_ to a global data.
>
Will not modify or update the global data.
>>
>>
>>>> + link_node->id = (node->links[node->num_links - 1])->id;
>>>> + }
>>>>
>>>> data->nodes[i] = node;
>>>> }
>>>> @@ -278,6 +283,7 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
>>>> static const struct of_device_id osm_l3_of_match[] = {
>>>> { .compatible = "qcom,epss-l3", .data = &epss_l3_l3_vote },
>>>> { .compatible = "qcom,osm-l3", .data = &osm_l3 },
>>>> + { .compatible = "qcom,sa8775p-epss-l3", .data = &epss_l3_perf_state },
>>>> { .compatible = "qcom,sc7180-osm-l3", .data = &osm_l3 },
>>>> { .compatible = "qcom,sc7280-epss-l3", .data = &epss_l3_perf_state },
>>>> { .compatible = "qcom,sdm845-osm-l3", .data = &osm_l3 },
>>>> --
>>>> 2.39.2
>>>>
>>>
>>
>
