On Sat, 4 Jul 2015 20:31:47 +0200 Bernhard Nortmann <[email protected]> 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 <[email protected]> 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. > --- > 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) { 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== > + 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); 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 -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
