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

[Simon Glass]

Hi Ilias,

On Mon, 2 Oct 2023 at 01:33, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> Hi Simon
>
> On Mon, 2 Oct 2023 at 04:23, Simon Glass <sjg at chromium.org> wrote:
> >
> > 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
>
> That's not what I am suggesting though.  Let me try and sum it up in
> case it helps
>
> - Remove OF_PRIOR_STAGE
> - Rename OF_BOARD -> OF_PRIOR_STAGE
> - Add the bloblist scanning code for DT under the OF_PRIOR_STAGE

What happens if the bloblist does not contain a DT?

What do we do with boards with have a magic way of getting the DT, not
using bloblist?

Regards,
Simon


>
> Thanks
> /Ilias
> >
> > 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