[PATCH] sunxi-tools: [RFC] extend fel utility to handle SPL + U-Boot binary

[Bernhard Nortmann]

This patch introduces a new "fel uboot" command. Its purpose is to
handle not only the SPL part of a combined u-boot-sunxi-with-spl.bin
boot file, but additionally transfer the main u-boot binary (image)
contained within the second part of such files.

With recent (dtb-based) U-Boot versions, the idea of this modification
is to prevent users mixing up u-boot.bin and u-boot-dtb.bin (possibly
unconscious, e.g. by relying on outdated FEL scripts).
see e.g. http://lists.denx.de/pipermail/u-boot/2015-June/217476.html

Signed-off-by: Bernhard Nortmann <bernhard...@web.de>
---
fel.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 67 insertions(+), 1 deletion(-)

diff --git a/fel.c b/fel.c
index adf8f35..843be14 100644
--- a/fel.c
+++ b/fel.c
@@ -599,6 +599,11 @@ void aw_restore_and_enable_mmu(libusb_device_handle *usb, uint32_t *tt)
free(tt);
}

+/*
+ * Maximum size of SPL, at the same time this is the start offset
+ * of the main U-Boot image within u-boot-sunxi-with-spl.bin
+ */
+static const int SPL_LEN_LIMIT = 0x8000;

void aw_fel_write_and_execute_spl(libusb_device_handle *usb,
uint8_t *buf, size_t len)
@@ -608,7 +613,7 @@ void aw_fel_write_and_execute_spl(libusb_device_handle *usb,
char header_signature[9] = { 0 };
size_t i, thunk_size;
uint32_t *thunk_buf;
- uint32_t spl_checksum, spl_len, spl_len_limit = 0x8000;
+ uint32_t spl_checksum, spl_len, spl_len_limit = SPL_LEN_LIMIT;
uint32_t *buf32 = (uint32_t *)buf;
uint32_t written = 0;
uint32_t *tt = NULL;
@@ -722,6 +727,58 @@ void aw_fel_write_and_execute_spl(libusb_device_handle *usb,
aw_restore_and_enable_mmu(usb, tt);
}

+/* Constants taken from ${U-BOOT}/include/image.h */
+#define IH_MAGIC 0x27051956 /* Image Magic Number */
+#define IH_ARCH_ARM 2 /* ARM */
+#define IH_TYPE_FIRMWARE 5 /* Firmware Image */
+#define IH_NMLEN 32 /* Image Name Length */
+
+#define HEADER_NAME_OFFSET 32 /* offset of name field */
+#define HEADER_SIZE (HEADER_NAME_OFFSET + IH_NMLEN)
+
+void aw_fel_write_uboot_image(libusb_device_handle *usb,
+ uint8_t *buf, size_t len)
+{
+ uint32_t *buf32 = (uint32_t *)buf;
+ uint32_t load_addr, data_size;
+
+ /* Check for a valid mkimage header */
+ if (be32toh(buf32[0]) != IH_MAGIC) {
+ fprintf(stderr, "U-Boot image verification failure: "
+ "expected IH_MAGIC, got 0x%X\n", be32toh(buf32[0]));
+ exit(1);
+ }
+ if (buf[29] != IH_ARCH_ARM|| buf[30] != IH_TYPE_FIRMWARE) {
+ fprintf(stderr, "U-Boot image verification failure: "
+ "expected ARM firmware, got %02X %02X\n", buf[29], buf[30]);
+ exit(1);
+ }
+ data_size = be32toh(buf32[3]); /* Image Data Size */
+ load_addr = be32toh(buf32[4]); /* Data Load Address */
+ if (data_size != len - HEADER_SIZE) {
+ fprintf(stderr, "U-Boot image data size mismatch: "
+ "expected %u, got %u\n", len - HEADER_SIZE, data_size);
+ exit(1);
+ }
+ /* TODO: Verify image data integrity using the checksum field ih_dcrc,
+ * available from be32toh(buf32[6])
+ *
+ * However, this requires CRC routines that mimic their U-Boot
+ * counterparts, namely image_check_dcrc() in ${U-BOOT}/common/image.cabs
+ * and crc_wd() in ${U-BOOT}/lib/crc32.c
+ *
+ * It should be investigated if existing CRC routines in sunxi-tools
+ * could be factored out and reused for this purpose - e.g. calc_crc32()
+ * from nand-part-main.c
+ */
+
+ /* If we get here, we're "good to go" (i.e. actually write the data) */
+ pr_info("Writing image \"%.*s\", %u bytes @ 0x%X\n",
+ IH_NMLEN, buf + HEADER_NAME_OFFSET, data_size, load_addr);
+
+ aw_fel_write(usb, buf + HEADER_SIZE, load_addr, data_size);
+}
+
static int aw_fel_get_endpoint(libusb_device_handle *usb)
{
struct libusb_device *dev = libusb_get_device(usb);
@@ -790,6 +847,7 @@ int main(int argc, char **argv)
" clear address length Clear memory\n"
" fill address length value Fill memory\n"
" spl file Load and execute U-Boot SPL\n"
+ " uboot file-with-spl Load and execute SPL, and write U-Boot\n"
, argv[0]
);
}
@@ -874,6 +932,14 @@ int main(int argc, char **argv)
uint8_t *buf = load_file(argv[2], &size);
aw_fel_write_and_execute_spl(handle, buf, size);
skip=2;
+ } else if (strcmp(argv[1], "uboot") == 0 && argc > 2) {
+ /* Write (and execute) SPL, then write (main) U-Boot */
+ size_t size;
+ uint8_t *buf = load_file(argv[2], &size);
+ aw_fel_write_and_execute_spl(handle, buf, size);
+ usleep(500000); // give SPL execution some time (0.5s) to complete
+ aw_fel_write_uboot_image(handle, buf + SPL_LEN_LIMIT, size - SPL_LEN_LIMIT);
+ skip=2;
} else {
fprintf(stderr,"Invalid command %s\n", argv[1]);
exit(1);
--
2.0.5

[Hans de Goede]

Hi,

On 04-07-15 20:31, Bernhard Nortmann wrote:
> This patch introduces a new "fel uboot" command. Its purpose is to
> handle not only the SPL part of a combined u-boot-sunxi-with-spl.bin
> boot file, but additionally transfer the main u-boot binary (image)
> contained within the second part of such files.
>
> With recent (dtb-based) U-Boot versions, the idea of this modification
> is to prevent users mixing up u-boot.bin and u-boot-dtb.bin (possibly
> unconscious, e.g. by relying on outdated FEL scripts).
> see e.g. http://lists.denx.de/pipermail/u-boot/2015-June/217476.html
>
> Signed-off-by: Bernhard Nortmann <bernhard...@web.de>

This seems like a good idea to me, am I reading both the --help
and the code correctly that this only writes u-boot but does not
do the necessary fel exe 0x..... ? It seems to me that such a
command should also execute u-boot.

Other then that this looks good to me any reason this is an RFC?
I would be happy to merge this even with the TODO in place.

Siarhei, your input on this would be very welcome, if it is just an
ack :)

Regards,

Hans

[Bernhard Nortmann]

Hello Hans!

Am 05.07.2015 11:54, schrieb Hans de Goede:
>
> This seems like a good idea to me, am I reading both the --help
> and the code correctly that this only writes u-boot but does not
> do the necessary fel exe 0x..... ? It seems to me that such a
> command should also execute u-boot.
>

You're right - the "fel exe" is kept separate. This is done on purpose
to allow uploading other elements via FEL (e.g. boot script, kernel,
initrd). Execution of the bootloader always comes last.

> Other then that this looks good to me any reason this is an RFC?
> I would be happy to merge this even with the TODO in place.
>

I just wanted further input / some more opinions on it, as I'm
not completely sure it would be the best way to achieve this.
For example, I originally intended to extend the existing "spl"
command with a load address (to tell it to transfer the u-boot
part to that destination). But then I realized that the image
already contains a target address, and that "fel spl" would still
have a purpose of its own (when used with u-boot-spl.bin).

Would we want more flexibility by allowing an optional (dest)
address specification? I'd have gone straight for that, but
unfortunately argument parsing / handling in fel.c isn't too
flexible...

Nevertheless the patch is fully functional and has been tested to
work on a Banana Pi (sun7i).

Regards, B. Nortmann

[Hans de Goede]

Hi,

On 06-07-15 09:02, Bernhard Nortmann wrote:
> Hello Hans!
>
> Am 05.07.2015 11:54, schrieb Hans de Goede:
>>
>> This seems like a good idea to me, am I reading both the --help
>> and the code correctly that this only writes u-boot but does not
>> do the necessary fel exe 0x..... ? It seems to me that such a
>> command should also execute u-boot.
>>
> You're right - the "fel exe" is kept separate. This is done on purpose
> to allow uploading other elements via FEL (e.g. boot script, kernel,
> initrd). Execution of the bootloader always comes last.

Ah right, I guess that makes sense, but if this is not going to
be a fire and forget command, and people still need to run more
fel commands after it then I'm wondering it this is worth the trouble
at all.

What might be interesting is having a command where one can
specify a u-boot-sunxi-with-spl.bin + zImage + kernel-dtb + optional
ramdisk + "optional kernel cmdline" and then have the fel tool
boot all of that in one go, this will likely require some small
u-boot mods to execute a script from a pre-agreed upon location
when booting in FEL mode. Since the fel tool already patches the
eGON boot0 magic, we could use a different marker here to tell
u-boot that we are doing a full auto boot and that it should
execute the scr loaded to the pre agreed upon address. That scr
can then contain the kernel cmdline + load addresses, etc.
for the kernel, dtb and initrd.

>
>> Other then that this looks good to me any reason this is an RFC?
>> I would be happy to merge this even with the TODO in place.
>>
> I just wanted further input / some more opinions on it, as I'm
> not completely sure it would be the best way to achieve this.
> For example, I originally intended to extend the existing "spl"
> command with a load address (to tell it to transfer the u-boot
> part to that destination). But then I realized that the image
> already contains a target address, and that "fel spl" would still
> have a purpose of its own (when used with u-boot-spl.bin).
>
> Would we want more flexibility by allowing an optional (dest)
> address specification? I'd have gone straight for that, but
> unfortunately argument parsing / handling in fel.c isn't too
> flexible...

See above since the user still needs to do the exec I wonder
what the value is in saving the user the single extra
fel write for the u-boot-dtb.bin file. I think having a full
auto mode as described above would be better.

Regards,

Hans

[Ian Campbell]

On Mon, 2015-07-06 at 14:11 +0200, Hans de Goede wrote:
> Hi,
>
> On 06-07-15 09:02, Bernhard Nortmann wrote:
> > Hello Hans!
> >
> > Am 05.07.2015 11:54, schrieb Hans de Goede:
> >>
> >> This seems like a good idea to me, am I reading both the --help
> >> and the code correctly that this only writes u-boot but does not
> >> do the necessary fel exe 0x..... ? It seems to me that such a
> >> command should also execute u-boot.
> >>
> > You're right - the "fel exe" is kept separate. This is done on purpose
> > to allow uploading other elements via FEL (e.g. boot script, kernel,
> > initrd). Execution of the bootloader always comes last.
>
> Ah right, I guess that makes sense, but if this is not going to
> be a fire and forget command, and people still need to run more
> fel commands after it then I'm wondering it this is worth the trouble
> at all.

Perhaps do the exe by default but provide a way to ask not to? (Or vice
versa, but that seems less useful to me)

[...]

> See above since the user still needs to do the exec I wonder
> what the value is in saving the user the single extra
> fel write for the u-boot-dtb.bin file. I think having a full
> auto mode as described above would be better.

Historically wasn't this what the usb-boot script did (effectively a
wrapper around the fel tool). It's be nice if it either a) started
working again, b) said something when it wasn't supposed to work and
pointed in the right direction or c) went away so it stops being
confusingly tempting to people like me ;-)

a) was what I was thinking of when I wrote the last bit of
http://lists.denx.de/pipermail/u-boot/2015-June/217743.html (which I've
still not gotten around to)

Ian.

[Hans de Goede]

Hi,

Right, this is what the usb-boot script used to do, but the usb-boot
script uses a u-boot script (.scr) file to tell u-boot where to
laod the kernel, etc. This used to work because the FEL build had
a separate u-boot env, which had a boot_cmd env setting which would
execute said script form a fixed address.

However with mainlining the sunxi code the separate env got dropped,
and now we no longer even have a separate FEL build.

So as said I think part of fixing this will be a u-boot patch,
what I've in mind is a bit of code which gets called after the env
is loaded, but before (auto)booting. This can check the EGON0 magic
signature in the first 32k of SRAM. Currently "fel spl" already
mangles this signature, and this way u-boot knows it is not
booting from mmc/nand, but in FEL mode and that it should return
from the SPL to the FEL ROM, so that u-boot-dtb.bin can be loaded
to the DRAM.

We could agree upon a special magic signature between the fel tool
and the sunxi u-boot code which means that u-boot should patch
its boot_cmd in the env to load a script from DRAM (at a fixed
offset). We could always do the boot_cmd patching when in fel-mode
(which we already detect) but I do not believe this is desirable.

So instead what I suggest is a new "fel linux" command which
replaces the old usb-boot script, this command would generate
an scr file on the fly (including setting kernel options specified
on the fel cmdline), patch the boot-signature, upload spl + kernel +
dtb + initrd + generated-scr + u-boot-dtb.bin and then execute
u-boot, which will then autoboot if not interrupted, execute the
scr and boot into the loaded Linux.

Note parts of this could still be done by a shell script, but I
do not believe this will be very hard to do in C, and it would
be good to have this contained in one binary IMHO.

We could also add Bernard's u-boot command and that would then
just load spl + u-boot and execute u-boot.

Regards,

Hans

[Bernhard Nortmann]

I tend to agree with Ian here. My impression is that the fel utility
is meant to be the "one-trick pony", a low-level tool that handles
a single task (one command, out of several available) - but handles
that well / "right". Anything more sophisticated should probably be
done via scripting - that's what "usb-boot" is supposed to achieve.

I had fooled myself on this too (by missing the u-boot-dtb.bin
requirement and not adapting my FEL script) - and thought it
to be a problem in the SPL -> main U-Boot transition after the
switch to device model. Of course I was providing a 'broken'
u-boot that way (lacking the dtb data).

The "benefit" of the fel modification presented here is that the
utility would be able to gracefully handle _any_ u-boot-sunxi-with-spl.bin,
be it from 2015.04 ("pre-DTB"), or 2015.07+; without the user
(or script) having to pay attention to the correct filename for
the main binary.

Regards, B. Nortmann

[Hans de Goede]

Hi Bernhard,

Please see the mail I just send where I explain why the usb-boot script
currently cannot work. I really believe we need an u-boot patch to do
somthing akin to:

setenv boot_cmd "source 0x41000000"

When booting in FEL mode in certain cases, I believe that always doing
this is wrong, so we need some way for the fel tool to communicate to
u-boot that it should run a boot-script from ram. Then we can use
that to fix usb-boot, or simply fold the usb-boot functionality into
the fel tool.

I'm fine with keeping this functionality in a fixed usb-boot script,
but then I see little value in your "fel" patch instead a patch to
add a cmdline option to the "fel spl" command to set the magic which
u-boot will use to detect it should run a script from ram would be
better, and then everything else can be dealt with in the usb-boot
script.

Regards,

Hans

[Bernhard Nortmann]

Am 07.07.2015 11:19, schrieb Hans de Goede:
>
> We could agree upon a special magic signature between the fel tool
> and the sunxi u-boot code which means that u-boot should patch
> its boot_cmd in the env to load a script from DRAM (at a fixed
> offset). We could always do the boot_cmd patching when in fel-mode
> (which we already detect) but I do not believe this is desirable.
>

Until now I've use a simple (and somewhat crude) workaround for this,
i.e. to achieve a more flexible FEL boot sequence.

I defined a different bootcmd for u-boot to check for the presence of
a script file (uploaded via fel):
#define CONFIG_BOOTCOMMAND "source ${scriptaddr} || run distro_bootcmd"

A "normal" boot is expected to have invalid data at ${scriptaddr},
so the source command would fail (due to checksum mismatch).
But if a FEL-provided script is present, it will be executed before
anything else.

Regards, B. Nortmann

[Hans de Goede]

Hi,

I've been thinking about doing something similar, but can we perhaps
only do this when we know we are booting in FEL mode ?

So take the FEL or not FEL check from spl_boot_device(), and when setting
up the env, do

setenv boot_cmd "source ${scriptaddr} || run distro_bootcmd"

If we're booting from FEL. We should probably also do something wrt
where the env gets loaded from when booting in FEL mode, ensuring that
we always use the default env then.

I'm not sure if the env loading code already supports dynamically
figuring out where to load the env from, if it does not then we
should probably fix that as we will need the same when running
from mmc or nand with a single unified binary.

Regards,

Hans

[Ian Campbell]

Yes.

Personally I rarely use(d) it in that mode but just used it load SPL +
main u-boot and get me to a prompt, which is also broken today and is an
easies fix.

I, of course, have no in-principal objections to supporting the Linux
loading thing too, since it is clearly useful even if I don't often use
it myself.

> We could agree upon a special magic signature between the fel tool
> and the sunxi u-boot code which means that u-boot should patch
> its boot_cmd in the env to load a script from DRAM (at a fixed
> offset).

Having mangled the header could we also find/define a field for the DRAM
address?

Perhaps rather than patching the boot_cmd we could instead have this
early code set a flag (in the env) which the normal bootcmd could check
(first) as part of the normal series of attempts to boot from various
sources? That would let the user configure it via the env if they
wanted.

> We could always do the boot_cmd patching when in fel-mode
> (which we already detect) but I do not believe this is desirable.

Agreed.

Ian.

[Siarhei Siamashka]

On Sat, 4 Jul 2015 20:31:47 +0200
Bernhard Nortmann <bernhard...@web.de> wrote:

> This patch introduces a new "fel uboot" command. Its purpose is to
> handle not only the SPL part of a combined u-boot-sunxi-with-spl.bin
> boot file, but additionally transfer the main u-boot binary (image)
> contained within the second part of such files.
>
> With recent (dtb-based) U-Boot versions, the idea of this modification
> is to prevent users mixing up u-boot.bin and u-boot-dtb.bin (possibly
> unconscious, e.g. by relying on outdated FEL scripts).
> see e.g. http://lists.denx.de/pipermail/u-boot/2015-June/217476.html
>
> Signed-off-by: Bernhard Nortmann <bernhard...@web.de>

Thanks, it was a very good find about the u-boot load address, which
is already stored in the 'u-boot-sunxi-with-spl.bin' binary. This
indeed allows us to make the usage of the 'fel' tool more simple.

Here 'len' can be 0. For example, if somebody tries to use the
'sunxi-spl.bin' binary by mistake:

$ valgrind ./fel -v uboot sunxi-spl.bin
==3817== Memcheck, a memory error detector
==3817== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==3817== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright
info ==3817== Command: ./fel -v uboot sunxi-spl.bin
==3817==
Reading the MMU translation table from 0x00020000
Disabling I-cache, MMU and branch prediction... done.
=> Executing the SPL... done.
Setting write-combine mapping for DRAM.
Setting cached mapping for BROM.
Writing back the MMU translation table.
Enabling I-cache, MMU and branch prediction... done.
==3817== Invalid read of size 4
==3817== at 0x402A2F: aw_fel_write_uboot_image (fel.c:746)
==3817== by 0x4034DB: main (fel.c:941)
==3817== Address 0x588dbd0 is 0 bytes after a block of size 32,768
alloc'd ==3817== at 0x4C2B24E: realloc (vg_replace_malloc.c:692)
==3817== by 0x401B6F: load_file (fel.c:285)
==3817== by 0x40348A: main (fel.c:938)
==3817==

fel.c: In function ‘aw_fel_write_uboot_image’:
fel.c:760:4: warning: format ‘%u’ expects argument of type ‘unsigned
int’, but argument 3 has type ‘size_t’ [-Wformat=] "expected %u, got
%u\n", len - HEADER_SIZE, data_size); ^

> +
> + aw_fel_write(usb, buf + HEADER_SIZE, load_addr, data_size);
> +}
> +
> static int aw_fel_get_endpoint(libusb_device_handle *usb)
> {
> struct libusb_device *dev = libusb_get_device(usb);
> @@ -790,6 +847,7 @@ int main(int argc, char **argv)
> " clear address length Clear memory\n"
> " fill address length value Fill memory\n"
> " spl file Load and execute U-Boot SPL\n"
> + " uboot file-with-spl Load and execute SPL, and write U-Boot\n"
> , argv[0]
> );
> }
> @@ -874,6 +932,14 @@ int main(int argc, char **argv)
> uint8_t *buf = load_file(argv[2], &size);
> aw_fel_write_and_execute_spl(handle, buf, size);
> skip=2;
> + } else if (strcmp(argv[1], "uboot") == 0 && argc > 2) {
> + /* Write (and execute) SPL, then write (main) U-Boot */
> + size_t size;
> + uint8_t *buf = load_file(argv[2], &size);
> + aw_fel_write_and_execute_spl(handle, buf, size);
> + usleep(500000); // give SPL execution some time (0.5s) to complete

This delay is already handled in the 'aw_fel_write_and_execute_spl'
function. It is set to 0.25s there and seems to already provide a
sufficient safety headroom.

In general, interrupts are disabled while the SPL is running and this
already reduces the likelihood of encountering problems even if the
delay is removed. For example, if we run a dummy busyloop instead of
running the real SPL, then I was never able to reproduce any problems
and the attempts to issue FEL commands are just held until the busyloop
code returns and re-enables the interrupts. If we don't disable
interrupts, then this busyloop testcase also fails. However this is
still not enough when dealing with the real SPL. In the past I have
done some experiments trying to insert delays in various parts of
the SPL trying to identify the exact location where problems are
likely to happen. These experiments seemed to have pointed to the
re-clocking part of the SPL code as the most likely culprit. But I
did not have time to come with a better solution, so the delay
workaround is still in use.

Anyway, if the current 0.25s delay is not enough, then we can always
increase this delay later.

> + aw_fel_write_uboot_image(handle, buf + SPL_LEN_LIMIT, size - SPL_LEN_LIMIT);
> + skip=2;
> } else {
> fprintf(stderr,"Invalid command %s\n", argv[1]);
> exit(1);

Regarding the new "uboot" command. We can probably make it execute the
u-boot code too, but just postpone the execution (so that it is done
only after the command line parsing is fully finished). For additional
safety, the subsequent "write" and "clear" commands can get some extra
checks to bail out with an error if they try to overwrite the u-boot
code location.

Additionally, the existing "spl" command can be also modified to do
u-boot loading automatically but without executing it (the same
behaviour as the "uboot" command in your current patch). This does
not break compatibility with old scripts or usage instructions because
having u-boot automatically loaded to some memory location is not any
worse than having uninitialized garbage there.

--
Best regards,
Siarhei Siamashka

[Bernhard Nortmann]

Hello Siarhei!

Am 12.07.2015 13:49, schrieb Siarhei Siamashka:

> Regarding the new "uboot" command. We can probably make it execute the
> u-boot code too, but just postpone the execution (so that it is done
> only after the command line parsing is fully finished). For additional
> safety, the subsequent "write" and "clear" commands can get some extra
> checks to bail out with an error if they try to overwrite the u-boot
> code location.
>
> Additionally, the existing "spl" command can be also modified to do
> u-boot loading automatically but without executing it (the same
> behaviour as the "uboot" command in your current patch). This does
> not break compatibility with old scripts or usage instructions because
> having u-boot automatically loaded to some memory location is not any
> worse than having uninitialized garbage there.

A very good suggestion - I feel that actually is the "way to go", and it
would make
the separate "uboot" command obsolete. ("spl" would handle it nicely)

Regarding the 'auto-execution' of code: I think this is somewhat
independent of
the suggested extension to load the main U-Boot binary. It would probably be
useful if the fel utility was able to keep track of addresses it has
written to (this
includes the "write" command), so a later "fel exe" could refer - or
even default -
to them. Maybe it's possible to also introduce 'symbolic' names for specific
addresses? I'm thinking "fel exe uboot" here... (with "uboot" being set /
preserved from the aw_fel_write_uboot_image function)

> This delay is already handled in the 'aw_fel_write_and_execute_spl'
> function. It is set to 0.25s there and seems to already provide a
> sufficient safety headroom.

> [...]

> Anyway, if the current 0.25s delay is not enough, then we can always
> increase this delay later.

You're right. I came up with that as the "usb-boot" script introduces a
delay
(sleep) at that point, and I somehow seem to remember that a delay was
required in my first experiments. However, after retesting this, the
existing
SPL delay seems to be just fine, and I had no issues when removing the
additional wait again.

I'll now prepare a v2 patch, that also addresses the other style/logic
issues
you pointed out.

Thanks, B. Nortmann

[Ian Campbell]

On Tue, 2015-07-14 at 14:22 +0200, Bernhard Nortmann wrote:
> Regarding the 'auto-execution' of code: I think this is somewhat
> independent of
> the suggested extension to load the main U-Boot binary. It would probably be
> useful if the fel utility was able to keep track of addresses it has
> written to (this
> includes the "write" command), so a later "fel exe" could refer - or
> even default -
> to them. Maybe it's possible to also introduce 'symbolic' names for specific
> addresses? I'm thinking "fel exe uboot" here... (with "uboot" being set /
> preserved from the aw_fel_write_uboot_image function)

That does sound cool, but perhaps a lot of work to achieve?

Perhaps a simpler thing which would get 80% of the benefit would be to
add an --exec flag to the existing command? (Or default on and a flag to
not do it, as people prefer).

Ian.

[Siarhei Siamashka]

We can surely do a lot of things. However it's best to implement what
is requested by the users and is convenient for them. There are
a few major use cases to consider.

One of them (that's what Ian Campbell wants) is just to load U-Boot via
FEL with as little hassle as possible. This is good for the devices,
where the default U-Boot environment is sufficient to boot the system
by scanning the default locations (USB stick, SD card or maybe NAND).
For this use case, it would be easier for the users to just run
"fel uboot u-boot-sunxi-with-spl.bin"
instead of something like (that's what you are suggesting, right?)
"fel spl u-boot-sunxi-with-spl.bin exe uboot"

Another use case is uploading the kernel and initramfs via FEL too.
This way we don't depend on the kernel being available on the device.
Especially in the case of some tablets, where we have nothing but the
USB OTG connector if the SD card slot is used for UART serial. We
still could do booting with a single, but admittedly long "fel" tool
command line:
"fel uboot u-boot-sunxi-with-spl.bin write 0x42000000 uImage write
0x43000000 sun7i-a20-cubietruck.dtb write 0x43100000 boot.scr write
0x43300000 rootfs.cpio.lzma.uboot"
The whole point of introducing an extra "uboot" command is to make this
long command line shorter (no need to have an explicit "exe" command
with either a real or symbolic address), completely hide the U-Boot
load address from the user (but still ensure that none of the "write"
commands can overwrite the U-Boot location).

And finally, there is a kind of "bare metal" use case. The user may
want to have DRAM initialized, but upload and run his own code
fragments instead of U-Boot. That's why the "spl" command still
remains useful. There could be advanced scripts, which can upload, run
and read back the results from memory. By the way, if the "spl" command
is modified to print the U-Boot load address to stdout, then we could
do (it's an alternative to your 'symbolic' address name approach):
"fel exe `fel spl u-boot-sunxi-with-spl.bin`"

Still, the point is that a new command is much more intuitive for the
end users. And the majority of them probably just wants to execute
U-Boot instead of studying complicated usage instructions and writing
wrapper scripts ;-)

[Siarhei Siamashka]

Yes, that's what I'm suggesting too.

But instead of "fel --exec spl u-boot-sunxi-with-spl.bin", in my opinion
it would be much better to use the "fel uboot u-boot-sunxi-with-spl.bin"
syntax.

It is shorter to type and is more intuitive ("spl" implies that only
the SPL part is executed and "uboot" implies that the whole U-Boot is
executed).

Also I'm not a big fan of adding a flag, which only applies to a single
command.

[Bernhard Nortmann]

"--exec" may actually have other purposes, when we combine it with the
more general use case of the "write" command. "fel --exec write <addr>
<file>" would save you from having to issue a separate "exe" command,
repeating the same address. But it's again a rather special case, and I
don't see how it would fit in well with multiple "write" commands in a
single invocation (use the address of the block that was written last?).

But I agree that "fel --exec spl" is not very intuitive as a replacement
for "fel uboot". It would somewhat imply "execution of SPL", which takes
place anyway - regardless of whether we're aiming for the "uboot" case
or not. So my feeling is a separate "uboot" command is more useful,
including the suggested logic (i.e. the ability to combine it with other
"write"s needed to transfer the required boot files, and the execution
of U-Boot as the final step). I'll prepare another patch for that shortly.

As you have also pointed out, my initial idea of preserving addresses
quickly becomes moot with separate"fel" invocations (e.g. from a
script), as there is no easy way to preserve values across those. And
workarounds (like using stdout or environment variables) tend to quickly
become ugly...

Regards, B. Nortmann

[Siarhei Siamashka]

In the IRC chat a few days ago, I promised to Bernhard to post some
ideas to the mailing list regarding 'boot.scr' handling by the 'fel'
tool. And it seems like it's best to pick up this discussion.

Right now the 'mksunxiboot' tool creates 32 bytes header in the
beginning of the SPL. This header size can be always extended if
necessary by changing the first branch instruction. Only the first
20 bytes are used currently. And the 12 byte padding block at the
offset 0x14 is currently filled with garbage (which typically ends
up being zeros in real SPL files, but we can't rely on this):
http://git.denx.de/?p=u-boot.git;a=blob;f=tools/mksunxiboot.c;h=3361251c8e7fffcb4ccbc3e29eb5e2e956608b0c#l19

In the boot0 binary, the offset 0x14 contains the 'pub_head_size'
field, which is a 32-bit integer:
https://github.com/linux-sunxi/sunxi-tools/blob/v1.2/bootinfo.c#L38

In order to easily distinguish between boot0 and U-Boot SPL, some sort
of a 32-bit magic value can be placed by the mksunxiboot tool at the
offset 0x14. Because the 'pub_head_size' value in boot0 can't be large
(we can't reasonably expect it to be larger than the limit imposed
by the SRAM size), the high 16 bits of 'pub_head_size' are expected
to be zero. So the high 16 bits of the word at 0x14 can be used for
identification. It is a good idea to also implement some kind of
versioning scheme (in the lower 16 bites?) in order to allow future
extensions. The rest of the header should be explicitly cleared
instead of writing garbage there.

Now on the 'fel' tool side. If the 'fel' tool recognizes the U-Boot
SPL magic at the offset 0x14, then it knows that there are 8 bytes
of storage space at 0x18 (initilized to 0 by mksunxiboot).

And when handling the command line parsing, just like it is recognizing
the main U-Boot binary format now, the 'fel' tool can easily recognize
the format of the 'boot.scr' blob when it is written to some arbitrary
location in DRAM via the 'write' command. If it detects the boot.scr
format, then the address of the loaded boot.scr can be stored at the
offset 0x18 in the header. And instead of abusing the "write" command
functionality, we can alternatively introduce a new one, but this is
a minor detail.

Now when U-Boot is running, the 'misc_init_r' function can check if
the value at the address 0x18 is non-zero, then set something like
'fel_scriptaddr' or 'ram_scriptaddr' variable in the environment.
And this variable can be later checked and used by bootcmd from the
default environment.

Regarding the custom user-saved environment. When booting over USB, we
don't transfer the environment blob to the device and have no way to
save it over USB back to the PC. Picking the environment from the SD
card is not a great idea in the FEL mode (this can lead to unexpected
surprises), and we probably want to make sure to suppress this somehow.

[Hans de Goede]

Hi,

I think having the ability for a fel-loaded u-boot to automatically
execute a fel-loaded boot.scr from its default environment, and to
only do so when fel loaded and not do so in other cases is a good idea.

As for the actual implementation I've no preference.

> Regarding the custom user-saved environment. When booting over USB, we
> don't transfer the environment blob to the device and have no way to
> save it over USB back to the PC. Picking the environment from the SD
> card is not a great idea in the FEL mode (this can lead to unexpected
> surprises), and we probably want to make sure to suppress this somehow.

With the upcoming / wip nand support this is already a problem, since
I want to have a single/unified build for both mmc and nand booting.

The plan here is to teach (add code for) u-boot to dynamically select
where the environment comes from. This will basically mean calling
spl_boot_device() not only from the spl but also in u-boot itself,
and then based on the answer load the env from either mmc or nand,
or in the case it returns BOOT_DEVICE_BOARD, not load any env at all.

This should solve the problem with fel + env more or less for free.

Regards,

Hans

[Siarhei Siamashka]

I don't have any preference either. In the current documentation,
we just advise patching U-Boot sources with a tiny patch. So this
particular use case is covered. Maybe not in the best way, but there
is no urgent problem to fix.

I guess, right now only Bernhard is interested in doing real work on
improvements in this area.

> > Regarding the custom user-saved environment. When booting over USB, we
> > don't transfer the environment blob to the device and have no way to
> > save it over USB back to the PC. Picking the environment from the SD
> > card is not a great idea in the FEL mode (this can lead to unexpected
> > surprises), and we probably want to make sure to suppress this somehow.
>
> With the upcoming / wip nand support this is already a problem, since
> I want to have a single/unified build for both mmc and nand booting.

I'm glad that we don't have a disagreement at least here :-)

> The plan here is to teach (add code for) u-boot to dynamically select
> where the environment comes from. This will basically mean calling
> spl_boot_device() not only from the spl but also in u-boot itself,
> and then based on the answer load the env from either mmc or nand,
> or in the case it returns BOOT_DEVICE_BOARD, not load any env at all.
>
> This should solve the problem with fel + env more or less for free.

Sounds good.

[Bernhard Nortmann]

Hi Siarhei, hello Hans!

One question that comes to my mind is if it's necessary to involve the
U-Boot
SPL here. As the main binary gets transferred to RAM via the fel utility, it
could easily apply some similar 'tagging' logic to the "main" U-Boot header?

*If* an actual mechanism to pass boot information from SPL to the main
binary
gets devised (which to my knowledge current doesn't exist), then Hans has a
good point in favor of the SPL variant, which then could pass on the FEL
flag.

>>>
>>> Now when U-Boot is running, the 'misc_init_r' function can check if
>>> the value at the address 0x18 is non-zero, then set something like
>>> 'fel_scriptaddr' or 'ram_scriptaddr' variable in the environment.
>>> And this variable can be later checked and used by bootcmd from the
>>> default environment.
>>
>> I think having the ability for a fel-loaded u-boot to automatically
>> execute a fel-loaded boot.scr from its default environment, and to
>> only do so when fel loaded and not do so in other cases is a good idea.
>>

I think this may involve two distinct pieces of information - one being the
"how was I booted?" question (e.g. by setting certain env variables, like
"fel_booted"). The other is whether an actual bootscript got provided (and
where, i.e. *_scriptaddr). Both could be useful on their own.

Instead of directly passing the (boot) script address, it would also be
possible to set some "boot parameter block" information - which then in turn
might provide further information (versioning might be an example).

>> As for the actual implementation I've no preference.
>
> I don't have any preference either. In the current documentation,
> we just advise patching U-Boot sources with a tiny patch. So this
> particular use case is covered. Maybe not in the best way, but there
> is no urgent problem to fix.
>
> I guess, right now only Bernhard is interested in doing real work on
> improvements in this area.
>

I agree that this is not a pressing matter. It's probably safe to assume
that
the normal user won't get in touch with FEL booting very often, and the
people
who use it are aware of the rough edges and possible workarounds.
Personally,
I'm also just using a customized U-Boot with the 'naive'
"source ${scriptaddr} || run distro_bootcmd" approach (while being conscious
of and avoiding the pitfalls this might imply).

But (if possible) it would be nice to 'streamline' this to handle
various boot
media gracefully. As Hans pointed out, the addition of a NAND boot
capability
already creates some new challenges in this domain.

>>> Regarding the custom user-saved environment. When booting over USB, we
>>> don't transfer the environment blob to the device and have no way to
>>> save it over USB back to the PC. Picking the environment from the SD
>>> card is not a great idea in the FEL mode (this can lead to unexpected
>>> surprises), and we probably want to make sure to suppress this somehow.
>>
>> With the upcoming / wip nand support this is already a problem, since
>> I want to have a single/unified build for both mmc and nand booting.
>
> I'm glad that we don't have a disagreement at least here :-)
>
>> The plan here is to teach (add code for) u-boot to dynamically select
>> where the environment comes from. This will basically mean calling
>> spl_boot_device() not only from the spl but also in u-boot itself,
>> and then based on the answer load the env from either mmc or nand,
>> or in the case it returns BOOT_DEVICE_BOARD, not load any env at all.
>>
>> This should solve the problem with fel + env more or less for free.
>
> Sounds good.
>

Thanks for following up the discussion and your writeup on the subject!

Regards, B. Nortmann

[Hans de Goede]

Hi,

On 09/01/2015 02:16 PM, Bernhard Nortmann wrote:

> But (if possible) it would be nice to 'streamline' this to handle various boot
> media gracefully. As Hans pointed out, the addition of a NAND boot capability
> already creates some new challenges in this domain.

+1 for streamlining this. As for the technical details, just do what you think
is best, then submit patches and we'll see from there :)

Regards,

Hans

[Siarhei Siamashka]

The question is why would we want to involve both SPL and main U-Boot
headers if just the SPL header alone is enough?

The SPL header is Allwinner specific and we are kinda in control of it.
The U-Boot header is used for a wide range of devices and architectures
and you should be prepared to encounter a (perfectly justified) opposition
if you propose changes to it.

> *If* an actual mechanism to pass boot information from SPL to the main
> binary
> gets devised (which to my knowledge current doesn't exist), then Hans has a
> good point in favor of the SPL variant, which then could pass on the FEL
> flag.

You can read SRAM from the main U-Boot binary and access the SPL header
just fine. That's the mechanism to pass boot information, which already
implicitly exists.

Reading from SRAM can be used, for example, to get the console log
messages from the SPL also show on HDMI monitor or LCD screen:
http://lists.denx.de/pipermail/u-boot/2015-January/201128.html

We need to be careful with the FEL booting though, because the data
in SRAM gets scrambled after returning from the SPL back to the FEL
handler. But the first ~6K of data in SRAM is fine and this area
includes the SPL header.

> >>>
> >>> Now when U-Boot is running, the 'misc_init_r' function can check if
> >>> the value at the address 0x18 is non-zero, then set something like
> >>> 'fel_scriptaddr' or 'ram_scriptaddr' variable in the environment.
> >>> And this variable can be later checked and used by bootcmd from the
> >>> default environment.
> >>
> >> I think having the ability for a fel-loaded u-boot to automatically
> >> execute a fel-loaded boot.scr from its default environment, and to
> >> only do so when fel loaded and not do so in other cases is a good idea.
> >>
>
> I think this may involve two distinct pieces of information - one being the
> "how was I booted?" question (e.g. by setting certain env variables, like
> "fel_booted"). The other is whether an actual bootscript got provided (and
> where, i.e. *_scriptaddr). Both could be useful on their own.

That's a minor implementation detail. Some variations are possible,
people just need to come to an agreement about it.

> Instead of directly passing the (boot) script address, it would also be
> possible to set some "boot parameter block" information - which then in turn
> might provide further information (versioning might be an example).

Well, we have the SPL header with some spare space in it. You can name
it as "boot parameter block" if you want to.

> >> As for the actual implementation I've no preference.
> >
> > I don't have any preference either. In the current documentation,
> > we just advise patching U-Boot sources with a tiny patch. So this
> > particular use case is covered. Maybe not in the best way, but there
> > is no urgent problem to fix.
> >
> > I guess, right now only Bernhard is interested in doing real work on
> > improvements in this area.
> >
>
> I agree that this is not a pressing matter. It's probably safe to assume
> that
> the normal user won't get in touch with FEL booting very often, and the
> people
> who use it are aware of the rough edges and possible workarounds.
> Personally,
> I'm also just using a customized U-Boot with the 'naive'
> "source ${scriptaddr} || run distro_bootcmd" approach (while being conscious
> of and avoiding the pitfalls this might imply).

What I wanted to say here is that I don't have time for working on this
code right now, and it's not a critical bugfix. I'm also sorry for a
late reply. If you want to implement this boot.scr handling and
submit patches to U-Boot, then you are welcome.

There is one more interesting thing, which can potentially speed up
booting (the fastboot patches from Maxime):
http://lists.denx.de/pipermail/u-boot/2015-August/226053.html
http://lists.denx.de/pipermail/u-boot/2015-September/226207.html
http://lists.denx.de/pipermail/u-boot/2015-September/226211.html

It does not have to be fastboot, but any USB protocol would be fine
as long as it provides the necessary features (upload of the kernel,
DTB blob, initrd, ...) and works faster than FEL transfers :-)

> But (if possible) it would be nice to 'streamline' this to handle
> various boot
> media gracefully. As Hans pointed out, the addition of a NAND boot
> capability
> already creates some new challenges in this domain.

U-Boot already handles SDCARD and USB (FEL) boot methods gracefully with
a single build configuration. Now NAND boot method is also going to be
added to the mix.

The environment data handling is a minor quirk, which has been
discovered and is probably going to be fixed. There is no real
challenge.

> >>> Regarding the custom user-saved environment. When booting over USB, we
> >>> don't transfer the environment blob to the device and have no way to
> >>> save it over USB back to the PC. Picking the environment from the SD
> >>> card is not a great idea in the FEL mode (this can lead to unexpected
> >>> surprises), and we probably want to make sure to suppress this somehow.
> >>
> >> With the upcoming / wip nand support this is already a problem, since
> >> I want to have a single/unified build for both mmc and nand booting.
> >
> > I'm glad that we don't have a disagreement at least here :-)
> >
> >> The plan here is to teach (add code for) u-boot to dynamically select
> >> where the environment comes from. This will basically mean calling
> >> spl_boot_device() not only from the spl but also in u-boot itself,
> >> and then based on the answer load the env from either mmc or nand,
> >> or in the case it returns BOOT_DEVICE_BOARD, not load any env at all.
> >>
> >> This should solve the problem with fel + env more or less for free.
> >
> > Sounds good.
> >
>
> Thanks for following up the discussion and your writeup on the subject!
>
> Regards, B. Nortmann

[Bernhard Nortmann]

Hi Siarhei!

Am 01.09.2015 um 15:54 schrieb Siarhei Siamashka:
> The SPL header is Allwinner specific and we are kinda in control of it.
> The U-Boot header is used for a wide range of devices and architectures
> and you should be prepared to encounter a (perfectly justified) opposition
> if you propose changes to it.
>

> You can read SRAM from the main U-Boot binary and access the SPL header
> just fine. That's the mechanism to pass boot information, which already
> implicitly exists.

After looking a bit closer at actual U-Boot and mksunxiboot code, I fully
agree. Having a "sunxi" header (that could be extended as needed) under
our control, and readily accessible in SRAM, seems indeed the natural
"transport" choice.

> There is one more interesting thing, which can potentially speed up
> booting (the fastboot patches from Maxime):
> http://lists.denx.de/pipermail/u-boot/2015-August/226053.html
> http://lists.denx.de/pipermail/u-boot/2015-September/226207.html
> http://lists.denx.de/pipermail/u-boot/2015-September/226211.html
>
> It does not have to be fastboot, but any USB protocol would be fine
> as long as it provides the necessary features (upload of the kernel,
> DTB blob, initrd, ...) and works faster than FEL transfers :-)

From what I've read so far, fastboot is a transport mechanism that has to
be implemented by U-Boot (once it's up and running). So I take it this
wouldn't immediately affect the new SPL header (logic) and "fel" utility?
(And the newly gained ability to recognize/support FEL boot would still
come in handy if we plan to actually support a "two-stage" boot,
including a transition to a different transfer protocol.)

Regards, B. Nortmann