Re: [PATCH v2 1/5] drivers: of: add initialization code for reserved memory

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Hi,

On 11.02.2014 13:13, Grant Likely wrote:
On Tue, 11 Feb 2014 12:45:50 +0100, Marek Szyprowski <m.szyprowski@xxxxxxxxxxx> wrote:
Hello,

On 2014-02-05 12:05, Grant Likely wrote:
On Tue, 04 Feb 2014 13:09:29 +0100, Marek Szyprowski <m.szyprowski@xxxxxxxxxxx> wrote:
This patch adds device tree support for contiguous and reserved memory
regions defined in device tree.

Large memory blocks can be reliably reserved only during early boot.
This must happen before the whole memory management subsystem is
initialized, because we need to ensure that the given contiguous blocks
are not yet allocated by kernel. Also it must happen before kernel
mappings for the whole low memory are created, to ensure that there will
be no mappings (for reserved blocks) or mapping with special properties
can be created (for CMA blocks). This all happens before device tree
structures are unflattened, so we need to get reserved memory layout
directly from fdt.

Later, those reserved memory regions are assigned to devices on each
device structure initialization.

Cc: Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx>
Cc: Laura Abbott <lauraa@xxxxxxxxxxxxxx>
Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
[joshc: rework to implement new DT binding, provide mechanism for
  plugging in new reserved-memory node handlers via
  RESERVEDMEM_OF_DECLARE]
Signed-off-by: Josh Cartwright <joshc@xxxxxxxxxxxxxx>
[mszyprow: little code cleanup]
Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
---
  drivers/of/Kconfig                |    6 +
  drivers/of/Makefile               |    1 +
  drivers/of/of_reserved_mem.c      |  219 +++++++++++++++++++++++++++++++++++++
  drivers/of/platform.c             |    7 ++
  include/asm-generic/vmlinux.lds.h |   11 ++
  include/linux/of_reserved_mem.h   |   62 +++++++++++
  6 files changed, 306 insertions(+)
  create mode 100644 drivers/of/of_reserved_mem.c
  create mode 100644 include/linux/of_reserved_mem.h

diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index c6973f101a3e..aba13df56f3a 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -75,4 +75,10 @@ config OF_MTD
  	depends on MTD
  	def_bool y

+config OF_RESERVED_MEM
+	depends on HAVE_MEMBLOCK
+	def_bool y
+	help
+	  Helpers to allow for reservation of memory regions
+
  endmenu # OF
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index efd05102c405..ed9660adad77 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -9,3 +9,4 @@ 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
new file mode 100644
index 000000000000..f17cd56e68d9
--- /dev/null
+++ b/drivers/of/of_reserved_mem.c
@@ -0,0 +1,219 @@
+/*
+ * Device tree based initialization code for reserved memory.
+ *
+ * Copyright (c) 2013, The Linux Foundation. All Rights Reserved.
+ * Copyright (c) 2013 Samsung Electronics Co., Ltd.
+ *		http://www.samsung.com
+ * Author: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
+ * Author: Josh Cartwright <joshc@xxxxxxxxxxxxxx>
+ *
+ * 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/of_reserved_mem.h>
+
+#define MAX_RESERVED_REGIONS	16
+static struct reserved_mem reserved_mem[MAX_RESERVED_REGIONS];
+static int reserved_mem_count;
+
+int __init of_parse_flat_dt_reg(unsigned long node, const char *uname,
+				   phys_addr_t *base, phys_addr_t *size)

Useful utility function; move to drivers/of/fdt.c

+{
+	unsigned long len;
+	__be32 *prop;
+
+	prop = of_get_flat_dt_prop(node, "reg", &len);
+	if (!prop)
+		return -EINVAL;
+
+	if (len < (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32)) {
+		pr_err("Reserved memory: invalid reg property in '%s' node.\n",
+				uname);
+		return -EINVAL;
+	}

This is /okay/ for an initial implementation, but it is naive. While I
suggested making #address-cells and #size-cells equal the root node
values for the purpose of simplicity, it should still be perfectly valid
to have different values if the ranges property is correctly formed.

+
+	*base = dt_mem_next_cell(dt_root_addr_cells, &prop);
+	*size = dt_mem_next_cell(dt_root_size_cells, &prop);

Future enhancement; allow for parsing more than just the first reg
tuple.

One more question. Does it really makes any sense to support more than
one tuple for reg property? For consistency we should also allow more
than one entry in size, align and alloc-ranges property, but I don't
see any benefits for defining more than one range for a single region.
Same can be achieved by defining more regions instead if one really
needs such configuration.

Yes, if only because it is an define usage of the reg property. If a
devtree has multiple tuples in reg, then all of those tuples should be
treated as reserved, even if the kernel doesn't know how to use them.

I would not do the same for size/align/alloc-ranges unless there is a
very specific use case that you can define. These ones are different
from the static regions because they aren't ever used to protect
something that already exists in the memory.

Is there a reason why multiple regions could not be used for this purpose, instead of adding extra complexity of having multiple reg entries per region?

I.e. I don't see a difference between

reg1: region@00000000 {
	reg = <0x00000000 0x1000>;
};

reg2: region@10000000 {
	reg = <0x10000000 0x1000>;
};

user {
	regions = <&reg1>, <&reg2>;
};

and

reg: region@00000000 {
	reg = <0x00000000 0x1000>, <0x10000000 0x1000>;
};

user {
	regions = <&reg>;
};

except that the former IMHO better suits the definition of memory region, which I see as a single contiguous range of memory and can be simplified to have a single reg entry per region.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux