[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#884003: FDT overlay support

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/overlays


Are 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.heider@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.heider@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}"; then 

Why 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.heider@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.heider@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
+done


I 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.heider@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.heider@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}
    fi


I'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.heider@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.heider@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

Attachment: overlay.tar.xz
Description: application/xz

Reply to: