On Fri, Oct 20, 2023 at 11:21:29AM +0300, Ilias Apalodimas 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
The rest of this series, minus this patch has been merged. We should just start a new series I think. -- Tom
signature.asc
Description: PGP signature
