From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bernhard Nortmann Date: Fri, 3 Jun 2016 09:02:37 +0200 Subject: [U-Boot] [RFC] sunxi: Store the device tree name in the SPL header In-Reply-To: <1464863537-2307-1-git-send-email-siarhei.siamashka@gmail.com> References: <1464863537-2307-1-git-send-email-siarhei.siamashka@gmail.com> Message-ID: <57512B8D.5070702@web.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Am 02.06.2016 um 12:32 schrieb Siarhei Siamashka: > This patch updates the mksunxiboot tool to optionally add > the default device tree name string to the SPL header. This > information can be used by the firmware upgrade tools to > protect users from harming themselves by trying to upgrade > to an incompatible bootloader. > > The primary use case here is a non-removable bootable media > (such as NAND, eMMC or SPI flash), which already may have > a properly working, but a little bit outdated bootloader > installed. For example, the user may download or build a > new U-Boot image for "Cubieboard", and then attemept to > install it on a "Cubieboard2" hardware by mistake as a > replacement for the already existing bootloader. If this > happens, the flash programming tool can identify this > problem and warn the user. > > CONFIG_OF_LIST may provide more than one compatible device > tree name when supporting multiple boards by a single > defconfig. In such cases, the right device tree name is > getting identified at runtime from the list of multiple > options. The patch calculates a 32-bit hash value (crc32) > of this list and stores it to the SPL header too, because > this is better than nothing and still can be used to warn > about potentially incompatible upgrades. > > The size of the SPL header is also increased from 64 bytes > to 96 bytes to provide enough space for the device tree name > string. > > Signed-off-by: Siarhei Siamashka > --- > arch/arm/include/asm/arch-sunxi/spl.h | 26 +++++++++++-- > include/configs/sunxi-common.h | 12 +++--- > scripts/Makefile.spl | 3 +- > tools/Makefile | 1 + > tools/mksunxiboot.c | 71 +++++++++++++++++++++++++++++++++-- > 5 files changed, 98 insertions(+), 15 deletions(-) Even with this, I guess there are still (slim) chances that users get themselves into trouble - e.g. by having 'bricked' their devices after they flashed an incompatible SPL/U-Boot, and then trying to repair the damage via "FEL recovery". (The existing SPL would carry the wrong info.) But it seems a useful approach, as long as the flasher only warns and allows to continue - possibly requiring confirmation (enter "yes" to proceed) or the use of some "--force" option. In general, I like the idea. > diff --git a/arch/arm/include/asm/arch-sunxi/spl.h b/arch/arm/include/asm/arch-sunxi/spl.h > index a0f33b0..f370222 100644 > --- a/arch/arm/include/asm/arch-sunxi/spl.h > +++ b/arch/arm/include/asm/arch-sunxi/spl.h > @@ -10,7 +10,7 @@ > > #define BOOT0_MAGIC "eGON.BT0" > #define SPL_SIGNATURE "SPL" /* marks "sunxi" SPL header */ > -#define SPL_HEADER_VERSION 1 > +#define SPL_HEADER_VERSION 2 > > #if defined(CONFIG_MACH_SUN9I) || defined(CONFIG_MACH_SUN50I) > #define SPL_ADDR 0x10000 > @@ -49,9 +49,27 @@ struct boot_file_head { > uint8_t spl_signature[4]; > }; > uint32_t fel_script_address; > - uint32_t reserved1[3]; > - uint32_t boot_media; /* written here by the boot ROM */ > - uint32_t reserved2[5]; /* padding, align to 64 bytes */ > + uint32_t reserved1[1]; There's no "reserved2" any more, and no need to keep that an array, so just "uint32_t reserved;"? > + /* > + * Offset of an ASCIIZ string (relative to the SPL header), which > + * contains the default device tree name (CONFIG_DEFAULT_DEVICE_TREE). > + * This is optional and may be set to NULL. Is intended to be used > + * by flash programming tools for providing nice informative messages > + * to the users. > + */ > + uint32_t offset_of_the_default_device_tree_name; > + /* > + * A 32-bit hash value, calculated from the list of compatible device > + * tree names (CONFIG_OF_LIST or CONFIG_DEFAULT_DEVICE_TREE). Is > + * intended to be used by flash programming tools for safeguearding > + * against upgrading to an incompatible firmware. > + */ > + uint32_t crc32_of_the_compatible_device_tree_list; > + /* The 'boot_media' code is written here by the boot ROM */ > + uint32_t boot_media; > + /* A padding area (may be used for storing text strings) */ > + uint32_t string_pool[13]; > + /* The header must be a multiple of 32 bytes (for VBAR alignment) */ I see no particular reason why this should be uint32_t[13] instead of char[52], with a suitable (multiple of 4) array size to ensure proper header alignment. You're introducing a check / safeguard for the header size anyway, and char[] saves us from having to do a typecast later on. > }; > > #define is_boot0_magic(addr) (memcmp((void *)addr, BOOT0_MAGIC, 8) == 0) > diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h > index b33cfb8..e184d0c 100644 > --- a/include/configs/sunxi-common.h > +++ b/include/configs/sunxi-common.h > @@ -189,14 +189,14 @@ > #define CONFIG_SPL_BOARD_LOAD_IMAGE > > #if defined(CONFIG_MACH_SUN9I) > -#define CONFIG_SPL_TEXT_BASE 0x10040 /* sram start+header */ > -#define CONFIG_SPL_MAX_SIZE 0x5fc0 /* ? KiB on sun9i */ > +#define CONFIG_SPL_TEXT_BASE 0x10060 /* sram start+header */ > +#define CONFIG_SPL_MAX_SIZE 0x5fa0 /* ? KiB on sun9i */ > #elif defined(CONFIG_MACH_SUN50I) > -#define CONFIG_SPL_TEXT_BASE 0x10040 /* sram start+header */ > -#define CONFIG_SPL_MAX_SIZE 0x7fc0 /* 32 KiB on sun50i */ > +#define CONFIG_SPL_TEXT_BASE 0x10060 /* sram start+header */ > +#define CONFIG_SPL_MAX_SIZE 0x7fa0 /* 32 KiB on sun50i */ > #else > -#define CONFIG_SPL_TEXT_BASE 0x40 /* sram start+header */ > -#define CONFIG_SPL_MAX_SIZE 0x5fc0 /* 24KB on sun4i/sun7i */ > +#define CONFIG_SPL_TEXT_BASE 0x60 /* sram start+header */ > +#define CONFIG_SPL_MAX_SIZE 0x5fa0 /* 24KB on sun4i/sun7i */ > #endif > > #define CONFIG_SPL_LIBDISK_SUPPORT > diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl > index 6d2017d..54d8a2e 100644 > --- a/scripts/Makefile.spl > +++ b/scripts/Makefile.spl > @@ -240,7 +240,8 @@ $(obj)/$(SPL_BIN).sfp: $(obj)/$(SPL_BIN).bin FORCE > $(call if_changed,mkimage) > > quiet_cmd_mksunxiboot = MKSUNXI $@ > -cmd_mksunxiboot = $(objtree)/tools/mksunxiboot $< $@ > +cmd_mksunxiboot = $(objtree)/tools/mksunxiboot --default-dt "$(CONFIG_DEFAULT_DEVICE_TREE)" \ > + --compatible-dt-list "$(CONFIG_OF_LIST)" $< $@ > $(obj)/sunxi-spl.bin: $(obj)/$(SPL_BIN).bin FORCE > $(call if_changed,mksunxiboot) > > diff --git a/tools/Makefile b/tools/Makefile > index 63355aa..e6c0481 100644 > --- a/tools/Makefile > +++ b/tools/Makefile > @@ -155,6 +155,7 @@ hostprogs-$(CONFIG_MX28) += mxsboot > HOSTCFLAGS_mxsboot.o := -pedantic > > hostprogs-$(CONFIG_SUNXI) += mksunxiboot > +mksunxiboot-objs := mksunxiboot.o lib/crc32.o > > hostprogs-$(CONFIG_NETCONSOLE) += ncb > hostprogs-$(CONFIG_SHA1_CHECK_UB_IMG) += ubsha1 > diff --git a/tools/mksunxiboot.c b/tools/mksunxiboot.c > index 9c1c5b7..b45acec 100644 > --- a/tools/mksunxiboot.c > +++ b/tools/mksunxiboot.c > @@ -17,6 +17,8 @@ > #include > #include "asm/arch/spl.h" > > +#include > + > #define STAMP_VALUE 0x5F0A6C39 > > /* check sum functon from sun4i boot code */ > @@ -70,11 +72,55 @@ int main(int argc, char *argv[]) > struct boot_img img; > unsigned file_size; > int count; > + char *tool_name = argv[0]; > + char *default_dt = NULL; > + char *compatible_dt_list = NULL; > + > + /* a sanity check */ > + if ((sizeof(img.header) % 32) != 0) { > + fprintf(stderr, "ERROR: the SPL header must be a multiple "); > + fprintf(stderr, "of 32 bytes.\n"); Nitpick / coding style: You're using multiple fprintf(stderr, ...) where a simple string concatenation would do. Same applies for some printf() further down. Not a real concern though. > + return EXIT_FAILURE; > + } > + > + /* process optional command line switches */ > + while (argc >= 2 && argv[1][0] == '-') { > + if (strcmp(argv[1], "--default-dt") == 0) { > + if (argc >= 3) { > + default_dt = argv[2]; > + argv += 2; > + argc -= 2; > + continue; > + } > + fprintf(stderr, "ERROR: no --default-dt arg\n"); > + return EXIT_FAILURE; > + } else if (strcmp(argv[1], "--compatible-dt-list") == 0) { > + if (argc >= 3) { > + compatible_dt_list = argv[2]; > + argv += 2; > + argc -= 2; > + continue; > + } > + fprintf(stderr, "ERROR: no --compatible-dt-list arg\n"); > + return EXIT_FAILURE; > + } else { > + fprintf(stderr, "ERROR: bad option '%s'\n", argv[1]); > + return EXIT_FAILURE; > + } > + } > > - if (argc < 2) { > - printf("\tThis program makes an input bin file to sun4i " \ > - "bootable image.\n" \ > - "\tUsage: %s input_file out_putfile\n", argv[0]); > + if (!compatible_dt_list || strlen(compatible_dt_list) == 0) > + compatible_dt_list = default_dt; > + > + if (argc < 3) { > + printf("This program converts an input binary file to a sunxi bootable image.\n"); > + printf("\nUsage: %s [options] input_file output_file\n", > + tool_name); > + printf("Where [options] may be:\n"); > + printf(" --default-dt arg - 'arg' is the default device tree name\n"); > + printf(" (CONFIG_DEFAULT_DEVICE_TREE).\n"); > + printf(" --compatible-dt-list arg - 'arg' is the space separated list of the\n"); > + printf(" compatible device tree names (CONFIG_OF_LIST).\n"); > return EXIT_FAILURE; > } > > @@ -122,6 +168,23 @@ int main(int argc, char *argv[]) > memcpy(img.header.spl_signature, SPL_SIGNATURE, 3); /* "sunxi" marker */ > img.header.spl_signature[3] = SPL_HEADER_VERSION; > > + if (default_dt) { > + if (strlen(default_dt) + 1 <= sizeof(img.header.string_pool)) { > + strcpy((char *)img.header.string_pool, default_dt); (The cast would be obsolete with a "char stringpool[];" type.) > + img.header.offset_of_the_default_device_tree_name = > + cpu_to_le32(offsetof(struct boot_file_head, > + string_pool)); > + } else { > + printf("WARNING: The SPL header is too small\n"); > + printf(" and has no space to store the dt name.\n"); > + } > + } > + if (compatible_dt_list) { > + img.header.crc32_of_the_compatible_device_tree_list = > + cpu_to_le32(crc32(0, (uint8_t *)compatible_dt_list, > + strlen(compatible_dt_list))); > + } > + > gen_check_sum(&img.header); > > count = write(fd_out, &img, le32_to_cpu(img.header.length)); Regards, B. Nortmann