Re: [PATCH v2] spl: fit: List DTOs applied by SPL in U-Boot control DT
Hi Marek, On Sun, 30 Jun 2024 at 06:24, Marek Vasut wrote: > > On 6/28/24 9:32 AM, Simon Glass wrote: > > Hi Marek, > > Hi, > > >> --- > >> common/spl/spl_fit.c| 29 +++-- > >> doc/device-tree-bindings/config.txt | 11 +++ > >> 2 files changed, 38 insertions(+), 2 deletions(-) > > > > Once this is figured out, can you extend test/image/spl_load_os.c to > > test this code? > > It seems there is nothing which would do even basic tests for SPL > fitImage DT handling in that test? Or am I reading the current test wrong ? That test handles loading of a FIT, but doesn't check what happens to the DT. You could add more asserts to spl_load_test(), or create a new test. You can run it in the sandbox_spl build with: sandbox_spl/spl/u-boot-spl -u -c "exit" > > >> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c > >> index 988125be008..0a6b5ea8212 100644 > >> --- a/common/spl/spl_fit.c > >> +++ b/common/spl/spl_fit.c > >> @@ -363,6 +363,7 @@ static int spl_fit_append_fdt(struct spl_image_info > >> *spl_image, > >> { > >> struct spl_image_info image_info; > >> int node, ret = 0, index = 0; > >> + char dtoname[256]; > >> > >> /* > >> * Use the address following the image as target address for the > >> @@ -448,9 +449,13 @@ static int spl_fit_append_fdt(struct spl_image_info > >> *spl_image, > >> if (ret < 0) > >> break; > >> > >> - /* Make room in FDT for changes from the overlay */ > >> + /* > >> +* Make room in FDT for changes from the overlay, > >> +* the overlay name and the trailing NUL byte in > >> +* that name. > >> +*/ > >> ret = fdt_increase_size(spl_image->fdt_addr, > >> - image_info.size); > >> + image_info.size + > >> strlen(str) + 1); > > > > Oh and I missed the room for the property, sorry. It needs something like > > this: > > > > ALIGN(strlen(str) + 1, 4) + 12 + 4 > > > > the first bit is the string-table size increase > > > > 12 is sizeof(struct fdt_property) > > 4 is the u32 size > > > > Alos, is there any way to check that there is actually enough space to > > increase the size? > > fdt_increase_size() would fail if there isn't enough space, so that > should cover the check. Yes it does, but I meant the memory about the DT. Do we know how much space space there is to increase the DT into? > > >> diff --git a/doc/device-tree-bindings/config.txt > >> b/doc/device-tree-bindings/config.txt > >> index f50c68bbdc3..7aa6d9a72c6 100644 > >> --- a/doc/device-tree-bindings/config.txt > >> +++ b/doc/device-tree-bindings/config.txt > >> @@ -95,6 +95,17 @@ rootdisk-offset (int) > >> silent-console (int) > >> If present and non-zero, the console is silenced by default on > >> boot. > >> > >> +u-boot,spl-applied-dto-* (int) > >> + Emitted by SPL into U-Boot control DT root node in case a DTO from > >> + fitImage was applied on top of U-Boot control DT by the SPL > >> fitImage > >> + loader. The single integer cell indicates in which order was the > >> DTO > >> + applied by the SPL and matches the index of the DTO in the > >> fitImage. > >> + DTOs in fitImage may be skipped using > >> board_spl_fit_append_fdt_skip(), > >> + therefore the integers might not be monotonically incrementing, > >> there > >> + may be gaps. This property can be used to determine which DTOs were > >> + applied by the SPL from running U-Boot by inspecting the U-Boot > >> + control DT. > > > > Should we send a binding with this? I wonder if it would be better to > > make the filename a property value rather than including it in the > > property name/string table? That way you would not need the > > integers...the ordering would be enough. > > > > E.g. > > > > u-boot,spl-applied-dtos = "fdt-dto-imx8mp-dhcom-som-overlay-eth2xfast", > >"fdt-dto-imx8mp-dhcom-pdk-overlay-eth2xfast"; > > > > But that might be more annoying to construct as you would probably > > need fdt_setprop_placeholder() > > It is easier to test for a presence of property from U-Boot shell, also > the property is integer where the integer indicates the index of the DTO > when it was applied. Right, but in my suggestion the ordering is obvious, from the stringlist. Regards, Simon
Re: [PATCH v2] spl: fit: List DTOs applied by SPL in U-Boot control DT
On 6/28/24 9:32 AM, Simon Glass wrote: Hi Marek, Hi, --- common/spl/spl_fit.c| 29 +++-- doc/device-tree-bindings/config.txt | 11 +++ 2 files changed, 38 insertions(+), 2 deletions(-) Once this is figured out, can you extend test/image/spl_load_os.c to test this code? It seems there is nothing which would do even basic tests for SPL fitImage DT handling in that test? Or am I reading the current test wrong ? diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c index 988125be008..0a6b5ea8212 100644 --- a/common/spl/spl_fit.c +++ b/common/spl/spl_fit.c @@ -363,6 +363,7 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image, { struct spl_image_info image_info; int node, ret = 0, index = 0; + char dtoname[256]; /* * Use the address following the image as target address for the @@ -448,9 +449,13 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image, if (ret < 0) break; - /* Make room in FDT for changes from the overlay */ + /* +* Make room in FDT for changes from the overlay, +* the overlay name and the trailing NUL byte in +* that name. +*/ ret = fdt_increase_size(spl_image->fdt_addr, - image_info.size); + image_info.size + strlen(str) + 1); Oh and I missed the room for the property, sorry. It needs something like this: ALIGN(strlen(str) + 1, 4) + 12 + 4 the first bit is the string-table size increase 12 is sizeof(struct fdt_property) 4 is the u32 size Alos, is there any way to check that there is actually enough space to increase the size? fdt_increase_size() would fail if there isn't enough space, so that should cover the check. diff --git a/doc/device-tree-bindings/config.txt b/doc/device-tree-bindings/config.txt index f50c68bbdc3..7aa6d9a72c6 100644 --- a/doc/device-tree-bindings/config.txt +++ b/doc/device-tree-bindings/config.txt @@ -95,6 +95,17 @@ rootdisk-offset (int) silent-console (int) If present and non-zero, the console is silenced by default on boot. +u-boot,spl-applied-dto-* (int) + Emitted by SPL into U-Boot control DT root node in case a DTO from + fitImage was applied on top of U-Boot control DT by the SPL fitImage + loader. The single integer cell indicates in which order was the DTO + applied by the SPL and matches the index of the DTO in the fitImage. + DTOs in fitImage may be skipped using board_spl_fit_append_fdt_skip(), + therefore the integers might not be monotonically incrementing, there + may be gaps. This property can be used to determine which DTOs were + applied by the SPL from running U-Boot by inspecting the U-Boot + control DT. Should we send a binding with this? I wonder if it would be better to make the filename a property value rather than including it in the property name/string table? That way you would not need the integers...the ordering would be enough. E.g. u-boot,spl-applied-dtos = "fdt-dto-imx8mp-dhcom-som-overlay-eth2xfast", "fdt-dto-imx8mp-dhcom-pdk-overlay-eth2xfast"; But that might be more annoying to construct as you would probably need fdt_setprop_placeholder() It is easier to test for a presence of property from U-Boot shell, also the property is integer where the integer indicates the index of the DTO when it was applied.
Re: [PATCH v2] spl: fit: List DTOs applied by SPL in U-Boot control DT
Hi Marek, On Fri, 28 Jun 2024 at 03:48, Marek Vasut wrote: > > Insert /u-boot,spl-applied-dto- = property into the > U-Boot control DT during SPL DTO application process. This can be used > by user to inspect which DTOs got applied by the SPL and in which order > from running U-Boot. > > Example: > ``` > u-boot=> fdt addr $fdtcontroladdr && fdt list / > Working FDT set to aee9aeb0 > / { > u-boot,spl-applied-dto-fdt-dto-imx8mp-dhcom-pdk3-overlay-rev100 = > <0x0005>; > u-boot,spl-applied-dto-fdt-dto-imx8mp-dhcom-som-overlay-rev100 = > <0x0004>; > u-boot,spl-applied-dto-fdt-dto-imx8mp-dhcom-pdk-overlay-eth2xfast = > <0x0003>; > u-boot,spl-applied-dto-fdt-dto-imx8mp-dhcom-som-overlay-eth2xfast = > <0x0002>; > ... > ``` > > Signed-off-by: Marek Vasut > --- > Cc: Manoj Sai > Cc: Sean Anderson > Cc: Simon Glass > Cc: Suniel Mahesh > Cc: Tom Rini > Cc: u-b...@dh-electronics.com > Cc: u-boot@lists.denx.de > --- > V2: - Expand the DT by one more byte to cater for trailing NUL byte > - Update comment related to the expansion of DT > - Flip the !ret conditional, which was incorrect and missed > - Expand prefix to u-boot,spl-applied-dto- > - Document the binding > --- > common/spl/spl_fit.c| 29 +++-- > doc/device-tree-bindings/config.txt | 11 +++ > 2 files changed, 38 insertions(+), 2 deletions(-) Once this is figured out, can you extend test/image/spl_load_os.c to test this code? > diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c > index 988125be008..0a6b5ea8212 100644 > --- a/common/spl/spl_fit.c > +++ b/common/spl/spl_fit.c > @@ -363,6 +363,7 @@ static int spl_fit_append_fdt(struct spl_image_info > *spl_image, > { > struct spl_image_info image_info; > int node, ret = 0, index = 0; > + char dtoname[256]; > > /* > * Use the address following the image as target address for the > @@ -448,9 +449,13 @@ static int spl_fit_append_fdt(struct spl_image_info > *spl_image, > if (ret < 0) > break; > > - /* Make room in FDT for changes from the overlay */ > + /* > +* Make room in FDT for changes from the overlay, > +* the overlay name and the trailing NUL byte in > +* that name. > +*/ > ret = fdt_increase_size(spl_image->fdt_addr, > - image_info.size); > + image_info.size + strlen(str) > + 1); Oh and I missed the room for the property, sorry. It needs something like this: ALIGN(strlen(str) + 1, 4) + 12 + 4 the first bit is the string-table size increase 12 is sizeof(struct fdt_property) 4 is the u32 size Alos, is there any way to check that there is actually enough space to increase the size? > if (ret < 0) > break; > > @@ -464,6 +469,26 @@ static int spl_fit_append_fdt(struct spl_image_info > *spl_image, > > debug("%s: DT overlay %s applied\n", __func__, > fit_get_name(ctx->fit, node, NULL)); > + > + /* > +* Insert /u-boot, = property into > +* the U-Boot control DT. This can be used by user > +* to inspect which DTOs got applied by the SPL from > +* a running U-Boot. > +*/ > + snprintf(dtoname, sizeof(dtoname), > "u-boot,spl-applied-dto-%s", str); > + ret = fdt_setprop_u32(spl_image->fdt_addr, 0, dtoname, > + index); > + if (ret) { > + /* > +* The DTO itself was applied, do not treat > the > +* insertion of /u-boot, as an error > +* so the system can possibly boot somehow. > +*/ > + debug("%s: DT overlay %s name not inserted > into / node (%d)\n", > + __func__, > + fit_get_name(ctx->fit, node, NULL), > ret); > + } > } > free(tmpbuffer); > if (ret) > diff --git a/doc/device-tree-bindings/config.txt > b/doc/device-tree-bindings/config.txt > index f50c68bbdc3..7aa6d9a72c6 100644 > --- a/doc/device-tree-bindings/config.txt > +++ b/doc/device-tree-bindings/config.txt > @@ -95,6 +95,17 @@ rootdisk-offset (int) > silent-console (int) > If present and non-zero, the console is silenced by default on boot. > > +u-boot,spl-applied-dto-* (int) > +