[PATCH v2 2/3] sunxi-tools: factor out persistent version and sram_info information

[Bernhard Nortmann]

FEL version information and SoC-specific memory layout are used
across several places in the fel utility code. To avoid having
to retrieve this information repeatedly, this patch introduces
suitable variables to the main() function scope.

Signed-off-by: Bernhard Nortmann <bernhard...@web.de>
---
fel.c | 41 +++++++++++++++++++----------------------
1 file changed, 19 insertions(+), 22 deletions(-)

diff --git a/fel.c b/fel.c
index 3fe72dc..e3566fb 100644
--- a/fel.c
+++ b/fel.c
@@ -222,13 +222,10 @@ void aw_fel_get_version(libusb_device_handle *usb, struct aw_fel_version *buf)
buf->pad[1] = le32toh(buf->pad[1]);
}

-void aw_fel_print_version(libusb_device_handle *usb)
+void aw_fel_print_version(struct aw_fel_version *buf)
{
- struct aw_fel_version buf;
- aw_fel_get_version(usb, &buf);
-
const char *soc_name="unknown";
- switch (buf.soc_id) {
+ switch (buf->soc_id) {
case 0x1623: soc_name="A10";break;
case 0x1625: soc_name="A13";break;
case 0x1633: soc_name="A31";break;
@@ -241,9 +238,9 @@ void aw_fel_print_version(libusb_device_handle *usb)
}

printf("%.8s soc=%08x(%s) %08x ver=%04x %02x %02x scratchpad=%08x %08x %08x\n",
- buf.signature, buf.soc_id, soc_name, buf.unknown_0a,
- buf.protocol, buf.unknown_12, buf.unknown_13,
- buf.scratchpad, buf.pad[0], buf.pad[1]);
+ buf->signature, buf->soc_id, soc_name, buf->unknown_0a,
+ buf->protocol, buf->unknown_12, buf->unknown_13,
+ buf->scratchpad, buf->pad[0], buf->pad[1]);
}

void aw_fel_read(libusb_device_handle *usb, uint32_t offset, void *buf, size_t len)
@@ -501,19 +498,15 @@ soc_sram_info generic_sram_info = {
.swap_buffers = generic_sram_swap_buffers,
};

-soc_sram_info *aw_fel_get_sram_info(libusb_device_handle *usb)
+soc_sram_info *aw_fel_get_sram_info(struct aw_fel_version *buf)
{
int i;
- struct aw_fel_version buf;
-
- aw_fel_get_version(usb, &buf);
-
for (i = 0; soc_sram_info_table[i].swap_buffers; i++)
- if (soc_sram_info_table[i].soc_id == buf.soc_id)
+ if (soc_sram_info_table[i].soc_id == buf->soc_id)
return &soc_sram_info_table[i];

printf("Warning: no 'soc_sram_info' data for your SoC (id=%04X)\n",
- buf.soc_id);
+ buf->soc_id);
return &generic_sram_info;
}

@@ -728,9 +721,8 @@ void aw_restore_and_enable_mmu(libusb_device_handle *usb,
#define SPL_LEN_LIMIT 0x8000

void aw_fel_write_and_execute_spl(libusb_device_handle *usb,
- uint8_t *buf, size_t len)
+ soc_sram_info *sram_info, uint8_t *buf, size_t len)
{
- soc_sram_info *sram_info = aw_fel_get_sram_info(usb);
sram_swap_buffers *swap_buffers;
char header_signature[9] = { 0 };
size_t i, thunk_size;
@@ -922,13 +914,13 @@ void aw_fel_write_uboot_image(libusb_device_handle *usb,
* This function handles the common part of both "spl" and "uboot" commands.
*/
void aw_fel_process_spl_and_uboot(libusb_device_handle *usb,
- const char *filename)
+ soc_sram_info *sram_info, const char *filename)
{
/* load file into memory buffer */
size_t size;
uint8_t *buf = load_file(filename, &size);
/* write and execute the SPL from the buffer */
- aw_fel_write_and_execute_spl(usb, buf, size);
+ aw_fel_write_and_execute_spl(usb, sram_info, buf, size);
/* check for optional main U-Boot binary (and transfer it, if applicable) */
if (size > SPL_LEN_LIMIT)
aw_fel_write_uboot_image(usb, buf + SPL_LEN_LIMIT, size - SPL_LEN_LIMIT);
@@ -1044,6 +1036,11 @@ int main(int argc, char **argv)
exit(1);
}

+ /* retrieve persistent version information and sram_info */
+ struct aw_fel_version fel_version;
+ aw_fel_get_version(handle, &fel_version);
+ soc_sram_info *sram_info = aw_fel_get_sram_info(&fel_version);
+
if (argc > 1 && (strcmp(argv[1], "--verbose") == 0 ||
strcmp(argv[1], "-v") == 0)) {
verbose = 1;
@@ -1064,7 +1061,7 @@ int main(int argc, char **argv)
aw_fel_execute(handle, strtoul(argv[2], NULL, 0));
skip=3;
} else if (strncmp(argv[1], "ver", 3) == 0 && argc > 1) {
- aw_fel_print_version(handle);
+ aw_fel_print_version(&fel_version);
skip=1;
} else if (strcmp(argv[1], "write") == 0 && argc > 3) {
double t1, t2;
@@ -1093,10 +1090,10 @@ int main(int argc, char **argv)
aw_fel_fill(handle, strtoul(argv[2], NULL, 0), strtoul(argv[3], NULL, 0), (unsigned char)strtoul(argv[4], NULL, 0));
skip=4;
} else if (strcmp(argv[1], "spl") == 0 && argc > 2) {
- aw_fel_process_spl_and_uboot(handle, argv[2]);
+ aw_fel_process_spl_and_uboot(handle, sram_info, argv[2]);
skip=2;
} else if (strcmp(argv[1], "uboot") == 0 && argc > 2) {
- aw_fel_process_spl_and_uboot(handle, argv[2]);
+ aw_fel_process_spl_and_uboot(handle, sram_info, argv[2]);
uboot_autostart = (uboot_entry > 0 && uboot_size > 0);
if (!uboot_autostart)
printf("Warning: \"uboot\" command failed to detect image! Can't execute U-Boot.\n");
--
2.4.6

[Bernhard Nortmann]

This patch makes use of a new sunxi SPL header format, where
fields for passing data to U-Boot are reserved. This allows the
"fel" utility to provide specific pieces of information by
setting these fields (which will be evaluated by U-Boot then).
For now, we may specify a memory region by address and size.

Currently the typical use case is to provide the memory location
of the boot script file (boot.scr). A suitably modified U-Boot
can adjust the boot process accordingly and will auto-execute
the script.

Signed-off-by: Bernhard Nortmann <bernhard...@web.de>
---

fel.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/fel.c b/fel.c
index e3566fb..cd10dd3 100644
--- a/fel.c
+++ b/fel.c
@@ -926,6 +926,48 @@ void aw_fel_process_spl_and_uboot(libusb_device_handle *usb,

aw_fel_write_uboot_image(usb, buf + SPL_LEN_LIMIT, size - SPL_LEN_LIMIT);
}

+/*
+ * Test the SPL header for our "sunxi" variant. We want to make sure that
+ * we can safely use specific header fields to pass information to U-Boot.
+ * In case of a missing signature (e.g. Allwinner boot0) or header version
+ * mismatch, this function will return "false". If all seems fine,
+ * the result is "true".
+ */
+#define SPL_SIGNATURE "SPL" /* marks "sunxi" header */
+#define SPL_EXPECTED_VERSION 1
+int have_sunxi_spl(libusb_device_handle *usb, soc_sram_info *sram_info)
+{
+ const uint8_t expected[] = SPL_SIGNATURE;
+ uint8_t spl_signature[4];
+
+ aw_fel_read(usb, sram_info->spl_addr + 0x14,
+ &spl_signature, sizeof(spl_signature));
+ return (memcmp(spl_signature, expected, 3) == 0
+ && spl_signature[3] == SPL_EXPECTED_VERSION);
+}
+
+/*
+ * Pass information to U-Boot via specialized fields in the SPL header
+ * (see "boot_file_head" in ${U-BOOT}/tools/mksunxiboot.c), providing
+ * information about some data address and size.
+ * Typically this will be the DRAM location of a boot script (boot.scr).
+ */
+void pass_fel_information(libusb_device_handle *usb, soc_sram_info *sram_info,
+ uint32_t data_addr, size_t data_size)
+{
+ /* write something _only_ if we have a suitable SPL header */
+ if (have_sunxi_spl(usb, sram_info)) {
+ struct {
+ uint32_t fel_data_address;
+ uint32_t fel_data_size;
+ } transfer = {data_addr, data_size};
+ pr_info("Passing boot info via sunxi SPL: data addr 0x%08X, size 0x%X\n",
+ transfer.fel_data_address, transfer.fel_data_size);
+ aw_fel_write(usb, &transfer,
+ sram_info->spl_addr + 0x18, sizeof(transfer));
+ }
+}
+
static int aw_fel_get_endpoint(libusb_device_handle *usb)
{
struct libusb_device *dev = libusb_get_device(usb);
@@ -1067,13 +1109,24 @@ int main(int argc, char **argv)
double t1, t2;
size_t size;
void *buf = load_file(argv[3], &size);
+ uint32_t offset = strtoul(argv[2], NULL, 0);
t1 = gettime();
- aw_fel_write(handle, buf, strtoul(argv[2], NULL, 0), size);
+ aw_fel_write(handle, buf, offset, size);
t2 = gettime();
if (t2 > t1)
pr_info("Written %.1f KB in %.1f sec (speed: %.1f KB/s)\n",
(double)size / 1000., t2 - t1,
(double)size / (t2 - t1) / 1000.);
+
+ /*
+ * If we have transferred a script, try to inform U-Boot about it.
+ * An IH_TYPE_SCRIPT has a well-defined header (including length),
+ * so we pass only the address and keep the size value at zero.
+ * (Reserving size > 0 for different data / future extensions.)
+ */
+ if (get_image_type(buf, size) == IH_TYPE_SCRIPT)
+ pass_fel_information(handle, sram_info, offset, 0);
+
free(buf);
skip=3;
} else if (strcmp(argv[1], "read") == 0 && argc > 4) {
--
2.4.6

[Bernhard Nortmann]

This patch moves retrieving the image header type into a function
of its own. The idea is that aw_fel_write() can and will also make
use of this function to detect boot scripts (by IH_TYPE_SCRIPT).

Signed-off-by: Bernhard Nortmann <bernhard...@web.de>
---

fel.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++------------------
1 file changed, 53 insertions(+), 19 deletions(-)

diff --git a/fel.c b/fel.c
index 98e8d89..3fe72dc 100644
--- a/fel.c
+++ b/fel.c
@@ -108,6 +108,46 @@ void usb_bulk_recv(libusb_device_handle *usb, int ep, void *data, int length)
}
}

+/* Constants taken from ${U-BOOT}/include/image.h */
+#define IH_MAGIC 0x27051956 /* Image Magic Number */
+#define IH_ARCH_ARM 2 /* ARM */
+#define IH_TYPE_INVALID 0 /* Invalid Image */
+#define IH_TYPE_FIRMWARE 5 /* Firmware Image */
+#define IH_TYPE_SCRIPT 6 /* Script file */
+#define IH_NMLEN 32 /* Image Name Length */
+
+/* Additional error codes, newly introduced for get_image_type() */
+#define IH_TYPE_ARCH_MISMATCH -1
+
+#define HEADER_NAME_OFFSET 32 /* offset of name field */
+#define HEADER_SIZE (HEADER_NAME_OFFSET + IH_NMLEN)
+
+/*
+ * Utility function to determine the image type from a mkimage-compatible
+ * header at given buffer (address).
+ *
+ * For invalid headers (insufficient size or 'magic' mismatch) the function
+ * will return IH_TYPE_INVALID. Negative return values might indicate
+ * special error conditions, e.g. IH_TYPE_ARCH_MISMATCH signals that the
+ * image doesn't match the expected (ARM) architecture.
+ * Otherwise the function will return the "ih_type" field for valid headers.
+ */
+int get_image_type(const uint8_t *buf, size_t len)
+{
+ uint32_t *buf32 = (uint32_t *)buf;
+
+ if (len <= HEADER_SIZE) /* insufficient length/size */
+ return IH_TYPE_INVALID;
+ if (be32toh(buf32[0]) != IH_MAGIC) /* signature mismatch */
+ return IH_TYPE_INVALID;
+ /* For sunxi, we always expect ARM architecture here */
+ if (buf[29] != IH_ARCH_ARM)
+ return IH_TYPE_ARCH_MISMATCH;
+
+ /* assume a valid header, and return ih_type */
+ return buf[30];
+}
+
void aw_send_usb_request(libusb_device_handle *usb, int type, int length)
{
struct aw_usb_request req;
@@ -819,15 +859,6 @@ void aw_fel_write_and_execute_spl(libusb_device_handle *usb,
aw_restore_and_enable_mmu(usb, sram_info, 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)
-
/*
* This function tests a given buffer address and length for a valid U-Boot
* image. Upon success, the image data gets transferred to the default memory
@@ -840,25 +871,28 @@ void aw_fel_write_uboot_image(libusb_device_handle *usb,
if (len <= HEADER_SIZE)
return; /* Insufficient size (no actual data), just bail out */

- /* Check for a valid mkimage header */
uint32_t *buf32 = (uint32_t *)buf;

- if (be32toh(buf32[0]) != IH_MAGIC) {
- fprintf(stderr, "U-Boot image verification failure: "
- "expected IH_MAGIC, got 0x%X\n", be32toh(buf32[0]));
+ /* Check for a valid mkimage header */
+ int image_type = get_image_type(buf, len);
+ if (image_type == IH_TYPE_INVALID) {
+ fprintf(stderr, "Invalid U-Boot image: bad size or signature\n");
+ exit(1);
+ }
+ if (image_type == IH_TYPE_ARCH_MISMATCH) {
+ fprintf(stderr, "Invalid U-Boot image: wrong architecture\n");
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]);
+ if (image_type != IH_TYPE_FIRMWARE) {
+ fprintf(stderr, "U-Boot image type mismatch: "
+ "expected IH_TYPE_FIRMWARE, got %02X\n", image_type);
exit(1);
}
uint32_t data_size = be32toh(buf32[3]); /* Image Data Size */
uint32_t load_addr = be32toh(buf32[4]); /* Data Load Address */
- if ((size_t)data_size != len - HEADER_SIZE) {
+ if (data_size != len - HEADER_SIZE) {
fprintf(stderr, "U-Boot image data size mismatch: "
- "expected %d, got %u\n", (int)len - HEADER_SIZE,
- data_size);
+ "expected %u, got %u\n", len - HEADER_SIZE, data_size);
exit(1);
}
/* TODO: Verify image data integrity using the checksum field ih_dcrc,
--
2.4.6

[Siarhei Siamashka]

On Wed, 9 Sep 2015 14:53:24 +0200
Bernhard Nortmann <bernhard...@web.de> wrote:

> This patch moves retrieving the image header type into a function
> of its own. The idea is that aw_fel_write() can and will also make
> use of this function to detect boot scripts (by IH_TYPE_SCRIPT).
>
> Signed-off-by: Bernhard Nortmann <bernhard...@web.de>

Thanks, this patch looks much better now.

A minor nitpick. Basically, if we want to know if there was anything
wrong, then this can be done by checking if the return code is less
than or equal to IH_TYPE_INVALID. Such check could be also wrapped in
a macro.

This is somewhat related to my previous comment. What if we add a new
negative error code later? Then the program will still print this
"U-Boot image type mismatch: expected IH_TYPE_FIRMWARE ..." error
message, unless somebody adds a new 'if' branch for the new error
code similar to how IH_TYPE_INVALID and IH_TYPE_ARCH_MISMATCH cases
are handled. This is generally not very nice, though not really
critical either.

> uint32_t data_size = be32toh(buf32[3]); /* Image Data Size */
> uint32_t load_addr = be32toh(buf32[4]); /* Data Load Address */
> - if ((size_t)data_size != len - HEADER_SIZE) {
> + if (data_size != len - HEADER_SIZE) {
> fprintf(stderr, "U-Boot image data size mismatch: "
> - "expected %d, got %u\n", (int)len - HEADER_SIZE,
> - data_size);
> + "expected %u, got %u\n", len - HEADER_SIZE, data_size);

fel.c: In function ‘aw_fel_write_uboot_image’:
fel.c:895: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);

^

Why were these type casts dropped?

> exit(1);
> }
> /* TODO: Verify image data integrity using the checksum field ih_dcrc,

--

Best regards,
Siarhei Siamashka

[Siarhei Siamashka]

With this change, we are going to print "Warning: no 'soc_sram_info'
data for your SoC" message regardless of what kind of FEL command
is requested in the command line. Simple FEL commands don't need any
SoC-specific information, so we are going to be nagging the users
of the new SoC variants.

On the other hand, maybe this is not too bad?

[Siarhei Siamashka]

Version mismatch probably deserves a special error message. So that
the user knows what exactly is outdated (U-Boot or sunxi-tools) and
needs upgrading.

And also about SPL header versioning in general. Most likely we are
going to prefer backwards compatible changes (if any) in the future.
So that the offset 0x14 will be still used for storing a pointer to
boot.scr data even if we add more fields to the SPL header later.
However we can't rule out compatibility breaking changes either. So
it might be a good idea to have a 'major' version number and a 'minor'
version number. In the case only a minor version number is increased,
then we know that the changes are backwards compatible. But if a major
version number is changed, then we can't assume anything and should
treat it as an error.

Am I trying to over-engineer it unnecessarily?

> +}
> +
> +/*
> + * Pass information to U-Boot via specialized fields in the SPL header
> + * (see "boot_file_head" in ${U-BOOT}/tools/mksunxiboot.c), providing
> + * information about some data address and size.
> + * Typically this will be the DRAM location of a boot script (boot.scr).
> + */
> +void pass_fel_information(libusb_device_handle *usb, soc_sram_info *sram_info,
> + uint32_t data_addr, size_t data_size)
> +{
> + /* write something _only_ if we have a suitable SPL header */
> + if (have_sunxi_spl(usb, sram_info)) {
> + struct {
> + uint32_t fel_data_address;
> + uint32_t fel_data_size;

The necessity of this 'fel_data_size' field (and the naming of the SPL
header fields) is still being discussed in U-Boot:

http://lists.denx.de/pipermail/u-boot/2015-September/227650.html

After the discussion in the U-Boot mailing list comes to a conclusion,
we are going to know what to do.

Best regards,
Siarhei Siamashka

[Bernhard Nortmann]

Hi Siarhei!

Am 13.09.2015 um 23:30 schrieb Siarhei Siamashka:
> [...]

> A minor nitpick. Basically, if we want to know if there was anything
> wrong, then this can be done by checking if the return code is less
> than or equal to IH_TYPE_INVALID. Such check could be also wrapped in
> a macro.
>

> [...]

> This is somewhat related to my previous comment. What if we add a new
> negative error code later? Then the program will still print this
> "U-Boot image type mismatch: expected IH_TYPE_FIRMWARE ..." error
> message, unless somebody adds a new 'if' branch for the new error
> code similar to how IH_TYPE_INVALID and IH_TYPE_ARCH_MISMATCH cases
> are handled. This is generally not very nice, though not really
> critical either.

Yes, I was assuming that someone adding a new (negative) error code
would also arrange for handling that. But it's probably not the best
coding style. The solution I would suggest is an
"if (image_type <= IH_TYPE_INVALID) {}" block containing a switch
statement. The switch would handle specific error codes, or 'fall
back' to a default error message, and the if block would always
exit(1) at the end.

> fel.c: In function ‘aw_fel_write_uboot_image’:
> fel.c:895: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);
> ^
>
> Why were these type casts dropped?

Oh. It didn't result in an error/warning for me. The first cast is
obsolete after len is now size_t (i.e. unsigned) and guaranteed to
be equal to or larger than HEADER_SIZE. The second cast just seemed
quirky to me, but it looks like my replacement isn't really portable.
The correct format specificer for a size_t should be "%zu", I'll fix
that.

Regards, B. Nortmann

[Bernhard Nortmann]

Hello Siarhei!

Am 13.09.2015 um 23:42 schrieb Siarhei Siamashka:
> With this change, we are going to print "Warning: no 'soc_sram_info'
> data for your SoC" message regardless of what kind of FEL command
> is requested in the command line. Simple FEL commands don't need any
> SoC-specific information, so we are going to be nagging the users
> of the new SoC variants.
>
> On the other hand, maybe this is not too bad?

Good catch. I also think we could "get away" with that, but it may
not be the nicest way to deal with it. I would think that handling
this might be achieved by initializing sram_info to NULL, and only
call aw_fel_get_sram_info() later when sram_info is actually needed
("spl"/"uboot", or script transfer with "write")? It leads to slightly
more complicated code, but seems reasonable.

Regards, B. Nortmann

[Bernhard Nortmann]

Hi!

Am 14.09.2015 um 00:05 schrieb Siarhei Siamashka:
> Version mismatch probably deserves a special error message. So that
> the user knows what exactly is outdated (U-Boot or sunxi-tools) and
> needs upgrading.
>
> And also about SPL header versioning in general. Most likely we are
> going to prefer backwards compatible changes (if any) in the future.
> So that the offset 0x14 will be still used for storing a pointer to
> boot.scr data even if we add more fields to the SPL header later.
> However we can't rule out compatibility breaking changes either. So
> it might be a good idea to have a 'major' version number and a 'minor'
> version number. In the case only a minor version number is increased,
> then we know that the changes are backwards compatible. But if a major
> version number is changed, then we can't assume anything and should
> treat it as an error.
>
> Am I trying to over-engineer it unnecessarily?

Maybe. :D (also see below for my "size" field)

For now I had introduced a very basic check. The test for equality
may be overly simple, and should probably be replaced by some "<="
(for 'backwards' compatibility) once actual versioning actually starts
to matter. For the time being I anticipate(d) the "sunxi" SPL to
preserve fields once they are established (and not discard them later),
following your argument that it's simple enough to extend the header
(space) when necessary.

I haven't really thought of / planned on more sophisticated checks
(SPL_MIN_VERSION, SPL_MAX_VERSION) or major/minor numbers. The latter
might be achieved by (ab)using nibbles of the version uint8_t, turning
our initial version to 0.1 (with room for up to 0.15) and making 0x10
the next "major" version 1.0.

But I think all of this could be dealt with later, when the need for a
SPL change arises that would actually introduce "incompatible" versions.

> The necessity of this 'fel_data_size' field (and the naming of the SPL
> header fields) is still being discussed in U-Boot:
>
> http://lists.denx.de/pipermail/u-boot/2015-September/227650.html
>
> After the discussion in the U-Boot mailing list comes to a conclusion,
> we are going to know what to do.

Yes. Hans seems to share your initial thought on this field (being not
really necessary), and argues against "covering exotic variants of what
is already a corner case". So I feel inclined to follow this train of
thought more strictly, and avoid the "overengineering" when there's no
need for it. This includes dropping the "size" field and simply revert
it to a "uint32_t reserved;" instead.

Regards, B. Nortmann

[Siarhei Siamashka]

No, we can't just do nothing now and fix the problem later in this
particular case.

Right now your patch is silently doing nothing if it does not recognize
exactly the first version of the SPL header. Now imagine that we decide
that it is good enough for now. Then some Linux distribution packages
sunxi-tools in this state. Then at a later date, the SPL header in
U-Boot may need to be extended (for example, add uEnv.txt support or
introduce a flag to redirect serial console to the SD breakout board
or anything else). Then we can have the users trying to follow some
tutorial and expecting their boot.scr to be picked up by U-Boot. And
if boot.scr is just silently ignored, then they can have a lot of fun
troubleshooting or pestering people for support.

If the boot.scr format is recognized by the 'fel' tool and the U-Boot
image is recent enough to have the SPL header with the slot for boot.scr
address reserved, then the users should get comprehensive error messages
in the case of encountering an unexpected version of the SPL header.

> > The necessity of this 'fel_data_size' field (and the naming of the SPL
> > header fields) is still being discussed in U-Boot:
> >
> > http://lists.denx.de/pipermail/u-boot/2015-September/227650.html
> >
> > After the discussion in the U-Boot mailing list comes to a conclusion,
> > we are going to know what to do.
>
> Yes. Hans seems to share your initial thought on this field (being not
> really necessary), and argues against "covering exotic variants of what
> is already a corner case". So I feel inclined to follow this train of
> thought more strictly, and avoid the "overengineering" when there's no
> need for it. This includes dropping the "size" field and simply revert
> it to a "uint32_t reserved;" instead.

OK. Let's follow what happens in the U-Boot mailing list.

[Bernhard Nortmann]

This patch moves retrieving the image header type into a function

of its own. The idea is that fel "write" can and will also make

use of this function to detect boot scripts (by IH_TYPE_SCRIPT).

Signed-off-by: Bernhard Nortmann <bernhard...@web.de>
---

fel.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++-----------------
1 file changed, 59 insertions(+), 19 deletions(-)

diff --git a/fel.c b/fel.c
index 98e8d89..52b785f 100644
--- a/fel.c
+++ b/fel.c

@@ -108,6 +108,46 @@ void usb_bulk_recv(libusb_device_handle *usb, int ep, void *data, int length)
}
}

+/* Constants taken from ${U-BOOT}/include/image.h */
+#define IH_MAGIC 0x27051956 /* Image Magic Number */
+#define IH_ARCH_ARM 2 /* ARM */
+#define IH_TYPE_INVALID 0 /* Invalid Image */
+#define IH_TYPE_FIRMWARE 5 /* Firmware Image */
+#define IH_TYPE_SCRIPT 6 /* Script file */
+#define IH_NMLEN 32 /* Image Name Length */
+
+/* Additional error codes, newly introduced for get_image_type() */
+#define IH_TYPE_ARCH_MISMATCH -1
+
+#define HEADER_NAME_OFFSET 32 /* offset of name field */
+#define HEADER_SIZE (HEADER_NAME_OFFSET + IH_NMLEN)
+
+/*
+ * Utility function to determine the image type from a mkimage-compatible
+ * header at given buffer (address).
+ *
+ * For invalid headers (insufficient size or 'magic' mismatch) the function
+ * will return IH_TYPE_INVALID. Negative return values might indicate
+ * special error conditions, e.g. IH_TYPE_ARCH_MISMATCH signals that the
+ * image doesn't match the expected (ARM) architecture.
+ * Otherwise the function will return the "ih_type" field for valid headers.

@@ -840,25 +871,34 @@ void aw_fel_write_uboot_image(libusb_device_handle *usb,

if (len <= HEADER_SIZE)
return; /* Insufficient size (no actual data), just bail out */

- /* Check for a valid mkimage header */
uint32_t *buf32 = (uint32_t *)buf;

- if (be32toh(buf32[0]) != IH_MAGIC) {
- fprintf(stderr, "U-Boot image verification failure: "
- "expected IH_MAGIC, got 0x%X\n", be32toh(buf32[0]));
+ /* Check for a valid mkimage header */
+ int image_type = get_image_type(buf, len);

+ if (image_type <= IH_TYPE_INVALID) {
+ switch (image_type) {
+ case IH_TYPE_INVALID:

+ fprintf(stderr, "Invalid U-Boot image: bad size or signature\n");

+ break;
+ case IH_TYPE_ARCH_MISMATCH:

+ fprintf(stderr, "Invalid U-Boot image: wrong architecture\n");

+ break;
+ default:
+ fprintf(stderr, "Invalid U-Boot image: error code %d\n",
+ image_type);
+ }

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]);
+ if (image_type != IH_TYPE_FIRMWARE) {
+ fprintf(stderr, "U-Boot image type mismatch: "
+ "expected IH_TYPE_FIRMWARE, got %02X\n", image_type);
exit(1);
}

uint32_t data_size = be32toh(buf32[3]); /* Image Data Size */
uint32_t load_addr = be32toh(buf32[4]); /* Data Load Address */
- if ((size_t)data_size != len - HEADER_SIZE) {
+ if (data_size != len - HEADER_SIZE) {
fprintf(stderr, "U-Boot image data size mismatch: "
- "expected %d, got %u\n", (int)len - HEADER_SIZE,
- data_size);

+ "expected %zu, got %u\n", len - HEADER_SIZE, data_size);

exit(1);
}
/* TODO: Verify image data integrity using the checksum field ih_dcrc,
--

2.4.6

[Bernhard Nortmann]

FEL version information and SoC-specific memory layout are used
across several places in the fel utility code. To avoid having
to retrieve this information repeatedly, this patch introduces
suitable variables to the main() function scope.

Signed-off-by: Bernhard Nortmann <bernhard...@web.de>
---

fel.c | 41 +++++++++++++++++++----------------------
1 file changed, 19 insertions(+), 22 deletions(-)

diff --git a/fel.c b/fel.c
index 52b785f..7bfd25b 100644
--- a/fel.c
+++ b/fel.c

@@ -928,13 +920,13 @@ void aw_fel_write_uboot_image(libusb_device_handle *usb,

* This function handles the common part of both "spl" and "uboot" commands.
*/
void aw_fel_process_spl_and_uboot(libusb_device_handle *usb,
- const char *filename)
+ soc_sram_info *sram_info, const char *filename)
{
/* load file into memory buffer */
size_t size;
uint8_t *buf = load_file(filename, &size);
/* write and execute the SPL from the buffer */
- aw_fel_write_and_execute_spl(usb, buf, size);
+ aw_fel_write_and_execute_spl(usb, sram_info, buf, size);
/* check for optional main U-Boot binary (and transfer it, if applicable) */
if (size > SPL_LEN_LIMIT)

aw_fel_write_uboot_image(usb, buf + SPL_LEN_LIMIT, size - SPL_LEN_LIMIT);

@@ -1050,6 +1042,11 @@ int main(int argc, char **argv)

exit(1);
}

+ /* retrieve persistent version information and sram_info */
+ struct aw_fel_version fel_version;
+ aw_fel_get_version(handle, &fel_version);
+ soc_sram_info *sram_info = aw_fel_get_sram_info(&fel_version);
+
if (argc > 1 && (strcmp(argv[1], "--verbose") == 0 ||
strcmp(argv[1], "-v") == 0)) {
verbose = 1;

@@ -1070,7 +1067,7 @@ int main(int argc, char **argv)

aw_fel_execute(handle, strtoul(argv[2], NULL, 0));
skip=3;
} else if (strncmp(argv[1], "ver", 3) == 0 && argc > 1) {
- aw_fel_print_version(handle);
+ aw_fel_print_version(&fel_version);
skip=1;
} else if (strcmp(argv[1], "write") == 0 && argc > 3) {
double t1, t2;

@@ -1099,10 +1096,10 @@ int main(int argc, char **argv)

aw_fel_fill(handle, strtoul(argv[2], NULL, 0), strtoul(argv[3], NULL, 0), (unsigned char)strtoul(argv[4], NULL, 0));
skip=4;
} else if (strcmp(argv[1], "spl") == 0 && argc > 2) {
- aw_fel_process_spl_and_uboot(handle, argv[2]);
+ aw_fel_process_spl_and_uboot(handle, sram_info, argv[2]);
skip=2;
} else if (strcmp(argv[1], "uboot") == 0 && argc > 2) {
- aw_fel_process_spl_and_uboot(handle, argv[2]);
+ aw_fel_process_spl_and_uboot(handle, sram_info, argv[2]);
uboot_autostart = (uboot_entry > 0 && uboot_size > 0);
if (!uboot_autostart)
printf("Warning: \"uboot\" command failed to detect image! Can't execute U-Boot.\n");

--
2.4.6

[Bernhard Nortmann]

This patch makes use of a new sunxi SPL header format, where
fields for passing data to U-Boot are reserved. This allows the
"fel" utility to provide specific pieces of information by
setting these fields (which will be evaluated by U-Boot then).

For now, we provide the memory location of the boot script file
(boot.scr) this way. A suitably modified U-Boot can adjust the

boot process accordingly and will auto-execute the script.

Signed-off-by: Bernhard Nortmann <bernhard...@web.de>
---

fel.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 61 insertions(+), 1 deletion(-)

diff --git a/fel.c b/fel.c
index 7bfd25b..f697735 100644
--- a/fel.c
+++ b/fel.c
@@ -932,6 +932,60 @@ void aw_fel_process_spl_and_uboot(libusb_device_handle *usb,

aw_fel_write_uboot_image(usb, buf + SPL_LEN_LIMIT, size - SPL_LEN_LIMIT);
}

+/*
+ * Test the SPL header for our "sunxi" variant. We want to make sure that
+ * we can safely use specific header fields to pass information to U-Boot.
+ * In case of a missing signature (e.g. Allwinner boot0) or header version
+ * mismatch, this function will return "false". If all seems fine,
+ * the result is "true".
+ */
+#define SPL_SIGNATURE "SPL" /* marks "sunxi" header */

+#define SPL_MIN_VERSION 1 /* minimum required version */
+#define SPL_MAX_VERSION 1 /* maximum supported version */

+int have_sunxi_spl(libusb_device_handle *usb, soc_sram_info *sram_info)
+{

+ uint8_t spl_signature[4];
+
+ aw_fel_read(usb, sram_info->spl_addr + 0x14,
+ &spl_signature, sizeof(spl_signature));
+

+ if (memcmp(spl_signature, SPL_SIGNATURE, 3) != 0)
+ return 0; /* signature mismatch, no "sunxi" SPL */
+
+ if (spl_signature[3] < SPL_MIN_VERSION) {
+ fprintf(stderr, "sunxi SPL version mismatch: "
+ "found 0x%02X < required minimum 0x%02X\n",
+ spl_signature[3], SPL_MIN_VERSION);
+ fprintf(stderr, "You need to update your U-Boot (mksunxiboot) to a more recent version.\n");
+ return 0;
+ }
+ if (spl_signature[3] > SPL_MAX_VERSION) {
+ fprintf(stderr, "sunxi SPL version mismatch: "
+ "found 0x%02X > maximum supported 0x%02X\n",
+ spl_signature[3], SPL_MAX_VERSION);
+ fprintf(stderr, "You need a more recent version of this (sunxi-tools) fel utility.\n");
+ return 0;
+ }
+ return 1; /* sunxi SPL and suitable version */

+}
+
+/*
+ * Pass information to U-Boot via specialized fields in the SPL header
+ * (see "boot_file_head" in ${U-BOOT}/tools/mksunxiboot.c), providing

+ * information about the boot script address (DRAM location of boot.scr).

+ */
+void pass_fel_information(libusb_device_handle *usb, soc_sram_info *sram_info,

+ uint32_t script_address)

+{
+ /* write something _only_ if we have a suitable SPL header */
+ if (have_sunxi_spl(usb, sram_info)) {

+ pr_info("Passing boot info via sunxi SPL: script address = 0x%08X\n",
+ script_address);
+ aw_fel_write(usb, &script_address,
+ sram_info->spl_addr + 0x18, sizeof(script_address));

+ }
+}
+
static int aw_fel_get_endpoint(libusb_device_handle *usb)
{
struct libusb_device *dev = libusb_get_device(usb);

@@ -1073,13 +1127,19 @@ int main(int argc, char **argv)

double t1, t2;
size_t size;
void *buf = load_file(argv[3], &size);
+ uint32_t offset = strtoul(argv[2], NULL, 0);
t1 = gettime();
- aw_fel_write(handle, buf, strtoul(argv[2], NULL, 0), size);
+ aw_fel_write(handle, buf, offset, size);
t2 = gettime();
if (t2 > t1)
pr_info("Written %.1f KB in %.1f sec (speed: %.1f KB/s)\n",
(double)size / 1000., t2 - t1,
(double)size / (t2 - t1) / 1000.);
+

+ /* If we have transferred a script, try to inform U-Boot about it. */

+ if (get_image_type(buf, size) == IH_TYPE_SCRIPT)

+ pass_fel_information(handle, sram_info, offset);
+
free(buf);
skip=3;
} else if (strcmp(argv[1], "read") == 0 && argc > 4) {
--
2.4.6

[Bernhard Nortmann]

It might be preferable to actually make "fel_version" global.
aw_fel_get_sram_info() could operate on that data, and cache its result,
so the soc_sram_info pointer gets initialized only once (upon the first
request that makes actual use of sram_info). This way it's safe to call
the function multiple times / wherever needed.

What do you think?

Regards, B. Nortmann

[Siarhei Siamashka]

On Tue, 15 Sep 2015 14:02:28 +0200
Bernhard Nortmann <bernhard...@web.de> wrote:

> Am 14.09.2015 um 11:55 schrieb Bernhard Nortmann:
> > Hello Siarhei!
> >
> > Am 13.09.2015 um 23:42 schrieb Siarhei Siamashka:
> >> With this change, we are going to print "Warning: no 'soc_sram_info'
> >> data for your SoC" message regardless of what kind of FEL command
> >> is requested in the command line. Simple FEL commands don't need any
> >> SoC-specific information, so we are going to be nagging the users
> >> of the new SoC variants.
> >>
> >> On the other hand, maybe this is not too bad?
> >
> > Good catch. I also think we could "get away" with that, but it may
> > not be the nicest way to deal with it. I would think that handling
> > this might be achieved by initializing sram_info to NULL, and only
> > call aw_fel_get_sram_info() later when sram_info is actually needed
> > ("spl"/"uboot", or script transfer with "write")? It leads to slightly
> > more complicated code, but seems reasonable.
>

> It might be preferable to actually make "fel_version" global.
> aw_fel_get_sram_info() could operate on that data, and cache its result,
> so the soc_sram_info pointer gets initialized only once (upon the first
> request that makes actual use of sram_info). This way it's safe to call
> the function multiple times / wherever needed.
>
> What do you think?

Yes, caching the pointer inside of aw_fel_get_sram_info() function and
avoiding repeated usb requests is a good idea. Just make it a static
variable instead of global.

[Bernhard Nortmann]

Am 16.09.2015 um 03:15 schrieb Siarhei Siamashka:
> Yes, caching the pointer inside of aw_fel_get_sram_info() function and
> avoiding repeated usb requests is a good idea. Just make it a static
> variable instead of global.

Retrieving the version information (struct aw_fel_version) involves a
USB operation, but is 'safe' even for unknown SoCs. So main() is doing
that only once, and as soon as it got a usable USB endpoint.

It's a question of visibility. I'd like aw_fel_get_sram_info() to work
with that existing data - so we either need it within global scope, or
have to pass it as parameter to the function. The latter once again
tends to make the function calls more complicated / less readable.

I could work around it by applying a similar "caching" logic to
aw_fel_get_version(), but my impression is that we might be trying
(too?) hard to 'hide' data that actually is of "global" nature.

Regards, B. Nortmann

[Siarhei Siamashka]

What kind of problem are you trying to solve in the first place?
Is it just for suppressing multiple prints of the "unsupported soc
type" warning?

Getting rid of a few extra USB requests is unlikely to improve
performance. If you are really worried about the performance, then
we should preferably focus on trying to get rid of the 250ms sleep
and improving data transfer speeds.

Global variables are not nice. It's not always easy to track when
they are already initialized and safe to use.

Regarding the "global" nature of this data. Just as a theoretical
example, we may consider adding support for handling multiple
connected devices at once. Of course not in the "fel" command
line tool. But as some sort of a daemon, which waits for connected
Allwinner devices in FEL mode, retrieves the unique SID value
from each device and uploads appropriate bootloader images to
them. With the accessor function, reusing the existing code
would be easy. But with global variables, more significant
changes would be needed.

Anyway, I really don't care and will just push your pending
patches to the sunxi-tools repository after some minimal
testing.

[Bernhard Nortmann]

Am 26.09.2015 um 22:06 schrieb Siarhei Siamashka:
> What kind of problem are you trying to solve in the first place?
> Is it just for suppressing multiple prints of the "unsupported soc
> type" warning?

Basically: yes. Initially you got me triggered on that with your very
first comment. Admittedly I went overboard here - after all, the idea
of caching the aw_fel_get_sram_info() result already solves my "problem".
There's no need to rework and/or constrain the other USB operations,
namely retrieving aw_fel_version information.

I've prepared and submitted a v4 iteration of the series, that I would
consider 'final':
https://groups.google.com/forum/#!topic/linux-sunxi/1B3V7gmBnsk

Regards, B. Nortmann