[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 at linaro.org> wrote:
>
> Hi Simon,
>
> On Fri, 28 Mar 2025 at 07:35, Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Raymond,
> >
> > On Thu, 27 Mar 2025 at 17:13, Raymond Mao <raymond.mao at 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 at 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