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

[Ralph Siemsen]

On Sat, Aug 13, 2022 at 10:47 AM Sean Anderson <seanga2 at 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