linux-kernel - Re: [PATCH V9 3/7] interconnect: qcom: Add multidev EPSS L3 support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <a2hpdflba4wmho3wzphr2w5htgolksq43nuewcwqr7goy6rc5q@6fot5l6q6ods>
Date: Thu, 27 Feb 2025 18:39:55 -0600
From: Bjorn Andersson <andersson@...nel.org>
To: Raviteja Laggyshetty <quic_rlaggysh@...cinc.com>
Cc: Georgi Djakov <djakov@...nel.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Konrad Dybcio <konradybcio@...nel.org>, Odelu Kukatla <quic_okukatla@...cinc.com>, 
	Jeff Johnson <jeff.johnson@....qualcomm.com>, Mike Tipton <mdtipton@...cinc.com>, 
	Jagadeesh Kona <quic_jkona@...cinc.com>, Sibi Sankar <quic_sibis@...cinc.com>, 
	linux-arm-msm@...r.kernel.org, linux-pm@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH V9 3/7] interconnect: qcom: Add multidev EPSS L3 support

On Thu, Feb 27, 2025 at 03:52:09PM +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.
> EPSS topology includes a single master-slave pair within the same
> provider, the node linking logic is simplified by directly connecting
> the master node to the slave node.
> 
> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@...cinc.com>
> ---
>  drivers/interconnect/qcom/osm-l3.c | 37 ++++++++++--------------------
>  1 file changed, 12 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/interconnect/qcom/osm-l3.c b/drivers/interconnect/qcom/osm-l3.c
> index 6a656ed44d49..a9dacc4b7f2e 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>
> @@ -48,16 +49,10 @@ struct qcom_osm_l3_icc_provider {
>  /**
>   * struct qcom_osm_l3_node - Qualcomm specific interconnect nodes
>   * @name: the node name used in debugfs
> - * @links: an array of nodes where we can go next while traversing
> - * @id: a unique node identifier
> - * @num_links: the total number of @links
>   * @buswidth: width of the interconnect between a node and the bus
>   */
>  struct qcom_osm_l3_node {
>  	const char *name;
> -	u16 links[OSM_L3_MAX_LINKS];
> -	u16 id;
> -	u16 num_links;
>  	u16 buswidth;
>  };
>  
> @@ -69,30 +64,22 @@ struct qcom_osm_l3_desc {
>  	unsigned int reg_perf_state;
>  };
>  
> -enum {
> -	OSM_L3_MASTER_NODE = 10000,
> -	OSM_L3_SLAVE_NODE,
> -};
> -
> -#define DEFINE_QNODE(_name, _id, _buswidth, ...)			\
> +#define DEFINE_QNODE(_name, _buswidth)					\
>  	static const struct qcom_osm_l3_node _name = {			\
>  		.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_master, 16);
> +DEFINE_QNODE(osm_l3_slave, 16);
>  
>  static const 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);
> +DEFINE_QNODE(epss_l3_slave, 32);
>  
>  static const struct qcom_osm_l3_node * const epss_l3_nodes[] = {
>  	[MASTER_EPSS_L3_APPS] = &epss_l3_master,
> @@ -242,10 +229,9 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
>  
>  	icc_provider_init(provider);
>  
> +	/* Create nodes */
>  	for (i = 0; i < num_nodes; i++) {
> -		size_t j;
> -
> -		node = icc_node_create(qnodes[i]->id);
> +		node = icc_node_create(ALLOC_DYN_ID);

Rather than passing a "magic value" as argument to alter how node
creation happens, and in particular if we can avoid sprinkling
ALLOC_DYN_ID across the provider.

How about creating a new function icc_node_create_dyn() and hide the
ALLOC_DYN_ID within the interconnect framework?


Other than that I really like how this is turning out.

Regards,
Bjorn

>  		if (IS_ERR(node)) {
>  			ret = PTR_ERR(node);
>  			goto err;
> @@ -256,12 +242,12 @@ static int qcom_osm_l3_probe(struct platform_device *pdev)
>  		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]);
> -
>  		data->nodes[i] = node;
>  	}
>  
> +	/* Create link */
> +	icc_link_create(data->nodes[MASTER_OSM_L3_APPS], data->nodes[SLAVE_OSM_L3]->id);
> +
>  	ret = icc_provider_register(provider);
>  	if (ret)
>  		goto err;
> @@ -278,6 +264,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.43.0
>