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

[Sean Anderson <seanga2@gmail.com>]

On 8/16/22 10:33 AM, Ralph Siemsen wrote:
> 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@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?

Yes, that is fine.

>>>> +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.

That sounds good.

>>>> +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...

Huh. I did a quick grep so maybe I missed something. IMO this *should*
work without a function, because we have tons of drivers which just
have an equality check. In any case, you can just do

return type == IH_TYPE_RENESAS_SPKG ? 0 : -EINVAL;

--Sean