| Message ID | 20240628024804.149871-1-marex@denx.de |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | [v2] spl: fit: List DTOs applied by SPL in U-Boot control DT | expand |
Hi Marek, On Fri, 28 Jun 2024 at 03:48, Marek Vasut <marex@denx.de> wrote: > > Insert /u-boot,spl-applied-dto-<dto-name> = <index> 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 = <0x00000005>; > u-boot,spl-applied-dto-fdt-dto-imx8mp-dhcom-som-overlay-rev100 = <0x00000004>; > u-boot,spl-applied-dto-fdt-dto-imx8mp-dhcom-pdk-overlay-eth2xfast = <0x00000003>; > u-boot,spl-applied-dto-fdt-dto-imx8mp-dhcom-som-overlay-eth2xfast = <0x00000002>; > ... > ``` > > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Manoj Sai <abbaraju.manojsai@amarulasolutions.com> > Cc: Sean Anderson <seanga2@gmail.com> > Cc: Simon Glass <sjg@chromium.org> > Cc: Suniel Mahesh <sunil@amarulasolutions.com> > Cc: Tom Rini <trini@konsulko.com> > Cc: u-boot@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,<dto-name> = <index> 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,<dto-name> 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) > + 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() > + > u-boot,spl-payload-offset (int) > If present (and SPL is controlled by the device-tree), this allows > to override the CONFIG_SYS_SPI_U_BOOT_OFFS setting using a value > -- > 2.43.0 > Regards, Simon
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.
Hi Marek, On Sun, 30 Jun 2024 at 06:24, Marek Vasut <marex@denx.de> 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
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); 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,<dto-name> = <index> 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,<dto-name> 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) + 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. + u-boot,spl-payload-offset (int) If present (and SPL is controlled by the device-tree), this allows to override the CONFIG_SYS_SPI_U_BOOT_OFFS setting using a value
Insert /u-boot,spl-applied-dto-<dto-name> = <index> 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 = <0x00000005>; u-boot,spl-applied-dto-fdt-dto-imx8mp-dhcom-som-overlay-rev100 = <0x00000004>; u-boot,spl-applied-dto-fdt-dto-imx8mp-dhcom-pdk-overlay-eth2xfast = <0x00000003>; u-boot,spl-applied-dto-fdt-dto-imx8mp-dhcom-som-overlay-eth2xfast = <0x00000002>; ... ``` Signed-off-by: Marek Vasut <marex@denx.de> --- Cc: Manoj Sai <abbaraju.manojsai@amarulasolutions.com> Cc: Sean Anderson <seanga2@gmail.com> Cc: Simon Glass <sjg@chromium.org> Cc: Suniel Mahesh <sunil@amarulasolutions.com> Cc: Tom Rini <trini@konsulko.com> Cc: u-boot@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(-)