| Message ID | 20220705171350.1765454-4-andre.przywara@arm.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | fdt: introduce "apply-all" command | expand |
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 >
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 > >
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
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"
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(-)