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

[Simon Glass]

Hi Ilias,

On Fri, 20 Oct 2023 at 01:22, Ilias Apalodimas <ilias.apalodimas at linaro.org>
wrote:
>
> Hi Simon,
>
> On Wed, 18 Oct 2023 at 18:26, Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Mon, 25 Sept 2023 at 04:19, Ilias Apalodimas
> > <ilias.apalodimas at linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > [...]
> > >
> > > > > > >>>>>>>>>> +config OF_BLOBLIST
> > > > > > >>>>>>>>>> +     bool "DTB is provided by a bloblist"
> > > > > > >>>>>>>>>> +     help
> > > > > > >>>>>>>>>> +       Select this to read the devicetree from the
bloblist. This allows
> > > > > > >>>>>>>>>> +       using a bloblist to transfer the devicetree
between  U-Boot phases.
> > > > > > >>>>>>>>>> +       The devicetree is stored in the bloblist by
an early phase so that
> > > > > > >>>>>>>>>> +       U-Boot can read it.
> > > > > > >>>>>>>>>> +
> > > > > > >>>>>>>>>
> > > > > > >>>>>>>>> I dont think this is a good idea.  We used to have
4-5 different options
> > > > > > >>>>>>>>> here, which we tried to clean up and ended up with
two very discrete
> > > > > > >>>>>>>>> options.  Why do we have to reintroduce a new one?
Doesn't that bloblist
> > > > > > >>>>>>>>> have a header of some sort?  The bloblist literally
comes from a previous
> > > > > > >>>>>>>>> stage bootloader which is what OF_BOARD is here for.
So why can't we just
> > > > > > >>>>>>>>> read the header and figure out if the magic of the
bloblist matches?
> > > > > > >>>>>>>>
> > > > > > >>>>>>>> No, OF_BOARD is a hack to allow boards to do what they
like with the FDT.
> > > > > > >>>>>>>>
> > > > > > >>>>>>>> This patch is a standard mechanism to pass the DT from
one firmware
> > > > > > >>>>>>>> phase to the next. We have spent quite a bit of time
creating a spec
> > > > > > >>>>>>>> for it, and we should use it.
> > > > > > >>>>>>>
> > > > > > >>>>>>> Where exactly am I objecting using the spec?   Can you
please re-read my email?
> > > > > > >>>>>>> I am actually pointing out we should use the spec
*properly*.  So
> > > > > > >>>>>>> instead of having a Kconfig option for the DT, which is
pretty
> > > > > > >>>>>>> pointless,  we should parse the bloblist.  If the
header defined by
> > > > > > >>>>>>> the *spec* is found, we should just search for the DT
in there.
> > > > > > >>>>>>> What you are doing here, is take the spec, pick a very
specific item
> > > > > > >>>>>>> that the list contains, and create a Kconfig option out
of it.  Which
> > > > > > >>>>>>> basically ignores the discoverable options of the
bloblist.  For
> > > > > > >>>>>>> example, that bloblist can also contain an entry to a
TPM eventlog.
> > > > > > >>>>>>> Should we start creating Kconfig options for all the
firmware handoff
> > > > > > >>>>>>> entries that are defined on that spec?
> > > > > > >>>>>>
> > > > > > >>>>>> OK so that is a different thing. What should it do if it
expects to find a bloblist but cannot? I want it to throw an error, because
I am trying to make the boot deterministic. What do you think?
> > > > > > >>>>>
> > > > > > >>>>> That's fine by me.  You can even put that under
IS_ENABLED for the
> > > > > > >>>>> bloblist inside the existing OF_BOARD check.  So I was
thinking
> > > > > > >>>>> - If no bloblist is required in Kconfig options we do the
hacks we used to
> > > > > > >>>>> - if bloblist is selected and the config option is
OF_BOARD, throw an
> > > > > > >>>>> error and mention that the previous stage loader should
hand over a DT
> > > > > > >>>>>
> > > > > > >>>>> Is that what you had in mind?
> > > > > > >>>>
> > > > > > >>>> Yes, that sounds good to me.
> > > > > > >>>>
> > > > > > >>>> But I still think we need an OF_BLOBLIST option to control
whether the
> > > > > > >>>> devicetree comes from there.
> > > > > > >>>>    Otherwise we will end up with people
> > > > > > >>>> using OF_BOARD to work around it. We also have the SPL
case which does
> > > > > > >>>> not pass the DT in a bloblist...in fact SPL typically
doesn't even
> > > > > > >>>> have the full DT.
> > > > > > >>>
> > > > > > >>> Wouldn't IS_ENABLED(BLOBLIST) || IS_ENABLED(SPL_BLOBLIST)
be enough?
> > > > > > >>> Inside the OF_BOARD portion of the function, search for a
bloblist if
> > > > > > >>> the option is enabled.  If you can't find a bloblist and
the DT within
> > > > > > >>> that bloblist, then error out
> > > > > > >>
> > > > > > >> Just my 2c here.
> > > > > > >> I think all options should be possible to disable. It means
I can imagine to
> > > > > > >> disable u-boot not to take care about DT provided from
previous stage. The same
> > > > > > >> is for TPM event log. IMHO every stage should have an option
to simply ignore
> > > > > > >> data pass from previous stage. I don't really mind how it is
done but there
> > > > > > >> should be an option to by purpose say to ignore the part of
pass data.
> > > > > > >
> > > > > > > That would be done by disabling BLOBLIST.  I don't think
having a Kconfig to say
> > > > > > > "I want a bloblist, but I only want the DT from there" is
reasonable
> > > > > > > (or for any other item the bloblist can carry).   OF_BOARD
means the
> > > > > > > DT will come from a previous stage loader. If bloblist is
enabled then
> > > > > > > the DT must come from there.
> > > > > >
> > > > > > I don't agree on this. If bloblist is enabled and DT is passed
SW should have a
> > > > > > freedom to ignore it.
> > > > > >
> > > > > > At start everything is likely in sync but later stages before
U-Boot can stay at
> > > > > > certain version but there could be a need to update u-boot
where DT from
> > > > > > previous stage could be broken.
> > > > >
> > > > > But you *can* ignore it and load a different one later.  The only
> > > > > restriction is that if you compile u-boot with the assumption an
early
> > > > > stage bootloader provides a DT you *must* find it.  But we will
still
> > > > > just keep 2 options in U-Boot of how you get a DT.
> > > > > A previous loader provided it or U-Boot provided it.  Whether that
> > > > > comes from a bloblist or a register is irrelevant no ?
> > > >
> > > > I'm not sure what is being requested here, so I'll leave this as is
for v2.
> > >
> > > Please don't.   A few mails above there's a discussion of how I'd
> > > prefer things to look like, please have a look and let me know if
> > > something isn't clear.
> > > tl;dr
> > >  "That's fine by me.  You can even put that under IS_ENABLED for the
> > >  bloblist inside the existing OF_BOARD check.  So I was thinking
> > >  - If no bloblist is required in Kconfig options we do the hacks we
used to
> > >  - if bloblist is selected and the config option is OF_BOARD, throw an
> > > error and mention that the previous stage loader should hand over a
DT"
> > >
> > > >
> > > > The main struggle I have is how to tell whether you expect to
> > > > *receive* the DT in the bloblist, or expect it to be attached to the
> > > > image and be *sent* to the next phase.
> > >
> > > bloblist
> > >
> > > >
> > > > SPL - attached to image, send in bloblist
> > > > U-Boot proper - not attached to image, receive it from bloblist
> > > >
> > > > This is exactly the problem that is solved by the 'standard passage'
> > > > stuff [1] but that depends on Firmware Handoff and [2] which are not
> > > > ready yet...
> > > >
> > > > So I think what I have is the best we can do for now.
> > >
> > > We can avoid the extra complication in Kconfigs.  The DT either comes
> > > from u-boot itself or from a previous stage loader. we don't need the
> > > extra "it comes from a bloblist".
> > > If they come from a previous stage loader and BLOBLIST *is* enabled,
> > > then just scan for the DT, if you don't find it error out.
> >
> > Are you planning to send a patch for this? Otherwise, what do you
> > think about going with this one and dealing with OF_BOARD board by
> > board?
>
> Yes, I can do that.  Since this patch is part of your series, I'll
> send it over to you and carry it there

Thanks. The rest of it was applied, but we didn't want to apply this one
due to the discussion. So you can just send the patch.

But I see Tom already said this.

Regards,
Simon