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.rip...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
