All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH 1/2] efi_loader: Pass fdt address directly to bootefi cmd
@ 2016-04-13 21:22 Alexander Graf
  2016-04-13 21:22 ` [U-Boot] [PATCH 2/2] efi_loader: Fall back to fdtfile naming convention Alexander Graf
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Alexander Graf @ 2016-04-13 21:22 UTC (permalink / raw)
  To: u-boot

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@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="                                               \
-- 
1.8.5.6

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [U-Boot] [PATCH 2/2] efi_loader: Fall back to fdtfile naming convention
  2016-04-13 21:22 [U-Boot] [PATCH 1/2] efi_loader: Pass fdt address directly to bootefi cmd Alexander Graf
@ 2016-04-13 21:22 ` Alexander Graf
  2016-04-13 23:24   ` Stephen Warren
  2016-04-14 13:43   ` Andreas Färber
  2016-04-13 22:26 ` [U-Boot] [PATCH 1/2] efi_loader: Pass fdt address directly to bootefi cmd Andreas Färber
  2016-04-13 23:20 ` Stephen Warren
  2 siblings, 2 replies; 11+ messages in thread
From: Alexander Graf @ 2016-04-13 21:22 UTC (permalink / raw)
  To: u-boot

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 */
-- 
1.8.5.6

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [U-Boot] [PATCH 1/2] efi_loader: Pass fdt address directly to bootefi cmd
  2016-04-13 21:22 [U-Boot] [PATCH 1/2] efi_loader: Pass fdt address directly to bootefi cmd Alexander Graf
  2016-04-13 21:22 ` [U-Boot] [PATCH 2/2] efi_loader: Fall back to fdtfile naming convention Alexander Graf
@ 2016-04-13 22:26 ` Andreas Färber
  2016-04-13 22:41   ` Alexander Graf
  2016-04-13 23:20 ` Stephen Warren
  2 siblings, 1 reply; 11+ messages in thread
From: Andreas Färber @ 2016-04-13 22:26 UTC (permalink / raw)
  To: u-boot

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");
>  		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;

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"                                                    \
>  	\
>  	"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="                                               \

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 N?rnberg, Germany
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [U-Boot] [PATCH 1/2] efi_loader: Pass fdt address directly to bootefi cmd
  2016-04-13 22:26 ` [U-Boot] [PATCH 1/2] efi_loader: Pass fdt address directly to bootefi cmd Andreas Färber
@ 2016-04-13 22:41   ` Alexander Graf
  0 siblings, 0 replies; 11+ messages in thread
From: Alexander Graf @ 2016-04-13 22:41 UTC (permalink / raw)
  To: u-boot



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");
>>  		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;
> 
> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [U-Boot] [PATCH 1/2] efi_loader: Pass fdt address directly to bootefi cmd
  2016-04-13 21:22 [U-Boot] [PATCH 1/2] efi_loader: Pass fdt address directly to bootefi cmd Alexander Graf
  2016-04-13 21:22 ` [U-Boot] [PATCH 2/2] efi_loader: Fall back to fdtfile naming convention Alexander Graf
  2016-04-13 22:26 ` [U-Boot] [PATCH 1/2] efi_loader: Pass fdt address directly to bootefi cmd Andreas Färber
@ 2016-04-13 23:20 ` Stephen Warren
  2016-04-14 13:48   ` Alexander Graf
  2 siblings, 1 reply; 11+ messages in thread
From: Stephen Warren @ 2016-04-13 23:20 UTC (permalink / raw)
  To: u-boot

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [U-Boot] [PATCH 2/2] efi_loader: Fall back to fdtfile naming convention
  2016-04-13 21:22 ` [U-Boot] [PATCH 2/2] efi_loader: Fall back to fdtfile naming convention Alexander Graf
@ 2016-04-13 23:24   ` Stephen Warren
  2016-04-14 13:53     ` Alexander Graf
  2016-04-14 13:43   ` Andreas Färber
  1 sibling, 1 reply; 11+ messages in thread
From: Stephen Warren @ 2016-04-13 23:24 UTC (permalink / raw)
  To: u-boot

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [U-Boot] [PATCH 2/2] efi_loader: Fall back to fdtfile naming convention
  2016-04-13 21:22 ` [U-Boot] [PATCH 2/2] efi_loader: Fall back to fdtfile naming convention Alexander Graf
  2016-04-13 23:24   ` Stephen Warren
@ 2016-04-14 13:43   ` Andreas Färber
  2016-04-14 13:46     ` Alexander Graf
  1 sibling, 1 reply; 11+ messages in thread
From: Andreas Färber @ 2016-04-14 13:43 UTC (permalink / raw)
  To: u-boot

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"          \
>  	\
>  	"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 */
> 


-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 N?rnberg, Germany
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [U-Boot] [PATCH 2/2] efi_loader: Fall back to fdtfile naming convention
  2016-04-14 13:43   ` Andreas Färber
@ 2016-04-14 13:46     ` Alexander Graf
  0 siblings, 0 replies; 11+ messages in thread
From: Alexander Graf @ 2016-04-14 13:46 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [U-Boot] [PATCH 1/2] efi_loader: Pass fdt address directly to bootefi cmd
  2016-04-13 23:20 ` Stephen Warren
@ 2016-04-14 13:48   ` Alexander Graf
  2016-04-14 15:27     ` Stephen Warren
  0 siblings, 1 reply; 11+ messages in thread
From: Alexander Graf @ 2016-04-14 13:48 UTC (permalink / raw)
  To: u-boot

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 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).

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [U-Boot] [PATCH 2/2] efi_loader: Fall back to fdtfile naming convention
  2016-04-13 23:24   ` Stephen Warren
@ 2016-04-14 13:53     ` Alexander Graf
  0 siblings, 0 replies; 11+ messages in thread
From: Alexander Graf @ 2016-04-14 13:53 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [U-Boot] [PATCH 1/2] efi_loader: Pass fdt address directly to bootefi cmd
  2016-04-14 13:48   ` Alexander Graf
@ 2016-04-14 15:27     ` Stephen Warren
  0 siblings, 0 replies; 11+ messages in thread
From: Stephen Warren @ 2016-04-14 15:27 UTC (permalink / raw)
  To: u-boot

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 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).
>
> 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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2016-04-14 15:27 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-13 21:22 [U-Boot] [PATCH 1/2] efi_loader: Pass fdt address directly to bootefi cmd Alexander Graf
2016-04-13 21:22 ` [U-Boot] [PATCH 2/2] efi_loader: Fall back to fdtfile naming convention Alexander Graf
2016-04-13 23:24   ` Stephen Warren
2016-04-14 13:53     ` Alexander Graf
2016-04-14 13:43   ` Andreas Färber
2016-04-14 13:46     ` Alexander Graf
2016-04-13 22:26 ` [U-Boot] [PATCH 1/2] efi_loader: Pass fdt address directly to bootefi cmd Andreas Färber
2016-04-13 22:41   ` Alexander Graf
2016-04-13 23:20 ` Stephen Warren
2016-04-14 13:48   ` Alexander Graf
2016-04-14 15:27     ` Stephen Warren

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.