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)
> +
[PATCH v2] spl: fit: List DTOs applied by SPL in U-Boot control DT
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(-)
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, = 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)
+ 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
