Hi Ilias, On Tue, 26 Sept 2023 at 06:32, Ilias Apalodimas <ilias.apalodi...@linaro.org> wrote: > > Hi Simon > > On Tue, 26 Sept 2023 at 14:37, Simon Glass <s...@chromium.org> wrote: > > > > Hi Ilias, > > > > On Mon, 25 Sept 2023 at 13:48, Ilias Apalodimas > > <ilias.apalodi...@linaro.org> wrote: > > > > > > Hi Simon, > > > > > > I commented on the v1 thread, but let's continue the discussion here > > > > > > On Thu, 21 Sept 2023 at 04:58, Simon Glass <s...@chromium.org> wrote: > > > > > > > > Standard passage provides for a bloblist to be passed from one firmware > > > > phase to the next. That can be used to pass the devicetree along as > > > > well. > > > > Add an option to support this. > > > > > > > > Tests for this will be added as part of the Universal Payload work. > > > > > > > > Signed-off-by: Simon Glass <s...@chromium.org> > > > > --- > > > > > > > > Changes in v2: > > > > - No changes as it still seems unclear what should be done > > > > > > > > > > [...] > > > > > > > > > > > /* BLOBLISTT_VENDOR_AREA */ > > > > diff --git a/doc/develop/devicetree/control.rst > > > > b/doc/develop/devicetree/control.rst > > > > index cbb65c9b177f..56e00090166f 100644 > > > > --- a/doc/develop/devicetree/control.rst > > > > +++ b/doc/develop/devicetree/control.rst > > > > @@ -108,6 +108,9 @@ If CONFIG_OF_BOARD is defined, a board-specific > > > > routine will provide the > > > > devicetree at runtime, for example if an earlier bootloader stage > > > > creates > > > > it and passes it to U-Boot. > > > > > > > > +If CONFIG_OF_BLOBLIST is defined, the devicetree comes from a bloblist > > > > passed > > > > +from a previous stage. > > > > > > What I argued before is that we don't need to be this explicit. > > > The bloblist can carry a bunch of options that might be used by > > > U-Boot. It would be better if we had a more generic approach instead > > > of adding Kconfig options per bloblist entry > > > > Yes, fair enough. It would really get out of hand if we had a lot of > > Kconfig options for everything that could be in a bloblist. > > > > I believe someone objected to this in the other thread, so there may > > be board-specific issues to resolve. > > I had the same objection to that first version as well > > > > > > > > > [...] > > > > > > > #ifndef USE_HOSTCC > > > > + > > > > +#define LOG_CATEGORY LOGC_DT > > > > + > > > > #include <common.h> > > > > +#include <bloblist.h> > > > > #include <boot_fit.h> > > > > #include <display_options.h> > > > > #include <dm.h> > > > > @@ -87,6 +91,7 @@ static const char *const fdt_src_name[] = { > > > > [FDTSRC_BOARD] = "board", > > > > [FDTSRC_EMBED] = "embed", > > > > [FDTSRC_ENV] = "env", > > > > + [FDTSRC_BLOBLIST] = "bloblist", > > > > }; > > > > > > > > const char *fdtdec_get_srcname(void) > > > > @@ -1666,20 +1671,35 @@ int fdtdec_setup(void) > > > > int ret; > > > > > > > > /* The devicetree is typically appended to U-Boot */ > > > > - if (IS_ENABLED(CONFIG_OF_SEPARATE)) { > > > > - gd->fdt_blob = fdt_find_separate(); > > > > - gd->fdt_src = FDTSRC_SEPARATE; > > > > - } else { /* embed dtb in ELF file for testing / development */ > > > > - gd->fdt_blob = dtb_dt_embedded(); > > > > - gd->fdt_src = FDTSRC_EMBED; > > > > - } > > > > - > > > > - /* Allow the board to override the fdt address. */ > > > > - if (IS_ENABLED(CONFIG_OF_BOARD)) { > > > > - gd->fdt_blob = board_fdt_blob_setup(&ret); > > > > + if (CONFIG_IS_ENABLED(OF_BLOBLIST)) { > > > > + ret = bloblist_maybe_init(); > > > > if (ret) > > > > return ret; > > > > - gd->fdt_src = FDTSRC_BOARD; > > > > > > So, instead of adding OF_BLOBLIST, just move this code under OF_BOARD, > > > inside an IS_ENABLED(BLOBLIST) check. If a bloblist is required and > > > the previous stage loader is supposed to provide a DT we can just > > > throw an error and stop booting > > > > This is the bit I don't get. > > > > The OF_BOARD thing is a hack, in that the board can do what it likes. > > It is our way of handling board-specific mechanisms. > > > > But I am wanting a standard mechanism, i.e. like 'standard passage', a > > way of passing the DT through the phases. > > > > If I put this under OF_BOARD, then the board gets to override the > > mechanism, so which is it? > > No, it's the other way around in my head. OF_BOARD description is 'a > previous stage loader hands me over the DT', which is a superset of > the bloblist. > Whether it comes in a firmware handoff format, or a hacky register the > previous bootloader filled in is a detail we have to deal with and we > need to keep backwards compatibility. > > Maybe adding a coding snip would help > if (IS_ENABLED(CONFIG_OF_BOARD)) { > if (CONFIG_IS_ENABLED(BLOBLIST)) { <- This instead of OF_BLOBLIST > ret = bloblist_maybe_init(); > if (ret) > return ret; > /* Dynamically scan for a DT in the bloblist. */ > gd->fdt_blob = bloblist_find(BLOBLISTT_CONTROL_FDT, 0); > if (!gd->fdt_blob) { > printf("Not FDT found in bloblist\n"); > bloblist_show_list(); > // We can choose to not return an error here and keep > scanning in case the DT is in a register, but I am fine with both > return -ENOENT; > } > gd->fdt_src = FDTSRC_BLOBLIST; > bloblist_show_list(); > log_debug("Devicetree is in bloblist at %p\n", gd->fdt_blob); > // We can also bail out of this entirely if we do find a DT via > a bloblist. > } else { > gd->fdt_blob = board_fdt_blob_setup(&ret); > if (ret) > return ret; > gd->fdt_src = FDTSRC_BOARD; > } > } > > I haven't even compiled the code above, but it should give you a > better idea of what I am suggesting
OK I see...yes that is along the lines of what I thought you meant. But OF_BOARD does not mean 'previous stage loader hands me over the DT'. I means call board_fdt_blob_setup() which could do anything. If some boards use that function to implement getting a DT from the prior stage, that's fine, but it isn't limited to that. There is an HAS_PRIOR_STAGE option which sets OF_BOARD at present. But that doesn't seem right in the long term. The goal here is to standardise the passing of DT from a prior stage, so that all boards (except perhaps the dark child rpi) use standard passage (bloblist) to do so. That should not depend on OF_BOARD and in fact I would like to make OF_BOARD the exception, used when OF_BLOBLIST doesn't suit. > > Hope that helps > /Ilias > > > > You say 'we can just throw and error' but what is the error? If the DT > > is provided via the bloblist, there is no error. If it is not, I > > already show an error and halt as you can see in the code below. > > > > I guess I'm just confused about what you are saying here. > > > > > > > > > > > > + gd->fdt_blob = bloblist_find(BLOBLISTT_CONTROL_FDT, 0); > > > > + if (!gd->fdt_blob) { > > > > + printf("Not FDT found in bloblist\n"); > > > > + bloblist_show_list(); > > > > + return -ENOENT; > > > > + } > > > > > > [...] > > > Regards, Simon