Tegra234 Memory interconnect support

For Tegra234, add the "nvidia,bpmp" property within the Memory Controller (MC) node to reference BPMP node. This is needed in the MC driver to pass the client info to the BPMP-FW when memory interconnect support is available.

Signed-off-by Sumit Gupta <[email protected]>

---
 .../bindings/memory-controllers/nvidia,tegra186-mc.yaml    | 7 +++++++
 1 file changed, 7 insertions(+)

Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml +0 -0

index 935d63d181d9..398d27bb2373 100644

Quoted 18 lines · Sumit Gupta

For Tegra234, add the "nvidia,bpmp" property within the Memory Controller (MC) node to reference BPMP node. This is needed in the MC driver to pass the client info to the BPMP-FW when memory interconnect support is available.

Signed-off-by Sumit Gupta <[email protected]>

---
 .../bindings/memory-controllers/nvidia,tegra186-mc.yaml    | 7 +++++++
 1 file changed, 7 insertions(+)

Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml +0 -0

index 935d63d181d9..398d27bb2373 100644
@@ -58,6 +58,10 @@ properties:
   "#interconnect-cells":
     const: 1
 

Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml properties: · line 63

Quoted 3 lines

Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml +3 -0

+  nvidia,bpmp:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: phandle of the node representing the BPMP

Why do you need this multiple times? Both in parent and all external-mc children?

Quoted 19 lines · Krzysztof Kozlowski

Quoted 18 lines · Sumit Gupta

For Tegra234, add the "nvidia,bpmp" property within the Memory Controller (MC) node to reference BPMP node. This is needed in the MC driver to pass the client info to the BPMP-FW when memory interconnect support is available.

Signed-off-by Sumit Gupta <[email protected]>

---
 .../bindings/memory-controllers/nvidia,tegra186-mc.yaml    | 7 +++++++
 1 file changed, 7 insertions(+)

Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml +0 -0

index 935d63d181d9..398d27bb2373 100644
@@ -58,6 +58,10 @@ properties:
   "#interconnect-cells":
     const: 1
 

Quoted 6 lines

Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml properties: · line 63

Quoted 3 lines

Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml +3 -0

+  nvidia,bpmp:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: phandle of the node representing the BPMP

Why do you need this multiple times? Both in parent and all external-mc children?

We've had nvidia,bpmp in the external memory controller node since basically the beginning because we've always needed it there. For newer chips we now also need it for the memory controller.

Ideally I think we would only have this in the MC and have the EMC driver reference it via the EMC's parent (i.e. MC), but that would break backwards-compatibility. Reaching into the EMC's DT node from the MC was another option that we discussed internally, but it didn't look right given how this is also needed by the MC.

One thing we could potentially do is deprecate the nvidia,bpmp phandle in the EMC and only keep it as a fallback in the drivers in case the parent MC doesn't find it's own in the DT.

Quoted 37 lines · Thierry Reding

Quoted 19 lines · Krzysztof Kozlowski

Quoted 18 lines · Sumit Gupta

For Tegra234, add the "nvidia,bpmp" property within the Memory Controller (MC) node to reference BPMP node. This is needed in the MC driver to pass the client info to the BPMP-FW when memory interconnect support is available.

Signed-off-by Sumit Gupta <[email protected]>

---
 .../bindings/memory-controllers/nvidia,tegra186-mc.yaml    | 7 +++++++
 1 file changed, 7 insertions(+)

Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml +0 -0

index 935d63d181d9..398d27bb2373 100644
@@ -58,6 +58,10 @@ properties:
   "#interconnect-cells":
     const: 1
 

Quoted 6 lines

Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml properties: · line 63

Quoted 3 lines

Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml +3 -0

+  nvidia,bpmp:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: phandle of the node representing the BPMP

Why do you need this multiple times? Both in parent and all external-mc children?

We've had nvidia,bpmp in the external memory controller node since basically the beginning because we've always needed it there. For newer chips we now also need it for the memory controller.

Ideally I think we would only have this in the MC and have the EMC driver reference it via the EMC's parent (i.e. MC), but that would break backwards-compatibility. Reaching into the EMC's DT node from the MC was another option that we discussed internally, but it didn't look right given how this is also needed by the MC.

Quoted 3 lines

One thing we could potentially do is deprecate the nvidia,bpmp phandle in the EMC and only keep it as a fallback in the drivers in case the parent MC doesn't find it's own in the DT.

Yes, deprecation would answer to my question.

Quoted 42 lines · Krzysztof Kozlowski

Quoted 40 lines · Thierry Reding

Quoted 19 lines · Krzysztof Kozlowski

Quoted 18 lines · Sumit Gupta

For Tegra234, add the "nvidia,bpmp" property within the Memory Controller (MC) node to reference BPMP node. This is needed in the MC driver to pass the client info to the BPMP-FW when memory interconnect support is available.

Signed-off-by Sumit Gupta <[email protected]>

---
  .../bindings/memory-controllers/nvidia,tegra186-mc.yaml    | 7 +++++++
  1 file changed, 7 insertions(+)

Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml +0 -0

index 935d63d181d9..398d27bb2373 100644
@@ -58,6 +58,10 @@ properties:
    "#interconnect-cells":
      const: 1
 

Quoted 6 lines

Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml properties: · line 63

Quoted 3 lines

Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml +3 -0

+  nvidia,bpmp:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: phandle of the node representing the BPMP

Why do you need this multiple times? Both in parent and all external-mc children?

We've had nvidia,bpmp in the external memory controller node since basically the beginning because we've always needed it there. For newer chips we now also need it for the memory controller.

Ideally I think we would only have this in the MC and have the EMC driver reference it via the EMC's parent (i.e. MC), but that would break backwards-compatibility. Reaching into the EMC's DT node from the MC was another option that we discussed internally, but it didn't look right given how this is also needed by the MC.

One thing we could potentially do is deprecate the nvidia,bpmp phandle in the EMC and only keep it as a fallback in the drivers in case the parent MC doesn't find it's own in the DT.

Quoted 1 line

Yes, deprecation would answer to my question.

Okay, great. Sumit, you can resolve this by adding a "deprecated: true" to the EMC's nvidia,bpmp property schema. In the driver we can then try to look at the MC's ->bpmp and if it exists reuse that. If it doesn't exist, we can keep the existing lookup as a fallback for device trees that haven't been updated yet.

Quoted 43 lines · Thierry Reding

Quoted 42 lines · Krzysztof Kozlowski

Quoted 40 lines · Thierry Reding

Quoted 19 lines · Krzysztof Kozlowski

Quoted 18 lines · Sumit Gupta

For Tegra234, add the "nvidia,bpmp" property within the Memory Controller (MC) node to reference BPMP node. This is needed in the MC driver to pass the client info to the BPMP-FW when memory interconnect support is available.

Signed-off-by Sumit Gupta <[email protected]>

---
  .../bindings/memory-controllers/nvidia,tegra186-mc.yaml    | 7 +++++++
  1 file changed, 7 insertions(+)

Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml +0 -0

index 935d63d181d9..398d27bb2373 100644
@@ -58,6 +58,10 @@ properties:
    "#interconnect-cells":
      const: 1
 

Quoted 6 lines

Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml properties: · line 63

Quoted 3 lines

Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml +3 -0

+  nvidia,bpmp:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: phandle of the node representing the BPMP

Why do you need this multiple times? Both in parent and all external-mc children?

We've had nvidia,bpmp in the external memory controller node since basically the beginning because we've always needed it there. For newer chips we now also need it for the memory controller.

Ideally I think we would only have this in the MC and have the EMC driver reference it via the EMC's parent (i.e. MC), but that would break backwards-compatibility. Reaching into the EMC's DT node from the MC was another option that we discussed internally, but it didn't look right given how this is also needed by the MC.

One thing we could potentially do is deprecate the nvidia,bpmp phandle in the EMC and only keep it as a fallback in the drivers in case the parent MC doesn't find it's own in the DT.

Quoted 7 lines

Quoted 1 line

Yes, deprecation would answer to my question.

Okay, great. Sumit, you can resolve this by adding a "deprecated: true" to the EMC's nvidia,bpmp property schema. In the driver we can then try to look at the MC's ->bpmp and if it exists reuse that. If it doesn't exist, we can keep the existing lookup as a fallback for device trees that haven't been updated yet.

We can't use MC's->bpmp in the EMC driver's probe as it will be NULL. This is because MC driver uses "arch_initcall" and gets probed earlier than BPMP. We can do this in another way as below change. This way we can use the existing "nvidia,bpmp" property from EMC node and don't need to move it to the MC node. Please share if this change sounds OK.

Quoted 54 lines · Sumit Gupta

Quoted 50 lines · Thierry Reding

Quoted 38 lines · Krzysztof Kozlowski

Quoted 37 lines · Thierry Reding

Quoted 19 lines · Krzysztof Kozlowski

Quoted 18 lines · Sumit Gupta

For Tegra234, add the "nvidia,bpmp" property within the Memory Controller (MC) node to reference BPMP node. This is needed in the MC driver to pass the client info to the BPMP-FW when memory interconnect support is available.

Signed-off-by Sumit Gupta <[email protected]>

---
  .../bindings/memory-controllers/nvidia,tegra186-mc.yaml    | 7 +++++++
  1 file changed, 7 insertions(+)

Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml +0 -0

index 935d63d181d9..398d27bb2373 100644
@@ -58,6 +58,10 @@ properties:
    "#interconnect-cells":
      const: 1
 

Quoted 6 lines

Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml properties: · line 63

Quoted 3 lines

Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml +3 -0

+  nvidia,bpmp:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: phandle of the node representing the BPMP

Why do you need this multiple times? Both in parent and all external-mc children?

We've had nvidia,bpmp in the external memory controller node since basically the beginning because we've always needed it there. For newer chips we now also need it for the memory controller.

Ideally I think we would only have this in the MC and have the EMC driver reference it via the EMC's parent (i.e. MC), but that would break backwards-compatibility. Reaching into the EMC's DT node from the MC was another option that we discussed internally, but it didn't look right given how this is also needed by the MC.

Quoted 5 lines

Quoted 3 lines

One thing we could potentially do is deprecate the nvidia,bpmp phandle in the EMC and only keep it as a fallback in the drivers in case the parent MC doesn't find it's own in the DT.

Yes, deprecation would answer to my question.

Okay, great. Sumit, you can resolve this by adding a "deprecated: true" to the EMC's nvidia,bpmp property schema. In the driver we can then try to look at the MC's ->bpmp and if it exists reuse that. If it doesn't exist, we can keep the existing lookup as a fallback for device trees that haven't been updated yet.

Quoted 5 lines

We can't use MC's->bpmp in the EMC driver's probe as it will be NULL. This is because MC driver uses "arch_initcall" and gets probed earlier than BPMP. We can do this in another way as below change. This way we can use the existing "nvidia,bpmp" property from EMC node and don't need to move it to the MC node. Please share if this change sounds OK.

Then rather it sounds like time to fix these
orderings/arch_initcall/missing defer.

For Tegra234, add the "nvidia,bpmp" property within the Memory Controller (MC) node to reference BPMP node. This is needed in the MC driver to pass the client info to the BPMP-FW when memory interconnect support is available.

Signed-off-by Sumit Gupta <[email protected]>

---
 .../bindings/memory-controllers/nvidia,tegra186-mc.yaml    | 7 +++++++
 1 file changed, 7 insertions(+)

Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml +4 -0

index 935d63d181d9..398d27bb2373 100644
@@ -58,6 +58,10 @@ properties:
   "#interconnect-cells":
     const: 1
 
+  nvidia,bpmp:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: phandle of the node representing the BPMP
+
 patternProperties:
   "^external-memory-controller@[0-9a-f]+$":
     description:

Add the "nvidia,bpmp" property within the "memory-controller" node to reference BPMP node. This is needed by the MC driver to pass the client info to the BPMP-FW when memory interconnect support is available.

Signed-off-by Sumit Gupta <[email protected]>

External email: Use caution opening links or attachments

Quoted 5 lines · Sumit Gupta

Add the "nvidia,bpmp" property within the "memory-controller" node to reference BPMP node. This is needed by the MC driver to pass the client info to the BPMP-FW when memory interconnect support is available.

Quoted 1 line

Signed-off-by Sumit Gupta <[email protected]>

DTS goes to the end of patchset. If you put it here to fix bisectability, then your patchset is already broken.

Best regards,
Krzysztof

Add Interconnect framework support to dynamically set the DRAM bandwidth from different clients. Both the MC and EMC drivers are added as ICC providers. The path for any request is:
MC-Client[1-n] -> MC -> EMC -> EMEM/DRAM

MC client's request for bandwidth will go to the MC driver which passes the client request info like BPMP Client ID, Client type and the Bandwidth to the BPMP-FW. The final DRAM freq to achieve the requested bandwidth is set by the BPMP-FW based on the passed parameters.

Signed-off-by Sumit Gupta <[email protected]>

---
 drivers/memory/tegra/mc.c           |   5 +
 drivers/memory/tegra/tegra186-emc.c | 125 ++++++++++++++++++++++++
 drivers/memory/tegra/tegra186.c     |   3 +
 drivers/memory/tegra/tegra234.c     | 143 +++++++++++++++++++++++++++-
 include/linux/tegra-icc.h           |  65 +++++++++++++
 include/soc/tegra/mc.h              |   7 ++
 6 files changed, 347 insertions(+), 1 deletion(-)
 create mode 100644 include/linux/tegra-icc.h

drivers/memory/tegra/mc.c +5 -0

index 9082b6c3763d..983455b1f98d 100644
@@ -15,6 +15,7 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/sort.h>
+#include <linux/tegra-icc.h>
 
 #include <soc/tegra/fuse.h>
 

@@ -792,6 +793,8 @@ static int tegra_mc_interconnect_setup(struct tegra_mc *mc)
 	mc->provider.data = &mc->provider;
 	mc->provider.set = mc->soc->icc_ops->set;
 	mc->provider.aggregate = mc->soc->icc_ops->aggregate;
+	mc->provider.get_bw = mc->soc->icc_ops->get_bw;
+	mc->provider.xlate = mc->soc->icc_ops->xlate;
 	mc->provider.xlate_extended = mc->soc->icc_ops->xlate_extended;
 
 	icc_provider_init(&mc->provider);

@@ -824,6 +827,8 @@ static int tegra_mc_interconnect_setup(struct tegra_mc *mc)
 		err = icc_link_create(node, TEGRA_ICC_MC);
 		if (err)
 			goto remove_nodes;
+
+		node->data = (struct tegra_mc_client *)&(mc->soc->clients[i]);
 	}
 
 	err = icc_provider_register(&mc->provider);

drivers/memory/tegra/tegra186-emc.c +83 -0

index e935ad4e95b6..1eefcf2ac0c7 100644
@@ -7,9 +7,11 @@
 #include <linux/debugfs.h>
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
+#include <linux/of_platform.h>
 #include <linux/platform_device.h>
 
 #include <soc/tegra/bpmp.h>
+#include "mc.h"
 
 struct tegra186_emc_dvfs {
 	unsigned long latency;

@@ -29,8 +31,15 @@ struct tegra186_emc {
 		unsigned long min_rate;
 		unsigned long max_rate;
 	} debugfs;
+
+	struct icc_provider provider;
 };
 
+static inline struct tegra186_emc *to_tegra186_emc(struct icc_provider *provider)
+{
+	return container_of(provider, struct tegra186_emc, provider);
+}
+
 /*
  * debugfs interface
  *

@@ -146,11 +155,104 @@ DEFINE_DEBUGFS_ATTRIBUTE(tegra186_emc_debug_max_rate_fops,
 			  tegra186_emc_debug_max_rate_get,
 			  tegra186_emc_debug_max_rate_set, "%llu\n");
 
+/*
+ * tegra_emc_icc_set_bw() - Set BW api for EMC provider
+ * @src: ICC node for External Memory Controller (EMC)
+ * @dst: ICC node for External Memory (DRAM)
+ *
+ * Do nothing here as info to BPMP-FW is now passed in the BW set function
+ * of the MC driver. BPMP-FW sets the final Freq based on the passed values.
+ */
+static int tegra_emc_icc_set_bw(struct icc_node *src, struct icc_node *dst)
+{
+	return 0;
+}
+
+static struct icc_node *
+tegra_emc_of_icc_xlate(struct of_phandle_args *spec, void *data)
+{
+	struct icc_provider *provider = data;
+	struct icc_node *node;
+
+	/* External Memory is the only possible ICC route */
+	list_for_each_entry(node, &provider->nodes, node_list) {
+		if (node->id != TEGRA_ICC_EMEM)
+			continue;
+
+		return node;
+	}
+
+	return ERR_PTR(-EPROBE_DEFER);
+}
+
+static int tegra_emc_icc_get_init_bw(struct icc_node *node, u32 *avg, u32 *peak)
+{
+	*avg = 0;
+	*peak = 0;
+
+	return 0;
+}
+
+static int tegra_emc_interconnect_init(struct tegra186_emc *emc)
+{
+	struct tegra_mc *mc = dev_get_drvdata(emc->dev->parent);
+	const struct tegra_mc_soc *soc = mc->soc;
+	struct icc_node *node;
+	int err;
+
+	emc->provider.dev = emc->dev;
+	emc->provider.set = tegra_emc_icc_set_bw;
+	emc->provider.data = &emc->provider;
+	emc->provider.aggregate = soc->icc_ops->aggregate;
+	emc->provider.xlate = tegra_emc_of_icc_xlate;
+	emc->provider.get_bw = tegra_emc_icc_get_init_bw;
+
+	icc_provider_init(&emc->provider);
+
+	/* create External Memory Controller node */
+	node = icc_node_create(TEGRA_ICC_EMC);
+	if (IS_ERR(node)) {
+		err = PTR_ERR(node);
+		goto err_msg;
+	}
+
+	node->name = "External Memory Controller";
+	icc_node_add(node, &emc->provider);
+
+	/* link External Memory Controller to External Memory (DRAM) */
+	err = icc_link_create(node, TEGRA_ICC_EMEM);
+	if (err)
+		goto remove_nodes;
+
+	/* create External Memory node */
+	node = icc_node_create(TEGRA_ICC_EMEM);
+	if (IS_ERR(node)) {
+		err = PTR_ERR(node);
+		goto remove_nodes;

drivers/memory/tegra/tegra186-emc.c +9 -0

+remove_nodes:
+	icc_nodes_remove(&emc->provider);
+err_msg:
+	dev_err(emc->dev, "failed to initialize ICC: %d\n", err);
+
+	return err;
+}
+
 static int tegra186_emc_probe(struct platform_device *pdev)
 {
 	struct mrq_emc_dvfs_latency_response response;
 	struct tegra_bpmp_message msg;
 	struct tegra186_emc *emc;
+	struct tegra_mc *mc;
 	unsigned int i;
 	int err;
 

drivers/memory/tegra/tegra186-emc.c +7 -0

+
 	emc->bpmp = tegra_bpmp_get(&pdev->dev);
 	if (IS_ERR(emc->bpmp))
 		return dev_err_probe(&pdev->dev, PTR_ERR(emc->bpmp), "failed to get BPMP\n");

@@ -236,6 +341,25 @@ static int tegra186_emc_probe(struct platform_device *pdev)
 	debugfs_create_file("max_rate", S_IRUGO | S_IWUSR, emc->debugfs.root,
 			    emc, &tegra186_emc_debug_max_rate_fops);
 
+	mc = dev_get_drvdata(emc->dev->parent);
+	if (mc && mc->soc->icc_ops) {
+		/*
+		 * Initialize the ICC even if BPMP-FW doesn't support 'MRQ_BWMGR_INT'.
+		 * Use the flag 'mc->bwmgr_mrq_supported' within MC driver and return
+		 * EINVAL instead of passing the request to BPMP-FW later when the BW

drivers/memory/tegra/tegra186-emc.c +3 -0

+	}
+
 	return 0;
 
 put_bpmp:

@@ -272,6 +396,7 @@ static struct platform_driver tegra186_emc_driver = {
 		.name = "tegra186-emc",
 		.of_match_table = tegra186_emc_of_match,
 		.suppress_bind_attrs = true,
+		.sync_state = icc_sync_state,
 	},
 	.probe = tegra186_emc_probe,
 	.remove = tegra186_emc_remove,

drivers/memory/tegra/tegra186.c +3 -0

index 7bb73f06fad3..386e029e41bb 100644
@@ -10,6 +10,7 @@
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 
+#include <soc/tegra/bpmp.h>
 #include <soc/tegra/mc.h>
 
 #if defined(CONFIG_ARCH_TEGRA_186_SOC)

@@ -65,6 +66,8 @@ static int tegra186_mc_probe(struct tegra_mc *mc)
 static void tegra186_mc_remove(struct tegra_mc *mc)
 {
 	of_platform_depopulate(mc->dev);
+
+	tegra_bpmp_put(mc->bpmp);
 }
 
 #if IS_ENABLED(CONFIG_IOMMU_API)

drivers/memory/tegra/tegra234.c +0 -0

index 02dcc5748bba..4f34247c9bda 100644

drivers/memory/tegra/tegra234.c +2 -0

+#include <linux/tegra-icc.h>
 
+#include <soc/tegra/bpmp.h>
 #include "mc.h"

On 27/03/2023 18:14, Sumit Gupta wrote:

Quoted 1 line

drivers/memory/tegra/tegra186-emc.c +0 -0

drivers/memory/tegra/tegra186-emc.c static int tegra186_emc_probe(struct platform_device *pdev) · line 264

Quoted 6 lines

drivers/memory/tegra/tegra186-emc.c +2 -0

@@ -158,6 +260,9 @@ static int tegra186_emc_probe(struct platform_device *pdev)
 	if (!emc)
 		return -ENOMEM;
 
+	platform_set_drvdata(pdev, emc);
+	emc->dev = &pdev->dev;

This patch looks like stiched from two or more patches... emc->dev does not look like new member of emc, thus why do you set in exisitng function in this patch? Why it wasn't needed before?

Quoted 13 lines

drivers/memory/tegra/tegra186-emc.c +6 -0

 	emc->bpmp = tegra_bpmp_get(&pdev->dev);
 	if (IS_ERR(emc->bpmp))
 		return dev_err_probe(&pdev->dev, PTR_ERR(emc->bpmp), "failed to get BPMP\n");

@@ -236,6 +341,25 @@ static int tegra186_emc_probe(struct platform_device *pdev)
 	debugfs_create_file("max_rate", S_IRUGO | S_IWUSR, emc->debugfs.root,
 			    emc, &tegra186_emc_debug_max_rate_fops);
 
+	mc = dev_get_drvdata(emc->dev->parent);
+	if (mc && mc->soc->icc_ops) {
+		/*
+		 * Initialize the ICC even if BPMP-FW doesn't support 'MRQ_BWMGR_INT'.
+		 * Use the flag 'mc->bwmgr_mrq_supported' within MC driver and return
+		 * EINVAL instead of passing the request to BPMP-FW later when the BW

drivers/memory/tegra/tegra186-emc.c static int tegra186_emc_probe(struct platform_device *pdev) · line 359

Quoted 10 lines

drivers/memory/tegra/tegra186-emc.c +10 -0

+		 * request is made by client with 'icc_set_bw()' call.
+		 */
+		err = tegra_emc_interconnect_init(emc);
+		if (err)
+			goto put_bpmp;
+
+		if (tegra_bpmp_mrq_is_supported(emc->bpmp, MRQ_BWMGR_INT))
+			mc->bwmgr_mrq_supported = true;
+		else
+

Drop blank line.

drivers/memory/tegra/tegra186-emc.c static int tegra186_emc_probe(struct platform_device *pdev) · line 360

Quoted 1 line

drivers/memory/tegra/tegra186-emc.c +1 -0

+			dev_info(&pdev->dev, "MRQ_BWMGR_INT not present\n");

And what user is supposed to do with this? Either make it descriptive or drop.

Quoted 1 line · Krzysztof Kozlowski

On 27/03/2023 18:14, Sumit Gupta wrote:

[...]

Quoted 1 line

Quoted 1 line

drivers/memory/tegra/tegra186-emc.c +0 -0

[...]

Quoted 10 lines

drivers/memory/tegra/tegra186-emc.c static int tegra186_emc_probe(struct platform_device *pdev) · line 264

Quoted 6 lines

drivers/memory/tegra/tegra186-emc.c +2 -0

@@ -158,6 +260,9 @@ static int tegra186_emc_probe(struct platform_device *pdev)
  	if (!emc)
  		return -ENOMEM;
 
+	platform_set_drvdata(pdev, emc);
+	emc->dev = &pdev->dev;

This patch looks like stiched from two or more patches... emc->dev does not look like new member of emc, thus why do you set in exisitng function in this patch? Why it wasn't needed before?

This looks like it may be leftover from some development. These two lines exist in this driver a few lines further down. Either one pair should be removed. I don't see why this would need to be moved, so probably the above additions can just be dropped.

Thierry

Quoted 18 lines

Quoted 18 lines

drivers/memory/tegra/tegra186-emc.c +11 -0

  	emc->bpmp = tegra_bpmp_get(&pdev->dev);
  	if (IS_ERR(emc->bpmp))
  		return dev_err_probe(&pdev->dev, PTR_ERR(emc->bpmp), "failed to get BPMP\n");

@@ -236,6 +341,25 @@ static int tegra186_emc_probe(struct platform_device *pdev)
  	debugfs_create_file("max_rate", S_IRUGO | S_IWUSR, emc->debugfs.root,
  			    emc, &tegra186_emc_debug_max_rate_fops);
 
+	mc = dev_get_drvdata(emc->dev->parent);
+	if (mc && mc->soc->icc_ops) {
+		/*
+		 * Initialize the ICC even if BPMP-FW doesn't support 'MRQ_BWMGR_INT'.
+		 * Use the flag 'mc->bwmgr_mrq_supported' within MC driver and return
+		 * EINVAL instead of passing the request to BPMP-FW later when the BW
+		 * request is made by client with 'icc_set_bw()' call.
+		 */
+		err = tegra_emc_interconnect_init(emc);
+		if (err)
+			goto put_bpmp;

Quoted 8 lines

drivers/memory/tegra/tegra186-emc.c static int tegra186_emc_probe(struct platform_device *pdev) · line 359

Quoted 5 lines

drivers/memory/tegra/tegra186-emc.c +5 -0

+
+		if (tegra_bpmp_mrq_is_supported(emc->bpmp, MRQ_BWMGR_INT))
+			mc->bwmgr_mrq_supported = true;
+		else
+

Drop blank line.

Quoted 4 lines

drivers/memory/tegra/tegra186-emc.c static int tegra186_emc_probe(struct platform_device *pdev) · line 360

Quoted 1 line

drivers/memory/tegra/tegra186-emc.c +1 -0

+			dev_info(&pdev->dev, "MRQ_BWMGR_INT not present\n");

And what user is supposed to do with this? Either make it descriptive or drop.

Agreed. I think we can just drop this. If the intention was to provide a quick way for people to detect whether BWMGR is available or not, using something from sysfs/debugfs would be preferable.

Thierry

External email: Use caution opening links or attachments

Quoted 162 lines · Sumit Gupta

Add Interconnect framework support to dynamically set the DRAM bandwidth from different clients. Both the MC and EMC drivers are added as ICC providers. The path for any request is:
MC-Client[1-n] -> MC -> EMC -> EMEM/DRAM

MC client's request for bandwidth will go to the MC driver which passes the client request info like BPMP Client ID, Client type and the Bandwidth to the BPMP-FW. The final DRAM freq to achieve the requested bandwidth is set by the BPMP-FW based on the passed parameters.

Signed-off-by Sumit Gupta <[email protected]>

---
  drivers/memory/tegra/mc.c           |   5 +
  drivers/memory/tegra/tegra186-emc.c | 125 ++++++++++++++++++++++++
  drivers/memory/tegra/tegra186.c     |   3 +
  drivers/memory/tegra/tegra234.c     | 143 +++++++++++++++++++++++++++-
  include/linux/tegra-icc.h           |  65 +++++++++++++
  include/soc/tegra/mc.h              |   7 ++
  6 files changed, 347 insertions(+), 1 deletion(-)

create mode 100644 include/linux/tegra-icc.h

drivers/memory/tegra/mc.c +5 -0

index 9082b6c3763d..983455b1f98d 100644
@@ -15,6 +15,7 @@
  #include <linux/platform_device.h>
  #include <linux/slab.h>
  #include <linux/sort.h>
+#include <linux/tegra-icc.h>
 
  #include <soc/tegra/fuse.h>
 

@@ -792,6 +793,8 @@ static int tegra_mc_interconnect_setup(struct tegra_mc *mc)
       mc->provider.data = &mc->provider;
       mc->provider.set = mc->soc->icc_ops->set;
       mc->provider.aggregate = mc->soc->icc_ops->aggregate;
+     mc->provider.get_bw = mc->soc->icc_ops->get_bw;
+     mc->provider.xlate = mc->soc->icc_ops->xlate;
       mc->provider.xlate_extended = mc->soc->icc_ops->xlate_extended;
 
       icc_provider_init(&mc->provider);

@@ -824,6 +827,8 @@ static int tegra_mc_interconnect_setup(struct tegra_mc *mc)
               err = icc_link_create(node, TEGRA_ICC_MC);
               if (err)
                       goto remove_nodes;
+
+             node->data = (struct tegra_mc_client *)&(mc->soc->clients[i]);
       }
 
       err = icc_provider_register(&mc->provider);

drivers/memory/tegra/tegra186-emc.c +83 -0

index e935ad4e95b6..1eefcf2ac0c7 100644
@@ -7,9 +7,11 @@
  #include <linux/debugfs.h>
  #include <linux/module.h>
  #include <linux/mod_devicetable.h>
+#include <linux/of_platform.h>
  #include <linux/platform_device.h>
 
  #include <soc/tegra/bpmp.h>
+#include "mc.h"
 
  struct tegra186_emc_dvfs {
       unsigned long latency;

@@ -29,8 +31,15 @@ struct tegra186_emc {
               unsigned long min_rate;
               unsigned long max_rate;
       } debugfs;
+
+     struct icc_provider provider;
  };
 
+static inline struct tegra186_emc *to_tegra186_emc(struct icc_provider *provider)
+{
+     return container_of(provider, struct tegra186_emc, provider);
+}
+
  /*
   * debugfs interface
   *

@@ -146,11 +155,104 @@ DEFINE_DEBUGFS_ATTRIBUTE(tegra186_emc_debug_max_rate_fops,
                         tegra186_emc_debug_max_rate_get,
                         tegra186_emc_debug_max_rate_set, "%llu\n");
 
+/*
+ * tegra_emc_icc_set_bw() - Set BW api for EMC provider
+ * @src: ICC node for External Memory Controller (EMC)
+ * @dst: ICC node for External Memory (DRAM)
+ *
+ * Do nothing here as info to BPMP-FW is now passed in the BW set function
+ * of the MC driver. BPMP-FW sets the final Freq based on the passed values.
+ */
+static int tegra_emc_icc_set_bw(struct icc_node *src, struct icc_node *dst)
+{
+     return 0;
+}
+
+static struct icc_node *
+tegra_emc_of_icc_xlate(struct of_phandle_args *spec, void *data)
+{
+     struct icc_provider *provider = data;
+     struct icc_node *node;
+
+     /* External Memory is the only possible ICC route */
+     list_for_each_entry(node, &provider->nodes, node_list) {
+             if (node->id != TEGRA_ICC_EMEM)
+                     continue;
+
+             return node;
+     }
+
+     return ERR_PTR(-EPROBE_DEFER);
+}
+
+static int tegra_emc_icc_get_init_bw(struct icc_node *node, u32 *avg, u32 *peak)
+{
+     *avg = 0;
+     *peak = 0;
+
+     return 0;
+}
+
+static int tegra_emc_interconnect_init(struct tegra186_emc *emc)
+{
+     struct tegra_mc *mc = dev_get_drvdata(emc->dev->parent);
+     const struct tegra_mc_soc *soc = mc->soc;
+     struct icc_node *node;
+     int err;
+
+     emc->provider.dev = emc->dev;
+     emc->provider.set = tegra_emc_icc_set_bw;
+     emc->provider.data = &emc->provider;
+     emc->provider.aggregate = soc->icc_ops->aggregate;
+     emc->provider.xlate = tegra_emc_of_icc_xlate;
+     emc->provider.get_bw = tegra_emc_icc_get_init_bw;
+
+     icc_provider_init(&emc->provider);
+
+     /* create External Memory Controller node */
+     node = icc_node_create(TEGRA_ICC_EMC);
+     if (IS_ERR(node)) {
+             err = PTR_ERR(node);
+             goto err_msg;
+     }
+
+     node->name = "External Memory Controller";
+     icc_node_add(node, &emc->provider);
+
+     /* link External Memory Controller to External Memory (DRAM) */
+     err = icc_link_create(node, TEGRA_ICC_EMEM);
+     if (err)
+             goto remove_nodes;
+
+     /* create External Memory node */
+     node = icc_node_create(TEGRA_ICC_EMEM);
+     if (IS_ERR(node)) {
+             err = PTR_ERR(node);
+             goto remove_nodes;

drivers/memory/tegra/tegra186-emc.c DEFINE_DEBUGFS_ATTRIBUTE(tegra186_emc_debug_max_rate_fops, · line 241

Quoted 10 lines

drivers/memory/tegra/tegra186-emc.c +10 -0

+     }
+
+     node->name = "External Memory (DRAM)";
+     icc_node_add(node, &emc->provider);
+
+     err = icc_provider_register(&emc->provider);
+     if (err)
+             goto remove_nodes;
+
+     return 0;

Blank line

Quoted 21 lines

drivers/memory/tegra/tegra186-emc.c +9 -0

+remove_nodes:
+     icc_nodes_remove(&emc->provider);
+err_msg:
+     dev_err(emc->dev, "failed to initialize ICC: %d\n", err);
+
+     return err;
+}
+
  static int tegra186_emc_probe(struct platform_device *pdev)
  {
       struct mrq_emc_dvfs_latency_response response;
       struct tegra_bpmp_message msg;
       struct tegra186_emc *emc;
+     struct tegra_mc *mc;
       unsigned int i;
       int err;
 

@@ -158,6 +260,9 @@ static int tegra186_emc_probe(struct platform_device *pdev)
       if (!emc)
               return -ENOMEM;
 

drivers/memory/tegra/tegra186-emc.c static int tegra186_emc_probe(struct platform_device *pdev) · line 264

Quoted 2 lines

drivers/memory/tegra/tegra186-emc.c +2 -0

+     platform_set_drvdata(pdev, emc);
+     emc->dev = &pdev->dev;

This patch looks like stiched from two or more patches... emc->dev does not look like new member of emc, thus why do you set in exisitng function in this patch? Why it wasn't needed before?

Same about line before.

Quoted 14 lines

drivers/memory/tegra/tegra186-emc.c +7 -0

+
       emc->bpmp = tegra_bpmp_get(&pdev->dev);
       if (IS_ERR(emc->bpmp))
               return dev_err_probe(&pdev->dev, PTR_ERR(emc->bpmp), "failed to get BPMP\n");

@@ -236,6 +341,25 @@ static int tegra186_emc_probe(struct platform_device *pdev)
       debugfs_create_file("max_rate", S_IRUGO | S_IWUSR, emc->debugfs.root,
                           emc, &tegra186_emc_debug_max_rate_fops);
 
+     mc = dev_get_drvdata(emc->dev->parent);
+     if (mc && mc->soc->icc_ops) {
+             /*
+              * Initialize the ICC even if BPMP-FW doesn't support 'MRQ_BWMGR_INT'.
+              * Use the flag 'mc->bwmgr_mrq_supported' within MC driver and return
+              * EINVAL instead of passing the request to BPMP-FW later when the BW

drivers/memory/tegra/tegra186-emc.c static int tegra186_emc_probe(struct platform_device *pdev) · line 359

Quoted 10 lines

drivers/memory/tegra/tegra186-emc.c +10 -0

+              * request is made by client with 'icc_set_bw()' call.
+              */
+             err = tegra_emc_interconnect_init(emc);
+             if (err)
+                     goto put_bpmp;
+
+             if (tegra_bpmp_mrq_is_supported(emc->bpmp, MRQ_BWMGR_INT))
+                     mc->bwmgr_mrq_supported = true;
+             else
+

Drop blank line.

drivers/memory/tegra/tegra186-emc.c static int tegra186_emc_probe(struct platform_device *pdev) · line 360

Quoted 1 line

drivers/memory/tegra/tegra186-emc.c +1 -0

+                     dev_info(&pdev->dev, "MRQ_BWMGR_INT not present\n");

And what user is supposed to do with this? Either make it descriptive or drop.

Quoted 33 lines

drivers/memory/tegra/tegra186-emc.c +3 -0

+     }
+
       return 0;
 
  put_bpmp:

@@ -272,6 +396,7 @@ static struct platform_driver tegra186_emc_driver = {
               .name = "tegra186-emc",
               .of_match_table = tegra186_emc_of_match,
               .suppress_bind_attrs = true,
+             .sync_state = icc_sync_state,
       },
       .probe = tegra186_emc_probe,
       .remove = tegra186_emc_remove,

drivers/memory/tegra/tegra186.c +3 -0

index 7bb73f06fad3..386e029e41bb 100644
@@ -10,6 +10,7 @@
  #include <linux/of_device.h>
  #include <linux/platform_device.h>
 
+#include <soc/tegra/bpmp.h>
  #include <soc/tegra/mc.h>
 
  #if defined(CONFIG_ARCH_TEGRA_186_SOC)

@@ -65,6 +66,8 @@ static int tegra186_mc_probe(struct tegra_mc *mc)
  static void tegra186_mc_remove(struct tegra_mc *mc)
  {
       of_platform_depopulate(mc->dev);
+
+     tegra_bpmp_put(mc->bpmp);
  }
 

drivers/memory/tegra/tegra234.c line 3

Quoted 10 lines

drivers/memory/tegra/tegra186.c +0 -0

  #if IS_ENABLED(CONFIG_IOMMU_API)

drivers/memory/tegra/tegra234.c +1 -1

index 02dcc5748bba..4f34247c9bda 100644
@@ -1,18 +1,24 @@
  // SPDX-License-Identifier: GPL-2.0-only
  /*
- * Copyright (C) 2021-2022, NVIDIA CORPORATION.  All rights reserved.
+ * Copyright (C) 20212-2023, NVIDIA CORPORATION.  All rights reserved.

Typo, 2021.

drivers/memory/tegra/tegra234.c line 10

Quoted 7 lines

drivers/memory/tegra/tegra234.c +2 -0

   */
 
  #include <soc/tegra/mc.h>
 
  #include <dt-bindings/memory/tegra234-mc.h>
+#include <linux/interconnect.h>
+#include <linux/of_device.h>

One more suprising change...

Quoted 4 lines

drivers/memory/tegra/tegra234.c +2 -0

+#include <linux/tegra-icc.h>
 
+#include <soc/tegra/bpmp.h>
  #include "mc.h"

drivers/memory/tegra/tegra234.c line 24

Quoted 10 lines

drivers/memory/tegra/tegra234.c +2 -0

 
  static const struct tegra_mc_client tegra234_mc_clients[] = {
       {
               .id = TEGRA234_MEMORY_CLIENT_MGBEARD,
               .name = "mgbeard",
+             .bpmp_id = TEGRA_ICC_BPMP_EQOS,
+             .type = TEGRA_ICC_NISO,
               .sid = TEGRA234_SID_MGBE,
               .regs = {
                       .sid = {

Best regards,
Krzysztof

Add support to request DRAM bandwidth with Memory Interconnect in Tegra234 SoC. The DRAM BW required for different modes depends on speed (Gen-1/2/3/4) and width/lanes (x1/x2/x4/x8).

Suggested-by Manikanta Maddireddy <[email protected]>

Signed-off-by Sumit Gupta <[email protected]>

---
 drivers/pci/controller/dwc/pcie-tegra194.c | 40 +++++++++++++++++-----
 1 file changed, 32 insertions(+), 8 deletions(-)

drivers/pci/controller/dwc/pcie-tegra194.c +0 -0

index 09825b4a075e..d2513c9d3feb 100644

drivers/pci/controller/dwc/pcie-tegra194.c +7 -0

 #include <linux/iopoll.h>
 #include <linux/kernel.h>
 #include <linux/module.h>

@@ -287,6 +288,7 @@ struct tegra_pcie_dw {
 	unsigned int pex_rst_irq;
 	int ep_state;
 	long link_status;
+	struct icc_path *icc_path;
 };
 
 static inline struct tegra_pcie_dw *to_tegra_pcie(struct dw_pcie *pci)

@@ -309,6 +311,24 @@ struct tegra_pcie_soc {
 	enum dw_pcie_device_mode mode;
 };
 
+static void tegra_pcie_icc_set(struct tegra_pcie_dw *pcie)
+{
+	struct dw_pcie *pci = &pcie->pci;
+	u32 val, speed, width;
+
+	val = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKSTA);

drivers/pci/controller/dwc/pcie-tegra194.c +4 -4

+}
+
 static void apply_bad_link_workaround(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);

@@ -452,14 +472,12 @@ static irqreturn_t tegra_pcie_ep_irq_thread(int irq, void *arg)
 	struct tegra_pcie_dw *pcie = arg;
 	struct dw_pcie_ep *ep = &pcie->pci.ep;
 	struct dw_pcie *pci = &pcie->pci;
-	u32 val, speed;
+	u32 val;
 
 	if (test_and_clear_bit(0, &pcie->link_status))
 		dw_pcie_ep_linkup(ep);
 
-	speed = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKSTA) &
-		PCI_EXP_LNKSTA_CLS;
-	clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
+	tegra_pcie_icc_set(pcie);

External email: Use caution opening links or attachments

Capitalize subject line please, to match pcie-tegra194.c history.

Quoted 10 lines · Sumit Gupta

Add support to request DRAM bandwidth with Memory Interconnect in Tegra234 SoC. The DRAM BW required for different modes depends on speed (Gen-1/2/3/4) and width/lanes (x1/x2/x4/x8).

Suggested-by Manikanta Maddireddy <[email protected]>

Signed-off-by Sumit Gupta <[email protected]>

---
  drivers/pci/controller/dwc/pcie-tegra194.c | 40 +++++++++++++++++-----
  1 file changed, 32 insertions(+), 8 deletions(-)

drivers/pci/controller/dwc/pcie-tegra194.c line 18

Quoted 9 lines

drivers/pci/controller/dwc/pcie-tegra194.c +1 -0

index 09825b4a075e..d2513c9d3feb 100644
@@ -15,6 +15,7 @@
  #include <linux/gpio.h>
  #include <linux/gpio/consumer.h>
  #include <linux/interrupt.h>
+#include <linux/interconnect.h>

Almost alphabetized, swap interrupt.h and interconnect.h.

Quoted 21 lines

drivers/pci/controller/dwc/pcie-tegra194.c +7 -0

  #include <linux/iopoll.h>
  #include <linux/kernel.h>
  #include <linux/module.h>

@@ -287,6 +288,7 @@ struct tegra_pcie_dw {
       unsigned int pex_rst_irq;
       int ep_state;
       long link_status;
+     struct icc_path *icc_path;
  };
 
  static inline struct tegra_pcie_dw *to_tegra_pcie(struct dw_pcie *pci)

@@ -309,6 +311,24 @@ struct tegra_pcie_soc {
       enum dw_pcie_device_mode mode;
  };
 
+static void tegra_pcie_icc_set(struct tegra_pcie_dw *pcie)
+{
+     struct dw_pcie *pci = &pcie->pci;
+     u32 val, speed, width;
+
+     val = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKSTA);

drivers/pci/controller/dwc/pcie-tegra194.c struct tegra_pcie_soc { · line 329

Quoted 10 lines

drivers/pci/controller/dwc/pcie-tegra194.c +10 -0

+
+     speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val);
+     width = FIELD_GET(PCI_EXP_LNKSTA_NLW, val);
+
+     val = width * (PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]) / BITS_PER_BYTE);
+
+     if (icc_set_bw(pcie->icc_path, MBps_to_icc(val), 0))
+             dev_err(pcie->dev, "can't set bw[%u]\n", val);
+
+     clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);

Array bounds violation; PCI_EXP_LNKSTA_CLS is 0x000f, so possible speed (CLS) values are 0..0xf and "speed - 1" values are -1..0xe.

pcie_gen_freq[] is of size 4 (valid indices 0..3).

I see that you're just *moving* this code, but might as well fix it.

Quoted 19 lines

drivers/pci/controller/dwc/pcie-tegra194.c +4 -4

+}
+
  static void apply_bad_link_workaround(struct dw_pcie_rp *pp)
  {
       struct dw_pcie *pci = to_dw_pcie_from_pp(pp);

@@ -452,14 +472,12 @@ static irqreturn_t tegra_pcie_ep_irq_thread(int irq, void *arg)
       struct tegra_pcie_dw *pcie = arg;
       struct dw_pcie_ep *ep = &pcie->pci.ep;
       struct dw_pcie *pci = &pcie->pci;
-     u32 val, speed;
+     u32 val;
 
       if (test_and_clear_bit(0, &pcie->link_status))
               dw_pcie_ep_linkup(ep);
 
-     speed = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKSTA) &
-             PCI_EXP_LNKSTA_CLS;
-     clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
+     tegra_pcie_icc_set(pcie);

Quoted 20 lines · Bjorn Helgaas

Quoted 6 lines

drivers/pci/controller/dwc/pcie-tegra194.c +6 -0

+static void tegra_pcie_icc_set(struct tegra_pcie_dw *pcie)
+{
+     struct dw_pcie *pci = &pcie->pci;
+     u32 val, speed, width;
+
+     val = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKSTA);

drivers/pci/controller/dwc/pcie-tegra194.c struct tegra_pcie_soc { · line 329

Quoted 10 lines

drivers/pci/controller/dwc/pcie-tegra194.c +10 -0

+
+     speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val);
+     width = FIELD_GET(PCI_EXP_LNKSTA_NLW, val);
+
+     val = width * (PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]) / BITS_PER_BYTE);
+
+     if (icc_set_bw(pcie->icc_path, MBps_to_icc(val), 0))
+             dev_err(pcie->dev, "can't set bw[%u]\n", val);
+
+     clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);

Array bounds violation; PCI_EXP_LNKSTA_CLS is 0x000f, so possible speed (CLS) values are 0..0xf and "speed - 1" values are -1..0xe.

Quoted 4 lines

pcie_gen_freq[] is of size 4 (valid indices 0..3).

I see that you're just *moving* this code, but might as well fix it.

Thank you for the review.
Will include the below change in the same patch. Please let me know if any issue.

- clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
+ if (speed && (speed <= ARRAY_SIZE(pcie_gen_freq)))
+ clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
+ else
+ clk_set_rate(pcie->core_clk, pcie_gen_freq[0]);

External email: Use caution opening links or attachments

Quoted 29 lines · Sumit Gupta

Quoted 20 lines · Bjorn Helgaas

Quoted 6 lines

drivers/pci/controller/dwc/pcie-tegra194.c +6 -0

+static void tegra_pcie_icc_set(struct tegra_pcie_dw *pcie)
+{
+     struct dw_pcie *pci = &pcie->pci;
+     u32 val, speed, width;
+
+     val = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKSTA);

drivers/pci/controller/dwc/pcie-tegra194.c struct tegra_pcie_soc { · line 329

Quoted 10 lines

drivers/pci/controller/dwc/pcie-tegra194.c +10 -0

+
+     speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val);
+     width = FIELD_GET(PCI_EXP_LNKSTA_NLW, val);
+
+     val = width * (PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]) / BITS_PER_BYTE);
+
+     if (icc_set_bw(pcie->icc_path, MBps_to_icc(val), 0))
+             dev_err(pcie->dev, "can't set bw[%u]\n", val);
+
+     clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);

Array bounds violation; PCI_EXP_LNKSTA_CLS is 0x000f, so possible speed (CLS) values are 0..0xf and "speed - 1" values are -1..0xe.

Quoted 4 lines

pcie_gen_freq[] is of size 4 (valid indices 0..3).

I see that you're just *moving* this code, but might as well fix it.

Thank you for the review.
Will include the below change in the same patch. Please let me know if any issue.

Quoted 5 lines

- clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
+ if (speed && (speed <= ARRAY_SIZE(pcie_gen_freq)))
+ clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
+ else
+ clk_set_rate(pcie->core_clk, pcie_gen_freq[0]);

I didn't notice that speed is a u32, so -1 is not a possible value. Also, it's used earlier for PCIE_SPEED2MBS_ENC(), so you could do something like this:

   speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val) - 1;
   if (speed >= ARRAY_SIZE(pcie_gen_freq))
     speed = 0;

   val = width * (PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]) /
         BITS_PER_BYTE);
   ...
   clk_set_rate(pcie->core_clk, pcie_gen_freq[speed]);

Quoted 44 lines · Bjorn Helgaas

Quoted 29 lines · Sumit Gupta

Quoted 20 lines · Bjorn Helgaas

Quoted 6 lines

drivers/pci/controller/dwc/pcie-tegra194.c +6 -0

+static void tegra_pcie_icc_set(struct tegra_pcie_dw *pcie)
+{
+     struct dw_pcie *pci = &pcie->pci;
+     u32 val, speed, width;
+
+     val = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKSTA);

drivers/pci/controller/dwc/pcie-tegra194.c struct tegra_pcie_soc { · line 329

Quoted 10 lines

drivers/pci/controller/dwc/pcie-tegra194.c +10 -0

+
+     speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val);
+     width = FIELD_GET(PCI_EXP_LNKSTA_NLW, val);
+
+     val = width * (PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]) / BITS_PER_BYTE);
+
+     if (icc_set_bw(pcie->icc_path, MBps_to_icc(val), 0))
+             dev_err(pcie->dev, "can't set bw[%u]\n", val);
+
+     clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);

Array bounds violation; PCI_EXP_LNKSTA_CLS is 0x000f, so possible speed (CLS) values are 0..0xf and "speed - 1" values are -1..0xe.

Quoted 4 lines

pcie_gen_freq[] is of size 4 (valid indices 0..3).

I see that you're just *moving* this code, but might as well fix it.

Thank you for the review.
Will include the below change in the same patch. Please let me know if any issue.

Quoted 5 lines

- clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
+ if (speed && (speed <= ARRAY_SIZE(pcie_gen_freq)))
+ clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
+ else
+ clk_set_rate(pcie->core_clk, pcie_gen_freq[0]);

I didn't notice that speed is a u32, so -1 is not a possible value. Also, it's used earlier for PCIE_SPEED2MBS_ENC(), so you could do something like this:

   speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val) - 1;
   if (speed >= ARRAY_SIZE(pcie_gen_freq))
     speed = 0;

Quoted 4 lines

   val = width * (PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]) /
         BITS_PER_BYTE);
   ...
   clk_set_rate(pcie->core_clk, pcie_gen_freq[speed]);

I tried this change but PCIE_SPEED2MBS_ENC gives zero when speed value is one. The speed value ranges from "1 to 4" and for value "1", pcie_link_speed[speed] gives '0xff'.

The below change works fine. Please share if its OK to add it in patch.

  speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val);
  if (!speed || speed >= ARRAY_SIZE(pcie_gen_freq))
          speed = 1;

val = width * (PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]) /
BITS_PER_BYTE);

  if (icc_set_bw(pcie->icc_path, MBps_to_icc(val), 0))
          dev_err(pcie->dev, "can't set bw[%u]\n", val);

  clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);

External email: Use caution opening links or attachments

Quoted 45 lines · Sumit Gupta

Quoted 44 lines · Bjorn Helgaas

Quoted 29 lines · Sumit Gupta

Quoted 20 lines · Bjorn Helgaas

Quoted 6 lines

drivers/pci/controller/dwc/pcie-tegra194.c +6 -0

+static void tegra_pcie_icc_set(struct tegra_pcie_dw *pcie)
+{
+     struct dw_pcie *pci = &pcie->pci;
+     u32 val, speed, width;
+
+     val = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKSTA);

drivers/pci/controller/dwc/pcie-tegra194.c struct tegra_pcie_soc { · line 329

Quoted 10 lines

drivers/pci/controller/dwc/pcie-tegra194.c +10 -0

+
+     speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val);
+     width = FIELD_GET(PCI_EXP_LNKSTA_NLW, val);
+
+     val = width * (PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]) / BITS_PER_BYTE);
+
+     if (icc_set_bw(pcie->icc_path, MBps_to_icc(val), 0))
+             dev_err(pcie->dev, "can't set bw[%u]\n", val);
+
+     clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);

Array bounds violation; PCI_EXP_LNKSTA_CLS is 0x000f, so possible speed (CLS) values are 0..0xf and "speed - 1" values are -1..0xe.

Quoted 4 lines

pcie_gen_freq[] is of size 4 (valid indices 0..3).

I see that you're just *moving* this code, but might as well fix it.

Thank you for the review.
Will include the below change in the same patch. Please let me know if any issue.

Quoted 5 lines

- clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
+ if (speed && (speed <= ARRAY_SIZE(pcie_gen_freq)))
+ clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
+ else
+ clk_set_rate(pcie->core_clk, pcie_gen_freq[0]);

I didn't notice that speed is a u32, so -1 is not a possible value. Also, it's used earlier for PCIE_SPEED2MBS_ENC(), so you could do something like this:

    speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val) - 1;
    if (speed >= ARRAY_SIZE(pcie_gen_freq))
      speed = 0;

Quoted 8 lines

Quoted 4 lines

    val = width * (PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]) /
          BITS_PER_BYTE);
    ...
    clk_set_rate(pcie->core_clk, pcie_gen_freq[speed]);

I tried this change but PCIE_SPEED2MBS_ENC gives zero when speed value is one. The speed value ranges from "1 to 4" and for value "1", pcie_link_speed[speed] gives '0xff'.

Oh, my fault, sorry! I thought both places indexed the same array, but the first is pcie_link_speed[] (where all the possible values (0..0xf) are valid indices) and the second is pcie_gen_freq[] (where only 0..3 are valid).

Quoted 8 lines

The below change works fine. Please share if its OK to add it in patch.

   speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val);
   if (!speed || speed >= ARRAY_SIZE(pcie_gen_freq))
           speed = 1;

val = width * (PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]) /
BITS_PER_BYTE);

So I don't think you need to clamp "speed" for indexing pcie_link_speed[] at all.

Quoted 4 lines

   if (icc_set_bw(pcie->icc_path, MBps_to_icc(val), 0))
           dev_err(pcie->dev, "can't set bw[%u]\n", val);

   clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);

What if you added a 0th entry to pcie_gen_freq[] so you can index it directly with the PCI_EXP_LNKSTA_CLS value the same way as pcie_link_speed[]? Then you wouldn't need the "- 1" and only have to worry about going off the end:

   static const unsigned int pcie_gen_freq[] = {
     GEN1_CORE_CLK_FREQ,   /* PCI_EXP_LNKSTA_CLS == 0; undefined */
     GEN1_CORE_CLK_FREQ,
     GEN2_CORE_CLK_FREQ,
     GEN3_CORE_CLK_FREQ,
     GEN4_CORE_CLK_FREQ,
   };

   speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val);

   val = width * (PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]) /
         BITS_PER_BYTE);

   if (speed >= ARRAY_SIZE(pcie_gen_freq))
     speed = 0;
   clk_set_rate(pcie->core_clk, pcie_gen_freq[speed]);

Bjorn