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

[Ilias Apalodimas]

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

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