[PATCH v2] spl: fit: List DTOs applied by SPL in U-Boot control DT

[Simon Glass]

Hi Marek,

On Sun, 30 Jun 2024 at 06:24, Marek Vasut <marex at 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