[U-Boot] [PATCH 1/6] fdtdec: Add cpu_to_fdt_{addr,size}() macros

[U-Boot] [PATCH 1/6] fdtdec: Add cpu_to_fdt_{addr,size}() macros

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

These macros are useful for converting the endianness of variables of
type fdt_addr_t and fdt_size_t.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 include/fdtdec.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/fdtdec.h b/include/fdtdec.h
index b7e35cd87c55..a965c33157c9 100644
--- a/include/fdtdec.h
+++ b/include/fdtdec.h
@@ -27,11 +27,15 @@ typedef phys_size_t fdt_size_t;
 #define FDT_ADDR_T_NONE (-1U)
 #define fdt_addr_to_cpu(reg) be64_to_cpu(reg)
 #define fdt_size_to_cpu(reg) be64_to_cpu(reg)
+#define cpu_to_fdt_addr(reg) cpu_to_be64(reg)
+#define cpu_to_fdt_size(reg) cpu_to_be64(reg)
 typedef fdt64_t fdt_val_t;
 #else
 #define FDT_ADDR_T_NONE (-1U)
 #define fdt_addr_to_cpu(reg) be32_to_cpu(reg)
 #define fdt_size_to_cpu(reg) be32_to_cpu(reg)
+#define cpu_to_fdt_addr(reg) cpu_to_be32(reg)
+#define cpu_to_fdt_size(reg) cpu_to_be32(reg)
 typedef fdt32_t fdt_val_t;
 #endif
 
-- 
2.20.1

[U-Boot] [PATCH 2/6] fdtdec: Implement fdtdec_get_max_phandle()

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

This function allows looking up the highest phandle value stored in a
device tree, which is useful to determine the next best phandle value
for new nodes.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 include/fdtdec.h | 12 ++++++++++++
 lib/fdtdec.c     | 28 ++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/include/fdtdec.h b/include/fdtdec.h
index a965c33157c9..5eb3c0c237a9 100644
--- a/include/fdtdec.h
+++ b/include/fdtdec.h
@@ -956,6 +956,18 @@ int fdtdec_setup_mem_size_base(void);
  */
 int fdtdec_setup_memory_banksize(void);
 
+/**
+ * fdtdec_get_max_phandle() - return the highest phandle in an FDT blob
+ *
+ * Returns the highest phandle in the given FDT blob. The result of this can
+ * be used to generate a new phandle by incrementing by one.
+ *
+ * @param blob	FDT blob
+ * @param maxp	return location for the highest phandle in the FDT blob
+ * @return 0 on success or a negative error code on failure
+ */
+int fdtdec_get_max_phandle(const void *blob, uint32_t *maxp);
+
 /**
  * Set up the device tree ready for use
  */
diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index 09a7e133a539..f2af947c106e 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -1243,6 +1243,34 @@ __weak void *board_fdt_blob_setup(void)
 }
 #endif
 
+int fdtdec_get_max_phandle(const void *blob, uint32_t *maxp)
+{
+	uint32_t max = 0;
+	int offset = -1;
+
+	while (true) {
+		uint32_t phandle;
+
+		offset = fdt_next_node(blob, offset, NULL);
+		if (offset < 0) {
+			if (offset == -FDT_ERR_NOTFOUND)
+				break;
+
+			return offset;
+		}
+
+		phandle = fdt_get_phandle(blob, offset);
+
+		if (phandle > max)
+			max = phandle;
+	}
+
+	if (maxp)
+		*maxp = max;
+
+	return 0;
+}
+
 int fdtdec_setup(void)
 {
 #if CONFIG_IS_ENABLED(OF_CONTROL)
-- 
2.20.1

[U-Boot] [PATCH 3/6] fdtdec: Implement fdtdec_set_phandle()

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

This function can be used to set a phandle for a given node.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 include/fdtdec.h | 11 +++++++++++
 lib/fdtdec.c     | 16 ++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/include/fdtdec.h b/include/fdtdec.h
index 5eb3c0c237a9..997103a87cdf 100644
--- a/include/fdtdec.h
+++ b/include/fdtdec.h
@@ -968,6 +968,17 @@ int fdtdec_setup_memory_banksize(void);
  */
 int fdtdec_get_max_phandle(const void *blob, uint32_t *maxp);
 
+/**
+ * fdtdec_set_phandle() - sets the phandle of a given node
+ *
+ * @param blob		FDT blob
+ * @param node		offset in the FDT blob of the node whose phandle is to
+ *			be set
+ * @param phandle	phandle to set for the given node
+ * @return 0 on success or a negative error code on failure
+ */
+int fdtdec_set_phandle(void *blob, int node, uint32_t phandle);
+
 /**
  * Set up the device tree ready for use
  */
diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index f2af947c106e..9195a05d1129 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -1271,6 +1271,22 @@ int fdtdec_get_max_phandle(const void *blob, uint32_t *maxp)
 	return 0;
 }
 
+int fdtdec_set_phandle(void *blob, int node, uint32_t phandle)
+{
+	fdt32_t value = cpu_to_fdt32(phandle);
+	int err;
+
+	err = fdt_setprop(blob, node, "linux,phandle", &value, sizeof(value));
+	if (err < 0)
+		return err;
+
+	err = fdt_setprop(blob, node, "phandle", &value, sizeof(value));
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+
 int fdtdec_setup(void)
 {
 #if CONFIG_IS_ENABLED(OF_CONTROL)
-- 
2.20.1

[U-Boot] [PATCH 4/6] fdtdec: Implement fdtdec_add_reserved_memory()

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

This function can be used to add subnodes in the /reserved-memory node.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 include/fdtdec.h |  17 +++++
 lib/fdtdec.c     | 158 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 175 insertions(+)

diff --git a/include/fdtdec.h b/include/fdtdec.h
index 997103a87cdf..5c9108ced571 100644
--- a/include/fdtdec.h
+++ b/include/fdtdec.h
@@ -979,6 +979,23 @@ int fdtdec_get_max_phandle(const void *blob, uint32_t *maxp);
  */
 int fdtdec_set_phandle(void *blob, int node, uint32_t phandle);
 
+/**
+ * fdtdec_add_reserved_memory() - add or find a reserved-memory node
+ *
+ * If a reserved-memory node already exists for the given carveout, a phandle
+ * for that node will be returned. Otherwise a new node will be created and a
+ * phandle corresponding to it will be returned.
+ *
+ * @param blob		FDT blob
+ * @param basename	base name of the node to create
+ * @param carveout	information about the carveout region
+ * @param phandlep	return location for the phandle of the carveout region
+ * @return 0 on success or a negative error code on failure
+ */
+int fdtdec_add_reserved_memory(void *blob, const char *basename,
+			       const struct fdt_memory *carveout,
+			       uint32_t *phandlep);
+
 /**
  * Set up the device tree ready for use
  */
diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index 9195a05d1129..a8b35c144ae0 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -1287,6 +1287,164 @@ int fdtdec_set_phandle(void *blob, int node, uint32_t phandle)
 	return 0;
 }
 
+static int fdtdec_init_reserved_memory(void *blob)
+{
+	int na, ns, node, err;
+	fdt32_t value;
+
+	/* inherit #address-cells and #size-cells from the root node */
+	na = fdt_address_cells(blob, 0);
+	ns = fdt_size_cells(blob, 0);
+
+	node = fdt_add_subnode(blob, 0, "reserved-memory");
+	if (node < 0)
+		return node;
+
+	err = fdt_setprop(blob, node, "ranges", NULL, 0);
+	if (err < 0)
+		return err;
+
+	value = cpu_to_fdt32(na);
+
+	err = fdt_setprop(blob, node, "#address-cells", &value, sizeof(value));
+	if (err < 0)
+		return err;
+
+	value = cpu_to_fdt32(ns);
+
+	err = fdt_setprop(blob, node, "#size-cells", &value, sizeof(value));
+	if (err < 0)
+		return err;
+
+	return node;
+}
+
+static void fdt_addr_unpack(fdt_addr_t addr, fdt32_t *upper, fdt32_t *lower)
+{
+#ifdef CONFIG_PHYS_64BIT
+	*upper = addr >> 32;
+#else
+	*upper = 0;
+#endif
+
+	*lower = addr;
+}
+
+static void fdt_size_unpack(fdt_size_t size, fdt32_t *upper, fdt32_t *lower)
+{
+#ifdef CONFIG_PHYS_64BIT
+	*upper = size >> 32;
+#else
+	*upper = 0;
+#endif
+
+	*lower = size;
+}
+
+int fdtdec_add_reserved_memory(void *blob, const char *basename,
+			       const struct fdt_memory *carveout,
+			       uint32_t *phandlep)
+{
+	fdt32_t cells[4] = {}, *ptr = cells;
+	uint32_t upper, lower, phandle;
+	int parent, node, na, ns, err;
+	char name[64];
+
+	/* create an empty /reserved-memory node if one doesn't exist */
+	parent = fdt_path_offset(blob, "/reserved-memory");
+	if (parent < 0) {
+		parent = fdtdec_init_reserved_memory(blob);
+		if (parent < 0)
+			return parent;
+	}
+
+	/* only 1 or 2 #address-cells and #size-cells are supported */
+	na = fdt_address_cells(blob, parent);
+	if (na < 1 || na > 2)
+		return -FDT_ERR_BADNCELLS;
+
+	ns = fdt_address_cells(blob, parent);
+	if (ns < 1 || ns > 2)
+		return -FDT_ERR_BADNCELLS;
+
+	/* find a matching node and return the phandle to that */
+	fdt_for_each_subnode(node, blob, parent) {
+		const char *name = fdt_get_name(blob, node, NULL);
+		phys_addr_t addr, size;
+
+		addr = fdtdec_get_addr_size(blob, node, "reg", &size);
+		if (addr == FDT_ADDR_T_NONE) {
+			printf("failed to read address/size for %s\n", name);
+			continue;
+		}
+
+		if (addr == carveout->start && (addr + size) == carveout->end) {
+			*phandlep = fdt_get_phandle(blob, node);
+			return 0;
+		}
+	}
+
+	/*
+	 * Unpack the start address and generate the name of the new node
+	 * base on the basename and the unit-address.
+	 */
+	fdt_addr_unpack(carveout->start, &upper, &lower);
+
+	if (na > 1)
+		snprintf(name, sizeof(name), "%s@%x,%x", basename, upper,
+			 lower);
+	else {
+		if (upper) {
+			printf("address %08x:%08x exceeds addressable space\n",
+			       upper, lower);
+			return -FDT_ERR_BADVALUE;
+		}
+
+		snprintf(name, sizeof(name), "%s@%x", basename, lower);
+	}
+
+	node = fdt_add_subnode(blob, parent, name);
+	if (node < 0)
+		return node;
+
+	/*
+	 * Generate a new phandle for the reserved-memory node. Look up the
+	 * highest phandle number currently in used and use the next higher
+	 * one.
+	 */
+	err = fdtdec_get_max_phandle(blob, &phandle);
+	if (err < 0)
+		return err;
+
+	err = fdtdec_set_phandle(blob, node, phandle + 1);
+	if (err < 0)
+		return err;
+
+	/* store one or two address cells */
+	if (na > 1)
+		*ptr++ = cpu_to_fdt32(upper);
+
+	*ptr++ = cpu_to_fdt32(lower);
+
+	/* store one or two size cells */
+	fdt_size_unpack(carveout->end - carveout->start, &upper, &lower);
+
+	if (ns > 1)
+		*ptr++ = cpu_to_fdt32(upper);
+
+	*ptr++ = cpu_to_fdt32(lower);
+
+	err = fdt_setprop(blob, node, "reg", cells, ptr - cells);
+	if (err < 0)
+		return err;
+
+	/* return the phandle for the new node for the caller to use */
+	if (phandlep)
+		*phandlep = phandle + 1;
+
+	return 0;
+}
+
 int fdtdec_setup(void)
 {
 #if CONFIG_IS_ENABLED(OF_CONTROL)
-- 
2.20.1

[U-Boot] [PATCH 5/6] fdtdec: Implement carveout support functions

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

The fdtdec_get_carveout() and fdtdec_set_carveout() function can be used
to read a carveout from a given node or add a carveout to a given node
using the standard device tree bindings (involving reserved-memory nodes
and the memory-region property).

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 include/fdtdec.h | 39 ++++++++++++++++++++++
 lib/fdtdec.c     | 85 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 124 insertions(+)

diff --git a/include/fdtdec.h b/include/fdtdec.h
index 5c9108ced571..56f3cec551bb 100644
--- a/include/fdtdec.h
+++ b/include/fdtdec.h
@@ -996,6 +996,45 @@ int fdtdec_add_reserved_memory(void *blob, const char *basename,
 			       const struct fdt_memory *carveout,
 			       uint32_t *phandlep);
 
+/**
+ * fdtdec_get_carveout() - reads a carveout from an FDT
+ *
+ * Reads information about a carveout region from an FDT. The carveout is a
+ * referenced by its phandle that is read from a given property in a given
+ * node.
+ *
+ * @param blob		FDT blob
+ * @param node		name of a node
+ * @param name		name of the property in the given node that contains
+ *			the phandle for the carveout
+ * @param index		index of the phandle for which to read the carveout
+ * @param carveout	return location for the carveout information
+ * @return 0 on success or a negative error code on failure
+ */
+int fdtdec_get_carveout(const void *blob, const char *node, const char *name,
+			unsigned int index, struct fdt_memory *carveout);
+
+/**
+ * fdtdec_set_carveout() - sets a carveout region for a given node
+ *
+ * Sets a carveout region for a given node. If a reserved-memory node already
+ * exists for the carveout, the phandle for that node will be reused. If no
+ * such node exists, a new one will be created and a phandle to it stored in
+ * a specified property of the given node.
+ *
+ * @param blob		FDT blob
+ * @param node		name of the node to add the carveout to
+ * @param prop_name	name of the property in which to store the phandle of
+ *			the carveout
+ * @param index		index of the phandle to store
+ * @param name		base name of the reserved-memory node to create
+ * @param carveout	information about the carveout to add
+ * @return 0 on success or a negative error code on failure
+ */
+int fdtdec_set_carveout(void *blob, const char *node, const char *prop_name,
+			unsigned int index, const char *name,
+			const struct fdt_memory *carveout);
+
 /**
  * Set up the device tree ready for use
  */
diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index a8b35c144ae0..a6aefb336267 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -1445,6 +1445,91 @@ int fdtdec_add_reserved_memory(void *blob, const char *basename,
 	return 0;
 }
 
+int fdtdec_get_carveout(const void *blob, const char *node, const char *name,
+			unsigned int index, struct fdt_memory *carveout)
+{
+	const fdt32_t *prop;
+	uint32_t phandle;
+	int offset, len;
+
+	offset = fdt_path_offset(blob, node);
+	if (offset < 0)
+		return offset;
+
+	prop = fdt_getprop(blob, offset, name, &len);
+	if (!prop) {
+		printf("failed to get %s for %s\n", name, node);
+		return -FDT_ERR_NOTFOUND;
+	}
+
+	if ((len % sizeof(phandle)) != 0) {
+		printf("invalid phandle property\n");
+		return -FDT_ERR_BADPHANDLE;
+	}
+
+	if (len < (sizeof(phandle) * (index + 1))) {
+		printf("invalid phandle index\n");
+		return -FDT_ERR_BADPHANDLE;
+	}
+
+	phandle = fdt32_to_cpu(prop[index]);
+
+	offset = fdt_node_offset_by_phandle(blob, phandle);
+	if (offset < 0) {
+		printf("failed to find node for phandle %u\n", phandle);
+		return offset;
+	}
+
+	carveout->start = fdtdec_get_addr_size(blob, offset, "reg",
+					       &carveout->end);
+	if (carveout->start == FDT_ADDR_T_NONE) {
+		printf("failed to read address/size from \"reg\" property\n");
+		return -FDT_ERR_NOTFOUND;
+	}
+
+	carveout->end += carveout->start;
+
+	return 0;
+}
+
+int fdtdec_set_carveout(void *blob, const char *node, const char *prop_name,
+			unsigned int index, const char *name,
+			const struct fdt_memory *carveout)
+{
+	uint32_t phandle;
+	int err, offset;
+	fdt32_t value;
+
+	/* XXX implement support for multiple phandles */
+	if (index > 0) {
+		debug("invalid index %u\n", index);
+		return -FDT_ERR_BADOFFSET;
+	}
+
+	err = fdtdec_add_reserved_memory(blob, name, carveout, &phandle);
+	if (err < 0) {
+		debug("failed to add reserved memory: %d\n", err);
+		return err;
+	}
+
+	offset = fdt_path_offset(blob, node);
+	if (offset < 0) {
+		debug("failed to find offset for node %s: %d\n", node, offset);
+		return offset;
+	}
+
+	value = cpu_to_fdt32(phandle);
+
+	err = fdt_setprop(blob, offset, prop_name, &value, sizeof(value));
+	if (err < 0) {
+		debug("failed to set %s property for node %s: %d\n", prop_name,
+		      node, err);
+		return err;
+	}
+
+	return 0;
+}
+
 int fdtdec_setup(void)
 {
 #if CONFIG_IS_ENABLED(OF_CONTROL)
-- 
2.20.1

[U-Boot] [PATCH 6/6] p2371-2180: Add support for framebuffer carveouts

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

If early firmware initialized the display hardware and the display
controllers are scanning out a framebuffer (e.g. a splash screen), make
sure to pass information about the memory location of that framebuffer
to the kernel before booting to avoid the kernel from using that memory
for the buddy allocator.

This same mechanism can also be used in the kernel to set up early SMMU
mappings and avoid SMMU faults caused by the display controller reading
from memory for which it has no mapping.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 board/nvidia/p2371-2180/p2371-2180.c | 50 ++++++++++++++++++++++++++++
 configs/p2371-2180_defconfig         |  1 +
 2 files changed, 51 insertions(+)

diff --git a/board/nvidia/p2371-2180/p2371-2180.c b/board/nvidia/p2371-2180/p2371-2180.c
index 212037da5ac0..d81deb0cd320 100644
--- a/board/nvidia/p2371-2180/p2371-2180.c
+++ b/board/nvidia/p2371-2180/p2371-2180.c
@@ -6,8 +6,11 @@
 
 #include <common.h>
 #include <i2c.h>
+#include <linux/libfdt.h>
+#include <fdtdec.h>
 #include <asm/arch/gpio.h>
 #include <asm/arch/pinmux.h>
+#include <asm/arch-tegra/cboot.h>
 #include "../p2571/max77620_init.h"
 #include "pinmux-config-p2371-2180.h"
 
@@ -94,3 +97,50 @@ int tegra_pcie_board_init(void)
 	return 0;
 }
 #endif /* PCI */
+
+static int ft_copy_carveout(void *dst, const void *src, const char *node)
+{
+	struct fdt_memory fb;
+	int err;
+
+	err = fdtdec_get_carveout(src, node, "memory-region", 0, &fb);
+	if (err < 0) {
+		if (err != -FDT_ERR_NOTFOUND)
+			printf("failed to get carveout for %s: %d\n", node,
+			       err);
+
+		return err;
+	}
+
+	err = fdtdec_set_carveout(dst, node, "memory-region", 0, "framebuffer",
+				  &fb);
+	if (err < 0) {
+		printf("failed to set carveout for %s: %d\n", node, err);
+		return err;
+	}
+
+	return 0;
+}
+
+int ft_board_setup(void *fdt, bd_t *bd)
+{
+	const void *cboot_fdt = (const void *)cboot_boot_x0;
+	static const char * const nodes[] = {
+		"/host1x at 50000000/dc at 54200000",
+		"/host1x at 50000000/dc@54240000",
+	};
+	unsigned int i;
+	int err;
+
+	for (i = 0; i < ARRAY_SIZE(nodes); i++) {
+		err = ft_copy_carveout(fdt, cboot_fdt, nodes[i]);
+		if (err < 0) {
+			if (err != -FDT_ERR_NOTFOUND)
+				printf("failed to copy carveout for %s: %d\n",
+				       nodes[i], err);
+			continue;
+		}
+	}
+
+	return 0;
+}
diff --git a/configs/p2371-2180_defconfig b/configs/p2371-2180_defconfig
index b662ef143141..b66459e379ac 100644
--- a/configs/p2371-2180_defconfig
+++ b/configs/p2371-2180_defconfig
@@ -5,6 +5,7 @@ CONFIG_TEGRA210=y
 CONFIG_TARGET_P2371_2180=y
 CONFIG_NR_DRAM_BANKS=2
 CONFIG_OF_SYSTEM_SETUP=y
+CONFIG_OF_BOARD_SETUP=y
 CONFIG_CONSOLE_MUX=y
 CONFIG_SYS_STDIO_DEREGISTER=y
 CONFIG_SYS_PROMPT="Tegra210 (P2371-2180) # "
-- 
2.20.1

[U-Boot] [PATCH 1/6] fdtdec: Add cpu_to_fdt_{addr, size}() macros

[Simon Glass]

On Fri, 8 Mar 2019 at 13:11, Thierry Reding <thierry.reding@gmail.com> wrote:
>
> From: Thierry Reding <treding@nvidia.com>
>
> These macros are useful for converting the endianness of variables of
> type fdt_addr_t and fdt_size_t.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  include/fdtdec.h | 4 ++++
>  1 file changed, 4 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 2/6] fdtdec: Implement fdtdec_get_max_phandle()

[Simon Glass]

Hi Thierry,

On Fri, 8 Mar 2019 at 13:11, Thierry Reding <thierry.reding@gmail.com> wrote:
>
> From: Thierry Reding <treding@nvidia.com>
>
> This function allows looking up the highest phandle value stored in a
> device tree, which is useful to determine the next best phandle value
> for new nodes.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  include/fdtdec.h | 12 ++++++++++++
>  lib/fdtdec.c     | 28 ++++++++++++++++++++++++++++
>  2 files changed, 40 insertions(+)

Can we use fdt_get_max_phandle() instead? If not, could you please add
a bit more detail to the commit message as we might consider changing
the upstream function.

Regards,
Simon

[U-Boot] [PATCH 3/6] fdtdec: Implement fdtdec_set_phandle()

[Simon Glass]

Hi Thierry,

On Fri, 8 Mar 2019 at 13:11, Thierry Reding <thierry.reding@gmail.com> wrote:
>
> From: Thierry Reding <treding@nvidia.com>
>
> This function can be used to set a phandle for a given node.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  include/fdtdec.h | 11 +++++++++++
>  lib/fdtdec.c     | 16 ++++++++++++++++
>  2 files changed, 27 insertions(+)

This seems OK, although I think it should have a test.

But what about livetree? I think it would make more sense to add a
high-level API which can deal with livetree/flattree.

>
> diff --git a/include/fdtdec.h b/include/fdtdec.h
> index 5eb3c0c237a9..997103a87cdf 100644
> --- a/include/fdtdec.h
> +++ b/include/fdtdec.h
> @@ -968,6 +968,17 @@ int fdtdec_setup_memory_banksize(void);
>   */
>  int fdtdec_get_max_phandle(const void *blob, uint32_t *maxp);
>
> +/**
> + * fdtdec_set_phandle() - sets the phandle of a given node
> + *
> + * @param blob         FDT blob
> + * @param node         offset in the FDT blob of the node whose phandle is to
> + *                     be set
> + * @param phandle      phandle to set for the given node
> + * @return 0 on success or a negative error code on failure
> + */
> +int fdtdec_set_phandle(void *blob, int node, uint32_t phandle);
> +
>  /**
>   * Set up the device tree ready for use
>   */
> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> index f2af947c106e..9195a05d1129 100644
> --- a/lib/fdtdec.c
> +++ b/lib/fdtdec.c
> @@ -1271,6 +1271,22 @@ int fdtdec_get_max_phandle(const void *blob, uint32_t *maxp)
>         return 0;
>  }
>
> +int fdtdec_set_phandle(void *blob, int node, uint32_t phandle)
> +{
> +       fdt32_t value = cpu_to_fdt32(phandle);
> +       int err;
> +
> +       err = fdt_setprop(blob, node, "linux,phandle", &value, sizeof(value));
> +       if (err < 0)
> +               return err;

Why set both properties?

> +
> +       err = fdt_setprop(blob, node, "phandle", &value, sizeof(value));
> +       if (err < 0)
> +               return err;
> +
> +       return 0;
> +}
> +
>  int fdtdec_setup(void)
>  {
>  #if CONFIG_IS_ENABLED(OF_CONTROL)
> --
> 2.20.1
>

Regards,
SImon

[U-Boot] [PATCH 4/6] fdtdec: Implement fdtdec_add_reserved_memory()

[Simon Glass]

On Fri, 8 Mar 2019 at 13:11, Thierry Reding <thierry.reding@gmail.com> wrote:
>
> From: Thierry Reding <treding@nvidia.com>
>
> This function can be used to add subnodes in the /reserved-memory node.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  include/fdtdec.h |  17 +++++
>  lib/fdtdec.c     | 158 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 175 insertions(+)
>

I think an example would be useful, or perhaps a pointer to some docs
(perhaps DT spec?) showing how this function is used?

Reviewed-by: Simon Glass <sjg@chromium.org>



> diff --git a/include/fdtdec.h b/include/fdtdec.h
> index 997103a87cdf..5c9108ced571 100644
> --- a/include/fdtdec.h
> +++ b/include/fdtdec.h
> @@ -979,6 +979,23 @@ int fdtdec_get_max_phandle(const void *blob, uint32_t *maxp);
>   */
>  int fdtdec_set_phandle(void *blob, int node, uint32_t phandle);
>
> +/**
> + * fdtdec_add_reserved_memory() - add or find a reserved-memory node
> + *
> + * If a reserved-memory node already exists for the given carveout, a phandle
> + * for that node will be returned. Otherwise a new node will be created and a
> + * phandle corresponding to it will be returned.
> + *
> + * @param blob         FDT blob
> + * @param basename     base name of the node to create
> + * @param carveout     information about the carveout region
> + * @param phandlep     return location for the phandle of the carveout region
> + * @return 0 on success or a negative error code on failure
> + */
> +int fdtdec_add_reserved_memory(void *blob, const char *basename,
> +                              const struct fdt_memory *carveout,
> +                              uint32_t *phandlep);
> +
>  /**
>   * Set up the device tree ready for use
>   */
> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> index 9195a05d1129..a8b35c144ae0 100644
> --- a/lib/fdtdec.c
> +++ b/lib/fdtdec.c
> @@ -1287,6 +1287,164 @@ int fdtdec_set_phandle(void *blob, int node, uint32_t phandle)
>         return 0;
>  }
>
> +static int fdtdec_init_reserved_memory(void *blob)
> +{
> +       int na, ns, node, err;
> +       fdt32_t value;
> +
> +       /* inherit #address-cells and #size-cells from the root node */
> +       na = fdt_address_cells(blob, 0);
> +       ns = fdt_size_cells(blob, 0);
> +
> +       node = fdt_add_subnode(blob, 0, "reserved-memory");
> +       if (node < 0)
> +               return node;
> +
> +       err = fdt_setprop(blob, node, "ranges", NULL, 0);
> +       if (err < 0)
> +               return err;
> +
> +       value = cpu_to_fdt32(na);
> +
> +       err = fdt_setprop(blob, node, "#address-cells", &value, sizeof(value));
> +       if (err < 0)
> +               return err;
> +
> +       value = cpu_to_fdt32(ns);
> +
> +       err = fdt_setprop(blob, node, "#size-cells", &value, sizeof(value));
> +       if (err < 0)
> +               return err;
> +
> +       return node;
> +}
> +
> +static void fdt_addr_unpack(fdt_addr_t addr, fdt32_t *upper, fdt32_t *lower)
> +{
> +#ifdef CONFIG_PHYS_64BIT
> +       *upper = addr >> 32;
> +#else
> +       *upper = 0;
> +#endif
> +
> +       *lower = addr;
> +}
> +
> +static void fdt_size_unpack(fdt_size_t size, fdt32_t *upper, fdt32_t *lower)
> +{
> +#ifdef CONFIG_PHYS_64BIT
> +       *upper = size >> 32;
> +#else
> +       *upper = 0;
> +#endif
> +
> +       *lower = size;
> +}
> +
> +int fdtdec_add_reserved_memory(void *blob, const char *basename,
> +                              const struct fdt_memory *carveout,
> +                              uint32_t *phandlep)
> +{
> +       fdt32_t cells[4] = {}, *ptr = cells;
> +       uint32_t upper, lower, phandle;
> +       int parent, node, na, ns, err;
> +       char name[64];
> +
> +       /* create an empty /reserved-memory node if one doesn't exist */
> +       parent = fdt_path_offset(blob, "/reserved-memory");
> +       if (parent < 0) {
> +               parent = fdtdec_init_reserved_memory(blob);
> +               if (parent < 0)
> +                       return parent;
> +       }
> +
> +       /* only 1 or 2 #address-cells and #size-cells are supported */
> +       na = fdt_address_cells(blob, parent);
> +       if (na < 1 || na > 2)
> +               return -FDT_ERR_BADNCELLS;
> +
> +       ns = fdt_address_cells(blob, parent);
> +       if (ns < 1 || ns > 2)
> +               return -FDT_ERR_BADNCELLS;
> +
> +       /* find a matching node and return the phandle to that */
> +       fdt_for_each_subnode(node, blob, parent) {
> +               const char *name = fdt_get_name(blob, node, NULL);
> +               phys_addr_t addr, size;
> +
> +               addr = fdtdec_get_addr_size(blob, node, "reg", &size);
> +               if (addr == FDT_ADDR_T_NONE) {
> +                       printf("failed to read address/size for %s\n", name);
> +                       continue;
> +               }
> +
> +               if (addr == carveout->start && (addr + size) == carveout->end) {
> +                       *phandlep = fdt_get_phandle(blob, node);
> +                       return 0;
> +               }
> +       }
> +
> +       /*
> +        * Unpack the start address and generate the name of the new node
> +        * base on the basename and the unit-address.
> +        */
> +       fdt_addr_unpack(carveout->start, &upper, &lower);
> +
> +       if (na > 1)
> +               snprintf(name, sizeof(name), "%s@%x,%x", basename, upper,
> +                        lower);
> +       else {
> +               if (upper) {
> +                       printf("address %08x:%08x exceeds addressable space\n",
> +                              upper, lower);
> +                       return -FDT_ERR_BADVALUE;
> +               }
> +
> +               snprintf(name, sizeof(name), "%s@%x", basename, lower);
> +       }
> +
> +       node = fdt_add_subnode(blob, parent, name);
> +       if (node < 0)
> +               return node;
> +
> +       /*
> +        * Generate a new phandle for the reserved-memory node. Look up the
> +        * highest phandle number currently in used and use the next higher
> +        * one.
> +        */
> +       err = fdtdec_get_max_phandle(blob, &phandle);
> +       if (err < 0)
> +               return err;
> +
> +       err = fdtdec_set_phandle(blob, node, phandle + 1);
> +       if (err < 0)
> +               return err;
> +
> +       /* store one or two address cells */
> +       if (na > 1)
> +               *ptr++ = cpu_to_fdt32(upper);
> +
> +       *ptr++ = cpu_to_fdt32(lower);
> +
> +       /* store one or two size cells */
> +       fdt_size_unpack(carveout->end - carveout->start, &upper, &lower);
> +
> +       if (ns > 1)
> +               *ptr++ = cpu_to_fdt32(upper);
> +
> +       *ptr++ = cpu_to_fdt32(lower);
> +
> +       err = fdt_setprop(blob, node, "reg", cells, ptr - cells);
> +       if (err < 0)
> +               return err;
> +
> +       /* return the phandle for the new node for the caller to use */
> +       if (phandlep)
> +               *phandlep = phandle + 1;
> +
> +       return 0;
> +}
> +
>  int fdtdec_setup(void)
>  {
>  #if CONFIG_IS_ENABLED(OF_CONTROL)
> --
> 2.20.1
>

[U-Boot] [PATCH 5/6] fdtdec: Implement carveout support functions

[Simon Glass]

Hi Thierry,

On Fri, 8 Mar 2019 at 13:11, Thierry Reding <thierry.reding@gmail.com> wrote:
>
> From: Thierry Reding <treding@nvidia.com>
>
> The fdtdec_get_carveout() and fdtdec_set_carveout() function can be used
> to read a carveout from a given node or add a carveout to a given node
> using the standard device tree bindings (involving reserved-memory nodes
> and the memory-region property).
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  include/fdtdec.h | 39 ++++++++++++++++++++++
>  lib/fdtdec.c     | 85 ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 124 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

Again I worry a bit about adding flattree-only functions. Also, is
there a test? Finally, I think debug() is pretty than printf() for
code size.

Regards,
SImon

[U-Boot] [PATCH 2/6] fdtdec: Implement fdtdec_get_max_phandle()

[Thierry Reding]

On Sun, Mar 10, 2019 at 03:51:31PM -0600, Simon Glass wrote:
> Hi Thierry,
> 
> On Fri, 8 Mar 2019 at 13:11, Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > From: Thierry Reding <treding@nvidia.com>
> >
> > This function allows looking up the highest phandle value stored in a
> > device tree, which is useful to determine the next best phandle value
> > for new nodes.
> >
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  include/fdtdec.h | 12 ++++++++++++
> >  lib/fdtdec.c     | 28 ++++++++++++++++++++++++++++
> >  2 files changed, 40 insertions(+)
> 
> Can we use fdt_get_max_phandle() instead? If not, could you please add
> a bit more detail to the commit message as we might consider changing
> the upstream function.

fdt_get_max_phandle() has a slightly awkward signature. It returns the
phandle via the return value, which means that it can distinguish
between error conditions and also uses 0 and -1 to signal no phandle
found and error conditions, respectively. Using the function requires
weird looking code like this:

	uint32_t phandle;

	phandle = fdt_get_max_phandle(fdt);
	if (phandle == 0 || phandle == (uint32_t)-1)
		/* process error */;

So the goal here was to add an alternative that would provide a more
standard interface where a regular error could be returned via the
return value and the phandle would be stored in an output parameter on
success.

I specifically didn't want to update the upstream function because it
was introduced about 2.5 years ago and will probably be used by some
number of users. I'm not sure if there's a stable API policy for libfdt,
but I would expect a lot of users to be annoyed if we just changed the
signature of fdt_get_max_phandle().

That said, perhaps another alternative would be to implement this as a
different function. If you look at the subsequent patches, the goal is
to generate a new phandle for newly added nodes, so perhaps something
like this could work:

	int fdtdec_generate_phandle(const void *fdt, uint32_t *phandle);

That would be slightly more in line with the higher level of fdtdec
compared to libfdt.

Or perhaps you'd prefer fdt_generate_phandle() in libfdt?

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190311/412b11fe/attachment.sig>

[U-Boot] [PATCH 3/6] fdtdec: Implement fdtdec_set_phandle()

[Thierry Reding]

On Sun, Mar 10, 2019 at 03:51:40PM -0600, Simon Glass wrote:
> Hi Thierry,
> 
> On Fri, 8 Mar 2019 at 13:11, Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > From: Thierry Reding <treding@nvidia.com>
> >
> > This function can be used to set a phandle for a given node.
> >
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  include/fdtdec.h | 11 +++++++++++
> >  lib/fdtdec.c     | 16 ++++++++++++++++
> >  2 files changed, 27 insertions(+)
> 
> This seems OK, although I think it should have a test.

I'll add something to test this to the test_fdtdec command. I'm not sure
how much sense it makes to test this individually, because the test is
fairly elaborate (needs to create one node to reference and another to
reference it from), so perhaps I can just add a complete test that will
at the same time validate that the reserved-memory and carveout stuff
works?

> But what about livetree? I think it would make more sense to add a
> high-level API which can deal with livetree/flattree.

I'm not sure it really makes sense to add this kind of information to a
livetree. The only use-case for this that I have is to convey
information about carveouts to the kernel, so by this information is
added to a DTB that is loaded from external storage along with the
kernel and then modified right before the DTB is passed to the kernel on
boot.

The only case where I think such functionality would be useful in a live
tree is if we construct the live tree in U-Boot, update it and then pass
it to the kernel upon boot. That's not something that works today, and I
can't test any of that, so I'm not sure it makes much sense to implement
it now.

> > diff --git a/include/fdtdec.h b/include/fdtdec.h
> > index 5eb3c0c237a9..997103a87cdf 100644
> > --- a/include/fdtdec.h
> > +++ b/include/fdtdec.h
> > @@ -968,6 +968,17 @@ int fdtdec_setup_memory_banksize(void);
> >   */
> >  int fdtdec_get_max_phandle(const void *blob, uint32_t *maxp);
> >
> > +/**
> > + * fdtdec_set_phandle() - sets the phandle of a given node
> > + *
> > + * @param blob         FDT blob
> > + * @param node         offset in the FDT blob of the node whose phandle is to
> > + *                     be set
> > + * @param phandle      phandle to set for the given node
> > + * @return 0 on success or a negative error code on failure
> > + */
> > +int fdtdec_set_phandle(void *blob, int node, uint32_t phandle);
> > +
> >  /**
> >   * Set up the device tree ready for use
> >   */
> > diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> > index f2af947c106e..9195a05d1129 100644
> > --- a/lib/fdtdec.c
> > +++ b/lib/fdtdec.c
> > @@ -1271,6 +1271,22 @@ int fdtdec_get_max_phandle(const void *blob, uint32_t *maxp)
> >         return 0;
> >  }
> >
> > +int fdtdec_set_phandle(void *blob, int node, uint32_t phandle)
> > +{
> > +       fdt32_t value = cpu_to_fdt32(phandle);
> > +       int err;
> > +
> > +       err = fdt_setprop(blob, node, "linux,phandle", &value, sizeof(value));
> > +       if (err < 0)
> > +               return err;
> 
> Why set both properties?

I was trying to mimic the output of DTC, but looking again it seems like
it doesn't even produce linux,phandle properties. Doing some research it
was changed to only produce "phandle" properties in v1.4.5 and the
commit message says:

	commit 0016f8c2aa32423f680ec6e94a00f1095b81b5fc
	Author: Rob Herring <robh@kernel.org>
	Date:   Wed Jul 12 17:20:30 2017 -0500

	    dtc: change default phandles to ePAPR style instead of both

	    Currently, both legacy (linux,phandle) and ePAPR (phandle) properties
	    are inserted into dtbs by default. The newer ePAPR style has been
	    supported in dtc and Linux kernel for 7 years. That should be a long
	    enough transition period. We can save a little space by not putting both
	    into the dtb.

	    Signed-off-by: Rob Herring <robh@kernel.org>
	    Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

So perhaps we no longer need to generate this from U-Boot either. I'll
remove the linux,phandle code.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190311/ff2d1fc4/attachment.sig>

[U-Boot] [PATCH 4/6] fdtdec: Implement fdtdec_add_reserved_memory()

[Thierry Reding]

On Sun, Mar 10, 2019 at 03:51:42PM -0600, Simon Glass wrote:
> On Fri, 8 Mar 2019 at 13:11, Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > From: Thierry Reding <treding@nvidia.com>
> >
> > This function can be used to add subnodes in the /reserved-memory node.
> >
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  include/fdtdec.h |  17 +++++
> >  lib/fdtdec.c     | 158 +++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 175 insertions(+)
> >
> 
> I think an example would be useful, or perhaps a pointer to some docs
> (perhaps DT spec?) showing how this function is used?

Yeah, I can add a pointer to the DT bindings. Do you want me to add a
copy of the DT bindings to the U-Boot source tree, or is it sufficient
to refer to the docs in Linux?

As for an example, patches 5 and 6 show how this should be used. Do you
want an additional example in the comment, or what did you have in mind?

Thierry

> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> 
> 
> > diff --git a/include/fdtdec.h b/include/fdtdec.h
> > index 997103a87cdf..5c9108ced571 100644
> > --- a/include/fdtdec.h
> > +++ b/include/fdtdec.h
> > @@ -979,6 +979,23 @@ int fdtdec_get_max_phandle(const void *blob, uint32_t *maxp);
> >   */
> >  int fdtdec_set_phandle(void *blob, int node, uint32_t phandle);
> >
> > +/**
> > + * fdtdec_add_reserved_memory() - add or find a reserved-memory node
> > + *
> > + * If a reserved-memory node already exists for the given carveout, a phandle
> > + * for that node will be returned. Otherwise a new node will be created and a
> > + * phandle corresponding to it will be returned.
> > + *
> > + * @param blob         FDT blob
> > + * @param basename     base name of the node to create
> > + * @param carveout     information about the carveout region
> > + * @param phandlep     return location for the phandle of the carveout region
> > + * @return 0 on success or a negative error code on failure
> > + */
> > +int fdtdec_add_reserved_memory(void *blob, const char *basename,
> > +                              const struct fdt_memory *carveout,
> > +                              uint32_t *phandlep);
> > +
> >  /**
> >   * Set up the device tree ready for use
> >   */
> > diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> > index 9195a05d1129..a8b35c144ae0 100644
> > --- a/lib/fdtdec.c
> > +++ b/lib/fdtdec.c
> > @@ -1287,6 +1287,164 @@ int fdtdec_set_phandle(void *blob, int node, uint32_t phandle)
> >         return 0;
> >  }
> >
> > +static int fdtdec_init_reserved_memory(void *blob)
> > +{
> > +       int na, ns, node, err;
> > +       fdt32_t value;
> > +
> > +       /* inherit #address-cells and #size-cells from the root node */
> > +       na = fdt_address_cells(blob, 0);
> > +       ns = fdt_size_cells(blob, 0);
> > +
> > +       node = fdt_add_subnode(blob, 0, "reserved-memory");
> > +       if (node < 0)
> > +               return node;
> > +
> > +       err = fdt_setprop(blob, node, "ranges", NULL, 0);
> > +       if (err < 0)
> > +               return err;
> > +
> > +       value = cpu_to_fdt32(na);
> > +
> > +       err = fdt_setprop(blob, node, "#address-cells", &value, sizeof(value));
> > +       if (err < 0)
> > +               return err;
> > +
> > +       value = cpu_to_fdt32(ns);
> > +
> > +       err = fdt_setprop(blob, node, "#size-cells", &value, sizeof(value));
> > +       if (err < 0)
> > +               return err;
> > +
> > +       return node;
> > +}
> > +
> > +static void fdt_addr_unpack(fdt_addr_t addr, fdt32_t *upper, fdt32_t *lower)
> > +{
> > +#ifdef CONFIG_PHYS_64BIT
> > +       *upper = addr >> 32;
> > +#else
> > +       *upper = 0;
> > +#endif
> > +
> > +       *lower = addr;
> > +}
> > +
> > +static void fdt_size_unpack(fdt_size_t size, fdt32_t *upper, fdt32_t *lower)
> > +{
> > +#ifdef CONFIG_PHYS_64BIT
> > +       *upper = size >> 32;
> > +#else
> > +       *upper = 0;
> > +#endif
> > +
> > +       *lower = size;
> > +}
> > +
> > +int fdtdec_add_reserved_memory(void *blob, const char *basename,
> > +                              const struct fdt_memory *carveout,
> > +                              uint32_t *phandlep)
> > +{
> > +       fdt32_t cells[4] = {}, *ptr = cells;
> > +       uint32_t upper, lower, phandle;
> > +       int parent, node, na, ns, err;
> > +       char name[64];
> > +
> > +       /* create an empty /reserved-memory node if one doesn't exist */
> > +       parent = fdt_path_offset(blob, "/reserved-memory");
> > +       if (parent < 0) {
> > +               parent = fdtdec_init_reserved_memory(blob);
> > +               if (parent < 0)
> > +                       return parent;
> > +       }
> > +
> > +       /* only 1 or 2 #address-cells and #size-cells are supported */
> > +       na = fdt_address_cells(blob, parent);
> > +       if (na < 1 || na > 2)
> > +               return -FDT_ERR_BADNCELLS;
> > +
> > +       ns = fdt_address_cells(blob, parent);
> > +       if (ns < 1 || ns > 2)
> > +               return -FDT_ERR_BADNCELLS;
> > +
> > +       /* find a matching node and return the phandle to that */
> > +       fdt_for_each_subnode(node, blob, parent) {
> > +               const char *name = fdt_get_name(blob, node, NULL);
> > +               phys_addr_t addr, size;
> > +
> > +               addr = fdtdec_get_addr_size(blob, node, "reg", &size);
> > +               if (addr == FDT_ADDR_T_NONE) {
> > +                       printf("failed to read address/size for %s\n", name);
> > +                       continue;
> > +               }
> > +
> > +               if (addr == carveout->start && (addr + size) == carveout->end) {
> > +                       *phandlep = fdt_get_phandle(blob, node);
> > +                       return 0;
> > +               }
> > +       }
> > +
> > +       /*
> > +        * Unpack the start address and generate the name of the new node
> > +        * base on the basename and the unit-address.
> > +        */
> > +       fdt_addr_unpack(carveout->start, &upper, &lower);
> > +
> > +       if (na > 1)
> > +               snprintf(name, sizeof(name), "%s@%x,%x", basename, upper,
> > +                        lower);
> > +       else {
> > +               if (upper) {
> > +                       printf("address %08x:%08x exceeds addressable space\n",
> > +                              upper, lower);
> > +                       return -FDT_ERR_BADVALUE;
> > +               }
> > +
> > +               snprintf(name, sizeof(name), "%s@%x", basename, lower);
> > +       }
> > +
> > +       node = fdt_add_subnode(blob, parent, name);
> > +       if (node < 0)
> > +               return node;
> > +
> > +       /*
> > +        * Generate a new phandle for the reserved-memory node. Look up the
> > +        * highest phandle number currently in used and use the next higher
> > +        * one.
> > +        */
> > +       err = fdtdec_get_max_phandle(blob, &phandle);
> > +       if (err < 0)
> > +               return err;
> > +
> > +       err = fdtdec_set_phandle(blob, node, phandle + 1);
> > +       if (err < 0)
> > +               return err;
> > +
> > +       /* store one or two address cells */
> > +       if (na > 1)
> > +               *ptr++ = cpu_to_fdt32(upper);
> > +
> > +       *ptr++ = cpu_to_fdt32(lower);
> > +
> > +       /* store one or two size cells */
> > +       fdt_size_unpack(carveout->end - carveout->start, &upper, &lower);
> > +
> > +       if (ns > 1)
> > +               *ptr++ = cpu_to_fdt32(upper);
> > +
> > +       *ptr++ = cpu_to_fdt32(lower);
> > +
> > +       err = fdt_setprop(blob, node, "reg", cells, ptr - cells);
> > +       if (err < 0)
> > +               return err;
> > +
> > +       /* return the phandle for the new node for the caller to use */
> > +       if (phandlep)
> > +               *phandlep = phandle + 1;
> > +
> > +       return 0;
> > +}
> > +
> >  int fdtdec_setup(void)
> >  {
> >  #if CONFIG_IS_ENABLED(OF_CONTROL)
> > --
> > 2.20.1
> >
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190311/5b5ed06d/attachment.sig>

[U-Boot] [PATCH 2/6] fdtdec: Implement fdtdec_get_max_phandle()

[Simon Glass]

Hi Thierry,

On Mon, 11 Mar 2019 at 17:27, Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Sun, Mar 10, 2019 at 03:51:31PM -0600, Simon Glass wrote:
> > Hi Thierry,
> >
> > On Fri, 8 Mar 2019 at 13:11, Thierry Reding <thierry.reding@gmail.com> wrote:
> > >
> > > From: Thierry Reding <treding@nvidia.com>
> > >
> > > This function allows looking up the highest phandle value stored in a
> > > device tree, which is useful to determine the next best phandle value
> > > for new nodes.
> > >
> > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > ---
> > >  include/fdtdec.h | 12 ++++++++++++
> > >  lib/fdtdec.c     | 28 ++++++++++++++++++++++++++++
> > >  2 files changed, 40 insertions(+)
> >
> > Can we use fdt_get_max_phandle() instead? If not, could you please add
> > a bit more detail to the commit message as we might consider changing
> > the upstream function.
>
> fdt_get_max_phandle() has a slightly awkward signature. It returns the
> phandle via the return value, which means that it can distinguish
> between error conditions and also uses 0 and -1 to signal no phandle
> found and error conditions, respectively. Using the function requires
> weird looking code like this:
>
>         uint32_t phandle;
>
>         phandle = fdt_get_max_phandle(fdt);
>         if (phandle == 0 || phandle == (uint32_t)-1)
>                 /* process error */;
>
> So the goal here was to add an alternative that would provide a more
> standard interface where a regular error could be returned via the
> return value and the phandle would be stored in an output parameter on
> success.
>
> I specifically didn't want to update the upstream function because it
> was introduced about 2.5 years ago and will probably be used by some
> number of users. I'm not sure if there's a stable API policy for libfdt,
> but I would expect a lot of users to be annoyed if we just changed the
> signature of fdt_get_max_phandle().
>
> That said, perhaps another alternative would be to implement this as a
> different function. If you look at the subsequent patches, the goal is
> to generate a new phandle for newly added nodes, so perhaps something
> like this could work:
>
>         int fdtdec_generate_phandle(const void *fdt, uint32_t *phandle);

That seems like a good idea.
>
> That would be slightly more in line with the higher level of fdtdec
> compared to libfdt.
>
> Or perhaps you'd prefer fdt_generate_phandle() in libfdt?

Well yes, then David weighs in and we avoid a blind alley which won't
pass muster upstream. If you do send an upstream patch, make sure to
add tests.

Regards,
Simon

[U-Boot] [PATCH 6/6] p2371-2180: Add support for framebuffer carveouts

[Simon Glass]

On Sat, 9 Mar 2019 at 04:11, Thierry Reding <thierry.reding@gmail.com> wrote:
>
> From: Thierry Reding <treding@nvidia.com>
>
> If early firmware initialized the display hardware and the display
> controllers are scanning out a framebuffer (e.g. a splash screen), make
> sure to pass information about the memory location of that framebuffer
> to the kernel before booting to avoid the kernel from using that memory
> for the buddy allocator.
>
> This same mechanism can also be used in the kernel to set up early SMMU
> mappings and avoid SMMU faults caused by the display controller reading
> from memory for which it has no mapping.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  board/nvidia/p2371-2180/p2371-2180.c | 50 ++++++++++++++++++++++++++++
>  configs/p2371-2180_defconfig         |  1 +
>  2 files changed, 51 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 3/6] fdtdec: Implement fdtdec_set_phandle()

[Simon Glass]

Hi Thierry,

On Mon, 11 Mar 2019 at 18:04, Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Sun, Mar 10, 2019 at 03:51:40PM -0600, Simon Glass wrote:
> > Hi Thierry,
> >
> > On Fri, 8 Mar 2019 at 13:11, Thierry Reding <thierry.reding@gmail.com> wrote:
> > >
> > > From: Thierry Reding <treding@nvidia.com>
> > >
> > > This function can be used to set a phandle for a given node.
> > >
> > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > ---
> > >  include/fdtdec.h | 11 +++++++++++
> > >  lib/fdtdec.c     | 16 ++++++++++++++++
> > >  2 files changed, 27 insertions(+)
> >
> > This seems OK, although I think it should have a test.
>
> I'll add something to test this to the test_fdtdec command. I'm not sure
> how much sense it makes to test this individually, because the test is
> fairly elaborate (needs to create one node to reference and another to
> reference it from), so perhaps I can just add a complete test that will
> at the same time validate that the reserved-memory and carveout stuff
> works?
>
> > But what about livetree? I think it would make more sense to add a
> > high-level API which can deal with livetree/flattree.
>
> I'm not sure it really makes sense to add this kind of information to a
> livetree. The only use-case for this that I have is to convey
> information about carveouts to the kernel, so by this information is
> added to a DTB that is loaded from external storage along with the
> kernel and then modified right before the DTB is passed to the kernel on
> boot.
>
> The only case where I think such functionality would be useful in a live
> tree is if we construct the live tree in U-Boot, update it and then pass
> it to the kernel upon boot. That's not something that works today, and I
> can't test any of that, so I'm not sure it makes much sense to implement
> it now.
>
> > > diff --git a/include/fdtdec.h b/include/fdtdec.h
> > > index 5eb3c0c237a9..997103a87cdf 100644
> > > --- a/include/fdtdec.h
> > > +++ b/include/fdtdec.h
> > > @@ -968,6 +968,17 @@ int fdtdec_setup_memory_banksize(void);
> > >   */
> > >  int fdtdec_get_max_phandle(const void *blob, uint32_t *maxp);
> > >
> > > +/**
> > > + * fdtdec_set_phandle() - sets the phandle of a given node
> > > + *
> > > + * @param blob         FDT blob
> > > + * @param node         offset in the FDT blob of the node whose phandle is to
> > > + *                     be set
> > > + * @param phandle      phandle to set for the given node
> > > + * @return 0 on success or a negative error code on failure
> > > + */
> > > +int fdtdec_set_phandle(void *blob, int node, uint32_t phandle);
> > > +
> > >  /**
> > >   * Set up the device tree ready for use
> > >   */
> > > diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> > > index f2af947c106e..9195a05d1129 100644
> > > --- a/lib/fdtdec.c
> > > +++ b/lib/fdtdec.c
> > > @@ -1271,6 +1271,22 @@ int fdtdec_get_max_phandle(const void *blob, uint32_t *maxp)
> > >         return 0;
> > >  }
> > >
> > > +int fdtdec_set_phandle(void *blob, int node, uint32_t phandle)
> > > +{
> > > +       fdt32_t value = cpu_to_fdt32(phandle);
> > > +       int err;
> > > +
> > > +       err = fdt_setprop(blob, node, "linux,phandle", &value, sizeof(value));
> > > +       if (err < 0)
> > > +               return err;
> >
> > Why set both properties?
>
> I was trying to mimic the output of DTC, but looking again it seems like
> it doesn't even produce linux,phandle properties. Doing some research it
> was changed to only produce "phandle" properties in v1.4.5 and the
> commit message says:
>
>         commit 0016f8c2aa32423f680ec6e94a00f1095b81b5fc
>         Author: Rob Herring <robh@kernel.org>
>         Date:   Wed Jul 12 17:20:30 2017 -0500
>
>             dtc: change default phandles to ePAPR style instead of both
>
>             Currently, both legacy (linux,phandle) and ePAPR (phandle) properties
>             are inserted into dtbs by default. The newer ePAPR style has been
>             supported in dtc and Linux kernel for 7 years. That should be a long
>             enough transition period. We can save a little space by not putting both
>             into the dtb.
>
>             Signed-off-by: Rob Herring <robh@kernel.org>
>             Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>
> So perhaps we no longer need to generate this from U-Boot either. I'll
> remove the linux,phandle code.
>

OK this all sounds good to me.

Regards,
Simon

[U-Boot] [PATCH 4/6] fdtdec: Implement fdtdec_add_reserved_memory()

[Simon Glass]

Hi Thierry,

On Mon, 11 Mar 2019 at 18:06, Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Sun, Mar 10, 2019 at 03:51:42PM -0600, Simon Glass wrote:
> > On Fri, 8 Mar 2019 at 13:11, Thierry Reding <thierry.reding@gmail.com> wrote:
> > >
> > > From: Thierry Reding <treding@nvidia.com>
> > >
> > > This function can be used to add subnodes in the /reserved-memory node.
> > >
> > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > ---
> > >  include/fdtdec.h |  17 +++++
> > >  lib/fdtdec.c     | 158 +++++++++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 175 insertions(+)
> > >
> >
> > I think an example would be useful, or perhaps a pointer to some docs
> > (perhaps DT spec?) showing how this function is used?
>
> Yeah, I can add a pointer to the DT bindings. Do you want me to add a
> copy of the DT bindings to the U-Boot source tree, or is it sufficient
> to refer to the docs in Linux?

We should have a copy.

>
> As for an example, patches 5 and 6 show how this should be used. Do you
> want an additional example in the comment, or what did you have in mind?

I was thinking of having an example of a node created by this
function, in the header-file comment.

Regards,
Simon