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
