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

[Ilias Apalodimas]

Hi Michal,

On Mon, 11 Sept 2023 at 09:38, Michal Simek <michal.simek at amd.com> wrote:
>
>
>
> On 9/11/23 08:17, Ilias Apalodimas wrote:
> > Hi Simon,
> >
> > On Sun, 10 Sept 2023 at 22:14, Simon Glass <sjg at chromium.org> wrote:
> >>
> >> Hi Ilias,
> >>
> >> On Mon, 4 Sept 2023 at 03:31, Ilias Apalodimas
> >> <ilias.apalodimas at linaro.org> wrote:
> >>>
> >>> Hi Simon,
> >>>
> >>> On Fri, 1 Sept 2023 at 18:51, Simon Glass <sjg at chromium.org> wrote:
> >>>>
> >>>> Hi Ilias,
> >>>>
> >>>> On Fri, 1 Sept 2023 at 01:50, Ilias Apalodimas <ilias.apalodimas at linaro.org> wrote:
> >>>>>
> >>>>> [...]
> >>>>>
> >>>>>>>
> >>>>>>>> +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.

>
> Regarding DT part. We are experimenting with TF-A injecting for example DDR
> memory reservation to DT.
> Injection is working properly if you are using single DT but in SOM case we are
> using FIT image with a lot of DTBs inside (issue with checksums calculation).
> I see couple of platforms (IIRC renesas/imx) which are using DT overlays and
> passing it via specific registers. For these using bloblist might be right way
> to go.

Yes that's in fact the purpose of that spec.

> I wasn't aware about the whole fdt bloblist discussion but based on my
> experiments you can create multiple FDT entries that's why I expect that there
> could be DT overlays from different stages. And I even think that all of them
> can be overlays without base DT which can be select later on.

If the bloblist has overlays, then we shouldn't be compiling with
OF_BOARD.  You will get a DT from u-boot and tghe adding this
overlays, is an matter of parsing the bloblist and appending the,

Thanks
/Ilias
>
> Thanks,
> Michal
>
>