[PATCH 0/3] fdt: introduce "apply-all" command

[PATCH 0/3] fdt: introduce "apply-all" command

[Andre Przywara]

Hi,

this is an attempt to simplify the usage of user provided devicetree
overlays. The idea is to let the user drop all desired .dtbo files into
one directory (for instance on the EFI system partition), and U-Boot just
applies all of them, with generic commands:
=> fdt move $fdtcontroladdr $fdt_addr_r
=> fdt resize
=> fdt apply-all mmc 0:1 overlays/

This would pick all files ending in .dtbo from the /overlays directory
found on the first partition of the first MMC device. Ideally the device
type, number and partition name would be provided by the distroboot
scripting, so it checks the media it scans for boot scripts anyways.

Patch 1 fixes the "fdt move" operation in the sandbox. Since hostfs does
not support fs_opendir/fs_readdir, which apply-all relies on, this cannot
be tested in the sandbox, but I figured this bug is worth fixing anyway.
Patch 2 avoids a redundant call to "fdt addr", if we just want to move
(actually copy) the DTB. This is needed when we want to build on the
control DT, since this might live in an area where it cannot be changed
or grow enough.
Patch 3 then implements the main attraction: It uses the U-Boot
filesystem API to fs_readdir() the given directory, reads all files
ending in ".dtbo" into memory, and tries to apply them to the working DT.

Please let me know what you think of the idea in general and the
implementation in particular.
The first two patches are actually bug fixes, and should be applied
regardless.

Cheers,
Andre

Andre Przywara (3):
  cmd: fdt: move: Use map_sysmem to convert pointers
  cmd: fdt: allow standalone "fdt move"
  fdt: introduce apply_all command

 cmd/fdt.c | 123 +++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 108 insertions(+), 15 deletions(-)

-- 
2.25.1

[PATCH 1/3] cmd: fdt: move: Use map_sysmem to convert pointers

[Andre Przywara]

The "fdt move" subcommand was using the provided DTB addresses directly,
without trying to "map" them into U-Boot's address space. This happened
to work since on the vast majority of "real" platforms there is a simple
1:1 mapping of VA to PAs, so either value works fine.

However this is not true on the sandbox, so the "fdt move" command fails
there miserably:
=> fdt addr $fdtcontroladdr
=> cp.l $fdtcontroladdr $fdt_addr_r 40  # simple memcpy works
=> fdt move $fdtcontroladdr $fdt_addr_r
Segmentation fault

Use the proper "map_sysmem" call to convert PAs to VAs, to make this
more robust in general and to enable operation in the sandbox.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 cmd/fdt.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/cmd/fdt.c b/cmd/fdt.c
index 842e6cb634..abdc553b2b 100644
--- a/cmd/fdt.c
+++ b/cmd/fdt.c
@@ -211,11 +211,11 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 		/*
 		 * Set the address and length of the fdt.
 		 */
-		working_fdt = (struct fdt_header *)hextoul(argv[2], NULL);
+		working_fdt = map_sysmem(hextoul(argv[2], NULL), 0);
 		if (!fdt_valid(&working_fdt))
 			return 1;
 
-		newaddr = (struct fdt_header *)hextoul(argv[3], NULL);
+		newaddr = map_sysmem(hextoul(argv[3], NULL), 0);
 
 		/*
 		 * If the user specifies a length, use that.  Otherwise use the
@@ -242,7 +242,7 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 				fdt_strerror(err));
 			return 1;
 		}
-		set_working_fdt_addr((ulong)newaddr);
+		set_working_fdt_addr(map_to_sysmem(newaddr));
 #ifdef CONFIG_OF_SYSTEM_SETUP
 	/* Call the board-specific fixup routine */
 	} else if (strncmp(argv[1], "sys", 3) == 0) {
-- 
2.25.1

[PATCH 2/3] cmd: fdt: allow standalone "fdt move"

[Andre Przywara]

At the moment every subcommand of "fdt", except "addr" itself, requires
the DT address to be set first. We explicitly check for that before even
comparing against the subcommands' string.
This early bailout also affects the "move" subcommand, even though that
does not require or rely on a previous call to "fdt addr". In fact it
even sets the FDT address to the target of the move command, so is a
perfect beginning for a sequence of fdt commands.

Move the check for a previously set FDT address to after we handle the
"move" command also, so we don't need a dummy call to "fdt addr" first,
before being able to move the devicetree.

This skips one pointless "fdt addr" call in scripts which aim to alter
the control DT, but need to copy it to a safe location first (for
instance to $fdt_addr_r).

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 cmd/fdt.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/cmd/fdt.c b/cmd/fdt.c
index abdc553b2b..d6878c96f1 100644
--- a/cmd/fdt.c
+++ b/cmd/fdt.c
@@ -188,19 +188,11 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 		}
 
 		return CMD_RET_SUCCESS;
-	}
-
-	if (!working_fdt) {
-		puts("No FDT memory address configured. Please configure\n"
-		     "the FDT address via \"fdt addr <address>\" command.\n"
-		     "Aborting!\n");
-		return CMD_RET_FAILURE;
-	}
 
 	/*
 	 * Move the working_fdt
 	 */
-	if (strncmp(argv[1], "mo", 2) == 0) {
+	} else if (strncmp(argv[1], "mo", 2) == 0) {
 		struct fdt_header *newaddr;
 		int  len;
 		int  err;
@@ -243,9 +235,20 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 			return 1;
 		}
 		set_working_fdt_addr(map_to_sysmem(newaddr));
+
+		return CMD_RET_SUCCESS;
+	}
+
+	if (!working_fdt) {
+		puts("No FDT memory address configured. Please configure\n"
+		     "the FDT address via \"fdt addr <address>\" command.\n"
+		     "Aborting!\n");
+		return CMD_RET_FAILURE;
+	}
+
 #ifdef CONFIG_OF_SYSTEM_SETUP
 	/* Call the board-specific fixup routine */
-	} else if (strncmp(argv[1], "sys", 3) == 0) {
+	if (strncmp(argv[1], "sys", 3) == 0) {
 		int err = ft_system_setup(working_fdt, gd->bd);
 
 		if (err) {
@@ -253,11 +256,14 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 			       fdt_strerror(err));
 			return CMD_RET_FAILURE;
 		}
+
+		return CMD_RET_SUCCESS;
+	}
 #endif
 	/*
 	 * Make a new node
 	 */
-	} else if (strncmp(argv[1], "mk", 2) == 0) {
+	if (strncmp(argv[1], "mk", 2) == 0) {
 		char *pathp;		/* path */
 		char *nodep;		/* new node to add */
 		int  nodeoffset;	/* node offset from libfdt */
-- 
2.25.1

[PATCH 3/3] fdt: introduce apply_all command

[Andre Przywara]

Explicitly specifying the exact filenames of devicetree overlays files
on a U-Boot command line can be quite tedious for users, especially
when it should be made persistent for every boot.

To simplify the task of applying (custom) DT overlays, introduce a
"fdt apply-all" subcommand, that iterates a given directory in any
supported filesystem, and tries to apply every .dtbo file found it
there.

This allows users to simply drop a DT overlay file into a magic
directory, and it will be applied on the next boot automatically,
by the virtue of just a generic U-Boot command call.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 cmd/fdt.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 88 insertions(+), 1 deletion(-)

diff --git a/cmd/fdt.c b/cmd/fdt.c
index d6878c96f1..dc80e13c3d 100644
--- a/cmd/fdt.c
+++ b/cmd/fdt.c
@@ -12,12 +12,14 @@
 #include <env.h>
 #include <image.h>
 #include <linux/ctype.h>
+#include <linux/sizes.h>
 #include <linux/types.h>
 #include <asm/global_data.h>
 #include <linux/libfdt.h>
 #include <fdt_support.h>
 #include <mapmem.h>
 #include <asm/io.h>
+#include <fs.h>
 
 #define MAX_LEVEL	32		/* how deeply nested we will go */
 #define SCRATCHPAD	1024		/* bytes of scratchpad memory */
@@ -107,6 +109,81 @@ static int fdt_get_header_value(int argc, char *const argv[])
 	return CMD_RET_FAILURE;
 }
 
+#ifdef CONFIG_OF_LIBFDT_OVERLAY
+static int apply_all_overlays(const char *ifname, const char *dev_part_str,
+			      const char *dirname)
+{
+	unsigned long addr;
+	struct fdt_header *dtbo;
+	const char *addr_str;
+	struct fs_dir_stream *dirs;
+	struct fs_dirent *dent;
+	char fname[256], *name_beg;
+	int ret;
+
+	addr_str = env_get("fdtoverlay_addr_r");
+	if (!addr_str) {
+		printf("Invalid fdtoverlay_addr_r for loading overlays\n");
+		return CMD_RET_FAILURE;
+	}
+	addr = hextoul(addr_str, NULL);
+
+	ret = fs_set_blk_dev(ifname, dev_part_str, FS_TYPE_ANY);
+	if (ret)
+		return CMD_RET_FAILURE;
+
+	if (!dirname)
+		dirname = "/";
+	dirs = fs_opendir(dirname);
+	if (!dirs) {
+		printf("Cannot find directory \"%s\"\n", dirname);
+		return CMD_RET_FAILURE;
+	}
+
+	strcpy(fname, dirname);
+	name_beg = strchr(fname, 0);
+	if (name_beg[-1] != '/')
+		*name_beg++ = '/';
+
+	dtbo = map_sysmem(addr, 0);
+	while ((dent = fs_readdir(dirs))) {
+		loff_t size = 0;
+
+		if (dent->type == FS_DT_DIR)
+			continue;
+
+		if (strcmp(dent->name + strlen(dent->name) - 5, ".dtbo"))
+			continue;
+
+		printf("%s: ", dent->name);
+		strcpy(name_beg, dent->name);
+		fs_set_blk_dev(ifname, dev_part_str, FS_TYPE_ANY);
+		if (dent->size > SZ_2M)
+			size = SZ_2M;
+		else
+			size = dent->size;
+		ret = fs_read(fname, addr, 0, size, &size);
+		if (ret) {
+			printf("  errno: %d\n", ret);
+			continue;
+		}
+		if (!fdt_valid(&dtbo)) {
+			/* fdt_valid() clears the pointer upon failure */
+			dtbo = map_sysmem(addr, 0);
+			continue;
+		}
+
+		if (fdt_overlay_apply_verbose(working_fdt, dtbo) == 0)
+			printf("applied\n");
+	}
+	unmap_sysmem(dtbo);
+
+	fs_closedir(dirs);
+
+	return 0;
+}
+#endif
+
 /*
  * Flattened Device Tree command, see the help for parameter definitions.
  */
@@ -703,7 +780,7 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	}
 #ifdef CONFIG_OF_LIBFDT_OVERLAY
 	/* apply an overlay */
-	else if (strncmp(argv[1], "ap", 2) == 0) {
+	else if (strcmp(argv[1], "apply") == 0) {
 		unsigned long addr;
 		struct fdt_header *blob;
 		int ret;
@@ -723,6 +800,15 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 		ret = fdt_overlay_apply_verbose(working_fdt, blob);
 		if (ret)
 			return CMD_RET_FAILURE;
+	/* apply all .dtbo files from a directory */
+	} else if (strncmp(argv[1], "apply", 5) == 0) {
+		if (argc != 5)
+			return CMD_RET_USAGE;
+
+		if (!working_fdt)
+			return CMD_RET_FAILURE;
+
+		return apply_all_overlays(argv[2], argv[3], argv[4]);
 	}
 #endif
 	/* resize the fdt */
@@ -1080,6 +1166,7 @@ static char fdt_help_text[] =
 	"addr [-c] [-q] <addr> [<size>]  - Set the [control] fdt location to <addr>\n"
 #ifdef CONFIG_OF_LIBFDT_OVERLAY
 	"fdt apply <addr>                    - Apply overlay to the DT\n"
+	"fdt apply_all <ifname> <dev:part> <dir> - Apply all overlays in directory\n"
 #endif
 #ifdef CONFIG_OF_BOARD_SETUP
 	"fdt boardsetup                      - Do board-specific set up\n"
-- 
2.25.1

Re: [PATCH 1/3] cmd: fdt: move: Use map_sysmem to convert pointers

[Simon Glass]

On Tue, 5 Jul 2022 at 11:14, Andre Przywara <andre.przywara@arm.com> wrote:
>
> The "fdt move" subcommand was using the provided DTB addresses directly,
> without trying to "map" them into U-Boot's address space. This happened
> to work since on the vast majority of "real" platforms there is a simple
> 1:1 mapping of VA to PAs, so either value works fine.
>
> However this is not true on the sandbox, so the "fdt move" command fails
> there miserably:
> => fdt addr $fdtcontroladdr
> => cp.l $fdtcontroladdr $fdt_addr_r 40  # simple memcpy works
> => fdt move $fdtcontroladdr $fdt_addr_r
> Segmentation fault
>
> Use the proper "map_sysmem" call to convert PAs to VAs, to make this
> more robust in general and to enable operation in the sandbox.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  cmd/fdt.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

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

Re: [PATCH 2/3] cmd: fdt: allow standalone "fdt move"

[Simon Glass]

On Tue, 5 Jul 2022 at 11:14, Andre Przywara <andre.przywara@arm.com> wrote:
>
> At the moment every subcommand of "fdt", except "addr" itself, requires
> the DT address to be set first. We explicitly check for that before even
> comparing against the subcommands' string.
> This early bailout also affects the "move" subcommand, even though that
> does not require or rely on a previous call to "fdt addr". In fact it
> even sets the FDT address to the target of the move command, so is a
> perfect beginning for a sequence of fdt commands.
>
> Move the check for a previously set FDT address to after we handle the
> "move" command also, so we don't need a dummy call to "fdt addr" first,
> before being able to move the devicetree.
>
> This skips one pointless "fdt addr" call in scripts which aim to alter
> the control DT, but need to copy it to a safe location first (for
> instance to $fdt_addr_r).
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  cmd/fdt.c | 28 +++++++++++++++++-----------
>  1 file changed, 17 insertions(+), 11 deletions(-)

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

Re: [PATCH 3/3] fdt: introduce apply_all command

[Simon Glass]

Hi Andre,

On Tue, 5 Jul 2022 at 11:14, Andre Przywara <andre.przywara@arm.com> wrote:
>
> Explicitly specifying the exact filenames of devicetree overlays files
> on a U-Boot command line can be quite tedious for users, especially
> when it should be made persistent for every boot.
>
> To simplify the task of applying (custom) DT overlays, introduce a
> "fdt apply-all" subcommand, that iterates a given directory in any
> supported filesystem, and tries to apply every .dtbo file found it
> there.
>
> This allows users to simply drop a DT overlay file into a magic
> directory, and it will be applied on the next boot automatically,
> by the virtue of just a generic U-Boot command call.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  cmd/fdt.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 88 insertions(+), 1 deletion(-)

This looks OK, but can you please add a test (see test/dm/acpi.c for
example) and doc/usage/cmd file?

Also, apply_all is a bit annoying as we try to allow command
completion and abbreviations to work. Given that the args are
different I don't think a -d (for dir) flag makes sense.

Perhaps 'fdt fsapply' ?

Regards,
Simon


>
> diff --git a/cmd/fdt.c b/cmd/fdt.c
> index d6878c96f1..dc80e13c3d 100644
> --- a/cmd/fdt.c
> +++ b/cmd/fdt.c
> @@ -12,12 +12,14 @@
>  #include <env.h>
>  #include <image.h>
>  #include <linux/ctype.h>
> +#include <linux/sizes.h>
>  #include <linux/types.h>
>  #include <asm/global_data.h>
>  #include <linux/libfdt.h>
>  #include <fdt_support.h>
>  #include <mapmem.h>
>  #include <asm/io.h>
> +#include <fs.h>
>
>  #define MAX_LEVEL      32              /* how deeply nested we will go */
>  #define SCRATCHPAD     1024            /* bytes of scratchpad memory */
> @@ -107,6 +109,81 @@ static int fdt_get_header_value(int argc, char *const argv[])
>         return CMD_RET_FAILURE;
>  }
>
> +#ifdef CONFIG_OF_LIBFDT_OVERLAY
> +static int apply_all_overlays(const char *ifname, const char *dev_part_str,
> +                             const char *dirname)
> +{
> +       unsigned long addr;
> +       struct fdt_header *dtbo;
> +       const char *addr_str;
> +       struct fs_dir_stream *dirs;
> +       struct fs_dirent *dent;
> +       char fname[256], *name_beg;
> +       int ret;
> +
> +       addr_str = env_get("fdtoverlay_addr_r");
> +       if (!addr_str) {
> +               printf("Invalid fdtoverlay_addr_r for loading overlays\n");
> +               return CMD_RET_FAILURE;
> +       }
> +       addr = hextoul(addr_str, NULL);
> +
> +       ret = fs_set_blk_dev(ifname, dev_part_str, FS_TYPE_ANY);
> +       if (ret)
> +               return CMD_RET_FAILURE;
> +
> +       if (!dirname)
> +               dirname = "/";
> +       dirs = fs_opendir(dirname);
> +       if (!dirs) {
> +               printf("Cannot find directory \"%s\"\n", dirname);
> +               return CMD_RET_FAILURE;
> +       }
> +
> +       strcpy(fname, dirname);
> +       name_beg = strchr(fname, 0);
> +       if (name_beg[-1] != '/')
> +               *name_beg++ = '/';
> +
> +       dtbo = map_sysmem(addr, 0);
> +       while ((dent = fs_readdir(dirs))) {
> +               loff_t size = 0;
> +
> +               if (dent->type == FS_DT_DIR)
> +                       continue;
> +
> +               if (strcmp(dent->name + strlen(dent->name) - 5, ".dtbo"))
> +                       continue;
> +
> +               printf("%s: ", dent->name);
> +               strcpy(name_beg, dent->name);
> +               fs_set_blk_dev(ifname, dev_part_str, FS_TYPE_ANY);
> +               if (dent->size > SZ_2M)
> +                       size = SZ_2M;
> +               else
> +                       size = dent->size;
> +               ret = fs_read(fname, addr, 0, size, &size);
> +               if (ret) {
> +                       printf("  errno: %d\n", ret);
> +                       continue;
> +               }
> +               if (!fdt_valid(&dtbo)) {
> +                       /* fdt_valid() clears the pointer upon failure */
> +                       dtbo = map_sysmem(addr, 0);
> +                       continue;
> +               }
> +
> +               if (fdt_overlay_apply_verbose(working_fdt, dtbo) == 0)
> +                       printf("applied\n");
> +       }
> +       unmap_sysmem(dtbo);
> +
> +       fs_closedir(dirs);
> +
> +       return 0;
> +}
> +#endif
> +
>  /*
>   * Flattened Device Tree command, see the help for parameter definitions.
>   */
> @@ -703,7 +780,7 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>         }
>  #ifdef CONFIG_OF_LIBFDT_OVERLAY
>         /* apply an overlay */
> -       else if (strncmp(argv[1], "ap", 2) == 0) {
> +       else if (strcmp(argv[1], "apply") == 0) {
>                 unsigned long addr;
>                 struct fdt_header *blob;
>                 int ret;
> @@ -723,6 +800,15 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>                 ret = fdt_overlay_apply_verbose(working_fdt, blob);
>                 if (ret)
>                         return CMD_RET_FAILURE;
> +       /* apply all .dtbo files from a directory */
> +       } else if (strncmp(argv[1], "apply", 5) == 0) {
> +               if (argc != 5)
> +                       return CMD_RET_USAGE;
> +
> +               if (!working_fdt)
> +                       return CMD_RET_FAILURE;
> +
> +               return apply_all_overlays(argv[2], argv[3], argv[4]);
>         }
>  #endif
>         /* resize the fdt */
> @@ -1080,6 +1166,7 @@ static char fdt_help_text[] =
>         "addr [-c] [-q] <addr> [<size>]  - Set the [control] fdt location to <addr>\n"
>  #ifdef CONFIG_OF_LIBFDT_OVERLAY
>         "fdt apply <addr>                    - Apply overlay to the DT\n"
> +       "fdt apply_all <ifname> <dev:part> <dir> - Apply all overlays in directory\n"
>  #endif
>  #ifdef CONFIG_OF_BOARD_SETUP
>         "fdt boardsetup                      - Do board-specific set up\n"
> --
> 2.25.1
>

Re: [PATCH 3/3] fdt: introduce apply_all command

[Andre Przywara]

On Tue, 12 Jul 2022 04:58:35 -0600
Simon Glass <sjg@chromium.org> wrote:

Hi Simon,

many thanks for having a look!

> On Tue, 5 Jul 2022 at 11:14, Andre Przywara <andre.przywara@arm.com> wrote:
> >
> > Explicitly specifying the exact filenames of devicetree overlays files
> > on a U-Boot command line can be quite tedious for users, especially
> > when it should be made persistent for every boot.
> >
> > To simplify the task of applying (custom) DT overlays, introduce a
> > "fdt apply-all" subcommand, that iterates a given directory in any
> > supported filesystem, and tries to apply every .dtbo file found it
> > there.
> >
> > This allows users to simply drop a DT overlay file into a magic
> > directory, and it will be applied on the next boot automatically,
> > by the virtue of just a generic U-Boot command call.
> >
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  cmd/fdt.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 88 insertions(+), 1 deletion(-)  
> 
> This looks OK, but can you please add a test (see test/dm/acpi.c for
> example) and doc/usage/cmd file?

Is that supposed to run inside the sandbox? I briefly tested this there,
only to realise that the sandbox' hostfs does not support the directory
operations (fs_opendir_unsupported). I haven't thought about it too much,
nor do I have much experience with U-Boot's test framework, but this sounds
like a problem?

> Also, apply_all is a bit annoying as we try to allow command
> completion and abbreviations to work. Given that the args are
> different I don't think a -d (for dir) flag makes sense.
> 
> Perhaps 'fdt fsapply' ?

Yeah, I wasn't happy with that name either, but couldn't come up with a
better name. "fsapply" seems to be a nice alternative, I will go with that!

Cheers,
Andre

> 
> Regards,
> Simon
> 
> 
> >
> > diff --git a/cmd/fdt.c b/cmd/fdt.c
> > index d6878c96f1..dc80e13c3d 100644
> > --- a/cmd/fdt.c
> > +++ b/cmd/fdt.c
> > @@ -12,12 +12,14 @@
> >  #include <env.h>
> >  #include <image.h>
> >  #include <linux/ctype.h>
> > +#include <linux/sizes.h>
> >  #include <linux/types.h>
> >  #include <asm/global_data.h>
> >  #include <linux/libfdt.h>
> >  #include <fdt_support.h>
> >  #include <mapmem.h>
> >  #include <asm/io.h>
> > +#include <fs.h>
> >
> >  #define MAX_LEVEL      32              /* how deeply nested we will go */
> >  #define SCRATCHPAD     1024            /* bytes of scratchpad memory */
> > @@ -107,6 +109,81 @@ static int fdt_get_header_value(int argc, char *const argv[])
> >         return CMD_RET_FAILURE;
> >  }
> >
> > +#ifdef CONFIG_OF_LIBFDT_OVERLAY
> > +static int apply_all_overlays(const char *ifname, const char *dev_part_str,
> > +                             const char *dirname)
> > +{
> > +       unsigned long addr;
> > +       struct fdt_header *dtbo;
> > +       const char *addr_str;
> > +       struct fs_dir_stream *dirs;
> > +       struct fs_dirent *dent;
> > +       char fname[256], *name_beg;
> > +       int ret;
> > +
> > +       addr_str = env_get("fdtoverlay_addr_r");
> > +       if (!addr_str) {
> > +               printf("Invalid fdtoverlay_addr_r for loading overlays\n");
> > +               return CMD_RET_FAILURE;
> > +       }
> > +       addr = hextoul(addr_str, NULL);
> > +
> > +       ret = fs_set_blk_dev(ifname, dev_part_str, FS_TYPE_ANY);
> > +       if (ret)
> > +               return CMD_RET_FAILURE;
> > +
> > +       if (!dirname)
> > +               dirname = "/";
> > +       dirs = fs_opendir(dirname);
> > +       if (!dirs) {
> > +               printf("Cannot find directory \"%s\"\n", dirname);
> > +               return CMD_RET_FAILURE;
> > +       }
> > +
> > +       strcpy(fname, dirname);
> > +       name_beg = strchr(fname, 0);
> > +       if (name_beg[-1] != '/')
> > +               *name_beg++ = '/';
> > +
> > +       dtbo = map_sysmem(addr, 0);
> > +       while ((dent = fs_readdir(dirs))) {
> > +               loff_t size = 0;
> > +
> > +               if (dent->type == FS_DT_DIR)
> > +                       continue;
> > +
> > +               if (strcmp(dent->name + strlen(dent->name) - 5, ".dtbo"))
> > +                       continue;
> > +
> > +               printf("%s: ", dent->name);
> > +               strcpy(name_beg, dent->name);
> > +               fs_set_blk_dev(ifname, dev_part_str, FS_TYPE_ANY);
> > +               if (dent->size > SZ_2M)
> > +                       size = SZ_2M;
> > +               else
> > +                       size = dent->size;
> > +               ret = fs_read(fname, addr, 0, size, &size);
> > +               if (ret) {
> > +                       printf("  errno: %d\n", ret);
> > +                       continue;
> > +               }
> > +               if (!fdt_valid(&dtbo)) {
> > +                       /* fdt_valid() clears the pointer upon failure */
> > +                       dtbo = map_sysmem(addr, 0);
> > +                       continue;
> > +               }
> > +
> > +               if (fdt_overlay_apply_verbose(working_fdt, dtbo) == 0)
> > +                       printf("applied\n");
> > +       }
> > +       unmap_sysmem(dtbo);
> > +
> > +       fs_closedir(dirs);
> > +
> > +       return 0;
> > +}
> > +#endif
> > +
> >  /*
> >   * Flattened Device Tree command, see the help for parameter definitions.
> >   */
> > @@ -703,7 +780,7 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> >         }
> >  #ifdef CONFIG_OF_LIBFDT_OVERLAY
> >         /* apply an overlay */
> > -       else if (strncmp(argv[1], "ap", 2) == 0) {
> > +       else if (strcmp(argv[1], "apply") == 0) {
> >                 unsigned long addr;
> >                 struct fdt_header *blob;
> >                 int ret;
> > @@ -723,6 +800,15 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> >                 ret = fdt_overlay_apply_verbose(working_fdt, blob);
> >                 if (ret)
> >                         return CMD_RET_FAILURE;
> > +       /* apply all .dtbo files from a directory */
> > +       } else if (strncmp(argv[1], "apply", 5) == 0) {
> > +               if (argc != 5)
> > +                       return CMD_RET_USAGE;
> > +
> > +               if (!working_fdt)
> > +                       return CMD_RET_FAILURE;
> > +
> > +               return apply_all_overlays(argv[2], argv[3], argv[4]);
> >         }
> >  #endif
> >         /* resize the fdt */
> > @@ -1080,6 +1166,7 @@ static char fdt_help_text[] =
> >         "addr [-c] [-q] <addr> [<size>]  - Set the [control] fdt location to <addr>\n"
> >  #ifdef CONFIG_OF_LIBFDT_OVERLAY
> >         "fdt apply <addr>                    - Apply overlay to the DT\n"
> > +       "fdt apply_all <ifname> <dev:part> <dir> - Apply all overlays in directory\n"
> >  #endif
> >  #ifdef CONFIG_OF_BOARD_SETUP
> >         "fdt boardsetup                      - Do board-specific set up\n"
> > --
> > 2.25.1
> >  

Re: [PATCH 3/3] fdt: introduce apply_all command

[Simon Glass]

Hi Andre,

On Wed, 13 Jul 2022 at 07:18, Andre Przywara <andre.przywara@arm.com> wrote:
>
> On Tue, 12 Jul 2022 04:58:35 -0600
> Simon Glass <sjg@chromium.org> wrote:
>
> Hi Simon,
>
> many thanks for having a look!
>
> > On Tue, 5 Jul 2022 at 11:14, Andre Przywara <andre.przywara@arm.com> wrote:
> > >
> > > Explicitly specifying the exact filenames of devicetree overlays files
> > > on a U-Boot command line can be quite tedious for users, especially
> > > when it should be made persistent for every boot.
> > >
> > > To simplify the task of applying (custom) DT overlays, introduce a
> > > "fdt apply-all" subcommand, that iterates a given directory in any
> > > supported filesystem, and tries to apply every .dtbo file found it
> > > there.
> > >
> > > This allows users to simply drop a DT overlay file into a magic
> > > directory, and it will be applied on the next boot automatically,
> > > by the virtue of just a generic U-Boot command call.
> > >
> > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > > ---
> > >  cmd/fdt.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 88 insertions(+), 1 deletion(-)
> >
> > This looks OK, but can you please add a test (see test/dm/acpi.c for
> > example) and doc/usage/cmd file?
>
> Is that supposed to run inside the sandbox? I briefly tested this there,
> only to realise that the sandbox' hostfs does not support the directory
> operations (fs_opendir_unsupported). I haven't thought about it too much,
> nor do I have much experience with U-Boot's test framework, but this sounds
> like a problem?

Yes that is a problem, although it would not be too hard to implement, I think.

Also I sent a little series to add a test for 'fdt addr' which might
make it easier for you.

>
> > Also, apply_all is a bit annoying as we try to allow command
> > completion and abbreviations to work. Given that the args are
> > different I don't think a -d (for dir) flag makes sense.
> >
> > Perhaps 'fdt fsapply' ?
>
> Yeah, I wasn't happy with that name either, but couldn't come up with a
> better name. "fsapply" seems to be a nice alternative, I will go with that!

OK good.

Regards,
Simon

Re: [PATCH 2/3] cmd: fdt: allow standalone "fdt move"

[Simon Glass]

On Tue, 5 Jul 2022 at 11:14, Andre Przywara <andre.przywara@arm.com> wrote:
>
> At the moment every subcommand of "fdt", except "addr" itself, requires
> the DT address to be set first. We explicitly check for that before even
> comparing against the subcommands' string.
> This early bailout also affects the "move" subcommand, even though that
> does not require or rely on a previous call to "fdt addr". In fact it
> even sets the FDT address to the target of the move command, so is a
> perfect beginning for a sequence of fdt commands.
>
> Move the check for a previously set FDT address to after we handle the
> "move" command also, so we don't need a dummy call to "fdt addr" first,
> before being able to move the devicetree.
>
> This skips one pointless "fdt addr" call in scripts which aim to alter
> the control DT, but need to copy it to a safe location first (for
> instance to $fdt_addr_r).
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  cmd/fdt.c | 28 +++++++++++++++++-----------
>  1 file changed, 17 insertions(+), 11 deletions(-)

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

Applied to u-boot-dm, thanks!

Re: [PATCH 1/3] cmd: fdt: move: Use map_sysmem to convert pointers

[Simon Glass]

On Tue, 5 Jul 2022 at 11:14, Andre Przywara <andre.przywara@arm.com> wrote:
>
> The "fdt move" subcommand was using the provided DTB addresses directly,
> without trying to "map" them into U-Boot's address space. This happened
> to work since on the vast majority of "real" platforms there is a simple
> 1:1 mapping of VA to PAs, so either value works fine.
>
> However this is not true on the sandbox, so the "fdt move" command fails
> there miserably:
> => fdt addr $fdtcontroladdr
> => cp.l $fdtcontroladdr $fdt_addr_r 40  # simple memcpy works
> => fdt move $fdtcontroladdr $fdt_addr_r
> Segmentation fault
>
> Use the proper "map_sysmem" call to convert PAs to VAs, to make this
> more robust in general and to enable operation in the sandbox.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  cmd/fdt.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

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

Applied to u-boot-dm, thanks!