| Message ID | 20231228194725.2482268-1-sjg@chromium.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [v6] fdt: Allow the devicetree to come from a bloblist | expand |
On Thu, Dec 28, 2023 at 07:47:25PM +0000, Simon Glass 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> > --- [snip] > diff --git a/dts/Kconfig b/dts/Kconfig > index 00c0aeff893..ae451b9caf7 100644 > --- a/dts/Kconfig > +++ b/dts/Kconfig > @@ -105,6 +105,19 @@ config OF_EMBED > > endchoice > > +config OF_BLOBLIST > + bool "Provided by a bloblist at runtime" > + depends on BLOBLIST && OF_SEPARATE This is now even more confusing, frankly. The help for OF_SEPARATE says: If this option is enabled, the device tree will be built and placed as a separate u-boot.dtb file alongside the U-Boot image. So why would you enable that to then have a device tree passed via bloblist instead? We should probably start by fixing all of this confusing naming / logic and then correct things such that: - OF_EMBED wins if set. This is the override-has-been-set we-must-use-it switch. First choice, not last choice. If binman needs tweaks so that it will still generate images for platforms in this case, that needs to happen. - If we have a bloblist, we scan the bloblist for DT and if found, use it. - If it looks like we've been booted as a fake Linux kernel, and we can start with just aarch64 and let riscv come in as a follow up, so what's documented within https://www.kernel.org/doc/html/latest/arch/arm64/booting.html#call-the-kernel-image then we use that device tree. - This _may_ just end up having to be "Does x0 (or similar) point to a valid DT?" as I don't know how correct everything using this method today is too what the spec above lists. - If we have a dtb appended to use by what we call today OF_SEPARATE but should really stop calling it that we use that. At that point, we can probably have zero "totally board specific kludge for device tree location", and kill off OF_HAS_PRIOR_STAGE too (since that's really just bloblist or fake-linux-kernel). We'll also be able to support migration from fake-linux-kernel to bloblist
Hi, On Thu, Dec 28, 2023 at 2:24 PM Tom Rini <trini@konsulko.com> wrote: > > On Thu, Dec 28, 2023 at 07:47:25PM +0000, Simon Glass 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> > > --- > [snip] > > diff --git a/dts/Kconfig b/dts/Kconfig > > index 00c0aeff893..ae451b9caf7 100644 > > --- a/dts/Kconfig > > +++ b/dts/Kconfig > > @@ -105,6 +105,19 @@ config OF_EMBED > > > > endchoice > > > > +config OF_BLOBLIST > > + bool "Provided by a bloblist at runtime" > > + depends on BLOBLIST && OF_SEPARATE > > This is now even more confusing, frankly. The help for OF_SEPARATE says: > If this option is enabled, the device tree will be built and placed as a > separate u-boot.dtb file alongside the U-Boot image. > > So why would you enable that to then have a device tree passed via > bloblist instead? Historical.... > > We should probably start by fixing all of this confusing naming / logic > and then correct things such that: > - OF_EMBED wins if set. This is the override-has-been-set we-must-use-it > switch. First choice, not last choice. If binman needs tweaks so that > it will still generate images for platforms in this case, that needs > to happen. Perhaps we should rename this to OF_EMBED_DEBUG ? Really it should not be used by any board. > - If we have a bloblist, we scan the bloblist for DT and if found, use > it. unless we don't want to, e.g. because that DT has a bug or we want to use a different one during development. > - If it looks like we've been booted as a fake Linux kernel, and we can > start with just aarch64 and let riscv come in as a follow up, so > what's documented within > https://www.kernel.org/doc/html/latest/arch/arm64/booting.html#call-the-kernel-image > then we use that device tree. Eek, really? Is this the rpi case and you are trying to make it generic? I would rather that be a board-specific thing, calling into a generic implementation. We should encourage bloblist. > - This _may_ just end up having to be "Does x0 (or similar) point to a > valid DT?" as I don't know how correct everything using this method > today is too what the spec above lists. OK, well a generic impl would be good I suppose, but dereferencing pointers to look for a magic number might not end well. > - If we have a dtb appended to use by what we call today OF_SEPARATE but > should really stop calling it that we use that. Yes it is a complete misnomer now. I will try to think of a better name. It really just controls generation of u-boot.dtb and what goes in u-boot.bin so perhaps we can just drop it and have OF_EMBED be false in the normal case. Or rename to OF_STANDARD ? > > At that point, we can probably have zero "totally board specific kludge > for device tree location", and kill off OF_HAS_PRIOR_STAGE too (since > that's really just bloblist or fake-linux-kernel). We'll also be able to > support migration from fake-linux-kernel to bloblist OF_HAS_PRIOR_STAGE controls things like building the DT and OF_BOARD ... which from what you say above can perhaps go away. But there are quite a lot of board_fdt_blob_setup() functions...it doesn't look like they are all the same. Automatic is OK I suppose. I just want to make sure that these things can be disabled easily so it is possible to use the DT built by U-Boot without hacking the code. That is my goal with having a Kconfig to enable this mechanisms. Regards, Simon
On Sun, Dec 31, 2023 at 05:47:30AM -0700, Simon Glass wrote: > Hi, > > On Thu, Dec 28, 2023 at 2:24 PM Tom Rini <trini@konsulko.com> wrote: > > > > On Thu, Dec 28, 2023 at 07:47:25PM +0000, Simon Glass 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> > > > --- > > [snip] > > > diff --git a/dts/Kconfig b/dts/Kconfig > > > index 00c0aeff893..ae451b9caf7 100644 > > > --- a/dts/Kconfig > > > +++ b/dts/Kconfig > > > @@ -105,6 +105,19 @@ config OF_EMBED > > > > > > endchoice > > > > > > +config OF_BLOBLIST > > > + bool "Provided by a bloblist at runtime" > > > + depends on BLOBLIST && OF_SEPARATE > > > > This is now even more confusing, frankly. The help for OF_SEPARATE says: > > If this option is enabled, the device tree will be built and placed as a > > separate u-boot.dtb file alongside the U-Boot image. > > > > So why would you enable that to then have a device tree passed via > > bloblist instead? > > Historical.... Sorry, I mean the help text doesn't make any sense, with your patch. So that needs to be fixed. That's part of how everyone else is going to try and understand this mess of options afterwards. > > We should probably start by fixing all of this confusing naming / logic > > and then correct things such that: > > - OF_EMBED wins if set. This is the override-has-been-set we-must-use-it > > switch. First choice, not last choice. If binman needs tweaks so that > > it will still generate images for platforms in this case, that needs > > to happen. > > Perhaps we should rename this to OF_EMBED_DEBUG ? Really it should not > be used by any board. It may or may not be appropriate for use by boards, in production. Maybe future designs can avoid this, but I think current ones cannot. But that's also a separate thread an cleanup task to bring up. Again, I suspect one of the use cases here is "embed the DTB so that we secure boot check and validate a single memory range". > > - If we have a bloblist, we scan the bloblist for DT and if found, use > > it. > > unless we don't want to, e.g. because that DT has a bug or we want to > use a different one during development. Nope. If the bloblist has a bug we either have to perform a fixup (the bloblist can't be changed but it's a situation where we're less strict) or for development go back up to OF_EMBED. That's the switch for development where you changed the device tree. > > - If it looks like we've been booted as a fake Linux kernel, and we can > > start with just aarch64 and let riscv come in as a follow up, so > > what's documented within > > https://www.kernel.org/doc/html/latest/arch/arm64/booting.html#call-the-kernel-image > > then we use that device tree. > > Eek, really? Is this the rpi case and you are trying to make it > generic? I would rather that be a board-specific thing, calling into a > generic implementation. We should encourage bloblist. No, it's the generic case. It's not board specific. It's what we do, iirc, on Apple M1/M2/etc, some subsets of Tegra I believe (I know I've done it for xavier-based systems, but then abandoned the effort), certainly some of the older qualcomm platforms, and I believe that's one of the entry points for the newer qualcomm platforms. I spelled out the case above the way that I did because it's a valid case that's not going away. In addition to the places where we can't change the firmware chain, it's also for the places where people don't want to change the firmware chain, or because it's an important bringup path. And part of the problem here is that we've just done it ad-hoc so many times that it's scattered about the tree. > > - This _may_ just end up having to be "Does x0 (or similar) point to a > > valid DT?" as I don't know how correct everything using this method > > today is too what the spec above lists. > > OK, well a generic impl would be good I suppose, but dereferencing > pointers to look for a magic number might not end well. It's a long standing ad-hoc standard. > > - If we have a dtb appended to use by what we call today OF_SEPARATE but > > should really stop calling it that we use that. > > Yes it is a complete misnomer now. I will try to think of a better > name. It really just controls generation of u-boot.dtb and what goes > in u-boot.bin so perhaps we can just drop it and have OF_EMBED be > false in the normal case. Or rename to OF_STANDARD ? I'm not sure about the name either. > > At that point, we can probably have zero "totally board specific kludge > > for device tree location", and kill off OF_HAS_PRIOR_STAGE too (since > > that's really just bloblist or fake-linux-kernel). We'll also be able to > > support migration from fake-linux-kernel to bloblist > > OF_HAS_PRIOR_STAGE controls things like building the DT No, it doesn't. Please re-read the code as it stands today. What it does control is.. > and OF_BOARD Yes, it then has OF_BOARD be enabled by default. And also OF_OMIT_DTB, which is a funny negative symbol (enable OF_OMIT_DTB to not perform action). > ... which from what you say above can perhaps go away. But there are > quite a lot of board_fdt_blob_setup() functions...it doesn't look like > they are all the same. Looks fairly common to me, I see a handful of "implement arm64 linux kernel boot", then "whatever riscv is doing", and "get dtb from ATF". Where that last one should be able to migrate to bloblist if I understand things right and so we'll need to ask the maintainers what they need long term here. There's very few "grab dtb from NOR flash", but they do exist. > Automatic is OK I suppose. I just want to make sure that these things > can be disabled easily so it is possible to use the DT built by U-Boot > without hacking the code. That is my goal with having a Kconfig to > enable this mechanisms. But the rest of us still don't understand your use case, or all of your use cases, is the sticking point. Ideally, if a bloblist is possible, it should have the dtb. Otherwise, if we're passed a DTB because something thinks it's booting the OS, that's the DTB the OS must get (or at least the starting point of it). Otherwise, yes, if we have our own DTB, we can give that to the OS. To try and rephrase something you've said, BLOBLIST=y && OF_BLOBLIST=n should only be a developer option. So why can't we just use the developer option we have today? Other than that it's currently last-case, not first-case, which is wrong.
diff --git a/common/bloblist.c b/common/bloblist.c index a22f6c12b0c..b07ede11cfe 100644 --- a/common/bloblist.c +++ b/common/bloblist.c @@ -48,6 +48,7 @@ static struct tag_name { { BLOBLISTT_ACPI_TABLES, "ACPI tables for x86" }, { BLOBLISTT_SMBIOS_TABLES, "SMBIOS tables for x86" }, { BLOBLISTT_VBOOT_CTX, "Chrome OS vboot context" }, + { BLOBLISTT_CONTROL_FDT, "Control FDT" }, /* BLOBLISTT_PROJECT_AREA */ { BLOBLISTT_U_BOOT_SPL_HANDOFF, "SPL hand-off" }, diff --git a/doc/develop/devicetree/control.rst b/doc/develop/devicetree/control.rst index cbb65c9b177..444c55de4e4 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, if present. + If CONFIG_SANDBOX is defined, then it will be read from a file on startup. Use the -d flag to U-Boot to specify the file to read, -D for the default and -T for the test devicetree, used to run sandbox unit tests. diff --git a/dts/Kconfig b/dts/Kconfig index 00c0aeff893..ae451b9caf7 100644 --- a/dts/Kconfig +++ b/dts/Kconfig @@ -105,6 +105,19 @@ config OF_EMBED endchoice +config OF_BLOBLIST + bool "Provided by a bloblist at runtime" + depends on BLOBLIST && OF_SEPARATE + default y + 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. + + If the bloblist does not contain a devicetree, then other methods will + be used. + config OF_BOARD bool "Provided by the board (e.g a previous loader) at runtime" default y if SANDBOX || OF_HAS_PRIOR_STAGE diff --git a/include/bloblist.h b/include/bloblist.h index 080cc46a126..e16d122f4fb 100644 --- a/include/bloblist.h +++ b/include/bloblist.h @@ -103,6 +103,11 @@ enum bloblist_tag_t { BLOBLISTT_ACPI_TABLES = 0x104, /* ACPI tables for x86 */ BLOBLISTT_SMBIOS_TABLES = 0x105, /* SMBIOS tables for x86 */ BLOBLISTT_VBOOT_CTX = 0x106, /* Chromium OS verified boot context */ + /* + * Devicetree for use by firmware. On some platforms this is passed to + * the OS also + */ + BLOBLISTT_CONTROL_FDT = 0x107, /* * Project-specific tags are permitted here. Projects can be open source diff --git a/include/fdtdec.h b/include/fdtdec.h index bd1149f46d0..e80de24076c 100644 --- a/include/fdtdec.h +++ b/include/fdtdec.h @@ -72,7 +72,7 @@ struct bd_info; * U-Boot is packaged as an ELF file, e.g. for debugging purposes * @FDTSRC_ENV: Provided by the fdtcontroladdr environment variable. This should * be used for debugging/development only - * @FDTSRC_NONE: No devicetree at all + * @FDTSRC_BLOBLIST: Provided by a bloblist from an earlier phase */ enum fdt_source_t { FDTSRC_SEPARATE, @@ -80,6 +80,7 @@ enum fdt_source_t { FDTSRC_BOARD, FDTSRC_EMBED, FDTSRC_ENV, + FDTSRC_BLOBLIST, }; /* @@ -1190,7 +1191,8 @@ int fdtdec_resetup(int *rescan); * * The existing devicetree is available at gd->fdt_blob * - * @err internal error code if we fail to setup a DTB + * @err: 0 on success, -EEXIST if the devicetree is already correct, or other + * internal error code if we fail to setup a DTB * @returns new devicetree blob pointer */ void *board_fdt_blob_setup(int *err); diff --git a/lib/fdtdec.c b/lib/fdtdec.c index 4016bf3c113..af3026dd42b 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -7,6 +7,10 @@ */ #ifndef USE_HOSTCC + +#define LOG_CATEGORY LOGC_DT + +#include <bloblist.h> #include <boot_fit.h> #include <display_options.h> #include <dm.h> @@ -86,6 +90,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) @@ -1662,23 +1667,42 @@ static void setup_multi_dtb_fit(void) int fdtdec_setup(void) { - int ret; + int ret = -ENOENT; + + /* If allowing a bloblist, check that first */ + if (CONFIG_IS_ENABLED(OF_BLOBLIST)) { + ret = bloblist_maybe_init(); + if (!ret) { + gd->fdt_blob = bloblist_find(BLOBLISTT_CONTROL_FDT, 0); + if (gd->fdt_blob) { + gd->fdt_src = FDTSRC_BLOBLIST; + log_debug("Devicetree is in bloblist at %p\n", + gd->fdt_blob); + } else { + log_debug("No FDT found in bloblist\n"); + ret = -ENOENT; + } + } + } - /* 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; + /* Otherwise, the devicetree is typically appended to U-Boot */ + if (ret) { + 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 (ret) + if (!ret) + gd->fdt_src = FDTSRC_BOARD; + else if (ret != -EEXIST) return ret; - gd->fdt_src = FDTSRC_BOARD; } /* Allow the early environment to override the fdt address */
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> --- The discussion on this was not resolved and is now important due to the bloblist series from Raymond. So I am sending it again since I believe this is a better starting point than building on OF_BOARD Changes in v6: - Don't allow bloblist with OF_EMBED Changes in v5: - Make OF_BLOBLIST default y - Make OF_BLOBLIST optional at runtime Changes in v4: - Rebase to -next common/bloblist.c | 1 + doc/develop/devicetree/control.rst | 3 ++ dts/Kconfig | 13 +++++++++ include/bloblist.h | 5 ++++ include/fdtdec.h | 6 ++-- lib/fdtdec.c | 44 +++++++++++++++++++++++------- 6 files changed, 60 insertions(+), 12 deletions(-)