[Patch v4 00/10] Tegra234 Memory interconnect support

[Patch v4 00/10] Tegra234 Memory interconnect support

[Sumit Gupta]

Hi All,

Thank you for the review. Have incorporated the changes suggested in v3.
Requesting for ACK on the remaining patches if all fine.

The patches are divided into below groups:

- Patch [10]  : Memory Interconnect support in PCI (MC client)
  -- Need ACK.

- Patch [4-9]: Memory Interconnect support in CPUFREQ (MC client)
  -- 'Patch 5, 6 & 9' have ACK provided in v3 from Krzysztof.
  -- 'Patch 7 & 8' need ACK. 

- Patch [3-4]: Memory Interconnect base support
  -- 'Patch 4' has ACK provided in v3 from Krzysztof.
  -- 'Patch 3' removed ACK from Krzysztof as did suggested changes.

- Patch [1-2]: DT binding patch
  -- So, need ACK on 'Patch 1,2 & 3'.

Both the Memory Controller (MC) client patches are dependent on the 
'Patch [1-4]'.

Thank you,
Sumit Gupta
============

This patch series adds memory interconnect support for Tegra234 SoC.
It is used to dynamically scale DRAM Frequency as per the bandwidth
requests from different Memory Controller (MC) clients.
MC Clients use ICC Framework's icc_set_bw() api to dynamically request
for the DRAM bandwidth (BW). As per path, the request will be routed
from MC to the EMC driver. MC driver passes the request info like the
Client ID, type, and frequency request info to the BPMP-FW which will
set the final DRAM freq considering all exisiting requests.

MC and EMC are the ICC providers. Nodes in path for a request will be:
     Client[1-n] -> MC -> EMC -> EMEM/DRAM

The patch series also adds interconnect support in below clients:
1) CPUFREQ driver for scaling bandwidth with CPU frequency. For that,
   add per cluster OPP table which will be used in the CPUFREQ driver
   by requesting the minimum BW respective to the given CPU frequency
   in the OPP table of it's cluster.
2) PCIE driver to request BW required for different modes.

---
v3[3] -> v4:
- dropped 'patch 1' from v3 which returns 'struct tegra_bpmp *'.
- added 'patch 1 & 2' to get bpmp ref using 'nvidia,bpmp' property.
- dropped 'patch 10 & 11' from v3 and added those changes in 'patch 3'.
- added static to prototype of 'tegra_cpufreq_init_cpufreq_table()' to
  fix the warning reported by 'kernel test robot'.

v2[2] -> v3:
- in 'patch 7', set 'icc_dram_bw_scaling' to false if set_opp call failed
  to avoid flooding of uart with 'Failed to set bw' messages.
- added 'patch 10' to handle if the bpmp-fw is old and not support bwmgr mrq.
- added 'patch 11' to fix interconnect registration in tegra186-emc.
  ref patch link in linux next:
  [https://lore.kernel.org/all/20230306075651.2449-21-johan+linaro@kernel.org/]

v1[1] -> v2:
- moved BW setting to tegra234_mc_icc_set() from EMC driver.
- moved sw clients to the 'tegra_mc_clients' table.
- point 'node->data' to the entry within 'tegra_mc_clients'.
- removed 'struct tegra_icc_node' and get client info using 'node->data'.
- changed error handling in and around tegra_emc_interconnect_init().
- moved 'tegra-icc.h' from 'include/soc/tegra' to 'include/linux'.
- added interconnect support to PCIE driver in 'Patch 9'.
- merged 'Patch 9 & 10' from [1] to get num_channels and use.
- merged 'Patch 2 & 3' from [1] to add ISO and NISO clients.
- added 'Acked-by' of Krzysztof from 'Patch 05/10' of [1].
- Removed 'Patch 7' from [1] as that is merged now.

Sumit Gupta (10):
  dt-bindings: memory: tegra: add bpmp ref in tegra234-mc node
  arm64: tegra: add bpmp ref in tegra234-mc node
  memory: tegra: add interconnect support for DRAM scaling in Tegra234
  memory: tegra: add mc clients for Tegra234
  memory: tegra: add software mc clients in Tegra234
  dt-bindings: tegra: add icc ids for dummy MC clients
  arm64: tegra: Add cpu OPP tables and interconnects property
  cpufreq: tegra194: add OPP support and set bandwidth
  memory: tegra: make cpu cluster bw request a multiple of mc channels
  PCI: tegra194: add interconnect support in Tegra234

 .../nvidia,tegra186-mc.yaml                   |   7 +
 arch/arm64/boot/dts/nvidia/tegra234.dtsi      | 278 ++++++++
 drivers/cpufreq/tegra194-cpufreq.c            | 156 ++++-
 drivers/memory/tegra/mc.c                     |  24 +
 drivers/memory/tegra/mc.h                     |   1 +
 drivers/memory/tegra/tegra186-emc.c           | 125 ++++
 drivers/memory/tegra/tegra186.c               |   3 +
 drivers/memory/tegra/tegra234.c               | 600 +++++++++++++++++-
 drivers/pci/controller/dwc/pcie-tegra194.c    |  40 +-
 include/dt-bindings/memory/tegra234-mc.h      |   5 +
 include/linux/tegra-icc.h                     |  65 ++
 include/soc/tegra/mc.h                        |   8 +
 12 files changed, 1290 insertions(+), 22 deletions(-)
 create mode 100644 include/linux/tegra-icc.h

[1] https://lore.kernel.org/lkml/20221220160240.27494-1-sumitg@nvidia.com/T/
[2] https://lore.kernel.org/linux-tegra/20230220140559.28289-1-sumitg@nvidia.com/
[3] https://lore.kernel.org/lkml/20230320182441.11904-3-sumitg@nvidia.com/T/

-- 
2.17.1

[Patch v4 01/10] dt-bindings: memory: tegra: add bpmp ref in tegra234-mc node

[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 <sumitg@nvidia.com>
---
 .../bindings/memory-controllers/nvidia,tegra186-mc.yaml    | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
index 935d63d181d9..398d27bb2373 100644
--- a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
@@ -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:
@@ -220,6 +224,9 @@ allOf:
             - const: ch14
             - const: ch15
 
+        nvidia,bpmp:
+          description: phandle of the node representing the BPMP
+
 additionalProperties: false
 
 required:
-- 
2.17.1

[Patch v4 02/10] arm64: tegra: add bpmp ref in tegra234-mc node

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

Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
---
 arch/arm64/boot/dts/nvidia/tegra234.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/nvidia/tegra234.dtsi b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
index 959d659dd5cd..348113b4928a 100644
--- a/arch/arm64/boot/dts/nvidia/tegra234.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
@@ -656,6 +656,8 @@
 			 */
 			dma-ranges = <0x0 0x0 0x0 0x0 0x80 0x0>;
 
+			nvidia,bpmp = <&bpmp>;
+
 			emc: external-memory-controller@2c60000 {
 				compatible = "nvidia,tegra234-emc";
 				reg = <0x0 0x02c60000 0x0 0x90000>,
-- 
2.17.1

[Patch v4 03/10] memory: tegra: add interconnect support for DRAM scaling in Tegra234

[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 <sumitg@nvidia.com>
---
 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

diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
index 9082b6c3763d..983455b1f98d 100644
--- a/drivers/memory/tegra/mc.c
+++ b/drivers/memory/tegra/mc.c
@@ -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);
diff --git a/drivers/memory/tegra/tegra186-emc.c b/drivers/memory/tegra/tegra186-emc.c
index e935ad4e95b6..1eefcf2ac0c7 100644
--- a/drivers/memory/tegra/tegra186-emc.c
+++ b/drivers/memory/tegra/tegra186-emc.c
@@ -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;
+	}
+
+	node->name = "External Memory (DRAM)";
+	icc_node_add(node, &emc->provider);
+
+	err = icc_provider_register(&emc->provider);
+	if (err)
+		goto remove_nodes;
+
+	return 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;
 
+	platform_set_drvdata(pdev, emc);
+	emc->dev = &pdev->dev;
+
 	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;
+
+		if (tegra_bpmp_mrq_is_supported(emc->bpmp, MRQ_BWMGR_INT))
+			mc->bwmgr_mrq_supported = true;
+		else
+
+			dev_info(&pdev->dev, "MRQ_BWMGR_INT not present\n");
+	}
+
 	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,
diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
index 7bb73f06fad3..386e029e41bb 100644
--- a/drivers/memory/tegra/tegra186.c
+++ b/drivers/memory/tegra/tegra186.c
@@ -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)
diff --git a/drivers/memory/tegra/tegra234.c b/drivers/memory/tegra/tegra234.c
index 02dcc5748bba..4f34247c9bda 100644
--- a/drivers/memory/tegra/tegra234.c
+++ b/drivers/memory/tegra/tegra234.c
@@ -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.
  */
 
 #include <soc/tegra/mc.h>
 
 #include <dt-bindings/memory/tegra234-mc.h>
+#include <linux/interconnect.h>
+#include <linux/of_device.h>
+#include <linux/tegra-icc.h>
 
+#include <soc/tegra/bpmp.h>
 #include "mc.h"
 
 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 = {
@@ -23,6 +29,8 @@ static const struct tegra_mc_client tegra234_mc_clients[] = {
 	}, {
 		.id = TEGRA234_MEMORY_CLIENT_MGBEBRD,
 		.name = "mgbebrd",
+		.bpmp_id = TEGRA_ICC_BPMP_EQOS,
+		.type = TEGRA_ICC_NISO,
 		.sid = TEGRA234_SID_MGBE_VF1,
 		.regs = {
 			.sid = {
@@ -33,6 +41,8 @@ static const struct tegra_mc_client tegra234_mc_clients[] = {
 	}, {
 		.id = TEGRA234_MEMORY_CLIENT_MGBECRD,
 		.name = "mgbecrd",
+		.bpmp_id = TEGRA_ICC_BPMP_EQOS,
+		.type = TEGRA_ICC_NISO,
 		.sid = TEGRA234_SID_MGBE_VF2,
 		.regs = {
 			.sid = {
@@ -43,6 +53,8 @@ static const struct tegra_mc_client tegra234_mc_clients[] = {
 	}, {
 		.id = TEGRA234_MEMORY_CLIENT_MGBEDRD,
 		.name = "mgbedrd",
+		.bpmp_id = TEGRA_ICC_BPMP_EQOS,
+		.type = TEGRA_ICC_NISO,
 		.sid = TEGRA234_SID_MGBE_VF3,
 		.regs = {
 			.sid = {
@@ -52,6 +64,8 @@ static const struct tegra_mc_client tegra234_mc_clients[] = {
 		},
 	}, {
 		.id = TEGRA234_MEMORY_CLIENT_MGBEAWR,
+		.bpmp_id = TEGRA_ICC_BPMP_EQOS,
+		.type = TEGRA_ICC_NISO,
 		.name = "mgbeawr",
 		.sid = TEGRA234_SID_MGBE,
 		.regs = {
@@ -63,6 +77,8 @@ static const struct tegra_mc_client tegra234_mc_clients[] = {
 	}, {
 		.id = TEGRA234_MEMORY_CLIENT_MGBEBWR,
 		.name = "mgbebwr",
+		.bpmp_id = TEGRA_ICC_BPMP_EQOS,
+		.type = TEGRA_ICC_NISO,
 		.sid = TEGRA234_SID_MGBE_VF1,
 		.regs = {
 			.sid = {
@@ -73,6 +89,8 @@ static const struct tegra_mc_client tegra234_mc_clients[] = {
 	}, {
 		.id = TEGRA234_MEMORY_CLIENT_MGBECWR,
 		.name = "mgbecwr",
+		.bpmp_id = TEGRA_ICC_BPMP_EQOS,
+		.type = TEGRA_ICC_NISO,
 		.sid = TEGRA234_SID_MGBE_VF2,
 		.regs = {
 			.sid = {
@@ -83,6 +101,8 @@ static const struct tegra_mc_client tegra234_mc_clients[] = {
 	}, {
 		.id = TEGRA234_MEMORY_CLIENT_SDMMCRAB,
 		.name = "sdmmcrab",
+		.bpmp_id = TEGRA_ICC_BPMP_SDMMC_4,
+		.type = TEGRA_ICC_NISO,
 		.sid = TEGRA234_SID_SDMMC4,
 		.regs = {
 			.sid = {
@@ -93,6 +113,8 @@ static const struct tegra_mc_client tegra234_mc_clients[] = {
 	}, {
 		.id = TEGRA234_MEMORY_CLIENT_MGBEDWR,
 		.name = "mgbedwr",
+		.bpmp_id = TEGRA_ICC_BPMP_EQOS,
+		.type = TEGRA_ICC_NISO,
 		.sid = TEGRA234_SID_MGBE_VF3,
 		.regs = {
 			.sid = {
@@ -103,6 +125,8 @@ static const struct tegra_mc_client tegra234_mc_clients[] = {
 	}, {
 		.id = TEGRA234_MEMORY_CLIENT_SDMMCWAB,
 		.name = "sdmmcwab",
+		.bpmp_id = TEGRA_ICC_BPMP_SDMMC_4,
+		.type = TEGRA_ICC_NISO,
 		.sid = TEGRA234_SID_SDMMC4,
 		.regs = {
 			.sid = {
@@ -153,6 +177,8 @@ static const struct tegra_mc_client tegra234_mc_clients[] = {
 	}, {
 		.id = TEGRA234_MEMORY_CLIENT_APEDMAR,
 		.name = "apedmar",
+		.bpmp_id = TEGRA_ICC_BPMP_APEDMA,
+		.type = TEGRA_ICC_ISO_AUDIO,
 		.sid = TEGRA234_SID_APE,
 		.regs = {
 			.sid = {
@@ -163,6 +189,8 @@ static const struct tegra_mc_client tegra234_mc_clients[] = {
 	}, {
 		.id = TEGRA234_MEMORY_CLIENT_APEDMAW,
 		.name = "apedmaw",
+		.bpmp_id = TEGRA_ICC_BPMP_APEDMA,
+		.type = TEGRA_ICC_ISO_AUDIO,
 		.sid = TEGRA234_SID_APE,
 		.regs = {
 			.sid = {
@@ -333,6 +361,118 @@ static const struct tegra_mc_client tegra234_mc_clients[] = {
 	},
 };
 
+/*
+ * tegra234_mc_icc_set() - Pass MC client info to the BPMP-FW
+ * @src: ICC node for Memory Controller's (MC) Client
+ * @dst: ICC node for Memory Controller (MC)
+ *
+ * Passing the current request info from the MC to the BPMP-FW where
+ * LA and PTSA registers are accessed and the final EMC freq is set
+ * based on client_id, type, latency and bandwidth.
+ * icc_set_bw() makes set_bw calls for both MC and EMC providers in
+ * sequence. Both the calls are protected by 'mutex_lock(&icc_lock)'.
+ * So, the data passed won't be updated by concurrent set calls from
+ * other clients.
+ */
+static int tegra234_mc_icc_set(struct icc_node *src, struct icc_node *dst)
+{
+	struct tegra_mc *mc = icc_provider_to_tegra_mc(dst->provider);
+	struct mrq_bwmgr_int_request bwmgr_req = { 0 };
+	struct mrq_bwmgr_int_response bwmgr_resp = { 0 };
+	const struct tegra_mc_client *pclient = src->data;
+	struct tegra_bpmp_message msg;
+	int ret;
+
+	/*
+	 * Same Src and Dst node will happen during boot from icc_node_add().
+	 * This can be used to pre-initialize and set bandwidth for all clients
+	 * before their drivers are loaded. We are skipping this case as for us,
+	 * the pre-initialization already happened in Bootloader(MB2) and BPMP-FW.
+	 */
+	if (src->id == dst->id)
+		return 0;
+
+	if (!mc->bwmgr_mrq_supported)
+		return -EINVAL;
+
+	if (!mc->bpmp) {
+		mc->bpmp = tegra_bpmp_get(mc->dev);
+		if (IS_ERR(mc->bpmp)) {
+			ret = PTR_ERR(mc->bpmp);
+			dev_err(mc->dev, "failed to get BPMP: %d\n", ret);
+			return ret;
+		}
+	}
+
+	if (pclient->type == TEGRA_ICC_NISO)
+		bwmgr_req.bwmgr_calc_set_req.niso_bw = src->avg_bw;
+	else
+		bwmgr_req.bwmgr_calc_set_req.iso_bw = src->avg_bw;
+
+	bwmgr_req.bwmgr_calc_set_req.client_id = pclient->bpmp_id;
+
+	bwmgr_req.cmd = CMD_BWMGR_INT_CALC_AND_SET;
+	bwmgr_req.bwmgr_calc_set_req.mc_floor = src->peak_bw;
+	bwmgr_req.bwmgr_calc_set_req.floor_unit = BWMGR_INT_UNIT_KBPS;
+
+	memset(&msg, 0, sizeof(msg));
+	msg.mrq = MRQ_BWMGR_INT;
+	msg.tx.data = &bwmgr_req;
+	msg.tx.size = sizeof(bwmgr_req);
+	msg.rx.data = &bwmgr_resp;
+	msg.rx.size = sizeof(bwmgr_resp);
+
+	ret = tegra_bpmp_transfer(mc->bpmp, &msg);
+	if (ret < 0) {
+		dev_err(mc->dev, "BPMP transfer failed: %d\n", ret);
+		goto error;
+	}
+	if (msg.rx.ret < 0) {
+		pr_err("failed to set bandwidth for %u: %d\n",
+		       bwmgr_req.bwmgr_calc_set_req.client_id, msg.rx.ret);
+		ret = -EINVAL;
+	}
+
+error:
+	return ret;
+}
+
+static struct icc_node*
+tegra234_mc_of_icc_xlate(struct of_phandle_args *spec, void *data)
+{
+	struct tegra_mc *mc = icc_provider_to_tegra_mc(data);
+	unsigned int cl_id = spec->args[0];
+	struct icc_node *node;
+
+	list_for_each_entry(node, &mc->provider.nodes, node_list) {
+		if (node->id != cl_id)
+			continue;
+
+		return node;
+	}
+
+	/*
+	 * If a client driver calls devm_of_icc_get() before the MC driver
+	 * is probed, then return EPROBE_DEFER to the client driver.
+	 */
+	return ERR_PTR(-EPROBE_DEFER);
+}
+
+static int tegra234_mc_icc_get_init_bw(struct icc_node *node, u32 *avg, u32 *peak)
+{
+	*avg = 0;
+	*peak = 0;
+
+	return 0;
+}
+
+static const struct tegra_mc_icc_ops tegra234_mc_icc_ops = {
+	.xlate = tegra234_mc_of_icc_xlate,
+	.aggregate = icc_std_aggregate,
+	.get_bw = tegra234_mc_icc_get_init_bw,
+	.set = tegra234_mc_icc_set,
+};
+
 const struct tegra_mc_soc tegra234_mc_soc = {
 	.num_clients = ARRAY_SIZE(tegra234_mc_clients),
 	.clients = tegra234_mc_clients,
@@ -345,6 +485,7 @@ const struct tegra_mc_soc tegra234_mc_soc = {
 		   MC_INT_SECURITY_VIOLATION | MC_INT_DECERR_EMEM,
 	.has_addr_hi_reg = true,
 	.ops = &tegra186_mc_ops,
+	.icc_ops = &tegra234_mc_icc_ops,
 	.ch_intmask = 0x0000ff00,
 	.global_intstatus_channel_shift = 8,
 	/*
diff --git a/include/linux/tegra-icc.h b/include/linux/tegra-icc.h
new file mode 100644
index 000000000000..4b4d4bee290c
--- /dev/null
+++ b/include/linux/tegra-icc.h
@@ -0,0 +1,65 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2022-2023 NVIDIA CORPORATION.  All rights reserved.
+ */
+
+#ifndef LINUX_TEGRA_ICC_H
+#define LINUX_TEGRA_ICC_H
+
+enum tegra_icc_client_type {
+	TEGRA_ICC_NONE,
+	TEGRA_ICC_NISO,
+	TEGRA_ICC_ISO_DISPLAY,
+	TEGRA_ICC_ISO_VI,
+	TEGRA_ICC_ISO_AUDIO,
+	TEGRA_ICC_ISO_VIFAL,
+};
+
+/* ICC ID's for MC client's used in BPMP */
+#define TEGRA_ICC_BPMP_DEBUG		1
+#define TEGRA_ICC_BPMP_CPU_CLUSTER0	2
+#define TEGRA_ICC_BPMP_CPU_CLUSTER1	3
+#define TEGRA_ICC_BPMP_CPU_CLUSTER2	4
+#define TEGRA_ICC_BPMP_GPU		5
+#define TEGRA_ICC_BPMP_CACTMON		6
+#define TEGRA_ICC_BPMP_DISPLAY		7
+#define TEGRA_ICC_BPMP_VI		8
+#define TEGRA_ICC_BPMP_EQOS		9
+#define TEGRA_ICC_BPMP_PCIE_0		10
+#define TEGRA_ICC_BPMP_PCIE_1		11
+#define TEGRA_ICC_BPMP_PCIE_2		12
+#define TEGRA_ICC_BPMP_PCIE_3		13
+#define TEGRA_ICC_BPMP_PCIE_4		14
+#define TEGRA_ICC_BPMP_PCIE_5		15
+#define TEGRA_ICC_BPMP_PCIE_6		16
+#define TEGRA_ICC_BPMP_PCIE_7		17
+#define TEGRA_ICC_BPMP_PCIE_8		18
+#define TEGRA_ICC_BPMP_PCIE_9		19
+#define TEGRA_ICC_BPMP_PCIE_10		20
+#define TEGRA_ICC_BPMP_DLA_0		21
+#define TEGRA_ICC_BPMP_DLA_1		22
+#define TEGRA_ICC_BPMP_SDMMC_1		23
+#define TEGRA_ICC_BPMP_SDMMC_2		24
+#define TEGRA_ICC_BPMP_SDMMC_3		25
+#define TEGRA_ICC_BPMP_SDMMC_4		26
+#define TEGRA_ICC_BPMP_NVDEC		27
+#define TEGRA_ICC_BPMP_NVENC		28
+#define TEGRA_ICC_BPMP_NVJPG_0		29
+#define TEGRA_ICC_BPMP_NVJPG_1		30
+#define TEGRA_ICC_BPMP_OFAA		31
+#define TEGRA_ICC_BPMP_XUSB_HOST	32
+#define TEGRA_ICC_BPMP_XUSB_DEV		33
+#define TEGRA_ICC_BPMP_TSEC		34
+#define TEGRA_ICC_BPMP_VIC		35
+#define TEGRA_ICC_BPMP_APE		36
+#define TEGRA_ICC_BPMP_APEDMA		37
+#define TEGRA_ICC_BPMP_SE		38
+#define TEGRA_ICC_BPMP_ISP		39
+#define TEGRA_ICC_BPMP_HDA		40
+#define TEGRA_ICC_BPMP_VIFAL		41
+#define TEGRA_ICC_BPMP_VI2FAL		42
+#define TEGRA_ICC_BPMP_VI2		43
+#define TEGRA_ICC_BPMP_RCE		44
+#define TEGRA_ICC_BPMP_PVA		45
+
+#endif /* LINUX_TEGRA_ICC_H */
diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h
index 51a2263e1bc5..900d88b26fae 100644
--- a/include/soc/tegra/mc.h
+++ b/include/soc/tegra/mc.h
@@ -13,6 +13,7 @@
 #include <linux/irq.h>
 #include <linux/reset-controller.h>
 #include <linux/types.h>
+#include <linux/tegra-icc.h>
 
 struct clk;
 struct device;
@@ -26,6 +27,8 @@ struct tegra_mc_timing {
 
 struct tegra_mc_client {
 	unsigned int id;
+	unsigned int bpmp_id;
+	enum tegra_icc_client_type type;
 	const char *name;
 	/*
 	 * For Tegra210 and earlier, this is the SWGROUP ID used for IOVA translations in the
@@ -166,8 +169,10 @@ struct tegra_mc_icc_ops {
 	int (*set)(struct icc_node *src, struct icc_node *dst);
 	int (*aggregate)(struct icc_node *node, u32 tag, u32 avg_bw,
 			 u32 peak_bw, u32 *agg_avg, u32 *agg_peak);
+	struct icc_node* (*xlate)(struct of_phandle_args *spec, void *data);
 	struct icc_node_data *(*xlate_extended)(struct of_phandle_args *spec,
 						void *data);
+	int (*get_bw)(struct icc_node *node, u32 *avg, u32 *peak);
 };
 
 struct tegra_mc_ops {
@@ -214,6 +219,7 @@ struct tegra_mc_soc {
 };
 
 struct tegra_mc {
+	struct tegra_bpmp *bpmp;
 	struct device *dev;
 	struct tegra_smmu *smmu;
 	struct gart_device *gart;
@@ -229,6 +235,7 @@ struct tegra_mc {
 	struct tegra_mc_timing *timings;
 	unsigned int num_timings;
 
+	bool bwmgr_mrq_supported;
 	struct reset_controller_dev reset;
 
 	struct icc_provider provider;
-- 
2.17.1

[Patch v4 04/10] memory: tegra: add mc clients for Tegra234

[Sumit Gupta]

Add few Isochronous (ISO) and Non-ISO MC clients.
ISO clients have guaranteed bandwidth requirement.
PCIE clients added to the mc_clients table represent
each controller in Tegra234.

Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/memory/tegra/tegra234.c | 420 ++++++++++++++++++++++++++++++++
 1 file changed, 420 insertions(+)

diff --git a/drivers/memory/tegra/tegra234.c b/drivers/memory/tegra/tegra234.c
index 4f34247c9bda..353f5ef688b2 100644
--- a/drivers/memory/tegra/tegra234.c
+++ b/drivers/memory/tegra/tegra234.c
@@ -15,6 +15,30 @@
 
 static const struct tegra_mc_client tegra234_mc_clients[] = {
 	{
+		.id = TEGRA234_MEMORY_CLIENT_HDAR,
+		.name = "hdar",
+		.bpmp_id = TEGRA_ICC_BPMP_HDA,
+		.type = TEGRA_ICC_ISO_AUDIO,
+		.sid = TEGRA234_SID_HDA,
+		.regs = {
+			.sid = {
+				.override = 0xa8,
+				.security = 0xac,
+			},
+		},
+	}, {
+		.id = TEGRA234_MEMORY_CLIENT_HDAW,
+		.name = "hdaw",
+		.bpmp_id = TEGRA_ICC_BPMP_HDA,
+		.type = TEGRA_ICC_ISO_AUDIO,
+		.sid = TEGRA234_SID_HDA,
+		.regs = {
+			.sid = {
+				.override = 0x1a8,
+				.security = 0x1ac,
+			},
+		},
+	}, {
 		.id = TEGRA234_MEMORY_CLIENT_MGBEARD,
 		.name = "mgbeard",
 		.bpmp_id = TEGRA_ICC_BPMP_EQOS,
@@ -134,6 +158,90 @@ static const struct tegra_mc_client tegra234_mc_clients[] = {
 				.security = 0x33c,
 			},
 		},
+	}, {
+		.id = TEGRA234_MEMORY_CLIENT_VI2W,
+		.name = "vi2w",
+		.bpmp_id = TEGRA_ICC_BPMP_VI2,
+		.type = TEGRA_ICC_ISO_VI,
+		.sid = TEGRA234_SID_ISO_VI2,
+		.regs = {
+			.sid = {
+				.override = 0x380,
+				.security = 0x384,
+			},
+		},
+	}, {
+		.id = TEGRA234_MEMORY_CLIENT_VI2FALR,
+		.name = "vi2falr",
+		.bpmp_id = TEGRA_ICC_BPMP_VI2FAL,
+		.type = TEGRA_ICC_ISO_VIFAL,
+		.sid = TEGRA234_SID_ISO_VI2FALC,
+		.regs = {
+			.sid = {
+				.override = 0x388,
+				.security = 0x38c,
+			},
+		},
+	}, {
+		.id = TEGRA234_MEMORY_CLIENT_VI2FALW,
+		.name = "vi2falw",
+		.bpmp_id = TEGRA_ICC_BPMP_VI2FAL,
+		.type = TEGRA_ICC_ISO_VIFAL,
+		.sid = TEGRA234_SID_ISO_VI2FALC,
+		.regs = {
+			.sid = {
+				.override = 0x3e0,
+				.security = 0x3e4,
+			},
+		},
+	}, {
+		.id = TEGRA234_MEMORY_CLIENT_APER,
+		.name = "aper",
+		.bpmp_id = TEGRA_ICC_BPMP_APE,
+		.type = TEGRA_ICC_ISO_AUDIO,
+		.sid = TEGRA234_SID_APE,
+		.regs = {
+			.sid = {
+				.override = 0x3d0,
+				.security = 0x3d4,
+			},
+		},
+	}, {
+		.id = TEGRA234_MEMORY_CLIENT_APEW,
+		.name = "apew",
+		.bpmp_id = TEGRA_ICC_BPMP_APE,
+		.type = TEGRA_ICC_ISO_AUDIO,
+		.sid = TEGRA234_SID_APE,
+		.regs = {
+			.sid = {
+				.override = 0x3d8,
+				.security = 0x3dc,
+			},
+		},
+	}, {
+		.id = TEGRA234_MEMORY_CLIENT_NVDISPLAYR,
+		.name = "nvdisplayr",
+		.bpmp_id = TEGRA_ICC_BPMP_DISPLAY,
+		.type = TEGRA_ICC_ISO_DISPLAY,
+		.sid = TEGRA234_SID_ISO_NVDISPLAY,
+		.regs = {
+			.sid = {
+				.override = 0x490,
+				.security = 0x494,
+			},
+		},
+	}, {
+		.id = TEGRA234_MEMORY_CLIENT_NVDISPLAYR1,
+		.name = "nvdisplayr1",
+		.bpmp_id = TEGRA_ICC_BPMP_DISPLAY,
+		.type = TEGRA_ICC_ISO_DISPLAY,
+		.sid = TEGRA234_SID_ISO_NVDISPLAY,
+		.regs = {
+			.sid = {
+				.override = 0x508,
+				.security = 0x50c,
+			},
+		},
 	}, {
 		.id = TEGRA234_MEMORY_CLIENT_BPMPR,
 		.name = "bpmpr",
@@ -358,6 +466,318 @@ static const struct tegra_mc_client tegra234_mc_clients[] = {
 				.security = 0x37c,
 			},
 		},
+	}, {
+		.id = TEGRA234_MEMORY_CLIENT_PCIE0R,
+		.name = "pcie0r",
+		.bpmp_id = TEGRA_ICC_BPMP_PCIE_0,
+		.type = TEGRA_ICC_NISO,
+		.sid = TEGRA234_SID_PCIE0,
+		.regs = {
+			.sid = {
+				.override = 0x6c0,
+				.security = 0x6c4,
+			},
+		},
+	}, {
+		.id = TEGRA234_MEMORY_CLIENT_PCIE0W,
+		.name = "pcie0w",
+		.bpmp_id = TEGRA_ICC_BPMP_PCIE_0,
+		.type = TEGRA_ICC_NISO,
+		.sid = TEGRA234_SID_PCIE0,
+		.regs = {
+			.sid = {
+				.override = 0x6c8,
+				.security = 0x6cc,
+			},
+		},
+	}, {
+		.id = TEGRA234_MEMORY_CLIENT_PCIE1R,
+		.name = "pcie1r",
+		.bpmp_id = TEGRA_ICC_BPMP_PCIE_1,
+		.type = TEGRA_ICC_NISO,
+		.sid = TEGRA234_SID_PCIE1,
+		.regs = {
+			.sid = {
+				.override = 0x6d0,
+				.security = 0x6d4,
+			},
+		},
+	}, {
+		.id = TEGRA234_MEMORY_CLIENT_PCIE1W,
+		.name = "pcie1w",
+		.bpmp_id = TEGRA_ICC_BPMP_PCIE_1,
+		.type = TEGRA_ICC_NISO,
+		.sid = TEGRA234_SID_PCIE1,
+		.regs = {
+			.sid = {
+				.override = 0x6d8,
+				.security = 0x6dc,
+			},
+		},
+	}, {
+		.id = TEGRA234_MEMORY_CLIENT_PCIE2AR,
+		.name = "pcie2ar",
+		.bpmp_id = TEGRA_ICC_BPMP_PCIE_2,
+		.type = TEGRA_ICC_NISO,
+		.sid = TEGRA234_SID_PCIE2,
+		.regs = {
+			.sid = {
+				.override = 0x6e0,
+				.security = 0x6e4,
+			},
+		},
+	}, {
+		.id = TEGRA234_MEMORY_CLIENT_PCIE2AW,
+		.name = "pcie2aw",
+		.bpmp_id = TEGRA_ICC_BPMP_PCIE_2,
+		.type = TEGRA_ICC_NISO,
+		.sid = TEGRA234_SID_PCIE2,
+		.regs = {
+			.sid = {
+				.override = 0x6e8,
+				.security = 0x6ec,
+			},
+		},
+	}, {
+		.id = TEGRA234_MEMORY_CLIENT_PCIE3R,
+		.name = "pcie3r",
+		.bpmp_id = TEGRA_ICC_BPMP_PCIE_3,
+		.type = TEGRA_ICC_NISO,
+		.sid = TEGRA234_SID_PCIE3,
+		.regs = {
+			.sid = {
+				.override = 0x6f0,
+				.security = 0x6f4,
+			},
+		},
+	}, {
+		.id = TEGRA234_MEMORY_CLIENT_PCIE3W,
+		.name = "pcie3w",
+		.bpmp_id = TEGRA_ICC_BPMP_PCIE_3,
+		.type = TEGRA_ICC_NISO,
+		.sid = TEGRA234_SID_PCIE3,
+		.regs = {
+			.sid = {
+				.override = 0x6f8,
+				.security = 0x6fc,
+			},
+		},
+	}, {
+		.id = TEGRA234_MEMORY_CLIENT_PCIE4R,
+		.name = "pcie4r",
+		.bpmp_id = TEGRA_ICC_BPMP_PCIE_4,
+		.type = TEGRA_ICC_NISO,
+		.sid = TEGRA234_SID_PCIE4,
+		.regs = {
+			.sid = {
+				.override = 0x700,
+				.security = 0x704,
+			},
+		},
+	}, {
+		.id = TEGRA234_MEMORY_CLIENT_PCIE4W,
+		.name = "pcie4w",
+		.bpmp_id = TEGRA_ICC_BPMP_PCIE_4,
+		.type = TEGRA_ICC_NISO,
+		.sid = TEGRA234_SID_PCIE4,
+		.regs = {
+			.sid = {
+				.override = 0x708,
+				.security = 0x70c,
+			},
+		},
+	}, {
+		.id = TEGRA234_MEMORY_CLIENT_PCIE5R,
+		.name = "pcie5r",
+		.bpmp_id = TEGRA_ICC_BPMP_PCIE_5,
+		.type = TEGRA_ICC_NISO,
+		.sid = TEGRA234_SID_PCIE5,
+		.regs = {
+			.sid = {
+				.override = 0x710,
+				.security = 0x714,
+			},
+		},
+	}, {
+		.id = TEGRA234_MEMORY_CLIENT_PCIE5W,
+		.name = "pcie5w",
+		.bpmp_id = TEGRA_ICC_BPMP_PCIE_5,
+		.type = TEGRA_ICC_NISO,
+		.sid = TEGRA234_SID_PCIE5,
+		.regs = {
+			.sid = {
+				.override = 0x718,
+				.security = 0x71c,
+			},
+		},
+	}, {
+		.id = TEGRA234_MEMORY_CLIENT_PCIE5R1,
+		.name = "pcie5r1",
+		.bpmp_id = TEGRA_ICC_BPMP_PCIE_5,
+		.type = TEGRA_ICC_NISO,
+		.sid = TEGRA234_SID_PCIE5,
+		.regs = {
+			.sid = {
+				.override = 0x778,
+				.security = 0x77c,
+			},
+		},
+	}, {
+		.id = TEGRA234_MEMORY_CLIENT_PCIE6AR,
+		.name = "pcie6ar",
+		.bpmp_id = TEGRA_ICC_BPMP_PCIE_6,
+		.type = TEGRA_ICC_NISO,
+		.sid = TEGRA234_SID_PCIE6,
+		.regs = {
+			.sid = {
+				.override = 0x140,
+				.security = 0x144,
+			},
+		},
+	}, {
+		.id = TEGRA234_MEMORY_CLIENT_PCIE6AW,
+		.name = "pcie6aw",
+		.bpmp_id = TEGRA_ICC_BPMP_PCIE_6,
+		.type = TEGRA_ICC_NISO,
+		.sid = TEGRA234_SID_PCIE6,
+		.regs = {
+			.sid = {
+				.override = 0x148,
+				.security = 0x14c,
+			},
+		},
+	}, {
+		.id = TEGRA234_MEMORY_CLIENT_PCIE6AR1,
+		.name = "pcie6ar1",
+		.bpmp_id = TEGRA_ICC_BPMP_PCIE_6,
+		.type = TEGRA_ICC_NISO,
+		.sid = TEGRA234_SID_PCIE6,
+		.regs = {
+			.sid = {
+				.override = 0x1e8,
+				.security = 0x1ec,
+			},
+		},
+	}, {
+		.id = TEGRA234_MEMORY_CLIENT_PCIE7AR,
+		.name = "pcie7ar",
+		.bpmp_id = TEGRA_ICC_BPMP_PCIE_7,
+		.type = TEGRA_ICC_NISO,
+		.sid = TEGRA234_SID_PCIE7,
+		.regs = {
+			.sid = {
+				.override = 0x150,
+				.security = 0x154,
+			},
+		},
+	}, {
+		.id = TEGRA234_MEMORY_CLIENT_PCIE7AW,
+		.name = "pcie7aw",
+		.bpmp_id = TEGRA_ICC_BPMP_PCIE_7,
+		.type = TEGRA_ICC_NISO,
+		.sid = TEGRA234_SID_PCIE7,
+		.regs = {
+			.sid = {
+				.override = 0x180,
+				.security = 0x184,
+			},
+		},
+	}, {
+		.id = TEGRA234_MEMORY_CLIENT_PCIE7AR1,
+		.name = "pcie7ar1",
+		.bpmp_id = TEGRA_ICC_BPMP_PCIE_7,
+		.type = TEGRA_ICC_NISO,
+		.sid = TEGRA234_SID_PCIE7,
+		.regs = {
+			.sid = {
+				.override = 0x248,
+				.security = 0x24c,
+			},
+		},
+	}, {
+		.id = TEGRA234_MEMORY_CLIENT_PCIE8AR,
+		.name = "pcie8ar",
+		.bpmp_id = TEGRA_ICC_BPMP_PCIE_8,
+		.type = TEGRA_ICC_NISO,
+		.sid = TEGRA234_SID_PCIE8,
+		.regs = {
+			.sid = {
+				.override = 0x190,
+				.security = 0x194,
+			},
+		},
+	}, {
+		.id = TEGRA234_MEMORY_CLIENT_PCIE8AW,
+		.name = "pcie8aw",
+		.bpmp_id = TEGRA_ICC_BPMP_PCIE_8,
+		.type = TEGRA_ICC_NISO,
+		.sid = TEGRA234_SID_PCIE8,
+		.regs = {
+			.sid = {
+				.override = 0x1d8,
+				.security = 0x1dc,
+			},
+		},
+	}, {
+		.id = TEGRA234_MEMORY_CLIENT_PCIE9AR,
+		.name = "pcie9ar",
+		.bpmp_id = TEGRA_ICC_BPMP_PCIE_9,
+		.type = TEGRA_ICC_NISO,
+		.sid = TEGRA234_SID_PCIE9,
+		.regs = {
+			.sid = {
+				.override = 0x1e0,
+				.security = 0x1e4,
+			},
+		},
+	}, {
+		.id = TEGRA234_MEMORY_CLIENT_PCIE9AW,
+		.name = "pcie9aw",
+		.bpmp_id = TEGRA_ICC_BPMP_PCIE_9,
+		.type = TEGRA_ICC_NISO,
+		.sid = TEGRA234_SID_PCIE9,
+		.regs = {
+			.sid = {
+				.override = 0x1f0,
+				.security = 0x1f4,
+			},
+		},
+	}, {
+		.id = TEGRA234_MEMORY_CLIENT_PCIE10AR,
+		.name = "pcie10ar",
+		.bpmp_id = TEGRA_ICC_BPMP_PCIE_10,
+		.type = TEGRA_ICC_NISO,
+		.sid = TEGRA234_SID_PCIE10,
+		.regs = {
+			.sid = {
+				.override = 0x1f8,
+				.security = 0x1fc,
+			},
+		},
+	}, {
+		.id = TEGRA234_MEMORY_CLIENT_PCIE10AW,
+		.name = "pcie10aw",
+		.bpmp_id = TEGRA_ICC_BPMP_PCIE_10,
+		.type = TEGRA_ICC_NISO,
+		.sid = TEGRA234_SID_PCIE10,
+		.regs = {
+			.sid = {
+				.override = 0x200,
+				.security = 0x204,
+			},
+		},
+	}, {
+		.id = TEGRA234_MEMORY_CLIENT_PCIE10AR1,
+		.name = "pcie10ar1",
+		.bpmp_id = TEGRA_ICC_BPMP_PCIE_10,
+		.type = TEGRA_ICC_NISO,
+		.sid = TEGRA234_SID_PCIE10,
+		.regs = {
+			.sid = {
+				.override = 0x240,
+				.security = 0x244,
+			},
+		},
 	},
 };
 
-- 
2.17.1

[Patch v4 05/10] memory: tegra: add software mc clients in Tegra234

[Sumit Gupta]

Add dummy Memory Controller clients to represent CPU clusters.
They will be used by the CPUFREQ driver to scale DRAM FREQ
with the CPU FREQ.

Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/memory/tegra/tegra234.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/memory/tegra/tegra234.c b/drivers/memory/tegra/tegra234.c
index 353f5ef688b2..4c8929a88778 100644
--- a/drivers/memory/tegra/tegra234.c
+++ b/drivers/memory/tegra/tegra234.c
@@ -778,6 +778,21 @@ static const struct tegra_mc_client tegra234_mc_clients[] = {
 				.security = 0x244,
 			},
 		},
+	}, {
+		.id = TEGRA_ICC_MC_CPU_CLUSTER0,
+		.name = "sw_cluster0",
+		.bpmp_id = TEGRA_ICC_BPMP_CPU_CLUSTER0,
+		.type = TEGRA_ICC_NISO,
+	}, {
+		.id = TEGRA_ICC_MC_CPU_CLUSTER1,
+		.name = "sw_cluster1",
+		.bpmp_id = TEGRA_ICC_BPMP_CPU_CLUSTER1,
+		.type = TEGRA_ICC_NISO,
+	}, {
+		.id = TEGRA_ICC_MC_CPU_CLUSTER2,
+		.name = "sw_cluster2",
+		.bpmp_id = TEGRA_ICC_BPMP_CPU_CLUSTER2,
+		.type = TEGRA_ICC_NISO,
 	},
 };
 
-- 
2.17.1

[Patch v4 06/10] dt-bindings: tegra: add icc ids for dummy MC clients

[Sumit Gupta]

Add ICC id's for dummy software clients representing CCPLEX clusters.

Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 include/dt-bindings/memory/tegra234-mc.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/dt-bindings/memory/tegra234-mc.h b/include/dt-bindings/memory/tegra234-mc.h
index 347e55e89a2a..6e60d55491b3 100644
--- a/include/dt-bindings/memory/tegra234-mc.h
+++ b/include/dt-bindings/memory/tegra234-mc.h
@@ -536,4 +536,9 @@
 #define TEGRA234_MEMORY_CLIENT_NVJPG1SRD 0x123
 #define TEGRA234_MEMORY_CLIENT_NVJPG1SWR 0x124
 
+/* ICC ID's for dummy MC clients used to represent CPU Clusters */
+#define TEGRA_ICC_MC_CPU_CLUSTER0       1003
+#define TEGRA_ICC_MC_CPU_CLUSTER1       1004
+#define TEGRA_ICC_MC_CPU_CLUSTER2       1005
+
 #endif
-- 
2.17.1

[Patch v4 07/10] arm64: tegra: Add cpu OPP tables and interconnects property

[Sumit Gupta]

Add OPP table and interconnects property to scale DDR frequency with
CPU frequency for better performance. Each operating point entry of
the OPP table has CPU freq to per MC channel bandwidth mapping.
One table is added for each cluster even though the table data is
same because the bandwidth request is per cluster. This is done
because OPP framework creates a single icc path and hence single
bandwidth request if the table is marked as 'opp-shared' and shared
among all clusters. For us, the OPP table data is same but the MC
Client ID argument to interconnects property is different for each
cluster. So, having per cluster table makes different icc path for
each cluster and helps to make per cluster BW requests.

Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
---
 arch/arm64/boot/dts/nvidia/tegra234.dtsi | 276 +++++++++++++++++++++++
 1 file changed, 276 insertions(+)

diff --git a/arch/arm64/boot/dts/nvidia/tegra234.dtsi b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
index 348113b4928a..6f4aa1a93688 100644
--- a/arch/arm64/boot/dts/nvidia/tegra234.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
@@ -3013,6 +3013,9 @@
 
 			enable-method = "psci";
 
+			operating-points-v2 = <&cl0_opp_tbl>;
+			interconnects = <&mc TEGRA_ICC_MC_CPU_CLUSTER0 &emc>;
+
 			i-cache-size = <65536>;
 			i-cache-line-size = <64>;
 			i-cache-sets = <256>;
@@ -3029,6 +3032,9 @@
 
 			enable-method = "psci";
 
+			operating-points-v2 = <&cl0_opp_tbl>;
+			interconnects = <&mc TEGRA_ICC_MC_CPU_CLUSTER0 &emc>;
+
 			i-cache-size = <65536>;
 			i-cache-line-size = <64>;
 			i-cache-sets = <256>;
@@ -3045,6 +3051,9 @@
 
 			enable-method = "psci";
 
+			operating-points-v2 = <&cl0_opp_tbl>;
+			interconnects = <&mc TEGRA_ICC_MC_CPU_CLUSTER0 &emc>;
+
 			i-cache-size = <65536>;
 			i-cache-line-size = <64>;
 			i-cache-sets = <256>;
@@ -3061,6 +3070,9 @@
 
 			enable-method = "psci";
 
+			operating-points-v2 = <&cl0_opp_tbl>;
+			interconnects = <&mc TEGRA_ICC_MC_CPU_CLUSTER0 &emc>;
+
 			i-cache-size = <65536>;
 			i-cache-line-size = <64>;
 			i-cache-sets = <256>;
@@ -3077,6 +3089,9 @@
 
 			enable-method = "psci";
 
+			operating-points-v2 = <&cl1_opp_tbl>;
+			interconnects = <&mc TEGRA_ICC_MC_CPU_CLUSTER1 &emc>;
+
 			i-cache-size = <65536>;
 			i-cache-line-size = <64>;
 			i-cache-sets = <256>;
@@ -3093,6 +3108,9 @@
 
 			enable-method = "psci";
 
+			operating-points-v2 = <&cl1_opp_tbl>;
+			interconnects = <&mc TEGRA_ICC_MC_CPU_CLUSTER1 &emc>;
+
 			i-cache-size = <65536>;
 			i-cache-line-size = <64>;
 			i-cache-sets = <256>;
@@ -3109,6 +3127,9 @@
 
 			enable-method = "psci";
 
+			operating-points-v2 = <&cl1_opp_tbl>;
+			interconnects = <&mc TEGRA_ICC_MC_CPU_CLUSTER1 &emc>;
+
 			i-cache-size = <65536>;
 			i-cache-line-size = <64>;
 			i-cache-sets = <256>;
@@ -3125,6 +3146,9 @@
 
 			enable-method = "psci";
 
+			operating-points-v2 = <&cl1_opp_tbl>;
+			interconnects = <&mc TEGRA_ICC_MC_CPU_CLUSTER1 &emc>;
+
 			i-cache-size = <65536>;
 			i-cache-line-size = <64>;
 			i-cache-sets = <256>;
@@ -3141,6 +3165,9 @@
 
 			enable-method = "psci";
 
+			operating-points-v2 = <&cl2_opp_tbl>;
+			interconnects = <&mc TEGRA_ICC_MC_CPU_CLUSTER2 &emc>;
+
 			i-cache-size = <65536>;
 			i-cache-line-size = <64>;
 			i-cache-sets = <256>;
@@ -3157,6 +3184,9 @@
 
 			enable-method = "psci";
 
+			operating-points-v2 = <&cl2_opp_tbl>;
+			interconnects = <&mc TEGRA_ICC_MC_CPU_CLUSTER2 &emc>;
+
 			i-cache-size = <65536>;
 			i-cache-line-size = <64>;
 			i-cache-sets = <256>;
@@ -3173,6 +3203,9 @@
 
 			enable-method = "psci";
 
+			operating-points-v2 = <&cl2_opp_tbl>;
+			interconnects = <&mc TEGRA_ICC_MC_CPU_CLUSTER2 &emc>;
+
 			i-cache-size = <65536>;
 			i-cache-line-size = <64>;
 			i-cache-sets = <256>;
@@ -3189,6 +3222,9 @@
 
 			enable-method = "psci";
 
+			operating-points-v2 = <&cl2_opp_tbl>;
+			interconnects = <&mc TEGRA_ICC_MC_CPU_CLUSTER2 &emc>;
+
 			i-cache-size = <65536>;
 			i-cache-line-size = <64>;
 			i-cache-sets = <256>;
@@ -3445,4 +3481,244 @@
 		interrupt-parent = <&gic>;
 		always-on;
 	};
+
+	cl0_opp_tbl: opp-table-cluster0 {
+		compatible = "operating-points-v2";
+		opp-shared;
+
+		cl0_ch1_opp1: opp-115200000 {
+			  opp-hz = /bits/ 64 <115200000>;
+			  opp-peak-kBps = <816000>;
+		};
+
+		cl0_ch1_opp2: opp-268800000 {
+			opp-hz = /bits/ 64 <268800000>;
+			opp-peak-kBps = <816000>;
+		};
+
+		cl0_ch1_opp3: opp-422400000 {
+			opp-hz = /bits/ 64 <422400000>;
+			opp-peak-kBps = <816000>;
+		};
+
+		cl0_ch1_opp4: opp-576000000 {
+			opp-hz = /bits/ 64 <576000000>;
+			opp-peak-kBps = <816000>;
+		};
+
+		cl0_ch1_opp5: opp-729600000 {
+			opp-hz = /bits/ 64 <729600000>;
+			opp-peak-kBps = <816000>;
+		};
+
+		cl0_ch1_opp6: opp-883200000 {
+			opp-hz = /bits/ 64 <883200000>;
+			opp-peak-kBps = <816000>;
+		};
+
+		cl0_ch1_opp7: opp-1036800000 {
+			opp-hz = /bits/ 64 <1036800000>;
+			opp-peak-kBps = <816000>;
+		};
+
+		cl0_ch1_opp8: opp-1190400000 {
+			opp-hz = /bits/ 64 <1190400000>;
+			opp-peak-kBps = <816000>;
+		};
+
+		cl0_ch1_opp9: opp-1344000000 {
+			opp-hz = /bits/ 64 <1344000000>;
+			opp-peak-kBps = <1632000>;
+		};
+
+		cl0_ch1_opp10: opp-1497600000 {
+			opp-hz = /bits/ 64 <1497600000>;
+			opp-peak-kBps = <1632000>;
+		};
+
+		cl0_ch1_opp11: opp-1651200000 {
+			opp-hz = /bits/ 64 <1651200000>;
+			opp-peak-kBps = <2660000>;
+		};
+
+		cl0_ch1_opp12: opp-1804800000 {
+			opp-hz = /bits/ 64 <1804800000>;
+			opp-peak-kBps = <2660000>;
+		};
+
+		cl0_ch1_opp13: opp-1958400000 {
+			opp-hz = /bits/ 64 <1958400000>;
+			opp-peak-kBps = <3200000>;
+		};
+
+		cl0_ch1_opp14: opp-2112000000 {
+			opp-hz = /bits/ 64 <2112000000>;
+			opp-peak-kBps = <6400000>;
+		};
+
+		cl0_ch1_opp15: opp-2201600000 {
+			opp-hz = /bits/ 64 <2201600000>;
+			opp-peak-kBps = <6400000>;
+		};
+	};
+
+	cl1_opp_tbl: opp-table-cluster1 {
+		compatible = "operating-points-v2";
+		opp-shared;
+
+		cl1_ch1_opp1: opp-115200000 {
+			  opp-hz = /bits/ 64 <115200000>;
+			  opp-peak-kBps = <816000>;
+		};
+
+		cl1_ch1_opp2: opp-268800000 {
+			opp-hz = /bits/ 64 <268800000>;
+			opp-peak-kBps = <816000>;
+		};
+
+		cl1_ch1_opp3: opp-422400000 {
+			opp-hz = /bits/ 64 <422400000>;
+			opp-peak-kBps = <816000>;
+		};
+
+		cl1_ch1_opp4: opp-576000000 {
+			opp-hz = /bits/ 64 <576000000>;
+			opp-peak-kBps = <816000>;
+		};
+
+		cl1_ch1_opp5: opp-729600000 {
+			opp-hz = /bits/ 64 <729600000>;
+			opp-peak-kBps = <816000>;
+		};
+
+		cl1_ch1_opp6: opp-883200000 {
+			opp-hz = /bits/ 64 <883200000>;
+			opp-peak-kBps = <816000>;
+		};
+
+		cl1_ch1_opp7: opp-1036800000 {
+			opp-hz = /bits/ 64 <1036800000>;
+			opp-peak-kBps = <816000>;
+		};
+
+		cl1_ch1_opp8: opp-1190400000 {
+			opp-hz = /bits/ 64 <1190400000>;
+			opp-peak-kBps = <816000>;
+		};
+
+		cl1_ch1_opp9: opp-1344000000 {
+			opp-hz = /bits/ 64 <1344000000>;
+			opp-peak-kBps = <1632000>;
+		};
+
+		cl1_ch1_opp10: opp-1497600000 {
+			opp-hz = /bits/ 64 <1497600000>;
+			opp-peak-kBps = <1632000>;
+		};
+
+		cl1_ch1_opp11: opp-1651200000 {
+			opp-hz = /bits/ 64 <1651200000>;
+			opp-peak-kBps = <2660000>;
+		};
+
+		cl1_ch1_opp12: opp-1804800000 {
+			opp-hz = /bits/ 64 <1804800000>;
+			opp-peak-kBps = <2660000>;
+		};
+
+		cl1_ch1_opp13: opp-1958400000 {
+			opp-hz = /bits/ 64 <1958400000>;
+			opp-peak-kBps = <3200000>;
+		};
+
+		cl1_ch1_opp14: opp-2112000000 {
+			opp-hz = /bits/ 64 <2112000000>;
+			opp-peak-kBps = <6400000>;
+		};
+
+		cl1_ch1_opp15: opp-2201600000 {
+			opp-hz = /bits/ 64 <2201600000>;
+			opp-peak-kBps = <6400000>;
+		};
+	};
+
+	cl2_opp_tbl: opp-table-cluster2 {
+		compatible = "operating-points-v2";
+		opp-shared;
+
+		cl2_ch1_opp1: opp-115200000 {
+			  opp-hz = /bits/ 64 <115200000>;
+			  opp-peak-kBps = <816000>;
+		};
+
+		cl2_ch1_opp2: opp-268800000 {
+			opp-hz = /bits/ 64 <268800000>;
+			opp-peak-kBps = <816000>;
+		};
+
+		cl2_ch1_opp3: opp-422400000 {
+			opp-hz = /bits/ 64 <422400000>;
+			opp-peak-kBps = <816000>;
+		};
+
+		cl2_ch1_opp4: opp-576000000 {
+			opp-hz = /bits/ 64 <576000000>;
+			opp-peak-kBps = <816000>;
+		};
+
+		cl2_ch1_opp5: opp-729600000 {
+			opp-hz = /bits/ 64 <729600000>;
+			opp-peak-kBps = <816000>;
+		};
+
+		cl2_ch1_opp6: opp-883200000 {
+			opp-hz = /bits/ 64 <883200000>;
+			opp-peak-kBps = <816000>;
+		};
+
+		cl2_ch1_opp7: opp-1036800000 {
+			opp-hz = /bits/ 64 <1036800000>;
+			opp-peak-kBps = <816000>;
+		};
+
+		cl2_ch1_opp8: opp-1190400000 {
+			opp-hz = /bits/ 64 <1190400000>;
+			opp-peak-kBps = <816000>;
+		};
+
+		cl2_ch1_opp9: opp-1344000000 {
+			opp-hz = /bits/ 64 <1344000000>;
+			opp-peak-kBps = <1632000>;
+		};
+
+		cl2_ch1_opp10: opp-1497600000 {
+			opp-hz = /bits/ 64 <1497600000>;
+			opp-peak-kBps = <1632000>;
+		};
+
+		cl2_ch1_opp11: opp-1651200000 {
+			opp-hz = /bits/ 64 <1651200000>;
+			opp-peak-kBps = <2660000>;
+		};
+
+		cl2_ch1_opp12: opp-1804800000 {
+			opp-hz = /bits/ 64 <1804800000>;
+			opp-peak-kBps = <2660000>;
+		};
+
+		cl2_ch1_opp13: opp-1958400000 {
+			opp-hz = /bits/ 64 <1958400000>;
+			opp-peak-kBps = <3200000>;
+		};
+
+		cl2_ch1_opp14: opp-2112000000 {
+			opp-hz = /bits/ 64 <2112000000>;
+			opp-peak-kBps = <6400000>;
+		};
+
+		cl2_ch1_opp15: opp-2201600000 {
+			opp-hz = /bits/ 64 <2201600000>;
+			opp-peak-kBps = <6400000>;
+		};
+	};
 };
-- 
2.17.1

[Patch v4 08/10] cpufreq: tegra194: add OPP support and set bandwidth

[Sumit Gupta]

Add support to use OPP table from DT in Tegra194 cpufreq driver.
Tegra SoC's receive the frequency lookup table (LUT) from BPMP-FW.
Cross check the OPP's present in DT against the LUT from BPMP-FW
and enable only those DT OPP's which are present in LUT also.

The OPP table in DT has CPU Frequency to bandwidth mapping where
the bandwidth value is per MC channel. DRAM bandwidth depends on the
number of MC channels which can vary as per the boot configuration.
This per channel bandwidth from OPP table will be later converted by
MC driver to final bandwidth value by multiplying with number of
channels before sending the request to BPMP-FW.

If OPP table is not present in DT, then use the LUT from BPMP-FW
directy as the CPU frequency table and not do the DRAM frequency
scaling which is same as the current behavior.

Now, as the CPU Frequency table is being controlling through OPP
table in DT. Keeping fewer entries in the table will create less
frequency steps and can help to scale fast to high frequencies
when required.

Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
---
 drivers/cpufreq/tegra194-cpufreq.c | 156 ++++++++++++++++++++++++++---
 1 file changed, 143 insertions(+), 13 deletions(-)

diff --git a/drivers/cpufreq/tegra194-cpufreq.c b/drivers/cpufreq/tegra194-cpufreq.c
index 5890e25d7f77..c8d03346068a 100644
--- a/drivers/cpufreq/tegra194-cpufreq.c
+++ b/drivers/cpufreq/tegra194-cpufreq.c
@@ -12,6 +12,7 @@
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
+#include <linux/units.h>
 
 #include <asm/smp_plat.h>
 
@@ -65,12 +66,36 @@ struct tegra_cpufreq_soc {
 
 struct tegra194_cpufreq_data {
 	void __iomem *regs;
-	struct cpufreq_frequency_table **tables;
+	struct cpufreq_frequency_table **bpmp_luts;
 	const struct tegra_cpufreq_soc *soc;
+	bool icc_dram_bw_scaling;
 };
 
 static struct workqueue_struct *read_counters_wq;
 
+static int tegra_cpufreq_set_bw(struct cpufreq_policy *policy, unsigned long freq_khz)
+{
+	struct tegra194_cpufreq_data *data = cpufreq_get_driver_data();
+	struct dev_pm_opp *opp;
+	struct device *dev;
+	int ret;
+
+	dev = get_cpu_device(policy->cpu);
+	if (!dev)
+		return -ENODEV;
+
+	opp = dev_pm_opp_find_freq_exact(dev, freq_khz * KHZ, true);
+	if (IS_ERR(opp))
+		return PTR_ERR(opp);
+
+	ret = dev_pm_opp_set_opp(dev, opp);
+	if (ret)
+		data->icc_dram_bw_scaling = false;
+
+	dev_pm_opp_put(opp);
+	return ret;
+}
+
 static void tegra_get_cpu_mpidr(void *mpidr)
 {
 	*((u64 *)mpidr) = read_cpuid_mpidr() & MPIDR_HWID_BITMASK;
@@ -354,7 +379,7 @@ static unsigned int tegra194_get_speed(u32 cpu)
 	 * to the last written ndiv value from freq_table. This is
 	 * done to return consistent value.
 	 */
-	cpufreq_for_each_valid_entry(pos, data->tables[clusterid]) {
+	cpufreq_for_each_valid_entry(pos, data->bpmp_luts[clusterid]) {
 		if (pos->driver_data != ndiv)
 			continue;
 
@@ -369,16 +394,93 @@ static unsigned int tegra194_get_speed(u32 cpu)
 	return rate;
 }
 
+static int tegra_cpufreq_init_cpufreq_table(struct cpufreq_policy *policy,
+					    struct cpufreq_frequency_table *bpmp_lut,
+					    struct cpufreq_frequency_table **opp_table)
+{
+	struct tegra194_cpufreq_data *data = cpufreq_get_driver_data();
+	struct cpufreq_frequency_table *freq_table = NULL;
+	struct cpufreq_frequency_table *pos;
+	struct device *cpu_dev;
+	struct dev_pm_opp *opp;
+	unsigned long rate;
+	int ret, max_opps;
+	int j = 0;
+
+	cpu_dev = get_cpu_device(policy->cpu);
+	if (!cpu_dev) {
+		pr_err("%s: failed to get cpu%d device\n", __func__, policy->cpu);
+		return -ENODEV;
+	}
+
+	/* Initialize OPP table mentioned in operating-points-v2 property in DT */
+	ret = dev_pm_opp_of_add_table_indexed(cpu_dev, 0);
+	if (!ret) {
+		max_opps = dev_pm_opp_get_opp_count(cpu_dev);
+		if (max_opps <= 0) {
+			dev_err(cpu_dev, "Failed to add OPPs\n");
+			return max_opps;
+		}
+
+		/* Disable all opps and cross-validate against LUT later */
+		for (rate = 0; ; rate++) {
+			opp = dev_pm_opp_find_freq_ceil(cpu_dev, &rate);
+			if (IS_ERR(opp))
+				break;
+
+			dev_pm_opp_put(opp);
+			dev_pm_opp_disable(cpu_dev, rate);
+		}
+	} else {
+		dev_err(cpu_dev, "Invalid or empty opp table in device tree\n");
+		data->icc_dram_bw_scaling = false;
+		return ret;
+	}
+
+	freq_table = kcalloc((max_opps + 1), sizeof(*freq_table), GFP_KERNEL);
+	if (!freq_table)
+		return -ENOMEM;
+
+	/*
+	 * Cross check the frequencies from BPMP-FW LUT against the OPP's present in DT.
+	 * Enable only those DT OPP's which are present in LUT also.
+	 */
+	cpufreq_for_each_valid_entry(pos, bpmp_lut) {
+		opp = dev_pm_opp_find_freq_exact(cpu_dev, pos->frequency * KHZ, false);
+		if (IS_ERR(opp))
+			continue;
+
+		ret = dev_pm_opp_enable(cpu_dev, pos->frequency * KHZ);
+		if (ret < 0)
+			return ret;
+
+		freq_table[j].driver_data = pos->driver_data;
+		freq_table[j].frequency = pos->frequency;
+		j++;
+	}
+
+	freq_table[j].driver_data = pos->driver_data;
+	freq_table[j].frequency = CPUFREQ_TABLE_END;
+
+	*opp_table = &freq_table[0];
+
+	dev_pm_opp_set_sharing_cpus(cpu_dev, policy->cpus);
+
+	return ret;
+}
+
 static int tegra194_cpufreq_init(struct cpufreq_policy *policy)
 {
 	struct tegra194_cpufreq_data *data = cpufreq_get_driver_data();
 	int maxcpus_per_cluster = data->soc->maxcpus_per_cluster;
+	struct cpufreq_frequency_table *freq_table;
+	struct cpufreq_frequency_table *bpmp_lut;
 	u32 start_cpu, cpu;
 	u32 clusterid;
+	int ret;
 
 	data->soc->ops->get_cpu_cluster_id(policy->cpu, NULL, &clusterid);
-
-	if (clusterid >= data->soc->num_clusters || !data->tables[clusterid])
+	if (clusterid >= data->soc->num_clusters || !data->bpmp_luts[clusterid])
 		return -EINVAL;
 
 	start_cpu = rounddown(policy->cpu, maxcpus_per_cluster);
@@ -387,9 +489,22 @@ static int tegra194_cpufreq_init(struct cpufreq_policy *policy)
 		if (cpu_possible(cpu))
 			cpumask_set_cpu(cpu, policy->cpus);
 	}
-	policy->freq_table = data->tables[clusterid];
 	policy->cpuinfo.transition_latency = TEGRA_CPUFREQ_TRANSITION_LATENCY;
 
+	bpmp_lut = data->bpmp_luts[clusterid];
+
+	if (data->icc_dram_bw_scaling) {
+		ret = tegra_cpufreq_init_cpufreq_table(policy, bpmp_lut, &freq_table);
+		if (!ret) {
+			policy->freq_table = freq_table;
+			return 0;
+		}
+	}
+
+	data->icc_dram_bw_scaling = false;
+	policy->freq_table = bpmp_lut;
+	pr_info("OPP tables missing from DT, EMC frequency scaling disabled\n");
+
 	return 0;
 }
 
@@ -406,6 +521,9 @@ static int tegra194_cpufreq_set_target(struct cpufreq_policy *policy,
 	 */
 	data->soc->ops->set_cpu_ndiv(policy, (u64)tbl->driver_data);
 
+	if (data->icc_dram_bw_scaling)
+		tegra_cpufreq_set_bw(policy, tbl->frequency);
+
 	return 0;
 }
 
@@ -439,8 +557,8 @@ static void tegra194_cpufreq_free_resources(void)
 }
 
 static struct cpufreq_frequency_table *
-init_freq_table(struct platform_device *pdev, struct tegra_bpmp *bpmp,
-		unsigned int cluster_id)
+tegra_cpufreq_bpmp_read_lut(struct platform_device *pdev, struct tegra_bpmp *bpmp,
+			    unsigned int cluster_id)
 {
 	struct cpufreq_frequency_table *freq_table;
 	struct mrq_cpu_ndiv_limits_response resp;
@@ -515,6 +633,7 @@ static int tegra194_cpufreq_probe(struct platform_device *pdev)
 	const struct tegra_cpufreq_soc *soc;
 	struct tegra194_cpufreq_data *data;
 	struct tegra_bpmp *bpmp;
+	struct device *cpu_dev;
 	int err, i;
 
 	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
@@ -530,9 +649,9 @@ static int tegra194_cpufreq_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	data->tables = devm_kcalloc(&pdev->dev, data->soc->num_clusters,
-				    sizeof(*data->tables), GFP_KERNEL);
-	if (!data->tables)
+	data->bpmp_luts = devm_kcalloc(&pdev->dev, data->soc->num_clusters,
+				       sizeof(*data->bpmp_luts), GFP_KERNEL);
+	if (!data->bpmp_luts)
 		return -ENOMEM;
 
 	if (soc->actmon_cntr_base) {
@@ -556,15 +675,26 @@ static int tegra194_cpufreq_probe(struct platform_device *pdev)
 	}
 
 	for (i = 0; i < data->soc->num_clusters; i++) {
-		data->tables[i] = init_freq_table(pdev, bpmp, i);
-		if (IS_ERR(data->tables[i])) {
-			err = PTR_ERR(data->tables[i]);
+		data->bpmp_luts[i] = tegra_cpufreq_bpmp_read_lut(pdev, bpmp, i);
+		if (IS_ERR(data->bpmp_luts[i])) {
+			err = PTR_ERR(data->bpmp_luts[i]);
 			goto err_free_res;
 		}
 	}
 
 	tegra194_cpufreq_driver.driver_data = data;
 
+	/* Check for optional OPPv2 and interconnect paths on CPU0 to enable ICC scaling */
+	cpu_dev = get_cpu_device(0);
+	if (!cpu_dev)
+		return -EPROBE_DEFER;
+
+	if (dev_pm_opp_of_get_opp_desc_node(cpu_dev)) {
+		err = dev_pm_opp_of_find_icc_paths(cpu_dev, NULL);
+		if (!err)
+			data->icc_dram_bw_scaling = true;
+	}
+
 	err = cpufreq_register_driver(&tegra194_cpufreq_driver);
 	if (!err)
 		goto put_bpmp;
-- 
2.17.1

[Patch v4 09/10] memory: tegra: make cpu cluster bw request a multiple of mc channels

[Sumit Gupta]

Make CPU cluster's bandwidth (BW) request a multiple of MC channels.
CPU OPP tables have BW info per MC channel. But, the actual BW depends
on the number of MC channels which can change as per the boot config.
Get the number of MC channels which are actually enabled in current
boot configuration and multiply the BW request from a CPU cluster with
the number of enabled MC channels. This is not required to be done for
other MC clients.

Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/memory/tegra/mc.c       | 19 +++++++++++++++++++
 drivers/memory/tegra/mc.h       |  1 +
 drivers/memory/tegra/tegra234.c | 24 +++++++++++++++++++++++-
 include/soc/tegra/mc.h          |  1 +
 4 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
index 983455b1f98d..4a750da1c12a 100644
--- a/drivers/memory/tegra/mc.c
+++ b/drivers/memory/tegra/mc.c
@@ -843,6 +843,23 @@ static int tegra_mc_interconnect_setup(struct tegra_mc *mc)
 	return err;
 }
 
+static void tegra_mc_num_channel_enabled(struct tegra_mc *mc)
+{
+	unsigned int i;
+	u32 value;
+
+	value = mc_ch_readl(mc, 0, MC_EMEM_ADR_CFG_CHANNEL_ENABLE);
+	if (value <= 0) {
+		mc->num_channels = mc->soc->num_channels;
+		return;
+	}
+
+	for (i = 0; i < 32; i++) {
+		if (value & BIT(i))
+			mc->num_channels++;
+	}
+}
+
 static int tegra_mc_probe(struct platform_device *pdev)
 {
 	struct tegra_mc *mc;
@@ -881,6 +898,8 @@ static int tegra_mc_probe(struct platform_device *pdev)
 			return err;
 	}
 
+	tegra_mc_num_channel_enabled(mc);
+
 	if (mc->soc->ops && mc->soc->ops->handle_irq) {
 		mc->irq = platform_get_irq(pdev, 0);
 		if (mc->irq < 0)
diff --git a/drivers/memory/tegra/mc.h b/drivers/memory/tegra/mc.h
index bc01586b6560..c3f6655bec60 100644
--- a/drivers/memory/tegra/mc.h
+++ b/drivers/memory/tegra/mc.h
@@ -53,6 +53,7 @@
 #define MC_ERR_ROUTE_SANITY_ADR				0x9c4
 #define MC_ERR_GENERALIZED_CARVEOUT_STATUS		0xc00
 #define MC_ERR_GENERALIZED_CARVEOUT_ADR			0xc04
+#define MC_EMEM_ADR_CFG_CHANNEL_ENABLE			0xdf8
 #define MC_GLOBAL_INTSTATUS				0xf24
 #define MC_ERR_ADR_HI					0x11fc
 
diff --git a/drivers/memory/tegra/tegra234.c b/drivers/memory/tegra/tegra234.c
index 4c8929a88778..23597e3c2d28 100644
--- a/drivers/memory/tegra/tegra234.c
+++ b/drivers/memory/tegra/tegra234.c
@@ -872,6 +872,28 @@ static int tegra234_mc_icc_set(struct icc_node *src, struct icc_node *dst)
 	return ret;
 }
 
+static int tegra234_mc_icc_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
+				     u32 peak_bw, u32 *agg_avg, u32 *agg_peak)
+{
+	struct icc_provider *p = node->provider;
+	struct tegra_mc *mc = icc_provider_to_tegra_mc(p);
+
+	if (!mc->bwmgr_mrq_supported)
+		return -EINVAL;
+
+	if (node->id == TEGRA_ICC_MC_CPU_CLUSTER0 ||
+	    node->id == TEGRA_ICC_MC_CPU_CLUSTER1 ||
+	    node->id == TEGRA_ICC_MC_CPU_CLUSTER2) {
+		if (mc)
+			peak_bw = peak_bw * mc->num_channels;
+	}
+
+	*agg_avg += avg_bw;
+	*agg_peak = max(*agg_peak, peak_bw);
+
+	return 0;
+}
+
 static struct icc_node*
 tegra234_mc_of_icc_xlate(struct of_phandle_args *spec, void *data)
 {
@@ -903,7 +925,7 @@ static int tegra234_mc_icc_get_init_bw(struct icc_node *node, u32 *avg, u32 *pea
 
 static const struct tegra_mc_icc_ops tegra234_mc_icc_ops = {
 	.xlate = tegra234_mc_of_icc_xlate,
-	.aggregate = icc_std_aggregate,
+	.aggregate = tegra234_mc_icc_aggregate,
 	.get_bw = tegra234_mc_icc_get_init_bw,
 	.set = tegra234_mc_icc_set,
 };
diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h
index 900d88b26fae..fc3001483e62 100644
--- a/include/soc/tegra/mc.h
+++ b/include/soc/tegra/mc.h
@@ -234,6 +234,7 @@ struct tegra_mc {
 
 	struct tegra_mc_timing *timings;
 	unsigned int num_timings;
+	unsigned int num_channels;
 
 	bool bwmgr_mrq_supported;
 	struct reset_controller_dev reset;
-- 
2.17.1

[Patch v4 10/10] PCI: tegra194: add interconnect support in Tegra234

[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 <mmaddireddy@nvidia.com>
Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
---
 drivers/pci/controller/dwc/pcie-tegra194.c | 40 +++++++++++++++++-----
 1 file changed, 32 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
index 09825b4a075e..d2513c9d3feb 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -15,6 +15,7 @@
 #include <linux/gpio.h>
 #include <linux/gpio/consumer.h>
 #include <linux/interrupt.h>
+#include <linux/interconnect.h>
 #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);
+
+	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]);
+}
+
 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);
 
 	if (pcie->of_data->has_ltr_req_fix)
 		return IRQ_HANDLED;
@@ -945,9 +963,9 @@ static int tegra_pcie_dw_host_init(struct dw_pcie_rp *pp)
 
 static int tegra_pcie_dw_start_link(struct dw_pcie *pci)
 {
-	u32 val, offset, speed, tmp;
 	struct tegra_pcie_dw *pcie = to_tegra_pcie(pci);
 	struct dw_pcie_rp *pp = &pci->pp;
+	u32 val, offset, tmp;
 	bool retry = true;
 
 	if (pcie->of_data->mode == DW_PCIE_EP_TYPE) {
@@ -1018,9 +1036,7 @@ static int tegra_pcie_dw_start_link(struct dw_pcie *pci)
 		goto retry_link;
 	}
 
-	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);
 
 	tegra_pcie_enable_interrupts(pp);
 
@@ -2224,6 +2240,14 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, pcie);
 
+	pcie->icc_path = devm_of_icc_get(&pdev->dev, "write");
+	ret = PTR_ERR_OR_ZERO(pcie->icc_path);
+	if (ret) {
+		tegra_bpmp_put(pcie->bpmp);
+		dev_err_probe(&pdev->dev, ret, "failed to get write interconnect\n");
+		return ret;
+	}
+
 	switch (pcie->of_data->mode) {
 	case DW_PCIE_RC_TYPE:
 		ret = devm_request_irq(dev, pp->irq, tegra_pcie_rp_irq_handler,
-- 
2.17.1

Re: [Patch v4 02/10] arm64: tegra: add bpmp ref in tegra234-mc node

[Krzysztof Kozlowski]

On 27/03/2023 18:14, Sumit Gupta wrote:
> 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 <sumitg@nvidia.com>

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

Best regards,
Krzysztof

Re: [Patch v4 01/10] dt-bindings: memory: tegra: add bpmp ref in tegra234-mc node

[Krzysztof Kozlowski]

On 27/03/2023 18:14, Sumit Gupta wrote:
> 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 <sumitg@nvidia.com>
> ---
>  .../bindings/memory-controllers/nvidia,tegra186-mc.yaml    | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
> index 935d63d181d9..398d27bb2373 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
> +++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
> @@ -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

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

> +
>  patternProperties:
>    "^external-memory-controller@[0-9a-f]+$":
>      description:
> @@ -220,6 +224,9 @@ allOf:
>              - const: ch14
>              - const: ch15
>  
> +        nvidia,bpmp:
> +          description: phandle of the node representing the BPMP

I don't understand for what this hunk is. It does not look like in
correct place at all.

Best regards,
Krzysztof

Re: [Patch v4 03/10] memory: tegra: add interconnect support for DRAM scaling in Tegra234

[Krzysztof Kozlowski]

On 27/03/2023 18:14, Sumit Gupta wrote:
> 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 <sumitg@nvidia.com>
> ---
>  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
> 
> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
> index 9082b6c3763d..983455b1f98d 100644
> --- a/drivers/memory/tegra/mc.c
> +++ b/drivers/memory/tegra/mc.c
> @@ -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);
> diff --git a/drivers/memory/tegra/tegra186-emc.c b/drivers/memory/tegra/tegra186-emc.c
> index e935ad4e95b6..1eefcf2ac0c7 100644
> --- a/drivers/memory/tegra/tegra186-emc.c
> +++ b/drivers/memory/tegra/tegra186-emc.c
> @@ -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;
> +	}
> +
> +	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

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

> +
>  	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;
> +
> +		if (tegra_bpmp_mrq_is_supported(emc->bpmp, MRQ_BWMGR_INT))
> +			mc->bwmgr_mrq_supported = true;
> +		else
> +

Drop blank line.

> +			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.

> +	}
> +
>  	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,
> diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
> index 7bb73f06fad3..386e029e41bb 100644
> --- a/drivers/memory/tegra/tegra186.c
> +++ b/drivers/memory/tegra/tegra186.c
> @@ -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)
> diff --git a/drivers/memory/tegra/tegra234.c b/drivers/memory/tegra/tegra234.c
> index 02dcc5748bba..4f34247c9bda 100644
> --- a/drivers/memory/tegra/tegra234.c
> +++ b/drivers/memory/tegra/tegra234.c
> @@ -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.

>   */
>  
>  #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...

> +#include <linux/tegra-icc.h>
>  
> +#include <soc/tegra/bpmp.h>
>  #include "mc.h"
>  
>  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

Re: [Patch v4 01/10] dt-bindings: memory: tegra: add bpmp ref in tegra234-mc node

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1529 bytes --]

On Mon, Mar 27, 2023 at 09:44:17PM +0530, Sumit Gupta wrote:
> 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 <sumitg@nvidia.com>
> ---
>  .../bindings/memory-controllers/nvidia,tegra186-mc.yaml    | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
> index 935d63d181d9..398d27bb2373 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
> +++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
> @@ -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:
> @@ -220,6 +224,9 @@ allOf:
>              - const: ch14
>              - const: ch15
>  
> +        nvidia,bpmp:
> +          description: phandle of the node representing the BPMP
> +

Why do we need this one? There's already an nvidia,bpmp phandle defined
in the patternProperties section for external memory controllers.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Patch v4 01/10] dt-bindings: memory: tegra: add bpmp ref in tegra234-mc node

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1997 bytes --]

On Tue, Mar 28, 2023 at 09:23:04AM +0200, Krzysztof Kozlowski wrote:
> On 27/03/2023 18:14, Sumit Gupta wrote:
> > 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 <sumitg@nvidia.com>
> > ---
> >  .../bindings/memory-controllers/nvidia,tegra186-mc.yaml    | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
> > index 935d63d181d9..398d27bb2373 100644
> > --- a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
> > +++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
> > @@ -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
> 
> 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.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Patch v4 03/10] memory: tegra: add interconnect support for DRAM scaling in Tegra234

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2304 bytes --]

On Tue, Mar 28, 2023 at 09:31:58AM +0200, Krzysztof Kozlowski wrote:
> On 27/03/2023 18:14, Sumit Gupta wrote:
[...]
> > diff --git a/drivers/memory/tegra/tegra186-emc.c b/drivers/memory/tegra/tegra186-emc.c
[...]
> > @@ -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

> >  	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;
> > +
> > +		if (tegra_bpmp_mrq_is_supported(emc->bpmp, MRQ_BWMGR_INT))
> > +			mc->bwmgr_mrq_supported = true;
> > +		else
> > +
> 
> Drop blank line.
> 
> > +			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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Patch v4 01/10] dt-bindings: memory: tegra: add bpmp ref in tegra234-mc node

[Krzysztof Kozlowski]

On 28/03/2023 12:48, Thierry Reding wrote:
> On Tue, Mar 28, 2023 at 09:23:04AM +0200, Krzysztof Kozlowski wrote:
>> On 27/03/2023 18:14, Sumit Gupta wrote:
>>> 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 <sumitg@nvidia.com>
>>> ---
>>>  .../bindings/memory-controllers/nvidia,tegra186-mc.yaml    | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
>>> index 935d63d181d9..398d27bb2373 100644
>>> --- a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
>>> +++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
>>> @@ -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
>>
>> 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.

Yes, deprecation would answer to my question.

Best regards,
Krzysztof

Re: [Patch v4 03/10] memory: tegra: add interconnect support for DRAM scaling in Tegra234

[Sumit Gupta]

On 28/03/23 16:35, Thierry Reding wrote:
> On Tue, Mar 28, 2023 at 09:31:58AM +0200, Krzysztof Kozlowski wrote:
>> On 27/03/2023 18:14, Sumit Gupta wrote:
> [...]
>>> diff --git a/drivers/memory/tegra/tegra186-emc.c b/drivers/memory/tegra/tegra186-emc.c
> [...]
>>> @@ -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
> 
Yes, sorry i was left over. Will remove this.
Thank you for catching.

>>>   	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;
>>> +
>>> +		if (tegra_bpmp_mrq_is_supported(emc->bpmp, MRQ_BWMGR_INT))
>>> +			mc->bwmgr_mrq_supported = true;
>>> +		else
>>> +
>>
>> Drop blank line.
>>
Ok.

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

Sure, will drop this.

Thank you,
Sumit Gupta

Re: [Patch v4 03/10] memory: tegra: add interconnect support for DRAM scaling in Tegra234

[Sumit Gupta]

On 28/03/23 13:01, Krzysztof Kozlowski wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 27/03/2023 18:14, Sumit Gupta wrote:
>> 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 <sumitg@nvidia.com>
>> ---
>>   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
>>
>> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
>> index 9082b6c3763d..983455b1f98d 100644
>> --- a/drivers/memory/tegra/mc.c
>> +++ b/drivers/memory/tegra/mc.c
>> @@ -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);
>> diff --git a/drivers/memory/tegra/tegra186-emc.c b/drivers/memory/tegra/tegra186-emc.c
>> index e935ad4e95b6..1eefcf2ac0c7 100644
>> --- a/drivers/memory/tegra/tegra186-emc.c
>> +++ b/drivers/memory/tegra/tegra186-emc.c
>> @@ -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;
>> +     }
>> +
>> +     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
> 
>> +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;
>>
>> +     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.
> 
Replied in other mail. will fix this.

>> +
>>        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;
>> +
>> +             if (tegra_bpmp_mrq_is_supported(emc->bpmp, MRQ_BWMGR_INT))
>> +                     mc->bwmgr_mrq_supported = true;
>> +             else
>> +
> 
> Drop blank line.
> 
>> +                     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.
> 
Replied in other mail. will fix this.


>> +     }
>> +
>>        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,
>> diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
>> index 7bb73f06fad3..386e029e41bb 100644
>> --- a/drivers/memory/tegra/tegra186.c
>> +++ b/drivers/memory/tegra/tegra186.c
>> @@ -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)
>> diff --git a/drivers/memory/tegra/tegra234.c b/drivers/memory/tegra/tegra234.c
>> index 02dcc5748bba..4f34247c9bda 100644
>> --- a/drivers/memory/tegra/tegra234.c
>> +++ b/drivers/memory/tegra/tegra234.c
>> @@ -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.
> 
Will fix.

>>    */
>>
>>   #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...
> 
Will remove the header file "of_device.h".

>> +#include <linux/tegra-icc.h>
>>
>> +#include <soc/tegra/bpmp.h>
>>   #include "mc.h"
>>
>>   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
> 

Re: [Patch v4 02/10] arm64: tegra: add bpmp ref in tegra234-mc node

[Sumit Gupta]

On 28/03/23 12:51, Krzysztof Kozlowski wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 27/03/2023 18:14, Sumit Gupta wrote:
>> 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 <sumitg@nvidia.com>
> 
> DTS goes to the end of patchset. If you put it here to fix
> bisectability, then your patchset is already broken.
> 
> Best regards,
> Krzysztof
> 

Ok, will move the 'Patch 2 & 7' to the end of patch set (i.e. Patch 9 & 10)
   [Patch v4 02/10] arm64: tegra: add bpmp ref in tegra234-mc node
   [Patch v4 07/10] arm64: tegra: Add cpu OPP tables and interconnects 
property

Thank you,
Sumit Gupta

Re: [Patch v4 01/10] dt-bindings: memory: tegra: add bpmp ref in tegra234-mc node

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2588 bytes --]

On Tue, Mar 28, 2023 at 01:22:26PM +0200, Krzysztof Kozlowski wrote:
> On 28/03/2023 12:48, Thierry Reding wrote:
> > On Tue, Mar 28, 2023 at 09:23:04AM +0200, Krzysztof Kozlowski wrote:
> >> On 27/03/2023 18:14, Sumit Gupta wrote:
> >>> 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 <sumitg@nvidia.com>
> >>> ---
> >>>  .../bindings/memory-controllers/nvidia,tegra186-mc.yaml    | 7 +++++++
> >>>  1 file changed, 7 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
> >>> index 935d63d181d9..398d27bb2373 100644
> >>> --- a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
> >>> +++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
> >>> @@ -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
> >>
> >> 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.
> 
> 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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Patch v4 10/10] PCI: tegra194: add interconnect support in Tegra234

[Bjorn Helgaas]

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

On Mon, Mar 27, 2023 at 09:44:26PM +0530, Sumit Gupta wrote:
> 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 <mmaddireddy@nvidia.com>
> Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
> ---
>  drivers/pci/controller/dwc/pcie-tegra194.c | 40 +++++++++++++++++-----
>  1 file changed, 32 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> index 09825b4a075e..d2513c9d3feb 100644
> --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> @@ -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.

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

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

Re: [Patch v4 10/10] PCI: tegra194: add interconnect support in Tegra234

[Sumit Gupta]

On 28/03/23 23:23, Bjorn Helgaas wrote:
> External email: Use caution opening links or attachments
> 
> 
> Capitalize subject line please, to match pcie-tegra194.c history.
> 
> On Mon, Mar 27, 2023 at 09:44:26PM +0530, Sumit Gupta wrote:
>> 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 <mmaddireddy@nvidia.com>
>> Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
>> ---
>>   drivers/pci/controller/dwc/pcie-tegra194.c | 40 +++++++++++++++++-----
>>   1 file changed, 32 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
>> index 09825b4a075e..d2513c9d3feb 100644
>> --- a/drivers/pci/controller/dwc/pcie-tegra194.c
>> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
>> @@ -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.
> 
Ok, will swap.

>>   #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);
>> +
>> +     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.
> 
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]);

Thank you,
Sumit Gupta

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

Re: [Patch v4 10/10] PCI: tegra194: add interconnect support in Tegra234

[Bjorn Helgaas]

On Wed, Mar 29, 2023 at 02:44:34PM +0530, Sumit Gupta wrote:
> On 28/03/23 23:23, Bjorn Helgaas wrote:
> > > +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);
> > > +
> > > +     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.
> > 
> 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]);

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

Re: [Patch v4 01/10] dt-bindings: memory: tegra: add bpmp ref in tegra234-mc node

[Sumit Gupta]

On 28/03/23 18:18, Thierry Reding wrote:
> On Tue, Mar 28, 2023 at 01:22:26PM +0200, Krzysztof Kozlowski wrote:
>> On 28/03/2023 12:48, Thierry Reding wrote:
>>> On Tue, Mar 28, 2023 at 09:23:04AM +0200, Krzysztof Kozlowski wrote:
>>>> On 27/03/2023 18:14, Sumit Gupta wrote:
>>>>> 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 <sumitg@nvidia.com>
>>>>> ---
>>>>>   .../bindings/memory-controllers/nvidia,tegra186-mc.yaml    | 7 +++++++
>>>>>   1 file changed, 7 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
>>>>> index 935d63d181d9..398d27bb2373 100644
>>>>> --- a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
>>>>> +++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
>>>>> @@ -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
>>>>
>>>> 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.
>>
>> 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.

  +++ b/drivers/memory/tegra/tegra186-emc.c
  @@ static int tegra186_emc_probe(struct platform_device *pdev)
	
  -          if (tegra_bpmp_mrq_is_supported(emc->bpmp, MRQ_BWMGR_INT))
  +          if (tegra_bpmp_mrq_is_supported(emc->bpmp, MRQ_BWMGR_INT)) {
                     mc->bwmgr_mrq_supported = true;
  +                  mc->bpmp = emc->bpmp;
  +          }
     }

     return 0;

   put_bpmp:
  -  tegra_bpmp_put(emc->bpmp);
  +  if (IS_ERR_OR_NULL(mc->bpmp))
  +          tegra_bpmp_put(emc->bpmp);
     return err;
   }

   static int tegra186_emc_remove(struct platform_device *pdev)
   {
     struct tegra186_emc *emc = platform_get_drvdata(pdev);
  +  struct tegra_mc *mc = dev_get_drvdata(emc->dev->parent);

     debugfs_remove_recursive(emc->debugfs.root);
  -  tegra_bpmp_put(emc->bpmp);
  +  if (IS_ERR_OR_NULL(mc->bpmp))
  +          tegra_bpmp_put(emc->bpmp);

     return 0;
   }

Re: [Patch v4 10/10] PCI: tegra194: add interconnect support in Tegra234

[Sumit Gupta]

On 29/03/23 22:29, Bjorn Helgaas wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Wed, Mar 29, 2023 at 02:44:34PM +0530, Sumit Gupta wrote:
>> On 28/03/23 23:23, Bjorn Helgaas wrote:
>>>> +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);
>>>> +
>>>> +     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.
>>>
>> 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]);
> 
> 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]);

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'.

  const unsigned char pcie_link_speed[] = {
          PCI_SPEED_UNKNOWN,              /* 0 */

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


Thank you,
Sumit Gupta

Re: [Patch v4 10/10] PCI: tegra194: add interconnect support in Tegra234

[Bjorn Helgaas]

On Wed, Mar 29, 2023 at 11:28:40PM +0530, Sumit Gupta wrote:
> On 29/03/23 22:29, Bjorn Helgaas wrote:
> > On Wed, Mar 29, 2023 at 02:44:34PM +0530, Sumit Gupta wrote:
> > > On 28/03/23 23:23, Bjorn Helgaas wrote:
> > > > > +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);
> > > > > +
> > > > > +     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.
> > > > 
> > > 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]);
> > 
> > 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]);
> 
> 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).

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

>   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

Re: [Patch v4 10/10] PCI: tegra194: add interconnect support in Tegra234

[Sumit Gupta]

On 30/03/23 00:47, Bjorn Helgaas wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Wed, Mar 29, 2023 at 11:28:40PM +0530, Sumit Gupta wrote:
>> On 29/03/23 22:29, Bjorn Helgaas wrote:
>>> On Wed, Mar 29, 2023 at 02:44:34PM +0530, Sumit Gupta wrote:
>>>> On 28/03/23 23:23, Bjorn Helgaas wrote:
>>>>>> +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);
>>>>>> +
>>>>>> +     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.
>>>>>
>>>> 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]);
>>>
>>> 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]);
>>
>> 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).
> 
>> 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.
> 
>>    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

Yes, this is working fine. Will add it in the patch.

Thank you,
Sumit Gupta

Re: [Patch v4 01/10] dt-bindings: memory: tegra: add bpmp ref in tegra234-mc node

[Krzysztof Kozlowski]

On 29/03/2023 19:12, Sumit Gupta wrote:
> 
> 
> On 28/03/23 18:18, Thierry Reding wrote:
>> On Tue, Mar 28, 2023 at 01:22:26PM +0200, Krzysztof Kozlowski wrote:
>>> On 28/03/2023 12:48, Thierry Reding wrote:
>>>> On Tue, Mar 28, 2023 at 09:23:04AM +0200, Krzysztof Kozlowski wrote:
>>>>> On 27/03/2023 18:14, Sumit Gupta wrote:
>>>>>> 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 <sumitg@nvidia.com>
>>>>>> ---
>>>>>>   .../bindings/memory-controllers/nvidia,tegra186-mc.yaml    | 7 +++++++
>>>>>>   1 file changed, 7 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
>>>>>> index 935d63d181d9..398d27bb2373 100644
>>>>>> --- a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
>>>>>> @@ -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
>>>>>
>>>>> 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.
>>>
>>> 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.

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


Best regards,
Krzysztof

Re: [Patch v4 01/10] dt-bindings: memory: tegra: add bpmp ref in tegra234-mc node

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4220 bytes --]

On Sun, Apr 02, 2023 at 12:47:01PM +0200, Krzysztof Kozlowski wrote:
> On 29/03/2023 19:12, Sumit Gupta wrote:
> > 
> > 
> > On 28/03/23 18:18, Thierry Reding wrote:
> >> On Tue, Mar 28, 2023 at 01:22:26PM +0200, Krzysztof Kozlowski wrote:
> >>> On 28/03/2023 12:48, Thierry Reding wrote:
> >>>> On Tue, Mar 28, 2023 at 09:23:04AM +0200, Krzysztof Kozlowski wrote:
> >>>>> On 27/03/2023 18:14, Sumit Gupta wrote:
> >>>>>> 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 <sumitg@nvidia.com>
> >>>>>> ---
> >>>>>>   .../bindings/memory-controllers/nvidia,tegra186-mc.yaml    | 7 +++++++
> >>>>>>   1 file changed, 7 insertions(+)
> >>>>>>
> >>>>>> diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
> >>>>>> index 935d63d181d9..398d27bb2373 100644
> >>>>>> --- a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
> >>>>>> +++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
> >>>>>> @@ -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
> >>>>>
> >>>>> 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.
> >>>
> >>> 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.
> 
> Then rather it sounds like time to fix these
> orderings/arch_initcall/missing defer.

We can't fix this because there's a circular dependency between MC and
BPMP. Essentially BPMP requires the IOMMU for mappings and the IOMMU
needs the MC to do some register programming in order for the mappings
to take effect.

The MC programming isn't required for BPMP, but it is required for other
devices that are hooked up to an IOMMU. The dependency from MC on BPMP
is also a different area of functionality, so we know that there's no
actual problem.

If there was something like a "transitive" dependency we might be able
to resolve this (i.e. if creating the IOMMU mapping itself would trigger
the probe deferral chain, rather than the driver binding). However, all
dependencies are modelled between devices, so not much we can do in this
case other than try to work around it.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]