[PATCH 0/2] Revert support for reserved memory regions defined in device tree

[PATCH 0/2] Revert support for reserved memory regions defined in device tree

[Marek Szyprowski]

Hi all!

Benjamin Herrenschmidt pointed a few issues in the proposed design of
device tree bindings for contiguous memory allocator and reserved memory
regions:
https://lkml.org/lkml/2013/9/15/151
http://www.spinics.net/lists/arm-kernel/msg273548.html

Some time has passed, but there is still no consensus on the bindings
for the reserved memory and various drawback of this solution has been
shown, so in my opinion the best I can do now is to revert them
completely and start from scratch again later.

This patch series reverts patches related to device tree bindings
proposed in the following thread:
http://thread.gmane.org/gmane.linux.ports.arm.kernel/263216
and merged by commit 64c353864e3f7ccba0ade1bd6f562f9a3bc7e68d ("Merge
branch 'for-v3.12' of git://git.linaro.org/people/mszyprowski/linux-dma-mapping").

Best regards
Marek Szyprowski
Samsung R&D Institute Poland

Marek Szyprowski (2):
  Revert "ARM: init: add support for reserved memory defined by device tree"
  Revert "drivers: of: add initialization code for dma reserved memory"

 Documentation/devicetree/bindings/memory.txt |  168 -------------------------
 arch/arm/mm/init.c                           |    3 -
 drivers/of/Kconfig                           |    6 -
 drivers/of/Makefile                          |    1 -
 drivers/of/of_reserved_mem.c                 |  173 --------------------------
 drivers/of/platform.c                        |    4 -
 include/linux/of_reserved_mem.h              |   14 ---
 7 files changed, 369 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/memory.txt
 delete mode 100644 drivers/of/of_reserved_mem.c
 delete mode 100644 include/linux/of_reserved_mem.h

-- 
1.7.9.5

[PATCH 1/2] Revert "ARM: init: add support for reserved memory defined by device tree"

[Marek Szyprowski]

This reverts commit 10bcdfb8ba24760f715f0a700c3812747eddddf5. There is
no consensus on the bindings for the reserved memory, so the code for
handing it will be reverted.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 arch/arm/mm/init.c |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index febaee7..18ec4c5 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -17,7 +17,6 @@
 #include <linux/nodemask.h>
 #include <linux/initrd.h>
 #include <linux/of_fdt.h>
-#include <linux/of_reserved_mem.h>
 #include <linux/highmem.h>
 #include <linux/gfp.h>
 #include <linux/memblock.h>
@@ -379,8 +378,6 @@ void __init arm_memblock_init(struct meminfo *mi,
 	if (mdesc->reserve)
 		mdesc->reserve();
 
-	early_init_dt_scan_reserved_mem();
-
 	/*
 	 * reserve memory for DMA contigouos allocations,
 	 * must come from DMA area inside low memory
-- 
1.7.9.5

[PATCH 2/2] Revert "drivers: of: add initialization code for dma reserved memory"

[Marek Szyprowski]

This reverts commit 9d8eab7af79cb4ce2de5de39f82c455b1f796963. There is
still no consensus on the bindings for the reserved memory and various
drawbacks of the proposed solution has been shown, so the best now is to
revert it completely and start again from scratch later.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 Documentation/devicetree/bindings/memory.txt |  168 -------------------------
 drivers/of/Kconfig                           |    6 -
 drivers/of/Makefile                          |    1 -
 drivers/of/of_reserved_mem.c                 |  173 --------------------------
 drivers/of/platform.c                        |    4 -
 include/linux/of_reserved_mem.h              |   14 ---
 6 files changed, 366 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/memory.txt
 delete mode 100644 drivers/of/of_reserved_mem.c
 delete mode 100644 include/linux/of_reserved_mem.h

diff --git a/Documentation/devicetree/bindings/memory.txt b/Documentation/devicetree/bindings/memory.txt
deleted file mode 100644
index eb24693..0000000
--- a/Documentation/devicetree/bindings/memory.txt
+++ /dev/null
@@ -1,168 +0,0 @@
-*** Memory binding ***
-
-The /memory node provides basic information about the address and size
-of the physical memory. This node is usually filled or updated by the
-bootloader, depending on the actual memory configuration of the given
-hardware.
-
-The memory layout is described by the following node:
-
-/ {
-	#address-cells = <(n)>;
-	#size-cells = <(m)>;
-	memory {
-		device_type = "memory";
-		reg =  <(baseaddr1) (size1)
-			(baseaddr2) (size2)
-			...
-			(baseaddrN) (sizeN)>;
-	};
-	...
-};
-
-A memory node follows the typical device tree rules for "reg" property:
-n:		number of cells used to store base address value
-m:		number of cells used to store size value
-baseaddrX:	defines a base address of the defined memory bank
-sizeX:		the size of the defined memory bank
-
-
-More than one memory bank can be defined.
-
-
-*** Reserved memory regions ***
-
-In /memory/reserved-memory node one can create child nodes describing
-particular reserved (excluded from normal use) memory regions. Such
-memory regions are usually designed for the special usage by various
-device drivers. A good example are contiguous memory allocations or
-memory sharing with other operating system on the same hardware board.
-Those special memory regions might depend on the board configuration and
-devices used on the target system.
-
-Parameters for each memory region can be encoded into the device tree
-with the following convention:
-
-[(label):] (name) {
-	compatible = "linux,contiguous-memory-region", "reserved-memory-region";
-	reg = <(address) (size)>;
-	(linux,default-contiguous-region);
-};
-
-compatible:	one or more of:
-	- "linux,contiguous-memory-region" - enables binding of this
-	  region to Contiguous Memory Allocator (special region for
-	  contiguous memory allocations, shared with movable system
-	  memory, Linux kernel-specific).
-	- "reserved-memory-region" - compatibility is defined, given
-	  region is assigned for exclusive usage for by the respective
-	  devices.
-
-reg:	standard property defining the base address and size of
-	the memory region
-
-linux,default-contiguous-region: property indicating that the region
-	is the default region for all contiguous memory
-	allocations, Linux specific (optional)
-
-It is optional to specify the base address, so if one wants to use
-autoconfiguration of the base address, '0' can be specified as a base
-address in the 'reg' property.
-
-The /memory/reserved-memory node must contain the same #address-cells
-and #size-cells value as the root node.
-
-
-*** Device node's properties ***
-
-Once regions in the /memory/reserved-memory node have been defined, they
-may be referenced by other device nodes. Bindings that wish to reference
-memory regions should explicitly document their use of the following
-property:
-
-memory-region = <&phandle_to_defined_region>;
-
-This property indicates that the device driver should use the memory
-region pointed by the given phandle.
-
-
-*** Example ***
-
-This example defines a memory consisting of 4 memory banks. 3 contiguous
-regions are defined for Linux kernel, one default of all device drivers
-(named contig_mem, placed at 0x72000000, 64MiB), one dedicated to the
-framebuffer device (labelled display_mem, placed at 0x78000000, 8MiB)
-and one for multimedia processing (labelled multimedia_mem, placed at
-0x77000000, 64MiB). 'display_mem' region is then assigned to fb at 12300000
-device for DMA memory allocations (Linux kernel drivers will use CMA is
-available or dma-exclusive usage otherwise). 'multimedia_mem' is
-assigned to scaler at 12500000 and codec at 12600000 devices for contiguous
-memory allocations when CMA driver is enabled.
-
-The reason for creating a separate region for framebuffer device is to
-match the framebuffer base address to the one configured by bootloader,
-so once Linux kernel drivers starts no glitches on the displayed boot
-logo appears. Scaller and codec drivers should share the memory
-allocations.
-
-/ {
-	#address-cells = <1>;
-	#size-cells = <1>;
-
-	/* ... */
-
-	memory {
-		reg =  <0x40000000 0x10000000
-			0x50000000 0x10000000
-			0x60000000 0x10000000
-			0x70000000 0x10000000>;
-
-		reserved-memory {
-			#address-cells = <1>;
-			#size-cells = <1>;
-
-			/*
-			 * global autoconfigured region for contiguous allocations
-			 * (used only with Contiguous Memory Allocator)
-			 */
-			contig_region at 0 {
-				compatible = "linux,contiguous-memory-region";
-				reg = <0x0 0x4000000>;
-				linux,default-contiguous-region;
-			};
-
-			/*
-			 * special region for framebuffer
-			 */
-			display_region: region at 78000000 {
-				compatible = "linux,contiguous-memory-region", "reserved-memory-region";
-				reg = <0x78000000 0x800000>;
-			};
-
-			/*
-			 * special region for multimedia processing devices
-			 */
-			multimedia_region: region at 77000000 {
-				compatible = "linux,contiguous-memory-region";
-				reg = <0x77000000 0x4000000>;
-			};
-		};
-	};
-
-	/* ... */
-
-	fb0: fb at 12300000 {
-		status = "okay";
-		memory-region = <&display_region>;
-	};
-
-	scaler: scaler at 12500000 {
-		status = "okay";
-		memory-region = <&multimedia_region>;
-	};
-
-	codec: codec at 12600000 {
-		status = "okay";
-		memory-region = <&multimedia_region>;
-	};
-};
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index 9d2009a..78cc760 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -74,10 +74,4 @@ config OF_MTD
 	depends on MTD
 	def_bool y
 
-config OF_RESERVED_MEM
-	depends on OF_FLATTREE && (DMA_CMA || (HAVE_GENERIC_DMA_COHERENT && HAVE_MEMBLOCK))
-	def_bool y
-	help
-	  Initialization code for DMA reserved memory
-
 endmenu # OF
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index ed9660a..efd0510 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -9,4 +9,3 @@ obj-$(CONFIG_OF_MDIO)	+= of_mdio.o
 obj-$(CONFIG_OF_PCI)	+= of_pci.o
 obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
 obj-$(CONFIG_OF_MTD)	+= of_mtd.o
-obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
deleted file mode 100644
index 0fe40c7..0000000
--- a/drivers/of/of_reserved_mem.c
+++ /dev/null
@@ -1,173 +0,0 @@
-/*
- * Device tree based initialization code for reserved memory.
- *
- * Copyright (c) 2013 Samsung Electronics Co., Ltd.
- *		http://www.samsung.com
- * Author: Marek Szyprowski <m.szyprowski@samsung.com>
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License as
- * published by the Free Software Foundation; either version 2 of the
- * License or (at your optional) any later version of the license.
- */
-
-#include <linux/memblock.h>
-#include <linux/err.h>
-#include <linux/of.h>
-#include <linux/of_fdt.h>
-#include <linux/of_platform.h>
-#include <linux/mm.h>
-#include <linux/sizes.h>
-#include <linux/mm_types.h>
-#include <linux/dma-contiguous.h>
-#include <linux/dma-mapping.h>
-#include <linux/of_reserved_mem.h>
-
-#define MAX_RESERVED_REGIONS	16
-struct reserved_mem {
-	phys_addr_t		base;
-	unsigned long		size;
-	struct cma		*cma;
-	char			name[32];
-};
-static struct reserved_mem reserved_mem[MAX_RESERVED_REGIONS];
-static int reserved_mem_count;
-
-static int __init fdt_scan_reserved_mem(unsigned long node, const char *uname,
-					int depth, void *data)
-{
-	struct reserved_mem *rmem = &reserved_mem[reserved_mem_count];
-	phys_addr_t base, size;
-	int is_cma, is_reserved;
-	unsigned long len;
-	const char *status;
-	__be32 *prop;
-
-	is_cma = IS_ENABLED(CONFIG_DMA_CMA) &&
-	       of_flat_dt_is_compatible(node, "linux,contiguous-memory-region");
-	is_reserved = of_flat_dt_is_compatible(node, "reserved-memory-region");
-
-	if (!is_reserved && !is_cma) {
-		/* ignore node and scan next one */
-		return 0;
-	}
-
-	status = of_get_flat_dt_prop(node, "status", &len);
-	if (status && strcmp(status, "okay") != 0) {
-		/* ignore disabled node nad scan next one */
-		return 0;
-	}
-
-	prop = of_get_flat_dt_prop(node, "reg", &len);
-	if (!prop || (len < (dt_root_size_cells + dt_root_addr_cells) *
-			     sizeof(__be32))) {
-		pr_err("Reserved mem: node %s, incorrect \"reg\" property\n",
-		       uname);
-		/* ignore node and scan next one */
-		return 0;
-	}
-	base = dt_mem_next_cell(dt_root_addr_cells, &prop);
-	size = dt_mem_next_cell(dt_root_size_cells, &prop);
-
-	if (!size) {
-		/* ignore node and scan next one */
-		return 0;
-	}
-
-	pr_info("Reserved mem: found %s, memory base %lx, size %ld MiB\n",
-		uname, (unsigned long)base, (unsigned long)size / SZ_1M);
-
-	if (reserved_mem_count == ARRAY_SIZE(reserved_mem))
-		return -ENOSPC;
-
-	rmem->base = base;
-	rmem->size = size;
-	strlcpy(rmem->name, uname, sizeof(rmem->name));
-
-	if (is_cma) {
-		struct cma *cma;
-		if (dma_contiguous_reserve_area(size, base, 0, &cma) == 0) {
-			rmem->cma = cma;
-			reserved_mem_count++;
-			if (of_get_flat_dt_prop(node,
-						"linux,default-contiguous-region",
-						NULL))
-				dma_contiguous_set_default(cma);
-		}
-	} else if (is_reserved) {
-		if (memblock_remove(base, size) == 0)
-			reserved_mem_count++;
-		else
-			pr_err("Failed to reserve memory for %s\n", uname);
-	}
-
-	return 0;
-}
-
-static struct reserved_mem *get_dma_memory_region(struct device *dev)
-{
-	struct device_node *node;
-	const char *name;
-	int i;
-
-	node = of_parse_phandle(dev->of_node, "memory-region", 0);
-	if (!node)
-		return NULL;
-
-	name = kbasename(node->full_name);
-	for (i = 0; i < reserved_mem_count; i++)
-		if (strcmp(name, reserved_mem[i].name) == 0)
-			return &reserved_mem[i];
-	return NULL;
-}
-
-/**
- * of_reserved_mem_device_init() - assign reserved memory region to given device
- *
- * This function assign memory region pointed by "memory-region" device tree
- * property to the given device.
- */
-void of_reserved_mem_device_init(struct device *dev)
-{
-	struct reserved_mem *region = get_dma_memory_region(dev);
-	if (!region)
-		return;
-
-	if (region->cma) {
-		dev_set_cma_area(dev, region->cma);
-		pr_info("Assigned CMA %s to %s device\n", region->name,
-			dev_name(dev));
-	} else {
-		if (dma_declare_coherent_memory(dev, region->base, region->base,
-		    region->size, DMA_MEMORY_MAP | DMA_MEMORY_EXCLUSIVE) != 0)
-			pr_info("Declared reserved memory %s to %s device\n",
-				region->name, dev_name(dev));
-	}
-}
-
-/**
- * of_reserved_mem_device_release() - release reserved memory device structures
- *
- * This function releases structures allocated for memory region handling for
- * the given device.
- */
-void of_reserved_mem_device_release(struct device *dev)
-{
-	struct reserved_mem *region = get_dma_memory_region(dev);
-	if (!region && !region->cma)
-		dma_release_declared_memory(dev);
-}
-
-/**
- * early_init_dt_scan_reserved_mem() - create reserved memory regions
- *
- * This function grabs memory from early allocator for device exclusive use
- * defined in device tree structures. It should be called by arch specific code
- * once the early allocator (memblock) has been activated and all other
- * subsystems have already allocated/reserved memory.
- */
-void __init early_init_dt_scan_reserved_mem(void)
-{
-	of_scan_flat_dt_by_path("/memory/reserved-memory",
-				fdt_scan_reserved_mem, NULL);
-}
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 9b439ac..f6dcde2 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -21,7 +21,6 @@
 #include <linux/of_device.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
-#include <linux/of_reserved_mem.h>
 #include <linux/platform_device.h>
 
 const struct of_device_id of_default_bus_match_table[] = {
@@ -219,8 +218,6 @@ static struct platform_device *of_platform_device_create_pdata(
 	dev->dev.bus = &platform_bus_type;
 	dev->dev.platform_data = platform_data;
 
-	of_reserved_mem_device_init(&dev->dev);
-
 	/* We do not fill the DMA ops for platform devices by default.
 	 * This is currently the responsibility of the platform code
 	 * to do such, possibly using a device notifier
@@ -228,7 +225,6 @@ static struct platform_device *of_platform_device_create_pdata(
 
 	if (of_device_add(dev) != 0) {
 		platform_device_put(dev);
-		of_reserved_mem_device_release(&dev->dev);
 		return NULL;
 	}
 
diff --git a/include/linux/of_reserved_mem.h b/include/linux/of_reserved_mem.h
deleted file mode 100644
index c841282..0000000
--- a/include/linux/of_reserved_mem.h
+++ /dev/null
@@ -1,14 +0,0 @@
-#ifndef __OF_RESERVED_MEM_H
-#define __OF_RESERVED_MEM_H
-
-#ifdef CONFIG_OF_RESERVED_MEM
-void of_reserved_mem_device_init(struct device *dev);
-void of_reserved_mem_device_release(struct device *dev);
-void early_init_dt_scan_reserved_mem(void);
-#else
-static inline void of_reserved_mem_device_init(struct device *dev) { }
-static inline void of_reserved_mem_device_release(struct device *dev) { }
-static inline void early_init_dt_scan_reserved_mem(void) { }
-#endif
-
-#endif /* __OF_RESERVED_MEM_H */
-- 
1.7.9.5

[PATCH 0/2] Revert support for reserved memory regions defined in device tree

[Benjamin Herrenschmidt]

On Fri, 2013-10-11 at 09:27 +0200, Marek Szyprowski wrote:
> Hi all!
> 
> Benjamin Herrenschmidt pointed a few issues in the proposed design of
> device tree bindings for contiguous memory allocator and reserved memory
> regions:
> https://lkml.org/lkml/2013/9/15/151
> http://www.spinics.net/lists/arm-kernel/msg273548.html

Are you guys in Edinburgh ?

I'll be there from Tuesday PM (so really Wed) to Friday (fly out on
Sat). We should catch up, brainstorm and come up with something.

Cheers,
Ben.

> Some time has passed, but there is still no consensus on the bindings
> for the reserved memory and various drawback of this solution has been
> shown, so in my opinion the best I can do now is to revert them
> completely and start from scratch again later.
> 
> This patch series reverts patches related to device tree bindings
> proposed in the following thread:
> http://thread.gmane.org/gmane.linux.ports.arm.kernel/263216
> and merged by commit 64c353864e3f7ccba0ade1bd6f562f9a3bc7e68d ("Merge
> branch 'for-v3.12' of git://git.linaro.org/people/mszyprowski/linux-dma-mapping").
> 
> Best regards
> Marek Szyprowski
> Samsung R&D Institute Poland
> 
> Marek Szyprowski (2):
>   Revert "ARM: init: add support for reserved memory defined by device tree"
>   Revert "drivers: of: add initialization code for dma reserved memory"
> 
>  Documentation/devicetree/bindings/memory.txt |  168 -------------------------
>  arch/arm/mm/init.c                           |    3 -
>  drivers/of/Kconfig                           |    6 -
>  drivers/of/Makefile                          |    1 -
>  drivers/of/of_reserved_mem.c                 |  173 --------------------------
>  drivers/of/platform.c                        |    4 -
>  include/linux/of_reserved_mem.h              |   14 ---
>  7 files changed, 369 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/memory.txt
>  delete mode 100644 drivers/of/of_reserved_mem.c
>  delete mode 100644 include/linux/of_reserved_mem.h
> 

[PATCH 0/2] Revert support for reserved memory regions defined in device tree

[Kumar Gala]

On Oct 11, 2013, at 2:48 AM, Benjamin Herrenschmidt wrote:

> On Fri, 2013-10-11 at 09:27 +0200, Marek Szyprowski wrote:
>> Hi all!
>> 
>> Benjamin Herrenschmidt pointed a few issues in the proposed design of
>> device tree bindings for contiguous memory allocator and reserved memory
>> regions:
>> https://lkml.org/lkml/2013/9/15/151
>> http://www.spinics.net/lists/arm-kernel/msg273548.html
> 
> Are you guys in Edinburgh ?
> 
> I'll be there from Tuesday PM (so really Wed) to Friday (fly out on
> Sat). We should catch up, brainstorm and come up with something.
> 
> Cheers,
> Ben.

We can try and do it during the DT-summit, or find another time.

- k

> 
>> Some time has passed, but there is still no consensus on the bindings
>> for the reserved memory and various drawback of this solution has been
>> shown, so in my opinion the best I can do now is to revert them
>> completely and start from scratch again later.
>> 
>> This patch series reverts patches related to device tree bindings
>> proposed in the following thread:
>> http://thread.gmane.org/gmane.linux.ports.arm.kernel/263216
>> and merged by commit 64c353864e3f7ccba0ade1bd6f562f9a3bc7e68d ("Merge
>> branch 'for-v3.12' of git://git.linaro.org/people/mszyprowski/linux-dma-mapping").
>> 
>> Best regards
>> Marek Szyprowski
>> Samsung R&D Institute Poland
>> 
>> Marek Szyprowski (2):
>>  Revert "ARM: init: add support for reserved memory defined by device tree"
>>  Revert "drivers: of: add initialization code for dma reserved memory"
>> 
>> Documentation/devicetree/bindings/memory.txt |  168 -------------------------
>> arch/arm/mm/init.c                           |    3 -
>> drivers/of/Kconfig                           |    6 -
>> drivers/of/Makefile                          |    1 -
>> drivers/of/of_reserved_mem.c                 |  173 --------------------------
>> drivers/of/platform.c                        |    4 -
>> include/linux/of_reserved_mem.h              |   14 ---
>> 7 files changed, 369 deletions(-)
>> delete mode 100644 Documentation/devicetree/bindings/memory.txt
>> delete mode 100644 drivers/of/of_reserved_mem.c
>> delete mode 100644 include/linux/of_reserved_mem.h
>> 
> 
> 

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH 0/2] Revert support for reserved memory regions defined in device tree

[Grant Likely]

On Fri, 11 Oct 2013 13:12:54 -0500, Kumar Gala <galak@codeaurora.org> wrote:
> 
> On Oct 11, 2013, at 2:48 AM, Benjamin Herrenschmidt wrote:
> 
> > On Fri, 2013-10-11 at 09:27 +0200, Marek Szyprowski wrote:
> >> Hi all!
> >> 
> >> Benjamin Herrenschmidt pointed a few issues in the proposed design of
> >> device tree bindings for contiguous memory allocator and reserved memory
> >> regions:
> >> https://lkml.org/lkml/2013/9/15/151
> >> http://www.spinics.net/lists/arm-kernel/msg273548.html
> > 
> > Are you guys in Edinburgh ?
> > 
> > I'll be there from Tuesday PM (so really Wed) to Friday (fly out on
> > Sat). We should catch up, brainstorm and come up with something.
> > 
> > Cheers,
> > Ben.
> 
> We can try and do it during the DT-summit, or find another time.

Let's plan on Wednesday afternoon.

g.

[PATCH 0/2] Revert support for reserved memory regions defined in device tree

[Matt Sealey]

On Fri, Oct 11, 2013 at 2:48 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Fri, 2013-10-11 at 09:27 +0200, Marek Szyprowski wrote:
>> Hi all!
>>
>> Benjamin Herrenschmidt pointed a few issues in the proposed design of
>> device tree bindings for contiguous memory allocator and reserved memory
>> regions:
>> https://lkml.org/lkml/2013/9/15/151
>> http://www.spinics.net/lists/arm-kernel/msg273548.html
>
> Are you guys in Edinburgh ?
>
> I'll be there from Tuesday PM (so really Wed) to Friday (fly out on
> Sat). We should catch up, brainstorm and come up with something.
>
> Cheers,
> Ben.

This may be late, but please can you consider re-using the CHRP
reserved node (i.e. device_type = "reserved")?

Since it does exactly the same thing, is well defined since the dark ages?

It's CHRP 1.7 section 5.9 by the way (just before /chosen gets defined).

It would solve a selection of the issues; and require zero binding
work except describing perhaps a couple freakish Linux-specific
properties that may be only as intrusive as, say, linux,initrd would
be in /chosen.

The most effective, multi-OS way of using it ("available" property not
currently implemented in Linux for some reason, but it could come in
so handy - not only because it matches the way Linux resource
structures are handled)

memory at 0x70000000 {
   device_type = "memory";
   reg = <0x70000000 0x40000000>;
   available = <0x70000000 0x10000000
                     0x90000000 0x1ffc00000>; /* top 16KiB of memory
is where the secure firmware keeps it's mailboxes */
};

freaky-codec-memory: reserved at 0x80000000 {
    device_type = "reserved";
    reg = <0x80000000 0x10000000>;
    available = <0x80000000 0x8000000
                      0x88000000 0x8000000>; /* two 128MiB buffers */
    non-objectionable-mark-as-contiguous-property-name-here;
    cacheable;
};

Any driver that has, previously, required a bunch of it's own memory
carved out of DDR *should* be gaining a phandle reference to that
reserved node however it likes (it would be up to that devices'
binding).

On Linux under CMA, it may well be ignored and just stuffed into the
generic CMA regions, and the driver MAY allocate anywhere it likes,
but it COULD ask for memory based on a region phandle or, horribly, by
name (since there's no other way to search for it, the OF "name" for
reserved SHALL be "reserved") and be given memory in that region
defined by the reserved node if it had any addressing restrictions.

/videodecoder at 0x43f01000 {
    compatible = "freaky,codec";
    :
    decode-buffer = &freaky-codec-memory;
};

On another OS it may manually map and use a custom allocator for that
memory region, since otherwise the OS would not have even looked at
it.

Also this discussion of Jeremy Kerr's proposal seems to be 'missing'
on Google. Do you happen to have a link to it?

Thanks,
Matt Sealey <neko@bakuhatsu.net>

[PATCH 0/2] Revert support for reserved memory regions defined in device tree

[Benjamin Herrenschmidt]

On Thu, 2013-10-17 at 13:37 -0500, Matt Sealey wrote:

> This may be late, but please can you consider re-using the CHRP
> reserved node (i.e. device_type = "reserved")?
> 
> Since it does exactly the same thing, is well defined since the dark ages?
> 
> It's CHRP 1.7 section 5.9 by the way (just before /chosen gets defined).
> 
> It would solve a selection of the issues; and require zero binding
> work except describing perhaps a couple freakish Linux-specific
> properties that may be only as intrusive as, say, linux,initrd would
> be in /chosen.
> 
> The most effective, multi-OS way of using it ("available" property not
> currently implemented in Linux for some reason, but it could come in
> so handy - not only because it matches the way Linux resource
> structures are handled)

First, the original /reserved on CHRP was supposed to be about reserved
bus space for things like hidden HW devices, but yes, it could be used for
that. However it would be nice to enrich the binding to provide at least
some kind of specific identification of what a given reserved area is about,
see my comments about that in the previous threads.

The available property is of no use to us. It purely indicates what is
available while OFW is still running. Once we get rid of OFW its content
is utterly meaningless.

The original OFW was design with the idea that OFW remains alive along
with the operating system, and that has been done on some platforms, but
that idea has been ditched very early on in powerpc space for many
reasons, one of them being that most implementations of OFW around were
way to broken to bother.

> memory at 0x70000000 {
>    device_type = "memory";
>    reg = <0x70000000 0x40000000>;
>    available = <0x70000000 0x10000000
>                      0x90000000 0x1ffc00000>; /* top 16KiB of memory
> is where the secure firmware keeps it's mailboxes */
> };

> freaky-codec-memory: reserved at 0x80000000 {
>     device_type = "reserved";
>     reg = <0x80000000 0x10000000>;
>     available = <0x80000000 0x8000000
>                       0x88000000 0x8000000>; /* two 128MiB buffers */
>     non-objectionable-mark-as-contiguous-property-name-here;
>     cacheable;
> };
> 
> Any driver that has, previously, required a bunch of it's own memory
> carved out of DDR *should* be gaining a phandle reference to that
> reserved node however it likes (it would be up to that devices'
> binding).
> 
> On Linux under CMA, it may well be ignored and just stuffed into the
> generic CMA regions, and the driver MAY allocate anywhere it likes,
> but it COULD ask for memory based on a region phandle or, horribly, by
> name (since there's no other way to search for it, the OF "name" for
> reserved SHALL be "reserved") and be given memory in that region
> defined by the reserved node if it had any addressing restrictions.
> 
> /videodecoder at 0x43f01000 {
>     compatible = "freaky,codec";
>     :
>     decode-buffer = &freaky-codec-memory;
> };
> 
> On another OS it may manually map and use a custom allocator for that
> memory region, since otherwise the OS would not have even looked at
> it.
> 
> Also this discussion of Jeremy Kerr's proposal seems to be 'missing'
> on Google. Do you happen to have a link to it?
> 
> Thanks,
> Matt Sealey <neko@bakuhatsu.net>

[PATCH 0/2] Revert support for reserved memory regions defined in device tree

[Grant Likely]

On Fri, 11 Oct 2013 09:27:26 +0200, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> Hi all!
> 
> Benjamin Herrenschmidt pointed a few issues in the proposed design of
> device tree bindings for contiguous memory allocator and reserved memory
> regions:
> https://lkml.org/lkml/2013/9/15/151
> http://www.spinics.net/lists/arm-kernel/msg273548.html
> 
> Some time has passed, but there is still no consensus on the bindings
> for the reserved memory and various drawback of this solution has been
> shown, so in my opinion the best I can do now is to revert them
> completely and start from scratch again later.

I had some fixup patches that I was getting ready to post, but your
right. We're really late to be doing anything like that. I've applied
the reverts and we'll plan on hammering something out in Edinburgh next
week so that it can be merged for v3.13

g.

[PATCH 0/2] Revert support for reserved memory regions defined in device tree

[Marek Szyprowski]

Hello,

On 2013-10-15 13:40, Grant Likely wrote:
> On Fri, 11 Oct 2013 09:27:26 +0200, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> > Hi all!
> >
> > Benjamin Herrenschmidt pointed a few issues in the proposed design of
> > device tree bindings for contiguous memory allocator and reserved memory
> > regions:
> > https://lkml.org/lkml/2013/9/15/151
> > http://www.spinics.net/lists/arm-kernel/msg273548.html
> >
> > Some time has passed, but there is still no consensus on the bindings
> > for the reserved memory and various drawback of this solution has been
> > shown, so in my opinion the best I can do now is to revert them
> > completely and start from scratch again later.
>
> I had some fixup patches that I was getting ready to post, but your
> right. We're really late to be doing anything like that. I've applied
> the reverts and we'll plan on hammering something out in Edinburgh next
> week so that it can be merged for v3.13

Sadly I won't be able to go to Edinburgh because of other business trip 
the same time. However Tomasz Figa will be there for the whole week, he 
will also attend DT-summit, so I hope that you will manage to find a 
consensus. I will implement whatever you will agree then.

Best regards
-- 
Marek Szyprowski
Samsung R&D Institute Poland

[PATCH 0/2] Revert support for reserved memory regions defined in device tree

[Grant Likely]

On Fri, 11 Oct 2013 09:27:26 +0200, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> Hi all!
> 
> Benjamin Herrenschmidt pointed a few issues in the proposed design of
> device tree bindings for contiguous memory allocator and reserved memory
> regions:
> https://lkml.org/lkml/2013/9/15/151
> http://www.spinics.net/lists/arm-kernel/msg273548.html
> 
> Some time has passed, but there is still no consensus on the bindings
> for the reserved memory and various drawback of this solution has been
> shown, so in my opinion the best I can do now is to revert them
> completely and start from scratch again later.

Hi Marek,

At the ARM summit last week in Edinburgh, several of us sat down and
hammered out a new proposal for handling reserved memory regions based
on the work that you started here. Below you will find a new binding
document. I started looking at implementing this, but haven't made much
progress.

Please take a look and let me know what you think.

Also, while I'm thinking about it, I took another look at the code and I
think the code supporting reserved regions should go directly into
drivers/of/fdt.c and drivers/of/memory.c.  Also, the reserved regions
parsing should be enabled unconditionally insted of filtered by (DMA_CMA
|| (HAVE_GENERIC_DMA_COHERENT && HAVE_MEMBLOCK). If the hardware
description says to reserve a region, then the kernel must always do so,
even if it doesn't actually use it for anything.

g.

-------

*** Reserved memory regions ***

Reserved memory is specified as a node under the /reserved-memory node.
The operating system shall exclude reserved memory from normal usage
one can create child nodes describing particular reserved (excluded from
normal use) memory regions. Such memory regions are usually designed for
the special usage by various device drivers.

Parameters for each memory region can be encoded into the device tree
with the following nodes:

/reserved-memory node
---------------------
#address-cells, #size-cells (required) - standard definition
    - Should use the same values as the root node
ranges (required) - standard definition
    - Should be empty

/reserved-memory/ child nodes
-----------------------------
Each child of the reserved-memory node specifies one or more regions of
reserved memory. Each child node may either use a 'reg' property to
specify a specific range of reserved memory, or a 'size' property with
optional constraints to request a dynamically allocated block of memory.

Following the generic-names recommended practice, node names should
reflect the purpose of the node (ie. "framebuffer" or "dma-pool"). Unit
address (@<address>) should be appended to the name if the node is a
static allocation.

Properties:
Requires either a) or b) below.
a) static allocation
   reg (required) - standard definition
b) dynamic allocation
   size (required) - length based on parent's #size-cells
                   - Size in bytes of memory to reserve.
   alignment (optional) - length based on parent's #size-cells
                        - Address boundary for alignment of allocation.
   alloc-ranges (optional) - prop-encoded-array (address, length pairs).
                           - Specifies regions of memory that are
                             acceptable to allocate from.

If both reg and size are present, then the reg property takes precedence
and size is ignored.

Additional properties:
compatible (optional) - standard definition
    - may contain the following strings:
        - shared-dma-pool: This indicates a region of memory meant to be
          used as a shared pool of DMA buffers for a set of devices. It can
          be used by an operating system to instanciate the necessary pool
          management subsystem if necessary.
        - vendor specific string in the form <vendor>,[<device>-]<usage>
no-map (optional) - empty property
    - Indicates the operating system must not create a virtual mapping
      of the region as part of its standard mapping of system memory,
      nor permit speculative access to it under any circumstances other
      than under the control of the device driver using the region.
reusable (optional) - empty property
    - The operating system can use the memory in this region with the
      limitation that the device driver(s) owning the region need to be
      able to reclaim it back. Typically that means that the operating
      system can use that region to store volatile or cached data that
      can be otherwise regenerated or migrated elsewhere.

Linux implementation note:
- If a "linux,cma-default" property is present, then Linux will use the
  region for the default pool of the contiguous memory allocator.

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 to child of /reserved-memory

Example
-------
This example defines 3 contiguous regions are defined for Linux kernel:
one default of all device drivers (named linux,cma at 72000000 and 64MiB in size),
one dedicated to the framebuffer device (named framebuffer at 78000000, 8MiB), and
one for multimedia processing (named multimedia-memory at 77000000, 64MiB).

/ {
	#address-cells = <1>;
	#size-cells = <1>;

	memory {
		reg = <0x40000000 0x40000000>;
	};

	reserved-memory {
		#address-cells = <1>;
		#size-cells = <1>;
		ranges;

		/* global autoconfigured region for contiguous allocations */
		linux,cma {
			compatible = "shared-dma-pool";
			size = <0x4000000>;
			alignment = <0x2000>;
			linux,cma-default;
		};

		display_reserved: framebuffer at 78000000 {
			reg = <0x78000000 0x800000>;
		};

		multimedia_reserved: multimedia at 77000000 {
			compatible = "acme,multimedia-memory";
			reg = <0x77000000 0x4000000>;
		};
	};

	/* ... */

	fb0: video at 12300000 {
		memory-region = <&display_reserved>;
		/* ... */
	};

	scaler: scaler at 12500000 {
		memory-region = <&multimedia_reserved>;
		/* ... */
	};

	codec: codec at 12600000 {
		memory-region = <&multimedia_reserved>;
		/* ... */
	};
};

[PATCH 0/2] Revert support for reserved memory regions defined in device tree

[Matt Sealey]

On Wed, Oct 30, 2013 at 8:47 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
>
> Hi Marek,
>
> Properties:
> Requires either a) or b) below.
> a) static allocation
>    reg (required) - standard definition
> b) dynamic allocation
>    size (required) - length based on parent's #size-cells
>                    - Size in bytes of memory to reserve.
>
>    alignment (optional) - length based on parent's #size-cells
>                         - Address boundary for alignment of allocation.
>    alloc-ranges (optional) - prop-encoded-array (address, length pairs).
>                            - Specifies regions of memory that are
>                              acceptable to allocate from.

"available"?

Let's not reinvent the wheel, if at all possible. "available" has a
well defined nature (exactly as you described).

I still say device_type = "reserved" fits the bill here very well.
Sure it had another intended purpose, but it does everything described
and they're easily collectible with device_type annotation than 100
different compatible properties, per-device, per-SoC, per-board..

> If both reg and size are present, then the reg property takes precedence
> and size is ignored.

What about the case where the address and size are larger than 32-bit
cells can hold, and reg cannot hold it (reg is not appropriate for any
case where #size-cells is not 1)?

In those cases (where #size-cells would be anything other than 1) reg
is not the right property to use, and another way needs to be used. If
'reg' and 'size' is present, and 'size' is bigger than the size in the
reg property, size should just override it. What would be an "error"
is if the size in the reg property was anything other than 0 if the
size property would override it (this would reduce mis-parsing).
Otherwise how would you carve out 8GiB on a system with 64GiB (which
is eminently possible with LPAE)? reg handles the "phys.hi phys.lo
size" case and that's about it for >32-bit addressing.

> Additional properties:
> compatible (optional) - standard definition
>     - may contain the following strings:
>         - shared-dma-pool: This indicates a region of memory meant to be
>           used as a shared pool of DMA buffers for a set of devices. It can
>           be used by an operating system to instanciate the necessary pool
>           management subsystem if necessary.
>         - vendor specific string in the form <vendor>,[<device>-]<usage>

So, the only way the memory parsers you're suggesting can pull these
out is based on the idea that they're children of whatever node had
"reserved-memory" as it's "name" property?

Don't we, and haven't we always, said matching against the "name"
property is evil, evil, don't do it?

> no-map (optional) - empty property
>     - Indicates the operating system must not create a virtual mapping
>       of the region as part of its standard mapping of system memory,
>       nor permit speculative access to it under any circumstances other
>       than under the control of the device driver using the region.
> reusable (optional) - empty property
>     - The operating system can use the memory in this region with the
>       limitation that the device driver(s) owning the region need to be
>       able to reclaim it back. Typically that means that the operating
>       system can use that region to store volatile or cached data that
>       can be otherwise regenerated or migrated elsewhere.
>
> Linux implementation note:
> - If a "linux,cma-default" property is present, then Linux will use the
>   region for the default pool of the contiguous memory allocator.

CMA should just pick up shared-dma-pool, reusable, and take that as a
given, rather than having a magic property to go look for. In this
case, why not ditch "shared-dma-pool" as a compatible and just have a
"shared" property? "shared", "reusable" fits CMA nicely - any memory
CMA has "carved out" will get used for buffer caches etc. under memory
pressure anyway if it's transient data.

>         reserved-memory {
>                 #address-cells = <1>;
>                 #size-cells = <1>;
>                 ranges;
>
>                 display_reserved: framebuffer at 78000000 {
>                         reg = <0x78000000 0x800000>;
>                 };
>
>         };
>
>         /* ... */
>
>         fb0: video at 12300000 {
>                 memory-region = <&display_reserved>;
>                 /* ... */
>         };

In this case, it makes absolutely no sense for the reserved node to
even HAVE a compatible property, does it?

-- 
Matt Sealey <neko@bakuhatsu.net>

[PATCH 0/2] Revert support for reserved memory regions defined in device tree

[Marek Szyprowski]

Hi Grant!

On 2013-10-30 14:47, Grant Likely wrote:
> On Fri, 11 Oct 2013 09:27:26 +0200, Marek Szyprowski<m.szyprowski@samsung.com>  wrote:
> > Hi all!
> >
> > Benjamin Herrenschmidt pointed a few issues in the proposed design of
> > device tree bindings for contiguous memory allocator and reserved memory
> > regions:
> >https://lkml.org/lkml/2013/9/15/151
> >http://www.spinics.net/lists/arm-kernel/msg273548.html
> >
> > Some time has passed, but there is still no consensus on the bindings
> > for the reserved memory and various drawback of this solution has been
> > shown, so in my opinion the best I can do now is to revert them
> > completely and start from scratch again later.
>
> Hi Marek,
>
> At the ARM summit last week in Edinburgh, several of us sat down and
> hammered out a new proposal for handling reserved memory regions based
> on the work that you started here. Below you will find a new binding
> document. I started looking at implementing this, but haven't made much
> progress.
>
> Please take a look and let me know what you think.
>
> Also, while I'm thinking about it, I took another look at the code and I
> think the code supporting reserved regions should go directly into
> drivers/of/fdt.c and drivers/of/memory.c.  Also, the reserved regions
> parsing should be enabled unconditionally insted of filtered by (DMA_CMA
> || (HAVE_GENERIC_DMA_COHERENT && HAVE_MEMBLOCK). If the hardware
> description says to reserve a region, then the kernel must always do so,
> even if it doesn't actually use it for anything.

Thanks for discussing this item. I'm really sorry for the late reply, but
various 'more_imporant_things(tm)' have eaten me completely last weeks.

The proposal look good for me. I'm not convinced that we really need the
support for 'reg' property, as the fixed memory region is a special case
of generic dynamic allocation specified by the size and alloc-ranges, but
I assume that there have been already a long discussion about this, so I
accept the common consensus.

Grant: have you started working on the code, which implements such binding?
If not, I will try to start do it and post the code soon for review.

Best regards
-- 
Marek Szyprowski
Samsung R&D Institute Poland

[PATCH 0/2] Revert support for reserved memory regions defined in device tree

[Grant Likely]

On Tue, 19 Nov 2013 13:27:53 +0100, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> Hi Grant!
> 
> On 2013-10-30 14:47, Grant Likely wrote:
> > On Fri, 11 Oct 2013 09:27:26 +0200, Marek Szyprowski<m.szyprowski@samsung.com>  wrote:
> > > Hi all!
> > >
> > > Benjamin Herrenschmidt pointed a few issues in the proposed design of
> > > device tree bindings for contiguous memory allocator and reserved memory
> > > regions:
> > >https://lkml.org/lkml/2013/9/15/151
> > >http://www.spinics.net/lists/arm-kernel/msg273548.html
> > >
> > > Some time has passed, but there is still no consensus on the bindings
> > > for the reserved memory and various drawback of this solution has been
> > > shown, so in my opinion the best I can do now is to revert them
> > > completely and start from scratch again later.
> >
> > Hi Marek,
> >
> > At the ARM summit last week in Edinburgh, several of us sat down and
> > hammered out a new proposal for handling reserved memory regions based
> > on the work that you started here. Below you will find a new binding
> > document. I started looking at implementing this, but haven't made much
> > progress.
> >
> > Please take a look and let me know what you think.
> >
> > Also, while I'm thinking about it, I took another look at the code and I
> > think the code supporting reserved regions should go directly into
> > drivers/of/fdt.c and drivers/of/memory.c.  Also, the reserved regions
> > parsing should be enabled unconditionally insted of filtered by (DMA_CMA
> > || (HAVE_GENERIC_DMA_COHERENT && HAVE_MEMBLOCK). If the hardware
> > description says to reserve a region, then the kernel must always do so,
> > even if it doesn't actually use it for anything.
> 
> Thanks for discussing this item. I'm really sorry for the late reply, but
> various 'more_imporant_things(tm)' have eaten me completely last weeks.
> 
> The proposal look good for me. I'm not convinced that we really need the
> support for 'reg' property, as the fixed memory region is a special case
> of generic dynamic allocation specified by the size and alloc-ranges, but
> I assume that there have been already a long discussion about this, so I
> accept the common consensus.

It is absolutely necessary for some use cases. For example, a
framebuffer enabled by firmware and passed onto the kernel for
flicker-free boot. Some platforms have fixed regions that cannot be
moved up.

> Grant: have you started working on the code, which implements such binding?
> If not, I will try to start do it and post the code soon for review.

No, please go ahead and start.

g.

[PATCH 0/2] Revert support for reserved memory regions defined in device tree

[Benjamin Herrenschmidt]

On Tue, 2013-11-19 at 15:14 +0000, Grant Likely wrote:
> > The proposal look good for me. I'm not convinced that we really need the
> > support for 'reg' property, as the fixed memory region is a special case
> > of generic dynamic allocation specified by the size and alloc-ranges, but
> > I assume that there have been already a long discussion about this, so I
> > accept the common consensus.
> 
> It is absolutely necessary for some use cases. For example, a
> framebuffer enabled by firmware and passed onto the kernel for
> flicker-free boot. Some platforms have fixed regions that cannot be
> moved up.

Arguably that could be covered with alloc-range and size by making the range be
the reg property content basically (and thus size == size of range) but I
prefer the reg property, it's a clearer statement of intent.

Cheers,
Ben.

[PATCH 0/2] Revert support for reserved memory regions defined in device tree

[Grant Likely]

On Wed, 20 Nov 2013 09:01:44 +1100, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> On Tue, 2013-11-19 at 15:14 +0000, Grant Likely wrote:
> > > The proposal look good for me. I'm not convinced that we really need the
> > > support for 'reg' property, as the fixed memory region is a special case
> > > of generic dynamic allocation specified by the size and alloc-ranges, but
> > > I assume that there have been already a long discussion about this, so I
> > > accept the common consensus.
> > 
> > It is absolutely necessary for some use cases. For example, a
> > framebuffer enabled by firmware and passed onto the kernel for
> > flicker-free boot. Some platforms have fixed regions that cannot be
> > moved up.
> 
> Arguably that could be covered with alloc-range and size by making the range be
> the reg property content basically (and thus size == size of range) but I
> prefer the reg property, it's a clearer statement of intent.

True, but I agree on liking 'reg'. Plus I suspect that the simplest
implementation will resolve the size+alloc-ranges property into a
generated 'reg' property during early boot so that the existing decode
APIs work on any reserved range.  :-)

g.