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