Re: [PATCH V5 3/4] interconnect: qcom: Add EPSS L3 support on SA8775P

[Krzysztof Kozlowski <krzk@kernel.org>]

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).



>  	[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