Re: [RFC PATCH v2 9/9] tools: spkgimage: add Renesas SPKG format

Sean Anderson
2022-08-13T14:47:38Z

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