[PATCH v2 30/32] fdt: Allow the devicetree to come from a bloblist
Simon Glass
sjg at chromium.org
Tue Sep 26 14:38:21 CEST 2023
Hi Ilias,
On Tue, 26 Sept 2023 at 06:32, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> Hi Simon
>
> On Tue, 26 Sept 2023 at 14:37, Simon Glass <sjg at chromium.org> wrote:
> >
> > 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.
>
> 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
OK I see...yes that is along the lines of what I thought you meant.
But OF_BOARD does not mean 'previous stage loader hands me over the
DT'. I means call board_fdt_blob_setup() which could do anything. If
some boards use that function to implement getting a DT from the prior
stage, that's fine, but it isn't limited to that.
There is an HAS_PRIOR_STAGE option which sets OF_BOARD at present. But
that doesn't seem right in the long term.
The goal here is to standardise the passing of DT from a prior stage,
so that all boards (except perhaps the dark child rpi) use standard
passage (bloblist) to do so. That should not depend on OF_BOARD and in
fact I would like to make OF_BOARD the exception, used when
OF_BLOBLIST doesn't suit.
>
> 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,
Simon
More information about the U-Boot
mailing list