| Message ID | 20240626235857.272264-1-marex@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | spl: fit: List DTOs applied by SPL in U-Boot control DT | expand |
Hi Marek, On Thu, 27 Jun 2024 at 00:59, Marek Vasut <marex@denx.de> wrote: > > Insert /u-boot,<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,fdt-dto-imx8mp-dhcom-pdk3-overlay-rev100 = <0x00000005>; > u-boot,fdt-dto-imx8mp-dhcom-som-overlay-rev100 = <0x00000004>; > u-boot,fdt-dto-imx8mp-dhcom-pdk-overlay-eth2xfast = <0x00000003>; > u-boot,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 > --- > common/spl/spl_fit.c | 23 ++++++++++++++++++++++- > 1 file changed, 22 insertions(+), 1 deletion(-) > > diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c > index 988125be008..261b72419b1 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 > @@ -450,7 +451,7 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image, > > /* Make room in FDT for changes from the overlay */ > ret = fdt_increase_size(spl_image->fdt_addr, > - image_info.size); > + image_info.size + strlen(str)); You may also need space for the string terminator (i.e. add 1 more byte). > if (ret < 0) > break; > > @@ -464,6 +465,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,%s", str); > + ret = fdt_setprop_u32(spl_image->fdt_addr, 0, dtoname, > + index); > + if (!ret) { This means there was no error, right? > + /* > + * 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); > + } log_debug() > } > free(tmpbuffer); > if (ret) > -- > 2.43.0 > Regards, Simon
Hi Marek, On 6/27/24 1:58 AM, Marek Vasut wrote: > Insert /u-boot,<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,fdt-dto-imx8mp-dhcom-pdk3-overlay-rev100 = <0x00000005>; > u-boot,fdt-dto-imx8mp-dhcom-som-overlay-rev100 = <0x00000004>; > u-boot,fdt-dto-imx8mp-dhcom-pdk-overlay-eth2xfast = <0x00000003>; > u-boot,fdt-dto-imx8mp-dhcom-som-overlay-eth2xfast = <0x00000002>; > ... > ``` > Shouldn't this rather be in /config node? Also, I am highly suggesting to have an additional prefix aside from u-boot to avoid unfortunate name clashes between DTO and existing properties. Or could be in its own child node of /config ? In any case, can you please add or edit a file somewhere to define this new dt-binding. Additional question, what is this index for? I am wondering if we cannot simply have a string array with overlays applied left-to-right or right-to-left instead? Or u-boot,fdt-dto-N = "imx8mp-dhcom-pdk3-overlay-rev100"; Cheers, Quentin
On 6/27/24 10:37 AM, Simon Glass wrote: Hi, >> >> /* Make room in FDT for changes from the overlay */ >> ret = fdt_increase_size(spl_image->fdt_addr, >> - image_info.size); >> + image_info.size + strlen(str)); > > You may also need space for the string terminator (i.e. add 1 more byte). Fixed in V2 with expanded comment, thanks. >> if (ret < 0) >> break; >> >> @@ -464,6 +465,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,%s", str); >> + ret = fdt_setprop_u32(spl_image->fdt_addr, 0, dtoname, >> + index); >> + if (!ret) { > > This means there was no error, right? Yes, this should be inverted, thanks. >> + /* >> + * 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); >> + } > > log_debug() The whole file uses debug(), so let's keep it consistent. Thanks !
On 6/27/24 2:42 PM, Quentin Schulz wrote: > Hi Marek, Hi, > On 6/27/24 1:58 AM, Marek Vasut wrote: >> Insert /u-boot,<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,fdt-dto-imx8mp-dhcom-pdk3-overlay-rev100 = <0x00000005>; >> u-boot,fdt-dto-imx8mp-dhcom-som-overlay-rev100 = <0x00000004>; >> u-boot,fdt-dto-imx8mp-dhcom-pdk-overlay-eth2xfast = >> <0x00000003>; >> u-boot,fdt-dto-imx8mp-dhcom-som-overlay-eth2xfast = >> <0x00000002>; >> ... >> ``` >> > > Shouldn't this rather be in /config node? This is what I had there originally, but then I realized that some DTs might not have the /config node in them (or am I mistaken?), so I moved the new properties into the root node, which surely exists. It also keeps the code simpler, as it doesn't have to fiddle with creation of the /config node if it doesn't exist. > Also, I am highly suggesting to have an additional prefix aside from > u-boot to avoid unfortunate name clashes between DTO and existing > properties. OK, fixed in V2. > Or could be in its own child node of /config ? Please see above. > In any case, can you please add or edit a file somewhere to define this > new dt-binding. Done in V2 > Additional question, what is this index for? To indicate in which order the DTOs were applied by the SPL. It matches the order in which the the DTOs are stored in the fitImage and the order in which SPL iterates over them. In case the SPL skips a DTO using board_spl_fit_append_fdt_skip(), the integer sequence might have gaps. > I am wondering if we cannot simply have a string array with overlays > applied left-to-right or right-to-left instead? Or u-boot,fdt-dto-N = > "imx8mp-dhcom-pdk3-overlay-rev100"; We can, but that is much harder to test for presence of specific DTO on U-Boot command line. It is easy to test for presence of boolean or integer DT property using 'fdt' command. Thanks !
Hi Marek, On 6/28/24 4:42 AM, Marek Vasut wrote: > On 6/27/24 2:42 PM, Quentin Schulz wrote: >> Hi Marek, > > Hi, > >> On 6/27/24 1:58 AM, Marek Vasut wrote: >>> Insert /u-boot,<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,fdt-dto-imx8mp-dhcom-pdk3-overlay-rev100 = <0x00000005>; >>> u-boot,fdt-dto-imx8mp-dhcom-som-overlay-rev100 = <0x00000004>; >>> u-boot,fdt-dto-imx8mp-dhcom-pdk-overlay-eth2xfast = >>> <0x00000003>; >>> u-boot,fdt-dto-imx8mp-dhcom-som-overlay-eth2xfast = >>> <0x00000002>; >>> ... >>> ``` >>> >> >> Shouldn't this rather be in /config node? > > This is what I had there originally, but then I realized that some DTs > might not have the /config node in them (or am I mistaken?), so I moved > the new properties into the root node, which surely exists. > > It also keeps the code simpler, as it doesn't have to fiddle with > creation of the /config node if it doesn't exist. > I don't think we should pollute the root node with this. If we want to be able to expose this in a proper binding to the upstream DT binding repo, I'm not sure this is going to fly :/ >> Also, I am highly suggesting to have an additional prefix aside from >> u-boot to avoid unfortunate name clashes between DTO and existing >> properties. > > OK, fixed in V2. > >> Or could be in its own child node of /config ? > > Please see above. > >> In any case, can you please add or edit a file somewhere to define >> this new dt-binding. > > Done in V2 > >> Additional question, what is this index for? > > To indicate in which order the DTOs were applied by the SPL. It matches > the order in which the the DTOs are stored in the fitImage and the order > in which SPL iterates over them. In case the SPL skips a DTO using > board_spl_fit_append_fdt_skip(), the integer sequence might have gaps. > >> I am wondering if we cannot simply have a string array with overlays >> applied left-to-right or right-to-left instead? Or u-boot,fdt-dto-N = >> "imx8mp-dhcom-pdk3-overlay-rev100"; > > We can, but that is much harder to test for presence of specific DTO on > U-Boot command line. It is easy to test for presence of boolean or > integer DT property using 'fdt' command. > Not sure dt-bindings people would love to hear that we adapt the fdt to be easier to work with from the U-Boot CLI :) In any case, another few thoughts that popped up overnight: 1- do we want to have this for SPL DTB modified by TPL? If so, I'm sure we want it different from the one passed from SPL to U-Boot proper IFF we start from a clean sheet (new DTB) and/or add the DTBO we applied on top the DTBOs already applied by the TPL to the SPL DTB before passing it to proper 2- Add info about which DTBOs were applied to the kernel DTB? Cheers, Quentin
On 6/28/24 10:27 AM, Quentin Schulz wrote: Hi, >>> Shouldn't this rather be in /config node? >> >> This is what I had there originally, but then I realized that some DTs >> might not have the /config node in them (or am I mistaken?), so I >> moved the new properties into the root node, which surely exists. >> >> It also keeps the code simpler, as it doesn't have to fiddle with >> creation of the /config node if it doesn't exist. >> > > I don't think we should pollute the root node with this. If we want to > be able to expose this in a proper binding to the upstream DT binding > repo, I'm not sure this is going to fly :/ The other option is to add more code into SPL, that's not great. I can check if /config node exists, and if so, add it there, otherwise do nothing. That should be some sort of compromise between bloat and config node. What do you think ? [...] >>> I am wondering if we cannot simply have a string array with overlays >>> applied left-to-right or right-to-left instead? Or u-boot,fdt-dto-N = >>> "imx8mp-dhcom-pdk3-overlay-rev100"; >> >> We can, but that is much harder to test for presence of specific DTO >> on U-Boot command line. It is easy to test for presence of boolean or >> integer DT property using 'fdt' command. >> > > Not sure dt-bindings people would love to hear that we adapt the fdt to > be easier to work with from the U-Boot CLI :) > > In any case, another few thoughts that popped up overnight: > 1- do we want to have this for SPL DTB modified by TPL? If so, I'm sure > we want it different from the one passed from SPL to U-Boot proper IFF > we start from a clean sheet (new DTB) and/or add the DTBO we applied on > top the DTBOs already applied by the TPL to the SPL DTB before passing > it to proper As far as I can tell, TPL currently does not support loading fitImages and applying DTOs from them. When this is available, I suspect the prefix of the newly added property would change from u-boot,spl-applied-dto- to u-boot,tpl-applied-dto- and that would be it. > 2- Add info about which DTBOs were applied to the kernel DTB? SPL DTOs or DTOs applied from kernel fitImage ? Or a merging of those ?
Hi Marek, On 6/30/24 4:33 AM, Marek Vasut wrote: > On 6/28/24 10:27 AM, Quentin Schulz wrote: > > Hi, > >>>> Shouldn't this rather be in /config node? >>> >>> This is what I had there originally, but then I realized that some >>> DTs might not have the /config node in them (or am I mistaken?), so I >>> moved the new properties into the root node, which surely exists. >>> >>> It also keeps the code simpler, as it doesn't have to fiddle with >>> creation of the /config node if it doesn't exist. >>> >> >> I don't think we should pollute the root node with this. If we want to >> be able to expose this in a proper binding to the upstream DT binding >> repo, I'm not sure this is going to fly :/ > > The other option is to add more code into SPL, that's not great. > > I can check if /config node exists, and if so, add it there, otherwise > do nothing. That should be some sort of compromise between bloat and > config node. What do you think ? > > [...] > Considering that we're trying to have the same DT for U-Boot and Linux kernel in the long run and that `git grep "\s+config {"` returns nothing in dts/upstream/src, I'm not sure this amounts to anything but dead-code? We could also always insert this node as pre/post process of the DT compilation? But I think some people want to reuse the exact same DTB (like, the binary itself) between U-Boot and the Linux kernel. I guess we can start with checking if the /config node exists and we can figure something smarter once we need it? [...] >> 2- Add info about which DTBOs were applied to the kernel DTB? > > SPL DTOs or DTOs applied from kernel fitImage ? Or a merging of those ? DTOs that U-Boot proper applies to the kernel DTB either via fitimage configs, maybe also via syslinux (via `fdtoverlays`/`devicetree-overlay`) and the like? Cheers, Quentin
On 7/1/24 10:42 AM, Quentin Schulz wrote: > Hi Marek, Hi, >>>>> Shouldn't this rather be in /config node? >>>> >>>> This is what I had there originally, but then I realized that some >>>> DTs might not have the /config node in them (or am I mistaken?), so >>>> I moved the new properties into the root node, which surely exists. >>>> >>>> It also keeps the code simpler, as it doesn't have to fiddle with >>>> creation of the /config node if it doesn't exist. >>>> >>> >>> I don't think we should pollute the root node with this. If we want >>> to be able to expose this in a proper binding to the upstream DT >>> binding repo, I'm not sure this is going to fly :/ >> >> The other option is to add more code into SPL, that's not great. >> >> I can check if /config node exists, and if so, add it there, otherwise >> do nothing. That should be some sort of compromise between bloat and >> config node. What do you think ? >> >> [...] >> > > Considering that we're trying to have the same DT for U-Boot and Linux > kernel in the long run and that `git grep "\s+config {"` returns nothing > in dts/upstream/src, I'm not sure this amounts to anything but > dead-code? We could also always insert this node as pre/post process of > the DT compilation? But I think some people want to reuse the exact same > DTB (like, the binary itself) between U-Boot and the Linux kernel. In my case, the /config node is inserted via *-u-boot.dtsi , I don't think those DT extras are going away anytime soon, esp. because of the 'bootph-*' properties. > I guess we can start with checking if the /config node exists and we can > figure something smarter once we need it? > > [...] > >>> 2- Add info about which DTBOs were applied to the kernel DTB? >> >> SPL DTOs or DTOs applied from kernel fitImage ? Or a merging of those ? > > DTOs that U-Boot proper applies to the kernel DTB either via fitimage > configs, maybe also via syslinux (via > `fdtoverlays`/`devicetree-overlay`) and the like? Isn't that something that would be better (=more flexible and fixable if broken without bootloader update) done in a boot script which could be embedded in a fitImage ?
Hi Marek, On 7/7/24 2:29 AM, Marek Vasut wrote: > On 7/1/24 10:42 AM, Quentin Schulz wrote: >> Hi Marek, > > Hi, > >>>>>> Shouldn't this rather be in /config node? >>>>> >>>>> This is what I had there originally, but then I realized that some >>>>> DTs might not have the /config node in them (or am I mistaken?), so >>>>> I moved the new properties into the root node, which surely exists. >>>>> >>>>> It also keeps the code simpler, as it doesn't have to fiddle with >>>>> creation of the /config node if it doesn't exist. >>>>> >>>> >>>> I don't think we should pollute the root node with this. If we want >>>> to be able to expose this in a proper binding to the upstream DT >>>> binding repo, I'm not sure this is going to fly :/ >>> >>> The other option is to add more code into SPL, that's not great. >>> >>> I can check if /config node exists, and if so, add it there, >>> otherwise do nothing. That should be some sort of compromise between >>> bloat and config node. What do you think ? >>> >>> [...] >>> >> >> Considering that we're trying to have the same DT for U-Boot and Linux >> kernel in the long run and that `git grep "\s+config {"` returns >> nothing in dts/upstream/src, I'm not sure this amounts to anything but >> dead-code? We could also always insert this node as pre/post process >> of the DT compilation? But I think some people want to reuse the exact >> same DTB (like, the binary itself) between U-Boot and the Linux kernel. > > In my case, the /config node is inserted via *-u-boot.dtsi , I don't > think those DT extras are going away anytime soon, esp. because of the > 'bootph-*' properties. > The bootph properties are standardized (?) now. The kernel accepts them (see the ti and xilinx DTSes for Aarch64 for example) and it's part of the dt-schema now: https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/bootph.yaml though I cannot find it in the DT specification (not sure they need to be there though). >> I guess we can start with checking if the /config node exists and we >> can figure something smarter once we need it? >> >> [...] >> >>>> 2- Add info about which DTBOs were applied to the kernel DTB? >>> >>> SPL DTOs or DTOs applied from kernel fitImage ? Or a merging of those ? >> >> DTOs that U-Boot proper applies to the kernel DTB either via fitimage >> configs, maybe also via syslinux (via >> `fdtoverlays`/`devicetree-overlay`) and the like? > > Isn't that something that would be better (=more flexible and fixable if > broken without bootloader update) done in a boot script which could be > embedded in a fitImage ? That's one way to do it though you risk desynchronization between the boot script and which overlays are applied and in which order. Additionally, can we actually already run a boot script from a configuration node? Because one can simply boot with a fit configuration and all of its DTBOs will be applied (if I'm not mistaken), if the boot script needs to be executed standalone before booting the configuration node, that's quite a surprising workflow (but that would work; we could probably also have the boot script directly call the bootm command with the proper configuration node, but then one would have to find a way on how to select which boot script to use between each representing their own configuration node?). Cheers, Quentin
diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c index 988125be008..261b72419b1 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 @@ -450,7 +451,7 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image, /* Make room in FDT for changes from the overlay */ ret = fdt_increase_size(spl_image->fdt_addr, - image_info.size); + image_info.size + strlen(str)); if (ret < 0) break; @@ -464,6 +465,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,%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)
Insert /u-boot,<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,fdt-dto-imx8mp-dhcom-pdk3-overlay-rev100 = <0x00000005>; u-boot,fdt-dto-imx8mp-dhcom-som-overlay-rev100 = <0x00000004>; u-boot,fdt-dto-imx8mp-dhcom-pdk-overlay-eth2xfast = <0x00000003>; u-boot,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 --- common/spl/spl_fit.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)