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

[Michal Simek]

Hi Ilias,

On 9/11/23 09:56, Ilias Apalodimas wrote:
> 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.

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.


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

Good that I read it properly.

Thanks,
Michal