[U-Boot] [PATCH 0/6] efi_loader: Improvements to .dtb handling

[U-Boot] [PATCH 0/6] efi_loader: Improvements to .dtb handling

[Andreas Färber]

Hi,

This series modifies the distro boot scripts to better cope with real-world
.dtb scenarios. In particular openSUSE images have the EFI GRUB2 and optional
.dtb files on separate partitions (fat vs. ext4) and installed to /boot/dtb-foo
symlink, which may be /dtb or /boot/dtb for U-Boot. The goal is to obsolete
less sophisticated openSUSE patches and to improve the testing experience.

Further, while testing on the DragonBoard 410c, I ran into U-Boot vs. Linux
naming inconsistencies once again and suggest a workaround: ${soc_vendor}.

For easier review a cleanup patch is prepended.

Regards,
Andreas

Cc: Alexander Graf <agraf@suse.de>
Cc: Tom Rini <trini@konsulko.com>
Cc: Stephen Warren <swarren@wwwdotorg.org>

Andreas F?rber (6):
  efi_loader: Cosmetic distro script cleanups
  efi_loader: Respect $boot_prefixes for EFI .dtb search
  efi_loader: Search .dtb on non-EFI partitions
  efi_loader: Improve .dtb search for arm64
  dragonboard410c: Set soc_vendor
  efi_loader: Display which .dtb we found

 include/config_distro_bootcmd.h   | 59 ++++++++++++++++++++++++++++++---------
 include/configs/dragonboard410c.h |  1 +
 2 files changed, 47 insertions(+), 13 deletions(-)

-- 
2.6.6

[U-Boot] [PATCH 1/6] efi_loader: Cosmetic distro script cleanups

[Andreas Färber]

Fix a macro line break whose alignment slipped in commit fba5f93.
Be consistent in having trailing spaces after semicolon.
While at it, drop a duplicate white line.

Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Andreas F?rber <afaerber@suse.de>
---
 include/config_distro_bootcmd.h | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
index 9ecaf38..b1c9d36 100644
--- a/include/config_distro_bootcmd.h
+++ b/include/config_distro_bootcmd.h
@@ -113,15 +113,14 @@
 #define BOOTENV_EFI_SET_FDTFILE_FALLBACK
 #endif
 
-
 #define BOOTENV_SHARED_EFI                                                \
 	"boot_efi_binary="                                                \
 		"load ${devtype} ${devnum}:${distro_bootpart} "           \
 			"${kernel_addr_r} efi/boot/"BOOTEFI_NAME"; "      \
 		"if fdt addr ${fdt_addr_r}; then "                        \
-			"bootefi ${kernel_addr_r} ${fdt_addr_r};"         \
-		"else "                                                    \
-			"bootefi ${kernel_addr_r} ${fdtcontroladdr};"     \
+			"bootefi ${kernel_addr_r} ${fdt_addr_r}; "        \
+		"else "                                                   \
+			"bootefi ${kernel_addr_r} ${fdtcontroladdr}; "    \
 		"fi\0"                                                    \
 	\
 	"load_efi_dtb="                                                   \
@@ -137,8 +136,8 @@
 					"${devnum}:${distro_bootpart} "   \
 					"${prefix}${efi_fdtfile}; then "  \
 				"run load_efi_dtb; "                      \
-			"fi;"                                             \
-		"done;"                                                   \
+			"fi; "                                            \
+		"done; "                                                  \
 		"if test -e ${devtype} ${devnum}:${distro_bootpart} "     \
 					"efi/boot/"BOOTEFI_NAME"; then "  \
 				"echo Found EFI removable media binary "  \
-- 
2.6.6

[U-Boot] [PATCH 2/6] efi_loader: Respect $boot_prefixes for EFI .dtb search

[Andreas Färber]

Just like boot configs or scripts, .dtb files may be in /boot.
Search $efi_dtb_prefixes for all $boot_prefixes.

Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Andreas F?rber <afaerber@suse.de>
---
 include/config_distro_bootcmd.h | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
index b1c9d36..087f576 100644
--- a/include/config_distro_bootcmd.h
+++ b/include/config_distro_bootcmd.h
@@ -125,19 +125,25 @@
 	\
 	"load_efi_dtb="                                                   \
 		"load ${devtype} ${devnum}:${distro_bootpart} "           \
-			"${fdt_addr_r} ${prefix}${efi_fdtfile}\0"         \
+			"${fdt_addr_r} "                                  \
+			"${prefix}${dtb_prefix}${efi_fdtfile}\0"          \
 	\
-	"efi_dtb_prefixes=/ /dtb/ /dtb/current/\0"                        \
+	"efi_dtb_prefixes=\"\" dtb/ dtb/current/\0"                       \
+	"scan_dev_for_efi_fdt="                                           \
+		"for prefix in ${boot_prefixes}; do "                     \
+			"for dtb_prefix in ${efi_dtb_prefixes}; do "      \
+				"if test -e ${devtype} "                  \
+					"${devnum}:${distro_bootpart} "   \
+					"${prefix}${dtb_prefix}"          \
+					"${efi_fdtfile}; then "           \
+					"run load_efi_dtb; "              \
+				"fi; "                                    \
+			"done; "                                          \
+		"done\0"                                                  \
 	"scan_dev_for_efi="                                               \
 		"setenv efi_fdtfile ${fdtfile}; "                         \
 		BOOTENV_EFI_SET_FDTFILE_FALLBACK                          \
-		"for prefix in ${efi_dtb_prefixes}; do "                  \
-			"if test -e ${devtype} "                          \
-					"${devnum}:${distro_bootpart} "   \
-					"${prefix}${efi_fdtfile}; then "  \
-				"run load_efi_dtb; "                      \
-			"fi; "                                            \
-		"done; "                                                  \
+		"run scan_dev_for_efi_fdt; "                              \
 		"if test -e ${devtype} ${devnum}:${distro_bootpart} "     \
 					"efi/boot/"BOOTEFI_NAME"; then "  \
 				"echo Found EFI removable media binary "  \
-- 
2.6.6

[U-Boot] [PATCH 3/6] efi_loader: Search .dtb on non-EFI partitions

[Andreas Färber]

A UEFI setup will typically have an EFI FAT partition, but device trees
are more likely to be located on, e.g., Linux volumes. By convention the
EFI partition will usually be placed first, so that we would find the
EFI removable media binary and enter it without device tree from disk.

Therefore after finding an EFI removable media binary, probe other
partitions on the same device for .dtb files.

In the default case the behavior will be unchanged in that only the
first partition is searched.
In the case that there are two partitions marked bootable and the first
being EFI, we will first check the first partition, then the second
partition for the .dtb.
Duplicate loading will only result from non-first EFI partition order.

Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Andreas F?rber <afaerber@suse.de>
---
 include/config_distro_bootcmd.h | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
index 087f576..eadec2e 100644
--- a/include/config_distro_bootcmd.h
+++ b/include/config_distro_bootcmd.h
@@ -124,8 +124,7 @@
 		"fi\0"                                                    \
 	\
 	"load_efi_dtb="                                                   \
-		"load ${devtype} ${devnum}:${distro_bootpart} "           \
-			"${fdt_addr_r} "                                  \
+		"load ${devtype} ${devnum}:${efi_bootpart} ${fdt_addr_r} "\
 			"${prefix}${dtb_prefix}${efi_fdtfile}\0"          \
 	\
 	"efi_dtb_prefixes=\"\" dtb/ dtb/current/\0"                       \
@@ -133,7 +132,7 @@
 		"for prefix in ${boot_prefixes}; do "                     \
 			"for dtb_prefix in ${efi_dtb_prefixes}; do "      \
 				"if test -e ${devtype} "                  \
-					"${devnum}:${distro_bootpart} "   \
+					"${devnum}:${efi_bootpart} "      \
 					"${prefix}${dtb_prefix}"          \
 					"${efi_fdtfile}; then "           \
 					"run load_efi_dtb; "              \
@@ -143,13 +142,22 @@
 	"scan_dev_for_efi="                                               \
 		"setenv efi_fdtfile ${fdtfile}; "                         \
 		BOOTENV_EFI_SET_FDTFILE_FALLBACK                          \
-		"run scan_dev_for_efi_fdt; "                              \
 		"if test -e ${devtype} ${devnum}:${distro_bootpart} "     \
 					"efi/boot/"BOOTEFI_NAME"; then "  \
 				"echo Found EFI removable media binary "  \
 					"efi/boot/"BOOTEFI_NAME"; "       \
+				"for efi_bootpart in ${devplist}; do "    \
+					"echo Scanning for device tree "  \
+						"on ${devtype} ${devnum}" \
+						":${efi_bootpart}...; "   \
+					"run scan_dev_for_efi_fdt; "      \
+				"done; "                                  \
 				"run boot_efi_binary; "                   \
 				"echo EFI LOAD FAILED: continuing...; "   \
+		"else "                                                   \
+			"setenv efi_bootpart ${distro_bootpart}; "        \
+			"run scan_dev_for_efi_fdt; "                      \
+			"setenv efi_bootpart; "                           \
 		"fi; "                                                    \
 		"setenv efi_fdtfile\0"
 #define SCAN_DEV_FOR_EFI "run scan_dev_for_efi;"
-- 
2.6.6

[U-Boot] [PATCH 4/6] efi_loader: Improve .dtb search for arm64

[Andreas Färber]

On arm64 Linux device trees are organized by SoC vendor. Therefore we
need to search the vendor subdirectory as well.

Since the SoC vendor may be different from ${vendor}, introduce a new
${soc_vendor}. If this is not set, the behavior remains unchanged.

Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Andreas F?rber <afaerber@suse.de>
---
 include/config_distro_bootcmd.h | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
index eadec2e..8f14457 100644
--- a/include/config_distro_bootcmd.h
+++ b/include/config_distro_bootcmd.h
@@ -113,6 +113,15 @@
 #define BOOTENV_EFI_SET_FDTFILE_FALLBACK
 #endif
 
+#if defined(CONFIG_ARM64)
+#define BOOTENV_EFI_SET_FDTFILE_VENDOR                                    \
+	"if test -n \"${soc_vendor}\"; then "                             \
+		"setenv efi_dtb_vendor_prefix ${soc_vendor}/; "           \
+	"fi; "
+#else
+#define BOOTENV_EFI_SET_FDTFILE_VENDOR
+#endif
+
 #define BOOTENV_SHARED_EFI                                                \
 	"boot_efi_binary="                                                \
 		"load ${devtype} ${devnum}:${distro_bootpart} "           \
@@ -125,18 +134,25 @@
 	\
 	"load_efi_dtb="                                                   \
 		"load ${devtype} ${devnum}:${efi_bootpart} ${fdt_addr_r} "\
-			"${prefix}${dtb_prefix}${efi_fdtfile}\0"          \
+			"${prefix}${dtb_prefix}${dtb_vendor_prefix}"      \
+			"${efi_fdtfile}\0"                                \
 	\
 	"efi_dtb_prefixes=\"\" dtb/ dtb/current/\0"                       \
 	"scan_dev_for_efi_fdt="                                           \
 		"for prefix in ${boot_prefixes}; do "                     \
 			"for dtb_prefix in ${efi_dtb_prefixes}; do "      \
+			    BOOTENV_EFI_SET_FDTFILE_VENDOR                \
+			    "for dtb_vendor_prefix in \"\" "              \
+					"${efi_dtb_vendor_prefix}; do "   \
 				"if test -e ${devtype} "                  \
 					"${devnum}:${efi_bootpart} "      \
 					"${prefix}${dtb_prefix}"          \
+					"${dtb_vendor_prefix}"            \
 					"${efi_fdtfile}; then "           \
 					"run load_efi_dtb; "              \
 				"fi; "                                    \
+			    "done; "                                      \
+			    "setenv efi_dtb_vendor_prefix; "              \
 			"done; "                                          \
 		"done\0"                                                  \
 	"scan_dev_for_efi="                                               \
-- 
2.6.6

[U-Boot] [PATCH 5/6] dragonboard410c: Set soc_vendor

[Andreas Färber]

Signed-off-by: Andreas F?rber <afaerber@suse.de>
---
 include/configs/dragonboard410c.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/configs/dragonboard410c.h b/include/configs/dragonboard410c.h
index 74a827d..8b019e9 100644
--- a/include/configs/dragonboard410c.h
+++ b/include/configs/dragonboard410c.h
@@ -124,6 +124,7 @@ REFLASH(dragonboard/u-boot.img, 8)\
 	"linux_image=Image\0" \
 	"kernel_addr_r=0x81000000\0"\
 	"fdtfile=apq8016-sbc.dtb\0" \
+	"soc_vendor=qcom\0" \
 	"fdt_addr_r=0x83000000\0"\
 	"ramdisk_addr_r=0x84000000\0"\
 	BOOTENV
-- 
2.6.6

[U-Boot] [PATCH 6/6] efi_loader: Display which .dtb we found

[Andreas Färber]

We do so for the EFI binary already and it aids debugging.

Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Andreas F?rber <afaerber@suse.de>
---
 include/config_distro_bootcmd.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
index 8f14457..0cf74e2 100644
--- a/include/config_distro_bootcmd.h
+++ b/include/config_distro_bootcmd.h
@@ -149,6 +149,10 @@
 					"${prefix}${dtb_prefix}"          \
 					"${dtb_vendor_prefix}"            \
 					"${efi_fdtfile}; then "           \
+					"echo Found ${prefix}"            \
+						"${dtb_prefix}"           \
+						"${dtb_vendor_prefix}"    \
+						"${efi_fdtfile}; "        \
 					"run load_efi_dtb; "              \
 				"fi; "                                    \
 			    "done; "                                      \
-- 
2.6.6

[U-Boot] [PATCH 3/6] efi_loader: Search .dtb on non-EFI partitions

[Alexander Graf]

On 12.07.16 06:21, Andreas F?rber wrote:
> A UEFI setup will typically have an EFI FAT partition, but device trees
> are more likely to be located on, e.g., Linux volumes. By convention the
> EFI partition will usually be placed first, so that we would find the
> EFI removable media binary and enter it without device tree from disk.
> 
> Therefore after finding an EFI removable media binary, probe other
> partitions on the same device for .dtb files.
> 
> In the default case the behavior will be unchanged in that only the
> first partition is searched.
> In the case that there are two partitions marked bootable and the first
> being EFI, we will first check the first partition, then the second
> partition for the .dtb.
> Duplicate loading will only result from non-first EFI partition order.
> 
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Andreas F?rber <afaerber@suse.de>
> ---
>  include/config_distro_bootcmd.h | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
> index 087f576..eadec2e 100644
> --- a/include/config_distro_bootcmd.h
> +++ b/include/config_distro_bootcmd.h
> @@ -124,8 +124,7 @@
>  		"fi\0"                                                    \
>  	\
>  	"load_efi_dtb="                                                   \
> -		"load ${devtype} ${devnum}:${distro_bootpart} "           \
> -			"${fdt_addr_r} "                                  \
> +		"load ${devtype} ${devnum}:${efi_bootpart} ${fdt_addr_r} "\
>  			"${prefix}${dtb_prefix}${efi_fdtfile}\0"          \
>  	\
>  	"efi_dtb_prefixes=\"\" dtb/ dtb/current/\0"                       \
> @@ -133,7 +132,7 @@
>  		"for prefix in ${boot_prefixes}; do "                     \
>  			"for dtb_prefix in ${efi_dtb_prefixes}; do "      \
>  				"if test -e ${devtype} "                  \
> -					"${devnum}:${distro_bootpart} "   \
> +					"${devnum}:${efi_bootpart} "      \
>  					"${prefix}${dtb_prefix}"          \
>  					"${efi_fdtfile}; then "           \
>  					"run load_efi_dtb; "              \
> @@ -143,13 +142,22 @@
>  	"scan_dev_for_efi="                                               \
>  		"setenv efi_fdtfile ${fdtfile}; "                         \
>  		BOOTENV_EFI_SET_FDTFILE_FALLBACK                          \
> -		"run scan_dev_for_efi_fdt; "                              \
>  		"if test -e ${devtype} ${devnum}:${distro_bootpart} "     \
>  					"efi/boot/"BOOTEFI_NAME"; then "  \
>  				"echo Found EFI removable media binary "  \
>  					"efi/boot/"BOOTEFI_NAME"; "       \
> +				"for efi_bootpart in ${devplist}; do "    \
> +					"echo Scanning for device tree "  \
> +						"on ${devtype} ${devnum}" \
> +						":${efi_bootpart}...; "   \

Printing can be quite slow on certain devices (think LCDs with uncached
vram access), but have a *lot* of partitions (think Android partition
scheme). I don't think we should print out every partition we're
searching in, only the ones we find something at.

Maybe something like

  Found EFI removable media binary in efi/boot/bootaa64.efi.
  Searching for dtb on mmc 0 ...
  Found dtb on mmc 0:1 boot/dtb/foobar.dtb

would be a sensible amount of debug output :).

Also given that we're now running 3 cascaded for loops, it might be
about time to think about how to break out of the loops in case we
actually found something, so that the "normal" case is faster.


Alex

> +					"run scan_dev_for_efi_fdt; "      \
> +				"done; "                                  \
>  				"run boot_efi_binary; "                   \
>  				"echo EFI LOAD FAILED: continuing...; "   \
> +		"else "                                                   \
> +			"setenv efi_bootpart ${distro_bootpart}; "        \
> +			"run scan_dev_for_efi_fdt; "                      \
> +			"setenv efi_bootpart; "                           \
>  		"fi; "                                                    \
>  		"setenv efi_fdtfile\0"
>  #define SCAN_DEV_FOR_EFI "run scan_dev_for_efi;"
> 

[U-Boot] [PATCH 4/6] efi_loader: Improve .dtb search for arm64

[Alexander Graf]

On 12.07.16 06:21, Andreas F?rber wrote:
> On arm64 Linux device trees are organized by SoC vendor. Therefore we
> need to search the vendor subdirectory as well.
> 
> Since the SoC vendor may be different from ${vendor}, introduce a new
> ${soc_vendor}. If this is not set, the behavior remains unchanged.
> 
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Andreas F?rber <afaerber@suse.de>

Stephen had pretty strong opinions on the naming, mostly because "pxe
boot" uses the same naming scheme. So we should either change it there
as well to stay consistent or just make the implicit ruling always work :).

I guess the best case would be to fix up the path names of boards so
that $vendor is always $soc_vendor. Do you have an example where that
wouldn't work?


Alex

> ---
>  include/config_distro_bootcmd.h | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
> index eadec2e..8f14457 100644
> --- a/include/config_distro_bootcmd.h
> +++ b/include/config_distro_bootcmd.h
> @@ -113,6 +113,15 @@
>  #define BOOTENV_EFI_SET_FDTFILE_FALLBACK
>  #endif
>  
> +#if defined(CONFIG_ARM64)
> +#define BOOTENV_EFI_SET_FDTFILE_VENDOR                                    \
> +	"if test -n \"${soc_vendor}\"; then "                             \
> +		"setenv efi_dtb_vendor_prefix ${soc_vendor}/; "           \
> +	"fi; "
> +#else
> +#define BOOTENV_EFI_SET_FDTFILE_VENDOR
> +#endif
> +
>  #define BOOTENV_SHARED_EFI                                                \
>  	"boot_efi_binary="                                                \
>  		"load ${devtype} ${devnum}:${distro_bootpart} "           \
> @@ -125,18 +134,25 @@
>  	\
>  	"load_efi_dtb="                                                   \
>  		"load ${devtype} ${devnum}:${efi_bootpart} ${fdt_addr_r} "\
> -			"${prefix}${dtb_prefix}${efi_fdtfile}\0"          \
> +			"${prefix}${dtb_prefix}${dtb_vendor_prefix}"      \
> +			"${efi_fdtfile}\0"                                \
>  	\
>  	"efi_dtb_prefixes=\"\" dtb/ dtb/current/\0"                       \
>  	"scan_dev_for_efi_fdt="                                           \
>  		"for prefix in ${boot_prefixes}; do "                     \
>  			"for dtb_prefix in ${efi_dtb_prefixes}; do "      \
> +			    BOOTENV_EFI_SET_FDTFILE_VENDOR                \
> +			    "for dtb_vendor_prefix in \"\" "              \
> +					"${efi_dtb_vendor_prefix}; do "   \
>  				"if test -e ${devtype} "                  \
>  					"${devnum}:${efi_bootpart} "      \
>  					"${prefix}${dtb_prefix}"          \
> +					"${dtb_vendor_prefix}"            \
>  					"${efi_fdtfile}; then "           \
>  					"run load_efi_dtb; "              \
>  				"fi; "                                    \
> +			    "done; "                                      \
> +			    "setenv efi_dtb_vendor_prefix; "              \
>  			"done; "                                          \
>  		"done\0"                                                  \
>  	"scan_dev_for_efi="                                               \
> 

[U-Boot] [PATCH 6/6] efi_loader: Display which .dtb we found

[Alexander Graf]

On 12.07.16 06:21, Andreas F?rber wrote:
> We do so for the EFI binary already and it aids debugging.
> 
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Andreas F?rber <afaerber@suse.de>
> ---
>  include/config_distro_bootcmd.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
> index 8f14457..0cf74e2 100644
> --- a/include/config_distro_bootcmd.h
> +++ b/include/config_distro_bootcmd.h
> @@ -149,6 +149,10 @@
>  					"${prefix}${dtb_prefix}"          \
>  					"${dtb_vendor_prefix}"            \
>  					"${efi_fdtfile}; then "           \
> +					"echo Found ${prefix}"            \

As mentioned in the other reply, I think this message is very useful,
but should contain the target device name as well, so that we don't need
to print out a message for every single partition we scan :).


Alex

[U-Boot] [PATCH 3/6] efi_loader: Search .dtb on non-EFI partitions

[Andreas Färber]

Am 12.07.2016 um 09:25 schrieb Alexander Graf:
> 
> 
> On 12.07.16 06:21, Andreas F?rber wrote:
>> A UEFI setup will typically have an EFI FAT partition, but device trees
>> are more likely to be located on, e.g., Linux volumes. By convention the
>> EFI partition will usually be placed first, so that we would find the
>> EFI removable media binary and enter it without device tree from disk.
>>
>> Therefore after finding an EFI removable media binary, probe other
>> partitions on the same device for .dtb files.
>>
>> In the default case the behavior will be unchanged in that only the
>> first partition is searched.
>> In the case that there are two partitions marked bootable and the first
>> being EFI, we will first check the first partition, then the second
>> partition for the .dtb.
>> Duplicate loading will only result from non-first EFI partition order.
>>
>> Cc: Alexander Graf <agraf@suse.de>
>> Signed-off-by: Andreas F?rber <afaerber@suse.de>
>> ---
>>  include/config_distro_bootcmd.h | 16 ++++++++++++----
>>  1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
>> index 087f576..eadec2e 100644
>> --- a/include/config_distro_bootcmd.h
>> +++ b/include/config_distro_bootcmd.h
>> @@ -124,8 +124,7 @@
>>  		"fi\0"                                                    \
>>  	\
>>  	"load_efi_dtb="                                                   \
>> -		"load ${devtype} ${devnum}:${distro_bootpart} "           \
>> -			"${fdt_addr_r} "                                  \
>> +		"load ${devtype} ${devnum}:${efi_bootpart} ${fdt_addr_r} "\
>>  			"${prefix}${dtb_prefix}${efi_fdtfile}\0"          \
>>  	\
>>  	"efi_dtb_prefixes=\"\" dtb/ dtb/current/\0"                       \
>> @@ -133,7 +132,7 @@
>>  		"for prefix in ${boot_prefixes}; do "                     \
>>  			"for dtb_prefix in ${efi_dtb_prefixes}; do "      \
>>  				"if test -e ${devtype} "                  \
>> -					"${devnum}:${distro_bootpart} "   \
>> +					"${devnum}:${efi_bootpart} "      \
>>  					"${prefix}${dtb_prefix}"          \
>>  					"${efi_fdtfile}; then "           \
>>  					"run load_efi_dtb; "              \
>> @@ -143,13 +142,22 @@
>>  	"scan_dev_for_efi="                                               \
>>  		"setenv efi_fdtfile ${fdtfile}; "                         \
>>  		BOOTENV_EFI_SET_FDTFILE_FALLBACK                          \
>> -		"run scan_dev_for_efi_fdt; "                              \
>>  		"if test -e ${devtype} ${devnum}:${distro_bootpart} "     \
>>  					"efi/boot/"BOOTEFI_NAME"; then "  \
>>  				"echo Found EFI removable media binary "  \
>>  					"efi/boot/"BOOTEFI_NAME"; "       \
>> +				"for efi_bootpart in ${devplist}; do "    \
>> +					"echo Scanning for device tree "  \
>> +						"on ${devtype} ${devnum}" \
>> +						":${efi_bootpart}...; "   \
> 
> Printing can be quite slow on certain devices (think LCDs with uncached
> vram access), but have a *lot* of partitions (think Android partition
> scheme). I don't think we should print out every partition we're
> searching in, only the ones we find something at.
> 
> Maybe something like
> 
>   Found EFI removable media binary in efi/boot/bootaa64.efi.
>   Searching for dtb on mmc 0 ...
>   Found dtb on mmc 0:1 boot/dtb/foobar.dtb
> 
> would be a sensible amount of debug output :).

No, please read again! I am not printing every partition we're
searching, only in the new inner loop, which will likely be exactly
twice, once for FAT, once for ext.

For the "else" below we already get that output from the regular distro
framework.

In particular "mmc 0" is not enough and absolutely not helping. I need
to see that we're searching the right partition if we don't find a .dtb,
i.e. mmc 0:2.

> Also given that we're now running 3 cascaded for loops, it might be
> about time to think about how to break out of the loops in case we
> actually found something, so that the "normal" case is faster.

True point, I noticed that as well. :)

Note that I thought about adding the vendor to $efi_dtb_prefixes but
couldn't think of a way other than looping at runtime.

Regards,
Andreas

> 
> 
> Alex
> 
>> +					"run scan_dev_for_efi_fdt; "      \
>> +				"done; "                                  \
>>  				"run boot_efi_binary; "                   \
>>  				"echo EFI LOAD FAILED: continuing...; "   \
>> +		"else "                                                   \
>> +			"setenv efi_bootpart ${distro_bootpart}; "        \
>> +			"run scan_dev_for_efi_fdt; "                      \
>> +			"setenv efi_bootpart; "                           \
>>  		"fi; "                                                    \
>>  		"setenv efi_fdtfile\0"
>>  #define SCAN_DEV_FOR_EFI "run scan_dev_for_efi;"
>>


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

[U-Boot] [PATCH 4/6] efi_loader: Improve .dtb search for arm64

[Andreas Färber]

Am 12.07.2016 um 09:29 schrieb Alexander Graf:
> On 12.07.16 06:21, Andreas F?rber wrote:
>> On arm64 Linux device trees are organized by SoC vendor. Therefore we
>> need to search the vendor subdirectory as well.
>>
>> Since the SoC vendor may be different from ${vendor}, introduce a new
>> ${soc_vendor}. If this is not set, the behavior remains unchanged.
>>
>> Cc: Alexander Graf <agraf@suse.de>
>> Signed-off-by: Andreas F?rber <afaerber@suse.de>
> 
> Stephen had pretty strong opinions on the naming, mostly because "pxe
> boot" uses the same naming scheme. So we should either change it there
> as well to stay consistent or just make the implicit ruling always work :).
> 
> I guess the best case would be to fix up the path names of boards so
> that $vendor is always $soc_vendor. Do you have an example where that
> wouldn't work?

AFAIU for U-Boot $vendor is often the board vendor and matches the
board/ subdirectory, so there may be plenty vendors.  I didn't see any
code setting this value, so I assume this comes from .config options.

My $soc_vendor is the SoC vendor instead and only for the environment.

And because there were opinions on how to form the 32-bit efi_fdtfile,
both Tom and Stephen were CC'ed on the cover letter. :)

On the dragonboard410c:
CONFIG_SYS_SOC="snapdragon"
CONFIG_SYS_VENDOR="qualcomm"
CONFIG_SYS_BOARD="dragonboard410c"
CONFIG_SYS_CONFIG_NAME="dragonboard410c"
soc=snapdragon
vendor=qualcomm
board=dragonboard410c
board_name=dragonboard410c
=> Therefore my previous patch setting fdtfile to apq8016-sbc.dtb,
because it is absolutely different. This patch allows searching qcom/
for our dtb-qcom aarch64 openSUSE package.

On the odroid-c2:
CONFIG_SYS_SOC="meson"
CONFIG_SYS_VENDOR="amlogic"
CONFIG_SYS_BOARD="odroid-c2"
CONFIG_SYS_CONFIG_NAME="odroid-c2"
board=odroid-c2
board_name=odroid-c2
soc=meson
vendor=amlogic
=> Here $vendor would match Linux' dts subdirectory, but $soc and $board
are wrong, I need fdtfile=meson-gxbb-odroidc2.dtb.

But maybe I'm just not understanding what all those configs are good for
and how deeply they're tied to board/ subdirectories and mach-foo? If we
can clean them up to match Linux then all the better. If that were
intended, I wonder why no one flagged that during review.

Anyway, maybe add an else clause to this patch and reuse $vendor if
$soc_vendor is unavailable?

Regards,
Andreas

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

[PATCH 4/6] efi_loader: Improve .dtb search for arm64

[Andreas Färber]

Am 12.07.2016 um 09:29 schrieb Alexander Graf:
> On 12.07.16 06:21, Andreas F?rber wrote:
>> On arm64 Linux device trees are organized by SoC vendor. Therefore we
>> need to search the vendor subdirectory as well.
>>
>> Since the SoC vendor may be different from ${vendor}, introduce a new
>> ${soc_vendor}. If this is not set, the behavior remains unchanged.
>>
>> Cc: Alexander Graf <agraf@suse.de>
>> Signed-off-by: Andreas F?rber <afaerber@suse.de>
> 
> Stephen had pretty strong opinions on the naming, mostly because "pxe
> boot" uses the same naming scheme. So we should either change it there
> as well to stay consistent or just make the implicit ruling always work :).
> 
> I guess the best case would be to fix up the path names of boards so
> that $vendor is always $soc_vendor. Do you have an example where that
> wouldn't work?

AFAIU for U-Boot $vendor is often the board vendor and matches the
board/ subdirectory, so there may be plenty vendors.  I didn't see any
code setting this value, so I assume this comes from .config options.

My $soc_vendor is the SoC vendor instead and only for the environment.

And because there were opinions on how to form the 32-bit efi_fdtfile,
both Tom and Stephen were CC'ed on the cover letter. :)

On the dragonboard410c:
CONFIG_SYS_SOC="snapdragon"
CONFIG_SYS_VENDOR="qualcomm"
CONFIG_SYS_BOARD="dragonboard410c"
CONFIG_SYS_CONFIG_NAME="dragonboard410c"
soc=snapdragon
vendor=qualcomm
board=dragonboard410c
board_name=dragonboard410c
=> Therefore my previous patch setting fdtfile to apq8016-sbc.dtb,
because it is absolutely different. This patch allows searching qcom/
for our dtb-qcom aarch64 openSUSE package.

On the odroid-c2:
CONFIG_SYS_SOC="meson"
CONFIG_SYS_VENDOR="amlogic"
CONFIG_SYS_BOARD="odroid-c2"
CONFIG_SYS_CONFIG_NAME="odroid-c2"
board=odroid-c2
board_name=odroid-c2
soc=meson
vendor=amlogic
=> Here $vendor would match Linux' dts subdirectory, but $soc and $board
are wrong, I need fdtfile=meson-gxbb-odroidc2.dtb.

But maybe I'm just not understanding what all those configs are good for
and how deeply they're tied to board/ subdirectories and mach-foo? If we
can clean them up to match Linux then all the better. If that were
intended, I wonder why no one flagged that during review.

Anyway, maybe add an else clause to this patch and reuse $vendor if
$soc_vendor is unavailable?

Regards,
Andreas

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

[U-Boot] [PATCH 6/6] efi_loader: Display which .dtb we found

[Andreas Färber]

Am 12.07.2016 um 09:30 schrieb Alexander Graf:
> On 12.07.16 06:21, Andreas F?rber wrote:
>> We do so for the EFI binary already and it aids debugging.
>>
>> Cc: Alexander Graf <agraf@suse.de>
>> Signed-off-by: Andreas F?rber <afaerber@suse.de>
>> ---
>>  include/config_distro_bootcmd.h | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
>> index 8f14457..0cf74e2 100644
>> --- a/include/config_distro_bootcmd.h
>> +++ b/include/config_distro_bootcmd.h
>> @@ -149,6 +149,10 @@
>>  					"${prefix}${dtb_prefix}"          \
>>  					"${dtb_vendor_prefix}"            \
>>  					"${efi_fdtfile}; then "           \
>> +					"echo Found ${prefix}"            \
> 
> As mentioned in the other reply, I think this message is very useful,
> but should contain the target device name as well, so that we don't need
> to print out a message for every single partition we scan :).

I originally had it that way, but remember that issues like with patches
4 and 5 will not lead to any Found message at all. Therefore I still
prefer decoupling the two.

Btw where does the "reading efi/boot/bootaa64.efi" message come from?
That one could be dropped as duplicate instead! :)

Regards,
Andreas

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

[U-Boot] [PATCH 4/6] efi_loader: Improve .dtb search for arm64

[Andreas Färber]

Am 12.07.2016 um 14:38 schrieb Andreas F?rber:
> Am 12.07.2016 um 09:29 schrieb Alexander Graf:
>> On 12.07.16 06:21, Andreas F?rber wrote:
>>> On arm64 Linux device trees are organized by SoC vendor. Therefore we
>>> need to search the vendor subdirectory as well.
>>>
>>> Since the SoC vendor may be different from ${vendor}, introduce a new
>>> ${soc_vendor}. If this is not set, the behavior remains unchanged.
>>>
>>> Cc: Alexander Graf <agraf@suse.de>
>>> Signed-off-by: Andreas F?rber <afaerber@suse.de>
>>
>> Stephen had pretty strong opinions on the naming, mostly because "pxe
>> boot" uses the same naming scheme. So we should either change it there
>> as well to stay consistent or just make the implicit ruling always work :).
>>
>> I guess the best case would be to fix up the path names of boards so
>> that $vendor is always $soc_vendor. Do you have an example where that
>> wouldn't work?
> 
> AFAIU for U-Boot $vendor is often the board vendor and matches the
> board/ subdirectory, so there may be plenty vendors.  I didn't see any
> code setting this value, so I assume this comes from .config options.
> 
> My $soc_vendor is the SoC vendor instead and only for the environment.
> 
> And because there were opinions on how to form the 32-bit efi_fdtfile,
> both Tom and Stephen were CC'ed on the cover letter. :)
> 
> On the dragonboard410c:
> CONFIG_SYS_SOC="snapdragon"
> CONFIG_SYS_VENDOR="qualcomm"
> CONFIG_SYS_BOARD="dragonboard410c"
> CONFIG_SYS_CONFIG_NAME="dragonboard410c"
> soc=snapdragon
> vendor=qualcomm
> board=dragonboard410c
> board_name=dragonboard410c
> => Therefore my previous patch setting fdtfile to apq8016-sbc.dtb,
> because it is absolutely different. This patch allows searching qcom/
> for our dtb-qcom aarch64 openSUSE package.
> 
> On the odroid-c2:
> CONFIG_SYS_SOC="meson"
> CONFIG_SYS_VENDOR="amlogic"
> CONFIG_SYS_BOARD="odroid-c2"
> CONFIG_SYS_CONFIG_NAME="odroid-c2"
> board=odroid-c2
> board_name=odroid-c2
> soc=meson
> vendor=amlogic
> => Here $vendor would match Linux' dts subdirectory, but $soc and $board
> are wrong, I need fdtfile=meson-gxbb-odroidc2.dtb.

Adding a 32-bit example:

On the firefly-rk3288:
CONFIG_SYS_SOC="rockchip"
CONFIG_SYS_VENDOR="firefly"
CONFIG_SYS_BOARD="firefly-rk3288"
CONFIG_SYS_CONFIG_NAME="firefly-rk3288"
board=firefly-rk3288
board_name=firefly-rk3288
soc=rockchip
vendor=firefly
=> $vendor is not the SoC vendor, it's the board vendor. My point.
=> $soc is not rk3288, as needed for the ${soc}-${board}${boardrev}.dtb
formula. We either need to fix $soc or define $fdtfile.

Further, I have an early board that needs rk3288-firefly-beta.dtb
instead of rk3288-firefly.dtb. No hits for `git grep firefly-beta`, so
there doesn't seem to be any auto-detection...

Note that ~2016.07 seemed to be completely busted on this board, see the
multiple discussions about disabling EFI_LOADER and other random options
as workaround. Not even bootz succeeded for me any more.

Having a default way to load some uboot.env or uEnv.txt file might help
the user set $fdtfile and similar options while still using the default
boot mechanism.

> 
> But maybe I'm just not understanding what all those configs are good for
> and how deeply they're tied to board/ subdirectories and mach-foo? If we
> can clean them up to match Linux then all the better. If that were
> intended, I wonder why no one flagged that during review.
> 
> Anyway, maybe add an else clause to this patch and reuse $vendor if
> $soc_vendor is unavailable?
> 
> Regards,
> Andreas

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

[U-Boot] [PATCH 4/6] efi_loader: Improve .dtb search for arm64

[Andreas Färber]

Am 12.07.2016 um 14:38 schrieb Andreas F?rber:
> Am 12.07.2016 um 09:29 schrieb Alexander Graf:
>> On 12.07.16 06:21, Andreas F?rber wrote:
>>> On arm64 Linux device trees are organized by SoC vendor. Therefore we
>>> need to search the vendor subdirectory as well.
>>>
>>> Since the SoC vendor may be different from ${vendor}, introduce a new
>>> ${soc_vendor}. If this is not set, the behavior remains unchanged.
>>>
>>> Cc: Alexander Graf <agraf@suse.de>
>>> Signed-off-by: Andreas F?rber <afaerber@suse.de>
>>
>> Stephen had pretty strong opinions on the naming, mostly because "pxe
>> boot" uses the same naming scheme. So we should either change it there
>> as well to stay consistent or just make the implicit ruling always work :).
>>
>> I guess the best case would be to fix up the path names of boards so
>> that $vendor is always $soc_vendor. Do you have an example where that
>> wouldn't work?
> 
> AFAIU for U-Boot $vendor is often the board vendor and matches the
> board/ subdirectory, so there may be plenty vendors.  I didn't see any
> code setting this value, so I assume this comes from .config options.
> 
> My $soc_vendor is the SoC vendor instead and only for the environment.
> 
> And because there were opinions on how to form the 32-bit efi_fdtfile,
> both Tom and Stephen were CC'ed on the cover letter. :)
> 
> On the dragonboard410c:
> CONFIG_SYS_SOC="snapdragon"
> CONFIG_SYS_VENDOR="qualcomm"
> CONFIG_SYS_BOARD="dragonboard410c"
> CONFIG_SYS_CONFIG_NAME="dragonboard410c"
> soc=snapdragon
> vendor=qualcomm
> board=dragonboard410c
> board_name=dragonboard410c
> => Therefore my previous patch setting fdtfile to apq8016-sbc.dtb,
> because it is absolutely different. This patch allows searching qcom/
> for our dtb-qcom aarch64 openSUSE package.
> 
> On the odroid-c2:
> CONFIG_SYS_SOC="meson"
> CONFIG_SYS_VENDOR="amlogic"
> CONFIG_SYS_BOARD="odroid-c2"
> CONFIG_SYS_CONFIG_NAME="odroid-c2"
> board=odroid-c2
> board_name=odroid-c2
> soc=meson
> vendor=amlogic
> => Here $vendor would match Linux' dts subdirectory, but $soc and $board
> are wrong, I need fdtfile=meson-gxbb-odroidc2.dtb.

Adding a 32-bit example:

On the firefly-rk3288:
CONFIG_SYS_SOC="rockchip"
CONFIG_SYS_VENDOR="firefly"
CONFIG_SYS_BOARD="firefly-rk3288"
CONFIG_SYS_CONFIG_NAME="firefly-rk3288"
board=firefly-rk3288
board_name=firefly-rk3288
soc=rockchip
vendor=firefly
=> $vendor is not the SoC vendor, it's the board vendor. My point.
=> $soc is not rk3288, as needed for the ${soc}-${board}${boardrev}.dtb
formula. We either need to fix $soc or define $fdtfile.

Further, I have an early board that needs rk3288-firefly-beta.dtb
instead of rk3288-firefly.dtb. No hits for `git grep firefly-beta`, so
there doesn't seem to be any auto-detection...

Note that ~2016.07 seemed to be completely busted on this board, see the
multiple discussions about disabling EFI_LOADER and other random options
as workaround. Not even bootz succeeded for me any more.

Having a default way to load some uboot.env or uEnv.txt file might help
the user set $fdtfile and similar options while still using the default
boot mechanism.

> 
> But maybe I'm just not understanding what all those configs are good for
> and how deeply they're tied to board/ subdirectories and mach-foo? If we
> can clean them up to match Linux then all the better. If that were
> intended, I wonder why no one flagged that during review.
> 
> Anyway, maybe add an else clause to this patch and reuse $vendor if
> $soc_vendor is unavailable?
> 
> Regards,
> Andreas

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

[U-Boot] [PATCH 6/6] efi_loader: Display which .dtb we found

[Alexander Graf]

On 12.07.16 14:45, Andreas F?rber wrote:
> Am 12.07.2016 um 09:30 schrieb Alexander Graf:
>> On 12.07.16 06:21, Andreas F?rber wrote:
>>> We do so for the EFI binary already and it aids debugging.
>>>
>>> Cc: Alexander Graf <agraf@suse.de>
>>> Signed-off-by: Andreas F?rber <afaerber@suse.de>
>>> ---
>>>  include/config_distro_bootcmd.h | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
>>> index 8f14457..0cf74e2 100644
>>> --- a/include/config_distro_bootcmd.h
>>> +++ b/include/config_distro_bootcmd.h
>>> @@ -149,6 +149,10 @@
>>>  					"${prefix}${dtb_prefix}"          \
>>>  					"${dtb_vendor_prefix}"            \
>>>  					"${efi_fdtfile}; then "           \
>>> +					"echo Found ${prefix}"            \
>>
>> As mentioned in the other reply, I think this message is very useful,
>> but should contain the target device name as well, so that we don't need
>> to print out a message for every single partition we scan :).
> 
> I originally had it that way, but remember that issues like with patches
> 4 and 5 will not lead to any Found message at all. Therefore I still
> prefer decoupling the two.

We do detect the case where we don't have a working fdt loaded already,
so we could probably print out the search paths there if we don't find a
working fdt?

> Btw where does the "reading efi/boot/bootaa64.efi" message come from?
> That one could be dropped as duplicate instead! :)

Phew. The "load" command maybe?


Alex

[U-Boot] [PATCH 6/6] efi_loader: Display which .dtb we found

[Andreas Färber]

Am 12.07.2016 um 16:25 schrieb Alexander Graf:
> On 12.07.16 14:45, Andreas F?rber wrote:
>> Am 12.07.2016 um 09:30 schrieb Alexander Graf:
>>> On 12.07.16 06:21, Andreas F?rber wrote:
>>>> We do so for the EFI binary already and it aids debugging.
>>>>
>>>> Cc: Alexander Graf <agraf@suse.de>
>>>> Signed-off-by: Andreas F?rber <afaerber@suse.de>
>>>> ---
>>>>  include/config_distro_bootcmd.h | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
>>>> index 8f14457..0cf74e2 100644
>>>> --- a/include/config_distro_bootcmd.h
>>>> +++ b/include/config_distro_bootcmd.h
>>>> @@ -149,6 +149,10 @@
>>>>  					"${prefix}${dtb_prefix}"          \
>>>>  					"${dtb_vendor_prefix}"            \
>>>>  					"${efi_fdtfile}; then "           \
>>>> +					"echo Found ${prefix}"            \
>>>
>>> As mentioned in the other reply, I think this message is very useful,
>>> but should contain the target device name as well, so that we don't need
>>> to print out a message for every single partition we scan :).
>>
>> I originally had it that way, but remember that issues like with patches
>> 4 and 5 will not lead to any Found message at all. Therefore I still
>> prefer decoupling the two.
> 
> We do detect the case where we don't have a working fdt loaded already,
> so we could probably print out the search paths there if we don't find a
> working fdt?

Feel free to make a follow-up patch...

>> Btw where does the "reading efi/boot/bootaa64.efi" message come from?
>> That one could be dropped as duplicate instead! :)
> 
> Phew. The "load" command maybe?

Then why isn't it printing the same for the .dtb files? :)
Maybe it's some FAT debug thing?

Andreas

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

[U-Boot] [PATCH 4/6] efi_loader: Improve .dtb search for arm64

[Tom Rini]

On Tue, Jul 12, 2016 at 06:21:45AM +0200, Andreas F?rber wrote:

> On arm64 Linux device trees are organized by SoC vendor. Therefore we
> need to search the vendor subdirectory as well.
> 
> Since the SoC vendor may be different from ${vendor}, introduce a new
> ${soc_vendor}. If this is not set, the behavior remains unchanged.
> 
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Andreas F?rber <afaerber@suse.de>
> ---
>  include/config_distro_bootcmd.h | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
> index eadec2e..8f14457 100644
> --- a/include/config_distro_bootcmd.h
> +++ b/include/config_distro_bootcmd.h
> @@ -113,6 +113,15 @@
>  #define BOOTENV_EFI_SET_FDTFILE_FALLBACK
>  #endif
>  
> +#if defined(CONFIG_ARM64)
> +#define BOOTENV_EFI_SET_FDTFILE_VENDOR                                    \
> +	"if test -n \"${soc_vendor}\"; then "                             \
> +		"setenv efi_dtb_vendor_prefix ${soc_vendor}/; "           \
> +	"fi; "
> +#else

OK.  Looking at the current Linux kernel, it's a given that for arm64 a
DTB will be in a subdirectory, always.  Perhaps we should fix this in
Kconfig and have... CONFIG_FDTFILE_VENDOR_DIRECTORY and set this
correctly per vendor (and yes, eventually there will be some "fun" as
NXP boards will sometimes be in freescale/ and sometimes nxp/ so maybe
try and futureproof ourselves so that we loop over this variable) ?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160712/082777c4/attachment.sig>

[U-Boot] [PATCH 4/6] efi_loader: Improve .dtb search for arm64

[Andreas Färber]

Am 12.07.2016 um 16:50 schrieb Tom Rini:
> On Tue, Jul 12, 2016 at 06:21:45AM +0200, Andreas F?rber wrote:
> 
>> On arm64 Linux device trees are organized by SoC vendor. Therefore we
>> need to search the vendor subdirectory as well.
>>
>> Since the SoC vendor may be different from ${vendor}, introduce a new
>> ${soc_vendor}. If this is not set, the behavior remains unchanged.
>>
>> Cc: Alexander Graf <agraf@suse.de>
>> Signed-off-by: Andreas F?rber <afaerber@suse.de>
>> ---
>>  include/config_distro_bootcmd.h | 18 +++++++++++++++++-
>>  1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
>> index eadec2e..8f14457 100644
>> --- a/include/config_distro_bootcmd.h
>> +++ b/include/config_distro_bootcmd.h
>> @@ -113,6 +113,15 @@
>>  #define BOOTENV_EFI_SET_FDTFILE_FALLBACK
>>  #endif
>>  
>> +#if defined(CONFIG_ARM64)
>> +#define BOOTENV_EFI_SET_FDTFILE_VENDOR                                    \
>> +	"if test -n \"${soc_vendor}\"; then "                             \
>> +		"setenv efi_dtb_vendor_prefix ${soc_vendor}/; "           \
>> +	"fi; "
>> +#else
> 
> OK.  Looking at the current Linux kernel, it's a given that for arm64 a
> DTB will be in a subdirectory, always.  Perhaps we should fix this in
> Kconfig and have... CONFIG_FDTFILE_VENDOR_DIRECTORY and set this
> correctly per vendor (and yes, eventually there will be some "fun" as
> NXP boards will sometimes be in freescale/ and sometimes nxp/ so maybe
> try and futureproof ourselves so that we loop over this variable) ?

Yes, it's a given that in the kernel the .dts is always in a vendor
subdirectory - as listed in an earlier thread also for mips etc.

And yes, it's also true that due to our scripted openSUSE packaging we
therefore install .dtb to /boot/dtb-foo/vendor/ - but that's not
guaranteed for other distros and users, is it? Therefore I include "" in
the loop further down.

Anyway, adding a config option sounds good. Will you look into that
yourself or should I do that as part of this series?

I had considered the ambiguous vendor case and am iterating over
$efi_dtb_vendor_prefix in a list of vendors already below, only the
addition of the trailing slash above is limiting us to one currently. Do
you have a concrete suggestion of how to make that more flexible?

Cheers,
Andreas

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160712/f168de3c/attachment.sig>

[U-Boot] [PATCH 4/6] efi_loader: Improve .dtb search for arm64

[Stephen Warren]

On 07/12/2016 01:29 AM, Alexander Graf wrote:
>
>
> On 12.07.16 06:21, Andreas F?rber wrote:
>> On arm64 Linux device trees are organized by SoC vendor. Therefore we
>> need to search the vendor subdirectory as well.
>>
>> Since the SoC vendor may be different from ${vendor}, introduce a new
>> ${soc_vendor}. If this is not set, the behavior remains unchanged.
>>
>> Cc: Alexander Graf <agraf@suse.de>
>> Signed-off-by: Andreas F?rber <afaerber@suse.de>
>
> Stephen had pretty strong opinions on the naming, mostly because "pxe
> boot" uses the same naming scheme. So we should either change it there
> as well to stay consistent or just make the implicit ruling always work :).
>
> I guess the best case would be to fix up the path names of boards so
> that $vendor is always $soc_vendor. Do you have an example where that
> wouldn't work?

All I'll say is that if the "auto calculation" algorithm of 
${soc}-${board}.dtb doesn't work, U-Boot should set ${fdtfile} with the 
correct full name. Introducing other more complex "auto calculation" is 
just going to lead to more complexity and yet still not solve 100% of 
the cases, which will be confusing.

I'm unlikely to review any other aspects of the series, since I'm still 
quite disappointed that distros wouldn't engage in the discussions boot 
methods and agree on one solution, and that config_distro*.h is being 
abused as a dumping ground for tons of different boot methods; the whole 
point of it was to unify on *one* method that distros could rely upon, 
which is the opposite of where it's now going:-(

[U-Boot] [PATCH 4/6] efi_loader: Improve .dtb search for arm64

[Tom Rini]

On Tue, Jul 12, 2016 at 05:59:58PM +0200, Andreas F?rber wrote:
> Am 12.07.2016 um 16:50 schrieb Tom Rini:
> > On Tue, Jul 12, 2016 at 06:21:45AM +0200, Andreas F?rber wrote:
> > 
> >> On arm64 Linux device trees are organized by SoC vendor. Therefore we
> >> need to search the vendor subdirectory as well.
> >>
> >> Since the SoC vendor may be different from ${vendor}, introduce a new
> >> ${soc_vendor}. If this is not set, the behavior remains unchanged.
> >>
> >> Cc: Alexander Graf <agraf@suse.de>
> >> Signed-off-by: Andreas F?rber <afaerber@suse.de>
> >> ---
> >>  include/config_distro_bootcmd.h | 18 +++++++++++++++++-
> >>  1 file changed, 17 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
> >> index eadec2e..8f14457 100644
> >> --- a/include/config_distro_bootcmd.h
> >> +++ b/include/config_distro_bootcmd.h
> >> @@ -113,6 +113,15 @@
> >>  #define BOOTENV_EFI_SET_FDTFILE_FALLBACK
> >>  #endif
> >>  
> >> +#if defined(CONFIG_ARM64)
> >> +#define BOOTENV_EFI_SET_FDTFILE_VENDOR                                    \
> >> +	"if test -n \"${soc_vendor}\"; then "                             \
> >> +		"setenv efi_dtb_vendor_prefix ${soc_vendor}/; "           \
> >> +	"fi; "
> >> +#else
> > 
> > OK.  Looking at the current Linux kernel, it's a given that for arm64 a
> > DTB will be in a subdirectory, always.  Perhaps we should fix this in
> > Kconfig and have... CONFIG_FDTFILE_VENDOR_DIRECTORY and set this
> > correctly per vendor (and yes, eventually there will be some "fun" as
> > NXP boards will sometimes be in freescale/ and sometimes nxp/ so maybe
> > try and futureproof ourselves so that we loop over this variable) ?
> 
> Yes, it's a given that in the kernel the .dts is always in a vendor
> subdirectory - as listed in an earlier thread also for mips etc.

Right, OK.  And it will be extra fun later on when arm and powerpc
(probably) switch over.

> And yes, it's also true that due to our scripted openSUSE packaging we
> therefore install .dtb to /boot/dtb-foo/vendor/ - but that's not
> guaranteed for other distros and users, is it? Therefore I include "" in
> the loop further down.

It feels unlikely that other distributions would unroll the directories
that exist already upstream.

> Anyway, adding a config option sounds good. Will you look into that
> yourself or should I do that as part of this series?

Please re-work the series around this idea.

> I had considered the ambiguous vendor case and am iterating over
> $efi_dtb_vendor_prefix in a list of vendors already below, only the
> addition of the trailing slash above is limiting us to one currently. Do
> you have a concrete suggestion of how to make that more flexible?

The more I think about it, the less worried I am about it, actually.
Lets just assume that we will have the right vendor set and if it turns
out that a 'git mv' is done for say freescale later on down the line,
we'll worry about it then.  Unless someone knows about how it might go
and wants to speak up now?  York?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160712/a2dccad0/attachment.sig>