[U-Boot] [PATCH 0/6] efi_loader: Improvements to .dtb handling
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(-)
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 " \
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 " \
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;"
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" \@@ -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; " \
"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; " \#define SCAN_DEV_FOR_EFI "run scan_dev_for_efi;"
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" \@@ -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; " \
"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; " \#define SCAN_DEV_FOR_EFI "run scan_dev_for_efi;"
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=" \
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" \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; " \
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
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
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:-(
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) ?
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
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?
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
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; " \
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
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
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
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
participants (4)
-
Alexander Graf -
Andreas Färber -
Stephen Warren -
Tom Rini