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