Hi Simon
On Tue, 26 Sept 2023 at 14:37, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Mon, 25 Sept 2023 at 13:48, Ilias Apalodimas ilias.apalodimas@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 sjg@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 sjg@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
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 /Ilias
Regards, Simon