Hi Ilias, On Fri, 20 Oct 2023 at 01:22, Ilias Apalodimas <ilias.apalodi...@linaro.org> wrote: > > Hi Simon, > > On Wed, 18 Oct 2023 at 18:26, Simon Glass <s...@chromium.org> wrote: > > > > Hi Ilias, > > > > On Mon, 25 Sept 2023 at 04:19, Ilias Apalodimas > > <ilias.apalodi...@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
