| Message ID | 20220812170325.485707-1-ralph.siemsen@linaro.org |
|---|---|
| State | RFC |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | None | expand |
On 8/12/22 1:03 PM, Ralph Siemsen wrote: > Renesas RZ/N1 devices contain BootROM code that loads a custom SPKG > image from QSPI, NAND or USB DFU. Support this format in mkimage tool. > > SPKGs can optionally be signed, however creation of signed SPKG is not > currently supported. > > Example of how to use it: > > tools/mkimage -n board/schneider/lces/spkgimage.cfg \ > -T spkgimage -a 0x20040000 -e 0x20040000 \ > -d u-boot.bin u-boot.bin.spkg > > The config file (spkgimage.cfg in this example) contains additional > parameters such as NAND ECC settings. > > Signed-off-by: Ralph Siemsen <ralph.siemsen@linaro.org> > --- > > Changes in v2: > - rewrote the stand-alone spkg_utility to integrate into mkimage > > board/schneider/lces/spkgimage.cfg | 26 +++ > boot/image.c | 1 + > include/image.h | 1 + > tools/Makefile | 1 + > tools/spkgimage.c | 303 +++++++++++++++++++++++++++++ > tools/spkgimage.h | 39 ++++ Please document your format in doc/mkimage.1 > 6 files changed, 371 insertions(+) > create mode 100644 board/schneider/lces/spkgimage.cfg > create mode 100644 tools/spkgimage.c > create mode 100644 tools/spkgimage.h > > diff --git a/board/schneider/lces/spkgimage.cfg b/board/schneider/lces/spkgimage.cfg > new file mode 100644 > index 0000000000..b5faf96b00 > --- /dev/null > +++ b/board/schneider/lces/spkgimage.cfg > @@ -0,0 +1,26 @@ > +# SPDX-License-Identifier: GPL-2.0+ > +# > +# (C) Copyright 2022 Schneider Electric > +# > +# SPKG image header, for booting on RZ/N1 > + > +# b[35:32] SPKG version > +VERSION 1 > + > +# b[42:41] ECC Block size: 0=256 bytes, 1=512 bytes, 2=1024 bytes > +NAND_ECC_BLOCK_SIZE 1 > + > +# b[45] NAND enable (boolean) > +NAND_ECC_ENABLE 1 > + > +# b[50:48] ECC Scheme: 0=BCH2 1=BCH4 2=BCH8 3=BCH16 4=BCH24 5=BCH32 > +NAND_ECC_SCHEME 3 > + > +# b[63:56] ECC bytes per block > +NAND_BYTES_PER_ECC_BLOCK 28 > + > +# Provide dummy BLp header (boolean) > +ADD_DUMMY_BLP 1 > + > +# Pad the image to a multiple of > +PADDING 64K > diff --git a/boot/image.c b/boot/image.c > index 5dcb55ba46..7d50d708ab 100644 > --- a/boot/image.c > +++ b/boot/image.c > @@ -179,6 +179,7 @@ static const table_entry_t uimage_type[] = { > { IH_TYPE_COPRO, "copro", "Coprocessor Image"}, > { IH_TYPE_SUNXI_EGON, "sunxi_egon", "Allwinner eGON Boot Image" }, > { IH_TYPE_SUNXI_TOC0, "sunxi_toc0", "Allwinner TOC0 Boot Image" }, > + { IH_TYPE_RENESAS_SPKG, "spkgimage", "Renesas SPKG Image" }, > { -1, "", "", }, > }; > > diff --git a/include/image.h b/include/image.h > index e4c6a50b88..98eaa384f4 100644 > --- a/include/image.h > +++ b/include/image.h > @@ -229,6 +229,7 @@ enum { > IH_TYPE_COPRO, /* Coprocessor Image for remoteproc*/ > IH_TYPE_SUNXI_EGON, /* Allwinner eGON Boot Image */ > IH_TYPE_SUNXI_TOC0, /* Allwinner TOC0 Boot Image */ > + IH_TYPE_RENESAS_SPKG, /* Renesas SPKG image */ > > IH_TYPE_COUNT, /* Number of image types */ > }; > diff --git a/tools/Makefile b/tools/Makefile > index 005e7362a3..7e24f3ecb9 100644 > --- a/tools/Makefile > +++ b/tools/Makefile > @@ -131,6 +131,7 @@ dumpimage-mkimage-objs := aisimage.o \ > stm32image.o \ > $(ROCKCHIP_OBS) \ > socfpgaimage.o \ > + spkgimage.o \ > sunxi_egon.o \ > lib/crc16-ccitt.o \ > lib/hash-checksum.o \ > diff --git a/tools/spkgimage.c b/tools/spkgimage.c > new file mode 100644 > index 0000000000..2e8c17d94a > --- /dev/null > +++ b/tools/spkgimage.c > @@ -0,0 +1,303 @@ > +// SPDX-License-Identifier: BSD-2-Clause > +/* > + * Generate Renesas RZ/N1 BootROM header (SPKG) > + * (C) Copyright 2022 Schneider Electric > + * > + * Based on spkg_utility.c > + * (C) Copyright 2016 Renesas Electronics Europe Ltd > + */ > + > +#include "imagetool.h" > +#include <limits.h> > +#include <image.h> > +#include <stdarg.h> > +#include <stdint.h> > +#include <u-boot/crc.h> > +#include "spkgimage.h" > + > +static struct spkg_file out_buf; > + > +static uint32_t padding; > + > +/* Note: the ordering of the bitfields does not matter */ > +static struct config_file { > + unsigned int version:1; > + unsigned int ecc_block_size:2; > + unsigned int ecc_enable:1; > + unsigned int ecc_scheme:3; > + unsigned int ecc_bytes:8; > + unsigned int blp_len; > + unsigned int padding; > +} conf; I wonder if you could just fill in the header directly. This is for a userspace tool, and this struct will be created at most once. It's OK to use 10 bytes :) > +static int spkgimage_parse_config_line(char *line) > +{ > + char *saveptr; > + char *delim = "\t "; > + char *name = strtok_r(line, delim, &saveptr); > + char *val_str = strtok_r(NULL, delim, &saveptr); > + int value = atoi(val_str); > + > + if (!strcmp("VERSION", name)) { > + conf.version = value; > + } else if (!strcmp("NAND_ECC_ENABLE", name)) { > + conf.ecc_enable = value; Can you add some checks for the valid range of values? E.g. NAND_ECC_SCHEME should be 0 <= value <= 5 > + } else if (!strcmp("NAND_ECC_BLOCK_SIZE", name)) { > + conf.ecc_block_size = value; > + } else if (!strcmp("NAND_ECC_SCHEME", name)) { > + conf.ecc_scheme = value; > + } else if (!strcmp("NAND_BYTES_PER_ECC_BLOCK", name)) { > + conf.ecc_bytes = value; > + } else if (!strcmp("ADD_DUMMY_BLP", name)) { > + conf.blp_len = value ? SPKG_BLP_SIZE : 0; > + } else if (!strcmp("PADDING", name)) { > + if (strrchr(val_str, 'K')) > + conf.padding = value * 1024; > + else if (strrchr(val_str, 'M')) > + conf.padding = value * 1024 * 1024; > + else > + conf.padding = value; > + } else { > + fprintf(stderr, "Error: unknown keyword '%s' in config\n", > + name); perhaps print the line number? > + return -EINVAL; > + } > + > + return 0; > +} > + > +static int spkgimage_parse_config_file(char *filename) > +{ > + FILE *fcfg; > + char line[256]; > + size_t len; > + > + fcfg = fopen(filename, "r"); > + if (!fcfg) > + return -EINVAL; > + > + while (fgets(line, sizeof(line), fcfg)) { > + /* Skip blank lines and comments */ > + if (line[0] == '\n' || line[0] == '#') > + continue; > + > + /* Strip the trailing newline */ > + len = strlen(line); > + if (line[len - 1] == '\n') > + line[--len] = 0; > + > + /* Parse the line */ > + if (spkgimage_parse_config_line(line)) > + return -EINVAL; > + } > + > + fclose(fcfg); > + > + /* Avoid divide-by-zero later on */ > + if (conf.padding == 0) if (!conf.padding) > + conf.padding = 1; > + > + return 0; > +} > + > +static int spkgimage_check_params(struct image_tool_params *params) > +{ > + if (!params->addr) { > + fprintf(stderr, "Error: Load Address must be set.\n"); > + return -EINVAL; > + } > + > + if (!params->imagename || !params->imagename[0]) { > + fprintf(stderr, "Error: Image name must be set.\n"); > + return -EINVAL; > + } > + > + if (!params->datafile) { > + fprintf(stderr, "Error: Data filename must be set.\n"); > + return -EINVAL; > + } > + > + return 0; > +} > + > +static int spkgimage_verify_header(unsigned char *ptr, int size, > + struct image_tool_params *param) > +{ > + struct spkg_file *file = (struct spkg_file *)ptr; > + struct spkg_hdr *header = (struct spkg_hdr *)ptr; > + char signature[4] = SPKG_HEADER_SIGNATURE; If this naming does not come from documentation, I would suggest something like SPKG_HEADER_MAGIC, since this is not a signature, or even a CRC. > + uint32_t payload_length; > + uint32_t crc; > + uint8_t *crc_buf; > + > + /* Check the signature */ > + if (memcmp(header->signature, signature, 4)) { > + fprintf(stderr, "Error: invalid signature bytes\n"); > + return -EINVAL; > + } > + > + /* Check the CRC */ > + crc = crc32(0, ptr, SPKG_HEADER_SIZE - SPKG_CRC_SIZE); > + if (crc != header->crc) { > + fprintf(stderr, "Error: invalid header CRC=\n"); > + return -EINVAL; > + } > + > + /* Check all copies of header are the same */ > + for (int i = 1; i < SPKG_HEADER_COUNT; i++) { > + if (memcmp(&header[0], &header[i], SPKG_HEADER_SIZE)) { > + fprintf(stderr, "Error: header %d mismatch\n", i); > + return -EINVAL; > + } > + } > + > + /* Check the payload CRC */ > + payload_length = le32_to_cpu(header->payload_length) >> 8; > + crc_buf = file->payload + payload_length - SPKG_CRC_SIZE; > + crc = crc32(0, file->payload, payload_length - SPKG_CRC_SIZE); > + if (crc_buf[0] != (crc & 0xff) || > + crc_buf[1] != (crc >> 8 & 0xff) || > + crc_buf[2] != (crc >> 16 & 0xff) || > + crc_buf[3] != (crc >> 24 & 0xff)) { > + fprintf(stderr, "Error: invalid payload CRC\n"); > + return -EINVAL; > + } > + > + return 0; > +} > + > +static void spkgimage_print_header(const void *ptr) > +{ > + const struct spkg_hdr *h = ptr; > + uint32_t offset = le32_to_cpu(h->execution_offset); > + > + printf("Image type\t: Renesas SPKG Image\n"); > + printf("Marker\t\t: %c%c%c%c\n", h->signature[0], h->signature[1], > + h->signature[2], h->signature[3]); > + printf("Version\t\t: %d\n", h->version); > + printf("ECC\t\t: "); > + if (h->ecc & 0x20) > + printf("Scheme %d, Block size %d, Strength %d\n", > + h->ecc_scheme, (h->ecc >> 1) & 3, h->ecc_bytes); > + else > + printf("Not enabled\n"); > + printf("Payload length\t: %d\n", le32_to_cpu(h->payload_length) >> 8); > + printf("Load address\t: 0x%08x\n", le32_to_cpu(h->load_address)); > + printf("Execution offset: 0x%08x (%s mode)\n", offset & ~1, > + offset & 1 ? "THUMB" : "ARM"); > + printf("Header checksum\t: 0x%08x\n", le32_to_cpu(h->crc)); > +} > + > +static inline uint32_t roundup(uint32_t x, uint32_t y) > +{ > + return ((x + y - 1) / y) * y; > +} > + > +static int spkgimage_vrec_header(struct image_tool_params *params, > + struct image_type_params *tparams) > +{ > + struct stat s; > + > + /* Parse the config file */ > + if (spkgimage_parse_config_file(params->imagename)) { > + fprintf(stderr, "Error parsing config file\n"); > + exit(EXIT_FAILURE); > + } > + > + /* Get size of input data file */ > + if (stat(params->datafile, &s)) { > + fprintf(stderr, "Could not stat data file: %s: %s\n", > + params->datafile, strerror(errno)); > + exit(EXIT_FAILURE); > + } > + params->orig_file_size = s.st_size; > + > + /* Determine size of resulting SPKG file */ > + uint32_t header_len = SPKG_HEADER_SIZE * SPKG_HEADER_COUNT; > + uint32_t payload_len = conf.blp_len + s.st_size + SPKG_CRC_SIZE; > + uint32_t total_len = header_len + payload_len; > + > + /* Round up to next multiple of padding size */ > + uint32_t padded_len = roundup(total_len, conf.padding); > + > + /* Number of padding bytes to add */ > + padding = padded_len - total_len; > + > + /* Fixup payload_len to include padding bytes */ > + payload_len += padding; > + > + /* Prepare the header */ > + struct spkg_hdr header = { > + .signature = SPKG_HEADER_SIGNATURE, > + .version = conf.version, > + .ecc = (conf.ecc_enable << 5) | (conf.ecc_block_size << 1), > + .ecc_scheme = conf.ecc_scheme, > + .ecc_bytes = conf.ecc_bytes, > + .payload_length = cpu_to_le32(payload_len << 8), > + .load_address = cpu_to_le32(params->addr), > + .execution_offset = cpu_to_le32(params->ep - params->addr), > + }; > + header.crc = crc32(0, (uint8_t *)&header, > + sizeof(header) - SPKG_CRC_SIZE); > + > + /* Fill the SPKG with the headers */ > + for (int i = 0; i < SPKG_HEADER_COUNT; i++) > + memcpy(&out_buf.header[i], &header, sizeof(header)); > + > + /* Extra bytes to allocate in the output file */ > + return conf.blp_len + padding + 4; > +} > + > +static void spkgimage_set_header(void *ptr, struct stat *sbuf, int ifd, > + struct image_tool_params *params) > +{ > + uint8_t *payload = ptr + SPKG_HEADER_SIZE * SPKG_HEADER_COUNT; > + uint8_t *file_end = payload + conf.blp_len + params->orig_file_size; > + uint8_t *crc_buf = file_end + padding; > + uint32_t crc; > + > + /* Make room for the Dummy BLp header */ > + memmove(payload + conf.blp_len, payload, params->orig_file_size); > + > + /* Fill the SPKG with the Dummy BLp */ > + memset(payload, 0x88, conf.blp_len); > + > + /* > + * mkimage copy_file() pads the input file with zeros. > + * Replace those zeros with flash friendly one bits. > + * The original version skipped the fist 4 bytes, nit: first > + * probably an oversight, but for consistency we > + * keep the same behaviour. > + */ > + memset(file_end + 4, 0xff, padding - 4); > + > + /* Add Payload CRC */ > + crc = crc32(0, payload, crc_buf - payload); > + crc_buf[0] = crc; > + crc_buf[1] = crc >> 8; > + crc_buf[2] = crc >> 16; > + crc_buf[3] = crc >> 24; > +} > + > +static int spkgimage_check_image_types(uint8_t type) > +{ > + return type == IH_TYPE_RENESAS_SPKG ? 0 : 1; This function is not necessary if you only support one type. > +} > + > +/* > + * spkgimage type parameter definition > + */ > +U_BOOT_IMAGE_TYPE( > + spkgimage, > + "Renesas SPKG Image", > + sizeof(out_buf), > + &out_buf, > + spkgimage_check_params, > + spkgimage_verify_header, > + spkgimage_print_header, > + spkgimage_set_header, > + NULL, > + spkgimage_check_image_types, > + NULL, > + spkgimage_vrec_header > +); > diff --git a/tools/spkgimage.h b/tools/spkgimage.h > new file mode 100644 > index 0000000000..db153bfc3f > --- /dev/null > +++ b/tools/spkgimage.h > @@ -0,0 +1,39 @@ > +/* SPDX-License-Identifier: BSD-2-Clause */ > +/* > + * Renesas RZ/N1 Package Table format > + * (C) 2015-2016 Renesas Electronics Europe, LTD > + * All rights reserved. > + * > + * Converted to mkimage plug-in > + * (C) Copyright 2022 Schneider Electric > + */ > + > +#ifndef _SPKGIMAGE_H_ > +#define _SPKGIMAGE_H_ > + > +#define SPKG_HEADER_SIGNATURE {'R', 'Z', 'N', '1'} > +#define SPKG_HEADER_SIZE 24 > +#define SPKG_HEADER_COUNT 8 What are the other 7 headers for? Should you print them out above? > +#define SPKG_BLP_SIZE 264 > +#define SPKG_CRC_SIZE 4 > + > +/* SPKG header */ > +struct spkg_hdr { > + uint8_t signature[4]; > + uint8_t version; > + uint8_t ecc; > + uint8_t ecc_scheme; > + uint8_t ecc_bytes; > + uint32_t payload_length; /* only HIGHER 24 bits */ > + uint32_t load_address; > + uint32_t execution_offset; > + uint32_t crc; /* of this header */ > +} __packed; > + > +struct spkg_file { > + struct spkg_hdr header[SPKG_HEADER_COUNT]; > + uint8_t payload[0]; > + /* then the CRC */ > +} __packed; > + > +#endif > --Sean
On Sat, Aug 13, 2022 at 10:47 AM Sean Anderson <seanga2@gmail.com> wrote: > > > > board/schneider/lces/spkgimage.cfg | 26 +++ > > boot/image.c | 1 + > > include/image.h | 1 + > > tools/Makefile | 1 + > > tools/spkgimage.c | 303 +++++++++++++++++++++++++++++ > > tools/spkgimage.h | 39 ++++ > > Please document your format in doc/mkimage.1 Okay, will be added in the next version. > > +/* Note: the ordering of the bitfields does not matter */ > > +static struct config_file { > > + unsigned int version:1; > > + unsigned int ecc_block_size:2; > > + unsigned int ecc_enable:1; > > + unsigned int ecc_scheme:3; > > + unsigned int ecc_bytes:8; > > + unsigned int blp_len; > > + unsigned int padding; > > +} conf; > > I wonder if you could just fill in the header directly. This is > for a userspace tool, and this struct will be created at most > once. It's OK to use 10 bytes :) I could fill the header directly, but I figured it would be cleaner to keep the config file parsing separate from header generation. As for the use of bitfields, this was not really about saving space, but rather a cheap way to ensure the values from the config file do not exceed their allocated bit-width in the header. The previous stand-alone code has a somewhat similar construct, from which I pinched the idea. > > > +static int spkgimage_parse_config_line(char *line) > > +{ > > + char *saveptr; > > + char *delim = "\t "; > > + char *name = strtok_r(line, delim, &saveptr); > > + char *val_str = strtok_r(NULL, delim, &saveptr); > > + int value = atoi(val_str); > > + > > + if (!strcmp("VERSION", name)) { > > + conf.version = value; > > + } else if (!strcmp("NAND_ECC_ENABLE", name)) { > > + conf.ecc_enable = value; > > Can you add some checks for the valid range of values? E.g. > NAND_ECC_SCHEME should be 0 <= value <= 5 Yes, will add some range checks where they make sense. Probably with a helper function. > > + } else if (!strcmp("NAND_ECC_BLOCK_SIZE", name)) { > > + conf.ecc_block_size = value; > > + } else if (!strcmp("NAND_ECC_SCHEME", name)) { > > + conf.ecc_scheme = value; > > + } else if (!strcmp("NAND_BYTES_PER_ECC_BLOCK", name)) { > > + conf.ecc_bytes = value; > > + } else if (!strcmp("ADD_DUMMY_BLP", name)) { > > + conf.blp_len = value ? SPKG_BLP_SIZE : 0; > > + } else if (!strcmp("PADDING", name)) { > > + if (strrchr(val_str, 'K')) > > + conf.padding = value * 1024; > > + else if (strrchr(val_str, 'M')) > > + conf.padding = value * 1024 * 1024; > > + else > > + conf.padding = value; > > + } else { > > + fprintf(stderr, "Error: unknown keyword '%s' in config\n", > > + name); > > perhaps print the line number? Good idea, will do. > > + /* Avoid divide-by-zero later on */ > > + if (conf.padding == 0) > > if (!conf.padding) Will do. > > +static int spkgimage_verify_header(unsigned char *ptr, int size, > > + struct image_tool_params *param) > > +{ > > + struct spkg_file *file = (struct spkg_file *)ptr; > > + struct spkg_hdr *header = (struct spkg_hdr *)ptr; > > + char signature[4] = SPKG_HEADER_SIGNATURE; > > If this naming does not come from documentation, I would suggest > something like SPKG_HEADER_MAGIC, since this is not a signature, > or even a CRC. The name does in fact come from the RZ/N1 documentation. However I agree that SPKG_HEADER_MAGIC would better reflect what these bytes actually are. > > + /* > > + * mkimage copy_file() pads the input file with zeros. > > + * Replace those zeros with flash friendly one bits. > > + * The original version skipped the fist 4 bytes, > > nit: first Well spotted, thanks. > > +static int spkgimage_check_image_types(uint8_t type) > > +{ > > + return type == IH_TYPE_RENESAS_SPKG ? 0 : 1; > > This function is not necessary if you only support one type. Without this function, mkimage kept telling me that my format (spkgimage) was not supported, and none of my callbacks got invoked. It only complained when trying to generate a header. When listing the supported formats, spkgimage showed up correctly. I'll take another look on Monday, maybe I missed something obvious. > > +#define SPKG_HEADER_SIGNATURE {'R', 'Z', 'N', '1'} > > +#define SPKG_HEADER_SIZE 24 > > +#define SPKG_HEADER_COUNT 8 > > What are the other 7 headers for? Should you print them out above? There are 8 identical copies of the 24-byte header. This is meant to help with NAND booting, where the header is read before ECC settings are known. The BootROM validates the header CRC and will try up to 8 headers before giving up. Since they are identical, I only printed values from one header. However in the validation function, I do check that all eight copies match. Thanks for your feedback! -Ralph
Hi Sean, I've implemented most of the suggestions. I will post an updated series, since it seems that sending v2 of just one patch has confused patchwork. However so as not to entirely remove confusion, the updated series will be v3, since I already used v2 for the one patch. :-P On Sat, Aug 13, 2022 at 9:45 PM Ralph Siemsen <ralph.siemsen@linaro.org> wrote: > > > > I wonder if you could just fill in the header directly. This is > > for a userspace tool, and this struct will be created at most > > once. It's OK to use 10 bytes :) > > I could fill the header directly, but I figured it would be cleaner to > keep the config file parsing separate from header generation. Does it seem reasonable to keep these structures separated? > > > +static int spkgimage_verify_header(unsigned char *ptr, int size, > > > + struct image_tool_params *param) > > > +{ > > > + struct spkg_file *file = (struct spkg_file *)ptr; > > > + struct spkg_hdr *header = (struct spkg_hdr *)ptr; > > > + char signature[4] = SPKG_HEADER_SIGNATURE; > > > > If this naming does not come from documentation, I would suggest > > something like SPKG_HEADER_MAGIC, since this is not a signature, > > or even a CRC. > > The name does in fact come from the RZ/N1 documentation. However I > agree that SPKG_HEADER_MAGIC would better reflect what these bytes > actually are. Upon checking the documentation, it turns out they use the term "marker" rather than "signature" for these bytes. So I have switched the code to match. > > > +static int spkgimage_check_image_types(uint8_t type) > > > +{ > > > + return type == IH_TYPE_RENESAS_SPKG ? 0 : 1; > > > > This function is not necessary if you only support one type. > > Without this function, mkimage kept telling me that my format > (spkgimage) was not supported, and none of my callbacks got invoked. > It only complained when trying to generate a header. When listing the > supported formats, spkgimage showed up correctly. > > I'll take another look on Monday, maybe I missed something obvious. I have re-checked this: - without the function, mkimage complains that spkgimage is unknown - with a function that unconditionally returns 0, it works fine If it really is meant to work without the function, then a bug must have crept in elsewhere... Regards, -Ralph
On 8/16/22 10:33 AM, Ralph Siemsen wrote: > Hi Sean, > > I've implemented most of the suggestions. I will post an updated > series, since it seems that sending v2 of just one patch has confused > patchwork. > > However so as not to entirely remove confusion, the updated series > will be v3, since I already used v2 for the one patch. :-P > > On Sat, Aug 13, 2022 at 9:45 PM Ralph Siemsen <ralph.siemsen@linaro.org> wrote: >>> >>> I wonder if you could just fill in the header directly. This is >>> for a userspace tool, and this struct will be created at most >>> once. It's OK to use 10 bytes :) >> >> I could fill the header directly, but I figured it would be cleaner to >> keep the config file parsing separate from header generation. > > Does it seem reasonable to keep these structures separated? Yes, that is fine. >>>> +static int spkgimage_verify_header(unsigned char *ptr, int size, >>>> + struct image_tool_params *param) >>>> +{ >>>> + struct spkg_file *file = (struct spkg_file *)ptr; >>>> + struct spkg_hdr *header = (struct spkg_hdr *)ptr; >>>> + char signature[4] = SPKG_HEADER_SIGNATURE; >>> >>> If this naming does not come from documentation, I would suggest >>> something like SPKG_HEADER_MAGIC, since this is not a signature, >>> or even a CRC. >> >> The name does in fact come from the RZ/N1 documentation. However I >> agree that SPKG_HEADER_MAGIC would better reflect what these bytes >> actually are. > > Upon checking the documentation, it turns out they use the term > "marker" rather than "signature" for these bytes. So I have switched > the code to match. That sounds good. >>>> +static int spkgimage_check_image_types(uint8_t type) >>>> +{ >>>> + return type == IH_TYPE_RENESAS_SPKG ? 0 : 1; >>> >>> This function is not necessary if you only support one type. >> >> Without this function, mkimage kept telling me that my format >> (spkgimage) was not supported, and none of my callbacks got invoked. >> It only complained when trying to generate a header. When listing the >> supported formats, spkgimage showed up correctly. >> >> I'll take another look on Monday, maybe I missed something obvious. > > I have re-checked this: > - without the function, mkimage complains that spkgimage is unknown > - with a function that unconditionally returns 0, it works fine > > If it really is meant to work without the function, then a bug must > have crept in elsewhere... Huh. I did a quick grep so maybe I missed something. IMO this *should* work without a function, because we have tons of drivers which just have an equality check. In any case, you can just do return type == IH_TYPE_RENESAS_SPKG ? 0 : -EINVAL; --Sean
On Mon, Aug 22, 2022 at 11:42:54PM -0400, Sean Anderson wrote: >>>>>+static int spkgimage_check_image_types(uint8_t type) >>>>>+{ >>>>>+ return type == IH_TYPE_RENESAS_SPKG ? 0 : 1; >>>> >>>>This function is not necessary if you only support one type. >>> >>>Without this function, mkimage kept telling me that my format >>>(spkgimage) was not supported, and none of my callbacks got invoked. >>>It only complained when trying to generate a header. When listing the >>>supported formats, spkgimage showed up correctly. >>> >>>I'll take another look on Monday, maybe I missed something obvious. >> >>I have re-checked this: >>- without the function, mkimage complains that spkgimage is unknown >>- with a function that unconditionally returns 0, it works fine >> >>If it really is meant to work without the function, then a bug must >>have crept in elsewhere... > >Huh. I did a quick grep so maybe I missed something. IMO this *should* >work without a function, because we have tons of drivers which just >have an equality check. In any case, you can just do > >return type == IH_TYPE_RENESAS_SPKG ? 0 : -EINVAL; It works fine when I use the following for the function: static int spkgimage_check_image_types(uint8_t type) { return 0; } However if no function is provided, i.e. U_BOOT_IMAGE_TYPE has NULL for check_image_type field, then mkimage fails with the error: tools/mkimage: unsupported type Renesas SPKG Image Looking at this a bit more, it seems to be due to: struct image_type_params *imagetool_get_type(int type) { ...snip... for (curr = start; curr != end; curr++) { if ((*curr)->check_image_type) { if (!(*curr)->check_image_type(type)) return *curr; } } return NULL; } So the only way to get non-NULL from imagetool_get_type is for there to be a callback function, and it must return zero. And this in turn causes mkimage to bail out quite early in main(): /* set tparams as per input type_id */ tparams = imagetool_get_type(params.type); if (tparams == NULL && !params.lflag) { fprintf (stderr, "%s: unsupported type %s\n", params.cmdname, genimg_get_type_name(params.type)); exit (EXIT_FAILURE); } Unless I am missing something, it seems I must provide a function. -Ralph PS I will post an updated series (v3) eventually. I'm working on making changes to the clock driver on the kernel side, to keep it in sync with the changes you requested in the u-boot side.
diff --git a/board/schneider/lces/spkgimage.cfg b/board/schneider/lces/spkgimage.cfg new file mode 100644 index 0000000000..b5faf96b00 --- /dev/null +++ b/board/schneider/lces/spkgimage.cfg @@ -0,0 +1,26 @@ +# SPDX-License-Identifier: GPL-2.0+ +# +# (C) Copyright 2022 Schneider Electric +# +# SPKG image header, for booting on RZ/N1 + +# b[35:32] SPKG version +VERSION 1 + +# b[42:41] ECC Block size: 0=256 bytes, 1=512 bytes, 2=1024 bytes +NAND_ECC_BLOCK_SIZE 1 + +# b[45] NAND enable (boolean) +NAND_ECC_ENABLE 1 + +# b[50:48] ECC Scheme: 0=BCH2 1=BCH4 2=BCH8 3=BCH16 4=BCH24 5=BCH32 +NAND_ECC_SCHEME 3 + +# b[63:56] ECC bytes per block +NAND_BYTES_PER_ECC_BLOCK 28 + +# Provide dummy BLp header (boolean) +ADD_DUMMY_BLP 1 + +# Pad the image to a multiple of +PADDING 64K diff --git a/boot/image.c b/boot/image.c index 5dcb55ba46..7d50d708ab 100644 --- a/boot/image.c +++ b/boot/image.c @@ -179,6 +179,7 @@ static const table_entry_t uimage_type[] = { { IH_TYPE_COPRO, "copro", "Coprocessor Image"}, { IH_TYPE_SUNXI_EGON, "sunxi_egon", "Allwinner eGON Boot Image" }, { IH_TYPE_SUNXI_TOC0, "sunxi_toc0", "Allwinner TOC0 Boot Image" }, + { IH_TYPE_RENESAS_SPKG, "spkgimage", "Renesas SPKG Image" }, { -1, "", "", }, }; diff --git a/include/image.h b/include/image.h index e4c6a50b88..98eaa384f4 100644 --- a/include/image.h +++ b/include/image.h @@ -229,6 +229,7 @@ enum { IH_TYPE_COPRO, /* Coprocessor Image for remoteproc*/ IH_TYPE_SUNXI_EGON, /* Allwinner eGON Boot Image */ IH_TYPE_SUNXI_TOC0, /* Allwinner TOC0 Boot Image */ + IH_TYPE_RENESAS_SPKG, /* Renesas SPKG image */ IH_TYPE_COUNT, /* Number of image types */ }; diff --git a/tools/Makefile b/tools/Makefile index 005e7362a3..7e24f3ecb9 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -131,6 +131,7 @@ dumpimage-mkimage-objs := aisimage.o \ stm32image.o \ $(ROCKCHIP_OBS) \ socfpgaimage.o \ + spkgimage.o \ sunxi_egon.o \ lib/crc16-ccitt.o \ lib/hash-checksum.o \ diff --git a/tools/spkgimage.c b/tools/spkgimage.c new file mode 100644 index 0000000000..2e8c17d94a --- /dev/null +++ b/tools/spkgimage.c @@ -0,0 +1,303 @@ +// SPDX-License-Identifier: BSD-2-Clause +/* + * Generate Renesas RZ/N1 BootROM header (SPKG) + * (C) Copyright 2022 Schneider Electric + * + * Based on spkg_utility.c + * (C) Copyright 2016 Renesas Electronics Europe Ltd + */ + +#include "imagetool.h" +#include <limits.h> +#include <image.h> +#include <stdarg.h> +#include <stdint.h> +#include <u-boot/crc.h> +#include "spkgimage.h" + +static struct spkg_file out_buf; + +static uint32_t padding; + +/* Note: the ordering of the bitfields does not matter */ +static struct config_file { + unsigned int version:1; + unsigned int ecc_block_size:2; + unsigned int ecc_enable:1; + unsigned int ecc_scheme:3; + unsigned int ecc_bytes:8; + unsigned int blp_len; + unsigned int padding; +} conf; + +static int spkgimage_parse_config_line(char *line) +{ + char *saveptr; + char *delim = "\t "; + char *name = strtok_r(line, delim, &saveptr); + char *val_str = strtok_r(NULL, delim, &saveptr); + int value = atoi(val_str); + + if (!strcmp("VERSION", name)) { + conf.version = value; + } else if (!strcmp("NAND_ECC_ENABLE", name)) { + conf.ecc_enable = value; + } else if (!strcmp("NAND_ECC_BLOCK_SIZE", name)) { + conf.ecc_block_size = value; + } else if (!strcmp("NAND_ECC_SCHEME", name)) { + conf.ecc_scheme = value; + } else if (!strcmp("NAND_BYTES_PER_ECC_BLOCK", name)) { + conf.ecc_bytes = value; + } else if (!strcmp("ADD_DUMMY_BLP", name)) { + conf.blp_len = value ? SPKG_BLP_SIZE : 0; + } else if (!strcmp("PADDING", name)) { + if (strrchr(val_str, 'K')) + conf.padding = value * 1024; + else if (strrchr(val_str, 'M')) + conf.padding = value * 1024 * 1024; + else + conf.padding = value; + } else { + fprintf(stderr, "Error: unknown keyword '%s' in config\n", + name); + return -EINVAL; + } + + return 0; +} + +static int spkgimage_parse_config_file(char *filename) +{ + FILE *fcfg; + char line[256]; + size_t len; + + fcfg = fopen(filename, "r"); + if (!fcfg) + return -EINVAL; + + while (fgets(line, sizeof(line), fcfg)) { + /* Skip blank lines and comments */ + if (line[0] == '\n' || line[0] == '#') + continue; + + /* Strip the trailing newline */ + len = strlen(line); + if (line[len - 1] == '\n') + line[--len] = 0; + + /* Parse the line */ + if (spkgimage_parse_config_line(line)) + return -EINVAL; + } + + fclose(fcfg); + + /* Avoid divide-by-zero later on */ + if (conf.padding == 0) + conf.padding = 1; + + return 0; +} + +static int spkgimage_check_params(struct image_tool_params *params) +{ + if (!params->addr) { + fprintf(stderr, "Error: Load Address must be set.\n"); + return -EINVAL; + } + + if (!params->imagename || !params->imagename[0]) { + fprintf(stderr, "Error: Image name must be set.\n"); + return -EINVAL; + } + + if (!params->datafile) { + fprintf(stderr, "Error: Data filename must be set.\n"); + return -EINVAL; + } + + return 0; +} + +static int spkgimage_verify_header(unsigned char *ptr, int size, + struct image_tool_params *param) +{ + struct spkg_file *file = (struct spkg_file *)ptr; + struct spkg_hdr *header = (struct spkg_hdr *)ptr; + char signature[4] = SPKG_HEADER_SIGNATURE; + uint32_t payload_length; + uint32_t crc; + uint8_t *crc_buf; + + /* Check the signature */ + if (memcmp(header->signature, signature, 4)) { + fprintf(stderr, "Error: invalid signature bytes\n"); + return -EINVAL; + } + + /* Check the CRC */ + crc = crc32(0, ptr, SPKG_HEADER_SIZE - SPKG_CRC_SIZE); + if (crc != header->crc) { + fprintf(stderr, "Error: invalid header CRC=\n"); + return -EINVAL; + } + + /* Check all copies of header are the same */ + for (int i = 1; i < SPKG_HEADER_COUNT; i++) { + if (memcmp(&header[0], &header[i], SPKG_HEADER_SIZE)) { + fprintf(stderr, "Error: header %d mismatch\n", i); + return -EINVAL; + } + } + + /* Check the payload CRC */ + payload_length = le32_to_cpu(header->payload_length) >> 8; + crc_buf = file->payload + payload_length - SPKG_CRC_SIZE; + crc = crc32(0, file->payload, payload_length - SPKG_CRC_SIZE); + if (crc_buf[0] != (crc & 0xff) || + crc_buf[1] != (crc >> 8 & 0xff) || + crc_buf[2] != (crc >> 16 & 0xff) || + crc_buf[3] != (crc >> 24 & 0xff)) { + fprintf(stderr, "Error: invalid payload CRC\n"); + return -EINVAL; + } + + return 0; +} + +static void spkgimage_print_header(const void *ptr) +{ + const struct spkg_hdr *h = ptr; + uint32_t offset = le32_to_cpu(h->execution_offset); + + printf("Image type\t: Renesas SPKG Image\n"); + printf("Marker\t\t: %c%c%c%c\n", h->signature[0], h->signature[1], + h->signature[2], h->signature[3]); + printf("Version\t\t: %d\n", h->version); + printf("ECC\t\t: "); + if (h->ecc & 0x20) + printf("Scheme %d, Block size %d, Strength %d\n", + h->ecc_scheme, (h->ecc >> 1) & 3, h->ecc_bytes); + else + printf("Not enabled\n"); + printf("Payload length\t: %d\n", le32_to_cpu(h->payload_length) >> 8); + printf("Load address\t: 0x%08x\n", le32_to_cpu(h->load_address)); + printf("Execution offset: 0x%08x (%s mode)\n", offset & ~1, + offset & 1 ? "THUMB" : "ARM"); + printf("Header checksum\t: 0x%08x\n", le32_to_cpu(h->crc)); +} + +static inline uint32_t roundup(uint32_t x, uint32_t y) +{ + return ((x + y - 1) / y) * y; +} + +static int spkgimage_vrec_header(struct image_tool_params *params, + struct image_type_params *tparams) +{ + struct stat s; + + /* Parse the config file */ + if (spkgimage_parse_config_file(params->imagename)) { + fprintf(stderr, "Error parsing config file\n"); + exit(EXIT_FAILURE); + } + + /* Get size of input data file */ + if (stat(params->datafile, &s)) { + fprintf(stderr, "Could not stat data file: %s: %s\n", + params->datafile, strerror(errno)); + exit(EXIT_FAILURE); + } + params->orig_file_size = s.st_size; + + /* Determine size of resulting SPKG file */ + uint32_t header_len = SPKG_HEADER_SIZE * SPKG_HEADER_COUNT; + uint32_t payload_len = conf.blp_len + s.st_size + SPKG_CRC_SIZE; + uint32_t total_len = header_len + payload_len; + + /* Round up to next multiple of padding size */ + uint32_t padded_len = roundup(total_len, conf.padding); + + /* Number of padding bytes to add */ + padding = padded_len - total_len; + + /* Fixup payload_len to include padding bytes */ + payload_len += padding; + + /* Prepare the header */ + struct spkg_hdr header = { + .signature = SPKG_HEADER_SIGNATURE, + .version = conf.version, + .ecc = (conf.ecc_enable << 5) | (conf.ecc_block_size << 1), + .ecc_scheme = conf.ecc_scheme, + .ecc_bytes = conf.ecc_bytes, + .payload_length = cpu_to_le32(payload_len << 8), + .load_address = cpu_to_le32(params->addr), + .execution_offset = cpu_to_le32(params->ep - params->addr), + }; + header.crc = crc32(0, (uint8_t *)&header, + sizeof(header) - SPKG_CRC_SIZE); + + /* Fill the SPKG with the headers */ + for (int i = 0; i < SPKG_HEADER_COUNT; i++) + memcpy(&out_buf.header[i], &header, sizeof(header)); + + /* Extra bytes to allocate in the output file */ + return conf.blp_len + padding + 4; +} + +static void spkgimage_set_header(void *ptr, struct stat *sbuf, int ifd, + struct image_tool_params *params) +{ + uint8_t *payload = ptr + SPKG_HEADER_SIZE * SPKG_HEADER_COUNT; + uint8_t *file_end = payload + conf.blp_len + params->orig_file_size; + uint8_t *crc_buf = file_end + padding; + uint32_t crc; + + /* Make room for the Dummy BLp header */ + memmove(payload + conf.blp_len, payload, params->orig_file_size); + + /* Fill the SPKG with the Dummy BLp */ + memset(payload, 0x88, conf.blp_len); + + /* + * mkimage copy_file() pads the input file with zeros. + * Replace those zeros with flash friendly one bits. + * The original version skipped the fist 4 bytes, + * probably an oversight, but for consistency we + * keep the same behaviour. + */ + memset(file_end + 4, 0xff, padding - 4); + + /* Add Payload CRC */ + crc = crc32(0, payload, crc_buf - payload); + crc_buf[0] = crc; + crc_buf[1] = crc >> 8; + crc_buf[2] = crc >> 16; + crc_buf[3] = crc >> 24; +} + +static int spkgimage_check_image_types(uint8_t type) +{ + return type == IH_TYPE_RENESAS_SPKG ? 0 : 1; +} + +/* + * spkgimage type parameter definition + */ +U_BOOT_IMAGE_TYPE( + spkgimage, + "Renesas SPKG Image", + sizeof(out_buf), + &out_buf, + spkgimage_check_params, + spkgimage_verify_header, + spkgimage_print_header, + spkgimage_set_header, + NULL, + spkgimage_check_image_types, + NULL, + spkgimage_vrec_header +); diff --git a/tools/spkgimage.h b/tools/spkgimage.h new file mode 100644 index 0000000000..db153bfc3f --- /dev/null +++ b/tools/spkgimage.h @@ -0,0 +1,39 @@ +/* SPDX-License-Identifier: BSD-2-Clause */ +/* + * Renesas RZ/N1 Package Table format + * (C) 2015-2016 Renesas Electronics Europe, LTD + * All rights reserved. + * + * Converted to mkimage plug-in + * (C) Copyright 2022 Schneider Electric + */ + +#ifndef _SPKGIMAGE_H_ +#define _SPKGIMAGE_H_ + +#define SPKG_HEADER_SIGNATURE {'R', 'Z', 'N', '1'} +#define SPKG_HEADER_SIZE 24 +#define SPKG_HEADER_COUNT 8 +#define SPKG_BLP_SIZE 264 +#define SPKG_CRC_SIZE 4 + +/* SPKG header */ +struct spkg_hdr { + uint8_t signature[4]; + uint8_t version; + uint8_t ecc; + uint8_t ecc_scheme; + uint8_t ecc_bytes; + uint32_t payload_length; /* only HIGHER 24 bits */ + uint32_t load_address; + uint32_t execution_offset; + uint32_t crc; /* of this header */ +} __packed; + +struct spkg_file { + struct spkg_hdr header[SPKG_HEADER_COUNT]; + uint8_t payload[0]; + /* then the CRC */ +} __packed; + +#endif
Renesas RZ/N1 devices contain BootROM code that loads a custom SPKG image from QSPI, NAND or USB DFU. Support this format in mkimage tool. SPKGs can optionally be signed, however creation of signed SPKG is not currently supported. Example of how to use it: tools/mkimage -n board/schneider/lces/spkgimage.cfg \ -T spkgimage -a 0x20040000 -e 0x20040000 \ -d u-boot.bin u-boot.bin.spkg The config file (spkgimage.cfg in this example) contains additional parameters such as NAND ECC settings. Signed-off-by: Ralph Siemsen <ralph.siemsen@linaro.org> --- Changes in v2: - rewrote the stand-alone spkg_utility to integrate into mkimage board/schneider/lces/spkgimage.cfg | 26 +++ boot/image.c | 1 + include/image.h | 1 + tools/Makefile | 1 + tools/spkgimage.c | 303 +++++++++++++++++++++++++++++ tools/spkgimage.h | 39 ++++ 6 files changed, 371 insertions(+) create mode 100644 board/schneider/lces/spkgimage.cfg create mode 100644 tools/spkgimage.c create mode 100644 tools/spkgimage.h