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

[Ilias Apalodimas]

Hi Simon,

[...]

> > > >
> > > > 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.

I think it is limited. The help message says 'Provider of DTB for DT
control' and OF_BOARD help message is 'Provided by the board (e.g a
previous loader) at runtime '.
In fact, that's exactly what I tried to clean up with commit
e7fb789612e39.  Sandbox had its special OF_HOSTFILE for the DT.  We
cleaned that up and then reintroduced it with a different name in
commit 275b4832f6bf91c.

In any case, if people want to do 'more' we have
CONFIG_OF_BOARD_SETUP/FIXUP which should be used instead for other
platform-specific stuff.  While at it OF_PRIOR_STAGE is a much better
name that OF_BOARD.  So we could get rid of OF_PRIOR_STAGE and rename
OF_BOARD to that.

The function name that is called in the setup phase is
board_fdt_blob_setup(), so it's explicitly targeting the fdt setup, or
at least that's what the name suggets. Grepping for CONFIG_OF_BOARD
matches in
board/AndesTech/ae350/ae350.c
board/armltd/vexpress64/vexpress64.c
board/raspberrypi/rpi/rpi.c
board/sifive/unleashed/unleashed.c
board/sifive/unmatched/unmatched.c
board/starfive/visionfive2/starfive_visionfive2.c
board/xilinx/common/board.c:
All these just use it to setup the DT, apart from the rpi which
additionally sets up some memory for the DT.

Apart from that it's used on a few platform drivers to setup so DM
flags (most of them set DM_FLAG_PRE_RELOC), but we can easily change
that and use The only affected files are
drivers/pinctrl/broadcom/pinctrl-bcm283x.c
drivers/serial/serial_bcm283x_mu.c
drivers/serial/serial_bcm283x_pl011.c

So making OF_BOARD do the right thing is trivial and I can work with
you on that.  I don't think it's too much effort anyway

>
> 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.

We are on the same page regarding the standardization.  But I think
shoehorning an OF_BLOBLIST just papers over the problem.
The board should ideally have 2 options
1. a previous loader gives me the DT in X random ways (one of them is
the bloblist)
2. U-Boot provides it because it was built with it
A user can always load his own special DT if he wants to, but that's
done at a later stage.

But if we clean it up properly we can
1. Do Not allow people to sneak in random options in OF_BOARD or
whatever and keep the options sane and clean
2. enable BLOBLIST by default in all supported platforms and push to
make it the default.  We can even be stricter and crash on boot if
BLOBLIST is selected and we can't find the DT in there.  That would
have a similar effect to selecting it as a Kconfig option

Thanks
/Ilias


>
> >
> > 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