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

[Ralph Siemsen]

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 at 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