mbox series

[V7,0/5] Add EPSS L3 provider support on SA8775P SoC

Message ID 20250111161429.51-1-quic_rlaggysh@quicinc.com
Headers show
Series Add EPSS L3 provider support on SA8775P SoC | expand

Message

Raviteja Laggyshetty Jan. 11, 2025, 4:14 p.m. UTC 
Add Epoch Subsystem (EPSS) L3 provider support on SA8775P SoCs.

Current interconnect framework is based on static IDs for creating node
and registering with framework. This becomes a limitation for topologies
where there are multiple instances of same interconnect provider. Add
icc_node_create_alloc_id() API to create icc node with dynamic id, this
will help to overcome the dependency on static IDs.

Change since v6:
 - Added icc_node_create_alloc_id() API to dynamically allocate ID while 
   creating the node. Replaced the IDA (ID allocator) with
   icc_node_create_alloc_id() API to allocate node IDs dynamically.
 - Removed qcom,epss-l3-perf generic compatible as per the comment.
 - Added L3 ICC handles for CPU0 and CPU4 in DT, as per Bjorn comment.
   Link to comment:
   https://lore.kernel.org/lkml/ww3t3tu7p36qzlhcetaxif2xzrpgslydmuqo3fqvisbuar4bjh@qc2u43dck3qi/

Change since v5:
 - Reused qcom,sm8250-epss-l3 compatible for sa8775p SoC.
 - Rearranged the patches, moved dt changes to end of series.
 - Updated the commit text.

Changes since v4:
 - Added generic compatible "qcom,epss-l3-perf" and split the driver
   changes accordingly.

Changes since v3:
 - Removed epss-l3-perf generic compatible changes. These will be posted
   as separate patch until then SoC specific compatible will be used for
   probing.

Changes since v2:
 - Updated the commit text to reflect the reason for code change.
 - Added SoC-specific and generic compatible to driver match table.

Changes since v1:
 - Removed the usage of static IDs and implemented dynamic ID assignment
   for icc nodes using IDA.
 - Removed separate compatibles for cl0 and cl1. Both cl0 and cl1
   devices use the same compatible.
 - Added new generic compatible for epss-l3-perf.

Jagadeesh Kona (1):
  arm64: dts: qcom: sa8775p: Add CPU OPP tables to scale DDR/L3

Raviteja Laggyshetty (4):
  interconnect: core: Add dynamic id allocation support
  interconnect: qcom: Add multidev EPSS L3 support
  dt-bindings: interconnect: Add EPSS L3 compatible for SA8775P
  arm64: dts: qcom: sa8775p: add EPSS l3 interconnect provider

 .../bindings/interconnect/qcom,osm-l3.yaml    |   1 +
 arch/arm64/boot/dts/qcom/sa8775p.dtsi         | 229 ++++++++++++++++++
 drivers/interconnect/core.c                   |  32 +++
 drivers/interconnect/qcom/osm-l3.c            |  91 +++++--
 include/linux/interconnect-provider.h         |   6 +
 5 files changed, 335 insertions(+), 24 deletions(-)

Comments

Bjorn Andersson Jan. 11, 2025, 8:57 p.m. UTC | #1 
On Sat, Jan 11, 2025 at 04:14:25PM +0000, Raviteja Laggyshetty wrote:
> Current interconnect framework is based on static IDs for creating node
> and registering with framework. This becomes a limitation for topologies
> where there are multiple instances of same interconnect provider. Add
> icc_node_create_alloc_id() API to create icc node with dynamic id, this
> will help to overcome the dependency on static IDs.
> 
> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
> ---
>  drivers/interconnect/core.c           | 32 +++++++++++++++++++++++++++
>  include/linux/interconnect-provider.h |  6 +++++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
> index 9d5404a07e8a..0b7093eb51af 100644
> --- a/drivers/interconnect/core.c
> +++ b/drivers/interconnect/core.c
> @@ -858,6 +858,38 @@ struct icc_node *icc_node_create(int id)
>  }
>  EXPORT_SYMBOL_GPL(icc_node_create);
>  
> +/**
> + * icc_node_create_alloc_id() - create node and dynamically allocate id
> + * @start_id: min id to be allocated
> + *
> + * Return: icc_node pointer on success, or ERR_PTR() on error
> + */
> +struct icc_node *icc_node_create_alloc_id(int start_id)

By having clients pass in start_id, you distribute the decision of what
a "good number" is across multiple parts of the system (or you have
clients relying on getting [start_id, start_id + N) back).

Wouldn't it be better to hide that choice in one place (inside the icc
framework)?

Regards,
Bjorn

> +{
> +	struct icc_node *node;
> +	int id;
> +
> +	mutex_lock(&icc_lock);
> +
> +	node = kzalloc(sizeof(*node), GFP_KERNEL);
> +	if (!node)
> +		return ERR_PTR(-ENOMEM);
> +
> +	id = idr_alloc(&icc_idr, node, start_id, 0, GFP_KERNEL);
> +	if (id < 0) {
> +		WARN(1, "%s: couldn't get idr\n", __func__);
> +		kfree(node);
> +		node = ERR_PTR(id);
> +		goto out;
> +	}
> +	node->id = id;
> +out:
> +	mutex_unlock(&icc_lock);
> +
> +	return node;
> +}
> +EXPORT_SYMBOL_GPL(icc_node_create_alloc_id);
> +
>  /**
>   * icc_node_destroy() - destroy a node
>   * @id: node id
> diff --git a/include/linux/interconnect-provider.h b/include/linux/interconnect-provider.h
> index f5aef8784692..4fc7a5884374 100644
> --- a/include/linux/interconnect-provider.h
> +++ b/include/linux/interconnect-provider.h
> @@ -117,6 +117,7 @@ struct icc_node {
>  int icc_std_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
>  		      u32 peak_bw, u32 *agg_avg, u32 *agg_peak);
>  struct icc_node *icc_node_create(int id);
> +struct icc_node *icc_node_create_alloc_id(int start_id);
>  void icc_node_destroy(int id);
>  int icc_link_create(struct icc_node *node, const int dst_id);
>  void icc_node_add(struct icc_node *node, struct icc_provider *provider);
> @@ -141,6 +142,11 @@ static inline struct icc_node *icc_node_create(int id)
>  	return ERR_PTR(-ENOTSUPP);
>  }
>  
> +static inline struct icc_node *icc_node_create_alloc_id(int start_id)
> +{
> +	return ERR_PTR(-EOPNOTSUPP);
> +}
> +
>  static inline void icc_node_destroy(int id)
>  {
>  }
> -- 
> 2.39.2
>
Krzysztof Kozlowski Jan. 12, 2025, 9:31 a.m. UTC | #2 
On Sat, Jan 11, 2025 at 04:14:27PM +0000, Raviteja Laggyshetty wrote:
> Add Epoch Subsystem (EPSS) L3 interconnect provider binding on
> SA8775P SoCs.

1. And why is this not compatible with sm8250? There was lengthy
discussion and no outcome of it managed to get to commit msg. Really, so
we are going to repeat everything again and you will not get any acks.

You have entire commit msg to explain things but instead you repeat what
the patch does. We can read the diff for that.

2. Binding *ALWAYS* comes before the user.

> 
> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
> ---
>  Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
> index 21dae0b92819..94f7f283787a 100644
> --- a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
> +++ b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
> @@ -33,6 +33,7 @@ properties:
>                - qcom,sm6375-cpucp-l3
>                - qcom,sm8250-epss-l3
>                - qcom,sm8350-epss-l3
> +              - qcom,sa8775p-epss-l3
>            - const: qcom,epss-l3

Your driver suggests this is not really true - it is not compatible with
qcom,epss-l3. Maybe it is, maybe not, no clue, commit explains nothing.

Best regards,
Krzysztof
Dmitry Baryshkov Jan. 13, 2025, 8:14 a.m. UTC | #3 
On Sat, Jan 11, 2025 at 04:14:25PM +0000, Raviteja Laggyshetty wrote:
> Current interconnect framework is based on static IDs for creating node
> and registering with framework. This becomes a limitation for topologies
> where there are multiple instances of same interconnect provider. Add
> icc_node_create_alloc_id() API to create icc node with dynamic id, this
> will help to overcome the dependency on static IDs.

This doesn't overcome the dependency on static ID. Drivers still have to
manually lookup the resulting ID and use it to link the nodes. Instead
ICC framework should be providing a completely dynamic solution:
- icc_node_create() should get a completely dynamic counterpart. Use
  e.g. 1000000 as a dynamic start ID.
- icc_link_create() shold get a counterpart which can create a link
  between two icc_node instances directly, without an additional lookup.

You can check if your implementation is correct if you can refactor
existing ICC drivers (e.g. icc-clk and/or icc-rpm to drop ID arrays
completely).

> 
> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
> ---
>  drivers/interconnect/core.c           | 32 +++++++++++++++++++++++++++
>  include/linux/interconnect-provider.h |  6 +++++
>  2 files changed, 38 insertions(+)
>
Raviteja Laggyshetty Jan. 23, 2025, 10:35 a.m. UTC | #4 
On 1/12/2025 2:27 AM, Bjorn Andersson wrote:
> On Sat, Jan 11, 2025 at 04:14:25PM +0000, Raviteja Laggyshetty wrote:
>> Current interconnect framework is based on static IDs for creating node
>> and registering with framework. This becomes a limitation for topologies
>> where there are multiple instances of same interconnect provider. Add
>> icc_node_create_alloc_id() API to create icc node with dynamic id, this
>> will help to overcome the dependency on static IDs.
>>
>> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
>> ---
>>  drivers/interconnect/core.c           | 32 +++++++++++++++++++++++++++
>>  include/linux/interconnect-provider.h |  6 +++++
>>  2 files changed, 38 insertions(+)
>>
>> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
>> index 9d5404a07e8a..0b7093eb51af 100644
>> --- a/drivers/interconnect/core.c
>> +++ b/drivers/interconnect/core.c
>> @@ -858,6 +858,38 @@ struct icc_node *icc_node_create(int id)
>>  }
>>  EXPORT_SYMBOL_GPL(icc_node_create);
>>  
>> +/**
>> + * icc_node_create_alloc_id() - create node and dynamically allocate id
>> + * @start_id: min id to be allocated
>> + *
>> + * Return: icc_node pointer on success, or ERR_PTR() on error
>> + */
>> +struct icc_node *icc_node_create_alloc_id(int start_id)
> 
> By having clients pass in start_id, you distribute the decision of what
> a "good number" is across multiple parts of the system (or you have
> clients relying on getting [start_id, start_id + N) back).
> 
> Wouldn't it be better to hide that choice in one place (inside the icc
> framework)?
> 

Yes, inline to Dmitry's suggestion I will be moving the start_id to
framework and all dynamic allocations will start from 10000.

> Regards,
> Bjorn
> 
>> +{
>> +	struct icc_node *node;
>> +	int id;
>> +
>> +	mutex_lock(&icc_lock);
>> +
>> +	node = kzalloc(sizeof(*node), GFP_KERNEL);
>> +	if (!node)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	id = idr_alloc(&icc_idr, node, start_id, 0, GFP_KERNEL);
>> +	if (id < 0) {
>> +		WARN(1, "%s: couldn't get idr\n", __func__);
>> +		kfree(node);
>> +		node = ERR_PTR(id);
>> +		goto out;
>> +	}
>> +	node->id = id;
>> +out:
>> +	mutex_unlock(&icc_lock);
>> +
>> +	return node;
>> +}
>> +EXPORT_SYMBOL_GPL(icc_node_create_alloc_id);
>> +
>>  /**
>>   * icc_node_destroy() - destroy a node
>>   * @id: node id
>> diff --git a/include/linux/interconnect-provider.h b/include/linux/interconnect-provider.h
>> index f5aef8784692..4fc7a5884374 100644
>> --- a/include/linux/interconnect-provider.h
>> +++ b/include/linux/interconnect-provider.h
>> @@ -117,6 +117,7 @@ struct icc_node {
>>  int icc_std_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
>>  		      u32 peak_bw, u32 *agg_avg, u32 *agg_peak);
>>  struct icc_node *icc_node_create(int id);
>> +struct icc_node *icc_node_create_alloc_id(int start_id);
>>  void icc_node_destroy(int id);
>>  int icc_link_create(struct icc_node *node, const int dst_id);
>>  void icc_node_add(struct icc_node *node, struct icc_provider *provider);
>> @@ -141,6 +142,11 @@ static inline struct icc_node *icc_node_create(int id)
>>  	return ERR_PTR(-ENOTSUPP);
>>  }
>>  
>> +static inline struct icc_node *icc_node_create_alloc_id(int start_id)
>> +{
>> +	return ERR_PTR(-EOPNOTSUPP);
>> +}
>> +
>>  static inline void icc_node_destroy(int id)
>>  {
>>  }
>> -- 
>> 2.39.2
>>
Raviteja Laggyshetty Jan. 23, 2025, 10:47 a.m. UTC | #5 
On 1/13/2025 1:44 PM, Dmitry Baryshkov wrote:
> On Sat, Jan 11, 2025 at 04:14:25PM +0000, Raviteja Laggyshetty wrote:
>> Current interconnect framework is based on static IDs for creating node
>> and registering with framework. This becomes a limitation for topologies
>> where there are multiple instances of same interconnect provider. Add
>> icc_node_create_alloc_id() API to create icc node with dynamic id, this
>> will help to overcome the dependency on static IDs.
> 
> This doesn't overcome the dependency on static ID. Drivers still have to
> manually lookup the resulting ID and use it to link the nodes. Instead
> ICC framework should be providing a completely dynamic solution:
> - icc_node_create() should get a completely dynamic counterpart. Use
>   e.g. 1000000 as a dynamic start ID.
> - icc_link_create() shold get a counterpart which can create a link
>   between two icc_node instances directly, without an additional lookup.
> 

Agreed, with current implementation, still there is dependency on IDs
for linking the nodes.
Instead of relying on node names for the links, array of struct pointers
will be used, this will eliminate the need for ID lookup and avoids
extra loops.
Instead of providing counter part for the ICC framework APIs which
involves duplication of most of the code, I will modify the existing
icc_node_create, icc_link_create and icc_node_add APIs to support both
static and dynamic IDs.

> You can check if your implementation is correct if you can refactor
> existing ICC drivers (e.g. icc-clk and/or icc-rpm to drop ID arrays
> completely).
> 
ok, I will check the implementation on icc-rpmh driver for sa8775p SoC.
>>
>> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
>> ---
>>  drivers/interconnect/core.c           | 32 +++++++++++++++++++++++++++
>>  include/linux/interconnect-provider.h |  6 +++++
>>  2 files changed, 38 insertions(+)
>>
>