[PATCH v2 30/32] fdt: Allow the devicetree to come from a bloblist

[Simon Glass]

Hi Ilias,

On Mon, 25 Sept 2023 at 13:48, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> Hi Simon,
>
> I commented on the v1 thread, but let's continue the discussion here
>
> On Thu, 21 Sept 2023 at 04:58, Simon Glass <sjg at chromium.org> wrote:
> >
> > Standard passage provides for a bloblist to be passed from one firmware
> > phase to the next. That can be used to pass the devicetree along as well.
> > Add an option to support this.
> >
> > Tests for this will be added as part of the Universal Payload work.
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > ---
> >
> > Changes in v2:
> > - No changes as it still seems unclear what should be done
> >
>
> [...]
>
> >
> >         /* BLOBLISTT_VENDOR_AREA */
> > diff --git a/doc/develop/devicetree/control.rst b/doc/develop/devicetree/control.rst
> > index cbb65c9b177f..56e00090166f 100644
> > --- a/doc/develop/devicetree/control.rst
> > +++ b/doc/develop/devicetree/control.rst
> > @@ -108,6 +108,9 @@ If CONFIG_OF_BOARD is defined, a board-specific routine will provide the
> >  devicetree at runtime, for example if an earlier bootloader stage creates
> >  it and passes it to U-Boot.
> >
> > +If CONFIG_OF_BLOBLIST is defined, the devicetree comes from a bloblist passed
> > +from a previous stage.
>
> What I argued before is that we don't need to be this explicit.
> The bloblist can carry a bunch of options that might be used by
> U-Boot.  It would be better if we had a more generic approach instead
> of adding Kconfig options per bloblist entry

Yes, fair enough. It would really get out of hand if we had a lot of
Kconfig options for everything that could be in a bloblist.

I believe someone objected to this in the other thread, so there may
be board-specific issues to resolve.

>
> [...]
>
> >  #ifndef USE_HOSTCC
> > +
> > +#define LOG_CATEGORY   LOGC_DT
> > +
> >  #include <common.h>
> > +#include <bloblist.h>
> >  #include <boot_fit.h>
> >  #include <display_options.h>
> >  #include <dm.h>
> > @@ -87,6 +91,7 @@ static const char *const fdt_src_name[] = {
> >         [FDTSRC_BOARD] = "board",
> >         [FDTSRC_EMBED] = "embed",
> >         [FDTSRC_ENV] = "env",
> > +       [FDTSRC_BLOBLIST] = "bloblist",
> >  };
> >
> >  const char *fdtdec_get_srcname(void)
> > @@ -1666,20 +1671,35 @@ int fdtdec_setup(void)
> >         int ret;
> >
> >         /* The devicetree is typically appended to U-Boot */
> > -       if (IS_ENABLED(CONFIG_OF_SEPARATE)) {
> > -               gd->fdt_blob = fdt_find_separate();
> > -               gd->fdt_src = FDTSRC_SEPARATE;
> > -       } else { /* embed dtb in ELF file for testing / development */
> > -               gd->fdt_blob = dtb_dt_embedded();
> > -               gd->fdt_src = FDTSRC_EMBED;
> > -       }
> > -
> > -       /* Allow the board to override the fdt address. */
> > -       if (IS_ENABLED(CONFIG_OF_BOARD)) {
> > -               gd->fdt_blob = board_fdt_blob_setup(&ret);
> > +       if (CONFIG_IS_ENABLED(OF_BLOBLIST)) {
> > +               ret = bloblist_maybe_init();
> >                 if (ret)
> >                         return ret;
> > -               gd->fdt_src = FDTSRC_BOARD;
>
> So, instead of adding OF_BLOBLIST, just move this code under OF_BOARD,
> inside an IS_ENABLED(BLOBLIST) check. If a bloblist is required and
> the previous stage loader is supposed to provide a DT we can just
> throw an error and stop booting

This is the bit I don't get.

The OF_BOARD thing is a hack, in that the board can do what it likes.
It is our way of handling board-specific mechanisms.

But I am wanting a standard mechanism, i.e. like 'standard passage', a
way of passing the DT through the phases.

If I put this under OF_BOARD, then the board gets to override the
mechanism, so which is it?

You say 'we can just throw and error' but what is the error? If the DT
is provided via the bloblist, there is no error. If it is not, I
already show an error and halt as you can see in the code below.

I guess I'm just confused about what you are saying here.

>
>
> > +               gd->fdt_blob = bloblist_find(BLOBLISTT_CONTROL_FDT, 0);
> > +               if (!gd->fdt_blob) {
> > +                       printf("Not FDT found in bloblist\n");
> > +                       bloblist_show_list();
> > +                       return -ENOENT;
> > +               }
>
> [...]
>
> Regards
> /Ilias

Regards,
Simon