Re: [PATCH V9 4/7] interconnect: qcom: icc-rpmh: Add dynamic icc node id support

[Mike Tipton <quic_mdtipton@quicinc.com>]

On Thu, Feb 27, 2025 at 03:52:10PM +0000, Raviteja Laggyshetty wrote:
> To facilitate dynamic node ID support, the driver now uses
> node pointers for links instead of static node IDs.
> Additionally, the default node ID is set to -1 to prompt
> the ICC framework for dynamic node ID allocation.
> 
> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
> ---
>  drivers/interconnect/qcom/icc-rpmh.c | 16 ++++++++++++++--
>  drivers/interconnect/qcom/icc-rpmh.h |  3 ++-
>  2 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/interconnect/qcom/icc-rpmh.c b/drivers/interconnect/qcom/icc-rpmh.c
> index f2d63745be54..2e654917f535 100644
> --- a/drivers/interconnect/qcom/icc-rpmh.c
> +++ b/drivers/interconnect/qcom/icc-rpmh.c
> @@ -285,13 +285,25 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev)
>  			ret = PTR_ERR(node);
>  			goto err_remove_nodes;
>  		}
> +		qn->id = node->id;
>  
>  		node->name = qn->name;
>  		node->data = qn;
>  		icc_node_add(node, provider);
>  
> -		for (j = 0; j < qn->num_links; j++)
> -			icc_link_create(node, qn->links[j]);
> +		for (j = 0; j < qn->num_links; j++) {
> +			struct qcom_icc_node *qn_link_node = qn->link_nodes[j];
> +			struct icc_node *link_node;
> +
> +			if (qn_link_node) {
> +				link_node = icc_node_create(qn_link_node->id);
> +				qn_link_node->id = link_node->id;
> +				icc_link_create(node, qn_link_node->id);
> +			} else {
> +				/* backward compatibility for target using static IDs */
> +				icc_link_create(node, qn->links[j]);
> +			}
> +		}
>  
>  		data->nodes[i] = node;
>  	}
> diff --git a/drivers/interconnect/qcom/icc-rpmh.h b/drivers/interconnect/qcom/icc-rpmh.h
> index 82344c734091..cf4aa69c707c 100644
> --- a/drivers/interconnect/qcom/icc-rpmh.h
> +++ b/drivers/interconnect/qcom/icc-rpmh.h
> @@ -95,7 +95,8 @@ struct qcom_icc_qosbox {
>  struct qcom_icc_node {
>  	const char *name;
>  	u16 links[MAX_LINKS];
> -	u16 id;
> +	struct qcom_icc_node *link_nodes[MAX_LINKS];

This is very inefficient. MAX_LINKS = 128, which means we're adding an
additional 1KB *per-node*. The vast majority of nodes don't come
anywhere close to this number of links, so this is almost entirely
unused and wasted space.

As an example: sa8775p has 193 nodes, so we're adding 193K to the driver
from this alone. The current driver size is 84K, and the size after this
change is 283K.

Instead of embedding this array with a hardcoded size, we could point to
an array that's sized for the number of links required by the node:

    - struct qcom_icc_node *link_nodes[MAX_LINKS];
    + struct qcom_icc_node **link_nodes;

Then when initializing the arrays, we could:

    - .link_nodes = { &qns_a1noc_snoc },
    + .link_nodes = (struct qcom_icc_node *[]) { &qns_a1noc_snoc },

And for handling compatiblity with older drivers, we'd check for
link_nodes != NULL instead of checking the array indices.

Doing it this way would reduce the new sa8775p size from 283K to 88K.

A similar argument could be made for qcom_icc_node::links, since that's
also hardcoded to MAX_LINKS. But it's not quite as bad since it's an
array of u16 rather than an array of pointers. Still, if we implemented
similar changes for qcom_icc_node::links, then we'd save almost 256B
per-node, which for sa8775p would reduce the size by roughly another
50K. If we're ultimately planning on switching all the old drivers over
to link_nodes, then we could just wait and get rid of links entirely.
Regardless, optimizing links doesn't have to happen in this series, but
I don't want to further bloat the size from the addition of link_nodes.

> +	int id;
>  	u16 num_links;
>  	u16 channels;
>  	u16 buswidth;
> -- 
> 2.43.0
> 
>