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

[Ilias Apalodimas]

Hi Simon,

On Wed, 6 Dec 2023 at 05: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.

Because I cleaned this up in 2e8d2f88439d, 2ea63271e522f, and
d6f8ab30a2af46. HAS_OF_PRIOR_STAGE then magically reappeared in
275b4832f6bf91 without anyone acking or reviewing that patch and at
the same time it does nothing useful apart from a few prints. Also
because we keep going in circles. We've spent more time discussing
this than coding it.
Instead of cleaning this up now that we have the chance, we prefer
adding more DT options until we end up with a complete mess again.

> This patch does not even do what you have listed above, so far as I can tell.

Why? It scans for a bloblist if CONFIG_BLOBLIST is turned on,
otherwise, it falls back to board_fdt_blob_setup(). Another thing we
can do is add an 'imply OF_BLOBLIST' if BLOBLIST && OF_BOARD are
selected and use that in our if cases.

We can also change this in the future and prevent the board from
booting if (IS_ENABLED(OF_BLOBLIST)) fails to discover a DT. But I
think that should be done once we have some boards implementing a
bloblist. I had the same suggestion here [0].

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

We also have OF_EMBED and OF_SEPARATE. Funnily OF_EMBED says 'not
recommended' yet we have a ton of devices using it. On top of that, we
have CONFIG_OF_BOARD_SETUP and CONFIG_OF_BOARD_FIXUP that people just
use in random ways. CONFIG_OF_BOARD_FIXUP is obviously the right
option (and properly documented) to fixup DTs

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

My suggestion was to rename and clean stuff if OF_BOARD is confusing,
not remove it.  It certainly can't go away, because you will still
have boards reading it of an EEPROM or similar.[1]

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

Yes, but we are confusing 2 things here. What do we need to print vs
what the user needs to configure. The answer to this problem is not to
add more Kconfig options just to be able to print stuff. An enum to
the DT struct which would be set to whatever option we need to print
during the DT init phase is cleaner and keeps the Kconfig options
sane.

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

Can you explain 'bloblist in SPL'? Do you mean SPL gets the DT somehow
but then wants to add it in a bloblist for U-Boot?
I don't think we should be allowing things like that. You either use a
bloblist or you don't.

>  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 am not sure I understand this. Do you mean that the SPL wouldn't
want to pass the DT in a bloblist to U-Boot proper? If that's the
case, that's a design decision we have to make and I prefer
prohibiting that if BLOBLIST is enabled.

>
> So I believe, in fact, that we cannot get rid of OF_BOARD now, and
> perhaps for quite a long time.

We can't. Ever.

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

I haven't gone through that patch again, but I will today. OTOH I
don't see a problem with this. This does exactly what you asked, a
discrete way of getting the DT from a bloblist. Instead of adding yet
another option it relies on  if(BLOBLIST && OF_BOARD) (== OF_BLOBLIST)
Vendors need time to implement standards and prohibiting their boards
from working until they do make little sense to me. I am fine with the
fallback scenario for now.

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

I think I prefer this iteration (with the imply OF_BLOBLIST perhaps)
with a future cleanup in mind which I tried to explain in the past
[1]. A few extra suggestions...

- Get rid of OF_PRIOR_STAGE which is only used in a few prints.
Instead add an enum with the options you want to print, which we set
during the DT setup.
- Rename OF_BOARD to OF_EXTERNAL or something similar if it's still
confusing. If (OF_EXTERNAL && BLOBLIST) you have your OF_BLOBLIST
equivalent, without having people to have to explicitly configure it.
- Once the above is implemented fail to boot boards that have
(OF_EXTERNAL && BLOBLIST) enabled, but don't provide a DT in a
bloblist (we can even do that now, but I don't see the point till we
get users)
- See if we can get rid of CONFIG_OF_BOARD_SETUP or repurpose it to do
something board-specific if needed.  Looking at it we can probably
replace all the callsites with CONFIG_OF_BOARD_FIXUP

Then *the user* ends up with
- OF_EMBED which shouldn't really be used
- OF_SEPARATE which means the DT is appended to the U-Boot binary, but
comes from U-Boot.
- OF_EXTERNAL which means we can get the DT in a number of external
ways (EEPROM, BLOBLIST, CPU registers etc).

[...]

[0] https://lore.kernel.org/u-boot/CAC_iWjLS62kkMOYT6UiGoL66o-CaFst1nYWv7xgH+GNyeuaz9A@mail.gmail.com/
[1] https://lore.kernel.org/u-boot/CAC_iWj+hkVjftjfVfRFd27uF59=cpsJ0r+6jCg++EYFqZjBZyQ@mail.gmail.com/

Thanks
/Ilias

/Ilias