[PATCH V5 0/4] Add EPSS L3 provider support on SA8775P SoC

[PATCH V5 0/4] Add EPSS L3 provider support on SA8775P SoC

[Raviteja Laggyshetty]

Add Epoch Subsystem (EPSS) L3 provider support on SA8775P SoCs.

Changes since v4:
 - Added generic compatible "qcom,epss-l3-perf" changes.
 - Split the driver code into two patches, with one containing multidev
   support and other containing the compatible additions.

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.

Raviteja Laggyshetty (4):
  dt-bindings: interconnect: Add EPSS L3 compatible for SA8775P
  arm64: dts: qcom: sa8775p: add EPSS l3 interconnect provider
  interconnect: qcom: Add EPSS L3 support on SA8775P
  interconnect: qcom: osm-l3: Add epss compatibles for SA8775P SoC

 .../bindings/interconnect/qcom,osm-l3.yaml    |  4 +
 arch/arm64/boot/dts/qcom/sa8775p.dtsi         | 19 ++++
 drivers/interconnect/qcom/osm-l3.c            | 87 ++++++++++++++-----
 3 files changed, 88 insertions(+), 22 deletions(-)

-- 
2.39.2

[PATCH V5 1/4] dt-bindings: interconnect: Add EPSS L3 compatible for SA8775P

[Raviteja Laggyshetty]

Add Epoch Subsystem (EPSS) L3 interconnect provider binding on
SA8775P SoCs.

Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
---
 .../devicetree/bindings/interconnect/qcom,osm-l3.yaml         | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
index 21dae0b92819..042ca44c32ec 100644
--- a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
+++ b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
@@ -34,6 +34,10 @@ properties:
               - qcom,sm8250-epss-l3
               - qcom,sm8350-epss-l3
           - const: qcom,epss-l3
+      - items:
+          - enum:
+              - qcom,sa8775p-epss-l3
+          - const: qcom,epss-l3-perf
 
   reg:
     maxItems: 1
-- 
2.39.2

[PATCH V5 2/4] arm64: dts: qcom: sa8775p: add EPSS l3 interconnect provider

[Raviteja Laggyshetty]

Add Epoch Subsystem (EPSS) L3 interconnect provider node on SA8775P
SoCs.

Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sa8775p.dtsi | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sa8775p.dtsi b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
index 9f315a51a7c1..dd7207eb3616 100644
--- a/arch/arm64/boot/dts/qcom/sa8775p.dtsi
+++ b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
@@ -10,6 +10,7 @@
 #include <dt-bindings/clock/qcom,sa8775p-gcc.h>
 #include <dt-bindings/clock/qcom,sa8775p-gpucc.h>
 #include <dt-bindings/dma/qcom-gpi.h>
+#include <dt-bindings/interconnect/qcom,osm-l3.h>
 #include <dt-bindings/interconnect/qcom,sa8775p-rpmh.h>
 #include <dt-bindings/mailbox/qcom-ipcc.h>
 #include <dt-bindings/firmware/qcom,scm.h>
@@ -4282,6 +4283,15 @@ rpmhpd_opp_turbo_l1: opp-9 {
 			};
 		};
 
+		epss_l3_cl0: interconnect@18590000 {
+			compatible = "qcom,sa8775p-epss-l3",
+				     "qcom,epss-l3-perf";
+			reg = <0x0 0x18590000 0x0 0x1000>;
+			clocks = <&rpmhcc RPMH_CXO_CLK>, <&gcc GCC_GPLL0>;
+			clock-names = "xo", "alternate";
+			#interconnect-cells = <1>;
+		};
+
 		cpufreq_hw: cpufreq@18591000 {
 			compatible = "qcom,sa8775p-cpufreq-epss",
 				     "qcom,cpufreq-epss";
@@ -4295,6 +4305,15 @@ cpufreq_hw: cpufreq@18591000 {
 			#freq-domain-cells = <1>;
 		};
 
+		epss_l3_cl1: interconnect@18592000 {
+			compatible = "qcom,sa8775p-epss-l3",
+				     "qcom,epss-l3-perf";
+			reg = <0x0 0x18592000 0x0 0x1000>;
+			clocks = <&rpmhcc RPMH_CXO_CLK>, <&gcc GCC_GPLL0>;
+			clock-names = "xo", "alternate";
+			#interconnect-cells = <1>;
+		};
+
 		remoteproc_gpdsp0: remoteproc@20c00000 {
 			compatible = "qcom,sa8775p-gpdsp0-pas";
 			reg = <0x0 0x20c00000 0x0 0x10000>;
-- 
2.39.2

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

[Raviteja Laggyshetty]

Add Epoch Subsystem (EPSS) L3 interconnect provider on
SA8775P SoCs with multiple device support.

Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
---
 drivers/interconnect/qcom/osm-l3.c | 85 ++++++++++++++++++++++--------
 1 file changed, 63 insertions(+), 22 deletions(-)

diff --git a/drivers/interconnect/qcom/osm-l3.c b/drivers/interconnect/qcom/osm-l3.c
index 6a656ed44d49..a9405b7d251b 100644
--- a/drivers/interconnect/qcom/osm-l3.c
+++ b/drivers/interconnect/qcom/osm-l3.c
@@ -1,16 +1,19 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
  * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
  */
 
 #include <linux/args.h>
 #include <linux/bitfield.h>
 #include <linux/clk.h>
+#include <linux/idr.h>
 #include <linux/interconnect-provider.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/platform_device.h>
 
 #include <dt-bindings/interconnect/qcom,osm-l3.h>
@@ -34,9 +37,14 @@
 
 #define OSM_L3_MAX_LINKS		1
 
+#define OSM_L3_NODE_ID_START		10000
+#define OSM_NODE_NAME_SUFFIX_SIZE	10
+
 #define to_osm_l3_provider(_provider) \
 	container_of(_provider, struct qcom_osm_l3_icc_provider, provider)
 
+static DEFINE_IDA(osm_l3_id);
+
 struct qcom_osm_l3_icc_provider {
 	void __iomem *base;
 	unsigned int max_state;
@@ -55,46 +63,40 @@ struct qcom_osm_l3_icc_provider {
  */
 struct qcom_osm_l3_node {
 	const char *name;
-	u16 links[OSM_L3_MAX_LINKS];
+	const char *links[OSM_L3_MAX_LINKS];
 	u16 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 = {			\
 		.name = #_name,						\
-		.id = _id,						\
 		.buswidth = _buswidth,					\
 		.num_links = COUNT_ARGS(__VA_ARGS__),			\
-		.links = { __VA_ARGS__ },				\
+		__VA_OPT__(.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, 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[] = {
 	[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;
+
 		/* 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++) {
+			u16 link_node_id = get_node_id_by_name(qnodes[i]->links[j], desc);
+
+			if (link_node_id)
+				icc_link_create(node, link_node_id);
+			else
+				goto err;
+		}
 
 		data->nodes[i] = node;
 	}
-- 
2.39.2

[PATCH V5 4/4] interconnect: qcom: osm-l3: Add epss compatibles for SA8775P SoC

[Raviteja Laggyshetty]

The EPSS instance in SA8775P uses PERF_STATE register instead of
REG_L3_VOTE to scale L3 clocks.
Along with SoC specific compatible, add new generic compatible
"qcom,epss-l3-perf" for PERF_STATE register based L3 scaling.

Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
---
 drivers/interconnect/qcom/osm-l3.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/interconnect/qcom/osm-l3.c b/drivers/interconnect/qcom/osm-l3.c
index a9405b7d251b..285afaa1f61e 100644
--- a/drivers/interconnect/qcom/osm-l3.c
+++ b/drivers/interconnect/qcom/osm-l3.c
@@ -318,6 +318,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,epss-l3-perf", .data = &epss_l3_perf_state },
 	{ .compatible = "qcom,osm-l3", .data = &osm_l3 },
 	{ .compatible = "qcom,sc7180-osm-l3", .data = &osm_l3 },
 	{ .compatible = "qcom,sc7280-epss-l3", .data = &epss_l3_perf_state },
@@ -325,6 +326,7 @@ static const struct of_device_id osm_l3_of_match[] = {
 	{ .compatible = "qcom,sm8150-osm-l3", .data = &osm_l3 },
 	{ .compatible = "qcom,sc8180x-osm-l3", .data = &osm_l3 },
 	{ .compatible = "qcom,sm8250-epss-l3", .data = &epss_l3_perf_state },
+	{ .compatible = "qcom,sa8775p-epss-l3", .data = &epss_l3_perf_state },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, osm_l3_of_match);
-- 
2.39.2

Re: [PATCH V5 4/4] interconnect: qcom: osm-l3: Add epss compatibles for SA8775P SoC

[Krzysztof Kozlowski]

On 21/11/2024 12:30, Raviteja Laggyshetty wrote:
> The EPSS instance in SA8775P uses PERF_STATE register instead of
> REG_L3_VOTE to scale L3 clocks.
> Along with SoC specific compatible, add new generic compatible
> "qcom,epss-l3-perf" for PERF_STATE register based L3 scaling.
> 
> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
> ---
>  drivers/interconnect/qcom/osm-l3.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/interconnect/qcom/osm-l3.c b/drivers/interconnect/qcom/osm-l3.c
> index a9405b7d251b..285afaa1f61e 100644
> --- a/drivers/interconnect/qcom/osm-l3.c
> +++ b/drivers/interconnect/qcom/osm-l3.c
> @@ -318,6 +318,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,epss-l3-perf", .data = &epss_l3_perf_state },


Hm? Why?

>  	{ .compatible = "qcom,osm-l3", .data = &osm_l3 },
>  	{ .compatible = "qcom,sc7180-osm-l3", .data = &osm_l3 },
>  	{ .compatible = "qcom,sc7280-epss-l3", .data = &epss_l3_perf_state },
> @@ -325,6 +326,7 @@ static const struct of_device_id osm_l3_of_match[] = {
>  	{ .compatible = "qcom,sm8150-osm-l3", .data = &osm_l3 },
>  	{ .compatible = "qcom,sc8180x-osm-l3", .data = &osm_l3 },
>  	{ .compatible = "qcom,sm8250-epss-l3", .data = &epss_l3_perf_state },
> +	{ .compatible = "qcom,sa8775p-epss-l3", .data = &epss_l3_perf_state },


So this is compatible with sm8250. Use that one. Don't grow this table
needlessly.


Best regards,
Krzysztof

Re: [PATCH V5 1/4] dt-bindings: interconnect: Add EPSS L3 compatible for SA8775P

[Krzysztof Kozlowski]

On 21/11/2024 12:30, Raviteja Laggyshetty wrote:
> Add Epoch Subsystem (EPSS) L3 interconnect provider binding on
> SA8775P SoCs.

This we see from the diff. Explain the hardware, why adding epps-l3-perf.

> 
> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
> ---
>  .../devicetree/bindings/interconnect/qcom,osm-l3.yaml         | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
> index 21dae0b92819..042ca44c32ec 100644
> --- a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
> +++ b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
> @@ -34,6 +34,10 @@ properties:
>                - qcom,sm8250-epss-l3
>                - qcom,sm8350-epss-l3
>            - const: qcom,epss-l3
> +      - items:
> +          - enum:
> +              - qcom,sa8775p-epss-l3
> +          - const: qcom,epss-l3-perf

I don't understand this change in context of driver. These are the same.
Isn't this compatible with sm8250?

Sorry, this is all (binding plus driver) quite confusing.

Best regards,
Krzysztof

Re: [PATCH V5 2/4] arm64: dts: qcom: sa8775p: add EPSS l3 interconnect provider

[Krzysztof Kozlowski]

On 21/11/2024 12:30, Raviteja Laggyshetty wrote:
> Add Epoch Subsystem (EPSS) L3 interconnect provider node on SA8775P
> SoCs.
> 
> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sa8775p.dtsi | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8775p.dtsi b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
> index 9f315a51a7c1..dd7207eb3616 100644
> --- a/arch/arm64/boot/dts/qcom/sa8775p.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
> @@ -10,6 +10,7 @@
>  #include <dt-bindings/clock/qcom,sa8775p-gcc.h>
>  #include <dt-bindings/clock/qcom,sa8775p-gpucc.h>
>  #include <dt-bindings/dma/qcom-gpi.h>
> +#include <dt-bindings/interconnect/qcom,osm-l3.h>
>  #include <dt-bindings/interconnect/qcom,sa8775p-rpmh.h>
>  #include <dt-bindings/mailbox/qcom-ipcc.h>
>  #include <dt-bindings/firmware/qcom,scm.h>
> @@ -4282,6 +4283,15 @@ rpmhpd_opp_turbo_l1: opp-9 {
>  			};
>  		};
>  
> +		epss_l3_cl0: interconnect@18590000 {


Drop unused label.

BTW, DTS is never before the driver. It suggests dependency. If you have
dependency, it's a NAK.

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

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

Re: [PATCH V5 1/4] dt-bindings: interconnect: Add EPSS L3 compatible for SA8775P

[Raviteja Laggyshetty]

On 11/21/2024 5:23 PM, Krzysztof Kozlowski wrote:
> On 21/11/2024 12:30, Raviteja Laggyshetty wrote:
>> Add Epoch Subsystem (EPSS) L3 interconnect provider binding on
>> SA8775P SoCs.
> 
> This we see from the diff. Explain the hardware, why adding epps-l3-perf.
> 
The EPSS instance in SA8775P uses PERF_STATE register instead of REG_L3_VOTE to scale L3 clocks.Along with SoC specific compatible, add new generic compatible "qcom,epss-l3-perf" for PERF_STATE register based L3 scaling.

>>
>> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
>> ---
>>  .../devicetree/bindings/interconnect/qcom,osm-l3.yaml         | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
>> index 21dae0b92819..042ca44c32ec 100644
>> --- a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
>> +++ b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
>> @@ -34,6 +34,10 @@ properties:
>>                - qcom,sm8250-epss-l3
>>                - qcom,sm8350-epss-l3
>>            - const: qcom,epss-l3
>> +      - items:
>> +          - enum:
>> +              - qcom,sa8775p-epss-l3
>> +          - const: qcom,epss-l3-perf
> 
> I don't understand this change in context of driver. These are the same.
> Isn't this compatible with sm8250?
> 

The intention for adding "qcom,epss-l3-perf" generic compatible is to use it for the chipsets which use perf state register for l3 scaling.
Using generic compatible avoids the need for adding chipset specific compatible in match table.
But received comment from konrad to add both SoC-specific and generic compatibles.
Dmitry has suggested to update generic comaptibles for sc7280 and sm8250 SoCs, which makes use of perf state registers. 
It will be done as separate patch series.


> Sorry, this is all (binding plus driver) quite confusing.
> 
> Best regards,
> Krzysztof

Re: [PATCH V5 2/4] arm64: dts: qcom: sa8775p: add EPSS l3 interconnect provider

[Raviteja Laggyshetty]

On 11/21/2024 5:24 PM, Krzysztof Kozlowski wrote:
> On 21/11/2024 12:30, Raviteja Laggyshetty wrote:
>> Add Epoch Subsystem (EPSS) L3 interconnect provider node on SA8775P
>> SoCs.
>>
>> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
>> ---
>>  arch/arm64/boot/dts/qcom/sa8775p.dtsi | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sa8775p.dtsi b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
>> index 9f315a51a7c1..dd7207eb3616 100644
>> --- a/arch/arm64/boot/dts/qcom/sa8775p.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
>> @@ -10,6 +10,7 @@
>>  #include <dt-bindings/clock/qcom,sa8775p-gcc.h>
>>  #include <dt-bindings/clock/qcom,sa8775p-gpucc.h>
>>  #include <dt-bindings/dma/qcom-gpi.h>
>> +#include <dt-bindings/interconnect/qcom,osm-l3.h>
>>  #include <dt-bindings/interconnect/qcom,sa8775p-rpmh.h>
>>  #include <dt-bindings/mailbox/qcom-ipcc.h>
>>  #include <dt-bindings/firmware/qcom,scm.h>
>> @@ -4282,6 +4283,15 @@ rpmhpd_opp_turbo_l1: opp-9 {
>>  			};
>>  		};
>>  
>> +		epss_l3_cl0: interconnect@18590000 {
> 
> 
> Drop unused label.
> 
This will be used by DCVS driver for getting the interconnect path.

> BTW, DTS is never before the driver. It suggests dependency. If you have
> dependency, it's a NAK.
> 

There is no dependency, I will rearrange the patches in next revision.

> Best regards,
> Krzysztof

Re: [PATCH V5 1/4] dt-bindings: interconnect: Add EPSS L3 compatible for SA8775P

[Krzysztof Kozlowski]

On 21/11/2024 18:43, Raviteja Laggyshetty wrote:
> 
> 
> On 11/21/2024 5:23 PM, Krzysztof Kozlowski wrote:
>> On 21/11/2024 12:30, Raviteja Laggyshetty wrote:
>>> Add Epoch Subsystem (EPSS) L3 interconnect provider binding on
>>> SA8775P SoCs.
>>
>> This we see from the diff. Explain the hardware, why adding epps-l3-perf.
>>
> The EPSS instance in SA8775P uses PERF_STATE register instead of REG_L3_VOTE to scale L3 clocks.Along with SoC specific compatible, add new generic compatible "qcom,epss-l3-perf" for PERF_STATE register based L3 scaling.

Pasting the same replies as you pasted to others won't solve the
problem. Solve the problem - fix the commit msg.

> 
>>>
>>> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
>>> ---
>>>  .../devicetree/bindings/interconnect/qcom,osm-l3.yaml         | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
>>> index 21dae0b92819..042ca44c32ec 100644
>>> --- a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
>>> +++ b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
>>> @@ -34,6 +34,10 @@ properties:
>>>                - qcom,sm8250-epss-l3
>>>                - qcom,sm8350-epss-l3
>>>            - const: qcom,epss-l3
>>> +      - items:
>>> +          - enum:
>>> +              - qcom,sa8775p-epss-l3
>>> +          - const: qcom,epss-l3-perf
>>
>> I don't understand this change in context of driver. These are the same.
>> Isn't this compatible with sm8250?
>>
> 
> The intention for adding "qcom,epss-l3-perf" generic compatible is to use it for the chipsets which use perf state register for l3 scaling.
> Using generic compatible avoids the need for adding chipset specific compatible in match table.


Not true, specific compatibles used as fallback do the same and is a
preferred way.


> But received comment from konrad to add both SoC-specific and generic compatibles.

I went through the history and don't see anything like that. Point to
the specific email please, if you disagree.

> Dmitry has suggested to update generic comaptibles for sc7280 and sm8250 SoCs, which makes use of perf state registers. 

OK

> It will be done as separate patch series.

No. I expect to see full, correct picture, not half baked patches which
contradict what is in current code.


Best regards,
Krzysztof

Re: [PATCH V5 2/4] arm64: dts: qcom: sa8775p: add EPSS l3 interconnect provider

[Krzysztof Kozlowski]

On 21/11/2024 18:49, Raviteja Laggyshetty wrote:
> 
> 
> On 11/21/2024 5:24 PM, Krzysztof Kozlowski wrote:
>> On 21/11/2024 12:30, Raviteja Laggyshetty wrote:
>>> Add Epoch Subsystem (EPSS) L3 interconnect provider node on SA8775P
>>> SoCs.
>>>
>>> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
>>> ---
>>>  arch/arm64/boot/dts/qcom/sa8775p.dtsi | 19 +++++++++++++++++++
>>>  1 file changed, 19 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sa8775p.dtsi b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
>>> index 9f315a51a7c1..dd7207eb3616 100644
>>> --- a/arch/arm64/boot/dts/qcom/sa8775p.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
>>> @@ -10,6 +10,7 @@
>>>  #include <dt-bindings/clock/qcom,sa8775p-gcc.h>
>>>  #include <dt-bindings/clock/qcom,sa8775p-gpucc.h>
>>>  #include <dt-bindings/dma/qcom-gpi.h>
>>> +#include <dt-bindings/interconnect/qcom,osm-l3.h>
>>>  #include <dt-bindings/interconnect/qcom,sa8775p-rpmh.h>
>>>  #include <dt-bindings/mailbox/qcom-ipcc.h>
>>>  #include <dt-bindings/firmware/qcom,scm.h>
>>> @@ -4282,6 +4283,15 @@ rpmhpd_opp_turbo_l1: opp-9 {
>>>  			};
>>>  		};
>>>  
>>> +		epss_l3_cl0: interconnect@18590000 {
>>
>>
>> Drop unused label.
>>
> This will be used by DCVS driver for getting the interconnect path.

Fine then.


Best regards,
Krzysztof

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

[Raviteja Laggyshetty]

On 11/21/2024 5:28 PM, Krzysztof Kozlowski wrote:
> 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).

EPSS L3 on SA8775P has two instances and this requires creation of two device nodes in devicetree.
As Interconnect framework requires a unique node id, each device node needs to have different compatible and data.
To overcome the need of having two different compatibles and data, driver code has been modified to acquire unique node id from IDA 
and the node name is made dynamic (nodename@address).
Updating node id and node name is not possible with const.
> 
> 
> 
>>  	[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

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

[Dmitry Baryshkov]

On Thu, Nov 21, 2024 at 11:33:04PM +0530, Raviteja Laggyshetty wrote:
> 
> 
> On 11/21/2024 5:28 PM, Krzysztof Kozlowski wrote:
> > 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).
> 
> EPSS L3 on SA8775P has two instances and this requires creation of two device nodes in devicetree.
> As Interconnect framework requires a unique node id, each device node needs to have different compatible and data.
> To overcome the need of having two different compatibles and data, driver code has been modified to acquire unique node id from IDA 
> and the node name is made dynamic (nodename@address).
> Updating node id and node name is not possible with const.

Has this been described in the commit message? No. Have you had similar
questions since v1? Yes. What does that tell us?

Also, while we are at it. Please fix your email client settings to wrap
message body text on some sensible (72-75) width.

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

-- 
With best wishes
Dmitry

Re: [PATCH V5 1/4] dt-bindings: interconnect: Add EPSS L3 compatible for SA8775P

[Raviteja Laggyshetty]

On 11/21/2024 11:20 PM, Krzysztof Kozlowski wrote:
> On 21/11/2024 18:43, Raviteja Laggyshetty wrote:
>>
>>
>> On 11/21/2024 5:23 PM, Krzysztof Kozlowski wrote:
>>> On 21/11/2024 12:30, Raviteja Laggyshetty wrote:
>>>> Add Epoch Subsystem (EPSS) L3 interconnect provider binding on
>>>> SA8775P SoCs.
>>>
>>> This we see from the diff. Explain the hardware, why adding epps-l3-perf.
>>>
>> The EPSS instance in SA8775P uses PERF_STATE register instead of REG_L3_VOTE to scale L3 clocks.Along with SoC specific compatible, add new generic compatible "qcom,epss-l3-perf" for PERF_STATE register based L3 scaling.
> 
> Pasting the same replies as you pasted to others won't solve the
> problem. Solve the problem - fix the commit msg.
> 
>>
>>>>
>>>> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
>>>> ---
>>>>  .../devicetree/bindings/interconnect/qcom,osm-l3.yaml         | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
>>>> index 21dae0b92819..042ca44c32ec 100644
>>>> --- a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
>>>> +++ b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
>>>> @@ -34,6 +34,10 @@ properties:
>>>>                - qcom,sm8250-epss-l3
>>>>                - qcom,sm8350-epss-l3
>>>>            - const: qcom,epss-l3
>>>> +      - items:
>>>> +          - enum:
>>>> +              - qcom,sa8775p-epss-l3
>>>> +          - const: qcom,epss-l3-perf
>>>
>>> I don't understand this change in context of driver. These are the same.
>>> Isn't this compatible with sm8250?
>>>
>>
>> The intention for adding "qcom,epss-l3-perf" generic compatible is to use it for the chipsets which use perf state register for l3 scaling.
>> Using generic compatible avoids the need for adding chipset specific compatible in match table.
> 
> 
> Not true, specific compatibles used as fallback do the same and is a
> preferred way.
>

Will make use of sm8250 compatibles for sa8775p in the next patch revision.
 
> 
>> But received comment from konrad to add both SoC-specific and generic compatibles.
> 
> I went through the history and don't see anything like that. Point to
> the specific email please, if you disagree.
> 
https://patchwork.kernel.org/project/linux-pm/patch/20241026123058.28258-2-quic_rlaggysh@quicinc.com/#26104591


>> Dmitry has suggested to update generic comaptibles for sc7280 and sm8250 SoCs, which makes use of perf state registers. 
> 
> OK
> 
>> It will be done as separate patch series.
> 
> No. I expect to see full, correct picture, not half baked patches which
> contradict what is in current code.
> 
>
Ok, I will update the generic compatibles for sm8250, sc7280 SoCs in the 
next patch revision and will include them along with sa8775p dt patch.
 
> Best regards,
> Krzysztof

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

[Raviteja Laggyshetty]

On 11/22/2024 3:44 AM, Dmitry Baryshkov wrote:
> On Thu, Nov 21, 2024 at 11:33:04PM +0530, Raviteja Laggyshetty wrote:
>>
>>
>> On 11/21/2024 5:28 PM, Krzysztof Kozlowski wrote:
>>> 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).
>>
>> EPSS L3 on SA8775P has two instances and this requires creation of two device nodes in devicetree.
>> As Interconnect framework requires a unique node id, each device node needs to have different compatible and data.
>> To overcome the need of having two different compatibles and data, driver code has been modified to acquire unique node id from IDA 
>> and the node name is made dynamic (nodename@address).
>> Updating node id and node name is not possible with const.
> 
> Has this been described in the commit message? No. Have you had similar
> questions since v1? Yes. What does that tell us?

I will update the commit message in the next patch revision.
> 
> Also, while we are at it. Please fix your email client settings to wrap
> message body text on some sensible (72-75) width.

Thanks for suggesting!
> 
>>>
>>>
>>>
>>>>  	[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
>>
> 

Re: [PATCH V5 4/4] interconnect: qcom: osm-l3: Add epss compatibles for SA8775P SoC

[Raviteja Laggyshetty]

On 11/21/2024 5:21 PM, Krzysztof Kozlowski wrote:
> On 21/11/2024 12:30, Raviteja Laggyshetty wrote:
>> The EPSS instance in SA8775P uses PERF_STATE register instead of
>> REG_L3_VOTE to scale L3 clocks.
>> Along with SoC specific compatible, add new generic compatible
>> "qcom,epss-l3-perf" for PERF_STATE register based L3 scaling.
>>
>> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
>> ---
>>  drivers/interconnect/qcom/osm-l3.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/interconnect/qcom/osm-l3.c b/drivers/interconnect/qcom/osm-l3.c
>> index a9405b7d251b..285afaa1f61e 100644
>> --- a/drivers/interconnect/qcom/osm-l3.c
>> +++ b/drivers/interconnect/qcom/osm-l3.c
>> @@ -318,6 +318,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,epss-l3-perf", .data = &epss_l3_perf_state },
> 
> 
> Hm? Why?
> 
>>  	{ .compatible = "qcom,osm-l3", .data = &osm_l3 },
>>  	{ .compatible = "qcom,sc7180-osm-l3", .data = &osm_l3 },
>>  	{ .compatible = "qcom,sc7280-epss-l3", .data = &epss_l3_perf_state },
>> @@ -325,6 +326,7 @@ static const struct of_device_id osm_l3_of_match[] = {
>>  	{ .compatible = "qcom,sm8150-osm-l3", .data = &osm_l3 },
>>  	{ .compatible = "qcom,sc8180x-osm-l3", .data = &osm_l3 },
>>  	{ .compatible = "qcom,sm8250-epss-l3", .data = &epss_l3_perf_state },
>> +	{ .compatible = "qcom,sa8775p-epss-l3", .data = &epss_l3_perf_state },
> 
> 
> So this is compatible with sm8250. Use that one. Don't grow this table
> needlessly.
> 
Will make use of sm8250 compatible in next patch revision.
> 
> Best regards,
> Krzysztof

Re: [PATCH V5 1/4] dt-bindings: interconnect: Add EPSS L3 compatible for SA8775P

[Krzysztof Kozlowski]

On Fri, Nov 22, 2024 at 12:46:37PM +0530, Raviteja Laggyshetty wrote:
> > 
> >> But received comment from konrad to add both SoC-specific and generic compatibles.
> > 
> > I went through the history and don't see anything like that. Point to
> > the specific email please, if you disagree.
> > 
> https://patchwork.kernel.org/project/linux-pm/patch/20241026123058.28258-2-quic_rlaggysh@quicinc.com/#26104591

I read this message and it does not say to use generic compatible.

Best regards,
Krzysztof

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

[Konrad Dybcio]

On 21.11.2024 7:03 PM, Raviteja Laggyshetty wrote:
> 
> 
> On 11/21/2024 5:28 PM, Krzysztof Kozlowski wrote:
>> 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).
> 
> EPSS L3 on SA8775P has two instances and this requires creation of two device nodes in devicetree.
> As Interconnect framework requires a unique node id, each device node needs to have different compatible and data.
> To overcome the need of having two different compatibles and data, driver code has been modified to acquire unique node id from IDA 
> and the node name is made dynamic (nodename@address).
> Updating node id and node name is not possible with const.

I think it would be better to just fix the interconnect API to do it
internally

Konrad