[U-Boot] [PATCH 0/2] cmd: fdt: Add device tree overlays support

Stefan Agner stefan at agner.ch
Tue Apr 5 06:31:48 CEST 2016


Hi Maxime,

Interesting patch set! I am not a dt (overlay) expert, but some comments
below...

On 2016-04-02 14:06, Maxime Ripard 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 at 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);

I guess we should check the return value here and return
CMD_RET_FAILURE. This would allow to abort boot if we apply a DT overlay
in a script. 


> +	}
>  	/* 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

Shouldn't this be in lib/libfdt/?

> @@ -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;

phandle should be uint32_t I think.

Nit: call phandle max_phandle and new_phandle phandle. When reading the
code, new_phandle somehow suggests that something new gets generated or
assigned...

> +
> +	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);
> +}
> +
> +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))

As above, sizeof(*val) instead of a hardcoded 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);

s/ajust/adjust

> +			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;
> +		}
> +

As a matter of consistency, shouldn't we check tree_len here too?

> +		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",

s/ajust/adjust

> +			       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;

Is this table really needed?

As far as I see we could easily get rid of this malloc by encapsulating
the "fixup for-loop" in fdt_overlay_fixup_phandles into a function
(fdt_fixup_property), allocate label, path and name as character arrays
on stack and pass those three pointers to the new function...

Maybe I miss something though...

> +
> +	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++) {

see above, this for loop would go into something like
fdt_overlay_fixup_phandles(dt, property, label, path, name)..

> +			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;
> +	}

As far as I can see, all three of this functions already print error
messages on errors, so IMHO there is no need to do this here again.

--
Stefan

> +
> +	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


More information about the U-Boot mailing list