[PATCH v2 15/18] fdt: Allow the devicetree to come from a bloblist

Fri Dec 8 15:50:43 CET 2023

Hi Simon,
OK, I can try to remove them into two series.

Regards,
Raymond

On Thu, 7 Dec 2023 at 22:14, Simon Glass <sjg at chromium.org> wrote:

> Hi Raymond,
>
> On Wed, 6 Dec 2023 at 13:35, Raymond Mao <raymond.mao at linaro.org> wrote:
> >
> > Hi Simon,
> >
> > > The other thing missing here is SPL. The bloblist is designed such
> > > that it can be set up in any phase of U-Boot, then is passed to
> > > following phases. So using IS_ENABLED(BLOBLIST) doesn't do what we
> > > need. As I noted, it breaks sandbox_spl. What happens if a board wants
> > > to use bloblist in SPL but doesn't want the SPL DT to come from a
> > > prior stage? What happens if SPL wants doesn't want to pass the U-Boot
> > > DT through to U-Boot in the bloblist? It is all just horribly
> > > confusing.
> > I think the logic to decide whether to take the DT from bloblist really
> depends
> > on the board vendor.
> > I will move this kind of logics into board specific functions from
> fdtdec.c in the
> > next revision.
> > Board vendors can decide the behaviors they want. This can answer the
> question
> > above and for example whether they want a fallback when no DT exists in
> the
> > bloblist or the DT from bloblis is corrupted, or just print a warning
> message.
> > For Qemu-arm, as an example of design reference, I prefer to take the DT
> from the
> > bloblist automatically when (BLOBLIST && OF_BOARD) and keep the fallback
> > compatibility to load from `CFG_SYS_SDRAM_BASE` when DT from bloblist is
> > corrupted.
> > I will take a look into the sandbox_spl failures you mentioned.
>
> In the interests of my sanity, could you send just the bloblist
> updates and move the other patches into a separate series?
>
> Regards,
> SImon
>
>
> >
> > Thanks and regards,
> > Raymond
> >
> >
> > On Tue, 5 Dec 2023 at 22:57, Simon Glass <sjg at chromium.org> wrote:
> >>
> >> Hi Ilias,
> >>
> >> On Mon, 4 Dec 2023 at 23:22, Ilias Apalodimas
> >> <ilias.apalodimas at linaro.org> wrote:
> >> >
> >> > Hi Simon,
> >> >
> >> > We did discuss this in OSFC but perhaps you forgot.  The discussion
> >> > was based on the mail here [0].
> >>
> >> Perhaps I did? Or perhaps we had a different understanding of it?
> >>
> >> >
> >> > On Tue, 5 Dec 2023 at 02:52, Simon Glass <sjg at chromium.org> wrote:
> >> > >
> >> > > Hi Raymond,
> >> > >
> >> > > On Mon, 4 Dec 2023 at 12:25, Raymond Mao <raymond.mao at linaro.org>
> wrote:
> >> > > >
> >> > > > Hi Simon,
> >> > > >
> >> > > > When `OF_BOARD` is defined, the FDT should be from one of  the
> board-specific mechanisms:
> >> > > > 1. FDT from a bloblist via boot args
> >> > > > 2. FDT in memory
> >> > > > I don't think we need a new build option to distinguish these
> two, since it can be done in runtime by checking whether the boot args
> follow the FW Handoff spec conventions and the FDT exists in the bloblist.
> >> > >
> >> > > No, I would really like this to be deterministic, so we can fail if
> >> > > something goes wrong. The bloblist should be 'declared' as the way
> to
> >> > > boot, for a board.
> >> >
> >> > We *are* deterministic. I am just going to repeat what we discussed in
> >> > OSFC for completeness.
> >> > OF_BLOBLIST makes little sense because we are not going to add a
> >> > Kconfig per blolblist entry.
> >> > I haven't looked at what the patch does yet but the idea was really
> simple.
> >> > - Under OF_BOARD (which can arguably be renamed to OF_PREVIOUS_STAGE
> >> > or something like that)  if CONFIG_BLOB is enabled we scan for a
> >> > bloblist
> >> > - If CONFIG_BLOB is enabled but no bloblist is provided we fail the
> boot
> >> > - If CONFIG_BLOB is not enabled we do what we did up to now, maybe
> >> > with a message that this is going to be obsoleted once enough boards
> >> > migrate to a bloblist
> >> >
> >> > IOW if support for a bloblist is enabled we expect to find one from
> >> > the previous bootloader. I've lost count of how many times I repeated
> >> > this, but here it goes again
> >> > The device tree can come
> >> > - From u-boot
> >> > - From a previous stage loader *somehow* (eg. passed on a bloblist,
> >> > passed on a register, read from an EEPROM etc)
> >> >
> >> > We should keep that distinction and create subcategories for the
> >> > 'comes from a previous stage loader', rather than adding arbitrary
> >> > config options which only confuse people
> >>
> >> I don't know why you are so frustrated about this. This patch does not
> >> even do what you have listed above, so far as I can tell.
> >>
> >> We have:
> >> - OF_HAS_PRIOR_STAGE which indicates that U-Boot is not the first phase
> >> - OF_BOARD which indicates that the board can do anything it likes
> >> - BLOBLIST which indicates that there is a bloblist
> >>
> >> I proposed adding:
> >> - OF_BLOBLIST which indicates that U-Boot gets the DT from the bloblist
> >>
> >> I understood that your intention OF_BOARD would go away since there
> >> are almost no cases that need it. I was skeptical it could be done but
> >> I definitely agreed (and still do) that it would be great if that
> >> could go away, as we would not need OF_BLOBLIST since OF_BOARD would
> >> effectively mean the same thing.
> >>
> >> But when I look at this patch, that is not what is happening. In fact,
> >> there is no indication that the DT came from the bloblist. U-Boot will
> >> just report 'devicetree: board'. We must say where it came from.
> >>
> >> The other thing missing here is SPL. The bloblist is designed such
> >> that it can be set up in any phase of U-Boot, then is passed to
> >> following phases. So using IS_ENABLED(BLOBLIST) doesn't do what we
> >> need. As I noted, it breaks sandbox_spl. What happens if a board wants
> >> to use bloblist in SPL but doesn't want the SPL DT to come from a
> >> prior stage? What happens if SPL wants doesn't want to pass the U-Boot
> >> DT through to U-Boot in the bloblist? It is all just horribly
> >> confusing.
> >>
> >> So I believe, in fact, that we cannot get rid of OF_BOARD now, and
> >> perhaps for quite a long time. There is another discussion about
> >> Qualcomm where they want a gzipped DT attached to the end of U-Boot,
> >> for example. Perhaps the key problem with this patch is that it would
> >> ensure that OF_BOARD sticks around, since it means both a) Get the DT
> >> from bloblist if BLOBLIST; b) Get the DT from a board-specific
> >> mechanism if !BLOBLIST. That is not what we discussed.
> >>
> >> Why not just take my original patch and work from there? It will save
> >> a lot of time. We can then progressively migrate boards from OF_BOARD
> >> to OF_BLOBLIST and then perhaps one day remove OF_BOARD (I remain
> >> skeptical, but hopeful). The whole 'standard passage' thing [1] was
> >> carefully thought out *two years ago* and (I believe) dealt with all
> >> of the issues above.
> >>
> >> For now, we should apply the patches which bring in the spec changes,
> >> so we can figure out the rest of it. I have added my review tag for
> >> those.
> >>
> >> Regards,
> >> SImon
> >> >
> >> > Regards
> >> > /Ilias
> >> > >
> >> > > If in the future all boards a reusing bloblist for this feature,
> then
> >> > > sure, we can drop it. But we have things like rpi which do their own
> >> > > thing.
> >> > >
> >> > > > If it fulfills the above conditions, we can take the FDT from
> bloblist, otherwise from the predefined memory address.
> >> > > > This is fully backward compatible without adding a new build
> option.
> >> > >
> >> > > Again, we need to obtain the FDT from the bloblist if and only if
> the
> >> > > board requires it. It creates a lot of confusion to have it as an
> >> > > optional feature, when we know which way it should be.
> >> > >
> >> > > Ultimately OF_BOARD should go away. We just need to push bloblist to
> >> > > other projects and closed-source firmware as the correct way to
> pass a
> >> > > DT.
> >> > >
> >> > > Regards,
> >> > > Simon
> >> > > [.]
> >> >
> >> > [0]
> https://lore.kernel.org/u-boot/CAPnjgZ0SqVyj_drLEQrP21zPYCco3HOP-rYD+nKJMa0BVYpMDA@mail.gmail.com/
> >>
> >> [1]
> https://patchwork.ozlabs.org/project/uboot/list/?series=281465&state=*
>