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

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

[Raviteja Laggyshetty]

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

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.

Raviteja Laggyshetty (4):
  interconnect: qcom: Add multidev EPSS L3 support
  interconnect: qcom: osm-l3: Add generic compatible for epss-l3-perf
  dt-bindings: interconnect: Add generic compatible qcom,epss-l3-perf
  arm64: dts: qcom: sa8775p: add EPSS l3 interconnect provider

 .../bindings/interconnect/qcom,osm-l3.yaml    |  7 +-
 arch/arm64/boot/dts/qcom/sa8775p.dtsi         | 19 ++++
 arch/arm64/boot/dts/qcom/sc7280.dtsi          |  2 +-
 arch/arm64/boot/dts/qcom/sm8250.dtsi          |  2 +-
 drivers/interconnect/qcom/osm-l3.c            | 86 ++++++++++++++-----
 5 files changed, 90 insertions(+), 26 deletions(-)

-- 
2.39.2

[PATCH V6 1/4] interconnect: qcom: Add multidev EPSS L3 support

[Raviteja Laggyshetty]

EPSS on SA8775P has two instances which requires creation of two device
nodes with different compatible and device data because of unique
icc node id and name limitation in interconnect framework.
Add multidevice support to osm-l3 code to get unique node id from IDA
and node name is made unique by appending node address.

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 V6 2/4] interconnect: qcom: osm-l3: Add generic compatible for epss-l3-perf

[Raviteja Laggyshetty]

The EPSS instance in SA8775P uses PERF_STATE register instead of
REG_L3_VOTE to scale L3 clocks. 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 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/interconnect/qcom/osm-l3.c b/drivers/interconnect/qcom/osm-l3.c
index a9405b7d251b..7937e7c16c71 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 },
-- 
2.39.2

[PATCH V6 3/4] dt-bindings: interconnect: Add generic compatible qcom,epss-l3-perf

[Raviteja Laggyshetty]

EPSS instance on sc7280, sm8250 SoCs, use PERF_STATE register instead of
REG_L3_VOTE to scale L3 clocks, hence adding a new generic compatible
"qcom,epss-l3-perf" for these targets.

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

diff --git a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
index 21dae0b92819..e24399ca110f 100644
--- a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
+++ b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
@@ -28,12 +28,15 @@ properties:
           - const: qcom,osm-l3
       - items:
           - enum:
-              - qcom,sc7280-epss-l3
               - qcom,sc8280xp-epss-l3
               - qcom,sm6375-cpucp-l3
-              - qcom,sm8250-epss-l3
               - qcom,sm8350-epss-l3
           - const: qcom,epss-l3
+      - items:
+          - enum:
+              - qcom,sc7280-epss-l3
+              - qcom,sm8250-epss-l3
+          - const: qcom,epss-l3-perf
 
   reg:
     maxItems: 1
-- 
2.39.2

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

[Raviteja Laggyshetty]

Add Epoch Subsystem (EPSS) L3 interconnect provider node on SA8775P
SoCs.
Update the generic compatible for SM8250 and SC7280 SoCs to
"qcom,epss-l3-perf" as they use PERF_STATE register for L3 scaling.

Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sa8775p.dtsi | 19 +++++++++++++++++++
 arch/arm64/boot/dts/qcom/sc7280.dtsi  |  2 +-
 arch/arm64/boot/dts/qcom/sm8250.dtsi  |  2 +-
 3 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sa8775p.dtsi b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
index 9f315a51a7c1..0c2bd15f9ef0 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,sm8250-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,sm8250-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>;
diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index 55db1c83ef55..544c6d725764 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -6125,7 +6125,7 @@ rpmhcc: clock-controller {
 		};
 
 		epss_l3: interconnect@18590000 {
-			compatible = "qcom,sc7280-epss-l3", "qcom,epss-l3";
+			compatible = "qcom,sc7280-epss-l3", "qcom,epss-l3-perf";
 			reg = <0 0x18590000 0 0x1000>;
 			clocks = <&rpmhcc RPMH_CXO_CLK>, <&gcc GCC_GPLL0>;
 			clock-names = "xo", "alternate";
diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
index 48318ed1ce98..f4a223bfe748 100644
--- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
@@ -6230,7 +6230,7 @@ apps_bcm_voter: bcm-voter {
 		};
 
 		epss_l3: interconnect@18590000 {
-			compatible = "qcom,sm8250-epss-l3", "qcom,epss-l3";
+			compatible = "qcom,sm8250-epss-l3", "qcom,epss-l3-perf";
 			reg = <0 0x18590000 0 0x1000>;
 
 			clocks = <&rpmhcc RPMH_CXO_CLK>, <&gcc GPLL0>;
-- 
2.39.2

Re: [PATCH V6 3/4] dt-bindings: interconnect: Add generic compatible qcom,epss-l3-perf

[Krzysztof Kozlowski]

On 25/11/2024 18:45, Raviteja Laggyshetty wrote:
> EPSS instance on sc7280, sm8250 SoCs, use PERF_STATE register instead of


This should explain that these devices are actually 100% compatible.

> REG_L3_VOTE to scale L3 clocks, hence adding a new generic
> compatible "qcom,epss-l3-perf" for these targets.

Not the best reason... You add one more generic compatible, because
devices are different? With such reason answer is: no, do not add
generic compatibles, because they are not generic.

The entire point of having generic compatibles is that they really are
generic. Here you prove that they are not generic, so why creating one
more generic compatible which might not be generic at all?

I already expressed above concerns multiple times:
1. In previous versions of this patchset
2. In other threads

I am not against this change, but I am not going to Ack it. Get acks
from other maintainers, I am not happy with multiple
generic-but-actually-not-generic compatibles. I think that is poor approach.

Best regards,
Krzysztof

Re: [PATCH V6 3/4] dt-bindings: interconnect: Add generic compatible qcom,epss-l3-perf

[Rob Herring]

On Mon, Nov 25, 2024 at 05:45:10PM +0000, Raviteja Laggyshetty wrote:
> EPSS instance on sc7280, sm8250 SoCs, use PERF_STATE register instead of
> REG_L3_VOTE to scale L3 clocks, hence adding a new generic compatible
> "qcom,epss-l3-perf" for these targets.

Is this a h/w difference from prior blocks or you just want to use B 
instead of A while the h/w has both A and B? The latter sounds like 
driver policy.

It is also an ABI break for s/w that didn't understand 
qcom,epss-l3-perf.

> 
> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
> ---
>  .../devicetree/bindings/interconnect/qcom,osm-l3.yaml      | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
> index 21dae0b92819..e24399ca110f 100644
> --- a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
> +++ b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
> @@ -28,12 +28,15 @@ properties:
>            - const: qcom,osm-l3
>        - items:
>            - enum:
> -              - qcom,sc7280-epss-l3
>                - qcom,sc8280xp-epss-l3
>                - qcom,sm6375-cpucp-l3
> -              - qcom,sm8250-epss-l3
>                - qcom,sm8350-epss-l3
>            - const: qcom,epss-l3
> +      - items:
> +          - enum:
> +              - qcom,sc7280-epss-l3
> +              - qcom,sm8250-epss-l3
> +          - const: qcom,epss-l3-perf
>  
>    reg:
>      maxItems: 1
> -- 
> 2.39.2
> 

Re: [PATCH V6 3/4] dt-bindings: interconnect: Add generic compatible qcom,epss-l3-perf

[Dmitry Baryshkov]

On Wed, Nov 27, 2024 at 08:23:04AM -0600, Rob Herring wrote:
> On Mon, Nov 25, 2024 at 05:45:10PM +0000, Raviteja Laggyshetty wrote:
> > EPSS instance on sc7280, sm8250 SoCs, use PERF_STATE register instead of
> > REG_L3_VOTE to scale L3 clocks, hence adding a new generic compatible
> > "qcom,epss-l3-perf" for these targets.
> 
> Is this a h/w difference from prior blocks or you just want to use B 
> instead of A while the h/w has both A and B? The latter sounds like 
> driver policy.
> 
> It is also an ABI break for s/w that didn't understand 
> qcom,epss-l3-perf.

As the bindings keep old compatible strings in addition to the new
qcom,epss-l3-perf, where is the ABI break? Old SW will use old entries,
newer can use either of those.

> 
> > 
> > Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
> > ---
> >  .../devicetree/bindings/interconnect/qcom,osm-l3.yaml      | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
> > index 21dae0b92819..e24399ca110f 100644
> > --- a/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
> > +++ b/Documentation/devicetree/bindings/interconnect/qcom,osm-l3.yaml
> > @@ -28,12 +28,15 @@ properties:
> >            - const: qcom,osm-l3
> >        - items:
> >            - enum:
> > -              - qcom,sc7280-epss-l3
> >                - qcom,sc8280xp-epss-l3
> >                - qcom,sm6375-cpucp-l3
> > -              - qcom,sm8250-epss-l3
> >                - qcom,sm8350-epss-l3
> >            - const: qcom,epss-l3
> > +      - items:
> > +          - enum:
> > +              - qcom,sc7280-epss-l3
> > +              - qcom,sm8250-epss-l3
> > +          - const: qcom,epss-l3-perf
> >  
> >    reg:
> >      maxItems: 1
> > -- 
> > 2.39.2
> > 

-- 
With best wishes
Dmitry

Re: [PATCH V6 3/4] dt-bindings: interconnect: Add generic compatible qcom,epss-l3-perf

[Krzysztof Kozlowski]

On 27/11/2024 17:53, Dmitry Baryshkov wrote:
> On Wed, Nov 27, 2024 at 08:23:04AM -0600, Rob Herring wrote:
>> On Mon, Nov 25, 2024 at 05:45:10PM +0000, Raviteja Laggyshetty wrote:
>>> EPSS instance on sc7280, sm8250 SoCs, use PERF_STATE register instead of
>>> REG_L3_VOTE to scale L3 clocks, hence adding a new generic compatible
>>> "qcom,epss-l3-perf" for these targets.
>>
>> Is this a h/w difference from prior blocks or you just want to use B 
>> instead of A while the h/w has both A and B? The latter sounds like 
>> driver policy.
>>
>> It is also an ABI break for s/w that didn't understand 
>> qcom,epss-l3-perf.
> 
> As the bindings keep old compatible strings in addition to the new
> qcom,epss-l3-perf, where is the ABI break? Old SW will use old entries,
> newer can use either of those.
No, this change drops qcom,epss-l3 and adds new fallback. How old
software can work in such case? It's broken.

Best regards,
Krzysztof

Re: [PATCH V6 3/4] dt-bindings: interconnect: Add generic compatible qcom,epss-l3-perf

[Dmitry Baryshkov]

On 27 November 2024 20:27:27 EET, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>On 27/11/2024 17:53, Dmitry Baryshkov wrote:
>> On Wed, Nov 27, 2024 at 08:23:04AM -0600, Rob Herring wrote:
>>> On Mon, Nov 25, 2024 at 05:45:10PM +0000, Raviteja Laggyshetty wrote:
>>>> EPSS instance on sc7280, sm8250 SoCs, use PERF_STATE register instead of
>>>> REG_L3_VOTE to scale L3 clocks, hence adding a new generic compatible
>>>> "qcom,epss-l3-perf" for these targets.
>>>
>>> Is this a h/w difference from prior blocks or you just want to use B 
>>> instead of A while the h/w has both A and B? The latter sounds like 
>>> driver policy.
>>>
>>> It is also an ABI break for s/w that didn't understand 
>>> qcom,epss-l3-perf.
>> 
>> As the bindings keep old compatible strings in addition to the new
>> qcom,epss-l3-perf, where is the ABI break? Old SW will use old entries,
>> newer can use either of those.
>No, this change drops qcom,epss-l3 and adds new fallback. How old
>software can work in such case? It's broken.

Oh, I see. We had a platform-specific overrides for those two. Then I think we should completely drop the new qcom,epss-l3-perf idea and follow the sm8250 / sc7280 example. This means compatible = "qcom,sa8775p-perf", "qcom,epss-l3". 


>
>Best regards,
>Krzysztof

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

[Krzysztof Kozlowski]

On 25/11/2024 18:45, Raviteja Laggyshetty wrote:
> Add Epoch Subsystem (EPSS) L3 interconnect provider node on SA8775P
> SoCs.
> Update the generic compatible for SM8250 and SC7280 SoCs to
> "qcom,epss-l3-perf" as they use PERF_STATE register for L3 scaling.
> 
> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sa8775p.dtsi | 19 +++++++++++++++++++
>  arch/arm64/boot/dts/qcom/sc7280.dtsi  |  2 +-
>  arch/arm64/boot/dts/qcom/sm8250.dtsi  |  2 +-
>  3 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8775p.dtsi b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
> index 9f315a51a7c1..0c2bd15f9ef0 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,sm8250-epss-l3",
> +				     "qcom,epss-l3-perf";
This is sa8775p, not sm8250. Wrong compatible.

Best regards,
Krzysztof

Re: [PATCH V6 3/4] dt-bindings: interconnect: Add generic compatible qcom,epss-l3-perf

[Krzysztof Kozlowski]

On 27/11/2024 19:49, Dmitry Baryshkov wrote:
> On 27 November 2024 20:27:27 EET, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> On 27/11/2024 17:53, Dmitry Baryshkov wrote:
>>> On Wed, Nov 27, 2024 at 08:23:04AM -0600, Rob Herring wrote:
>>>> On Mon, Nov 25, 2024 at 05:45:10PM +0000, Raviteja Laggyshetty wrote:
>>>>> EPSS instance on sc7280, sm8250 SoCs, use PERF_STATE register instead of
>>>>> REG_L3_VOTE to scale L3 clocks, hence adding a new generic compatible
>>>>> "qcom,epss-l3-perf" for these targets.
>>>>
>>>> Is this a h/w difference from prior blocks or you just want to use B 
>>>> instead of A while the h/w has both A and B? The latter sounds like 
>>>> driver policy.
>>>>
>>>> It is also an ABI break for s/w that didn't understand 
>>>> qcom,epss-l3-perf.
>>>
>>> As the bindings keep old compatible strings in addition to the new
>>> qcom,epss-l3-perf, where is the ABI break? Old SW will use old entries,
>>> newer can use either of those.
>> No, this change drops qcom,epss-l3 and adds new fallback. How old
>> software can work in such case? It's broken.
> 
> Oh, I see. We had a platform-specific overrides for those two. Then I think we should completely drop the new qcom,epss-l3-perf idea and follow the sm8250 / sc7280 example. This means compatible = "qcom,sa8775p-perf", "qcom,epss-l3". 

It depends for example whether epss-l3 is valid at all. ABI is not
broken if nothing was working in the first place, assuming it is
explained in commit msg (not the case here).

Best regards,
Krzysztof

Re: [PATCH V6 3/4] dt-bindings: interconnect: Add generic compatible qcom,epss-l3-perf

[Dmitry Baryshkov]

On 27 November 2024 21:22:02 EET, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>On 27/11/2024 19:49, Dmitry Baryshkov wrote:
>> On 27 November 2024 20:27:27 EET, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>> On 27/11/2024 17:53, Dmitry Baryshkov wrote:
>>>> On Wed, Nov 27, 2024 at 08:23:04AM -0600, Rob Herring wrote:
>>>>> On Mon, Nov 25, 2024 at 05:45:10PM +0000, Raviteja Laggyshetty wrote:
>>>>>> EPSS instance on sc7280, sm8250 SoCs, use PERF_STATE register instead of
>>>>>> REG_L3_VOTE to scale L3 clocks, hence adding a new generic compatible
>>>>>> "qcom,epss-l3-perf" for these targets.
>>>>>
>>>>> Is this a h/w difference from prior blocks or you just want to use B 
>>>>> instead of A while the h/w has both A and B? The latter sounds like 
>>>>> driver policy.
>>>>>
>>>>> It is also an ABI break for s/w that didn't understand 
>>>>> qcom,epss-l3-perf.
>>>>
>>>> As the bindings keep old compatible strings in addition to the new
>>>> qcom,epss-l3-perf, where is the ABI break? Old SW will use old entries,
>>>> newer can use either of those.
>>> No, this change drops qcom,epss-l3 and adds new fallback. How old
>>> software can work in such case? It's broken.
>> 
>> Oh, I see. We had a platform-specific overrides for those two. Then I think we should completely drop the new qcom,epss-l3-perf idea and follow the sm8250 / sc7280 example. This means compatible = "qcom,sa8775p-perf", "qcom,epss-l3". 
>
>It depends for example whether epss-l3 is valid at all. ABI is not
>broken if nothing was working in the first place, assuming it is
>explained in commit msg (not the case here).

Judging by the current schema, epss-l3 is defined as new HW block of aka not OSM L3, no matter which register is used for programming.


>
>Best regards,
>Krzysztof

Re: [PATCH V6 1/4] interconnect: qcom: Add multidev EPSS L3 support

[Konrad Dybcio]

On 25.11.2024 6:45 PM, Raviteja Laggyshetty wrote:
> EPSS on SA8775P has two instances which requires creation of two device
> nodes with different compatible and device data because of unique
> icc node id and name limitation in interconnect framework.
> Add multidevice support to osm-l3 code to get unique node id from IDA
> and node name is made unique by appending node address.
> 
> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
> ---

[...]

> +	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);

As I've said in my previous emails, this is a framework-level problem.

Up until now we've simply silently ignored the possibility of an
interconnect provider having more than one instance, as conveniently
most previous SoCs had a bunch of distinct bus masters.

Currently, debugfs-client.c relies on the node names being unique.
Keeping them as such is also useful for having a sane sysfs/debugfs
interface. But it's not always feasible, and a hierarchical approach
(like in pmdomain) may be a better fit.

Then, node->id is used for creating links, and we unfortunately cannot
assume that both src and dst are within the same provider.
I'm not a fan of these IDs being hardcoded, but there are some drivers
that rely on that, which itself is also a bit unfortunate..


If Mike (who introduced debugfs-client and is probably the main user)
doesn't object to a small ABI break (which is "fine" with a debugfs
driver that requires editing the source code to be compiled), we could
add a property within icc_provider like `bool dynamic_ids` and have an
ICC-global IDA that would take care of any conflicts.

Provider drivers whose consumers don't already rely on programmatical
use of hardcoded IDs *and* don't have cross-provider links could then
enable that flag and have the node IDs and names set like you did in
this patch. This also sounds very useful for icc-clk.

Konrad

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

[Konrad Dybcio]

On 27.11.2024 8:21 PM, Krzysztof Kozlowski wrote:
> On 25/11/2024 18:45, Raviteja Laggyshetty wrote:
>> Add Epoch Subsystem (EPSS) L3 interconnect provider node on SA8775P
>> SoCs.
>> Update the generic compatible for SM8250 and SC7280 SoCs to
>> "qcom,epss-l3-perf" as they use PERF_STATE register for L3 scaling.
>>
>> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
>> ---
>>  arch/arm64/boot/dts/qcom/sa8775p.dtsi | 19 +++++++++++++++++++
>>  arch/arm64/boot/dts/qcom/sc7280.dtsi  |  2 +-
>>  arch/arm64/boot/dts/qcom/sm8250.dtsi  |  2 +-
>>  3 files changed, 21 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sa8775p.dtsi b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
>> index 9f315a51a7c1..0c2bd15f9ef0 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,sm8250-epss-l3",
>> +				     "qcom,epss-l3-perf";
> This is sa8775p, not sm8250. Wrong compatible.

The bigger issue here is that a treewide binding adjustment is
coupled with a feature addition in a single patch.

They should be separate.

Konrad

Re: [PATCH V6 1/4] interconnect: qcom: Add multidev EPSS L3 support

[Dmitry Baryshkov]

On Sat, Nov 30, 2024 at 01:49:56PM +0100, Konrad Dybcio wrote:
> On 25.11.2024 6:45 PM, Raviteja Laggyshetty wrote:
> > EPSS on SA8775P has two instances which requires creation of two device
> > nodes with different compatible and device data because of unique
> > icc node id and name limitation in interconnect framework.
> > Add multidevice support to osm-l3 code to get unique node id from IDA
> > and node name is made unique by appending node address.
> > 
> > Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
> > ---
> 
> [...]
> 
> > +	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);
> 
> As I've said in my previous emails, this is a framework-level problem.
> 
> Up until now we've simply silently ignored the possibility of an
> interconnect provider having more than one instance, as conveniently
> most previous SoCs had a bunch of distinct bus masters.
> 
> Currently, debugfs-client.c relies on the node names being unique.
> Keeping them as such is also useful for having a sane sysfs/debugfs
> interface. But it's not always feasible, and a hierarchical approach
> (like in pmdomain) may be a better fit.
> 
> Then, node->id is used for creating links, and we unfortunately cannot
> assume that both src and dst are within the same provider.
> I'm not a fan of these IDs being hardcoded, but there are some drivers
> that rely on that, which itself is also a bit unfortunate..
> 
> 
> If Mike (who introduced debugfs-client and is probably the main user)
> doesn't object to a small ABI break (which is "fine" with a debugfs
> driver that requires editing the source code to be compiled), we could
> add a property within icc_provider like `bool dynamic_ids` and have an
> ICC-global IDA that would take care of any conflicts.

Frankly speaking, I think this just delays the inevitable. We have been
there with GPIOs and with some other suppliers. In my opinion the ICC
subsystem needs to be refactored in order to support linking based on
the supplier (fwnode?) + offset_id, but that's a huuuge rework.

> Provider drivers whose consumers don't already rely on programmatical
> use of hardcoded IDs *and* don't have cross-provider links could then
> enable that flag and have the node IDs and names set like you did in
> this patch. This also sounds very useful for icc-clk.

-- 
With best wishes
Dmitry

Re: [PATCH V6 1/4] interconnect: qcom: Add multidev EPSS L3 support

[Konrad Dybcio]

On 30.11.2024 4:09 PM, Dmitry Baryshkov wrote:
> On Sat, Nov 30, 2024 at 01:49:56PM +0100, Konrad Dybcio wrote:
>> On 25.11.2024 6:45 PM, Raviteja Laggyshetty wrote:
>>> EPSS on SA8775P has two instances which requires creation of two device
>>> nodes with different compatible and device data because of unique
>>> icc node id and name limitation in interconnect framework.
>>> Add multidevice support to osm-l3 code to get unique node id from IDA
>>> and node name is made unique by appending node address.
>>>
>>> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
>>> ---
>>
>> [...]
>>
>>> +	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);
>>
>> As I've said in my previous emails, this is a framework-level problem.
>>
>> Up until now we've simply silently ignored the possibility of an
>> interconnect provider having more than one instance, as conveniently
>> most previous SoCs had a bunch of distinct bus masters.
>>
>> Currently, debugfs-client.c relies on the node names being unique.
>> Keeping them as such is also useful for having a sane sysfs/debugfs
>> interface. But it's not always feasible, and a hierarchical approach
>> (like in pmdomain) may be a better fit.
>>
>> Then, node->id is used for creating links, and we unfortunately cannot
>> assume that both src and dst are within the same provider.
>> I'm not a fan of these IDs being hardcoded, but there are some drivers
>> that rely on that, which itself is also a bit unfortunate..
>>
>>
>> If Mike (who introduced debugfs-client and is probably the main user)
>> doesn't object to a small ABI break (which is "fine" with a debugfs
>> driver that requires editing the source code to be compiled), we could
>> add a property within icc_provider like `bool dynamic_ids` and have an
>> ICC-global IDA that would take care of any conflicts.
> 
> Frankly speaking, I think this just delays the inevitable. We have been
> there with GPIOs and with some other suppliers. In my opinion the ICC
> subsystem needs to be refactored in order to support linking based on
> the supplier (fwnode?) + offset_id, but that's a huuuge rework.

I thought about this too, but ended up not including it in the email..

I think this will be more difficult with ICC, as tons of circular
dependencies are inevitable by design and we'd essentially have to
either provide placeholder nodes (like it's the case today) or probe
only parts of a device, recursively, to make sure all links can be
created

Konrad

>> Provider drivers whose consumers don't already rely on programmatical
>> use of hardcoded IDs *and* don't have cross-provider links could then
>> enable that flag and have the node IDs and names set like you did in
>> this patch. This also sounds very useful for icc-clk.
> 

Re: [PATCH V6 1/4] interconnect: qcom: Add multidev EPSS L3 support

[Dmitry Baryshkov]

On Sat, Nov 30, 2024 at 04:12:49PM +0100, Konrad Dybcio wrote:
> On 30.11.2024 4:09 PM, Dmitry Baryshkov wrote:
> > On Sat, Nov 30, 2024 at 01:49:56PM +0100, Konrad Dybcio wrote:
> >> On 25.11.2024 6:45 PM, Raviteja Laggyshetty wrote:
> >>> EPSS on SA8775P has two instances which requires creation of two device
> >>> nodes with different compatible and device data because of unique
> >>> icc node id and name limitation in interconnect framework.
> >>> Add multidevice support to osm-l3 code to get unique node id from IDA
> >>> and node name is made unique by appending node address.
> >>>
> >>> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
> >>> ---
> >>
> >> [...]
> >>
> >>> +	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);
> >>
> >> As I've said in my previous emails, this is a framework-level problem.
> >>
> >> Up until now we've simply silently ignored the possibility of an
> >> interconnect provider having more than one instance, as conveniently
> >> most previous SoCs had a bunch of distinct bus masters.
> >>
> >> Currently, debugfs-client.c relies on the node names being unique.
> >> Keeping them as such is also useful for having a sane sysfs/debugfs
> >> interface. But it's not always feasible, and a hierarchical approach
> >> (like in pmdomain) may be a better fit.
> >>
> >> Then, node->id is used for creating links, and we unfortunately cannot
> >> assume that both src and dst are within the same provider.
> >> I'm not a fan of these IDs being hardcoded, but there are some drivers
> >> that rely on that, which itself is also a bit unfortunate..
> >>
> >>
> >> If Mike (who introduced debugfs-client and is probably the main user)
> >> doesn't object to a small ABI break (which is "fine" with a debugfs
> >> driver that requires editing the source code to be compiled), we could
> >> add a property within icc_provider like `bool dynamic_ids` and have an
> >> ICC-global IDA that would take care of any conflicts.
> > 
> > Frankly speaking, I think this just delays the inevitable. We have been
> > there with GPIOs and with some other suppliers. In my opinion the ICC
> > subsystem needs to be refactored in order to support linking based on
> > the supplier (fwnode?) + offset_id, but that's a huuuge rework.
> 
> I thought about this too, but ended up not including it in the email..
> 
> I think this will be more difficult with ICC, as tons of circular
> dependencies are inevitable by design and we'd essentially have to
> either provide placeholder nodes (like it's the case today) or probe
> only parts of a device, recursively, to make sure all links can be
> created

Or just allow probing, but then fail path creation. It will be a
redesign, but I think it is inevitable in the end.

> 
> Konrad
> 
> >> Provider drivers whose consumers don't already rely on programmatical
> >> use of hardcoded IDs *and* don't have cross-provider links could then
> >> enable that flag and have the node IDs and names set like you did in
> >> this patch. This also sounds very useful for icc-clk.
> > 

-- 
With best wishes
Dmitry

Re: [PATCH V6 3/4] dt-bindings: interconnect: Add generic compatible qcom,epss-l3-perf

[Raviteja Laggyshetty]

On 11/28/2024 1:15 AM, Dmitry Baryshkov wrote:
> On 27 November 2024 21:22:02 EET, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> On 27/11/2024 19:49, Dmitry Baryshkov wrote:
>>> On 27 November 2024 20:27:27 EET, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>> On 27/11/2024 17:53, Dmitry Baryshkov wrote:
>>>>> On Wed, Nov 27, 2024 at 08:23:04AM -0600, Rob Herring wrote:
>>>>>> On Mon, Nov 25, 2024 at 05:45:10PM +0000, Raviteja Laggyshetty wrote:
>>>>>>> EPSS instance on sc7280, sm8250 SoCs, use PERF_STATE register instead of
>>>>>>> REG_L3_VOTE to scale L3 clocks, hence adding a new generic compatible
>>>>>>> "qcom,epss-l3-perf" for these targets.
>>>>>>
>>>>>> Is this a h/w difference from prior blocks or you just want to use B 
>>>>>> instead of A while the h/w has both A and B? The latter sounds like 
>>>>>> driver policy.
>>>>>>
>>>>>> It is also an ABI break for s/w that didn't understand 
>>>>>> qcom,epss-l3-perf.
>>>>>
>>>>> As the bindings keep old compatible strings in addition to the new
>>>>> qcom,epss-l3-perf, where is the ABI break? Old SW will use old entries,
>>>>> newer can use either of those.
>>>> No, this change drops qcom,epss-l3 and adds new fallback. How old
>>>> software can work in such case? It's broken.
>>>
>>> Oh, I see. We had a platform-specific overrides for those two. Then I think we should completely drop the new qcom,epss-l3-perf idea and follow the sm8250 / sc7280 example. This means compatible = "qcom,sa8775p-perf", "qcom,epss-l3". 
>>
>> It depends for example whether epss-l3 is valid at all. ABI is not
>> broken if nothing was working in the first place, assuming it is
>> explained in commit msg (not the case here).
> 
> Judging by the current schema, epss-l3 is defined as new HW block of aka not OSM L3, no matter which register is used for programming.
> 

I am going to remove the newly added "qcom,epss-l3-perf" compatible and
add target specific compatible "qcom,sa8775p-epss-l3" along with
existing generic "qcom,epss-l3" compatible.
> 
>>
>> Best regards,
>> Krzysztof
> 

Re: [PATCH V6 1/4] interconnect: qcom: Add multidev EPSS L3 support

[Raviteja Laggyshetty]

On 11/30/2024 9:02 PM, Dmitry Baryshkov wrote:
> On Sat, Nov 30, 2024 at 04:12:49PM +0100, Konrad Dybcio wrote:
>> On 30.11.2024 4:09 PM, Dmitry Baryshkov wrote:
>>> On Sat, Nov 30, 2024 at 01:49:56PM +0100, Konrad Dybcio wrote:
>>>> On 25.11.2024 6:45 PM, Raviteja Laggyshetty wrote:
>>>>> EPSS on SA8775P has two instances which requires creation of two device
>>>>> nodes with different compatible and device data because of unique
>>>>> icc node id and name limitation in interconnect framework.
>>>>> Add multidevice support to osm-l3 code to get unique node id from IDA
>>>>> and node name is made unique by appending node address.
>>>>>
>>>>> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
>>>>> ---
>>>>
>>>> [...]
>>>>
>>>>> +	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);
>>>>
>>>> As I've said in my previous emails, this is a framework-level problem.
>>>>
>>>> Up until now we've simply silently ignored the possibility of an
>>>> interconnect provider having more than one instance, as conveniently
>>>> most previous SoCs had a bunch of distinct bus masters.
>>>>
>>>> Currently, debugfs-client.c relies on the node names being unique.
>>>> Keeping them as such is also useful for having a sane sysfs/debugfs
>>>> interface. But it's not always feasible, and a hierarchical approach
>>>> (like in pmdomain) may be a better fit.
>>>>
>>>> Then, node->id is used for creating links, and we unfortunately cannot
>>>> assume that both src and dst are within the same provider.
>>>> I'm not a fan of these IDs being hardcoded, but there are some drivers
>>>> that rely on that, which itself is also a bit unfortunate..
>>>>
>>>>
>>>> If Mike (who introduced debugfs-client and is probably the main user)
>>>> doesn't object to a small ABI break (which is "fine" with a debugfs
>>>> driver that requires editing the source code to be compiled), we could
>>>> add a property within icc_provider like `bool dynamic_ids` and have an
>>>> ICC-global IDA that would take care of any conflicts.
>>>
>>> Frankly speaking, I think this just delays the inevitable. We have been
>>> there with GPIOs and with some other suppliers. In my opinion the ICC
>>> subsystem needs to be refactored in order to support linking based on
>>> the supplier (fwnode?) + offset_id, but that's a huuuge rework.
>>
>> I thought about this too, but ended up not including it in the email..
>>
>> I think this will be more difficult with ICC, as tons of circular
>> dependencies are inevitable by design and we'd essentially have to
>> either provide placeholder nodes (like it's the case today) or probe
>> only parts of a device, recursively, to make sure all links can be
>> created
> 
> Or just allow probing, but then fail path creation. It will be a
> redesign, but I think it is inevitable in the end.
> 

There are no two instances of l3 or NoC on any SoC except qcs9100 and
qcs8300. I dont expect any new SoC as well.
As second instance is needed only on qcs9100 and qcs8300, I am keeping
the patch (patchset v6) as is and limit the dynamic id addition to l3
provider only.

>>
>> Konrad
>>
>>>> Provider drivers whose consumers don't already rely on programmatical
>>>> use of hardcoded IDs *and* don't have cross-provider links could then
>>>> enable that flag and have the node IDs and names set like you did in
>>>> this patch. This also sounds very useful for icc-clk.
>>>
> 

Re: [PATCH V6 1/4] interconnect: qcom: Add multidev EPSS L3 support

[Dmitry Baryshkov]

On Thu, Dec 26, 2024 at 09:43:20PM +0530, Raviteja Laggyshetty wrote:
> 
> 
> On 11/30/2024 9:02 PM, Dmitry Baryshkov wrote:
> > On Sat, Nov 30, 2024 at 04:12:49PM +0100, Konrad Dybcio wrote:
> >> On 30.11.2024 4:09 PM, Dmitry Baryshkov wrote:
> >>> On Sat, Nov 30, 2024 at 01:49:56PM +0100, Konrad Dybcio wrote:
> >>>> On 25.11.2024 6:45 PM, Raviteja Laggyshetty wrote:
> >>>>> EPSS on SA8775P has two instances which requires creation of two device
> >>>>> nodes with different compatible and device data because of unique
> >>>>> icc node id and name limitation in interconnect framework.
> >>>>> Add multidevice support to osm-l3 code to get unique node id from IDA
> >>>>> and node name is made unique by appending node address.
> >>>>>
> >>>>> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
> >>>>> ---
> >>>>
> >>>> [...]
> >>>>
> >>>>> +	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);
> >>>>
> >>>> As I've said in my previous emails, this is a framework-level problem.
> >>>>
> >>>> Up until now we've simply silently ignored the possibility of an
> >>>> interconnect provider having more than one instance, as conveniently
> >>>> most previous SoCs had a bunch of distinct bus masters.
> >>>>
> >>>> Currently, debugfs-client.c relies on the node names being unique.
> >>>> Keeping them as such is also useful for having a sane sysfs/debugfs
> >>>> interface. But it's not always feasible, and a hierarchical approach
> >>>> (like in pmdomain) may be a better fit.
> >>>>
> >>>> Then, node->id is used for creating links, and we unfortunately cannot
> >>>> assume that both src and dst are within the same provider.
> >>>> I'm not a fan of these IDs being hardcoded, but there are some drivers
> >>>> that rely on that, which itself is also a bit unfortunate..
> >>>>
> >>>>
> >>>> If Mike (who introduced debugfs-client and is probably the main user)
> >>>> doesn't object to a small ABI break (which is "fine" with a debugfs
> >>>> driver that requires editing the source code to be compiled), we could
> >>>> add a property within icc_provider like `bool dynamic_ids` and have an
> >>>> ICC-global IDA that would take care of any conflicts.
> >>>
> >>> Frankly speaking, I think this just delays the inevitable. We have been
> >>> there with GPIOs and with some other suppliers. In my opinion the ICC
> >>> subsystem needs to be refactored in order to support linking based on
> >>> the supplier (fwnode?) + offset_id, but that's a huuuge rework.
> >>
> >> I thought about this too, but ended up not including it in the email..
> >>
> >> I think this will be more difficult with ICC, as tons of circular
> >> dependencies are inevitable by design and we'd essentially have to
> >> either provide placeholder nodes (like it's the case today) or probe
> >> only parts of a device, recursively, to make sure all links can be
> >> created
> > 
> > Or just allow probing, but then fail path creation. It will be a
> > redesign, but I think it is inevitable in the end.
> > 
> 
> There are no two instances of l3 or NoC on any SoC except qcs9100 and
> qcs8300. I dont expect any new SoC as well.
> As second instance is needed only on qcs9100 and qcs8300, I am keeping
> the patch (patchset v6) as is and limit the dynamic id addition to l3
> provider only.

As you could have noticed, it was suggested to change ICC subsystem API
to allow the dynamic IDs. This isssue is not limited to just EPSS L3
driver. So we were discussing if you or your colleagues could sign up
for updating the interconnect subsystem to use node+arguments approach
instead of using a global static ID list.

> 
> >>
> >> Konrad
> >>
> >>>> Provider drivers whose consumers don't already rely on programmatical
> >>>> use of hardcoded IDs *and* don't have cross-provider links could then
> >>>> enable that flag and have the node IDs and names set like you did in
> >>>> this patch. This also sounds very useful for icc-clk.
> >>>
> > 
> 

-- 
With best wishes
Dmitry

Re: [PATCH V6 1/4] interconnect: qcom: Add multidev EPSS L3 support

[Raviteja Laggyshetty]

On 12/29/2024 6:53 AM, Dmitry Baryshkov wrote:
> On Thu, Dec 26, 2024 at 09:43:20PM +0530, Raviteja Laggyshetty wrote:
>>
>>
>> On 11/30/2024 9:02 PM, Dmitry Baryshkov wrote:
>>> On Sat, Nov 30, 2024 at 04:12:49PM +0100, Konrad Dybcio wrote:
>>>> On 30.11.2024 4:09 PM, Dmitry Baryshkov wrote:
>>>>> On Sat, Nov 30, 2024 at 01:49:56PM +0100, Konrad Dybcio wrote:
>>>>>> On 25.11.2024 6:45 PM, Raviteja Laggyshetty wrote:
>>>>>>> EPSS on SA8775P has two instances which requires creation of two device
>>>>>>> nodes with different compatible and device data because of unique
>>>>>>> icc node id and name limitation in interconnect framework.
>>>>>>> Add multidevice support to osm-l3 code to get unique node id from IDA
>>>>>>> and node name is made unique by appending node address.
>>>>>>>
>>>>>>> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
>>>>>>> ---
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>> +	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);
>>>>>>
>>>>>> As I've said in my previous emails, this is a framework-level problem.
>>>>>>
>>>>>> Up until now we've simply silently ignored the possibility of an
>>>>>> interconnect provider having more than one instance, as conveniently
>>>>>> most previous SoCs had a bunch of distinct bus masters.
>>>>>>
>>>>>> Currently, debugfs-client.c relies on the node names being unique.
>>>>>> Keeping them as such is also useful for having a sane sysfs/debugfs
>>>>>> interface. But it's not always feasible, and a hierarchical approach
>>>>>> (like in pmdomain) may be a better fit.
>>>>>>
>>>>>> Then, node->id is used for creating links, and we unfortunately cannot
>>>>>> assume that both src and dst are within the same provider.
>>>>>> I'm not a fan of these IDs being hardcoded, but there are some drivers
>>>>>> that rely on that, which itself is also a bit unfortunate..
>>>>>>
>>>>>>
>>>>>> If Mike (who introduced debugfs-client and is probably the main user)
>>>>>> doesn't object to a small ABI break (which is "fine" with a debugfs
>>>>>> driver that requires editing the source code to be compiled), we could
>>>>>> add a property within icc_provider like `bool dynamic_ids` and have an
>>>>>> ICC-global IDA that would take care of any conflicts.
>>>>>
>>>>> Frankly speaking, I think this just delays the inevitable. We have been
>>>>> there with GPIOs and with some other suppliers. In my opinion the ICC
>>>>> subsystem needs to be refactored in order to support linking based on
>>>>> the supplier (fwnode?) + offset_id, but that's a huuuge rework.
>>>>
>>>> I thought about this too, but ended up not including it in the email..
>>>>
>>>> I think this will be more difficult with ICC, as tons of circular
>>>> dependencies are inevitable by design and we'd essentially have to
>>>> either provide placeholder nodes (like it's the case today) or probe
>>>> only parts of a device, recursively, to make sure all links can be
>>>> created
>>>
>>> Or just allow probing, but then fail path creation. It will be a
>>> redesign, but I think it is inevitable in the end.
>>>
>>
>> There are no two instances of l3 or NoC on any SoC except qcs9100 and
>> qcs8300. I dont expect any new SoC as well.
>> As second instance is needed only on qcs9100 and qcs8300, I am keeping
>> the patch (patchset v6) as is and limit the dynamic id addition to l3
>> provider only.
> 
> As you could have noticed, it was suggested to change ICC subsystem API
> to allow the dynamic IDs. This isssue is not limited to just EPSS L3
> driver. So we were discussing if you or your colleagues could sign up
> for updating the interconnect subsystem to use node+arguments approach
> instead of using a global static ID list.
> 
This problem is limited to EPSS L3 only, NoCs are not having
multi instances and don't expect this problem to arise in new
chipsets. we have multi instances of L3 only on qcs9100 and
qcs8300. we can limit the dynamic ID creation for L3 provider.
If we update the interconnect framework also, we will have to
limit it to provider only.

Thanks,
Raviteja
>>
>>>>
>>>> Konrad
>>>>
>>>>>> Provider drivers whose consumers don't already rely on programmatical
>>>>>> use of hardcoded IDs *and* don't have cross-provider links could then
>>>>>> enable that flag and have the node IDs and names set like you did in
>>>>>> this patch. This also sounds very useful for icc-clk.
>>>>>
>>>
>>
> 

Re: [PATCH V6 1/4] interconnect: qcom: Add multidev EPSS L3 support

[Dmitry Baryshkov]

On Fri, Jan 03, 2025 at 07:37:07PM +0530, Raviteja Laggyshetty wrote:
> 
> 
> On 12/29/2024 6:53 AM, Dmitry Baryshkov wrote:
> > On Thu, Dec 26, 2024 at 09:43:20PM +0530, Raviteja Laggyshetty wrote:
> >>
> >>
> >> On 11/30/2024 9:02 PM, Dmitry Baryshkov wrote:
> >>> On Sat, Nov 30, 2024 at 04:12:49PM +0100, Konrad Dybcio wrote:
> >>>> On 30.11.2024 4:09 PM, Dmitry Baryshkov wrote:
> >>>>> On Sat, Nov 30, 2024 at 01:49:56PM +0100, Konrad Dybcio wrote:
> >>>>>> On 25.11.2024 6:45 PM, Raviteja Laggyshetty wrote:
> >>>>>>> EPSS on SA8775P has two instances which requires creation of two device
> >>>>>>> nodes with different compatible and device data because of unique
> >>>>>>> icc node id and name limitation in interconnect framework.
> >>>>>>> Add multidevice support to osm-l3 code to get unique node id from IDA
> >>>>>>> and node name is made unique by appending node address.
> >>>>>>>
> >>>>>>> Signed-off-by: Raviteja Laggyshetty <quic_rlaggysh@quicinc.com>
> >>>>>>> ---
> >>>>>>
> >>>>>> [...]
> >>>>>>
> >>>>>>> +	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);
> >>>>>>
> >>>>>> As I've said in my previous emails, this is a framework-level problem.
> >>>>>>
> >>>>>> Up until now we've simply silently ignored the possibility of an
> >>>>>> interconnect provider having more than one instance, as conveniently
> >>>>>> most previous SoCs had a bunch of distinct bus masters.
> >>>>>>
> >>>>>> Currently, debugfs-client.c relies on the node names being unique.
> >>>>>> Keeping them as such is also useful for having a sane sysfs/debugfs
> >>>>>> interface. But it's not always feasible, and a hierarchical approach
> >>>>>> (like in pmdomain) may be a better fit.
> >>>>>>
> >>>>>> Then, node->id is used for creating links, and we unfortunately cannot
> >>>>>> assume that both src and dst are within the same provider.
> >>>>>> I'm not a fan of these IDs being hardcoded, but there are some drivers
> >>>>>> that rely on that, which itself is also a bit unfortunate..
> >>>>>>
> >>>>>>
> >>>>>> If Mike (who introduced debugfs-client and is probably the main user)
> >>>>>> doesn't object to a small ABI break (which is "fine" with a debugfs
> >>>>>> driver that requires editing the source code to be compiled), we could
> >>>>>> add a property within icc_provider like `bool dynamic_ids` and have an
> >>>>>> ICC-global IDA that would take care of any conflicts.
> >>>>>
> >>>>> Frankly speaking, I think this just delays the inevitable. We have been
> >>>>> there with GPIOs and with some other suppliers. In my opinion the ICC
> >>>>> subsystem needs to be refactored in order to support linking based on
> >>>>> the supplier (fwnode?) + offset_id, but that's a huuuge rework.
> >>>>
> >>>> I thought about this too, but ended up not including it in the email..
> >>>>
> >>>> I think this will be more difficult with ICC, as tons of circular
> >>>> dependencies are inevitable by design and we'd essentially have to
> >>>> either provide placeholder nodes (like it's the case today) or probe
> >>>> only parts of a device, recursively, to make sure all links can be
> >>>> created
> >>>
> >>> Or just allow probing, but then fail path creation. It will be a
> >>> redesign, but I think it is inevitable in the end.
> >>>
> >>
> >> There are no two instances of l3 or NoC on any SoC except qcs9100 and
> >> qcs8300. I dont expect any new SoC as well.
> >> As second instance is needed only on qcs9100 and qcs8300, I am keeping
> >> the patch (patchset v6) as is and limit the dynamic id addition to l3
> >> provider only.
> > 
> > As you could have noticed, it was suggested to change ICC subsystem API
> > to allow the dynamic IDs. This isssue is not limited to just EPSS L3
> > driver. So we were discussing if you or your colleagues could sign up
> > for updating the interconnect subsystem to use node+arguments approach
> > instead of using a global static ID list.
> > 
> This problem is limited to EPSS L3 only, NoCs are not having
> multi instances and don't expect this problem to arise in new
> chipsets. we have multi instances of L3 only on qcs9100 and
> qcs8300. we can limit the dynamic ID creation for L3 provider.
> If we update the interconnect framework also, we will have to
> limit it to provider only.

This problem is not limited to the EPSS L3 only. You just see one side
of a problem. The icc_clk framework has a similar issue. Each time we
register ICC node, we have to provide globally unique ID. This works in
simple cases, but if the platform is slightly more strange, things start
to be complicated.

> 
> Thanks,
> Raviteja
> >>
> >>>>
> >>>> Konrad
> >>>>
> >>>>>> Provider drivers whose consumers don't already rely on programmatical
> >>>>>> use of hardcoded IDs *and* don't have cross-provider links could then
> >>>>>> enable that flag and have the node IDs and names set like you did in
> >>>>>> this patch. This also sounds very useful for icc-clk.
> >>>>>
> >>>
> >>
> > 
> 

-- 
With best wishes
Dmitry