[PATCH 1/2] bloblist: fix the overriding of fdt from bloblist

[PATCH 1/2] bloblist: fix the overriding of fdt from bloblist

[Raymond Mao]

When a bloblist is valid and contains fdt, it explicitly means
a previous boot stage is passing transfer list compliant with
Firmware Handoff specification, thus the fdt from bloblist should
not be overridden with the ones from board or env variables.

Fixes: 70fe23859437 ("fdt: Allow the devicetree to come from a bloblist")
Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
---
 lib/fdtdec.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index f09c9926a7a..c38738b48c7 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -1708,7 +1708,7 @@ int fdtdec_setup(void)
 				gd->fdt_src = FDTSRC_BLOBLIST;
 				log_debug("Devicetree is in bloblist at %p\n",
 					  gd->fdt_blob);
-				ret = 0;
+				goto setup_fdt;
 			} else {
 				log_debug("No FDT found in bloblist\n");
 				ret = -ENOENT;
@@ -1752,6 +1752,7 @@ int fdtdec_setup(void)
 		}
 	}
 
+setup_fdt:
 	if (CONFIG_IS_ENABLED(MULTI_DTB_FIT))
 		setup_multi_dtb_fit();
 
-- 
2.25.1

[PATCH 2/2] env: point fdt address to the fdt in a bloblist

[Raymond Mao]

Point fdt_addr to the fdt embedded in the bloblist since fdt_addr
is a default address for bootefi, bootm and booti to look for the
device tree when launching the kernel.

Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
---
 env/common.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/env/common.c b/env/common.c
index a58955a4f42..a38e5d107a8 100644
--- a/env/common.c
+++ b/env/common.c
@@ -16,6 +16,7 @@
 #include <asm/global_data.h>
 #include <linux/printk.h>
 #include <linux/stddef.h>
+#include <mapmem.h>
 #include <search.h>
 #include <errno.h>
 #include <malloc.h>
@@ -368,6 +369,18 @@ int env_get_default_into(const char *name, char *buf, unsigned int len)
 	return env_get_from_linear(default_environment, name, buf, len);
 }
 
+static int env_update_fdt_addr_from_bloblist(void)
+{
+	/*
+	 * fdt_addr is by default used by booti, bootm and bootefi,
+	 * thus set it to point to the fdt embedded in a bloblist if it exists.
+	 */
+	if (!CONFIG_IS_ENABLED(BLOBLIST) || gd->fdt_src != FDTSRC_BLOBLIST)
+		return 0;
+
+	return env_set_hex("fdt_addr", (uintptr_t)map_to_sysmem(gd->fdt_blob));
+}
+
 void env_set_default(const char *s, int flags)
 {
 	if (s) {
@@ -392,6 +405,10 @@ void env_set_default(const char *s, int flags)
 
 	gd->flags |= GD_FLG_ENV_READY;
 	gd->flags |= GD_FLG_ENV_DEFAULT;
+
+	/* This has to be done after GD_FLG_ENV_READY is set */
+	if (env_update_fdt_addr_from_bloblist())
+		pr_err("Failed to set fdt_addr to point at DTB\n");
 }
 
 /* [re]set individual variables to their value in the default environment */
-- 
2.25.1

Re: [PATCH 1/2] bloblist: fix the overriding of fdt from bloblist

[Caleb Connolly]

Hi Raymond,

On 3/28/25 00:13, Raymond Mao wrote:
> When a bloblist is valid and contains fdt, it explicitly means
> a previous boot stage is passing transfer list compliant with
> Firmware Handoff specification, thus the fdt from bloblist should
> not be overridden with the ones from board or env variables.
> 
> Fixes: 70fe23859437 ("fdt: Allow the devicetree to come from a bloblist")
> Signed-off-by: Raymond Mao <raymond.mao@linaro.org>

Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org>
> ---
>   lib/fdtdec.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> index f09c9926a7a..c38738b48c7 100644
> --- a/lib/fdtdec.c
> +++ b/lib/fdtdec.c
> @@ -1708,7 +1708,7 @@ int fdtdec_setup(void)
>   				gd->fdt_src = FDTSRC_BLOBLIST;
>   				log_debug("Devicetree is in bloblist at %p\n",
>   					  gd->fdt_blob);
> -				ret = 0;
> +				goto setup_fdt;
>   			} else {
>   				log_debug("No FDT found in bloblist\n");
>   				ret = -ENOENT;
> @@ -1752,6 +1752,7 @@ int fdtdec_setup(void)
>   		}
>   	}
>   
> +setup_fdt:
>   	if (CONFIG_IS_ENABLED(MULTI_DTB_FIT))
>   		setup_multi_dtb_fit();
>   

-- 
Caleb (they/them)

Re: [PATCH 1/2] bloblist: fix the overriding of fdt from bloblist

[Simon Glass]

Hi Raymond,

On Thu, 27 Mar 2025 at 17:13, Raymond Mao <raymond.mao@linaro.org> wrote:
>
> When a bloblist is valid and contains fdt, it explicitly means
> a previous boot stage is passing transfer list compliant with
> Firmware Handoff specification, thus the fdt from bloblist should
> not be overridden with the ones from board or env variables.

Why is that? The board can choose to override it, if it wishes, but in
general, if gd->fdt_blob is already set up, it should return -EEXIST
from board_fdt_blob_setup().

>
> Fixes: 70fe23859437 ("fdt: Allow the devicetree to come from a bloblist")
> Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
> ---
>  lib/fdtdec.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> index f09c9926a7a..c38738b48c7 100644
> --- a/lib/fdtdec.c
> +++ b/lib/fdtdec.c
> @@ -1708,7 +1708,7 @@ int fdtdec_setup(void)
>                                 gd->fdt_src = FDTSRC_BLOBLIST;
>                                 log_debug("Devicetree is in bloblist at %p\n",
>                                           gd->fdt_blob);
> -                               ret = 0;
> +                               goto setup_fdt;
>                         } else {
>                                 log_debug("No FDT found in bloblist\n");
>                                 ret = -ENOENT;
> @@ -1752,6 +1752,7 @@ int fdtdec_setup(void)
>                 }
>         }
>
> +setup_fdt:
>         if (CONFIG_IS_ENABLED(MULTI_DTB_FIT))
>                 setup_multi_dtb_fit();
>
> --
> 2.25.1
>

Regards,
Simon

Re: [PATCH 2/2] env: point fdt address to the fdt in a bloblist

[Simon Glass]

Hi Raymond,

On Thu, 27 Mar 2025 at 17:13, Raymond Mao <raymond.mao@linaro.org> wrote:
>
> Point fdt_addr to the fdt embedded in the bloblist since fdt_addr
> is a default address for bootefi, bootm and booti to look for the
> device tree when launching the kernel.

I wonder if we can drop use of that environment variable at some
point? It seems strange to set a variable to gd->fdt_addr only to use
it later. Why not just use gd->fdt_addr ?

I hit this in the bootstd conversion but wasn't sure what to do. So
much confusion.

I see quite a few boards setting this variable in their environment.
So this patch would override that setting, wouldn't it? It's just not
clear what any of this means.

So I think at this point we should update the code to use the internal
control FDT directly.

>
> Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
> ---
>  env/common.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/env/common.c b/env/common.c
> index a58955a4f42..a38e5d107a8 100644
> --- a/env/common.c
> +++ b/env/common.c
> @@ -16,6 +16,7 @@
>  #include <asm/global_data.h>
>  #include <linux/printk.h>
>  #include <linux/stddef.h>
> +#include <mapmem.h>
>  #include <search.h>
>  #include <errno.h>
>  #include <malloc.h>
> @@ -368,6 +369,18 @@ int env_get_default_into(const char *name, char *buf, unsigned int len)
>         return env_get_from_linear(default_environment, name, buf, len);
>  }
>
> +static int env_update_fdt_addr_from_bloblist(void)
> +{
> +       /*
> +        * fdt_addr is by default used by booti, bootm and bootefi,
> +        * thus set it to point to the fdt embedded in a bloblist if it exists.
> +        */
> +       if (!CONFIG_IS_ENABLED(BLOBLIST) || gd->fdt_src != FDTSRC_BLOBLIST)
> +               return 0;

This is just too messy, sorry.

> +
> +       return env_set_hex("fdt_addr", (uintptr_t)map_to_sysmem(gd->fdt_blob));
> +}
> +
>  void env_set_default(const char *s, int flags)
>  {
>         if (s) {
> @@ -392,6 +405,10 @@ void env_set_default(const char *s, int flags)
>
>         gd->flags |= GD_FLG_ENV_READY;
>         gd->flags |= GD_FLG_ENV_DEFAULT;
> +
> +       /* This has to be done after GD_FLG_ENV_READY is set */
> +       if (env_update_fdt_addr_from_bloblist())
> +               pr_err("Failed to set fdt_addr to point at DTB\n");
>  }
>
>  /* [re]set individual variables to their value in the default environment */
> --
> 2.25.1
>

Regards,
Simon



On Thu, 27 Mar 2025 at 17:13, Raymond Mao <raymond.mao@linaro.org> wrote:
>
> Point fdt_addr to the fdt embedded in the bloblist since fdt_addr
> is a default address for bootefi, bootm and booti to look for the
> device tree when launching the kernel.
>
> Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
> ---
>  env/common.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/env/common.c b/env/common.c
> index a58955a4f42..a38e5d107a8 100644
> --- a/env/common.c
> +++ b/env/common.c
> @@ -16,6 +16,7 @@
>  #include <asm/global_data.h>
>  #include <linux/printk.h>
>  #include <linux/stddef.h>
> +#include <mapmem.h>
>  #include <search.h>
>  #include <errno.h>
>  #include <malloc.h>
> @@ -368,6 +369,18 @@ int env_get_default_into(const char *name, char *buf, unsigned int len)
>         return env_get_from_linear(default_environment, name, buf, len);
>  }
>
> +static int env_update_fdt_addr_from_bloblist(void)
> +{
> +       /*
> +        * fdt_addr is by default used by booti, bootm and bootefi,
> +        * thus set it to point to the fdt embedded in a bloblist if it exists.
> +        */
> +       if (!CONFIG_IS_ENABLED(BLOBLIST) || gd->fdt_src != FDTSRC_BLOBLIST)
> +               return 0;
> +
> +       return env_set_hex("fdt_addr", (uintptr_t)map_to_sysmem(gd->fdt_blob));
> +}
> +
>  void env_set_default(const char *s, int flags)
>  {
>         if (s) {
> @@ -392,6 +405,10 @@ void env_set_default(const char *s, int flags)
>
>         gd->flags |= GD_FLG_ENV_READY;
>         gd->flags |= GD_FLG_ENV_DEFAULT;
> +
> +       /* This has to be done after GD_FLG_ENV_READY is set */
> +       if (env_update_fdt_addr_from_bloblist())
> +               pr_err("Failed to set fdt_addr to point at DTB\n");
>  }
>
>  /* [re]set individual variables to their value in the default environment */
> --
> 2.25.1
>

Re: [PATCH 1/2] bloblist: fix the overriding of fdt from bloblist

[Ilias Apalodimas]

On Fri, 28 Mar 2025 at 01:13, Raymond Mao <raymond.mao@linaro.org> wrote:
>
> When a bloblist is valid and contains fdt, it explicitly means
> a previous boot stage is passing transfer list compliant with
> Firmware Handoff specification, thus the fdt from bloblist should
> not be overridden with the ones from board or env variables.
>
> Fixes: 70fe23859437 ("fdt: Allow the devicetree to come from a bloblist")
> Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
> ---
>  lib/fdtdec.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> index f09c9926a7a..c38738b48c7 100644
> --- a/lib/fdtdec.c
> +++ b/lib/fdtdec.c
> @@ -1708,7 +1708,7 @@ int fdtdec_setup(void)
>                                 gd->fdt_src = FDTSRC_BLOBLIST;
>                                 log_debug("Devicetree is in bloblist at %p\n",
>                                           gd->fdt_blob);
> -                               ret = 0;
> +                               goto setup_fdt;
>                         } else {
>                                 log_debug("No FDT found in bloblist\n");
>                                 ret = -ENOENT;
> @@ -1752,6 +1752,7 @@ int fdtdec_setup(void)
>                 }
>         }
>
> +setup_fdt:
>         if (CONFIG_IS_ENABLED(MULTI_DTB_FIT))
>                 setup_multi_dtb_fit();
>
> --
> 2.25.1
>\
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH 1/2] bloblist: fix the overriding of fdt from bloblist

[Caleb Connolly]

On 3/28/25 11:44, Caleb Connolly wrote:
> Hi Raymond,
> 
> On 3/28/25 00:13, Raymond Mao wrote:
>> When a bloblist is valid and contains fdt, it explicitly means
>> a previous boot stage is passing transfer list compliant with
>> Firmware Handoff specification, thus the fdt from bloblist should
>> not be overridden with the ones from board or env variables.

Actually, I'm a little confused here. Can you point to the documentation 
that says an FDT from a bloblist should always take priority?

I can see why this makes sense to lock down, but clarification would be 
good regardless.

Kind regards,
>>
>> Fixes: 70fe23859437 ("fdt: Allow the devicetree to come from a bloblist")
>> Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
> 
> Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org>
>> ---
>>   lib/fdtdec.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
>> index f09c9926a7a..c38738b48c7 100644
>> --- a/lib/fdtdec.c
>> +++ b/lib/fdtdec.c
>> @@ -1708,7 +1708,7 @@ int fdtdec_setup(void)
>>                   gd->fdt_src = FDTSRC_BLOBLIST;
>>                   log_debug("Devicetree is in bloblist at %p\n",
>>                         gd->fdt_blob);
>> -                ret = 0;
>> +                goto setup_fdt;
>>               } else {
>>                   log_debug("No FDT found in bloblist\n");
>>                   ret = -ENOENT;
>> @@ -1752,6 +1752,7 @@ int fdtdec_setup(void)
>>           }
>>       }
>> +setup_fdt:
>>       if (CONFIG_IS_ENABLED(MULTI_DTB_FIT))
>>           setup_multi_dtb_fit();
> 

-- 
Caleb (they/them)

Re: [PATCH 1/2] bloblist: fix the overriding of fdt from bloblist

[Raymond Mao]

Hi Simon,

On Fri, 28 Mar 2025 at 07:34, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Raymond,
>
> On Thu, 27 Mar 2025 at 17:13, Raymond Mao <raymond.mao@linaro.org> wrote:
> >
> > When a bloblist is valid and contains fdt, it explicitly means
> > a previous boot stage is passing transfer list compliant with
> > Firmware Handoff specification, thus the fdt from bloblist should
> > not be overridden with the ones from board or env variables.
>
> Why is that? The board can choose to override it, if it wishes, but in
> general, if gd->fdt_blob is already set up, it should return -EEXIST
> from board_fdt_blob_setup().
>
First, if a board is running in a "stand-alone" mode, there should not
be a transfer list passed from the previous boot stage.
Secondly, overriding the transfer list with the env variables does not
make sense, since booti, bootm and bootefi are actually using the env
variables to get the fdt when launching to the kernel.
We need to do the opposite way - override the env variable with the
one pointing to the bloblist fdt (See my patch [2/2]), then all boot
commands can work with the transfer list.

Regards,
Raymond

> >
> > Fixes: 70fe23859437 ("fdt: Allow the devicetree to come from a bloblist")
> > Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
> > ---
> >  lib/fdtdec.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> > index f09c9926a7a..c38738b48c7 100644
> > --- a/lib/fdtdec.c
> > +++ b/lib/fdtdec.c
> > @@ -1708,7 +1708,7 @@ int fdtdec_setup(void)
> >                                 gd->fdt_src = FDTSRC_BLOBLIST;
> >                                 log_debug("Devicetree is in bloblist at %p\n",
> >                                           gd->fdt_blob);
> > -                               ret = 0;
> > +                               goto setup_fdt;
> >                         } else {
> >                                 log_debug("No FDT found in bloblist\n");
> >                                 ret = -ENOENT;
> > @@ -1752,6 +1752,7 @@ int fdtdec_setup(void)
> >                 }
> >         }
> >
> > +setup_fdt:
> >         if (CONFIG_IS_ENABLED(MULTI_DTB_FIT))
> >                 setup_multi_dtb_fit();
> >
> > --
> > 2.25.1
> >
>
> Regards,
> Simon

Re: [PATCH 1/2] bloblist: fix the overriding of fdt from bloblist

[Simon Glass]

Hi Raymond,

On Fri, 28 Mar 2025 at 08:17, Raymond Mao <raymond.mao@linaro.org> wrote:
>
> Hi Simon,
>
> On Fri, 28 Mar 2025 at 07:34, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Raymond,
> >
> > On Thu, 27 Mar 2025 at 17:13, Raymond Mao <raymond.mao@linaro.org> wrote:
> > >
> > > When a bloblist is valid and contains fdt, it explicitly means
> > > a previous boot stage is passing transfer list compliant with
> > > Firmware Handoff specification, thus the fdt from bloblist should
> > > not be overridden with the ones from board or env variables.
> >
> > Why is that? The board can choose to override it, if it wishes, but in
> > general, if gd->fdt_blob is already set up, it should return -EEXIST
> > from board_fdt_blob_setup().
> >
> First, if a board is running in a "stand-alone" mode, there should not
> be a transfer list passed from the previous boot stage.
> Secondly, overriding the transfer list with the env variables does not
> make sense, since booti, bootm and bootefi are actually using the env
> variables to get the fdt when launching to the kernel.
> We need to do the opposite way - override the env variable with the
> one pointing to the bloblist fdt (See my patch [2/2]), then all boot
> commands can work with the transfer list.

OK, but now we are going back to the OF_BLOBLIST thing that I did 18
months ago and ended up being the major reason I had to create my own
U-Boot tree just to get my boards working.

Anyway, Tom has agreed to let me maintain bloblist (and maybe fdt?)
again, so I am going to send a series to 'clean this up' as Ilias
would say. Let's see if that passes muster, because I do agree with
you that if the devicetree is in the bloblist, it should be
definitive.

Re your second point, do you know why we have fdt_addr? Is it just for
the distro scripts? I'm wondering if we could restrict the use of
fdt_addr to just the places that actually need to use environment
variables.

Regards,
SImon

Re: [PATCH 1/2] bloblist: fix the overriding of fdt from bloblist

[Raymond Mao]

Hi Caleb,

On Fri, 28 Mar 2025 at 08:27, Caleb Connolly <caleb.connolly@linaro.org> wrote:
>
>
>
> On 3/28/25 11:44, Caleb Connolly wrote:
> > Hi Raymond,
> >
> > On 3/28/25 00:13, Raymond Mao wrote:
> >> When a bloblist is valid and contains fdt, it explicitly means
> >> a previous boot stage is passing transfer list compliant with
> >> Firmware Handoff specification, thus the fdt from bloblist should
> >> not be overridden with the ones from board or env variables.
>
> Actually, I'm a little confused here. Can you point to the documentation
> that says an FDT from a bloblist should always take priority?
>
> I can see why this makes sense to lock down, but clarification would be
> good regardless.
>

All the adaptations on bloblist are in order to make it compliant with
the Firmware Handoff spec [1].
In the spec, all data being handed over between boot stages should be
within a transfer list (aka. bloblist in U-Boot).
So the logic is clear to me, when the bloblist contains a valid fdt,
that explicitly means the previous boot stages are passing arguments
following the Firmware Handoff spec, and U-Boot should follow the same
way to pass them to the next stage - kernel.

[1]: https://github.com/FirmwareHandoff/firmware_handoff

Regards,
Raymond

> Kind regards,
> >>
> >> Fixes: 70fe23859437 ("fdt: Allow the devicetree to come from a bloblist")
> >> Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
> >
> > Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org>
> >> ---
> >>   lib/fdtdec.c | 3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> >> index f09c9926a7a..c38738b48c7 100644
> >> --- a/lib/fdtdec.c
> >> +++ b/lib/fdtdec.c
> >> @@ -1708,7 +1708,7 @@ int fdtdec_setup(void)
> >>                   gd->fdt_src = FDTSRC_BLOBLIST;
> >>                   log_debug("Devicetree is in bloblist at %p\n",
> >>                         gd->fdt_blob);
> >> -                ret = 0;
> >> +                goto setup_fdt;
> >>               } else {
> >>                   log_debug("No FDT found in bloblist\n");
> >>                   ret = -ENOENT;
> >> @@ -1752,6 +1752,7 @@ int fdtdec_setup(void)
> >>           }
> >>       }
> >> +setup_fdt:
> >>       if (CONFIG_IS_ENABLED(MULTI_DTB_FIT))
> >>           setup_multi_dtb_fit();
> >
>
> --
> Caleb (they/them)
>

Re: [PATCH 1/2] bloblist: fix the overriding of fdt from bloblist

[Raymond Mao]

Hi Simon,

On Fri, 28 Mar 2025 at 10:26, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Raymond,
>
> On Fri, 28 Mar 2025 at 08:17, Raymond Mao <raymond.mao@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > On Fri, 28 Mar 2025 at 07:34, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Raymond,
> > >
> > > On Thu, 27 Mar 2025 at 17:13, Raymond Mao <raymond.mao@linaro.org> wrote:
> > > >
> > > > When a bloblist is valid and contains fdt, it explicitly means
> > > > a previous boot stage is passing transfer list compliant with
> > > > Firmware Handoff specification, thus the fdt from bloblist should
> > > > not be overridden with the ones from board or env variables.
> > >
> > > Why is that? The board can choose to override it, if it wishes, but in
> > > general, if gd->fdt_blob is already set up, it should return -EEXIST
> > > from board_fdt_blob_setup().
> > >
> > First, if a board is running in a "stand-alone" mode, there should not
> > be a transfer list passed from the previous boot stage.
> > Secondly, overriding the transfer list with the env variables does not
> > make sense, since booti, bootm and bootefi are actually using the env
> > variables to get the fdt when launching to the kernel.
> > We need to do the opposite way - override the env variable with the
> > one pointing to the bloblist fdt (See my patch [2/2]), then all boot
> > commands can work with the transfer list.
>
> OK, but now we are going back to the OF_BLOBLIST thing that I did 18
> months ago and ended up being the major reason I had to create my own
> U-Boot tree just to get my boards working.
>
> Anyway, Tom has agreed to let me maintain bloblist (and maybe fdt?)
> again, so I am going to send a series to 'clean this up' as Ilias
> would say. Let's see if that passes muster, because I do agree with
> you that if the devicetree is in the bloblist, it should be
> definitive.
>
> Re your second point, do you know why we have fdt_addr? Is it just for
> the distro scripts? I'm wondering if we could restrict the use of
> fdt_addr to just the places that actually need to use environment
> variables.
>

I understand that there is a clean-up plan from you, so this time I am
just trying to minimize the changes (as you can see, just a few lines)
to make the transfer list work again.
I am also working on the OP-TEE repo project to enable qemu v8 build
with transfer list enabled, so users can just use a single command to
test the Firmware Handoff feature from TF-A to kernel.
But since 70fe23859437, it is broken in U-Boot due to the overriding
of the fdt, I made this hotfix in order to let us have the feature
work again before the upcoming release.

Regards,
Raymond


> Regards,
> SImon

Re: [PATCH 2/2] env: point fdt address to the fdt in a bloblist

[Raymond Mao]

Hi Simon,

On Fri, 28 Mar 2025 at 07:35, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Raymond,
>
> On Thu, 27 Mar 2025 at 17:13, Raymond Mao <raymond.mao@linaro.org> wrote:
> >
> > Point fdt_addr to the fdt embedded in the bloblist since fdt_addr
> > is a default address for bootefi, bootm and booti to look for the
> > device tree when launching the kernel.
>
> I wonder if we can drop use of that environment variable at some
> point? It seems strange to set a variable to gd->fdt_addr only to use
> it later. Why not just use gd->fdt_addr ?
>

First, the fdtdec happens in an early stage and it is not able to set
the env variable at that time.
Secondly, the fdt_addr is commonly used to boot the kernel by booti,
bootm and bootefi, if it is not pointed to gd->fdt_addr when
gd->fdt_src is FDTSRC_BLOBLIST, it will need a big refactoring on all
boot commands to observe where it should get the correct fdt.

I don't agree with the idea that the board or env can override a valid
fdt handed off from the previous boot stage, it just messes up without
any benefits.

> I hit this in the bootstd conversion but wasn't sure what to do. So
> much confusion.
>
> I see quite a few boards setting this variable in their environment.
> So this patch would override that setting, wouldn't it? It's just not
> clear what any of this means.
>

This will be only active when "gd->fdt_src is FDTSRC_BLOBLIST" - It
explicitly indicates a previous boot stage is passing a valid fdt.
What is the purpose of dropping it?

> So I think at this point we should update the code to use the internal
> control FDT directly.
>
> >
> > Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
> > ---
> >  env/common.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/env/common.c b/env/common.c
> > index a58955a4f42..a38e5d107a8 100644
> > --- a/env/common.c
> > +++ b/env/common.c
> > @@ -16,6 +16,7 @@
> >  #include <asm/global_data.h>
> >  #include <linux/printk.h>
> >  #include <linux/stddef.h>
> > +#include <mapmem.h>
> >  #include <search.h>
> >  #include <errno.h>
> >  #include <malloc.h>
> > @@ -368,6 +369,18 @@ int env_get_default_into(const char *name, char *buf, unsigned int len)
> >         return env_get_from_linear(default_environment, name, buf, len);
> >  }
> >
> > +static int env_update_fdt_addr_from_bloblist(void)
> > +{
> > +       /*
> > +        * fdt_addr is by default used by booti, bootm and bootefi,
> > +        * thus set it to point to the fdt embedded in a bloblist if it exists.
> > +        */
> > +       if (!CONFIG_IS_ENABLED(BLOBLIST) || gd->fdt_src != FDTSRC_BLOBLIST)
> > +               return 0;
>
> This is just too messy, sorry.

This means we update the variable only when the bloblist is being used
and a valid fdt is in the bloblist which is already saved in the gd.
In other cases, the transfer list is not being used and the value from
the user will not be replaced.
Not sure why you feel messy.

Regards,
Raymond

>
> > +
> > +       return env_set_hex("fdt_addr", (uintptr_t)map_to_sysmem(gd->fdt_blob));
> > +}
> > +
> >  void env_set_default(const char *s, int flags)
> >  {
> >         if (s) {
> > @@ -392,6 +405,10 @@ void env_set_default(const char *s, int flags)
> >
> >         gd->flags |= GD_FLG_ENV_READY;
> >         gd->flags |= GD_FLG_ENV_DEFAULT;
> > +
> > +       /* This has to be done after GD_FLG_ENV_READY is set */
> > +       if (env_update_fdt_addr_from_bloblist())
> > +               pr_err("Failed to set fdt_addr to point at DTB\n");
> >  }
> >
> >  /* [re]set individual variables to their value in the default environment */
> > --
> > 2.25.1
> >
>
> Regards,
> Simon
>
>
>
> On Thu, 27 Mar 2025 at 17:13, Raymond Mao <raymond.mao@linaro.org> wrote:
> >
> > Point fdt_addr to the fdt embedded in the bloblist since fdt_addr
> > is a default address for bootefi, bootm and booti to look for the
> > device tree when launching the kernel.
> >
> > Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
> > ---
> >  env/common.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/env/common.c b/env/common.c
> > index a58955a4f42..a38e5d107a8 100644
> > --- a/env/common.c
> > +++ b/env/common.c
> > @@ -16,6 +16,7 @@
> >  #include <asm/global_data.h>
> >  #include <linux/printk.h>
> >  #include <linux/stddef.h>
> > +#include <mapmem.h>
> >  #include <search.h>
> >  #include <errno.h>
> >  #include <malloc.h>
> > @@ -368,6 +369,18 @@ int env_get_default_into(const char *name, char *buf, unsigned int len)
> >         return env_get_from_linear(default_environment, name, buf, len);
> >  }
> >
> > +static int env_update_fdt_addr_from_bloblist(void)
> > +{
> > +       /*
> > +        * fdt_addr is by default used by booti, bootm and bootefi,
> > +        * thus set it to point to the fdt embedded in a bloblist if it exists.
> > +        */
> > +       if (!CONFIG_IS_ENABLED(BLOBLIST) || gd->fdt_src != FDTSRC_BLOBLIST)
> > +               return 0;
> > +
> > +       return env_set_hex("fdt_addr", (uintptr_t)map_to_sysmem(gd->fdt_blob));
> > +}
> > +
> >  void env_set_default(const char *s, int flags)
> >  {
> >         if (s) {
> > @@ -392,6 +405,10 @@ void env_set_default(const char *s, int flags)
> >
> >         gd->flags |= GD_FLG_ENV_READY;
> >         gd->flags |= GD_FLG_ENV_DEFAULT;
> > +
> > +       /* This has to be done after GD_FLG_ENV_READY is set */
> > +       if (env_update_fdt_addr_from_bloblist())
> > +               pr_err("Failed to set fdt_addr to point at DTB\n");
> >  }
> >
> >  /* [re]set individual variables to their value in the default environment */
> > --
> > 2.25.1
> >

Re: [PATCH 1/2] bloblist: fix the overriding of fdt from bloblist

[Simon Glass]

Hi Raymond,

On Fri, 28 Mar 2025 at 08:38, Raymond Mao <raymond.mao@linaro.org> wrote:
>
> Hi Simon,
>
> On Fri, 28 Mar 2025 at 10:26, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Raymond,
> >
> > On Fri, 28 Mar 2025 at 08:17, Raymond Mao <raymond.mao@linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Fri, 28 Mar 2025 at 07:34, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Raymond,
> > > >
> > > > On Thu, 27 Mar 2025 at 17:13, Raymond Mao <raymond.mao@linaro.org> wrote:
> > > > >
> > > > > When a bloblist is valid and contains fdt, it explicitly means
> > > > > a previous boot stage is passing transfer list compliant with
> > > > > Firmware Handoff specification, thus the fdt from bloblist should
> > > > > not be overridden with the ones from board or env variables.
> > > >
> > > > Why is that? The board can choose to override it, if it wishes, but in
> > > > general, if gd->fdt_blob is already set up, it should return -EEXIST
> > > > from board_fdt_blob_setup().
> > > >
> > > First, if a board is running in a "stand-alone" mode, there should not
> > > be a transfer list passed from the previous boot stage.
> > > Secondly, overriding the transfer list with the env variables does not
> > > make sense, since booti, bootm and bootefi are actually using the env
> > > variables to get the fdt when launching to the kernel.
> > > We need to do the opposite way - override the env variable with the
> > > one pointing to the bloblist fdt (See my patch [2/2]), then all boot
> > > commands can work with the transfer list.
> >
> > OK, but now we are going back to the OF_BLOBLIST thing that I did 18
> > months ago and ended up being the major reason I had to create my own
> > U-Boot tree just to get my boards working.
> >
> > Anyway, Tom has agreed to let me maintain bloblist (and maybe fdt?)
> > again, so I am going to send a series to 'clean this up' as Ilias
> > would say. Let's see if that passes muster, because I do agree with
> > you that if the devicetree is in the bloblist, it should be
> > definitive.
> >
> > Re your second point, do you know why we have fdt_addr? Is it just for
> > the distro scripts? I'm wondering if we could restrict the use of
> > fdt_addr to just the places that actually need to use environment
> > variables.
> >
>
> I understand that there is a clean-up plan from you, so this time I am
> just trying to minimize the changes (as you can see, just a few lines)
> to make the transfer list work again.
> I am also working on the OP-TEE repo project to enable qemu v8 build
> with transfer list enabled, so users can just use a single command to
> test the Firmware Handoff feature from TF-A to kernel.
> But since 70fe23859437, it is broken in U-Boot due to the overriding
> of the fdt, I made this hotfix in order to let us have the feature
> work again before the upcoming release.

Yes I looked up that commit but it was over a year ago so I assumed
that something else was going on.

Re your project, it would be good to get that running in CI. My old
standard passage stuff had such a test and I think it was pretty easy
within U-Boot.

Anyway, this patch doesn't make things any worse.

Reviewed-by: Simon Glass <sjg@chromium.org>

Regards,
SImon

Re: [PATCH 2/2] env: point fdt address to the fdt in a bloblist

[Simon Glass]

Hi Raymond,

On Fri, 28 Mar 2025 at 09:06, Raymond Mao <raymond.mao@linaro.org> wrote:
>
> Hi Simon,
>
> On Fri, 28 Mar 2025 at 07:35, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Raymond,
> >
> > On Thu, 27 Mar 2025 at 17:13, Raymond Mao <raymond.mao@linaro.org> wrote:
> > >
> > > Point fdt_addr to the fdt embedded in the bloblist since fdt_addr
> > > is a default address for bootefi, bootm and booti to look for the
> > > device tree when launching the kernel.
> >
> > I wonder if we can drop use of that environment variable at some
> > point? It seems strange to set a variable to gd->fdt_addr only to use
> > it later. Why not just use gd->fdt_addr ?
> >
>
> First, the fdtdec happens in an early stage and it is not able to set
> the env variable at that time.

No, I mean actually drop use of the environment var, so we wouldn't
set it at all.

> Secondly, the fdt_addr is commonly used to boot the kernel by booti,
> bootm and bootefi, if it is not pointed to gd->fdt_addr when
> gd->fdt_src is FDTSRC_BLOBLIST, it will need a big refactoring on all
> boot commands to observe where it should get the correct fdt.

Sure, but I already did most of that refactoring. The variable is only
accessed in pxe_utils and efi_helper.

For efi it can just use the control FDT. It uses that as a fallback
anyway in efi_install_fdt(), so just remove the var access.

For pxe_utils I've done most of the work there, but perhaps there are
boards which use one FDT for U-Boot and a different one for booting,
both of which come from a prior stage? I'm not sure.

rpi was using it but I sent a series to fix that about 6 months ago.

>
> I don't agree with the idea that the board or env can override a valid
> fdt handed off from the previous boot stage, it just messes up without
> any benefits.

Fair enough, I agree. But then you probably shouldn't look at the rpi code.

>
> > I hit this in the bootstd conversion but wasn't sure what to do. So
> > much confusion.
> >
> > I see quite a few boards setting this variable in their environment.
> > So this patch would override that setting, wouldn't it? It's just not
> > clear what any of this means.
> >
>
> This will be only active when "gd->fdt_src is FDTSRC_BLOBLIST" - It
> explicitly indicates a previous boot stage is passing a valid fdt.
> What is the purpose of dropping it?

The env var is there for the distro scripts, I believe. If we ever
restart bootstd migrations these will go away. We should not be adding
new code to use this old mechanism.

>
> > So I think at this point we should update the code to use the internal
> > control FDT directly.
> >
> > >
> > > Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
> > > ---
> > >  env/common.c | 17 +++++++++++++++++
> > >  1 file changed, 17 insertions(+)
> > >
> > > diff --git a/env/common.c b/env/common.c
> > > index a58955a4f42..a38e5d107a8 100644
> > > --- a/env/common.c
> > > +++ b/env/common.c
> > > @@ -16,6 +16,7 @@
> > >  #include <asm/global_data.h>
> > >  #include <linux/printk.h>
> > >  #include <linux/stddef.h>
> > > +#include <mapmem.h>
> > >  #include <search.h>
> > >  #include <errno.h>
> > >  #include <malloc.h>
> > > @@ -368,6 +369,18 @@ int env_get_default_into(const char *name, char *buf, unsigned int len)
> > >         return env_get_from_linear(default_environment, name, buf, len);
> > >  }
> > >
> > > +static int env_update_fdt_addr_from_bloblist(void)
> > > +{
> > > +       /*
> > > +        * fdt_addr is by default used by booti, bootm and bootefi,
> > > +        * thus set it to point to the fdt embedded in a bloblist if it exists.
> > > +        */
> > > +       if (!CONFIG_IS_ENABLED(BLOBLIST) || gd->fdt_src != FDTSRC_BLOBLIST)
> > > +               return 0;
> >
> > This is just too messy, sorry.
>
> This means we update the variable only when the bloblist is being used
> and a valid fdt is in the bloblist which is already saved in the gd.
> In other cases, the transfer list is not being used and the value from
> the user will not be replaced.
> Not sure why you feel messy.

I understand what the code does, but here you only sometimes set the
env var, which we likely don't need anyway. You are making a
complicated mess worse with this patch, because the poor sod who comes
along after you will have an even harder time figuring out what to do.

I'm not saying you created this mess, far from it, but try to
implement your feature without going near that environment variable,
if you can.

Regards,
Simon

Re: [PATCH 2/2] env: point fdt address to the fdt in a bloblist

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1994 bytes --]

On Thu, Mar 27, 2025 at 04:13:17PM -0700, Raymond Mao wrote:

> Point fdt_addr to the fdt embedded in the bloblist since fdt_addr
> is a default address for bootefi, bootm and booti to look for the
> device tree when launching the kernel.
> 
> Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
> ---
>  env/common.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/env/common.c b/env/common.c
> index a58955a4f42..a38e5d107a8 100644
> --- a/env/common.c
> +++ b/env/common.c
> @@ -16,6 +16,7 @@
>  #include <asm/global_data.h>
>  #include <linux/printk.h>
>  #include <linux/stddef.h>
> +#include <mapmem.h>
>  #include <search.h>
>  #include <errno.h>
>  #include <malloc.h>
> @@ -368,6 +369,18 @@ int env_get_default_into(const char *name, char *buf, unsigned int len)
>  	return env_get_from_linear(default_environment, name, buf, len);
>  }
>  
> +static int env_update_fdt_addr_from_bloblist(void)
> +{
> +	/*
> +	 * fdt_addr is by default used by booti, bootm and bootefi,
> +	 * thus set it to point to the fdt embedded in a bloblist if it exists.
> +	 */
> +	if (!CONFIG_IS_ENABLED(BLOBLIST) || gd->fdt_src != FDTSRC_BLOBLIST)
> +		return 0;
> +
> +	return env_set_hex("fdt_addr", (uintptr_t)map_to_sysmem(gd->fdt_blob));
> +}

This part is fine with me and makes enough sense, we can't do this
immediately so we need some check to know to do it later.

>  void env_set_default(const char *s, int flags)
>  {
>  	if (s) {
> @@ -392,6 +405,10 @@ void env_set_default(const char *s, int flags)
>  
>  	gd->flags |= GD_FLG_ENV_READY;
>  	gd->flags |= GD_FLG_ENV_DEFAULT;
> +
> +	/* This has to be done after GD_FLG_ENV_READY is set */
> +	if (env_update_fdt_addr_from_bloblist())
> +		pr_err("Failed to set fdt_addr to point at DTB\n");
>  }

I don't know this is the right spot. This won't work when for example
the environment is valid but stored in a file on FAT (or whatever) will
it?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/2] env: point fdt address to the fdt in a bloblist

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 853 bytes --]

On Fri, Mar 28, 2025 at 05:35:04AM -0600, Simon Glass wrote:
> Hi Raymond,
> 
> On Thu, 27 Mar 2025 at 17:13, Raymond Mao <raymond.mao@linaro.org> wrote:
> >
> > Point fdt_addr to the fdt embedded in the bloblist since fdt_addr
> > is a default address for bootefi, bootm and booti to look for the
> > device tree when launching the kernel.
> 
> I wonder if we can drop use of that environment variable at some
> point? It seems strange to set a variable to gd->fdt_addr only to use
> it later. Why not just use gd->fdt_addr ?

Doubtful. Both fdt_addr and fdt_addr_r are documented with what they are
for and we'll likely never not have those as valid use cases. While it's
possible some day that we might be able to allocate valid addresses
everywhere we would still want to expose that to the user to do as they
see fit.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/2] env: point fdt address to the fdt in a bloblist

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1091 bytes --]

On Fri, Mar 28, 2025 at 11:39:13PM +0000, Simon Glass wrote:
> Hi Raymond,
> 
> On Fri, 28 Mar 2025 at 09:06, Raymond Mao <raymond.mao@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > On Fri, 28 Mar 2025 at 07:35, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Raymond,
> > >
> > > On Thu, 27 Mar 2025 at 17:13, Raymond Mao <raymond.mao@linaro.org> wrote:
> > > >
> > > > Point fdt_addr to the fdt embedded in the bloblist since fdt_addr
> > > > is a default address for bootefi, bootm and booti to look for the
> > > > device tree when launching the kernel.
> > >
> > > I wonder if we can drop use of that environment variable at some
> > > point? It seems strange to set a variable to gd->fdt_addr only to use
> > > it later. Why not just use gd->fdt_addr ?
> > >
> >
> > First, the fdtdec happens in an early stage and it is not able to set
> > the env variable at that time.
> 
> No, I mean actually drop use of the environment var, so we wouldn't
> set it at all.

The environment is for humans and also other use cases. It's not going
away.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/2] env: point fdt address to the fdt in a bloblist

[Raymond Mao]

Hi Tom and Simon,

On Fri, 28 Mar 2025 at 20:03, Tom Rini <trini@konsulko.com> wrote:
>
> On Fri, Mar 28, 2025 at 11:39:13PM +0000, Simon Glass wrote:
> > Hi Raymond,
> >
> > On Fri, 28 Mar 2025 at 09:06, Raymond Mao <raymond.mao@linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Fri, 28 Mar 2025 at 07:35, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Raymond,
> > > >
> > > > On Thu, 27 Mar 2025 at 17:13, Raymond Mao <raymond.mao@linaro.org> wrote:
> > > > >
> > > > > Point fdt_addr to the fdt embedded in the bloblist since fdt_addr
> > > > > is a default address for bootefi, bootm and booti to look for the
> > > > > device tree when launching the kernel.
> > > >
> > > > I wonder if we can drop use of that environment variable at some
> > > > point? It seems strange to set a variable to gd->fdt_addr only to use
> > > > it later. Why not just use gd->fdt_addr ?
> > > >
> > >
> > > First, the fdtdec happens in an early stage and it is not able to set
> > > the env variable at that time.
> >
> > No, I mean actually drop use of the environment var, so we wouldn't
> > set it at all.
>
> The environment is for humans and also other use cases. It's not going
> away.
>

Since fdt_addr is used as a part of boot command in numerous projects,
and it is a reality that "a fdt via bloblist" and "boot cmd with
fdt_addr" can coexist if we cannot drop the env var from the boot
command.
This patch is trying to solve this in a compatible way, allowing users
to pass a fdt_addr as a reference DT address, but U-Boot prioritizes
on the fdt from bloblist if it exists - the var fdt_addr is treated as
the final fdt address for kernel.
I think we can add some documentation to describe this further, but
generally I think the approach is simple and reasonable.

Regards,
Raymond

> --
> Tom

Re: [PATCH 1/2] bloblist: fix the overriding of fdt from bloblist

[Raymond Mao]

Hi Simon,

On Fri, 28 Mar 2025 at 11:13, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Raymond,
>
> On Fri, 28 Mar 2025 at 08:38, Raymond Mao <raymond.mao@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > On Fri, 28 Mar 2025 at 10:26, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Raymond,
> > >
> > > On Fri, 28 Mar 2025 at 08:17, Raymond Mao <raymond.mao@linaro.org> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Fri, 28 Mar 2025 at 07:34, Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Raymond,
> > > > >
> > > > > On Thu, 27 Mar 2025 at 17:13, Raymond Mao <raymond.mao@linaro.org> wrote:
> > > > > >
> > > > > > When a bloblist is valid and contains fdt, it explicitly means
> > > > > > a previous boot stage is passing transfer list compliant with
> > > > > > Firmware Handoff specification, thus the fdt from bloblist should
> > > > > > not be overridden with the ones from board or env variables.
> > > > >
> > > > > Why is that? The board can choose to override it, if it wishes, but in
> > > > > general, if gd->fdt_blob is already set up, it should return -EEXIST
> > > > > from board_fdt_blob_setup().
> > > > >
> > > > First, if a board is running in a "stand-alone" mode, there should not
> > > > be a transfer list passed from the previous boot stage.
> > > > Secondly, overriding the transfer list with the env variables does not
> > > > make sense, since booti, bootm and bootefi are actually using the env
> > > > variables to get the fdt when launching to the kernel.
> > > > We need to do the opposite way - override the env variable with the
> > > > one pointing to the bloblist fdt (See my patch [2/2]), then all boot
> > > > commands can work with the transfer list.
> > >
> > > OK, but now we are going back to the OF_BLOBLIST thing that I did 18
> > > months ago and ended up being the major reason I had to create my own
> > > U-Boot tree just to get my boards working.
> > >
> > > Anyway, Tom has agreed to let me maintain bloblist (and maybe fdt?)
> > > again, so I am going to send a series to 'clean this up' as Ilias
> > > would say. Let's see if that passes muster, because I do agree with
> > > you that if the devicetree is in the bloblist, it should be
> > > definitive.
> > >
> > > Re your second point, do you know why we have fdt_addr? Is it just for
> > > the distro scripts? I'm wondering if we could restrict the use of
> > > fdt_addr to just the places that actually need to use environment
> > > variables.
> > >
> >
> > I understand that there is a clean-up plan from you, so this time I am
> > just trying to minimize the changes (as you can see, just a few lines)
> > to make the transfer list work again.
> > I am also working on the OP-TEE repo project to enable qemu v8 build
> > with transfer list enabled, so users can just use a single command to
> > test the Firmware Handoff feature from TF-A to kernel.
> > But since 70fe23859437, it is broken in U-Boot due to the overriding
> > of the fdt, I made this hotfix in order to let us have the feature
> > work again before the upcoming release.
>
> Yes I looked up that commit but it was over a year ago so I assumed
> that something else was going on.
>

I was mainly focused on the transfer list path from TF-A through
OP-TEE to U-Boot but not much on how U-Boot handles the fdt after it
is exacted from bloblist.
I just occasionally observed this issue when I was trying to use xtest
to run the transfer list unit test I added in OP-TEE a few days ago
and found that the OP-TEE commands didn't work at all in the kernel
due to the overriding of fdt by that commit.

Regards,
Raymond

> Re your project, it would be good to get that running in CI. My old
> standard passage stuff had such a test and I think it was pretty easy
> within U-Boot.
>
> Anyway, this patch doesn't make things any worse.
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> Regards,
> SImon

Re: [PATCH 2/2] env: point fdt address to the fdt in a bloblist

[Raymond Mao]

Hi Tom,

On Fri, 28 Mar 2025 at 19:51, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Mar 27, 2025 at 04:13:17PM -0700, Raymond Mao wrote:
>
> > Point fdt_addr to the fdt embedded in the bloblist since fdt_addr
> > is a default address for bootefi, bootm and booti to look for the
> > device tree when launching the kernel.
> >
> > Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
> > ---
> >  env/common.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/env/common.c b/env/common.c
> > index a58955a4f42..a38e5d107a8 100644
> > --- a/env/common.c
> > +++ b/env/common.c
> > @@ -16,6 +16,7 @@
> >  #include <asm/global_data.h>
> >  #include <linux/printk.h>
> >  #include <linux/stddef.h>
> > +#include <mapmem.h>
> >  #include <search.h>
> >  #include <errno.h>
> >  #include <malloc.h>
> > @@ -368,6 +369,18 @@ int env_get_default_into(const char *name, char *buf, unsigned int len)
> >       return env_get_from_linear(default_environment, name, buf, len);
> >  }
> >
> > +static int env_update_fdt_addr_from_bloblist(void)
> > +{
> > +     /*
> > +      * fdt_addr is by default used by booti, bootm and bootefi,
> > +      * thus set it to point to the fdt embedded in a bloblist if it exists.
> > +      */
> > +     if (!CONFIG_IS_ENABLED(BLOBLIST) || gd->fdt_src != FDTSRC_BLOBLIST)
> > +             return 0;
> > +
> > +     return env_set_hex("fdt_addr", (uintptr_t)map_to_sysmem(gd->fdt_blob));
> > +}
>
> This part is fine with me and makes enough sense, we can't do this
> immediately so we need some check to know to do it later.
>
> >  void env_set_default(const char *s, int flags)
> >  {
> >       if (s) {
> > @@ -392,6 +405,10 @@ void env_set_default(const char *s, int flags)
> >
> >       gd->flags |= GD_FLG_ENV_READY;
> >       gd->flags |= GD_FLG_ENV_DEFAULT;
> > +
> > +     /* This has to be done after GD_FLG_ENV_READY is set */
> > +     if (env_update_fdt_addr_from_bloblist())
> > +             pr_err("Failed to set fdt_addr to point at DTB\n");
> >  }
>
> I don't know this is the right spot. This won't work when for example
> the environment is valid but stored in a file on FAT (or whatever) will
> it?
>

I can add invoking 'env_update_fdt_addr_from_bloblist()' inside
'env_import()'. It should work in all the cases then.

Regards,
Raymond

> --
> Tom

Re: [PATCH 2/2] env: point fdt address to the fdt in a bloblist

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2723 bytes --]

On Mon, Mar 31, 2025 at 10:07:29AM -0400, Raymond Mao wrote:
> Hi Tom,
> 
> On Fri, 28 Mar 2025 at 19:51, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Thu, Mar 27, 2025 at 04:13:17PM -0700, Raymond Mao wrote:
> >
> > > Point fdt_addr to the fdt embedded in the bloblist since fdt_addr
> > > is a default address for bootefi, bootm and booti to look for the
> > > device tree when launching the kernel.
> > >
> > > Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
> > > ---
> > >  env/common.c | 17 +++++++++++++++++
> > >  1 file changed, 17 insertions(+)
> > >
> > > diff --git a/env/common.c b/env/common.c
> > > index a58955a4f42..a38e5d107a8 100644
> > > --- a/env/common.c
> > > +++ b/env/common.c
> > > @@ -16,6 +16,7 @@
> > >  #include <asm/global_data.h>
> > >  #include <linux/printk.h>
> > >  #include <linux/stddef.h>
> > > +#include <mapmem.h>
> > >  #include <search.h>
> > >  #include <errno.h>
> > >  #include <malloc.h>
> > > @@ -368,6 +369,18 @@ int env_get_default_into(const char *name, char *buf, unsigned int len)
> > >       return env_get_from_linear(default_environment, name, buf, len);
> > >  }
> > >
> > > +static int env_update_fdt_addr_from_bloblist(void)
> > > +{
> > > +     /*
> > > +      * fdt_addr is by default used by booti, bootm and bootefi,
> > > +      * thus set it to point to the fdt embedded in a bloblist if it exists.
> > > +      */
> > > +     if (!CONFIG_IS_ENABLED(BLOBLIST) || gd->fdt_src != FDTSRC_BLOBLIST)
> > > +             return 0;
> > > +
> > > +     return env_set_hex("fdt_addr", (uintptr_t)map_to_sysmem(gd->fdt_blob));
> > > +}
> >
> > This part is fine with me and makes enough sense, we can't do this
> > immediately so we need some check to know to do it later.
> >
> > >  void env_set_default(const char *s, int flags)
> > >  {
> > >       if (s) {
> > > @@ -392,6 +405,10 @@ void env_set_default(const char *s, int flags)
> > >
> > >       gd->flags |= GD_FLG_ENV_READY;
> > >       gd->flags |= GD_FLG_ENV_DEFAULT;
> > > +
> > > +     /* This has to be done after GD_FLG_ENV_READY is set */
> > > +     if (env_update_fdt_addr_from_bloblist())
> > > +             pr_err("Failed to set fdt_addr to point at DTB\n");
> > >  }
> >
> > I don't know this is the right spot. This won't work when for example
> > the environment is valid but stored in a file on FAT (or whatever) will
> > it?
> >
> 
> I can add invoking 'env_update_fdt_addr_from_bloblist()' inside
> 'env_import()'. It should work in all the cases then.

That's probably closer? I'm not sure if it will or won't be a problem on
say re-import with "env import .." in that case, so please check.
Thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]