Hi, On Mon, 11 Sept 2023 at 05:48, Ilias Apalodimas <ilias.apalodi...@linaro.org> wrote: > > Hi Michal, > > On Mon, 11 Sept 2023 at 13:58, Michal Simek <michal.si...@amd.com> wrote: > > > > Hi Ilias, > > > > On 9/11/23 09:56, Ilias Apalodimas wrote: > > > Hi Michal, > > > > > > On Mon, 11 Sept 2023 at 09:38, Michal Simek <michal.si...@amd.com> wrote: > > >> > > >> > > >> > > >> On 9/11/23 08:17, Ilias Apalodimas wrote: > > >>> Hi Simon, > > >>> > > >>> On Sun, 10 Sept 2023 at 22:14, Simon Glass <s...@chromium.org> wrote: > > >>>> > > >>>> Hi Ilias, > > >>>> > > >>>> On Mon, 4 Sept 2023 at 03:31, Ilias Apalodimas > > >>>> <ilias.apalodi...@linaro.org> wrote: > > >>>>> > > >>>>> Hi Simon, > > >>>>> > > >>>>> On Fri, 1 Sept 2023 at 18:51, Simon Glass <s...@chromium.org> wrote: > > >>>>>> > > >>>>>> Hi Ilias, > > >>>>>> > > >>>>>> On Fri, 1 Sept 2023 at 01:50, Ilias Apalodimas > > >>>>>> <ilias.apalodi...@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. > > 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. 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. 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. Regards, Simon [1] https://patchwork.ozlabs.org/project/uboot/list/?series=281465&state=* [2] https://patchwork.ozlabs.org/project/uboot/list/?series=365719
