[PATCH 0/4] of: reserved-memory: Various improvements

[PATCH 0/4] of: reserved-memory: Various improvements

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Hi Rob, all,

this is a set of patches that I've been working on to allow me to use
reserved memory regions more flexibly. One of the use-cases that I have
is an external memory controller driver that gets passed one or two
tables from firmware containing a set of EMC frequencies and the
corresponding register values to program for these frequencies.

One of these tables is the "nominal" table and an optional second table
is "derated" and is used when the DRAM chips are overheating. I want to
be able to pass these tables as separate memory-region entries.

So what this small patchset does is make the reserved-memory code adapt
to this situation better. On one hand, while the DT bindings currently
support multiple regions per device tree node, it's slightly unintuitive
to specify them. The first patch adds a memory-region-names property
that allows the DT to specify a "consumer" name for these regions much
like we do for things like clocks, resets or the reg property. At the
same time, a new alias for memory-region, named memory-regions, is
introduced to make this more consistent with other bindings.

Patches 2 and 3 add support in the core OF reserved-memory code for
these binding changes, with a fallback to the memory-region property if
no memory-regions property exists.

Patch 4 implements support for releasing multiple regions assigned to a
single device rather than just the first.

Thierry

Thierry Reding (4):
  dt-bindings: reserved-memory: Introduce memory-region{s,-names}
  of: reserved-memory: Support memory-regions property
  of: reserved-memory: Support lookup of regions by name
  of: reserved-memory: Support multiple regions per device

 .../reserved-memory/reserved-memory.txt       | 12 +++--
 drivers/of/of_reserved_mem.c                  | 52 +++++++++++++------
 include/linux/of_reserved_mem.h               | 11 ++++
 3 files changed, 55 insertions(+), 20 deletions(-)

-- 
2.24.1

[PATCH 1/4] dt-bindings: reserved-memory: Introduce memory-region{s,-names}

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

In order to make the reserved-memory bindings more consistent with other
existing bindings, add a memory-region-names property that contains an
array of strings that name the entries of the memory-region property and
allows these regions to be looked up by name.

Also add the memory-regions property as an alias for the existing
memory-region property. This is again for consistency with other similar
bindings and makes it clearer that this property can take multiple
<phandle, specifier> pairs.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 .../bindings/reserved-memory/reserved-memory.txt     | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
index bac4afa3b197..823e619cfca3 100644
--- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
+++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
@@ -76,7 +76,11 @@ Device node references to reserved memory
 Regions in the /reserved-memory node may be referenced by other device
 nodes by adding a memory-region property to the device node.
 
-memory-region (optional) - phandle, specifier pairs to children of /reserved-memory
+memory-regions (optional) - phandle, specifier pairs to children of /reserved-memory
+memory-region-names (optional) - a list of names, one for each corresponding
+  entry in the memory-regions property
+
+memory-region (optional) - alias for memory-regions
 
 Example
 -------
@@ -120,17 +124,17 @@ one for multimedia processing (named multimedia-memory@77000000, 64MiB).
 	/* ... */
 
 	fb0: video@12300000 {
-		memory-region = <&display_reserved>;
+		memory-regions = <&display_reserved>;
 		/* ... */
 	};
 
 	scaler: scaler@12500000 {
-		memory-region = <&multimedia_reserved>;
+		memory-regions = <&multimedia_reserved>;
 		/* ... */
 	};
 
 	codec: codec@12600000 {
-		memory-region = <&multimedia_reserved>;
+		memory-regions = <&multimedia_reserved>;
 		/* ... */
 	};
 };
-- 
2.24.1

[PATCH 2/4] of: reserved-memory: Support memory-regions property

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Implement the memory-regions property as the preferred way to get at the
list of reserved memory regions referenced by a device. For backwards-
compatibility, fallback to the memory-region property.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/of/of_reserved_mem.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 1a84bc0d5fa8..62a35422c28d 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -302,9 +302,9 @@ static DEFINE_MUTEX(of_rmem_assigned_device_mutex);
  * @idx:	Index of selected region
  *
  * This function assigns respective DMA-mapping operations based on reserved
- * memory region specified by 'memory-region' property in @np node to the @dev
- * device. When driver needs to use more than one reserved memory region, it
- * should allocate child devices and initialize regions by name for each of
+ * memory region specified by 'memory-region(s)' property in @np node to the
+ * @dev device. When driver needs to use more than one reserved memory region,
+ * it should allocate child devices and initialize regions by name for each of
  * child device.
  *
  * Returns error code or zero on success.
@@ -320,7 +320,10 @@ int of_reserved_mem_device_init_by_idx(struct device *dev,
 	if (!np || !dev)
 		return -EINVAL;
 
-	target = of_parse_phandle(np, "memory-region", idx);
+	target = of_parse_phandle(np, "memory-regions", idx);
+	if (!target)
+		target = of_parse_phandle(np, "memory-region", idx);
+
 	if (!target)
 		return -ENODEV;
 
-- 
2.24.1

[PATCH 3/4] of: reserved-memory: Support lookup of regions by name

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Add support for looking up memory regions by name. This looks up the
given name in the newly introduced memory-region-names property and
returns the memory region at the corresponding index in the memory-
region(s) property.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/of/of_reserved_mem.c    | 19 +++++++++++++++++++
 include/linux/of_reserved_mem.h | 11 +++++++++++
 2 files changed, 30 insertions(+)

diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 62a35422c28d..dae70b147552 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -360,6 +360,25 @@ int of_reserved_mem_device_init_by_idx(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(of_reserved_mem_device_init_by_idx);
 
+/**
+ * of_reserved_mem_device_init_by_name() - assign named reserved memory region
+ *					   to given device
+ * @dev: pointer to the device to configure
+ * @np: pointer to the device node with 'memory-region(s)' property
+ * @name: name of the selected memory region
+ *
+ * Returns: 0 on success or a negative error-code on failure.
+ */
+int of_reserved_mem_device_init_by_name(struct device *dev,
+					struct device_node *np,
+					const char *name)
+{
+	int idx = of_property_match_string(np, "memory-region-names", name);
+
+	return of_reserved_mem_device_init_by_idx(dev, np, idx);
+}
+EXPORT_SYMBOL_GPL(of_reserved_mem_device_init_by_name);
+
 /**
  * of_reserved_mem_device_release() - release reserved memory device structures
  * @dev:	Pointer to the device to deconfigure
diff --git a/include/linux/of_reserved_mem.h b/include/linux/of_reserved_mem.h
index 60f541912ccf..a1b427ac291b 100644
--- a/include/linux/of_reserved_mem.h
+++ b/include/linux/of_reserved_mem.h
@@ -33,6 +33,9 @@ typedef int (*reservedmem_of_init_fn)(struct reserved_mem *rmem);
 
 int of_reserved_mem_device_init_by_idx(struct device *dev,
 				       struct device_node *np, int idx);
+int of_reserved_mem_device_init_by_name(struct device *dev,
+					struct device_node *np,
+					const char *name);
 void of_reserved_mem_device_release(struct device *dev);
 
 void fdt_init_reserved_mem(void);
@@ -45,6 +48,14 @@ static inline int of_reserved_mem_device_init_by_idx(struct device *dev,
 {
 	return -ENOSYS;
 }
+
+static inline int of_reserved_mem_device_init_by_name(struct device *dev,
+						      struct device_node *np,
+						      const char *name)
+{
+	return -ENOSYS;
+}
+
 static inline void of_reserved_mem_device_release(struct device *pdev) { }
 
 static inline void fdt_init_reserved_mem(void) { }
-- 
2.24.1

[PATCH 4/4] of: reserved-memory: Support multiple regions per device

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

While the lookup/initialization code already supports multiple memory
regions per device, the release code will only ever release the first
matching memory region.

Enhance the code to release all matching regions. Each attachment of
a region to a device is uniquely identifiable using a struct device
pointer and a pointer to the memory region's struct reserved_mem.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/of/of_reserved_mem.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index dae70b147552..3ad24df0f673 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -388,24 +388,22 @@ EXPORT_SYMBOL_GPL(of_reserved_mem_device_init_by_name);
  */
 void of_reserved_mem_device_release(struct device *dev)
 {
-	struct rmem_assigned_device *rd;
-	struct reserved_mem *rmem = NULL;
+	struct rmem_assigned_device *rd, *tmp;
+	LIST_HEAD(release_list);
 
 	mutex_lock(&of_rmem_assigned_device_mutex);
-	list_for_each_entry(rd, &of_rmem_assigned_device_list, list) {
-		if (rd->dev == dev) {
-			rmem = rd->rmem;
-			list_del(&rd->list);
-			kfree(rd);
-			break;
-		}
+	list_for_each_entry_safe(rd, tmp, &of_rmem_assigned_device_list, list) {
+		if (rd->dev == dev)
+			list_move_tail(&rd->list, &release_list);
 	}
 	mutex_unlock(&of_rmem_assigned_device_mutex);
 
-	if (!rmem || !rmem->ops || !rmem->ops->device_release)
-		return;
+	list_for_each_entry_safe(rd, tmp, &release_list, list) {
+		if (rd->rmem && rd->rmem->ops && rd->rmem->ops->device_release)
+			rd->rmem->ops->device_release(rd->rmem, dev);
 
-	rmem->ops->device_release(rmem, dev);
+		kfree(rd);
+	}
 }
 EXPORT_SYMBOL_GPL(of_reserved_mem_device_release);
 
-- 
2.24.1

Re: [PATCH 0/4] of: reserved-memory: Various improvements

[Rob Herring]

On Fri, Apr 3, 2020 at 12:56 PM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> From: Thierry Reding <treding@nvidia.com>
>
> Hi Rob, all,
>
> this is a set of patches that I've been working on to allow me to use
> reserved memory regions more flexibly. One of the use-cases that I have
> is an external memory controller driver that gets passed one or two
> tables from firmware containing a set of EMC frequencies and the
> corresponding register values to program for these frequencies.
>
> One of these tables is the "nominal" table and an optional second table
> is "derated" and is used when the DRAM chips are overheating. I want to
> be able to pass these tables as separate memory-region entries.
>
> So what this small patchset does is make the reserved-memory code adapt
> to this situation better. On one hand, while the DT bindings currently
> support multiple regions per device tree node, it's slightly unintuitive
> to specify them. The first patch adds a memory-region-names property
> that allows the DT to specify a "consumer" name for these regions much
> like we do for things like clocks, resets or the reg property. At the
> same time, a new alias for memory-region, named memory-regions, is
> introduced to make this more consistent with other bindings.

It's just not worth supporting both flavors (forever). I don't want to
repeat gpio vs. gpios. Let's just stick with 'memory-region' and allow
that to be more than one entry.

I'm not a fan of *-names, but fine.

Rob

Re: [PATCH 0/4] of: reserved-memory: Various improvements

[Thierry Reding]

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

On Sat, Apr 04, 2020 at 07:24:25PM -0600, Rob Herring wrote:
> On Fri, Apr 3, 2020 at 12:56 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > From: Thierry Reding <treding@nvidia.com>
> >
> > Hi Rob, all,
> >
> > this is a set of patches that I've been working on to allow me to use
> > reserved memory regions more flexibly. One of the use-cases that I have
> > is an external memory controller driver that gets passed one or two
> > tables from firmware containing a set of EMC frequencies and the
> > corresponding register values to program for these frequencies.
> >
> > One of these tables is the "nominal" table and an optional second table
> > is "derated" and is used when the DRAM chips are overheating. I want to
> > be able to pass these tables as separate memory-region entries.
> >
> > So what this small patchset does is make the reserved-memory code adapt
> > to this situation better. On one hand, while the DT bindings currently
> > support multiple regions per device tree node, it's slightly unintuitive
> > to specify them. The first patch adds a memory-region-names property
> > that allows the DT to specify a "consumer" name for these regions much
> > like we do for things like clocks, resets or the reg property. At the
> > same time, a new alias for memory-region, named memory-regions, is
> > introduced to make this more consistent with other bindings.
> 
> It's just not worth supporting both flavors (forever). I don't want to
> repeat gpio vs. gpios. Let's just stick with 'memory-region' and allow
> that to be more than one entry.

Alright, I'll drop the corresponding changes from the bindings and the
OF core then.

> I'm not a fan of *-names, but fine.

I suppose I could work without them, but I like the descriptiveness that
they add to the device tree. There are also cases where they can be very
essential. For example, what if a device can take two separate memory
regions. One case that we have on Tegra is the display controller that
can have a framebuffer and a color-conversion lookup table assigned to
it on boot. I think in this particular case both are actually always
there, but what if either of them was optional. Without -names it would
be impossible to describe the context if only one is provided in device
tree.

Thierry

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