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

[Simon Glass <sjg@chromium.org>]

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