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

[Michal Simek <michal.simek@amd.com>]

On 9/11/23 08:17, Ilias Apalodimas wrote:
> Hi Simon,
> 
> On Sun, 10 Sept 2023 at 22:14, Simon Glass <sjg@chromium.org> wrote:
>>
>> Hi Ilias,
>>
>> On Mon, 4 Sept 2023 at 03:31, Ilias Apalodimas
>> <ilias.apalodimas@linaro.org> wrote:
>>>
>>> Hi Simon,
>>>
>>> On Fri, 1 Sept 2023 at 18:51, Simon Glass <sjg@chromium.org> wrote:
>>>>
>>>> Hi Ilias,
>>>>
>>>> On Fri, 1 Sept 2023 at 01:50, Ilias Apalodimas <ilias.apalodimas@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.

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

Thanks,
Michal