[PATCH v2 0/3] fdt: introduce "fsapply" command

[PATCH v2 0/3] fdt: introduce "fsapply" command

[Andre Przywara]

Hi,

updating the series, basically just changing the name of the command
to "fsapply", as suggested by Simon.
The first two patches are still the same fixes, Simon said he merged
them to u-boot-dm in July already, but I don't see them there or in
master.
I am still looking into a test for fsapply, but wanted to get at least
the fixes merged now, hence this new version.

===============================
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 fsapply 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 fsapply 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

Changelog v1 ... v2:
- add Simon's tags
- change command name from "apply-all" to "fsapply"


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

 cmd/fdt.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 107 insertions(+), 14 deletions(-)

-- 
2.25.1

[PATCH v2 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>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 cmd/fdt.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/cmd/fdt.c b/cmd/fdt.c
index 8e51a431261..0ba691c573b 100644
--- a/cmd/fdt.c
+++ b/cmd/fdt.c
@@ -231,11 +231,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
@@ -262,7 +262,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 v2 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>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 cmd/fdt.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/cmd/fdt.c b/cmd/fdt.c
index 0ba691c573b..1972490bdc2 100644
--- a/cmd/fdt.c
+++ b/cmd/fdt.c
@@ -208,19 +208,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;
@@ -263,9 +255,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) {
@@ -273,11 +276,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 v2 3/3] fdt: introduce fsapply 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 fsapply" 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 | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 87 insertions(+)

diff --git a/cmd/fdt.c b/cmd/fdt.c
index 1972490bdc2..00f92dbbb5d 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 */
@@ -127,6 +129,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.
  */
@@ -747,6 +824,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], "fsap", 4) == 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 */
@@ -1104,6 +1190,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 fsapply <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 v2 3/3] fdt: introduce fsapply command

[Lothar Waßmann]

Hi,

On Fri, 10 Feb 2023 11:02:13 +0000 Andre Przywara 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 fsapply" 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 | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 87 insertions(+)
> 
> diff --git a/cmd/fdt.c b/cmd/fdt.c
> index 1972490bdc2..00f92dbbb5d 100644
> --- a/cmd/fdt.c
> +++ b/cmd/fdt.c
> @@ -127,6 +129,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;
return CMD_RET_SUCCESS;


Lothar Waßmann

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

[Lothar Waßmann]

Hi,

On Fri, 10 Feb 2023 11:02:12 +0000 Andre Przywara 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>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
>  cmd/fdt.c | 28 +++++++++++++++++-----------
>  1 file changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/cmd/fdt.c b/cmd/fdt.c
> index 0ba691c573b..1972490bdc2 100644
> --- a/cmd/fdt.c
> +++ b/cmd/fdt.c
> @@ -208,19 +208,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;
> @@ -263,9 +255,20 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>  			return 1;
>  		}
It's not part of your changes, but this should rather be:
			return CMD_RET_FAILURE;



Lothar Waßmann

Re: [PATCH v2 3/3] fdt: introduce fsapply command

[Simon Glass]

Hi,

On Fri, 10 Feb 2023 at 04:32, Lothar Waßmann <LW@karo-electronics.de> wrote:
>
> Hi,
>
> On Fri, 10 Feb 2023 11:02:13 +0000 Andre Przywara 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 fsapply" 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 | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 87 insertions(+)

Please add some help at doc/usage/cmd

Also please add a test for this subcommand in test/cmd

> >
> > diff --git a/cmd/fdt.c b/cmd/fdt.c
> > index 1972490bdc2..00f92dbbb5d 100644
> > --- a/cmd/fdt.c
> > +++ b/cmd/fdt.c
> > @@ -127,6 +129,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;
> return CMD_RET_SUCCESS;

There is no need for that...0 means success in U-Boot. It is shorter
and clearer IMO.

Regards,
SImon

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

[Simon Glass]

Hi,

On Fri, 10 Feb 2023 11:02:12 +0000 Andre Przywara 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>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
>  cmd/fdt.c | 28 +++++++++++++++++-----------
>  1 file changed, 17 insertions(+), 11 deletions(-)
>
Applied to u-boot-dm, thanks!

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

[Simon Glass]

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>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 cmd/fdt.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Applied to u-boot-dm, thanks!

Re: [PATCH v2 3/3] fdt: introduce fsapply command

[Andre Przywara]

On Fri, 10 Feb 2023 09:05:34 -0700
Simon Glass <sjg@chromium.org> wrote:

Hi Simon,

> On Fri, 10 Feb 2023 at 04:32, Lothar Waßmann <LW@karo-electronics.de> wrote:
> >
> > Hi,
> >
> > On Fri, 10 Feb 2023 11:02:13 +0000 Andre Przywara 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 fsapply" 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 | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 87 insertions(+)  
> 
> Please add some help at doc/usage/cmd

Right, will do.

> Also please add a test for this subcommand in test/cmd

Yeah, I knew you would say that ;-)
It's still the same problem as last time: sandboxfs doesn't implement
.readdir, so this doesn't work easily there. So I started with filling this
gap, and was just wondering if I should piggy back on the already existing
sandbox_fs_ls abstraction, and somewhat re-translate this back into dirent
structures, or whether I should properly wrap
{open,read,close}dir in os_*dir() helpers, and build sandbox_fs_readdir()
based on that?

Any advice? Both seem equally doable.

Cheers,
Andre

> > > diff --git a/cmd/fdt.c b/cmd/fdt.c
> > > index 1972490bdc2..00f92dbbb5d 100644
> > > --- a/cmd/fdt.c
> > > +++ b/cmd/fdt.c
> > > @@ -127,6 +129,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;  
> > return CMD_RET_SUCCESS;  
> 
> There is no need for that...0 means success in U-Boot. It is shorter
> and clearer IMO.
> 
> Regards,
> SImon

Re: [PATCH v2 3/3] fdt: introduce fsapply command

[Simon Glass]

Hi Andre,

On Mon, 13 Feb 2023 at 10:26, Andre Przywara <andre.przywara@arm.com> wrote:
>
> On Fri, 10 Feb 2023 09:05:34 -0700
> Simon Glass <sjg@chromium.org> wrote:
>
> Hi Simon,
>
> > On Fri, 10 Feb 2023 at 04:32, Lothar Waßmann <LW@karo-electronics.de> wrote:
> > >
> > > Hi,
> > >
> > > On Fri, 10 Feb 2023 11:02:13 +0000 Andre Przywara 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 fsapply" 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 | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 87 insertions(+)
> >
> > Please add some help at doc/usage/cmd
>
> Right, will do.
>
> > Also please add a test for this subcommand in test/cmd
>
> Yeah, I knew you would say that ;-)
> It's still the same problem as last time: sandboxfs doesn't implement
> .readdir, so this doesn't work easily there. So I started with filling this
> gap, and was just wondering if I should piggy back on the already existing
> sandbox_fs_ls abstraction, and somewhat re-translate this back into dirent
> structures, or whether I should properly wrap
> {open,read,close}dir in os_*dir() helpers, and build sandbox_fs_readdir()
> based on that?
>
> Any advice? Both seem equally doable.

I like option two as I think it will help in the future too!

Regards,
SImon

>
> Cheers,
> Andre
>
> > > > diff --git a/cmd/fdt.c b/cmd/fdt.c
> > > > index 1972490bdc2..00f92dbbb5d 100644
> > > > --- a/cmd/fdt.c
> > > > +++ b/cmd/fdt.c
> > > > @@ -127,6 +129,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;
> > > return CMD_RET_SUCCESS;
> >
> > There is no need for that...0 means success in U-Boot. It is shorter
> > and clearer IMO.
> >
> > Regards,
> > SImon
>

Re: [PATCH v2 0/3] fdt: introduce "fsapply" command

[Heinrich Schuchardt]

On 2/10/23 12:02, Andre Przywara wrote:
> Hi,
>
> updating the series, basically just changing the name of the command
> to "fsapply", as suggested by Simon.
> The first two patches are still the same fixes, Simon said he merged
> them to u-boot-dm in July already, but I don't see them there or in
> master.
> I am still looking into a test for fsapply, but wanted to get at least
> the fixes merged now, hence this new version.
>
> ===============================
> 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 fsapply 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 fsapply 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
>
> Changelog v1 ... v2:
> - add Simon's tags
> - change command name from "apply-all" to "fsapply"
>
>
> Andre Przywara (3):
>    cmd: fdt: move: Use map_sysmem to convert pointers
>    cmd: fdt: allow standalone "fdt move"
>    fdt: introduce fsapply command
>
>   cmd/fdt.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++-------
>   1 file changed, 107 insertions(+), 14 deletions(-)
>
Hello Andre,

We should document this change in a document doc/usage/cmd/fdt.rst.

Best regards

Heinrich