Hi Simon

On Tue, 26 Sept 2023 at 14:37, Simon Glass <s...@chromium.org> wrote:
>
> Hi Ilias,
>
> On Mon, 25 Sept 2023 at 13:48, Ilias Apalodimas
> <ilias.apalodi...@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 <s...@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 <s...@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.

I had the same objection to that first version as well

>
> >
> > [...]
> >
> > >  #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?

No, it's the other way around in my head.  OF_BOARD description is 'a
previous stage loader hands me over the DT', which is a superset of
the bloblist.
Whether it comes in a firmware handoff format, or a hacky register the
previous bootloader filled in is a detail we have to deal with and we
need to keep backwards compatibility.

Maybe adding a coding snip would help
if (IS_ENABLED(CONFIG_OF_BOARD)) {
    if (CONFIG_IS_ENABLED(BLOBLIST)) { <- This instead of OF_BLOBLIST
        ret = bloblist_maybe_init();
        if (ret)
            return ret;
        /* Dynamically scan for a DT in the bloblist. */
        gd->fdt_blob = bloblist_find(BLOBLISTT_CONTROL_FDT, 0);
        if (!gd->fdt_blob) {
            printf("Not FDT found in bloblist\n");
            bloblist_show_list();
           // We can choose to not return an error here and keep
scanning in case the DT is in a register, but I am fine with both
            return -ENOENT;
        }
       gd->fdt_src = FDTSRC_BLOBLIST;
       bloblist_show_list();
       log_debug("Devicetree is in bloblist at %p\n", gd->fdt_blob);
      // We can also bail out of this entirely if we do find a DT via
a bloblist.
     } else {
         gd->fdt_blob = board_fdt_blob_setup(&ret);
         if (ret)
             return ret;
         gd->fdt_src = FDTSRC_BOARD;
    }
}

I haven't even compiled the code above, but it should give you a
better idea of what I am suggesting

Hope that helps
/Ilias
>
> 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

Reply via email to