| Message ID | 1412942554-752-14-git-send-email-ian.campbell@citrix.com |
|---|---|
| State | New |
| Headers | show |
Ian Campbell writes ("[PATCH RFC OSSTEST 14/19] Osstest/Debian: Support for loading an FDT from u-boot script"):
> The currently supported platform provides an FDT preloaded at
> 0x1000. Replace this with ${fdt_addr} (which the current platform
> exposes) and for platforms which do not provide an fdt arrange to
> load the relevant file as named in the ${fdtfile} (which is
> conventionally provided by u-boot for platforms which need this).
You should mention the extra `echo's in the commit message.
You have two copies of the same dtb-loading code. (You had two copies
of the `scsi scan' and `mw.l' previously, but this is now worse
because it's bigger.)
I confess I don't really understand some of this. `Platforms which do
not provide an fdt' are ones where ${fdt_addr} is empty ? And on
those platforms `fdt addr \${fdt_addr}' is a no-op ?
You might find the code clearer (less toothpick-counting) if you used
<<'delim' for some of it, at one or other of the levels.
Thanks,
Ian.
On Fri, 2014-10-10 at 15:29 +0100, Ian Jackson wrote: > I confess I don't really understand some of this. `Platforms which do > not provide an fdt' are ones where ${fdt_addr} is empty ? Yes. Midway (marilith) provides a dtb in the firmware, and it is located at $fdt_addr. This is how device tree is supposed to work, but is actually vanishingly rare on ARM+u-boot in real life (I've never seen another such platform...) In practice most ARM+u-boot platforms require you to load the dtb from somewhere by hand, and provide fdt_addr_r as the place you should put it. (EFI on arm64 is a bit better here...) > And on > those platforms `fdt addr \${fdt_addr}' is a no-op ? fdt_addr is set by the new code on those platforms. > You might find the code clearer (less toothpick-counting) if you used > <<'delim' for some of it, at one or other of the levels. You mean as opposed to <<delim because it changes the quoting behaviour? That would break places where I wanted to expand a Perl var, I think? Perhaps those uses can be broken out in some convenient way and concatenated back again. I'll give it a go and see how it looks. Ian.
Ian Campbell writes ("Re: [PATCH RFC OSSTEST 14/19] Osstest/Debian: Support for loading an FDT from u-boot script"):
> [stuff]
Thanks for the explanation.
> > You might find the code clearer (less toothpick-counting) if you used
> > <<'delim' for some of it, at one or other of the levels.
>
> You mean as opposed to <<delim because it changes the quoting behaviour?
>
> That would break places where I wanted to expand a Perl var, I think?
> Perhaps those uses can be broken out in some convenient way and
> concatenated back again. I'll give it a go and see how it looks.
Right. In Perl you can write
blah <<END.<<'END';
for example.
Ian.
diff --git a/Osstest/Debian.pm b/Osstest/Debian.pm index 9171c72..19d6a22 100644 --- a/Osstest/Debian.pm +++ b/Osstest/Debian.pm @@ -141,9 +141,11 @@ cp -n /boot/boot.scr /boot/boot.scr.bak xen=`readlink /boot/$xen` cat >/boot/boot <<EOF - -mw.l 800000 0 10000 -scsi scan +if test -z "\\\${fdt_addr}" && test -n "\\\${fdtfile}" ; then + echo Loading dtbs/\\\${fdtfile} + ext2load scsi 0 \\\${fdt_addr_r} dtbs/\\\${fdtfile} + setenv fdt_addr \\\${fdt_addr_r} +fi fdt addr \\\${fdt_addr} fdt resize @@ -689,11 +691,17 @@ initrd=`readlink \$r/initrd.img | sed -e 's|boot/||'` cat >\$r/boot/boot <<EOF setenv bootargs $bootargs -mw.l 800000 0 10000 -scsi scan +if test -z "\\\${fdt_addr}" && test -n "\\\${fdtfile}" ; then + echo Loading dtbs/\\\${fdtfile} + ext2load scsi 0 \\\${fdt_addr_r} dtbs/\\\${fdtfile} + setenv fdt_addr \\\${fdt_addr_r} +fi +echo Loading \$kernel ext2load scsi 0 \\\${kernel_addr_r} \$kernel +echo Loading \$initrd ext2load scsi 0 \\\${ramdisk_addr_r} \$initrd -bootz \\\${kernel_addr_r} \\\${ramdisk_addr_r}:\\\${filesize} 0x1000 +echo Booting +bootz \\\${kernel_addr_r} \\\${ramdisk_addr_r}:\\\${filesize} \\\${fdt_addr} EOF in-target mkimage -A arm -T script -d /boot/boot /boot/boot.scr
The currently supported platform provides an FDT preloaded at 0x1000. Replace this with ${fdt_addr} (which the current platform exposes) and for platforms which do not provide an fdt arrange to load the relevant file as named in the ${fdtfile} (which is conventionally provided by u-boot for platforms which need this). Drop some random memory clearing rune, I've no idea what the intended purpose was, 0x800000 doesn't correspond to any $foo_addr_r on the midway systems for example. Also get rid of the scsi scan which must necessarily have already happened (since the boot.scr itselfs lives on a scsi drive). Signed-off-by: Ian Campbell <ian.campbell@citrix.com> --- Osstest/Debian.pm | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)