Hi Raymond, On Wed, 6 Dec 2023 at 13:35, Raymond Mao <raymond....@linaro.org> wrote: > > Hi Simon, > > > The other thing missing here is SPL. The bloblist is designed such > > that it can be set up in any phase of U-Boot, then is passed to > > following phases. So using IS_ENABLED(BLOBLIST) doesn't do what we > > need. As I noted, it breaks sandbox_spl. What happens if a board wants > > to use bloblist in SPL but doesn't want the SPL DT to come from a > > prior stage? What happens if SPL wants doesn't want to pass the U-Boot > > DT through to U-Boot in the bloblist? It is all just horribly > > confusing. > I think the logic to decide whether to take the DT from bloblist really > depends > on the board vendor. > I will move this kind of logics into board specific functions from fdtdec.c > in the > next revision. > Board vendors can decide the behaviors they want. This can answer the question > above and for example whether they want a fallback when no DT exists in the > bloblist or the DT from bloblis is corrupted, or just print a warning message. > For Qemu-arm, as an example of design reference, I prefer to take the DT from > the > bloblist automatically when (BLOBLIST && OF_BOARD) and keep the fallback > compatibility to load from `CFG_SYS_SDRAM_BASE` when DT from bloblist is > corrupted. > I will take a look into the sandbox_spl failures you mentioned.
In the interests of my sanity, could you send just the bloblist updates and move the other patches into a separate series? Regards, SImon > > Thanks and regards, > Raymond > > > On Tue, 5 Dec 2023 at 22:57, Simon Glass <s...@chromium.org> wrote: >> >> Hi Ilias, >> >> On Mon, 4 Dec 2023 at 23:22, Ilias Apalodimas >> <ilias.apalodi...@linaro.org> wrote: >> > >> > Hi Simon, >> > >> > We did discuss this in OSFC but perhaps you forgot. The discussion >> > was based on the mail here [0]. >> >> Perhaps I did? Or perhaps we had a different understanding of it? >> >> > >> > On Tue, 5 Dec 2023 at 02:52, Simon Glass <s...@chromium.org> wrote: >> > > >> > > Hi Raymond, >> > > >> > > On Mon, 4 Dec 2023 at 12:25, Raymond Mao <raymond....@linaro.org> wrote: >> > > > >> > > > Hi Simon, >> > > > >> > > > When `OF_BOARD` is defined, the FDT should be from one of the >> > > > board-specific mechanisms: >> > > > 1. FDT from a bloblist via boot args >> > > > 2. FDT in memory >> > > > I don't think we need a new build option to distinguish these two, >> > > > since it can be done in runtime by checking whether the boot args >> > > > follow the FW Handoff spec conventions and the FDT exists in the >> > > > bloblist. >> > > >> > > No, I would really like this to be deterministic, so we can fail if >> > > something goes wrong. The bloblist should be 'declared' as the way to >> > > boot, for a board. >> > >> > We *are* deterministic. I am just going to repeat what we discussed in >> > OSFC for completeness. >> > OF_BLOBLIST makes little sense because we are not going to add a >> > Kconfig per blolblist entry. >> > I haven't looked at what the patch does yet but the idea was really simple. >> > - Under OF_BOARD (which can arguably be renamed to OF_PREVIOUS_STAGE >> > or something like that) if CONFIG_BLOB is enabled we scan for a >> > bloblist >> > - If CONFIG_BLOB is enabled but no bloblist is provided we fail the boot >> > - If CONFIG_BLOB is not enabled we do what we did up to now, maybe >> > with a message that this is going to be obsoleted once enough boards >> > migrate to a bloblist >> > >> > IOW if support for a bloblist is enabled we expect to find one from >> > the previous bootloader. I've lost count of how many times I repeated >> > this, but here it goes again >> > The device tree can come >> > - From u-boot >> > - From a previous stage loader *somehow* (eg. passed on a bloblist, >> > passed on a register, read from an EEPROM etc) >> > >> > We should keep that distinction and create subcategories for the >> > 'comes from a previous stage loader', rather than adding arbitrary >> > config options which only confuse people >> >> I don't know why you are so frustrated about this. This patch does not >> even do what you have listed above, so far as I can tell. >> >> We have: >> - OF_HAS_PRIOR_STAGE which indicates that U-Boot is not the first phase >> - OF_BOARD which indicates that the board can do anything it likes >> - BLOBLIST which indicates that there is a bloblist >> >> I proposed adding: >> - OF_BLOBLIST which indicates that U-Boot gets the DT from the bloblist >> >> I understood that your intention OF_BOARD would go away since there >> are almost no cases that need it. I was skeptical it could be done but >> I definitely agreed (and still do) that it would be great if that >> could go away, as we would not need OF_BLOBLIST since OF_BOARD would >> effectively mean the same thing. >> >> But when I look at this patch, that is not what is happening. In fact, >> there is no indication that the DT came from the bloblist. U-Boot will >> just report 'devicetree: board'. We must say where it came from. >> >> The other thing missing here is SPL. The bloblist is designed such >> that it can be set up in any phase of U-Boot, then is passed to >> following phases. So using IS_ENABLED(BLOBLIST) doesn't do what we >> need. As I noted, it breaks sandbox_spl. What happens if a board wants >> to use bloblist in SPL but doesn't want the SPL DT to come from a >> prior stage? What happens if SPL wants doesn't want to pass the U-Boot >> DT through to U-Boot in the bloblist? It is all just horribly >> confusing. >> >> So I believe, in fact, that we cannot get rid of OF_BOARD now, and >> perhaps for quite a long time. There is another discussion about >> Qualcomm where they want a gzipped DT attached to the end of U-Boot, >> for example. Perhaps the key problem with this patch is that it would >> ensure that OF_BOARD sticks around, since it means both a) Get the DT >> from bloblist if BLOBLIST; b) Get the DT from a board-specific >> mechanism if !BLOBLIST. That is not what we discussed. >> >> Why not just take my original patch and work from there? It will save >> a lot of time. We can then progressively migrate boards from OF_BOARD >> to OF_BLOBLIST and then perhaps one day remove OF_BOARD (I remain >> skeptical, but hopeful). The whole 'standard passage' thing [1] was >> carefully thought out *two years ago* and (I believe) dealt with all >> of the issues above. >> >> For now, we should apply the patches which bring in the spec changes, >> so we can figure out the rest of it. I have added my review tag for >> those. >> >> Regards, >> SImon >> > >> > Regards >> > /Ilias >> > > >> > > If in the future all boards a reusing bloblist for this feature, then >> > > sure, we can drop it. But we have things like rpi which do their own >> > > thing. >> > > >> > > > If it fulfills the above conditions, we can take the FDT from >> > > > bloblist, otherwise from the predefined memory address. >> > > > This is fully backward compatible without adding a new build option. >> > > >> > > Again, we need to obtain the FDT from the bloblist if and only if the >> > > board requires it. It creates a lot of confusion to have it as an >> > > optional feature, when we know which way it should be. >> > > >> > > Ultimately OF_BOARD should go away. We just need to push bloblist to >> > > other projects and closed-source firmware as the correct way to pass a >> > > DT. >> > > >> > > Regards, >> > > Simon >> > > [.] >> > >> > [0] >> > https://lore.kernel.org/u-boot/capnjgz0sqvyj_drleqrp21zpycco3hop-ryd+nkjma0bvyp...@mail.gmail.com/ >> >> [1] https://patchwork.ozlabs.org/project/uboot/list/?series=281465&state=*
