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.
> >> .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().
>
> >
> >> +
> >> 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.
> >
> >>
> >> 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.
>
>
> >> + 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
> >>
> >
>
--
With best wishes
Dmitry
