interconnect: Add imx support via devfreq

This series add imx support for interconnect via devfreq: the ICC framework is used to aggregate requests from devices and then those are converted to DEV_PM_QOS_MIN_FREQUENCY requests for devfreq.

The devfreq parts are posted separately, this series only intersects in
devicetree: https://patchwork.kernel.org/cover/11104113/

Since there is no single devicetree node that can represent the "interconnect" new API is added to allow individual devfreq nodes to act as parsing proxies all mapping to a single soc-level icc provider. This is still RFC because this

The rest of the changes are small and deal with review comments.

Changes since RFCv3:
* Remove the virtual "icc" node and add devfreq nodes as proxy providers
* Fix build on 32-bit arm (reported by kbuilt test robot)
* Remove ARCH_MXC_ARM64 (never existed in upstream)
* Remove _numlinks, calculate instead
* Replace __BUSFREQ_H header guard
* Improve commit message and comment spelling
* Fix checkpatch issues
Link to RFCv3: https://patchwork.kernel.org/cover/11078671/

Changes since RFCv2 and initial work by Alexandre Bailon:
* Relying on devfreq and dev_pm_qos instead of CLK
* No more "platform opp" stuff
* No more special suspend handling: use suspend-opp on devfreq instead.
* Replace all mentions of "busfreq" with "interconnect"
Link to v2: https://patchwork.kernel.org/patch/11056789/

Leonard Crestez (7):
PM / devfreq: Add devfreq_get_devfreq_by_node
interconnect: Add of_icc_add_proxy
dt-bindings: devfreq: imx: Describe interconnect properties
interconnect: Add imx core driver
interconnect: imx: Add platform driver for imx8mm
soc: imx8mm: Register interconnect platform device
arm64: dts: imx8mm: Add interconnect properties

 .../devicetree/bindings/devfreq/imx-ddrc.yaml |   5 +
 .../devicetree/bindings/devfreq/imx.yaml      |   5 +
 arch/arm64/boot/dts/freescale/imx8mm.dtsi     |   5 +
 drivers/devfreq/devfreq.c                     |  42 ++-
 drivers/interconnect/Kconfig                  |   1 +
 drivers/interconnect/Makefile                 |   1 +
 drivers/interconnect/core.c                   |  88 +++++-
 drivers/interconnect/imx/Kconfig              |   9 +
 drivers/interconnect/imx/Makefile             |   2 +
 drivers/interconnect/imx/imx.c                | 279 ++++++++++++++++++
 drivers/interconnect/imx/imx.h                |  60 ++++
 drivers/interconnect/imx/imx8mm.c             | 105 +++++++
 drivers/soc/imx/soc-imx8.c                    |   4 +
 include/dt-bindings/interconnect/imx8mm.h     |  49 +++
 include/linux/devfreq.h                       |   1 +
 include/linux/interconnect-provider.h         |   7 +
 16 files changed, 645 insertions(+), 18 deletions(-)
 create mode 100644 drivers/interconnect/imx/Kconfig
 create mode 100644 drivers/interconnect/imx/Makefile
 create mode 100644 drivers/interconnect/imx/imx.c
 create mode 100644 drivers/interconnect/imx/imx.h
 create mode 100644 drivers/interconnect/imx/imx8mm.c
 create mode 100644 include/dt-bindings/interconnect/imx8mm.h

2.17.1

Split off part of devfreq_get_devfreq_by_phandle into a separate function. This allows callers to fetch devfreq instances by enumerating devicetree instead of explicit phandles.

---
 drivers/devfreq/devfreq.c | 42 +++++++++++++++++++++++++++++----------
 include/linux/devfreq.h   |  1 +
 2 files changed, 32 insertions(+), 11 deletions(-)

index 687deadd08ed..57352a757d79 100644
@@ -907,10 +907,33 @@ struct devfreq *devm_devfreq_add_device(struct device *dev,
 	return devfreq;
 }
 EXPORT_SYMBOL(devm_devfreq_add_device);
 
 #ifdef CONFIG_OF
+/*
+ * devfreq_get_devfreq_by_node - Get the devfreq device from devicetree
+ * @np - pointer to device_node
+ *
+ * return the instance of devfreq device
+ */
+struct devfreq *devfreq_get_devfreq_by_node(struct device_node *node)
+{
+	struct devfreq *devfreq;
+
+	mutex_lock(&devfreq_list_lock);
+	list_for_each_entry(devfreq, &devfreq_list, node) {
+		if (devfreq->dev.parent
+			&& devfreq->dev.parent->of_node == node) {
+			mutex_unlock(&devfreq_list_lock);
+			return devfreq;
+		}
+	}
+	mutex_unlock(&devfreq_list_lock);
+
+	return ERR_PTR(-EPROBE_DEFER);
+}
+
 /*
  * devfreq_get_devfreq_by_phandle - Get the devfreq device from devicetree
  * @dev - instance to the given device
  * @index - index into list of devfreq
  *

@@ -929,25 +952,22 @@ struct devfreq *devfreq_get_devfreq_by_phandle(struct device *dev, int index)
 
 	node = of_parse_phandle(dev->of_node, "devfreq", index);
 	if (!node)
 		return ERR_PTR(-ENODEV);
 
-	mutex_lock(&devfreq_list_lock);
-	list_for_each_entry(devfreq, &devfreq_list, node) {
-		if (devfreq->dev.parent
-			&& devfreq->dev.parent->of_node == node) {
-			mutex_unlock(&devfreq_list_lock);
-			of_node_put(node);
-			return devfreq;
-		}
-	}
-	mutex_unlock(&devfreq_list_lock);
+	devfreq = devfreq_get_devfreq_by_node(node);
 	of_node_put(node);
 
-	return ERR_PTR(-EPROBE_DEFER);
+	return devfreq;
 }
+
 #else
+struct devfreq *devfreq_get_devfreq_by_node(struct device_node *node)
+{
+	return ERR_PTR(-ENODEV);
+}
+
 struct devfreq *devfreq_get_devfreq_by_phandle(struct device *dev, int index)
 {
 	return ERR_PTR(-ENODEV);
 }
 #endif /* CONFIG_OF */

index d2c5bb7add0a..4b5cc80abbe3 100644
@@ -242,10 +242,11 @@ extern int devm_devfreq_register_notifier(struct device *dev,
 				unsigned int list);
 extern void devm_devfreq_unregister_notifier(struct device *dev,
 				struct devfreq *devfreq,
 				struct notifier_block *nb,
 				unsigned int list);
+extern struct devfreq *devfreq_get_devfreq_by_node(struct device_node *node);
 extern struct devfreq *devfreq_get_devfreq_by_phandle(struct device *dev,
 						int index);
 
 #if IS_ENABLED(CONFIG_DEVFREQ_GOV_SIMPLE_ONDEMAND)
 /**

2.17.1

On many SOCs there is no single node that describes the "interconnect", instead are multiple pieces of bus fabric which already support scaling.

Add support for mapping multiple device nodes to the same icc_provider (likely a platform-level singleton). This is implemented at the devicetree parsing level: just add more device nodes which map to the same icc_provider instead.

---
 drivers/interconnect/core.c           | 88 ++++++++++++++++++++++++---
 include/linux/interconnect-provider.h |  7 +++
 2 files changed, 88 insertions(+), 7 deletions(-)

index 7b971228df38..01109e335baf 100644
@@ -17,12 +17,19 @@
 #include <linux/mutex.h>
 #include <linux/slab.h>
 #include <linux/of.h>
 #include <linux/overflow.h>
 
+struct of_icc_proxy {
+	struct device_node *of_node;
+	struct icc_provider *provider;
+	struct list_head list_node;
+};
+
 static DEFINE_IDR(icc_idr);
 static LIST_HEAD(icc_providers);
+static LIST_HEAD(icc_proxy_list);
 static DEFINE_MUTEX(icc_lock);
 static struct dentry *icc_debugfs_dir;
 
 /**
  * struct icc_req - constraints that are attached to each node

@@ -267,10 +274,61 @@ struct icc_node *of_icc_xlate_onecell(struct of_phandle_args *spec,
 
 	return icc_data->nodes[idx];
 }
 EXPORT_SYMBOL_GPL(of_icc_xlate_onecell);
 
+struct icc_provider *__of_icc_get_provider(struct device_node *np)
+{
+	struct of_icc_proxy *proxy;
+
+	lockdep_assert_held(&icc_lock);
+	list_for_each_entry(proxy, &icc_proxy_list, list_node)
+		if (proxy->of_node == np)
+			return proxy->provider;
+
+	return NULL;
+}
+
+static int __of_icc_add_proxy(struct device_node *np,
+			      struct icc_provider *provider)
+{
+	struct of_icc_proxy *proxy;
+
+	lockdep_assert_held(&icc_lock);
+	proxy = kmalloc(sizeof(*proxy), GFP_KERNEL);
+	if (!proxy)
+		return -ENOMEM;
+	proxy->of_node = np;
+	proxy->provider = provider;
+	list_add_tail(&proxy->list_node, &icc_proxy_list);
+
+	return 0;
+}
+
+/**
+ * of_icc_add_proxy() - Add another device_node for a provider
+ * @np: OF node to alias from
+ * @provider: Interconnect provider to map to
+ *
+ * Make another device_node map to the same provider.
+ *
+ * This lasts until icc_provider_del.
+ */
+int of_icc_add_proxy(struct device_node *np, struct icc_provider *provider)
+{
+	int ret;
+
+	mutex_lock(&icc_lock);
+
+	ret = __of_icc_add_proxy(np, provider);
+
+	mutex_unlock(&icc_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(of_icc_add_proxy);
+
 /**
  * of_icc_get_from_provider() - Look-up interconnect node
  * @spec: OF phandle args to use for look-up
  *
  * Looks for interconnect provider under the node specified by @spec and if

@@ -279,23 +337,22 @@ EXPORT_SYMBOL_GPL(of_icc_xlate_onecell);
  * Returns a valid pointer to struct icc_node on success or ERR_PTR()
  * on failure.
  */
 static struct icc_node *of_icc_get_from_provider(struct of_phandle_args *spec)
 {
-	struct icc_node *node = ERR_PTR(-EPROBE_DEFER);
 	struct icc_provider *provider;
+	struct icc_node *node;
 
 	if (!spec || spec->args_count != 1)
 		return ERR_PTR(-EINVAL);
 
 	mutex_lock(&icc_lock);
-	list_for_each_entry(provider, &icc_providers, provider_list) {
-		if (provider->dev->of_node == spec->np)
-			node = provider->xlate(spec, provider->data);
-		if (!IS_ERR(node))
-			break;
-	}
+	provider = __of_icc_get_provider(spec->np);
+	if (provider)
+		node = provider->xlate(spec, provider->data);
+	else
+		node = ERR_PTR(-EPROBE_DEFER);
 	mutex_unlock(&icc_lock);
 
 	return node;
 }
 

@@ -744,17 +801,26 @@ EXPORT_SYMBOL_GPL(icc_node_del);
  *
  * Return: 0 on success, or an error code otherwise
  */
 int icc_provider_add(struct icc_provider *provider)
 {
+	int ret;
+
 	if (WARN_ON(!provider->set))
 		return -EINVAL;
 	if (WARN_ON(!provider->xlate))
 		return -EINVAL;
 
 	mutex_lock(&icc_lock);
 
+	if (provider->dev) {
+		ret = __of_icc_add_proxy(provider->dev->of_node, provider);
+		if (ret) {
+			mutex_unlock(&icc_lock);
+			return ret;
+		}
+	}
 	INIT_LIST_HEAD(&provider->nodes);
 	list_add_tail(&provider->provider_list, &icc_providers);
 
 	mutex_unlock(&icc_lock);
 

@@ -770,10 +836,12 @@ EXPORT_SYMBOL_GPL(icc_provider_add);
  *
  * Return: 0 on success, or an error code otherwise
  */
 int icc_provider_del(struct icc_provider *provider)
 {
+	struct of_icc_proxy *proxy, *tmp;
+
 	mutex_lock(&icc_lock);
 	if (provider->users) {
 		pr_warn("interconnect provider still has %d users\n",
 			provider->users);
 		mutex_unlock(&icc_lock);

@@ -785,10 +853,16 @@ int icc_provider_del(struct icc_provider *provider)
 		mutex_unlock(&icc_lock);
 		return -EBUSY;
 	}
 
 	list_del(&provider->provider_list);
+	list_for_each_entry_safe(proxy, tmp, &icc_proxy_list, list_node)
+		if (proxy->provider == provider) {
+			list_del(&proxy->list_node);
+			of_node_put(proxy->of_node);
+			kfree(proxy);
+		}
 	mutex_unlock(&icc_lock);
 
 	return 0;
 }
 EXPORT_SYMBOL_GPL(icc_provider_del);

index b16f9effa555..e6773ecac164 100644
@@ -98,10 +98,11 @@ int icc_link_create(struct icc_node *node, const int dst_id);
 int icc_link_destroy(struct icc_node *src, struct icc_node *dst);
 void icc_node_add(struct icc_node *node, struct icc_provider *provider);
 void icc_node_del(struct icc_node *node);
 int icc_provider_add(struct icc_provider *provider);
 int icc_provider_del(struct icc_provider *provider);
+int of_icc_add_proxy(struct device_node *np, struct icc_provider *provider);
 
 #else
 
 static inline struct icc_node *icc_node_create(int id)
 {

@@ -138,8 +139,14 @@ static inline int icc_provider_add(struct icc_provider *provider)
 static inline int icc_provider_del(struct icc_provider *provider)
 {
 	return -ENOTSUPP;
 }
 
+static inline int of_icc_add_proxy(struct device_node *np,
+				   struct icc_provider *provider)
+{
+	return -ENOTSUPP;
+}
+
 #endif /* CONFIG_INTERCONNECT */
 
 #endif /* __LINUX_INTERCONNECT_PROVIDER_H */

2.17.1

The interconnect-node-id property is parsed by the imx interconnect driver to find nodes on which frequencies can be adjusted.

Add #interconnect-cells so that device drivers can request paths from bus nodes instead of requiring a separate "virtual" node to represent the interconnect itself.

---
 Documentation/devicetree/bindings/devfreq/imx-ddrc.yaml | 5 +++++
 Documentation/devicetree/bindings/devfreq/imx.yaml      | 5 +++++
 2 files changed, 10 insertions(+)

index 31db204e6845..014449a9dd01 100644
@@ -31,10 +31,15 @@ properties:
       - const: dram_alt
       - const: dram_apb
 
   operating-points-v2: true
 
+  interconnect-node-id:
+    $ref: /schemas/types.yaml#/definitions/uint32
+  '#interconnect-cells':
+    const: 1
+
   devfreq-events:
     description: Phandle of PMU node
     $ref: "/schemas/types.yaml#/definitions/phandle"
 
 required:

index 634870496d5e..f2f9b76c752f 100644
@@ -43,10 +43,15 @@ properties:
   clocks:
     maxItems: 1
 
   operating-points-v2: true
 
+  interconnect-node-id:
+    $ref: /schemas/types.yaml#/definitions/uint32
+  '#interconnect-cells':
+    const: 1
+
   devfreq:
     description: |
       Phandle to another devfreq device to match OPPs with by using the
       passive governor.
     $ref: "/schemas/types.yaml#/definitions/phandle"

2.17.1

Please combine this with the other series for devfreq support.

Rob

I understand that having two series which add to the same bindings file is odd but the devfreq and interconnect parts are independent to a very large degree and devfreq can be useful on it's own.

The only reason devfreq bindings are updated is to avoid adding a "virtual" node for interconnect. Since DT is a big source of confusion here can you read https://patchwork.kernel.org/cover/11111865/#22883457
and maybe offer some advice?

--
Regards,
Leonard

To start with, I'm suspicious of any 'devfreq' binding because that's a Linux thing. I somewhat expect that the interconnect binding should replace the devfreq binding, but I haven't been able to investigate.

Design something that matches the structure of the h/w not how Linux drivers happen to be structured. I can't tell what that is without any context around adding a couple of properties. Nor do I have the time to dig into each SoC vendor's bus structure if it's even documented publicly.

I also don't follow why you need 'interconnect-node-id' and if you do, it should be a common property.

Rob

Device tree binding files are organized based on linux subsystems but otherwise there's little particularly specific to "devfreq" in this new binding.

An imx NIC or NOC is a physical component of a SOC which can run at a variable clock speed. The device binding uses standard clk and opp tables in meaningful ways so that first part is reasonable.

I also want to implement an interconnect provider but an "interconnect" is not a single device but rather a graph of discrete buses. Some options:
* Add a custom virtual device, easy but not upstreamable.
* Have a single icc provider device use the individual buses as "proxies" for OF parting. Implemented in this series but not very pretty.
* Pick the "main noc" as the single interconnect provider?

Alternatively the approach of defining the graph in the driver could be dropped and everything could be described in DT (quite verbose). This seems to be what Samsung is doing:

https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=176291

It might even possible to use of_graph; this is complicated but the NICs and NOCs do in fact have discrete ports with assignable properties.

The "interconnect-node-id" property in devicetree identifies nodes from the interconnect graph; for example the DDRC node identifies itself as IMX8MM_ICS_DRAM.

--
Regards,
Leonard

This adds support for i.MX SoC family to interconnect framework.

Platform drivers can describe the interconnect graph and several adjustment knobs where icc node bandwidth is converted to a DEV_PM_QOS_MIN_FREQUENCY request.

The adjustable nodes are found based on an "interconnect-node-id" property by scanning the entire device tree.

The interconnect provider doesn't need an virtual OF node, instead those same adjustable nodes are registered as proxies which xlate to the platform-level provider.

The platform device for the interconnect needs to be registered from a SOC driver (similar to cpufreq).

---
 drivers/interconnect/Kconfig      |   1 +
 drivers/interconnect/Makefile     |   1 +
 drivers/interconnect/imx/Kconfig  |   5 +
 drivers/interconnect/imx/Makefile |   1 +
 drivers/interconnect/imx/imx.c    | 279 ++++++++++++++++++++++++++++++
 drivers/interconnect/imx/imx.h    |  60 +++++++
 6 files changed, 347 insertions(+)
 create mode 100644 drivers/interconnect/imx/Kconfig
 create mode 100644 drivers/interconnect/imx/Makefile
 create mode 100644 drivers/interconnect/imx/imx.c
 create mode 100644 drivers/interconnect/imx/imx.h

index bfa4ca3ab7a9..e61802230f90 100644
@@ -10,7 +10,8 @@ menuconfig INTERCONNECT
 	  If unsure, say no.
 
 if INTERCONNECT
 
 source "drivers/interconnect/qcom/Kconfig"
+source "drivers/interconnect/imx/Kconfig"
 
 endif

index 28f2ab0824d5..20a13b7eb37f 100644
@@ -2,5 +2,6 @@
 
 icc-core-objs				:= core.o
 
 obj-$(CONFIG_INTERCONNECT)		+= icc-core.o
 obj-$(CONFIG_INTERCONNECT_QCOM)		+= qcom/
+obj-$(CONFIG_INTERCONNECT_IMX)		+= imx/

new file mode 100644
index 000000000000..7d81d3c83a61
@@ -0,0 +1,5 @@
+config INTERCONNECT_IMX
+	bool "i.MX interconnect drivers"
+	depends on ARCH_MXC || COMPILE_TEST
+	help
+	  Generic interconnect driver for i.MX SOCs

new file mode 100644
index 000000000000..bb92fd9fe4a5
@@ -0,0 +1 @@
+obj-$(CONFIG_INTERCONNECT_IMX) += imx.o

2.17.1

Add a platform driver for the i.MX8MM SoC describing bus topology.

Bandwidth adjustments is currently only supported on the DDRC and main NOC. Scaling for the vpu/gpu/display NICs could be added in the future.

---
 drivers/interconnect/imx/Kconfig          |   4 +
 drivers/interconnect/imx/Makefile         |   1 +
 drivers/interconnect/imx/imx8mm.c         | 105 ++++++++++++++++++++++
 include/dt-bindings/interconnect/imx8mm.h |  49 ++++++++++
 4 files changed, 159 insertions(+)
 create mode 100644 drivers/interconnect/imx/imx8mm.c
 create mode 100644 include/dt-bindings/interconnect/imx8mm.h

index 7d81d3c83a61..15671fe7f600 100644
@@ -1,5 +1,9 @@
 config INTERCONNECT_IMX
 	bool "i.MX interconnect drivers"
 	depends on ARCH_MXC || COMPILE_TEST
 	help
 	  Generic interconnect driver for i.MX SOCs
+
+config INTERCONNECT_IMX8MM
+	def_bool y
+	depends on INTERCONNECT_IMX

index bb92fd9fe4a5..5f658c1608a6 100644
@@ -1 +1,2 @@
 obj-$(CONFIG_INTERCONNECT_IMX) += imx.o
+obj-$(CONFIG_INTERCONNECT_IMX8MM) += imx8mm.o

2.17.1

Since there is no virtual devicetree node representing the interconnect we need to probe the icc device externally. Probing this from the SOC driver allows the interconnect device to be built as a module.

This is very similar to imx-cpufreq-dt.

---
 drivers/soc/imx/soc-imx8.c | 4 ++++
 1 file changed, 4 insertions(+)

index b9831576dd25..24d515a9fdb2 100644
@@ -22,10 +22,11 @@
 /* Same as ANADIG_DIGPROG_IMX7D */
 #define ANADIG_DIGPROG_IMX8MM	0x800
 
 struct imx8_soc_data {
 	char *name;
+	char *icc_driver;
 	u32 (*soc_revision)(void);
 };
 
 static u64 soc_uid;
 

@@ -115,10 +116,11 @@ static const struct imx8_soc_data imx8mq_soc_data = {
 };
 
 static const struct imx8_soc_data imx8mm_soc_data = {
 	.name = "i.MX8MM",
 	.soc_revision = imx8mm_soc_revision,
+	.icc_driver = "imx8mm-interconnect",
 };
 
 static const struct imx8_soc_data imx8mn_soc_data = {
 	.name = "i.MX8MN",
 	.soc_revision = imx8mm_soc_revision,

@@ -185,10 +187,12 @@ static int __init imx8_soc_init(void)
 	if (ret)
 		goto free_rev;
 
 	if (IS_ENABLED(CONFIG_ARM_IMX_CPUFREQ_DT))
 		platform_device_register_simple("imx-cpufreq-dt", -1, NULL, 0);
+	if (IS_ENABLED(CONFIG_INTERCONNECT_IMX))
+		platform_device_register_simple(data->icc_driver, -1, NULL, 0);
 
 	return 0;
 
 free_rev:
 	if (strcmp(soc_dev_attr->revision, "unknown"))

2.17.1

Add #interconnect-cells and interconnect-node-id properties on devfreq nodes. The imx interconnect provider will scan these.

---
 arch/arm64/boot/dts/freescale/imx8mm.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

index 5474c50784c2..8b5442d8b1b2 100644
@@ -6,10 +6,11 @@
 #include <dt-bindings/clock/imx8mm-clock.h>
 #include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/input/input.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/thermal/thermal.h>
+#include <dt-bindings/interconnect/imx8mm.h>
 
 #include "imx8mm-pinfunc.h"
 
 / {
 	compatible = "fsl,imx8mm";

@@ -806,10 +807,12 @@
 		noc: noc@32700000 {
 			compatible = "fsl,imx8mm-noc", "fsl,imx8m-noc";
 			reg = <0x32700000 0x100000>;
 			clocks = <&clk IMX8MM_CLK_NOC>;
 			devfreq = <&ddrc>;
+			#interconnect-cells = <1>;
+			interconnect-node-id = <IMX8MM_ICN_NOC>;
 			operating-points-v2 = <&noc_opp_table>;
 		};
 
 		aips4: bus@32c00000 {
 			compatible = "fsl,aips-bus", "simple-bus";

@@ -896,10 +899,12 @@
 		};
 
 		ddrc: dram-controller@3d400000 {
 			compatible = "fsl,imx8mm-ddrc", "fsl,imx8m-ddrc";
 			reg = <0x3d400000 0x400000>;
+			#interconnect-cells = <1>;
+			interconnect-node-id = <IMX8MM_ICS_DRAM>;
 			clock-names = "dram_core",
 				      "dram_pll",
 				      "dram_alt",
 				      "dram_apb";
 			clocks = <&clk IMX8MM_CLK_DRAM_CORE>,

2.17.1

Any comments? I made a lot of changes relative to previous versions, most of them solely to avoid adding a virtual node in DT bindings.

The only current interconnect provider implementation is for qcom and it uses a firmware node as the provider node (with #interconnect-cells). However there is no obvious equivalent of that for imx and many other SOCs.

On imx there are multiple pieces of scalable fabric which can be defined in DT as devfreq devices and it sort of makes sense to add #interconnect-cells to those. However when it comes to describing the SOC interconnect graph it's much more convenient to have a single per-SOC platform driver.

My solution is to add an "icc_proxy" API so that a single platform-level interconnect provider can be referenced through multiple DT nodes. Does this make sense?

The implementation is not very pretty, the interconnect platform devices ends up enumerating the entire devicetree in order to find proxies.

Right now the interconnect API use a relatively standard split between consumer and "provider" but I think it might make sense to have a separate abstractions for "graph" and "midnode". A "midnode" could act as a DT proxy if there is no single representation of the "interconnect" and it could support custom scaling for itself (with the default being scaling kbps into MIN_FREQ).

There are also other options:
* Pick one "main" bus and bless it as the "interconnect provider". I
want to represent buses as devfreq devices so I would have to call from devfreq to ICC for registration somehow.
* Maybe the "no virtual device" rule could be relaxed for the
interconnect subsystem?

--
Regards,
Leonard

Hi Leonard,

Not sure if it will help, but have you seen the qcs404 interconnect driver? There is also mt8183 interconnect provider driver on LKML.

Is all the NoC configuration done only by ATF? Are there any NoC related memory mapped registers?

Thanks,
Georgi

Yes, but only yesterday. The qcs404 driver involves multiple DT devices so it seems closer to imx.

As far as I understand from reading qcs404 source:

* There is no struct device representing the entire graph.
* There are multiple NOCs and each registers itself as a separate interconnect provider.
* Each NOC registers multiple icc_nodes of various sorts:
* Device masters and slaves
* Some nodes representing NoC ports?
* Multiple internal nodes
* There is single per-SOC master list of QNOCs in the qcs404 driver.
* The QNOCs can reference each other between multiple providers.
* Each NOC registers an icc_provider and a subset of the graph.
* The multiple NoC inside a chip are distinguished by compat strings. This seems strange, aren't they really different instantiations of the same IP with small config changes?

This design is still quite odd, what would make sense to me is to register the "interconnect graph" once and then provide multiple "interconnect scalers" which handle the aggregated requests for certain specific nodes.

Registers are memory-mapped and visible to the A-cores but should only be accessed through secure transactions. This means that configuration needs be done by ATF in EL3 (we don't support running linux in secure world on imx8m). There is no "remote processor" managing this on imx8m.

On older imx6/7 chips we actually have two out-of-tree implementations of bus freq switching code: An older one in Linux (used when running in secure world) and a different one in optee for running Linux in non-secure world.

NoC registers can be used to control some "transaction priority" bits but I don't want to expose that part right now.

What determines bandwidth versus power consumption is the NoC clk rate and clocks are managed by Linux directly.

DVFS on the RAM controller (DDRC) is also important. That component is only a bus slave and frequency switching requires a complex sequence inside ATF.

--
Regards,
Leonard

Well, all nodes are representing ports.

No, they are different IPs - ahb, axi or custom based.

Can we create some noc DT node with it's memory mapped address and make it an interconnect provider? Sounds to me like a more correct representation of the hardware?
Other option would be to bless some PSCI DT node (for example) to be a provider.

This is very similar to some of the Qcom hardware.

So you will need to describe these clocks in the interconnect provider DT node like on qcs404.

Makes sense.

Thanks,
Georgi

On IMX some of the buses are just different instantiations.

Would it make sense to standardize an "interconnect-node-id" to identify middle nodes? For example if you have nearly identical "audio" "display" "vpu" NICs then this property would make it possible to map from a DT done to an ICC graph node.

This is what I did, it's just that the initial binding is in this series:
https://patchwork.kernel.org/cover/11104113/
https://patchwork.kernel.org/patch/11104137/
https://patchwork.kernel.org/patch/11104141/

The nodes are scaled via devfreq and interconnect comes "on top" to make device bandwidth requests.

I think using devfreq is valuable for example:
* DDRC can support reactive scaling based on performance counters
* The NOC can run at different voltages so it should have it's own OPP
table.

I don't think this can be a good fit, I want to support different interconnect nodes with different underlying interfaces on the same SOC.

There is no abstraction layer in firmware so abstractions for different interconnect midnodes should be in linux instead.

NoC IP is licensed from Arteris which was bought-out by Qcom. Documentation is not public though and there are likely many differences versus what Qcom has in their own chips.

I already implemented the nodes as devfreq provider and DDRC even includes ondemand reactive scaling support:

https://patchwork.kernel.org/patch/11104139/
https://patchwork.kernel.org/patch/11104145/
https://patchwork.kernel.org/patch/11104143/

I could just pick the "main" NOC and turn than into the "only" interconnect provider. Something like this:

if (has_property(noc_device, "#interconnect-cells")) {
     register_soc_icc_driver(noc_device);
}

This would get rid of the icc_proxy stuff but fetching references to other scalable nodes would require some other way to identify them.

--
Regards,
Leonard

hi Leonard,

When I run https://github.com/cdleonard/linux/tree/next_imx_busfreq and
https://github.com/cdleonard/arm-trusted-firmware/tree/imx_2.0.y_busfreq
on imx8mq, probe() fails:

[    1.082847] imx-ddrc-devfreq 3d400000.dram-controller: failed to init
firmware freq info: -19
[    1.091434] imx-ddrc-devfreq: probe of 3d400000.dram-controller

rejects match -19

in imx_ddrc_init_freq_info()'s check:

if (priv->freq_count <= 0 || priv->freq_count > IMX_DDRC_MAX_FREQ_COUNT)

That would indicate that I'm missing something in ATF? I'm pretty sure I'm running your tree though.

Does anything come to your mind what I might be doing wrong? Am I running your "correct" linux tree? Remember I'm on imx8mq, so I don't exactly test this RFC of yours.

Also, how are your plans to rebase / update your ATF tree?

And since there's a lot in there: what additions are necessary for this devfreq to work?

Lastly, how do you test? Is /sys/class/devfreq supposted to get populated?

thanks for your work!

martin

What is your board and uboot version? I tested on imx8mq-evk (SOC Rev B1) with NXP uboot. For example this uboot release works:

https://source.codeaurora.org/external/imx/uboot-imx/log/?h=imx_v2019.04_4.19.35_1.0.0

It is uboot which trains DDR for multiple frequencies and then passes that info to ATF. I'm not sure about the steps required to enable this for 3rd-party boards, should be same as for busfreq from NXP tree.

Getting this to work on a 3rd-party board would be interesting.

The ATF changes will show up in a future release of NXP ATF branch, when that happens I will drop my branch. NXP ATF releases are on CAF:

https://source.codeaurora.org/external/imx/imx-atf/

devfreq imx support here: https://patchwork.kernel.org/cover/11104113/
Interconnect support also needs PM QoS support for devfreq:

     https://patchwork.kernel.org/cover/11157649/

Yes, and only the devfreq patches are required for that.

# cat /sys/class/devfreq/devfreq0/available_frequencies
25000000 100000000 800000000
# cat /sys/class/devfreq/devfreq0/cur_freq
800000000

You should be able to control frequencies manually with the userspace governor:

# echo "userspace" > /sys/class/devfreq/devfreq0/governor
# echo "25000000" > /sys/class/devfreq/devfreq0/userspace/set_freq

This series allows devices to request guaranteed bandwidth for themselves through the interconnect subsystem, without it DRAM freq will still switch but you'll have problems like display corruption as it turns on before freq goes up. You can check that probe worked by doing

# cat /sys/kernel/debug/interconnect/interconnect_summary

--
Regards,
Leonard