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

[Simon Glass]

Hi Ilias,

On Tue, 26 Sept 2023 at 07:13, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> 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.

That commit adds HAS_PRIOR_STATE, though, not anything to do with sandbox.

OK. Perhaps it is just the OF_BOARD name that I object to?

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

To do that, we have to work out what to do with board-specific setup.

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

What is the right thing you refer to here?

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

OK

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

I am certainly keen to remove OF_BOARD, or print a warning when it is used.

> 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

Yes, that's what I am doing with this patch. So we end up with:

OF_SEPARATE - packed with U-Boot
OF_BLOBLIST - comes in a bloblist
OF_BOARD - escape hatch, please don't use

Regards,
Simon



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