[PATCH v4 0/5] memory: Introduce memory controller mini-framework

[PATCH v4 0/5] memory: Introduce memory controller mini-framework

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Hi,

this set of patches adds a new binding that allows device tree nodes to
explicitly define the DMA parent for a given device. This supplements
the existing interconnect bindings and is useful to disambiguate in the
case where a device has multiple paths to system memory. Beyond that it
can also be useful when there aren't any actual interconnect paths that
can be controlled, so in simple cases this can serve as a simpler
variant of interconnect paths.

One other case where this is useful is to describe the relationship
between devices such as the memory controller and an IOMMU, for example.
On Tegra186 and later, the memory controller is programmed with a set of
stream IDs that are to be associated with each memory client. This
programming needs to happen before translations through the IOMMU start,
otherwise the used stream IDs may deviate from the expected values. The
memory-controllers property is used in this case to ensure that the
memory controller driver has been probed (and hence has programmed the
stream ID mappings) before the IOMMU becomes available.

Patch 1 introduces the memory controller bindings, both from the
perspective of the provider and the consumer. Patch 2 makes use of a
memory-controllers property to determine the DMA parent for the purpose
of setting up DMA masks (based on the dma-ranges property of the DMA
parent). Patch 3 introduces a minimalistic framework that is used to
register memory controllers with along with a set of helpers to look up
the memory controller from device tree.

An example of how to register a memory controller is shown in patch 4
for Tegra186 (and later) and finally the ARM SMMU driver is extended to
become a consumer of an (optional) memory controller. As described
above, the goal is to defer probe as long as the memory controller has
not yet programmed the stream ID mappings.

Thierry

Thierry Reding (5):
  dt-bindings: Add memory controller bindings
  of: Use memory-controllers property for DMA parent
  memory: Introduce memory controller mini-framework
  memory: tegra186: Register as memory controller
  iommu: arm-smmu: Get reference to memory controller

 .../bindings/memory-controllers/consumer.yaml |  14 +
 .../memory-controllers/memory-controller.yaml |  32 +++
 drivers/iommu/arm-smmu.c                      |  11 +
 drivers/iommu/arm-smmu.h                      |   2 +
 drivers/memory/Makefile                       |   1 +
 drivers/memory/core.c                         | 248 ++++++++++++++++++
 drivers/memory/tegra/tegra186.c               |   9 +-
 drivers/of/address.c                          |  25 +-
 include/linux/memory-controller.h             |  34 +++
 9 files changed, 366 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/consumer.yaml
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/memory-controller.yaml
 create mode 100644 drivers/memory/core.c
 create mode 100644 include/linux/memory-controller.h

-- 
2.24.1

[PATCH v4 1/5] dt-bindings: Add memory controller bindings

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Add the DT schema for memory controller and consumer bindings.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 .../bindings/memory-controllers/consumer.yaml | 14 ++++++++
 .../memory-controllers/memory-controller.yaml | 32 +++++++++++++++++++
 2 files changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/consumer.yaml
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/memory-controller.yaml

diff --git a/Documentation/devicetree/bindings/memory-controllers/consumer.yaml b/Documentation/devicetree/bindings/memory-controllers/consumer.yaml
new file mode 100644
index 000000000000..7b71a6110c51
--- /dev/null
+++ b/Documentation/devicetree/bindings/memory-controllers/consumer.yaml
@@ -0,0 +1,14 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/memory-controllers/consumer.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Common memory controller consumer binding
+
+maintainers:
+  - Thierry Reding <thierry.reding@gmail.com>
+
+properties:
+  memory-controller:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
diff --git a/Documentation/devicetree/bindings/memory-controllers/memory-controller.yaml b/Documentation/devicetree/bindings/memory-controllers/memory-controller.yaml
new file mode 100644
index 000000000000..26257a666c3c
--- /dev/null
+++ b/Documentation/devicetree/bindings/memory-controllers/memory-controller.yaml
@@ -0,0 +1,32 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/memory-controllers/memory-controller.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Common memory controller binding
+
+maintainers:
+  - Thierry Reding <thierry.reding@gmail.com>
+
+description: |
+  The memory access hierarchy in a modern device can be fairly complicated.
+  Accesses to system memory typically end up going through a memory controller
+  that ensures that data is stored. Along the way, these accesses can undergo
+  classification and be prioritized and/or arbitrated.
+
+  The interconnect bindings (see ../interconnect/interconnect.txt) provides a
+  way of describing the data paths between devices and system memory. However
+  these interconnect paths, in order to be most flexible, describe the paths
+  in a very fine-grained way, so situations can arise where it is no longer
+  possible to derive a unique memory parent for any given device.
+
+  In order to remove such potential ambiguities, a memory controller can be
+  specified in device tree. A memory controller specified in this way will be
+  used as the DMA parent for a given device. The memory controller defines a
+  memory bus via the "dma-ranges" property, which will in turn be used to set
+  the range of memory accessible to DMA children of the memory controller.
+
+properties:
+  "#memory-controller-cells": true
+  dma-ranges: true
-- 
2.24.1

[PATCH v4 2/5] of: Use memory-controllers property for DMA parent

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Prefer the memory-controllers property to determine the DMA parent of a
device over the interconnects property, which can be ambiguous since it
can be used to describe multiple paths to system memory.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/of/address.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index e8a39c3ec4d4..ae841bd36bb0 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -697,17 +697,24 @@ static struct device_node *__of_get_dma_parent(const struct device_node *np)
 	struct of_phandle_args args;
 	int ret, index;
 
-	index = of_property_match_string(np, "interconnect-names", "dma-mem");
-	if (index < 0)
-		return of_get_parent(np);
+	ret = of_parse_phandle_with_args(np, "memory-controllers",
+					 "#memory-controller-cells",
+					 0, &args);
+	if (!ret) {
+		return of_node_get(args.np);
+	}
 
-	ret = of_parse_phandle_with_args(np, "interconnects",
-					 "#interconnect-cells",
-					 index, &args);
-	if (ret < 0)
-		return of_get_parent(np);
+	index = of_property_match_string(np, "interconnect-names", "dma-mem");
+	if (index >= 0) {
+		ret = of_parse_phandle_with_args(np, "interconnects",
+						 "#interconnect-cells",
+						 index, &args);
+		if (!ret) {
+			return of_node_get(args.np);
+		}
+	}
 
-	return of_node_get(args.np);
+	return of_get_parent(np);
 }
 
 static struct device_node *of_get_next_dma_parent(struct device_node *np)
-- 
2.24.1

[PATCH v4 3/5] memory: Introduce memory controller mini-framework

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

This new framework is currently nothing more than a registry of memory
controllers, with the goal being to order device probing. One use-case
where this is useful, for example, is a memory controller device which
needs to program some registers before the system MMU can be enabled.
Associating the memory controller with the SMMU allows the SMMU driver
to defer the probe until the memory controller has been registered.

One such example is Tegra186 where the memory controller contains some
registers that are used to program stream IDs for the various memory
clients (display, USB, PCI, ...) in the system. Programming these SIDs
is required for the memory clients to emit the proper SIDs as part of
their memory requests. The memory controller driver therefore needs to
be programmed prior to the SMMU driver. To achieve that, the memory
controller will be referenced via phandle from the SMMU device tree
node, the SMMU driver can then use the memory controller framework to
find it and defer probe until it has been registered.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v3:
- add device-managed variants of the consumer APIs
- add kerneldoc

Changes in v2:
- fix double unlock (Dan Carpenter, kbuild test robot)
- add helper to get optional memory controllers
- acquire and release module reference

 drivers/memory/Makefile           |   1 +
 drivers/memory/core.c             | 248 ++++++++++++++++++++++++++++++
 include/linux/memory-controller.h |  34 ++++
 3 files changed, 283 insertions(+)
 create mode 100644 drivers/memory/core.c
 create mode 100644 include/linux/memory-controller.h

diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
index 27b493435e61..d16e7dca8ef9 100644
--- a/drivers/memory/Makefile
+++ b/drivers/memory/Makefile
@@ -3,6 +3,7 @@
 # Makefile for memory devices
 #
 
+obj-y				+= core.o
 obj-$(CONFIG_DDR)		+= jedec_ddr_data.o
 ifeq ($(CONFIG_DDR),y)
 obj-$(CONFIG_OF)		+= of_memory.o
diff --git a/drivers/memory/core.c b/drivers/memory/core.c
new file mode 100644
index 000000000000..b2fbd2e808de
--- /dev/null
+++ b/drivers/memory/core.c
@@ -0,0 +1,248 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019-2020 NVIDIA Corporation.
+ */
+
+#include <linux/memory-controller.h>
+#include <linux/module.h>
+#include <linux/of.h>
+
+static DEFINE_MUTEX(controllers_lock);
+static LIST_HEAD(controllers);
+
+static void memory_controller_release(struct kref *ref)
+{
+	struct memory_controller *mc = container_of(ref, struct memory_controller, ref);
+
+	WARN_ON(!list_empty(&mc->list));
+}
+
+/**
+ * memory_controller_register() - register a memory controller
+ * @mc: memory controller
+ */
+int memory_controller_register(struct memory_controller *mc)
+{
+	kref_init(&mc->ref);
+
+	mutex_lock(&controllers_lock);
+	list_add_tail(&mc->list, &controllers);
+	mutex_unlock(&controllers_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(memory_controller_register);
+
+/**
+ * memory_controller_unregister() - unregister a memory controller
+ * @mc: memory controller
+ */
+void memory_controller_unregister(struct memory_controller *mc)
+{
+	mutex_lock(&controllers_lock);
+	list_del_init(&mc->list);
+	mutex_unlock(&controllers_lock);
+
+	kref_put(&mc->ref, memory_controller_release);
+}
+EXPORT_SYMBOL_GPL(memory_controller_unregister);
+
+static struct memory_controller *
+of_memory_controller_get(struct device *dev, struct device_node *np,
+			 const char *con_id)
+{
+	const char *cells = "#memory-controller-cells";
+	const char *names = "memory-controller-names";
+	const char *prop = "memory-controllers";
+	struct memory_controller *mc;
+	struct of_phandle_args args;
+	int index = 0, err;
+
+	if (con_id) {
+		index = of_property_match_string(np, names, con_id);
+		if (index < 0)
+			return ERR_PTR(index);
+	}
+
+	err = of_parse_phandle_with_args(np, prop, cells, index, &args);
+	if (err) {
+		if (err == -ENOENT)
+			err = -ENODEV;
+
+		return ERR_PTR(err);
+	}
+
+	mutex_lock(&controllers_lock);
+
+	list_for_each_entry(mc, &controllers, list) {
+		if (mc->dev && mc->dev->of_node == args.np) {
+			__module_get(mc->dev->driver->owner);
+			kref_get(&mc->ref);
+			goto unlock;
+		}
+	}
+
+	mc = ERR_PTR(-EPROBE_DEFER);
+
+unlock:
+	mutex_unlock(&controllers_lock);
+	of_node_put(args.np);
+	return mc;
+}
+
+/**
+ * memory_controller_get() - obtain a reference to a memory controller
+ * @dev: consumer device
+ * @con_id: consumer name
+ *
+ * Returns: A pointer to the requested memory controller or an ERR_PTR()-
+ * encoded error code on failure.
+ */
+struct memory_controller *
+memory_controller_get(struct device *dev, const char *con_id)
+{
+	if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
+		return of_memory_controller_get(dev, dev->of_node, con_id);
+
+	return ERR_PTR(-ENODEV);
+}
+EXPORT_SYMBOL_GPL(memory_controller_get);
+
+/**
+ * memory_controller_get_optional() - obtain a reference to an optional
+ *                                    memory controller
+ * @dev: consumer device
+ * @con_id: consumer name
+ *
+ * Returns: A pointer to the requested memory controller, NULL if no memory
+ * controller for the consumer device/name pair exists, or an ERR_PTR()-
+ * encoded error code on failure.
+ */
+struct memory_controller *
+memory_controller_get_optional(struct device *dev, const char *con_id)
+{
+	struct memory_controller *mc;
+
+	mc = memory_controller_get(dev, con_id);
+	if (IS_ERR(mc)) {
+		if (mc == ERR_PTR(-ENODEV))
+			return NULL;
+	}
+
+	return mc;
+}
+EXPORT_SYMBOL_GPL(memory_controller_get_optional);
+
+/**
+ * memory_controller_put() - release a reference to a memory controller
+ * @mc: memory controller
+ */
+void memory_controller_put(struct memory_controller *mc)
+{
+	if (mc) {
+		kref_put(&mc->ref, memory_controller_release);
+		module_put(mc->dev->driver->owner);
+	}
+}
+EXPORT_SYMBOL_GPL(memory_controller_put);
+
+static void devm_memory_controller_release(struct device *dev, void *res)
+{
+	memory_controller_put(*(struct memory_controller **)res);
+}
+
+/**
+ * devm_memory_controller_get() - obtain a reference to a memory controller
+ * @dev: consumer device
+ * @con_id: consumer name
+ *
+ * This is a device-managed variant of memory_controller_get(). The memory
+ * controller reference obtained with this function is automatically released
+ * when the device is unbound from its driver.
+ *
+ * Returns: A pointer to the requested memory controller or an ERR_PTR()-
+ * encoded error code on failure.
+ */
+struct memory_controller *devm_memory_controller_get(struct device *dev,
+						     const char *con_id)
+{
+	struct memory_controller **ptr, *mc;
+
+	ptr = devres_alloc(devm_memory_controller_release, sizeof(*ptr),
+			   GFP_KERNEL);
+	if (!ptr)
+		return ERR_PTR(-ENOMEM);
+
+	mc = memory_controller_get(dev, con_id);
+	if (!IS_ERR(mc)) {
+		*ptr = mc;
+		devres_add(dev, ptr);
+	} else {
+		devres_free(ptr);
+	}
+
+	return mc;
+}
+EXPORT_SYMBOL_GPL(devm_memory_controller_get);
+
+/**
+ * memory_controller_get_optional() - obtain a reference to an optional
+ *                                    memory controller
+ * @dev: consumer device
+ * @con_id: consumer name
+ *
+ * This is a device-managed variant of memory_controller_get_optional(). The
+ * memory controller reference obtained with this function is automatically
+ * released when the device is unbound from its driver.
+ *
+ * Returns: A pointer to the requested memory controller, NULL if no memory
+ * controller for the consumer device/name pair exists, or an ERR_PTR()-
+ * encoded error code on failure.
+ */
+struct memory_controller *
+devm_memory_controller_get_optional(struct device *dev, const char *con_id)
+{
+	struct memory_controller **ptr, *mc;
+
+	ptr = devres_alloc(devm_memory_controller_release, sizeof(*ptr),
+			   GFP_KERNEL);
+	if (!ptr)
+		return ERR_PTR(-ENOMEM);
+
+	mc = memory_controller_get_optional(dev, con_id);
+	if (!IS_ERR(mc)) {
+		*ptr = mc;
+		devres_add(dev, ptr);
+	} else {
+		devres_free(ptr);
+	}
+
+	return mc;
+}
+EXPORT_SYMBOL_GPL(devm_memory_controller_get_optional);
+
+static int devm_memory_controller_match(struct device *dev, void *res, void *data)
+{
+	struct memory_controller **mc = res;
+
+	if (WARN_ON(!mc || !*mc))
+		return 0;
+
+	return *mc == data;
+}
+
+/**
+ * devm_memory_controller_put() - release a reference to a memory controller
+ * @mc: memory controller
+ *
+ * This is a device-managed variant of memory_controller_put(). Typically it
+ * should never be necessary to call this function, since the device-managed
+ * code should take care of releasing the reference at the right time.
+ */
+void devm_memory_controller_put(struct device *dev,
+				struct memory_controller *mc)
+{
+	WARN_ON(devres_release(dev, devm_memory_controller_release,
+			       devm_memory_controller_match, mc));
+}
+EXPORT_SYMBOL_GPL(devm_memory_controller_put);
diff --git a/include/linux/memory-controller.h b/include/linux/memory-controller.h
new file mode 100644
index 000000000000..54490cb5e625
--- /dev/null
+++ b/include/linux/memory-controller.h
@@ -0,0 +1,34 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019-2020 NVIDIA Corporation.
+ */
+
+#ifndef _LINUX_MEMORY_CONTROLLER_H
+#define _LINUX_MEMORY_CONTROLLER_H
+
+#include <linux/device.h>
+#include <linux/list.h>
+
+struct memory_controller {
+	struct device *dev;
+	struct kref ref;
+	struct list_head list;
+};
+
+int memory_controller_register(struct memory_controller *mc);
+void memory_controller_unregister(struct memory_controller *mc);
+
+struct memory_controller *memory_controller_get(struct device *dev,
+						const char *con_id);
+struct memory_controller *memory_controller_get_optional(struct device *dev,
+							 const char *con_id);
+void memory_controller_put(struct memory_controller *mc);
+
+struct memory_controller *devm_memory_controller_get(struct device *dev,
+						     const char *con_id);
+struct memory_controller *
+devm_memory_controller_get_optional(struct device *dev, const char *con_id);
+void devm_memory_controller_put(struct device *dev,
+				struct memory_controller *mc);
+
+#endif
-- 
2.24.1

Re: [PATCH v4 3/5] memory: Introduce memory controller mini-framework

[Robin Murphy]

On 13/02/2020 4:39 pm, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> This new framework is currently nothing more than a registry of memory
> controllers, with the goal being to order device probing. One use-case
> where this is useful, for example, is a memory controller device which
> needs to program some registers before the system MMU can be enabled.
> Associating the memory controller with the SMMU allows the SMMU driver
> to defer the probe until the memory controller has been registered.

I'm doubtful of how generic an argument that really is - does anyone 
other than Tegra actually do this? (Most things I know of with 
programmable Stream IDs at least have the good grace to configure them 
in the bootloader or the devices' own drivers)

If the underlying aim is just "make SMMUs on Tegras wait for an extra 
thing", I'd suggest simply wiring up the existing tegra_mc APIs in your 
arm-smmu-nvidia.c hooks. (hmm, what did happen to those patches?)

Robin.

> One such example is Tegra186 where the memory controller contains some
> registers that are used to program stream IDs for the various memory
> clients (display, USB, PCI, ...) in the system. Programming these SIDs
> is required for the memory clients to emit the proper SIDs as part of
> their memory requests. The memory controller driver therefore needs to
> be programmed prior to the SMMU driver. To achieve that, the memory
> controller will be referenced via phandle from the SMMU device tree
> node, the SMMU driver can then use the memory controller framework to
> find it and defer probe until it has been registered.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v3:
> - add device-managed variants of the consumer APIs
> - add kerneldoc
> 
> Changes in v2:
> - fix double unlock (Dan Carpenter, kbuild test robot)
> - add helper to get optional memory controllers
> - acquire and release module reference
> 
>   drivers/memory/Makefile           |   1 +
>   drivers/memory/core.c             | 248 ++++++++++++++++++++++++++++++
>   include/linux/memory-controller.h |  34 ++++
>   3 files changed, 283 insertions(+)
>   create mode 100644 drivers/memory/core.c
>   create mode 100644 include/linux/memory-controller.h
> 
> diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
> index 27b493435e61..d16e7dca8ef9 100644
> --- a/drivers/memory/Makefile
> +++ b/drivers/memory/Makefile
> @@ -3,6 +3,7 @@
>   # Makefile for memory devices
>   #
>   
> +obj-y				+= core.o
>   obj-$(CONFIG_DDR)		+= jedec_ddr_data.o
>   ifeq ($(CONFIG_DDR),y)
>   obj-$(CONFIG_OF)		+= of_memory.o
> diff --git a/drivers/memory/core.c b/drivers/memory/core.c
> new file mode 100644
> index 000000000000..b2fbd2e808de
> --- /dev/null
> +++ b/drivers/memory/core.c
> @@ -0,0 +1,248 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019-2020 NVIDIA Corporation.
> + */
> +
> +#include <linux/memory-controller.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +
> +static DEFINE_MUTEX(controllers_lock);
> +static LIST_HEAD(controllers);
> +
> +static void memory_controller_release(struct kref *ref)
> +{
> +	struct memory_controller *mc = container_of(ref, struct memory_controller, ref);
> +
> +	WARN_ON(!list_empty(&mc->list));
> +}
> +
> +/**
> + * memory_controller_register() - register a memory controller
> + * @mc: memory controller
> + */
> +int memory_controller_register(struct memory_controller *mc)
> +{
> +	kref_init(&mc->ref);
> +
> +	mutex_lock(&controllers_lock);
> +	list_add_tail(&mc->list, &controllers);
> +	mutex_unlock(&controllers_lock);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(memory_controller_register);
> +
> +/**
> + * memory_controller_unregister() - unregister a memory controller
> + * @mc: memory controller
> + */
> +void memory_controller_unregister(struct memory_controller *mc)
> +{
> +	mutex_lock(&controllers_lock);
> +	list_del_init(&mc->list);
> +	mutex_unlock(&controllers_lock);
> +
> +	kref_put(&mc->ref, memory_controller_release);
> +}
> +EXPORT_SYMBOL_GPL(memory_controller_unregister);
> +
> +static struct memory_controller *
> +of_memory_controller_get(struct device *dev, struct device_node *np,
> +			 const char *con_id)
> +{
> +	const char *cells = "#memory-controller-cells";
> +	const char *names = "memory-controller-names";
> +	const char *prop = "memory-controllers";
> +	struct memory_controller *mc;
> +	struct of_phandle_args args;
> +	int index = 0, err;
> +
> +	if (con_id) {
> +		index = of_property_match_string(np, names, con_id);
> +		if (index < 0)
> +			return ERR_PTR(index);
> +	}
> +
> +	err = of_parse_phandle_with_args(np, prop, cells, index, &args);
> +	if (err) {
> +		if (err == -ENOENT)
> +			err = -ENODEV;
> +
> +		return ERR_PTR(err);
> +	}
> +
> +	mutex_lock(&controllers_lock);
> +
> +	list_for_each_entry(mc, &controllers, list) {
> +		if (mc->dev && mc->dev->of_node == args.np) {
> +			__module_get(mc->dev->driver->owner);
> +			kref_get(&mc->ref);
> +			goto unlock;
> +		}
> +	}
> +
> +	mc = ERR_PTR(-EPROBE_DEFER);
> +
> +unlock:
> +	mutex_unlock(&controllers_lock);
> +	of_node_put(args.np);
> +	return mc;
> +}
> +
> +/**
> + * memory_controller_get() - obtain a reference to a memory controller
> + * @dev: consumer device
> + * @con_id: consumer name
> + *
> + * Returns: A pointer to the requested memory controller or an ERR_PTR()-
> + * encoded error code on failure.
> + */
> +struct memory_controller *
> +memory_controller_get(struct device *dev, const char *con_id)
> +{
> +	if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
> +		return of_memory_controller_get(dev, dev->of_node, con_id);
> +
> +	return ERR_PTR(-ENODEV);
> +}
> +EXPORT_SYMBOL_GPL(memory_controller_get);
> +
> +/**
> + * memory_controller_get_optional() - obtain a reference to an optional
> + *                                    memory controller
> + * @dev: consumer device
> + * @con_id: consumer name
> + *
> + * Returns: A pointer to the requested memory controller, NULL if no memory
> + * controller for the consumer device/name pair exists, or an ERR_PTR()-
> + * encoded error code on failure.
> + */
> +struct memory_controller *
> +memory_controller_get_optional(struct device *dev, const char *con_id)
> +{
> +	struct memory_controller *mc;
> +
> +	mc = memory_controller_get(dev, con_id);
> +	if (IS_ERR(mc)) {
> +		if (mc == ERR_PTR(-ENODEV))
> +			return NULL;
> +	}
> +
> +	return mc;
> +}
> +EXPORT_SYMBOL_GPL(memory_controller_get_optional);
> +
> +/**
> + * memory_controller_put() - release a reference to a memory controller
> + * @mc: memory controller
> + */
> +void memory_controller_put(struct memory_controller *mc)
> +{
> +	if (mc) {
> +		kref_put(&mc->ref, memory_controller_release);
> +		module_put(mc->dev->driver->owner);
> +	}
> +}
> +EXPORT_SYMBOL_GPL(memory_controller_put);
> +
> +static void devm_memory_controller_release(struct device *dev, void *res)
> +{
> +	memory_controller_put(*(struct memory_controller **)res);
> +}
> +
> +/**
> + * devm_memory_controller_get() - obtain a reference to a memory controller
> + * @dev: consumer device
> + * @con_id: consumer name
> + *
> + * This is a device-managed variant of memory_controller_get(). The memory
> + * controller reference obtained with this function is automatically released
> + * when the device is unbound from its driver.
> + *
> + * Returns: A pointer to the requested memory controller or an ERR_PTR()-
> + * encoded error code on failure.
> + */
> +struct memory_controller *devm_memory_controller_get(struct device *dev,
> +						     const char *con_id)
> +{
> +	struct memory_controller **ptr, *mc;
> +
> +	ptr = devres_alloc(devm_memory_controller_release, sizeof(*ptr),
> +			   GFP_KERNEL);
> +	if (!ptr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	mc = memory_controller_get(dev, con_id);
> +	if (!IS_ERR(mc)) {
> +		*ptr = mc;
> +		devres_add(dev, ptr);
> +	} else {
> +		devres_free(ptr);
> +	}
> +
> +	return mc;
> +}
> +EXPORT_SYMBOL_GPL(devm_memory_controller_get);
> +
> +/**
> + * memory_controller_get_optional() - obtain a reference to an optional
> + *                                    memory controller
> + * @dev: consumer device
> + * @con_id: consumer name
> + *
> + * This is a device-managed variant of memory_controller_get_optional(). The
> + * memory controller reference obtained with this function is automatically
> + * released when the device is unbound from its driver.
> + *
> + * Returns: A pointer to the requested memory controller, NULL if no memory
> + * controller for the consumer device/name pair exists, or an ERR_PTR()-
> + * encoded error code on failure.
> + */
> +struct memory_controller *
> +devm_memory_controller_get_optional(struct device *dev, const char *con_id)
> +{
> +	struct memory_controller **ptr, *mc;
> +
> +	ptr = devres_alloc(devm_memory_controller_release, sizeof(*ptr),
> +			   GFP_KERNEL);
> +	if (!ptr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	mc = memory_controller_get_optional(dev, con_id);
> +	if (!IS_ERR(mc)) {
> +		*ptr = mc;
> +		devres_add(dev, ptr);
> +	} else {
> +		devres_free(ptr);
> +	}
> +
> +	return mc;
> +}
> +EXPORT_SYMBOL_GPL(devm_memory_controller_get_optional);
> +
> +static int devm_memory_controller_match(struct device *dev, void *res, void *data)
> +{
> +	struct memory_controller **mc = res;
> +
> +	if (WARN_ON(!mc || !*mc))
> +		return 0;
> +
> +	return *mc == data;
> +}
> +
> +/**
> + * devm_memory_controller_put() - release a reference to a memory controller
> + * @mc: memory controller
> + *
> + * This is a device-managed variant of memory_controller_put(). Typically it
> + * should never be necessary to call this function, since the device-managed
> + * code should take care of releasing the reference at the right time.
> + */
> +void devm_memory_controller_put(struct device *dev,
> +				struct memory_controller *mc)
> +{
> +	WARN_ON(devres_release(dev, devm_memory_controller_release,
> +			       devm_memory_controller_match, mc));
> +}
> +EXPORT_SYMBOL_GPL(devm_memory_controller_put);
> diff --git a/include/linux/memory-controller.h b/include/linux/memory-controller.h
> new file mode 100644
> index 000000000000..54490cb5e625
> --- /dev/null
> +++ b/include/linux/memory-controller.h
> @@ -0,0 +1,34 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019-2020 NVIDIA Corporation.
> + */
> +
> +#ifndef _LINUX_MEMORY_CONTROLLER_H
> +#define _LINUX_MEMORY_CONTROLLER_H
> +
> +#include <linux/device.h>
> +#include <linux/list.h>
> +
> +struct memory_controller {
> +	struct device *dev;
> +	struct kref ref;
> +	struct list_head list;
> +};
> +
> +int memory_controller_register(struct memory_controller *mc);
> +void memory_controller_unregister(struct memory_controller *mc);
> +
> +struct memory_controller *memory_controller_get(struct device *dev,
> +						const char *con_id);
> +struct memory_controller *memory_controller_get_optional(struct device *dev,
> +							 const char *con_id);
> +void memory_controller_put(struct memory_controller *mc);
> +
> +struct memory_controller *devm_memory_controller_get(struct device *dev,
> +						     const char *con_id);
> +struct memory_controller *
> +devm_memory_controller_get_optional(struct device *dev, const char *con_id);
> +void devm_memory_controller_put(struct device *dev,
> +				struct memory_controller *mc);
> +
> +#endif
> 

Re: [PATCH v4 3/5] memory: Introduce memory controller mini-framework

[Thierry Reding]

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

On Thu, Feb 13, 2020 at 05:03:10PM +0000, Robin Murphy wrote:
> On 13/02/2020 4:39 pm, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > This new framework is currently nothing more than a registry of memory
> > controllers, with the goal being to order device probing. One use-case
> > where this is useful, for example, is a memory controller device which
> > needs to program some registers before the system MMU can be enabled.
> > Associating the memory controller with the SMMU allows the SMMU driver
> > to defer the probe until the memory controller has been registered.
> 
> I'm doubtful of how generic an argument that really is - does anyone other
> than Tegra actually do this? (Most things I know of with programmable Stream
> IDs at least have the good grace to configure them in the bootloader or the
> devices' own drivers)

I'm not aware of any of the bootloaders doing anything with the SMMU, so
adding only the stream ID programming seems a little useless. Since it's
only at the kernel level that the SMMU will end up being used, it seems
natural to define the stream ID mapping there as well.

With regards to the devices' own drivers, they get probed way too late
for this to take any effect. If the DMA API is backed by an IOMMU, the
stream ID mappings will be required long before drivers actually take
control of their devices.

> If the underlying aim is just "make SMMUs on Tegras wait for an extra
> thing", I'd suggest simply wiring up the existing tegra_mc APIs in your
> arm-smmu-nvidia.c hooks. (hmm, what did happen to those patches?)

Passing around global symbols seems like a bit of a hack, whereas
encoding this in device tree actually gives us a way of properly
describing this relationship.

That said, I could look at moving the memory controller lookup code into
the Tegra-specific ARM SMMU implementation if it's not something that's
more broadly useful.

The NVIDIA implementation is currently blocked on two things. On one
hand we can't enable the SMMU before we have this series in place to
make sure that it starts up with the correct stream ID mapping. The
other blocker currently is that memory clients can access 40 bits of
addresses, but bit 39 has special meaning, so there's a bit more glue
that we need in device tree (via the DMA parent) to properly describe
the DMA ranges for these devices. Otherwise the IOMMU will hand out
IOVAs with bit 39 set (DMA API allocates from the top) and that causes
memory accesses to be jumbled in undesirable ways.

If I move the memory lookup code into the NVIDIA ARM SMMU implementation
it would probably easiest to integrate all of it into a single series.

Thierry

> > One such example is Tegra186 where the memory controller contains some
> > registers that are used to program stream IDs for the various memory
> > clients (display, USB, PCI, ...) in the system. Programming these SIDs
> > is required for the memory clients to emit the proper SIDs as part of
> > their memory requests. The memory controller driver therefore needs to
> > be programmed prior to the SMMU driver. To achieve that, the memory
> > controller will be referenced via phandle from the SMMU device tree
> > node, the SMMU driver can then use the memory controller framework to
> > find it and defer probe until it has been registered.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > Changes in v3:
> > - add device-managed variants of the consumer APIs
> > - add kerneldoc
> > 
> > Changes in v2:
> > - fix double unlock (Dan Carpenter, kbuild test robot)
> > - add helper to get optional memory controllers
> > - acquire and release module reference
> > 
> >   drivers/memory/Makefile           |   1 +
> >   drivers/memory/core.c             | 248 ++++++++++++++++++++++++++++++
> >   include/linux/memory-controller.h |  34 ++++
> >   3 files changed, 283 insertions(+)
> >   create mode 100644 drivers/memory/core.c
> >   create mode 100644 include/linux/memory-controller.h
> > 
> > diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
> > index 27b493435e61..d16e7dca8ef9 100644
> > --- a/drivers/memory/Makefile
> > +++ b/drivers/memory/Makefile
> > @@ -3,6 +3,7 @@
> >   # Makefile for memory devices
> >   #
> > +obj-y				+= core.o
> >   obj-$(CONFIG_DDR)		+= jedec_ddr_data.o
> >   ifeq ($(CONFIG_DDR),y)
> >   obj-$(CONFIG_OF)		+= of_memory.o
> > diff --git a/drivers/memory/core.c b/drivers/memory/core.c
> > new file mode 100644
> > index 000000000000..b2fbd2e808de
> > --- /dev/null
> > +++ b/drivers/memory/core.c
> > @@ -0,0 +1,248 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2019-2020 NVIDIA Corporation.
> > + */
> > +
> > +#include <linux/memory-controller.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +
> > +static DEFINE_MUTEX(controllers_lock);
> > +static LIST_HEAD(controllers);
> > +
> > +static void memory_controller_release(struct kref *ref)
> > +{
> > +	struct memory_controller *mc = container_of(ref, struct memory_controller, ref);
> > +
> > +	WARN_ON(!list_empty(&mc->list));
> > +}
> > +
> > +/**
> > + * memory_controller_register() - register a memory controller
> > + * @mc: memory controller
> > + */
> > +int memory_controller_register(struct memory_controller *mc)
> > +{
> > +	kref_init(&mc->ref);
> > +
> > +	mutex_lock(&controllers_lock);
> > +	list_add_tail(&mc->list, &controllers);
> > +	mutex_unlock(&controllers_lock);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(memory_controller_register);
> > +
> > +/**
> > + * memory_controller_unregister() - unregister a memory controller
> > + * @mc: memory controller
> > + */
> > +void memory_controller_unregister(struct memory_controller *mc)
> > +{
> > +	mutex_lock(&controllers_lock);
> > +	list_del_init(&mc->list);
> > +	mutex_unlock(&controllers_lock);
> > +
> > +	kref_put(&mc->ref, memory_controller_release);
> > +}
> > +EXPORT_SYMBOL_GPL(memory_controller_unregister);
> > +
> > +static struct memory_controller *
> > +of_memory_controller_get(struct device *dev, struct device_node *np,
> > +			 const char *con_id)
> > +{
> > +	const char *cells = "#memory-controller-cells";
> > +	const char *names = "memory-controller-names";
> > +	const char *prop = "memory-controllers";
> > +	struct memory_controller *mc;
> > +	struct of_phandle_args args;
> > +	int index = 0, err;
> > +
> > +	if (con_id) {
> > +		index = of_property_match_string(np, names, con_id);
> > +		if (index < 0)
> > +			return ERR_PTR(index);
> > +	}
> > +
> > +	err = of_parse_phandle_with_args(np, prop, cells, index, &args);
> > +	if (err) {
> > +		if (err == -ENOENT)
> > +			err = -ENODEV;
> > +
> > +		return ERR_PTR(err);
> > +	}
> > +
> > +	mutex_lock(&controllers_lock);
> > +
> > +	list_for_each_entry(mc, &controllers, list) {
> > +		if (mc->dev && mc->dev->of_node == args.np) {
> > +			__module_get(mc->dev->driver->owner);
> > +			kref_get(&mc->ref);
> > +			goto unlock;
> > +		}
> > +	}
> > +
> > +	mc = ERR_PTR(-EPROBE_DEFER);
> > +
> > +unlock:
> > +	mutex_unlock(&controllers_lock);
> > +	of_node_put(args.np);
> > +	return mc;
> > +}
> > +
> > +/**
> > + * memory_controller_get() - obtain a reference to a memory controller
> > + * @dev: consumer device
> > + * @con_id: consumer name
> > + *
> > + * Returns: A pointer to the requested memory controller or an ERR_PTR()-
> > + * encoded error code on failure.
> > + */
> > +struct memory_controller *
> > +memory_controller_get(struct device *dev, const char *con_id)
> > +{
> > +	if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
> > +		return of_memory_controller_get(dev, dev->of_node, con_id);
> > +
> > +	return ERR_PTR(-ENODEV);
> > +}
> > +EXPORT_SYMBOL_GPL(memory_controller_get);
> > +
> > +/**
> > + * memory_controller_get_optional() - obtain a reference to an optional
> > + *                                    memory controller
> > + * @dev: consumer device
> > + * @con_id: consumer name
> > + *
> > + * Returns: A pointer to the requested memory controller, NULL if no memory
> > + * controller for the consumer device/name pair exists, or an ERR_PTR()-
> > + * encoded error code on failure.
> > + */
> > +struct memory_controller *
> > +memory_controller_get_optional(struct device *dev, const char *con_id)
> > +{
> > +	struct memory_controller *mc;
> > +
> > +	mc = memory_controller_get(dev, con_id);
> > +	if (IS_ERR(mc)) {
> > +		if (mc == ERR_PTR(-ENODEV))
> > +			return NULL;
> > +	}
> > +
> > +	return mc;
> > +}
> > +EXPORT_SYMBOL_GPL(memory_controller_get_optional);
> > +
> > +/**
> > + * memory_controller_put() - release a reference to a memory controller
> > + * @mc: memory controller
> > + */
> > +void memory_controller_put(struct memory_controller *mc)
> > +{
> > +	if (mc) {
> > +		kref_put(&mc->ref, memory_controller_release);
> > +		module_put(mc->dev->driver->owner);
> > +	}
> > +}
> > +EXPORT_SYMBOL_GPL(memory_controller_put);
> > +
> > +static void devm_memory_controller_release(struct device *dev, void *res)
> > +{
> > +	memory_controller_put(*(struct memory_controller **)res);
> > +}
> > +
> > +/**
> > + * devm_memory_controller_get() - obtain a reference to a memory controller
> > + * @dev: consumer device
> > + * @con_id: consumer name
> > + *
> > + * This is a device-managed variant of memory_controller_get(). The memory
> > + * controller reference obtained with this function is automatically released
> > + * when the device is unbound from its driver.
> > + *
> > + * Returns: A pointer to the requested memory controller or an ERR_PTR()-
> > + * encoded error code on failure.
> > + */
> > +struct memory_controller *devm_memory_controller_get(struct device *dev,
> > +						     const char *con_id)
> > +{
> > +	struct memory_controller **ptr, *mc;
> > +
> > +	ptr = devres_alloc(devm_memory_controller_release, sizeof(*ptr),
> > +			   GFP_KERNEL);
> > +	if (!ptr)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	mc = memory_controller_get(dev, con_id);
> > +	if (!IS_ERR(mc)) {
> > +		*ptr = mc;
> > +		devres_add(dev, ptr);
> > +	} else {
> > +		devres_free(ptr);
> > +	}
> > +
> > +	return mc;
> > +}
> > +EXPORT_SYMBOL_GPL(devm_memory_controller_get);
> > +
> > +/**
> > + * memory_controller_get_optional() - obtain a reference to an optional
> > + *                                    memory controller
> > + * @dev: consumer device
> > + * @con_id: consumer name
> > + *
> > + * This is a device-managed variant of memory_controller_get_optional(). The
> > + * memory controller reference obtained with this function is automatically
> > + * released when the device is unbound from its driver.
> > + *
> > + * Returns: A pointer to the requested memory controller, NULL if no memory
> > + * controller for the consumer device/name pair exists, or an ERR_PTR()-
> > + * encoded error code on failure.
> > + */
> > +struct memory_controller *
> > +devm_memory_controller_get_optional(struct device *dev, const char *con_id)
> > +{
> > +	struct memory_controller **ptr, *mc;
> > +
> > +	ptr = devres_alloc(devm_memory_controller_release, sizeof(*ptr),
> > +			   GFP_KERNEL);
> > +	if (!ptr)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	mc = memory_controller_get_optional(dev, con_id);
> > +	if (!IS_ERR(mc)) {
> > +		*ptr = mc;
> > +		devres_add(dev, ptr);
> > +	} else {
> > +		devres_free(ptr);
> > +	}
> > +
> > +	return mc;
> > +}
> > +EXPORT_SYMBOL_GPL(devm_memory_controller_get_optional);
> > +
> > +static int devm_memory_controller_match(struct device *dev, void *res, void *data)
> > +{
> > +	struct memory_controller **mc = res;
> > +
> > +	if (WARN_ON(!mc || !*mc))
> > +		return 0;
> > +
> > +	return *mc == data;
> > +}
> > +
> > +/**
> > + * devm_memory_controller_put() - release a reference to a memory controller
> > + * @mc: memory controller
> > + *
> > + * This is a device-managed variant of memory_controller_put(). Typically it
> > + * should never be necessary to call this function, since the device-managed
> > + * code should take care of releasing the reference at the right time.
> > + */
> > +void devm_memory_controller_put(struct device *dev,
> > +				struct memory_controller *mc)
> > +{
> > +	WARN_ON(devres_release(dev, devm_memory_controller_release,
> > +			       devm_memory_controller_match, mc));
> > +}
> > +EXPORT_SYMBOL_GPL(devm_memory_controller_put);
> > diff --git a/include/linux/memory-controller.h b/include/linux/memory-controller.h
> > new file mode 100644
> > index 000000000000..54490cb5e625
> > --- /dev/null
> > +++ b/include/linux/memory-controller.h
> > @@ -0,0 +1,34 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2019-2020 NVIDIA Corporation.
> > + */
> > +
> > +#ifndef _LINUX_MEMORY_CONTROLLER_H
> > +#define _LINUX_MEMORY_CONTROLLER_H
> > +
> > +#include <linux/device.h>
> > +#include <linux/list.h>
> > +
> > +struct memory_controller {
> > +	struct device *dev;
> > +	struct kref ref;
> > +	struct list_head list;
> > +};
> > +
> > +int memory_controller_register(struct memory_controller *mc);
> > +void memory_controller_unregister(struct memory_controller *mc);
> > +
> > +struct memory_controller *memory_controller_get(struct device *dev,
> > +						const char *con_id);
> > +struct memory_controller *memory_controller_get_optional(struct device *dev,
> > +							 const char *con_id);
> > +void memory_controller_put(struct memory_controller *mc);
> > +
> > +struct memory_controller *devm_memory_controller_get(struct device *dev,
> > +						     const char *con_id);
> > +struct memory_controller *
> > +devm_memory_controller_get_optional(struct device *dev, const char *con_id);
> > +void devm_memory_controller_put(struct device *dev,
> > +				struct memory_controller *mc);
> > +
> > +#endif
> > 

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

[PATCH v4 4/5] memory: tegra186: Register as memory controller

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Registering as memory controller allows other drivers to obtain a
reference to it. This is mostly useful as a way of ordering probe
between devices depending on one another.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/memory/tegra/tegra186.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
index 5d53f11ca7b6..8c43702340e8 100644
--- a/drivers/memory/tegra/tegra186.c
+++ b/drivers/memory/tegra/tegra186.c
@@ -4,6 +4,7 @@
  */
 
 #include <linux/io.h>
+#include <linux/memory-controller.h>
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
 #include <linux/of_device.h>
@@ -32,6 +33,7 @@ struct tegra186_mc_soc {
 };
 
 struct tegra186_mc {
+	struct memory_controller base;
 	struct device *dev;
 	void __iomem *regs;
 
@@ -1532,13 +1534,18 @@ static int tegra186_mc_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	mc->soc = of_device_get_match_data(&pdev->dev);
+	mc->dev = &pdev->dev;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	mc->regs = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(mc->regs))
 		return PTR_ERR(mc->regs);
 
-	mc->dev = &pdev->dev;
+	mc->base.dev = &pdev->dev;
+
+	err = memory_controller_register(&mc->base);
+	if (err < 0)
+		return err;
 
 	err = of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
 	if (err < 0)
-- 
2.24.1

[PATCH v4 5/5] iommu: arm-smmu: Get reference to memory controller

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Use the memory controller framework to obtain a reference to the memory
controller to which the SMMU will make memory requests. This allows the
two drivers to properly order their probes so that the memory controller
can be programmed first.

An example where this is required is Tegra186 where the stream IDs need
to be associated with memory clients before memory requests are emitted
with the correct stream ID.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/iommu/arm-smmu.c | 11 +++++++++++
 drivers/iommu/arm-smmu.h |  2 ++
 2 files changed, 13 insertions(+)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 16c4b87af42b..862ea55018e8 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -2109,6 +2109,17 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 	}
 	smmu->dev = dev;
 
+	smmu->mc = devm_memory_controller_get_optional(dev, NULL);
+	if (IS_ERR(smmu->mc)) {
+		err = PTR_ERR(smmu->mc);
+
+		if (err != -EPROBE_DEFER)
+			dev_err(dev, "failed to get memory controller: %d\n",
+				err);
+
+		return err;
+	}
+
 	if (dev->of_node)
 		err = arm_smmu_device_dt_probe(pdev, smmu);
 	else
diff --git a/drivers/iommu/arm-smmu.h b/drivers/iommu/arm-smmu.h
index 8d1cd54d82a6..d38bcd3ce447 100644
--- a/drivers/iommu/arm-smmu.h
+++ b/drivers/iommu/arm-smmu.h
@@ -18,6 +18,7 @@
 #include <linux/io-64-nonatomic-hi-lo.h>
 #include <linux/io-pgtable.h>
 #include <linux/iommu.h>
+#include <linux/memory-controller.h>
 #include <linux/mutex.h>
 #include <linux/spinlock.h>
 #include <linux/types.h>
@@ -253,6 +254,7 @@ enum arm_smmu_implementation {
 
 struct arm_smmu_device {
 	struct device			*dev;
+	struct memory_controller	*mc;
 
 	void __iomem			*base;
 	unsigned int			numpage;
-- 
2.24.1

Re: [PATCH v4 0/5] memory: Introduce memory controller mini-framework

[Robin Murphy]

[+ Maxime]

On 13/02/2020 4:39 pm, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Hi,
> 
> this set of patches adds a new binding that allows device tree nodes to
> explicitly define the DMA parent for a given device. This supplements
> the existing interconnect bindings and is useful to disambiguate in the
> case where a device has multiple paths to system memory. Beyond that it
> can also be useful when there aren't any actual interconnect paths that
> can be controlled, so in simple cases this can serve as a simpler
> variant of interconnect paths.

Isn't that still squarely the intent of the "dma-mem" binding, though? 
i.e. it's not meant to be a 'real' interconnect provider, but a very 
simple way to encode DMA parentage piggybacked onto a more general 
binding (with the *option* of being a full-blown interconnect if it 
wants to, but certainly no expectation).

Robin.

> One other case where this is useful is to describe the relationship
> between devices such as the memory controller and an IOMMU, for example.
> On Tegra186 and later, the memory controller is programmed with a set of
> stream IDs that are to be associated with each memory client. This
> programming needs to happen before translations through the IOMMU start,
> otherwise the used stream IDs may deviate from the expected values. The
> memory-controllers property is used in this case to ensure that the
> memory controller driver has been probed (and hence has programmed the
> stream ID mappings) before the IOMMU becomes available.
> 
> Patch 1 introduces the memory controller bindings, both from the
> perspective of the provider and the consumer. Patch 2 makes use of a
> memory-controllers property to determine the DMA parent for the purpose
> of setting up DMA masks (based on the dma-ranges property of the DMA
> parent). Patch 3 introduces a minimalistic framework that is used to
> register memory controllers with along with a set of helpers to look up
> the memory controller from device tree.
> 
> An example of how to register a memory controller is shown in patch 4
> for Tegra186 (and later) and finally the ARM SMMU driver is extended to
> become a consumer of an (optional) memory controller. As described
> above, the goal is to defer probe as long as the memory controller has
> not yet programmed the stream ID mappings.
> 
> Thierry
> 
> Thierry Reding (5):
>    dt-bindings: Add memory controller bindings
>    of: Use memory-controllers property for DMA parent
>    memory: Introduce memory controller mini-framework
>    memory: tegra186: Register as memory controller
>    iommu: arm-smmu: Get reference to memory controller
> 
>   .../bindings/memory-controllers/consumer.yaml |  14 +
>   .../memory-controllers/memory-controller.yaml |  32 +++
>   drivers/iommu/arm-smmu.c                      |  11 +
>   drivers/iommu/arm-smmu.h                      |   2 +
>   drivers/memory/Makefile                       |   1 +
>   drivers/memory/core.c                         | 248 ++++++++++++++++++
>   drivers/memory/tegra/tegra186.c               |   9 +-
>   drivers/of/address.c                          |  25 +-
>   include/linux/memory-controller.h             |  34 +++
>   9 files changed, 366 insertions(+), 10 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/memory-controllers/consumer.yaml
>   create mode 100644 Documentation/devicetree/bindings/memory-controllers/memory-controller.yaml
>   create mode 100644 drivers/memory/core.c
>   create mode 100644 include/linux/memory-controller.h
> 

Re: [PATCH v4 0/5] memory: Introduce memory controller mini-framework

[Thierry Reding]

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

On Thu, Feb 13, 2020 at 05:23:23PM +0000, Robin Murphy wrote:
> [+ Maxime]
> 
> On 13/02/2020 4:39 pm, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Hi,
> > 
> > this set of patches adds a new binding that allows device tree nodes to
> > explicitly define the DMA parent for a given device. This supplements
> > the existing interconnect bindings and is useful to disambiguate in the
> > case where a device has multiple paths to system memory. Beyond that it
> > can also be useful when there aren't any actual interconnect paths that
> > can be controlled, so in simple cases this can serve as a simpler
> > variant of interconnect paths.
> 
> Isn't that still squarely the intent of the "dma-mem" binding, though? i.e.
> it's not meant to be a 'real' interconnect provider, but a very simple way
> to encode DMA parentage piggybacked onto a more general binding (with the
> *option* of being a full-blown interconnect if it wants to, but certainly no
> expectation).

The way that this works on Tegra is that we want to describe multiple
interconnect paths. A typical device will have a read and a write memory
client, which can be separately "tuned". Both of these paths will target
system memory, so they would both technically be "dma-mem" paths. But
that would make it impossible to treat them separately elsewhere.

So we could choose any of them to be the "dma-mem" path, but then we
need to be very careful about defining which one that is, so that
drivers know how to look them up, which is also not really desirable.

One other things we could do is to duplicate one of the entries, so that
we'd have "read", "write" and "dma-mem" interconnect paths, with
"dma-mem" referencing the same path as "read" or "write". That doesn't
sound *too* bad, but it's still a bit of a hack. Having an explicit
description for this sounds much clearer and less error prone to me.

Thierry

> > One other case where this is useful is to describe the relationship
> > between devices such as the memory controller and an IOMMU, for example.
> > On Tegra186 and later, the memory controller is programmed with a set of
> > stream IDs that are to be associated with each memory client. This
> > programming needs to happen before translations through the IOMMU start,
> > otherwise the used stream IDs may deviate from the expected values. The
> > memory-controllers property is used in this case to ensure that the
> > memory controller driver has been probed (and hence has programmed the
> > stream ID mappings) before the IOMMU becomes available.
> > 
> > Patch 1 introduces the memory controller bindings, both from the
> > perspective of the provider and the consumer. Patch 2 makes use of a
> > memory-controllers property to determine the DMA parent for the purpose
> > of setting up DMA masks (based on the dma-ranges property of the DMA
> > parent). Patch 3 introduces a minimalistic framework that is used to
> > register memory controllers with along with a set of helpers to look up
> > the memory controller from device tree.
> > 
> > An example of how to register a memory controller is shown in patch 4
> > for Tegra186 (and later) and finally the ARM SMMU driver is extended to
> > become a consumer of an (optional) memory controller. As described
> > above, the goal is to defer probe as long as the memory controller has
> > not yet programmed the stream ID mappings.
> > 
> > Thierry
> > 
> > Thierry Reding (5):
> >    dt-bindings: Add memory controller bindings
> >    of: Use memory-controllers property for DMA parent
> >    memory: Introduce memory controller mini-framework
> >    memory: tegra186: Register as memory controller
> >    iommu: arm-smmu: Get reference to memory controller
> > 
> >   .../bindings/memory-controllers/consumer.yaml |  14 +
> >   .../memory-controllers/memory-controller.yaml |  32 +++
> >   drivers/iommu/arm-smmu.c                      |  11 +
> >   drivers/iommu/arm-smmu.h                      |   2 +
> >   drivers/memory/Makefile                       |   1 +
> >   drivers/memory/core.c                         | 248 ++++++++++++++++++
> >   drivers/memory/tegra/tegra186.c               |   9 +-
> >   drivers/of/address.c                          |  25 +-
> >   include/linux/memory-controller.h             |  34 +++
> >   9 files changed, 366 insertions(+), 10 deletions(-)
> >   create mode 100644 Documentation/devicetree/bindings/memory-controllers/consumer.yaml
> >   create mode 100644 Documentation/devicetree/bindings/memory-controllers/memory-controller.yaml
> >   create mode 100644 drivers/memory/core.c
> >   create mode 100644 include/linux/memory-controller.h
> > 

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

Re: [PATCH v4 0/5] memory: Introduce memory controller mini-framework

[Maxime Ripard]

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

On Thu, Feb 13, 2020 at 07:15:55PM +0100, Thierry Reding wrote:
> On Thu, Feb 13, 2020 at 05:23:23PM +0000, Robin Murphy wrote:
> > [+ Maxime]
> >
> > On 13/02/2020 4:39 pm, Thierry Reding wrote:
> > > From: Thierry Reding <treding@nvidia.com>
> > >
> > > Hi,
> > >
> > > this set of patches adds a new binding that allows device tree nodes to
> > > explicitly define the DMA parent for a given device. This supplements
> > > the existing interconnect bindings and is useful to disambiguate in the
> > > case where a device has multiple paths to system memory. Beyond that it
> > > can also be useful when there aren't any actual interconnect paths that
> > > can be controlled, so in simple cases this can serve as a simpler
> > > variant of interconnect paths.
> >
> > Isn't that still squarely the intent of the "dma-mem" binding, though? i.e.
> > it's not meant to be a 'real' interconnect provider, but a very simple way
> > to encode DMA parentage piggybacked onto a more general binding (with the
> > *option* of being a full-blown interconnect if it wants to, but certainly no
> > expectation).
>
> The way that this works on Tegra is that we want to describe multiple
> interconnect paths. A typical device will have a read and a write memory
> client, which can be separately "tuned". Both of these paths will target
> system memory, so they would both technically be "dma-mem" paths. But
> that would make it impossible to treat them separately elsewhere.
>
> So we could choose any of them to be the "dma-mem" path, but then we
> need to be very careful about defining which one that is, so that
> drivers know how to look them up, which is also not really desirable.
>
> One other things we could do is to duplicate one of the entries, so that
> we'd have "read", "write" and "dma-mem" interconnect paths, with
> "dma-mem" referencing the same path as "read" or "write". That doesn't
> sound *too* bad, but it's still a bit of a hack. Having an explicit
> description for this sounds much clearer and less error prone to me.

IIRC the dmaengine binding allows to do that, so it would make sense
to me to have the same thing allowed for interconnects.

Maxime

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