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

Simon Glass
2023-02-22T19:18:35Z

Hi Ralph,

On Wed, 22 Feb 2023 at 08:44, Ralph Siemsen <ralph.siemsen@linaro.org> 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/rzn1-snarc/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 v3:
> - provide definition of __packed (as done in kwbimage.h)
> - explain why a local copy of roundup() is needed
> - document spkgimage in doc/mkimage.1
> - add range checks when parsing config file values
> - add line numbers for reporting errors in config file
> - rename SPKG_HEADER_SIGNATURE to SPKG_HEADER_MARKER
> - fix segfault when image is padded by less than 4 bytes
> - minor style and typo fixes
>
> Changes in v2:
> - rewrote the stand-alone spkg_utility to integrate into mkimage
>
> board/schneider/rzn1-snarc/spkgimage.cfg | 26 ++
> boot/image.c | 1 +
> doc/mkimage.1 | 45 ++++
> include/image.h | 1 +
> tools/Makefile | 1 +
> tools/spkgimage.c | 330 +++++++++++++++++++++++
> tools/spkgimage.h | 45 ++++
> 7 files changed, 449 insertions(+)
> create mode 100644 board/schneider/rzn1-snarc/spkgimage.cfg
> create mode 100644 tools/spkgimage.c
> create mode 100644 tools/spkgimage.h

[..]

Reviewed-by: Simon Glass <sjg@chromium.org>

Can you please add some details to doc/ for this SoC and how it boots,
the use of mkimage, etc.?

Also as a follow-on, can you add a binman entry type for this so that
building a functioning image is automatic?

Finally, please add comments to struct spkg_hdr members below

> diff --git a/tools/spkgimage.h b/tools/spkgimage.h
> new file mode 100644
> index 0000000000..7be127b50f
> --- /dev/null
> +++ b/tools/spkgimage.h
> @@ -0,0 +1,45 @@
> +/* 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_
> +
> +#ifdef __GNUC__
> +#define __packed __attribute((packed))
> +#else
> +#define __packed
> +#endif
> +
> +#define SPKG_HEADER_MARKER {'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 marker[4]; /* aka magic */
> + 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
> --
> 2.25.1
>

Regards,
Simon