[U-Boot,RESEND,2/2] cmd: fdt: add fdt overlay application subcommand

The device tree overlays are a good way to deal with user-modifyable
boards or boards with some kind of an expansion mechanism where we can
easily plug new board in (like the BBB or the raspberry pi).

However, so far, the usual mechanism to deal with it was to have in Linux
some driver detecting the expansion boards plugged in and then request
these overlays using the firmware interface.

That works in most cases, but in some cases, you might want to have the
overlays applied before the userspace comes in. Either because the new
board requires some kind of an early initialization, or because your root
filesystem is accessed through that expansion board.

The easiest solution in such a case is to simply have the component before
Linux applying that overlay, removing all these drawbacks.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 cmd/Makefile          |   2 +-
 cmd/fdt.c             |  19 +++
 cmd/fdt_overlay.c     | 464 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/fdt_support.h |   2 +-
 4 files changed, 485 insertions(+), 2 deletions(-)
 create mode 100644 cmd/fdt_overlay.c

Hi Maxime,

> On Apr 4, 2016, at 11:25 , Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> 
> The device tree overlays are a good way to deal with user-modifyable
> boards or boards with some kind of an expansion mechanism where we can
> easily plug new board in (like the BBB or the raspberry pi).
> 
> However, so far, the usual mechanism to deal with it was to have in Linux
> some driver detecting the expansion boards plugged in and then request
> these overlays using the firmware interface.
> 
> That works in most cases, but in some cases, you might want to have the
> overlays applied before the userspace comes in. Either because the new
> board requires some kind of an early initialization, or because your root
> filesystem is accessed through that expansion board.
> 
> The easiest solution in such a case is to simply have the component before
> Linux applying that overlay, removing all these drawbacks.
> 

This is an interesting patch. My comments inline.

> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
> cmd/Makefile          |   2 +-
> cmd/fdt.c             |  19 +++
> cmd/fdt_overlay.c     | 464 ++++++++++++++++++++++++++++++++++++++++++++++++++
> include/fdt_support.h |   2 +-
> 4 files changed, 485 insertions(+), 2 deletions(-)
> create mode 100644 cmd/fdt_overlay.c
> 
> diff --git a/cmd/Makefile b/cmd/Makefile
> index 03f7e0a21d2f..09f993971d93 100644
> --- a/cmd/Makefile
> +++ b/cmd/Makefile
> @@ -52,7 +52,7 @@ obj-$(CONFIG_CMD_EXT4) += ext4.o
> obj-$(CONFIG_CMD_EXT2) += ext2.o
> obj-$(CONFIG_CMD_FAT) += fat.o
> obj-$(CONFIG_CMD_FDC) += fdc.o
> -obj-$(CONFIG_OF_LIBFDT) += fdt.o
> +obj-$(CONFIG_OF_LIBFDT) += fdt.o fdt_overlay.o
> obj-$(CONFIG_CMD_FITUPD) += fitupd.o
> obj-$(CONFIG_CMD_FLASH) += flash.o
> ifdef CONFIG_FPGA
> diff --git a/cmd/fdt.c b/cmd/fdt.c
> index ca6c707dfb48..c875ba688d3b 100644
> --- a/cmd/fdt.c
> +++ b/cmd/fdt.c
> @@ -639,6 +639,24 @@ static int do_fdt(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> #endif
> 
> 	}
> +	/* apply an overlay */
> +	else if (strncmp(argv[1], "ap", 2) == 0) {
> +		unsigned long addr;
> +		struct fdt_header *blob;
> +
> +		if (argc != 3)
> +			return CMD_RET_USAGE;
> +
> +		if (!working_fdt)
> +			return CMD_RET_FAILURE;
> +
> +		addr = simple_strtoul(argv[2], NULL, 16);
> +		blob = map_sysmem(addr, 0);
> +		if (!fdt_valid(&blob))
> +			return CMD_RET_FAILURE;
> +
> +		fdt_overlay_apply(working_fdt, blob);
> +	}
> 	/* resize the fdt */
> 	else if (strncmp(argv[1], "re", 2) == 0) {
> 		fdt_shrink_to_minimum(working_fdt);
> @@ -1025,6 +1043,7 @@ static int fdt_print(const char *pathp, char *prop, int depth)
> #ifdef CONFIG_SYS_LONGHELP
> static char fdt_help_text[] =
> 	"addr [-c]  <addr> [<length>]   - Set the [control] fdt location to <addr>\n"
> +	"fdt apply <addr>                    - Apply overlay to the DT\n"
> #ifdef CONFIG_OF_BOARD_SETUP
> 	"fdt boardsetup                      - Do board-specific set up\n"
> #endif
> diff --git a/cmd/fdt_overlay.c b/cmd/fdt_overlay.c

This looks it’s general libfdt code.
It should end up in libfdt/ so that others can use it, and eventually
be pushed upstream.

> new file mode 100644
> index 000000000000..d2784d791a09
> --- /dev/null
> +++ b/cmd/fdt_overlay.c
> @@ -0,0 +1,464 @@
> +#include <common.h>
> +#include <malloc.h>
> +#include <libfdt.h>
> +
> +#define fdt_for_each_property(fdt, node, property)		\
> +	for (property = fdt_first_property_offset(fdt, node);	\
> +	     property >= 0;					\
> +	     property = fdt_next_property_offset(fdt, property))
> +
> +struct fdt_overlay_fixup {
> +	char	label[64];
> +	char	name[64];
> +	char	path[64];
^ I understand why you’re using fixed character arrays but there is no
guarantee that the sizes are going to be large enough.
The path is especially worrisome.

Since you’re going to dynamically allocate the fixup, make them pointers;

char *label, *name, *path;

> +	int	index;
> +};
> +
> +static int fdt_get_max_phandle(const void *dt)
> +{
> +	int offset, phandle = 0, new_phandle;
> +

phandle is a uint32_t or u32. Since this is libfdt uint32_t.


> +	for (offset = fdt_next_node(dt, -1, NULL); offset >= 0;
> +	     offset = fdt_next_node(dt, offset, NULL)) {
> +		new_phandle = fdt_get_phandle(dt, offset);
> +		if (new_phandle > phandle)
> +			phandle = new_phandle;
> +	}
> +
> +	return phandle;
> +}
> +
> +static uint32_t fdt_overlay_get_target_phandle(const void *dt, int node)
> +{
> +	const uint32_t *val;
> +	int len;
> +
> +	val = fdt_getprop(dt, node, "target", &len);
> +	if (!val || (len != sizeof(*val)))
> +		return 0;
> +
> +	return fdt32_to_cpu(*val);
> +}
> +
> +static int fdt_overlay_get_target(const void *dt, const void *dto, int fragment)
> +{
> +	uint32_t phandle;
> +	const char *path;
> +
> +	/* Try first to do a phandle based lookup */
> +	phandle = fdt_overlay_get_target_phandle(dto, fragment);
> +	if (phandle)
> +		return fdt_node_offset_by_phandle(dt, phandle);
> +
> +	/* And then a path based lookup */
> +	path = fdt_getprop(dto, fragment, "target-path", NULL);
> +	if (!path)
> +		return -FDT_ERR_NOTFOUND;
> +
> +	return fdt_path_offset(dt, path);
> +}

There are going to be more target options here FYI. For now you’re OK.

> +
> +static int fdt_overlay_adjust_node_phandles(void *dto, int node,
> +					    uint32_t delta)
> +{
> +	int property;
> +	int child;
> +
> +	fdt_for_each_property(dto, node, property) {
> +		const uint32_t *val;
> +		const char *name;
> +		uint32_t adj_val;
> +		int len;
> +		int ret;
> +
> +		val = fdt_getprop_by_offset(dto, property,
> +					    &name, &len);
> +		if (!val || (len != 4))
> +			continue;
> +

sizeof(uint32_t)

> +		if (strcmp(name, "phandle") && strcmp(name, "linux,phandle"))
> +			continue;
> +
> +		adj_val = fdt32_to_cpu(*val);
> +		adj_val += delta;
> +		ret = fdt_setprop_inplace_u32(dto, node, name, adj_val);
> +		if (ret) {
> +			printf("Failed to ajust property %s phandle\n", name);
> +			return ret;
> +		}
> +	}
> +
> +	fdt_for_each_subnode(dto, child, node)
> +		fdt_overlay_adjust_node_phandles(dto, child, delta);
> +
> +	return 0;
> +}
> +
> +static int fdt_overlay_adjust_local_phandles(void *overlay, uint32_t delta)
> +{
> +	int root;
> +
> +	root = fdt_path_offset(overlay, "/");
> +	if (root < 0) {
> +		printf("Couldn't locate the root of the overlay\n");
> +		return root;
> +	}
> +
> +	return fdt_overlay_adjust_node_phandles(overlay, root, delta);
> +}
> +
> +static int _fdt_overlay_update_local_references(void *dto,
> +						int tree_node,
> +						int fixup_node,
> +						uint32_t delta)
> +{
> +	int fixup_prop;
> +	int fixup_child;
> +
> +	fdt_for_each_property(dto, fixup_node, fixup_prop) {
> +		const char *fixup_name, *tree_name;
> +		const uint32_t *val;
> +		uint32_t adj_val;
> +		int fixup_len;
> +		int tree_prop;
> +		int ret;
> +
> +		fdt_getprop_by_offset(dto, fixup_prop,
> +				      &fixup_name, &fixup_len);
> +
> +		if (!strcmp(fixup_name, "phandle") ||
> +		    !strcmp(fixup_name, "linux,phandle"))
> +			continue;
> +
> +		if (fixup_len != 4) {
> +			printf("Illegal property size (%d) %s\n",
> +			       fixup_len, fixup_name);
> +			return -FDT_ERR_NOTFOUND;
> +		}
> +
> +		fdt_for_each_property(dto, tree_node, tree_prop) {
> +			int tree_len;
> +
> +			val = fdt_getprop_by_offset(dto, tree_prop,
> +						    &tree_name, &tree_len);
> +
> +			if (!strcmp(tree_name, fixup_name))
> +				break;
> +		}
> +
> +		if (!tree_name) {
> +			printf("Couldn't find target property %s\n",
> +			       fixup_name);
> +			return -FDT_ERR_NOTFOUND;
> +		}
> +
> +		adj_val = fdt32_to_cpu(*val);
> +		adj_val += delta;
> +		ret = fdt_setprop_inplace_u32(dto, tree_node, fixup_name,
> +					      adj_val);
> +		if (ret) {
> +			printf("Failed to ajust property %s phandle\n",
> +			       fixup_name);
> +			return ret;
> +		}
> +	}
> +
> +	fdt_for_each_subnode(dto, fixup_child, fixup_node) {
> +		const char *fixup_child_name = fdt_get_name(dto,
> +							    fixup_child, NULL);
> +		int tree_child;
> +
> +		fdt_for_each_subnode(dto, tree_child, tree_node) {
> +			const char *tree_child_name = fdt_get_name(dto,
> +								   tree_child,
> +								   NULL);
> +
> +			if (!strcmp(fixup_child_name, tree_child_name))
> +				break;
> +		}
> +
> +		_fdt_overlay_update_local_references(dto,
> +						     tree_child,
> +						     fixup_child,
> +						     delta);
> +	}
> +
> +	return 0;
> +}
> +
> +static int fdt_overlay_update_local_references(void *dto, uint32_t delta)
> +{
> +	int root, fixups;
> +
> +	root = fdt_path_offset(dto, "/");
> +	if (root < 0) {
> +		printf("Couldn't locate the root of the overlay\n");
> +		return root;
> +	}
> +
> +	fixups = fdt_path_offset(dto, "/__local_fixups__");
> +	if (root < 0) {
> +		printf("Couldn't locate the local fixups in the overlay\n");
> +		return root;
> +	}
> +
> +	return _fdt_overlay_update_local_references(dto, root, fixups,
> +						    delta);
> +}
> +
> +static struct fdt_overlay_fixup *fdt_fixups_parse_property(void *dto,
> +							   int property,
> +							   int *number)
> +{
> +	struct fdt_overlay_fixup *fixups;
> +	const char *value, *tmp_value;
> +	const char *name;
> +	int tmp_len, len, next;
> +	int table = 0;
> +	int i;
> +
> +	value = fdt_getprop_by_offset(dto, property,
> +				      &name, &len);
> +	tmp_value = value;
> +	tmp_len = len;
> +
> +	do {
> +		next = strlen(tmp_value) + 1;
> +		tmp_len -= next;
> +		tmp_value += next;
> +		table++;
> +	} while (tmp_len > 0);
> +
> +	fixups = malloc(table * sizeof(*fixups));
> +	if (!fixups)
> +		return NULL;
> +

Make a list instead of table here and return that.

> +	for (i = 0; i < table; i++) {
> +		struct fdt_overlay_fixup *fixup = fixups + i;
> +		const char *prop_string = value;
> +		char *sep;
> +		int stlen;
> +
> +

*fixup = malloc(sizeof(*fixup) + strlen(path) + 1 + strlen(name) + 1 + strlen(label) + 1

point the path, name, label pointers to the buffer space.

> 		stlen = strlen(prop_string);
> +
> +		sep = strchr(prop_string, ':');
> +		strncpy(fixup->path, prop_string, sep - prop_string);
> +		fixup->path[sep - prop_string] = '\0';
> +		prop_string = sep + 1;
> +
> +		sep = strchr(prop_string, ':');
> +		strncpy(fixup->name, prop_string, sep - prop_string);
> +		fixup->name[sep - prop_string] = '\0';
> +		prop_string = sep + 1;
> +
> +		fixup->index = simple_strtol(prop_string, NULL, 10);
> +		strncpy(fixup->label, name, 64);
> +
> +		value += stlen + 1;
> +		len -= stlen + 1;
> +	}
> +
> +	*number = table;
> +	return fixups;
> +}
> +
> +static int fdt_overlay_fixup_phandles(void *dt, void *dto)
> +{
> +	int fixups_off, symbols_off;
> +	int property;
> +
> +	symbols_off = fdt_path_offset(dt, "/__symbols__");
> +	fixups_off = fdt_path_offset(dto, "/__fixups__");
> +
> +	fdt_for_each_property(dto, fixups_off, property) {
> +		struct fdt_overlay_fixup *fixups;
> +		int n_fixups;
> +		int i;
> +
> +		fixups = fdt_fixups_parse_property(dto, property, &n_fixups);
> +		if (!fixups || n_fixups == 0)
> +			continue;
> +

^ list iteration now
> +		for (i = 0; i < n_fixups; i++) {
> +			struct fdt_overlay_fixup *fixup = fixups + i;
> +			const uint32_t *prop_val;
> +			const char *symbol_path;
> +			uint32_t *fixup_val;
> +			uint32_t phandle;
> +			int symbol_off, fixup_off;
> +			int prop_len;
> +			int ret;
> +
> +			symbol_path = fdt_getprop(dt, symbols_off, fixup->label,
> +						  &prop_len);
> +			if (!symbol_path) {
> +				printf("Couldn't lookup symbol %s in the main DT... Skipping\n",
> +				       fixup->label);
> +				continue;
> +			}
> +
> +			symbol_off = fdt_path_offset(dt, symbol_path);
> +			if (symbol_off < 0) {
> +				printf("Couldn't match the symbol %s to node %s... Skipping\n",
> +				       fixup->label, symbol_path);
> +				continue;
> +			}
> +
> +			phandle = fdt_get_phandle(dt, symbol_off);
> +			if (!phandle) {
> +				printf("Symbol node %s has no phandle... Skipping\n",
> +				       symbol_path);
> +				continue;
> +			}
> +
> +			fixup_off = fdt_path_offset(dto, fixup->path);
> +			if (fixup_off < 0) {
> +				printf("Invalid overlay node %s to fixup... Skipping\n",
> +				       fixup->path);
> +				continue;
> +			}
> +
> +			prop_val = fdt_getprop(dto, fixup_off, fixup->name,
> +					       &prop_len);
> +			if (!prop_val) {
> +				printf("Couldn't retrieve property %s/%s value... Skipping\n",
> +				       fixup->path, fixup->name);
> +				continue;
> +			}
> +
> +			fixup_val = malloc(prop_len);
> +			if (!fixup_val)
> +				return -FDT_ERR_INTERNAL;
> +			memcpy(fixup_val, prop_val, prop_len);
> +
> +			if (fdt32_to_cpu(fixup_val[fixup->index]) != 0xdeadbeef) {
> +				printf("Value pointed (0x%x) is not supposed to be fixed up... Skipping\n",
> +				       fdt32_to_cpu(fixup_val[fixup->index]));
> +				continue;
> +			}
> +
> +			fixup_val[fixup->index] = cpu_to_fdt32(phandle);
> +			ret = fdt_setprop_inplace(dto, fixup_off,
> +						  fixup->name, fixup_val,
> +						  prop_len);
> +			if (ret) {
> +				printf("Couldn't fixup phandle in overlay property %s/%s (%d)... Skipping\n",
> +				       fixup->path, fixup->name, ret);
> +			}
> +		}
> +
> +		free(fixups);
> +	}
> +
> +	return 0;
> +}
> +
> +static int fdt_apply_overlay_node(void *dt, void *dto,
> +				  int target, int overlay)
> +{
> +	int property;
> +	int node;
> +
> +	fdt_for_each_property(dto, overlay, property) {
> +		const char *name;
> +		const void *prop;
> +		int prop_len;
> +		int ret;
> +
> +		prop = fdt_getprop_by_offset(dto, property, &name,
> +					     &prop_len);
> +		if (!prop)
> +			return -FDT_ERR_INTERNAL;
> +
> +		ret = fdt_setprop(dt, target, name, prop, prop_len);
> +		if (ret) {
> +			printf("Couldn't set property %s\n", name);
> +			return ret;
> +		}
> +	}
> +
> +	fdt_for_each_subnode(dto, node, overlay) {
> +		const char *name = fdt_get_name(dto, node, NULL);
> +		int nnode;
> +		int ret;
> +
> +		nnode = fdt_add_subnode(dt, target, name);
> +		if (nnode < 0) {
> +			printf("Couldn't add subnode %s (%d)\n", name, nnode);
> +			return nnode;
> +		}
> +
> +		ret = fdt_apply_overlay_node(dt, dto, nnode, node);
> +		if (ret) {
> +			printf("Couldn't apply sub-overlay (%d)\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int fdt_overlay_merge(void *dt, void *dto)
> +{
> +	int root, fragment;
> +
> +	root = fdt_path_offset(dto, "/");
> +	if (root < 0) {
> +		printf("Couldn't locate the root of our overlay\n");
> +		return root;
> +	}
> +
> +	fdt_for_each_subnode(dto, fragment, root) {
> +		const char *name = fdt_get_name(dto, fragment, NULL);
> +		uint32_t target;
> +		int overlay;
> +		int ret;
> +
> +		if (strncmp(name, "fragment", 8))
> +			continue;
> +
> +		target = fdt_overlay_get_target(dt, dto, fragment);
> +		if (target < 0) {
> +			printf("Couldn't locate %s target\n", name);
> +			return target;
> +		}
> +
> +		overlay = fdt_subnode_offset(dto, fragment, "__overlay__");
> +		if (overlay < 0) {
> +			printf("Couldn't locate %s overlay\n", name);
> +			return overlay;
> +		}
> +
> +		ret = fdt_apply_overlay_node(dt, dto, target, overlay);
> +		if (ret) {
> +			printf("Couldn't apply %s overlay\n", name);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +int fdt_overlay_apply(void *dt, void *dto)
> +{
> +	uint32_t delta = fdt_get_max_phandle(dt) + 1;
> +	int ret;
> +
> +	ret = fdt_overlay_adjust_local_phandles(dto, delta);
> +	if (ret) {
> +		printf("Couldn't adjust local phandles\n");
> +		return ret;
> +	}
> +
> +	ret = fdt_overlay_update_local_references(dto, delta);
> +	if (ret) {
> +		printf("Couldn't update our local references\n");
> +		return ret;
> +	}
> +
> +	ret = fdt_overlay_fixup_phandles(dt, dto);
> +	if (ret) {
> +		printf("Couldn't resolve the global phandles\n");
> +		return ret;
> +	}
> +
> +	return fdt_overlay_merge(dt, dto);
> +}
> diff --git a/include/fdt_support.h b/include/fdt_support.h
> index 296add01f34f..b4184a767e28 100644
> --- a/include/fdt_support.h
> +++ b/include/fdt_support.h
> @@ -240,7 +240,7 @@ int arch_fixup_memory_node(void *blob);
> 
> int fdt_setup_simplefb_node(void *fdt, int node, u64 base_address, u32 width,
> 			    u32 height, u32 stride, const char *format);
> -
> +int fdt_overlay_apply(void *fdt, void *overlay);
> #endif /* ifdef CONFIG_OF_LIBFDT */
> 
> #ifdef USE_HOSTCC
> -- 
> 2.8.0
> 

In general looks good. Nice job.

Regards

— Pantelis

On Tue, Apr 5, 2016 at 5:03 PM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> Hi Maxime,
>
>> On Apr 4, 2016, at 11:25 , Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
>>
>> The device tree overlays are a good way to deal with user-modifyable
>> boards or boards with some kind of an expansion mechanism where we can
>> easily plug new board in (like the BBB or the raspberry pi).
>>
>> However, so far, the usual mechanism to deal with it was to have in Linux
>> some driver detecting the expansion boards plugged in and then request
>> these overlays using the firmware interface.
>>
>> That works in most cases, but in some cases, you might want to have the
>> overlays applied before the userspace comes in. Either because the new
>> board requires some kind of an early initialization, or because your root
>> filesystem is accessed through that expansion board.
>>
>> The easiest solution in such a case is to simply have the component before
>> Linux applying that overlay, removing all these drawbacks.
>>

[...]

>> diff --git a/cmd/fdt_overlay.c b/cmd/fdt_overlay.c
>
> This looks it’s general libfdt code.
> It should end up in libfdt/ so that others can use it, and eventually
> be pushed upstream.

+1. It really needs to go into libfdt first to avoid any re-licensing issues.

Another option which Grant has suggested would be to extend the FDT
format to include overlays as a whole. Then the kernel can apply them
during unflattening. This would simplify things on the bootloader
side.

Rob

Hi Maxime,

On 4 April 2016 at 12:25, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> The device tree overlays are a good way to deal with user-modifyable
> boards or boards with some kind of an expansion mechanism where we can
> easily plug new board in (like the BBB or the raspberry pi).
>
> However, so far, the usual mechanism to deal with it was to have in Linux
> some driver detecting the expansion boards plugged in and then request
> these overlays using the firmware interface.
>
> That works in most cases, but in some cases, you might want to have the
> overlays applied before the userspace comes in. Either because the new
> board requires some kind of an early initialization, or because your root
> filesystem is accessed through that expansion board.
>
> The easiest solution in such a case is to simply have the component before
> Linux applying that overlay, removing all these drawbacks.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  cmd/Makefile          |   2 +-
>  cmd/fdt.c             |  19 +++
>  cmd/fdt_overlay.c     | 464 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/fdt_support.h |   2 +-
>  4 files changed, 485 insertions(+), 2 deletions(-)
>  create mode 100644 cmd/fdt_overlay.c

I'm happy to take this into the U-Boot tree while you try to get it
applied upstream, but please confirm that you will do this and by
when. I suggest you sent an email referring to this patch.

Also fdt_overlay.c should go in lib/libfdt/. If this functionality is
rejected for libfdt (as was fdtgrep) then we'll move it to lib/.

Finally, please take a look at adding a test for this, with some
sample files. See test/py.

>
> diff --git a/cmd/Makefile b/cmd/Makefile
> index 03f7e0a21d2f..09f993971d93 100644
> --- a/cmd/Makefile
> +++ b/cmd/Makefile
> @@ -52,7 +52,7 @@ obj-$(CONFIG_CMD_EXT4) += ext4.o
>  obj-$(CONFIG_CMD_EXT2) += ext2.o
>  obj-$(CONFIG_CMD_FAT) += fat.o
>  obj-$(CONFIG_CMD_FDC) += fdc.o
> -obj-$(CONFIG_OF_LIBFDT) += fdt.o
> +obj-$(CONFIG_OF_LIBFDT) += fdt.o fdt_overlay.o
>  obj-$(CONFIG_CMD_FITUPD) += fitupd.o
>  obj-$(CONFIG_CMD_FLASH) += flash.o
>  ifdef CONFIG_FPGA
> diff --git a/cmd/fdt.c b/cmd/fdt.c
> index ca6c707dfb48..c875ba688d3b 100644
> --- a/cmd/fdt.c
> +++ b/cmd/fdt.c
> @@ -639,6 +639,24 @@ static int do_fdt(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  #endif
>
>         }
> +       /* apply an overlay */
> +       else if (strncmp(argv[1], "ap", 2) == 0) {
> +               unsigned long addr;
> +               struct fdt_header *blob;
> +
> +               if (argc != 3)
> +                       return CMD_RET_USAGE;
> +
> +               if (!working_fdt)
> +                       return CMD_RET_FAILURE;
> +
> +               addr = simple_strtoul(argv[2], NULL, 16);
> +               blob = map_sysmem(addr, 0);
> +               if (!fdt_valid(&blob))
> +                       return CMD_RET_FAILURE;
> +
> +               fdt_overlay_apply(working_fdt, blob);
> +       }
>         /* resize the fdt */
>         else if (strncmp(argv[1], "re", 2) == 0) {
>                 fdt_shrink_to_minimum(working_fdt);
> @@ -1025,6 +1043,7 @@ static int fdt_print(const char *pathp, char *prop, int depth)
>  #ifdef CONFIG_SYS_LONGHELP
>  static char fdt_help_text[] =
>         "addr [-c]  <addr> [<length>]   - Set the [control] fdt location to <addr>\n"
> +       "fdt apply <addr>                    - Apply overlay to the DT\n"
>  #ifdef CONFIG_OF_BOARD_SETUP
>         "fdt boardsetup                      - Do board-specific set up\n"
>  #endif
> diff --git a/cmd/fdt_overlay.c b/cmd/fdt_overlay.c
> new file mode 100644
> index 000000000000..d2784d791a09
> --- /dev/null
> +++ b/cmd/fdt_overlay.c
> @@ -0,0 +1,464 @@
> +#include <common.h>
> +#include <malloc.h>
> +#include <libfdt.h>
> +
> +#define fdt_for_each_property(fdt, node, property)             \
> +       for (property = fdt_first_property_offset(fdt, node);   \
> +            property >= 0;                                     \
> +            property = fdt_next_property_offset(fdt, property))
> +
> +struct fdt_overlay_fixup {
> +       char    label[64];
> +       char    name[64];
> +       char    path[64];
> +       int     index;
> +};
> +
> +static int fdt_get_max_phandle(const void *dt)
> +{
> +       int offset, phandle = 0, new_phandle;
> +
> +       for (offset = fdt_next_node(dt, -1, NULL); offset >= 0;
> +            offset = fdt_next_node(dt, offset, NULL)) {
> +               new_phandle = fdt_get_phandle(dt, offset);
> +               if (new_phandle > phandle)
> +                       phandle = new_phandle;
> +       }
> +
> +       return phandle;
> +}
> +
> +static uint32_t fdt_overlay_get_target_phandle(const void *dt, int node)
> +{
> +       const uint32_t *val;
> +       int len;
> +
> +       val = fdt_getprop(dt, node, "target", &len);
> +       if (!val || (len != sizeof(*val)))
> +               return 0;
> +
> +       return fdt32_to_cpu(*val);
> +}
> +
> +static int fdt_overlay_get_target(const void *dt, const void *dto, int fragment)

Please add function comment here and for others.

> +{
> +       uint32_t phandle;
> +       const char *path;
> +
> +       /* Try first to do a phandle based lookup */
> +       phandle = fdt_overlay_get_target_phandle(dto, fragment);
> +       if (phandle)
> +               return fdt_node_offset_by_phandle(dt, phandle);
> +
> +       /* And then a path based lookup */
> +       path = fdt_getprop(dto, fragment, "target-path", NULL);
> +       if (!path)
> +               return -FDT_ERR_NOTFOUND;
> +
> +       return fdt_path_offset(dt, path);
> +}
> +
> +static int fdt_overlay_adjust_node_phandles(void *dto, int node,
> +                                           uint32_t delta)
> +{
> +       int property;
> +       int child;
> +
> +       fdt_for_each_property(dto, node, property) {
> +               const uint32_t *val;
> +               const char *name;
> +               uint32_t adj_val;
> +               int len;
> +               int ret;
> +
> +               val = fdt_getprop_by_offset(dto, property,
> +                                           &name, &len);

Are you sure you are using 80 cols?

> +               if (!val || (len != 4))
> +                       continue;
> +
> +               if (strcmp(name, "phandle") && strcmp(name, "linux,phandle"))
> +                       continue;
> +
> +               adj_val = fdt32_to_cpu(*val);
> +               adj_val += delta;
> +               ret = fdt_setprop_inplace_u32(dto, node, name, adj_val);
> +               if (ret) {
> +                       printf("Failed to ajust property %s phandle\n", name);
> +                       return ret;
> +               }
> +       }
> +
> +       fdt_for_each_subnode(dto, child, node)
> +               fdt_overlay_adjust_node_phandles(dto, child, delta);
> +
> +       return 0;
> +}
> +
> +static int fdt_overlay_adjust_local_phandles(void *overlay, uint32_t delta)
> +{
> +       int root;
> +
> +       root = fdt_path_offset(overlay, "/");
> +       if (root < 0) {
> +               printf("Couldn't locate the root of the overlay\n");
> +               return root;
> +       }
> +
> +       return fdt_overlay_adjust_node_phandles(overlay, root, delta);
> +}
> +
> +static int _fdt_overlay_update_local_references(void *dto,
> +                                               int tree_node,
> +                                               int fixup_node,
> +                                               uint32_t delta)
> +{
> +       int fixup_prop;
> +       int fixup_child;
> +
> +       fdt_for_each_property(dto, fixup_node, fixup_prop) {
> +               const char *fixup_name, *tree_name;
> +               const uint32_t *val;
> +               uint32_t adj_val;
> +               int fixup_len;
> +               int tree_prop;
> +               int ret;
> +
> +               fdt_getprop_by_offset(dto, fixup_prop,
> +                                     &fixup_name, &fixup_len);
> +
> +               if (!strcmp(fixup_name, "phandle") ||
> +                   !strcmp(fixup_name, "linux,phandle"))
> +                       continue;
> +
> +               if (fixup_len != 4) {
> +                       printf("Illegal property size (%d) %s\n",
> +                              fixup_len, fixup_name);
> +                       return -FDT_ERR_NOTFOUND;
> +               }
> +
> +               fdt_for_each_property(dto, tree_node, tree_prop) {
> +                       int tree_len;
> +
> +                       val = fdt_getprop_by_offset(dto, tree_prop,
> +                                                   &tree_name, &tree_len);
> +
> +                       if (!strcmp(tree_name, fixup_name))
> +                               break;
> +               }
> +
> +               if (!tree_name) {
> +                       printf("Couldn't find target property %s\n",
> +                              fixup_name);
> +                       return -FDT_ERR_NOTFOUND;
> +               }
> +
> +               adj_val = fdt32_to_cpu(*val);
> +               adj_val += delta;
> +               ret = fdt_setprop_inplace_u32(dto, tree_node, fixup_name,
> +                                             adj_val);
> +               if (ret) {
> +                       printf("Failed to ajust property %s phandle\n",
> +                              fixup_name);
> +                       return ret;
> +               }
> +       }
> +
> +       fdt_for_each_subnode(dto, fixup_child, fixup_node) {
> +               const char *fixup_child_name = fdt_get_name(dto,
> +                                                           fixup_child, NULL);
> +               int tree_child;
> +
> +               fdt_for_each_subnode(dto, tree_child, tree_node) {
> +                       const char *tree_child_name = fdt_get_name(dto,
> +                                                                  tree_child,
> +                                                                  NULL);
> +
> +                       if (!strcmp(fixup_child_name, tree_child_name))
> +                               break;
> +               }
> +
> +               _fdt_overlay_update_local_references(dto,
> +                                                    tree_child,
> +                                                    fixup_child,
> +                                                    delta);
> +       }
> +
> +       return 0;
> +}
> +
> +static int fdt_overlay_update_local_references(void *dto, uint32_t delta)
> +{
> +       int root, fixups;
> +
> +       root = fdt_path_offset(dto, "/");
> +       if (root < 0) {
> +               printf("Couldn't locate the root of the overlay\n");
> +               return root;
> +       }
> +
> +       fixups = fdt_path_offset(dto, "/__local_fixups__");
> +       if (root < 0) {
> +               printf("Couldn't locate the local fixups in the overlay\n");
> +               return root;
> +       }
> +
> +       return _fdt_overlay_update_local_references(dto, root, fixups,
> +                                                   delta);
> +}
> +
> +static struct fdt_overlay_fixup *fdt_fixups_parse_property(void *dto,
> +                                                          int property,
> +                                                          int *number)

Can this return an error instead? It seems like failure due to
malloc() would be silent.

> +{
> +       struct fdt_overlay_fixup *fixups;
> +       const char *value, *tmp_value;
> +       const char *name;
> +       int tmp_len, len, next;
> +       int table = 0;
> +       int i;
> +
> +       value = fdt_getprop_by_offset(dto, property,
> +                                     &name, &len);
> +       tmp_value = value;
> +       tmp_len = len;
> +
> +       do {
> +               next = strlen(tmp_value) + 1;
> +               tmp_len -= next;
> +               tmp_value += next;
> +               table++;
> +       } while (tmp_len > 0);
> +
> +       fixups = malloc(table * sizeof(*fixups));
> +       if (!fixups)
> +               return NULL;
> +
> +       for (i = 0; i < table; i++) {
> +               struct fdt_overlay_fixup *fixup = fixups + i;
> +               const char *prop_string = value;
> +               char *sep;
> +               int stlen;
> +
> +               stlen = strlen(prop_string);
> +
> +               sep = strchr(prop_string, ':');
> +               strncpy(fixup->path, prop_string, sep - prop_string);
> +               fixup->path[sep - prop_string] = '\0';
> +               prop_string = sep + 1;
> +
> +               sep = strchr(prop_string, ':');
> +               strncpy(fixup->name, prop_string, sep - prop_string);
> +               fixup->name[sep - prop_string] = '\0';
> +               prop_string = sep + 1;
> +
> +               fixup->index = simple_strtol(prop_string, NULL, 10);
> +               strncpy(fixup->label, name, 64);
> +
> +               value += stlen + 1;
> +               len -= stlen + 1;
> +       }
> +
> +       *number = table;
> +       return fixups;
> +}
> +
> +static int fdt_overlay_fixup_phandles(void *dt, void *dto)
> +{
> +       int fixups_off, symbols_off;
> +       int property;
> +
> +       symbols_off = fdt_path_offset(dt, "/__symbols__");
> +       fixups_off = fdt_path_offset(dto, "/__fixups__");
> +
> +       fdt_for_each_property(dto, fixups_off, property) {
> +               struct fdt_overlay_fixup *fixups;
> +               int n_fixups;
> +               int i;
> +
> +               fixups = fdt_fixups_parse_property(dto, property, &n_fixups);
> +               if (!fixups || n_fixups == 0)
> +                       continue;
> +
> +               for (i = 0; i < n_fixups; i++) {
> +                       struct fdt_overlay_fixup *fixup = fixups + i;
> +                       const uint32_t *prop_val;
> +                       const char *symbol_path;
> +                       uint32_t *fixup_val;
> +                       uint32_t phandle;
> +                       int symbol_off, fixup_off;
> +                       int prop_len;
> +                       int ret;
> +
> +                       symbol_path = fdt_getprop(dt, symbols_off, fixup->label,
> +                                                 &prop_len);
> +                       if (!symbol_path) {
> +                               printf("Couldn't lookup symbol %s in the main DT... Skipping\n",
> +                                      fixup->label);
> +                               continue;
> +                       }
> +
> +                       symbol_off = fdt_path_offset(dt, symbol_path);
> +                       if (symbol_off < 0) {
> +                               printf("Couldn't match the symbol %s to node %s... Skipping\n",
> +                                      fixup->label, symbol_path);
> +                               continue;
> +                       }
> +
> +                       phandle = fdt_get_phandle(dt, symbol_off);
> +                       if (!phandle) {
> +                               printf("Symbol node %s has no phandle... Skipping\n",
> +                                      symbol_path);
> +                               continue;
> +                       }
> +
> +                       fixup_off = fdt_path_offset(dto, fixup->path);
> +                       if (fixup_off < 0) {
> +                               printf("Invalid overlay node %s to fixup... Skipping\n",
> +                                      fixup->path);
> +                               continue;
> +                       }
> +
> +                       prop_val = fdt_getprop(dto, fixup_off, fixup->name,
> +                                              &prop_len);
> +                       if (!prop_val) {
> +                               printf("Couldn't retrieve property %s/%s value... Skipping\n",
> +                                      fixup->path, fixup->name);
> +                               continue;
> +                       }
> +
> +                       fixup_val = malloc(prop_len);

Does this get freed?

> +                       if (!fixup_val)
> +                               return -FDT_ERR_INTERNAL;
> +                       memcpy(fixup_val, prop_val, prop_len);
> +
> +                       if (fdt32_to_cpu(fixup_val[fixup->index]) != 0xdeadbeef) {
> +                               printf("Value pointed (0x%x) is not supposed to be fixed up... Skipping\n",
> +                                      fdt32_to_cpu(fixup_val[fixup->index]));

How would you know what this error refers to? Do you need to print the
node path also?

> +                               continue;
> +                       }
> +
> +                       fixup_val[fixup->index] = cpu_to_fdt32(phandle);
> +                       ret = fdt_setprop_inplace(dto, fixup_off,
> +                                                 fixup->name, fixup_val,
> +                                                 prop_len);
> +                       if (ret) {
> +                               printf("Couldn't fixup phandle in overlay property %s/%s (%d)... Skipping\n",
> +                                      fixup->path, fixup->name, ret);
> +                       }
> +               }
> +
> +               free(fixups);
> +       }
> +
> +       return 0;
> +}
> +
> +static int fdt_apply_overlay_node(void *dt, void *dto,
> +                                 int target, int overlay)
> +{
> +       int property;
> +       int node;
> +
> +       fdt_for_each_property(dto, overlay, property) {
> +               const char *name;
> +               const void *prop;
> +               int prop_len;
> +               int ret;
> +
> +               prop = fdt_getprop_by_offset(dto, property, &name,
> +                                            &prop_len);
> +               if (!prop)
> +                       return -FDT_ERR_INTERNAL;
> +
> +               ret = fdt_setprop(dt, target, name, prop, prop_len);
> +               if (ret) {
> +                       printf("Couldn't set property %s\n", name);
> +                       return ret;
> +               }
> +       }
> +
> +       fdt_for_each_subnode(dto, node, overlay) {
> +               const char *name = fdt_get_name(dto, node, NULL);
> +               int nnode;
> +               int ret;
> +
> +               nnode = fdt_add_subnode(dt, target, name);
> +               if (nnode < 0) {
> +                       printf("Couldn't add subnode %s (%d)\n", name, nnode);
> +                       return nnode;
> +               }
> +
> +               ret = fdt_apply_overlay_node(dt, dto, nnode, node);
> +               if (ret) {
> +                       printf("Couldn't apply sub-overlay (%d)\n", ret);
> +                       return ret;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
> +static int fdt_overlay_merge(void *dt, void *dto)
> +{
> +       int root, fragment;
> +
> +       root = fdt_path_offset(dto, "/");
> +       if (root < 0) {
> +               printf("Couldn't locate the root of our overlay\n");
> +               return root;
> +       }
> +
> +       fdt_for_each_subnode(dto, fragment, root) {
> +               const char *name = fdt_get_name(dto, fragment, NULL);
> +               uint32_t target;
> +               int overlay;
> +               int ret;
> +
> +               if (strncmp(name, "fragment", 8))
> +                       continue;
> +
> +               target = fdt_overlay_get_target(dt, dto, fragment);
> +               if (target < 0) {
> +                       printf("Couldn't locate %s target\n", name);
> +                       return target;
> +               }
> +
> +               overlay = fdt_subnode_offset(dto, fragment, "__overlay__");
> +               if (overlay < 0) {
> +                       printf("Couldn't locate %s overlay\n", name);
> +                       return overlay;
> +               }
> +
> +               ret = fdt_apply_overlay_node(dt, dto, target, overlay);
> +               if (ret) {
> +                       printf("Couldn't apply %s overlay\n", name);
> +                       return ret;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
> +int fdt_overlay_apply(void *dt, void *dto)
> +{
> +       uint32_t delta = fdt_get_max_phandle(dt) + 1;
> +       int ret;
> +
> +       ret = fdt_overlay_adjust_local_phandles(dto, delta);
> +       if (ret) {
> +               printf("Couldn't adjust local phandles\n");
> +               return ret;
> +       }
> +
> +       ret = fdt_overlay_update_local_references(dto, delta);
> +       if (ret) {
> +               printf("Couldn't update our local references\n");
> +               return ret;
> +       }
> +
> +       ret = fdt_overlay_fixup_phandles(dt, dto);
> +       if (ret) {
> +               printf("Couldn't resolve the global phandles\n");
> +               return ret;
> +       }
> +
> +       return fdt_overlay_merge(dt, dto);
> +}
> diff --git a/include/fdt_support.h b/include/fdt_support.h
> index 296add01f34f..b4184a767e28 100644
> --- a/include/fdt_support.h
> +++ b/include/fdt_support.h
> @@ -240,7 +240,7 @@ int arch_fixup_memory_node(void *blob);
>
>  int fdt_setup_simplefb_node(void *fdt, int node, u64 base_address, u32 width,
>                             u32 height, u32 stride, const char *format);
> -
> +int fdt_overlay_apply(void *fdt, void *overlay);

Function comment here.

>  #endif /* ifdef CONFIG_OF_LIBFDT */
>
>  #ifdef USE_HOSTCC
> --
> 2.8.0
>

Regards,
Simon

On Fri, Apr 08, 2016 at 04:29:40PM -0500, Rob Herring wrote:
> On Tue, Apr 5, 2016 at 5:03 PM, Pantelis Antoniou
> <pantelis.antoniou@konsulko.com> wrote:
> > Hi Maxime,
> >
> >> On Apr 4, 2016, at 11:25 , Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> >>
> >> The device tree overlays are a good way to deal with user-modifyable
> >> boards or boards with some kind of an expansion mechanism where we can
> >> easily plug new board in (like the BBB or the raspberry pi).
> >>
> >> However, so far, the usual mechanism to deal with it was to have in Linux
> >> some driver detecting the expansion boards plugged in and then request
> >> these overlays using the firmware interface.
> >>
> >> That works in most cases, but in some cases, you might want to have the
> >> overlays applied before the userspace comes in. Either because the new
> >> board requires some kind of an early initialization, or because your root
> >> filesystem is accessed through that expansion board.
> >>
> >> The easiest solution in such a case is to simply have the component before
> >> Linux applying that overlay, removing all these drawbacks.
> >>
> 
> [...]
> 
> >> diff --git a/cmd/fdt_overlay.c b/cmd/fdt_overlay.c
> >
> > This looks it’s general libfdt code.
> > It should end up in libfdt/ so that others can use it, and eventually
> > be pushed upstream.
> 
> +1. It really needs to go into libfdt first to avoid any re-licensing issues.
> 
> Another option which Grant has suggested would be to extend the FDT
> format to include overlays as a whole. Then the kernel can apply them
> during unflattening. This would simplify things on the bootloader
> side.

Perhaps in some cases?  Part of the overall use case here is that
something has to dynamically grab the N overlays that apply on the
current hardware and make them available.  So the bootloader would still
need to grab them and pass along to the kernel to apply.  And I've
already received some pushback on saying it could wait until
initrd/initramfs.

Hi Tom,

> On Apr 13, 2016, at 22:42 , Tom Rini <trini@konsulko.com> wrote:
> 
> On Fri, Apr 08, 2016 at 04:29:40PM -0500, Rob Herring wrote:
>> On Tue, Apr 5, 2016 at 5:03 PM, Pantelis Antoniou
>> <pantelis.antoniou@konsulko.com> wrote:
>>> Hi Maxime,
>>> 
>>>> On Apr 4, 2016, at 11:25 , Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
>>>> 
>>>> The device tree overlays are a good way to deal with user-modifyable
>>>> boards or boards with some kind of an expansion mechanism where we can
>>>> easily plug new board in (like the BBB or the raspberry pi).
>>>> 
>>>> However, so far, the usual mechanism to deal with it was to have in Linux
>>>> some driver detecting the expansion boards plugged in and then request
>>>> these overlays using the firmware interface.
>>>> 
>>>> That works in most cases, but in some cases, you might want to have the
>>>> overlays applied before the userspace comes in. Either because the new
>>>> board requires some kind of an early initialization, or because your root
>>>> filesystem is accessed through that expansion board.
>>>> 
>>>> The easiest solution in such a case is to simply have the component before
>>>> Linux applying that overlay, removing all these drawbacks.
>>>> 
>> 
>> [...]
>> 
>>>> diff --git a/cmd/fdt_overlay.c b/cmd/fdt_overlay.c
>>> 
>>> This looks it’s general libfdt code.
>>> It should end up in libfdt/ so that others can use it, and eventually
>>> be pushed upstream.
>> 
>> +1. It really needs to go into libfdt first to avoid any re-licensing issues.
>> 
>> Another option which Grant has suggested would be to extend the FDT
>> format to include overlays as a whole. Then the kernel can apply them
>> during unflattening. This would simplify things on the bootloader
>> side.
> 
> Perhaps in some cases?  Part of the overall use case here is that
> something has to dynamically grab the N overlays that apply on the
> current hardware and make them available.  So the bootloader would still
> need to grab them and pass along to the kernel to apply.  And I've
> already received some pushback on saying it could wait until
> initrd/initramfs.
> 

There are a number of options about what to do.

This patch is made for a specific case of DRM drivers not handling overlays.
I expect this is because the DRM device core does not have a reconfig notifier.
I plan adding it when I get my CHIP setup.

Grant’s option is to have the kernel do the overlay application by appending
the overlay dtbos after the base tree blob. 

This patch is orthogonal to both, so let’s review and make up our minds on the
updated patch.

> -- 
> Tom

Regards

— Pantelis

Hi Maxime,

On 4 April 2016 at 12:25, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> The device tree overlays are a good way to deal with user-modifyable
> boards or boards with some kind of an expansion mechanism where we can
> easily plug new board in (like the BBB or the raspberry pi).
>
> However, so far, the usual mechanism to deal with it was to have in Linux
> some driver detecting the expansion boards plugged in and then request
> these overlays using the firmware interface.
>
> That works in most cases, but in some cases, you might want to have the
> overlays applied before the userspace comes in. Either because the new
> board requires some kind of an early initialization, or because your root
> filesystem is accessed through that expansion board.
>
> The easiest solution in such a case is to simply have the component before
> Linux applying that overlay, removing all these drawbacks.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Will there be a new version of this patch?

> ---
>  cmd/Makefile          |   2 +-
>  cmd/fdt.c             |  19 +++
>  cmd/fdt_overlay.c     | 464 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/fdt_support.h |   2 +-
>  4 files changed, 485 insertions(+), 2 deletions(-)
>  create mode 100644 cmd/fdt_overlay.c

Regards,
Simon

Hi Simon,

On Sun, May 01, 2016 at 12:55:07PM -0600, Simon Glass wrote:
> Hi Maxime,
> 
> On 4 April 2016 at 12:25, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > The device tree overlays are a good way to deal with user-modifyable
> > boards or boards with some kind of an expansion mechanism where we can
> > easily plug new board in (like the BBB or the raspberry pi).
> >
> > However, so far, the usual mechanism to deal with it was to have in Linux
> > some driver detecting the expansion boards plugged in and then request
> > these overlays using the firmware interface.
> >
> > That works in most cases, but in some cases, you might want to have the
> > overlays applied before the userspace comes in. Either because the new
> > board requires some kind of an early initialization, or because your root
> > filesystem is accessed through that expansion board.
> >
> > The easiest solution in such a case is to simply have the component before
> > Linux applying that overlay, removing all these drawbacks.
> >
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> 
> Will there be a new version of this patch?

Yeah, sorry, I haven't had the time to address all the comments you
made, but there will definitely be a second version addressing the
comments Pantelis, Rob, Stefan and you made.

Probably not before a couple of weeks though :/

Maxime

Hi Simon,

I'm finally taking the time to address the reviews that have been made
here.

On Sat, Apr 09, 2016 at 12:40:14PM -0600, Simon Glass wrote:
> Hi Maxime,
> 
> On 4 April 2016 at 12:25, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > The device tree overlays are a good way to deal with user-modifyable
> > boards or boards with some kind of an expansion mechanism where we can
> > easily plug new board in (like the BBB or the raspberry pi).
> >
> > However, so far, the usual mechanism to deal with it was to have in Linux
> > some driver detecting the expansion boards plugged in and then request
> > these overlays using the firmware interface.
> >
> > That works in most cases, but in some cases, you might want to have the
> > overlays applied before the userspace comes in. Either because the new
> > board requires some kind of an early initialization, or because your root
> > filesystem is accessed through that expansion board.
> >
> > The easiest solution in such a case is to simply have the component before
> > Linux applying that overlay, removing all these drawbacks.
> >
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > ---
> >  cmd/Makefile          |   2 +-
> >  cmd/fdt.c             |  19 +++
> >  cmd/fdt_overlay.c     | 464 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/fdt_support.h |   2 +-
> >  4 files changed, 485 insertions(+), 2 deletions(-)
> >  create mode 100644 cmd/fdt_overlay.c
> 
> I'm happy to take this into the U-Boot tree while you try to get it
> applied upstream, but please confirm that you will do this and by
> when. I suggest you sent an email referring to this patch.
> 
> Also fdt_overlay.c should go in lib/libfdt/. If this functionality is
> rejected for libfdt (as was fdtgrep) then we'll move it to lib/.

I'm definitely ok to send it to libfdt if that's what you meant by
upstream. I'll push it at the same time I'm pushing the next version
of these patches, just to make sure we progress as fast as we can on
this.

> Finally, please take a look at adding a test for this, with some
> sample files. See test/py.

However, that will be a bit difficult. In order to be used, the DT
overlays require the overlay support in dtc, which is still not
included upstream. So the majority of the users of U-Boot won't have
that support, meaning that these tests will likely fail for them (even
though the code might be correct), resulting in a lot of false
negative.

How do you want me to proceed?

Thanks!
Maxime

Hi Maxime,

On 10 May 2016 at 05:45, Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> Hi Simon,
>
> I'm finally taking the time to address the reviews that have been made
> here.
>
> On Sat, Apr 09, 2016 at 12:40:14PM -0600, Simon Glass wrote:
>> Hi Maxime,
>>
>> On 4 April 2016 at 12:25, Maxime Ripard
>> <maxime.ripard@free-electrons.com> wrote:
>> > The device tree overlays are a good way to deal with user-modifyable
>> > boards or boards with some kind of an expansion mechanism where we can
>> > easily plug new board in (like the BBB or the raspberry pi).
>> >
>> > However, so far, the usual mechanism to deal with it was to have in Linux
>> > some driver detecting the expansion boards plugged in and then request
>> > these overlays using the firmware interface.
>> >
>> > That works in most cases, but in some cases, you might want to have the
>> > overlays applied before the userspace comes in. Either because the new
>> > board requires some kind of an early initialization, or because your root
>> > filesystem is accessed through that expansion board.
>> >
>> > The easiest solution in such a case is to simply have the component before
>> > Linux applying that overlay, removing all these drawbacks.
>> >
>> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>> > ---
>> >  cmd/Makefile          |   2 +-
>> >  cmd/fdt.c             |  19 +++
>> >  cmd/fdt_overlay.c     | 464 ++++++++++++++++++++++++++++++++++++++++++++++++++
>> >  include/fdt_support.h |   2 +-
>> >  4 files changed, 485 insertions(+), 2 deletions(-)
>> >  create mode 100644 cmd/fdt_overlay.c
>>
>> I'm happy to take this into the U-Boot tree while you try to get it
>> applied upstream, but please confirm that you will do this and by
>> when. I suggest you sent an email referring to this patch.
>>
>> Also fdt_overlay.c should go in lib/libfdt/. If this functionality is
>> rejected for libfdt (as was fdtgrep) then we'll move it to lib/.
>
> I'm definitely ok to send it to libfdt if that's what you meant by
> upstream. I'll push it at the same time I'm pushing the next version
> of these patches, just to make sure we progress as fast as we can on
> this.

OK great.

>
>> Finally, please take a look at adding a test for this, with some
>> sample files. See test/py.
>
> However, that will be a bit difficult. In order to be used, the DT
> overlays require the overlay support in dtc, which is still not
> included upstream. So the majority of the users of U-Boot won't have
> that support, meaning that these tests will likely fail for them (even
> though the code might be correct), resulting in a lot of false
> negative.
>
> How do you want me to proceed?

Well I suppose you can write the test, but comment it out for now. We
can still try it with the out-of-tree dtc.

>
> Thanks!
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com

Regards,
Simon

Hi Maxime,

> On May 10, 2016, at 14:45 , Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> 

[snip]

> How do you want me to proceed?
> 
> Thanks!
> Maxime
> 
> -- 
> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com

FYI an updated dtc patch has been sent. Hopefully this time will get in.

Regards

— Pantelis