Hi Vagrant, On 20/12/17 22:32, Vagrant Cascadian wrote:
On 2017-12-12, Andre Heider wrote:I added the ability to concatenate multiple scripts/snippets for the final boot script. The new overlay handling snippet is supposed to be used with this. But the feature itself also allows nice cleanups, demonstrated on odroid-u3 and beaglebone (and there're quite some more cleanups possible).I very much like the idea of this; so many of the boot scripts copy-and-paste a lot of code between them, which makes maintenence more difficult as well as implementing features and changes across all the boot scripts...
Yeah, the copypasta is the first thing I noticed after git cloning. I tried to make the overlay support universal. So the less duplication there is, the easier it'll get to reuse it.
Unfortunately, I haven't had a chance to do more than a very cursory glance at your patches yet. Made some comments in-line on some of the individual patches below.
Thanks for the first glance! There's no need to hurry, especially with xmas around the corner.
To test this, you need: - u-boot with CONFIG_OF_LIBFDT_OVERLAY - base dtb with symbols (-@)Would the symbols (-@) be harmful to enable in Debian's kernels by default?
That's the big question. I looked around and found 3 cases where distros/downstreams enable symbols, see [1], [2] and [3]. But those 3 are in a different boat than debian: It's just per family of SoC.
I'm not sure if anything breaks if debian would enable it for its armhf multi platform build. I'm currently trying to find out with a solution appropriate for the upstream kernel [4], but I'm not sure if that pans out.
Is it feasible to use dtc to extract the .dts, rebuild it with -@, and then use that .dtb instead... or does it need more information from the original device tree(s)?
That's not going to work, without -@ the "__symbols__" node is missing. Without that an overlay can not reference e.g. the alias 'spi0'. You need the original dts to include these.
- your own overlays, again with symbols, in /boot/dtbs/overlaysAre there some very basic example overlays that would be feasible to just test that this feature is working? I don't have much experience with overlays, but have a beagleboneblack and a CHIP that in theory support this, and some devices I can attach that require overlays... Will try to test myself sometime in the coming weeks.
Since I used a boneblack too, you can find my basic overlays attached. I'm not a device tree expert, so they might not be correct, but they're good enough to test this and see the results (especially since commit [5]).
From: Andre Heider <a.hei...@gmail.com> Date: Tue, 12 Dec 2017 09:23:37 +0100 Subject: [PATCH 01/10] bootscr.uboot-generic: quote bootargs Signed-off-by: Andre Heider <a.hei...@gmail.com> --- bootscript/all/bootscr.uboot-generic | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bootscript/all/bootscr.uboot-generic b/bootscript/all/bootscr.uboot-generic index db4066a..bcf6e96 100644 --- a/bootscript/all/bootscr.uboot-generic +++ b/bootscript/all/bootscr.uboot-generic @@ -25,7 +25,7 @@ if test -n "${console}"; then setenv bootargs "${bootargs} console=${console}" fi-setenv bootargs @@LINUX_KERNEL_CMDLINE_DEFAULTS@@ ${bootargs} @@LINUX_KERNEL_CMDLINE@@+setenv bootargs "@@LINUX_KERNEL_CMDLINE_DEFAULTS@@ ${bootargs} @@LINUX_KERNEL_CMDLINE@@" @@UBOOT_ENV_EXTRA@@if test -z "${fk_kvers}"; thenWhy is this needed?
It's not, I did that just to be consistent (see 3 lines above). Patch can be dropped if you disagree.
From: Andre Heider <a.hei...@gmail.com> Date: Tue, 12 Dec 2017 09:25:26 +0100 Subject: [PATCH 05/10] bootscr.uboot-generic: support multiple prefixes to load from Allow custom boot scripts to set $fk_image_locations as a list of directories to load boot files from. If unset, $prefix will be the used - which is sufficient for all recent u-boot versions. This allows the clean up of various custom boot scripts. Code borrowed form the sunxi script. Signed-off-by: Andre Heider <a.hei...@gmail.com> --- bootscript/all/bootscr.uboot-generic | 38 +++++++++++++++++++++++++----------- 1 file changed, 27 insertions(+), 11 deletions(-) diff --git a/bootscript/all/bootscr.uboot-generic b/bootscript/all/bootscr.uboot-generic index bcf6e96..509efe7 100644 --- a/bootscript/all/bootscr.uboot-generic +++ b/bootscript/all/bootscr.uboot-generic @@ -48,14 +48,30 @@ else setenv partition ${distro_bootpart} fi-load ${devtype} ${devnum}:${partition} ${kernel_addr_r} ${prefix}vmlinuz-${fk_kvers} \-&& load ${devtype} ${devnum}:${partition} ${fdt_addr_r} ${prefix}${fdtpath} \ -&& load ${devtype} ${devnum}:${partition} ${ramdisk_addr_r} ${prefix}initrd.img-${fk_kvers} \ -&& echo "Booting Debian ${fk_kvers} from ${devtype} ${devnum}:${partition}..." \ -&& bootz ${kernel_addr_r} ${ramdisk_addr_r}:${filesize} ${fdt_addr_r} - -load ${devtype} ${devnum}:${partition} ${kernel_addr_r} ${prefix}vmlinuz \ -&& load ${devtype} ${devnum}:${partition} ${fdt_addr_r} ${prefix}dtb \ -&& load ${devtype} ${devnum}:${partition} ${ramdisk_addr_r} ${prefix}initrd.img \ -&& echo "Booting Debian from ${devtype} ${devnum}:${partition}..." \ -&& bootz ${kernel_addr_r} ${ramdisk_addr_r}:${filesize} ${fdt_addr_r} +if test -z "${fk_image_locations}"; then + setenv fk_image_locations ${prefix} +fi + +for pathprefix in ${fk_image_locations} +do + if test -e ${devtype} ${devnum}:${partition} ${pathprefix}vmlinuz-${fk_kvers} + then + load ${devtype} ${devnum}:${partition} ${kernel_addr_r} ${pathprefix}vmlinuz-${fk_kvers} \ + && load ${devtype} ${devnum}:${partition} ${fdt_addr_r} ${pathprefix}${fdtpath} \ + && load ${devtype} ${devnum}:${partition} ${ramdisk_addr_r} ${pathprefix}initrd.img-${fk_kvers} \ + && echo "Booting Debian ${fk_kvers} from ${devtype} ${devnum}:${partition}..." \ + && bootz ${kernel_addr_r} ${ramdisk_addr_r}:${filesize} ${fdt_addr_r} + fi +done + +for pathprefix in ${fk_image_locations} +do + if test -e ${devtype} ${devnum}:${partition} ${pathprefix}vmlinuz + then + load ${devtype} ${devnum}:${partition} ${kernel_addr_r} ${pathprefix}vmlinuz \ + && load ${devtype} ${devnum}:${partition} ${fdt_addr_r} ${pathprefix}dtb \ + && load ${devtype} ${devnum}:${partition} ${ramdisk_addr_r} ${pathprefix}initrd.img \ + && echo "Booting Debian from ${devtype} ${devnum}:${partition}..." \ + && bootz ${kernel_addr_r} ${ramdisk_addr_r}:${filesize} ${fdt_addr_r} + fi +doneI don't really understand the need for this. Prefix is set to where the boot script is loaded from, so I don't see what use fk_image_locations really provides. I guess there are some vendor or legacy u-boot versions that don't support distro_bootcmd and thus don't set prefix, but I'm not sure this is the best way to resolve that. I think it might be better to ship legacy boot scripts that people enable on a case-by-case basis when needed. The legacy u-boot versions most likely do not support overlays anyways.
The purpose of this patch was to allow more deduplication of boot scripts, e.g. bootscr.beaglebone and bootscr.sunxi. Both of these support overlays with a recent u-boot (well, sunxi soon [6]), but also contain this fallback loop for older and/or vendor-shipped versions.
With the reusability of the overlay snippets it's possible to shove all of that in another script and use that on those platforms. Would you prefer that?
From: Andre Heider <a.hei...@gmail.com> Date: Tue, 12 Dec 2017 20:08:21 +0100 Subject: [PATCH 08/10] Add a hook to bootscr.uboot-generic for post fdt loading tasks The optional hook $fk_fdt_cmd gets run after loading a ftd. Signed-off-by: Andre Heider <a.hei...@gmail.com> --- bootscript/all/bootscr.uboot-generic | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/bootscript/all/bootscr.uboot-generic b/bootscript/all/bootscr.uboot-generic index 509efe7..6d40779 100644 --- a/bootscript/all/bootscr.uboot-generic +++ b/bootscript/all/bootscr.uboot-generic @@ -52,13 +52,17 @@ if test -z "${fk_image_locations}"; then setenv fk_image_locations ${prefix} fi+if test -z "${fk_fdt_cmd}"; then+ setenv fk_fdt_cmd true +fi + for pathprefix in ${fk_image_locations} do if test -e ${devtype} ${devnum}:${partition} ${pathprefix}vmlinuz-${fk_kvers} then load ${devtype} ${devnum}:${partition} ${kernel_addr_r} ${pathprefix}vmlinuz-${fk_kvers} \ && load ${devtype} ${devnum}:${partition} ${fdt_addr_r} ${pathprefix}${fdtpath} \ - && load ${devtype} ${devnum}:${partition} ${ramdisk_addr_r} ${pathprefix}initrd.img-${fk_kvers} \ + && run fk_fdt_cmd && load ${devtype} ${devnum}:${partition} ${ramdisk_addr_r} ${pathprefix}initrd.img-${fk_kvers} \ && echo "Booting Debian ${fk_kvers} from ${devtype} ${devnum}:${partition}..." \ && bootz ${kernel_addr_r} ${ramdisk_addr_r}:${filesize} ${fdt_addr_r} fiI'd give fk_fdt_cmd it's own line here.
Oh I tried, but u-boot chokes on it (parser errors out with a syntax error at runtime). I don't know if it's not supported or if it's a bug, but I just couldn't make it work. I didn't yet report it upstream, but even if it's a bug and gets fixed we need a way this works everywhere anyway.
I'm not very experienced with u-boot scripts, so you're very much welcome to try yourself ;)
From: Andre Heider <a.hei...@gmail.com> Date: Tue, 12 Dec 2017 11:42:19 +0100 Subject: [PATCH 09/10] Introduce fdt overlay support Add a new 'fdt-overlays' script snippet to handle overlays. This script utilizes the new bootscr.uboot-generic hook to load all requested overlays. Per default this is the list specified with OVERLAYS in /etc/defaults/flash-kernel, but can be overwritten using $fk_overlays. Overlay support needs to be enabled specifically for each machine entry. If an overlay fails to load, the untouched dtb is restored - meaning that all overlays need to be valid (since they can depend on each other). Overlays need to be placed in /boot/dtbs/overlays. Signed-off-by: Andre Heider <a.hei...@gmail.com> --- bootscript/all/fdt-overlays | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 bootscript/all/fdt-overlays diff --git a/bootscript/all/fdt-overlays b/bootscript/all/fdt-overlays new file mode 100644 index 0000000..407d4be --- /dev/null +++ b/bootscript/all/fdt-overlays @@ -0,0 +1,12 @@ +if test -z "${fk_overlays}"; then + setenv fk_overlays "@@OVERLAYS@@" +fi + +if test -z "${ftdo_addr_r}"; then + setenv ftdo_addr_r ${ramdisk_addr_r} +fi + +setenv load_overlay_cmd 'echo loading overlay ${overlay}; load ${devtype} ${devnum}:${partition} ${ftdo_addr_r} ${pathprefix}dtbs/overlays/${overlay}.dtb && fdt apply ${ftdo_addr_r}' +setenv load_overlays_cmd 'fdt addr ${fdt_addr_r} && fdt resize 8192 && for overlay in ${fk_overlays}; do run load_overlay_cmd; done' +setenv restore_fdt_cmd 'echo "error loading overlays, using untouched dtb" && load ${devtype} ${devnum}:${partition} ${fdt_addr_r} ${pathprefix}${fdtpath}' +setenv fk_fdt_cmd 'if test -n "${fk_overlays}"; then run load_overlays_cmd || run restore_fdt_cmd; else true; fi'At a quick glance this looks reasonable to me.
Yeah, while this already works great, it's also the part I want to improve further. On error, this reloads the dtb file, so it relies on the var $fdtpath.
But u-boot's 'fdt' commands allows to copy the tree in memory, that would make it cleaner and more robust. I need to explore that though.
Regards, Andre[1] https://github.com/armbian/build/blob/master/patch/kernel/sunxi-next/add-overlay-compilation-support.patch#L98 [2] https://github.com/beagleboard/linux/commit/ed6b9450c2a2ec21149f14ff24770b69888abda6 [3] https://github.com/raspberrypi/linux/blob/rpi-4.15.y/arch/arm/boot/dts/Makefile#L1124 [4] http://lists.infradead.org/pipermail/linux-arm-kernel/2017-December/548366.html [5] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=956b200a846e324322f6211034c734c65a38e550
[6] https://lists.denx.de/pipermail/u-boot/2017-December/314474.html
overlay.tar.xz
Description: application/xz
