Hi Simon, OK, I can try to remove them into two series. Regards, Raymond
On Thu, 7 Dec 2023 at 22:14, Simon Glass <s...@chromium.org> wrote: > 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=* >