[U-Boot] [PATCH 1/2] efi_loader: Pass fdt address directly to bootefi cmd
The bootefi cmd today fetches its device tree pointer from either the location appointed by "fdt addr" with a fallback to the U-Boot control fdt.
This integration is unusual for U-Boot and diverges from the way we usually handle parameters to boot commands. So let's pass the fdt directly into the bootefi command instead and move the control fdt logic into the distro boot script.
Signed-off-by: Alexander Graf agraf@suse.de --- cmd/bootefi.c | 34 +++++++++++++--------------------- include/config_distro_bootcmd.h | 9 ++++++--- 2 files changed, 19 insertions(+), 24 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index b213ef1..ab39b95 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -142,12 +142,11 @@ static void *copy_fdt(void *fdt) * Load an EFI payload into a newly allocated piece of memory, register all * EFI objects it would want to access and jump to it. */ -static unsigned long do_bootefi_exec(void *efi) +static unsigned long do_bootefi_exec(void *efi, void *fdt) { ulong (*entry)(void *image_handle, struct efi_system_table *st); ulong fdt_pages, fdt_size, fdt_start, fdt_end; bootm_headers_t img = { 0 }; - void *fdt = working_fdt;
/* * gd lives in a fixed register which may get clobbered while we execute @@ -155,13 +154,7 @@ static unsigned long do_bootefi_exec(void *efi) */ efi_save_gd();
- /* Update system table to point to our currently loaded FDT */ - - /* Fall back to included fdt if none was manually loaded */ - if (!fdt && gd->fdt_blob) - fdt = (void *)gd->fdt_blob; - - if (fdt) { + if (fdt && !fdt_check_header(fdt)) { /* Prepare fdt for payload */ fdt = copy_fdt(fdt);
@@ -185,7 +178,7 @@ static unsigned long do_bootefi_exec(void *efi) efi_add_memory_map(fdt_start, fdt_pages, EFI_BOOT_SERVICES_DATA, true); } else { - printf("WARNING: No device tree loaded, expect boot to fail\n"); + printf("WARNING: Invalid device tree, expect boot to fail\n"); systab.nr_tables = 0; }
@@ -216,18 +209,20 @@ static unsigned long do_bootefi_exec(void *efi) /* Interpreter command to boot an arbitrary EFI image from memory */ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { - char *saddr; - unsigned long addr; + char *saddr, *sfdt; + unsigned long addr, fdt_addr; int r = 0;
- if (argc < 2) + if (argc < 3) return 1; saddr = argv[1]; + sfdt = argv[2];
addr = simple_strtoul(saddr, NULL, 16); + fdt_addr = simple_strtoul(sfdt, NULL, 16);
printf("## Starting EFI application at 0x%08lx ...\n", addr); - r = do_bootefi_exec((void *)addr); + r = do_bootefi_exec((void *)addr, (void*)fdt_addr); printf("## Application terminated, r = %d\n", r);
if (r != 0) @@ -238,16 +233,13 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
#ifdef CONFIG_SYS_LONGHELP static char bootefi_help_text[] = - "<image address>\n" - " - boot EFI payload stored at address <image address>\n" - "\n" - "Since most EFI payloads want to have a device tree provided, please\n" - "make sure you load a device tree using the fdt addr command before\n" - "executing bootefi.\n"; + "<image address> <fdt address>\n" + " - boot EFI payload stored at address <image address> with a device\n" + " tree located at <fdt address>\n"; #endif
U_BOOT_CMD( - bootefi, 2, 0, do_bootefi, + bootefi, 3, 0, do_bootefi, "Boots an EFI payload from memory\n", bootefi_help_text ); diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h index ad9045e..67eb8f2 100644 --- a/include/config_distro_bootcmd.h +++ b/include/config_distro_bootcmd.h @@ -103,12 +103,15 @@ "boot_efi_binary=" \ "load ${devtype} ${devnum}:${distro_bootpart} " \ "${kernel_addr_r} efi/boot/"BOOTEFI_NAME"; " \ - "bootefi ${kernel_addr_r}\0" \ + "if fdt addr ${fdt_addr_r}; then " \ + "bootefi ${kernel_addr_r} ${fdt_addr_r};" \ + "else" \ + "bootefi ${kernel_addr_r} ${fdtcontroladdr};" \ + "fi\0" \ \ "load_efi_dtb=" \ "load ${devtype} ${devnum}:${distro_bootpart} " \ - "${fdt_addr_r} ${prefix}${fdtfile}; " \ - "fdt addr ${fdt_addr_r}\0" \ + "${fdt_addr_r} ${prefix}${fdtfile}\0" \ \ "efi_dtb_prefixes=/ /dtb/ /dtb/current/\0" \ "scan_dev_for_efi=" \
When there is no $fdtfile variable set, we still have a good chance that on 32bit arm the fdtfile really is just called $soc-$board.dtb.
Enable the exports for $soc and $board in our distr defaults and make use of them in the efi boot script.
Reported-by: Andreas Faerber afaerber@suse.de Reported-by: Stephen Warren swarren@wwwdotorg.org Signed-off-by: Alexander Graf agraf@suse.de --- include/config_distro_bootcmd.h | 24 +++++++++++++++++++++--- include/config_distro_defaults.h | 1 + 2 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h index 67eb8f2..eaaf2cc 100644 --- a/include/config_distro_bootcmd.h +++ b/include/config_distro_bootcmd.h @@ -99,6 +99,21 @@ #endif
#ifdef BOOTEFI_NAME +#if defined(CONFIG_ARM) && !defined(CONFIG_ARM64) +/* + * On 32bit ARM systems there is a reasonable number of systems that follow + * the $soc-$board$boardver.dtb name scheme for their device trees. Use that + * scheme if we don't have an explicit fdtfile variable. + */ +#define BOOTENV_EFI_SET_FDTFILE_FALLBACK \ + "if test -z "${fdtfile}" -a -n "${soc}"; then " \ + "setenv efifdtfile ${soc}-${board}${boardver}.dtb; " \ + "fi; " +#else +#define BOOTENV_EFI_SET_FDTFILE_FALLBACK +#endif + + #define BOOTENV_SHARED_EFI \ "boot_efi_binary=" \ "load ${devtype} ${devnum}:${distro_bootpart} " \ @@ -111,14 +126,16 @@ \ "load_efi_dtb=" \ "load ${devtype} ${devnum}:${distro_bootpart} " \ - "${fdt_addr_r} ${prefix}${fdtfile}\0" \ + "${fdt_addr_r} ${prefix}${efifdtfile}\0" \ \ "efi_dtb_prefixes=/ /dtb/ /dtb/current/\0" \ "scan_dev_for_efi=" \ + "setenv efifdtfile ${fdtfile}; " \ + BOOTENV_EFI_SET_FDTFILE_FALLBACK \ "for prefix in ${efi_dtb_prefixes}; do " \ "if test -e ${devtype} " \ "${devnum}:${distro_bootpart} " \ - "${prefix}${fdtfile}; then " \ + "${prefix}${efifdtfile}; then " \ "run load_efi_dtb; " \ "fi;" \ "done;" \ @@ -128,7 +145,8 @@ "efi/boot/"BOOTEFI_NAME"; " \ "run boot_efi_binary; " \ "echo EFI LOAD FAILED: continuing...; " \ - "fi; \0" + "fi; " \ + "setenv efifdtfile\0" #define SCAN_DEV_FOR_EFI "run scan_dev_for_efi;" #else #define BOOTENV_SHARED_EFI diff --git a/include/config_distro_defaults.h b/include/config_distro_defaults.h index 076be4d..ae68e3a 100644 --- a/include/config_distro_defaults.h +++ b/include/config_distro_defaults.h @@ -65,5 +65,6 @@ #define CONFIG_ISO_PARTITION #define CONFIG_SUPPORT_RAW_INITRD #define CONFIG_SYS_HUSH_PARSER +#define CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG
#endif /* _CONFIG_CMD_DISTRO_DEFAULTS_H */
On 04/13/2016 03:22 PM, Alexander Graf wrote:
When there is no $fdtfile variable set, we still have a good chance that on 32bit arm the fdtfile really is just called $soc-$board.dtb.
Enable the exports for $soc and $board in our distr defaults and make use of them in the efi boot script.
diff --git a/include/config_distro_defaults.h b/include/config_distro_defaults.h
+#define CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG
Shouldn't that be CONFIG_ENV_VARS_UBOOT_CONFIG? That's what include/env_defaults.h uses to trigger defining $soc and $board. CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG seems to be about setting board_name and board_rev according to README.
On 04/14/2016 01:24 AM, Stephen Warren wrote:
On 04/13/2016 03:22 PM, Alexander Graf wrote:
When there is no $fdtfile variable set, we still have a good chance that on 32bit arm the fdtfile really is just called $soc-$board.dtb.
Enable the exports for $soc and $board in our distr defaults and make use of them in the efi boot script.
diff --git a/include/config_distro_defaults.h b/include/config_distro_defaults.h
+#define CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG
Shouldn't that be CONFIG_ENV_VARS_UBOOT_CONFIG? That's what include/env_defaults.h uses to trigger defining $soc and $board. CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG seems to be about setting board_name and board_rev according to README.
Oops, you're very right of course.
Alex
Am 13.04.2016 um 23:22 schrieb Alexander Graf:
When there is no $fdtfile variable set, we still have a good chance that on 32bit arm the fdtfile really is just called $soc-$board.dtb.
Enable the exports for $soc and $board in our distr defaults and make use of them in the efi boot script.
Reported-by: Andreas Faerber afaerber@suse.de Reported-by: Stephen Warren swarren@wwwdotorg.org Signed-off-by: Alexander Graf agraf@suse.de
include/config_distro_bootcmd.h | 24 +++++++++++++++++++++--- include/config_distro_defaults.h | 1 + 2 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h index 67eb8f2..eaaf2cc 100644 --- a/include/config_distro_bootcmd.h +++ b/include/config_distro_bootcmd.h @@ -99,6 +99,21 @@ #endif
#ifdef BOOTEFI_NAME +#if defined(CONFIG_ARM) && !defined(CONFIG_ARM64) +/*
- On 32bit ARM systems there is a reasonable number of systems that follow
- the $soc-$board$boardver.dtb name scheme for their device trees. Use that
- scheme if we don't have an explicit fdtfile variable.
- */
Why limit this to 32-bit? If fdtfile is not set and we drop the limitation above, then for CONFIG_ARM64 (and theoretically MIPS) we could add the vendor subdir to our prefixes. Even without the latter it does no harm, your config_distro_defaults.h change below does not seem to be limited to ARM anyway.
+#define BOOTENV_EFI_SET_FDTFILE_FALLBACK \
- "if test -z "${fdtfile}" -a -n "${soc}"; then " \
"setenv efifdtfile ${soc}-${board}${boardver}.dtb; " \
Please consistently use efi_ for readability.
The logic looks slightly weird on first sight, being separated from the below context. Can't we just set fdtfile here if unset and drop the four extra lines below? Once boot has executed, there may be leftover variables such as filesize anyway.
Regards, Andreas
- "fi; "
+#else +#define BOOTENV_EFI_SET_FDTFILE_FALLBACK +#endif
#define BOOTENV_SHARED_EFI \ "boot_efi_binary=" \ "load ${devtype} ${devnum}:${distro_bootpart} " \ @@ -111,14 +126,16 @@ \ "load_efi_dtb=" \ "load ${devtype} ${devnum}:${distro_bootpart} " \
"${fdt_addr_r} ${prefix}${fdtfile}\0" \
"${fdt_addr_r} ${prefix}${efifdtfile}\0" \"setenv efifdtfile ${fdtfile}; " \BOOTENV_EFI_SET_FDTFILE_FALLBACK \
"${prefix}${fdtfile}; then " \
"${prefix}${efifdtfile}; then " \ "run load_efi_dtb; " \ "fi;" \@@ -128,7 +145,8 @@ "efi/boot/"BOOTEFI_NAME"; " \ "run boot_efi_binary; " \ "echo EFI LOAD FAILED: continuing...; " \
"fi; \0"
"fi; " \"setenv efifdtfile\0"#define SCAN_DEV_FOR_EFI "run scan_dev_for_efi;" #else #define BOOTENV_SHARED_EFI diff --git a/include/config_distro_defaults.h b/include/config_distro_defaults.h index 076be4d..ae68e3a 100644 --- a/include/config_distro_defaults.h +++ b/include/config_distro_defaults.h @@ -65,5 +65,6 @@ #define CONFIG_ISO_PARTITION #define CONFIG_SUPPORT_RAW_INITRD #define CONFIG_SYS_HUSH_PARSER +#define CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG
#endif /* _CONFIG_CMD_DISTRO_DEFAULTS_H */
On 04/14/2016 03:43 PM, Andreas Färber wrote:
Am 13.04.2016 um 23:22 schrieb Alexander Graf:
When there is no $fdtfile variable set, we still have a good chance that on 32bit arm the fdtfile really is just called $soc-$board.dtb.
Enable the exports for $soc and $board in our distr defaults and make use of them in the efi boot script.
Reported-by: Andreas Faerber afaerber@suse.de Reported-by: Stephen Warren swarren@wwwdotorg.org Signed-off-by: Alexander Graf agraf@suse.de
include/config_distro_bootcmd.h | 24 +++++++++++++++++++++--- include/config_distro_defaults.h | 1 + 2 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h index 67eb8f2..eaaf2cc 100644 --- a/include/config_distro_bootcmd.h +++ b/include/config_distro_bootcmd.h @@ -99,6 +99,21 @@ #endif
#ifdef BOOTEFI_NAME +#if defined(CONFIG_ARM) && !defined(CONFIG_ARM64) +/*
- On 32bit ARM systems there is a reasonable number of systems that follow
- the $soc-$board$boardver.dtb name scheme for their device trees. Use that
- scheme if we don't have an explicit fdtfile variable.
- */
Why limit this to 32-bit? If fdtfile is not set and we drop the limitation above, then for CONFIG_ARM64 (and theoretically MIPS) we could add the vendor subdir to our prefixes. Even without the latter it does no harm, your config_distro_defaults.h change below does not seem to be limited to ARM anyway.
For ARM64 the pattern doesn't work because of the subdirectories, so we'll have to have a separate block that takes the soc vendor name into account as well. Does MIPS use dtb these days?
+#define BOOTENV_EFI_SET_FDTFILE_FALLBACK \
- "if test -z "${fdtfile}" -a -n "${soc}"; then " \
"setenv efifdtfile ${soc}-${board}${boardver}.dtb; " \Please consistently use efi_ for readability.
The logic looks slightly weird on first sight, being separated from the below context. Can't we just set fdtfile here if unset and drop the four extra lines below? Once boot has executed, there may be leftover variables such as filesize anyway.
The problem is that the boot may fail - and in that case you would have a potentially invalid fdtfile variable.
Alex
Am 13.04.2016 um 23:22 schrieb Alexander Graf:
The bootefi cmd today fetches its device tree pointer from either the location appointed by "fdt addr" with a fallback to the U-Boot control fdt.
This integration is unusual for U-Boot and diverges from the way we usually handle parameters to boot commands. So let's pass the fdt directly into the bootefi command instead and move the control fdt logic into the distro boot script.
Signed-off-by: Alexander Graf agraf@suse.de
cmd/bootefi.c | 34 +++++++++++++--------------------- include/config_distro_bootcmd.h | 9 ++++++--- 2 files changed, 19 insertions(+), 24 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index b213ef1..ab39b95 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -142,12 +142,11 @@ static void *copy_fdt(void *fdt)
- Load an EFI payload into a newly allocated piece of memory, register all
- EFI objects it would want to access and jump to it.
*/ -static unsigned long do_bootefi_exec(void *efi) +static unsigned long do_bootefi_exec(void *efi, void *fdt) { ulong (*entry)(void *image_handle, struct efi_system_table *st); ulong fdt_pages, fdt_size, fdt_start, fdt_end; bootm_headers_t img = { 0 };
void *fdt = working_fdt;
/*
- gd lives in a fixed register which may get clobbered while we execute
@@ -155,13 +154,7 @@ static unsigned long do_bootefi_exec(void *efi) */ efi_save_gd();
- /* Update system table to point to our currently loaded FDT */
- /* Fall back to included fdt if none was manually loaded */
- if (!fdt && gd->fdt_blob)
fdt = (void *)gd->fdt_blob;- if (fdt) {
- if (fdt && !fdt_check_header(fdt)) { /* Prepare fdt for payload */ fdt = copy_fdt(fdt);
@@ -185,7 +178,7 @@ static unsigned long do_bootefi_exec(void *efi) efi_add_memory_map(fdt_start, fdt_pages, EFI_BOOT_SERVICES_DATA, true); } else {
printf("WARNING: No device tree loaded, expect boot to fail\n");
printf("WARNING: Invalid device tree, expect boot to fail\n");@@ -216,18 +209,20 @@ static unsigned long do_bootefi_exec(void *efi) /* Interpreter command to boot an arbitrary EFI image from memory */ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) {
- char *saddr;
- unsigned long addr;
- char *saddr, *sfdt;
- unsigned long addr, fdt_addr; int r = 0;
- if (argc < 2)
- if (argc < 3) return 1;
Hm, I had specifically requested to make it an optional argument... do_bootefi_exec() seems to still handle !fdt, but here we seem to choke on absence of the third argument, not matching bootm/bootz/booti? GRUB does not need a DT to display its shell/menu, so we don't need to force it IMO. Think of people calling it from the prompt or from a script.
Or am I misunderstanding U-Boot argument handling?
saddr = argv[1];
sfdt = argv[2];
addr = simple_strtoul(saddr, NULL, 16);
fdt_addr = simple_strtoul(sfdt, NULL, 16);
printf("## Starting EFI application at 0x%08lx ...\n", addr);
- r = do_bootefi_exec((void *)addr);
r = do_bootefi_exec((void *)addr, (void*)fdt_addr); printf("## Application terminated, r = %d\n", r);
if (r != 0)
@@ -238,16 +233,13 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
#ifdef CONFIG_SYS_LONGHELP static char bootefi_help_text[] =
- "<image address>\n"
- " - boot EFI payload stored at address <image address>\n"
- "\n"
- "Since most EFI payloads want to have a device tree provided, please\n"
- "make sure you load a device tree using the fdt addr command before\n"
- "executing bootefi.\n";
- "<image address> <fdt address>\n"
- " - boot EFI payload stored at address <image address> with a device\n"
- " tree located at <fdt address>\n";
Nit: This could get a better explanation than "with a ...". :) U-Boot exposes it as some table/hook/callback/etc.?
#endif
U_BOOT_CMD(
- bootefi, 2, 0, do_bootefi,
- bootefi, 3, 0, do_bootefi, "Boots an EFI payload from memory\n", bootefi_help_text
); diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h index ad9045e..67eb8f2 100644 --- a/include/config_distro_bootcmd.h +++ b/include/config_distro_bootcmd.h @@ -103,12 +103,15 @@ "boot_efi_binary=" \ "load ${devtype} ${devnum}:${distro_bootpart} " \ "${kernel_addr_r} efi/boot/"BOOTEFI_NAME"; " \
"bootefi ${kernel_addr_r}\0" \
"if fdt addr ${fdt_addr_r}; then " \"bootefi ${kernel_addr_r} ${fdt_addr_r};" \"else" \
Spaces vs. tabs?
"bootefi ${kernel_addr_r} ${fdtcontroladdr};" \"fi\0" \
"${fdt_addr_r} ${prefix}${fdtfile}; " \"fdt addr ${fdt_addr_r}\0" \
"${fdt_addr_r} ${prefix}${fdtfile}\0" \
Regards, Andreas
On 14.04.16 00:26, Andreas Färber wrote:
Am 13.04.2016 um 23:22 schrieb Alexander Graf:
The bootefi cmd today fetches its device tree pointer from either the location appointed by "fdt addr" with a fallback to the U-Boot control fdt.
This integration is unusual for U-Boot and diverges from the way we usually handle parameters to boot commands. So let's pass the fdt directly into the bootefi command instead and move the control fdt logic into the distro boot script.
Signed-off-by: Alexander Graf agraf@suse.de
cmd/bootefi.c | 34 +++++++++++++--------------------- include/config_distro_bootcmd.h | 9 ++++++--- 2 files changed, 19 insertions(+), 24 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index b213ef1..ab39b95 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -142,12 +142,11 @@ static void *copy_fdt(void *fdt)
- Load an EFI payload into a newly allocated piece of memory, register all
- EFI objects it would want to access and jump to it.
*/ -static unsigned long do_bootefi_exec(void *efi) +static unsigned long do_bootefi_exec(void *efi, void *fdt) { ulong (*entry)(void *image_handle, struct efi_system_table *st); ulong fdt_pages, fdt_size, fdt_start, fdt_end; bootm_headers_t img = { 0 };
void *fdt = working_fdt;
/*
- gd lives in a fixed register which may get clobbered while we execute
@@ -155,13 +154,7 @@ static unsigned long do_bootefi_exec(void *efi) */ efi_save_gd();
- /* Update system table to point to our currently loaded FDT */
- /* Fall back to included fdt if none was manually loaded */
- if (!fdt && gd->fdt_blob)
fdt = (void *)gd->fdt_blob;- if (fdt) {
- if (fdt && !fdt_check_header(fdt)) { /* Prepare fdt for payload */ fdt = copy_fdt(fdt);
@@ -185,7 +178,7 @@ static unsigned long do_bootefi_exec(void *efi) efi_add_memory_map(fdt_start, fdt_pages, EFI_BOOT_SERVICES_DATA, true); } else {
printf("WARNING: No device tree loaded, expect boot to fail\n");
printf("WARNING: Invalid device tree, expect boot to fail\n");@@ -216,18 +209,20 @@ static unsigned long do_bootefi_exec(void *efi) /* Interpreter command to boot an arbitrary EFI image from memory */ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) {
- char *saddr;
- unsigned long addr;
- char *saddr, *sfdt;
- unsigned long addr, fdt_addr; int r = 0;
- if (argc < 2)
- if (argc < 3) return 1;
Hm, I had specifically requested to make it an optional argument... do_bootefi_exec() seems to still handle !fdt, but here we seem to choke on absence of the third argument, not matching bootm/bootz/booti? GRUB does not need a DT to display its shell/menu, so we don't need to force it IMO. Think of people calling it from the prompt or from a script.
Or am I misunderstanding U-Boot argument handling?
I thought that Stephen's argument was that booting Linux without a working device tree is moot anyway. I tend to agree ;).
If you really want to boot without a device tree, you can always just pass in "-" as the fdt argument. But by requiring a device tree we hopefully get less confused people using the uefi support.
Alex
On 04/13/2016 03:22 PM, Alexander Graf wrote:
The bootefi cmd today fetches its device tree pointer from either the location appointed by "fdt addr" with a fallback to the U-Boot control fdt.
This integration is unusual for U-Boot and diverges from the way we usually handle parameters to boot commands. So let's pass the fdt directly into the bootefi command instead and move the control fdt logic into the distro boot script.
diff --git a/cmd/bootefi.c b/cmd/bootefi.c
@@ -155,13 +154,7 @@ static unsigned long do_bootefi_exec(void *efi) */ efi_save_gd();
- /* Update system table to point to our currently loaded FDT */
- /* Fall back to included fdt if none was manually loaded */
- if (!fdt && gd->fdt_blob)
fdt = (void *)gd->fdt_blob;- if (fdt) {
- if (fdt && !fdt_check_header(fdt)) { /* Prepare fdt for payload */ fdt = copy_fdt(fdt);
@@ -185,7 +178,7 @@ static unsigned long do_bootefi_exec(void *efi) efi_add_memory_map(fdt_start, fdt_pages, EFI_BOOT_SERVICES_DATA, true); } else {
printf("WARNING: No device tree loaded, expect boot to fail\n");
printf("WARNING: Invalid device tree, expect boot to fail\n");
I'm not familiar with this code, so I'm not really sure what the implication of this if/else is.
Still, this looks like it's a move in the right direction. I do agree the FDT address parameter likely should be optional, and if not specified by the user default to whatever other bootz/booti default too (I suspect $fdt_addr_r).
Thanks for fixing this up so quickly.
On 04/14/2016 01:20 AM, Stephen Warren wrote:
On 04/13/2016 03:22 PM, Alexander Graf wrote:
The bootefi cmd today fetches its device tree pointer from either the location appointed by "fdt addr" with a fallback to the U-Boot control fdt.
This integration is unusual for U-Boot and diverges from the way we usually handle parameters to boot commands. So let's pass the fdt directly into the bootefi command instead and move the control fdt logic into the distro boot script.
diff --git a/cmd/bootefi.c b/cmd/bootefi.c
@@ -155,13 +154,7 @@ static unsigned long do_bootefi_exec(void *efi) */ efi_save_gd();
- /* Update system table to point to our currently loaded FDT */
- /* Fall back to included fdt if none was manually loaded */
- if (!fdt && gd->fdt_blob)
fdt = (void *)gd->fdt_blob;- if (fdt) {
- if (fdt && !fdt_check_header(fdt)) { /* Prepare fdt for payload */ fdt = copy_fdt(fdt);
@@ -185,7 +178,7 @@ static unsigned long do_bootefi_exec(void *efi) efi_add_memory_map(fdt_start, fdt_pages, EFI_BOOT_SERVICES_DATA, true); } else {
printf("WARNING: No device tree loaded, expect boot tofail\n");
printf("WARNING: Invalid device tree, expect boot to fail\n");I'm not familiar with this code, so I'm not really sure what the implication of this if/else is.
Still, this looks like it's a move in the right direction. I do agree the FDT address parameter likely should be optional, and if not specified by the user default to whatever other bootz/booti default too (I suspect $fdt_addr_r).
So what bootz/booti do is to either take the 3rd parameter as fdt, have the fdt included in the image you boot (FIT or legacy combined images) and when it's loaded, they call
set_working_fdt_addr((ulong)images.ft_addr);
which basically does the same as my previous "fdt addr". But I see no fallback to $fdt_addr_r. It just doesn't use an fdt if no fdt parameter / fit image was provided.
Alex
On 04/14/2016 07:48 AM, Alexander Graf wrote:
On 04/14/2016 01:20 AM, Stephen Warren wrote:
On 04/13/2016 03:22 PM, Alexander Graf wrote:
The bootefi cmd today fetches its device tree pointer from either the location appointed by "fdt addr" with a fallback to the U-Boot control fdt.
This integration is unusual for U-Boot and diverges from the way we usually handle parameters to boot commands. So let's pass the fdt directly into the bootefi command instead and move the control fdt logic into the distro boot script.
diff --git a/cmd/bootefi.c b/cmd/bootefi.c
@@ -155,13 +154,7 @@ static unsigned long do_bootefi_exec(void *efi) */ efi_save_gd();
- /* Update system table to point to our currently loaded FDT */
- /* Fall back to included fdt if none was manually loaded */
- if (!fdt && gd->fdt_blob)
fdt = (void *)gd->fdt_blob;- if (fdt) {
- if (fdt && !fdt_check_header(fdt)) { /* Prepare fdt for payload */ fdt = copy_fdt(fdt);
@@ -185,7 +178,7 @@ static unsigned long do_bootefi_exec(void *efi) efi_add_memory_map(fdt_start, fdt_pages, EFI_BOOT_SERVICES_DATA, true); } else {
printf("WARNING: No device tree loaded, expect boot tofail\n");
printf("WARNING: Invalid device tree, expect boot to fail\n");I'm not familiar with this code, so I'm not really sure what the implication of this if/else is.
Still, this looks like it's a move in the right direction. I do agree the FDT address parameter likely should be optional, and if not specified by the user default to whatever other bootz/booti default too (I suspect $fdt_addr_r).
So what bootz/booti do is to either take the 3rd parameter as fdt, have the fdt included in the image you boot (FIT or legacy combined images) and when it's loaded, they call
set_working_fdt_addr((ulong)images.ft_addr);
which basically does the same as my previous "fdt addr". But I see no fallback to $fdt_addr_r. It just doesn't use an fdt if no fdt parameter / fit image was provided.
Ah yes, I guess that's true. No DTB parameter to the command means ATAGS boot for bootz at least, right? I'd expect booti to be different, but I haven't looked at that.
participants (3)
-
Alexander Graf -
Andreas Färber -
Stephen Warren