[PATCH v2 01/16] board: Define GUIDs for firmware images

[Caleb Connolly]

On 12/04/2024 22:58, Jon Humphreys wrote:
> Ilias Apalodimas <ilias.apalodimas at linaro.org> writes:
> 
>> Hi both
>>
>> On Wed, 10 Apr 2024 at 10:36, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>>>
>>> On 09.04.24 23:05, Jon Humphreys wrote:
>>>> Heinrich Schuchardt <xypron.glpk at gmx.de> writes:
>>>>
>>>>> On 4/9/24 00:31, Jonathan Humphreys wrote:
>>>>>> Define GUIDs for the different firmware images (tiboot3.bin, tispl.bin,
>>>>>> u-boot.img, sysfw). >
>>>>>> Signed-off-by: Jonathan Humphreys <j-humphreys at ti.com>
>>>>>> ---
>>>>>>    include/configs/ti_armv7_common.h | 17 +++++++++++++++++
>>>>>>    1 file changed, 17 insertions(+)
>>>>>>
>>>>>> diff --git a/include/configs/ti_armv7_common.h b/include/configs/ti_armv7_common.h
>>>>>> index 3def7b1027e..4ce14a9b84c 100644
>>>>>> --- a/include/configs/ti_armv7_common.h
>>>>>> +++ b/include/configs/ti_armv7_common.h
>>>>>> @@ -16,6 +16,23 @@
>>>>>>    #ifndef __CONFIG_TI_ARMV7_COMMON_H__
>>>>>>    #define __CONFIG_TI_ARMV7_COMMON_H__
>>>>>>
>>>>>> +/* GUIDs for capsule updatable firmware images */
>>>>>
>>>>> Please, provide code comments for the GUIDs, e.g.
>>>>>
>>>>> /**
>>>>>    * define K3_TIBOOT3_IMAGE_GUID - firmware GUID for K3 tiboot3.bin
>>>>>    *
>>>>>    * This GUID is used in capsules updates to identify the tiboot3.bin
>>>>>    * binary.
>>>>>    */
>>
>> I'd go one step further tbh.  Those GUIDs can be used by OEMs/ODMs
>> producing capsules for their future products.
>> Currently, we don't have a documented way to allow users to redefine
>> this (but Linaro is working on it).  If both TI and an OEM use the
>> same GUID and try to upload the firmware to LVFS, we will have a GUID
>> clash.
>> I think we should have that on the comment as well, until we can
>> properly sort it out
> 
> Ilias, I realized I may have defined the GUID's a bit incorrectly.  I
> was thinking that the GUID's identify the firmware type (eg, SPL vs
> Uboot for TI devices).  However, the SPL binary for say am62 will be
> different than the SPL binary for say am64, so I should have unique
> GUID's per firmware type *per board*.  Correct?
> 
> If so, I'll need to move these definitions to the board .h files, and my
> generic capsule binman nodes will need to have each board override the
> image GUID used.

Yes, consider for example if you wanted to maintain community firmware
images on LVFS, for a given image (like U-Boot itself) you would
presumably update it for each board, which requires each board to have
it's own GUID so you can tell them apart.

If one of the payloads is identical for a bunch of boards then it might
make sense to use one GUID for it.
> 
> thanks
> Jon
> 
>>
>> Thanks
>> /Ilias
>>>>>
>>>>> Cf.
>>>>> https://docs.kernel.org/doc-guide/kernel-doc.html#object-like-macro-documentation
>>>>>
>>>>> Best regards
>>>>>
>>>>> Heinrich
>>>>>
>>>>
>>>> Heinrich, thanks for reviewing!
>>>>
>>>> I modelled the GUID macros after how other boards and even core code
>>>> defined there's.  (eg, include/configs/kontron-sl-mx8mm.h or
>>>> include/efi_api.h).
>>>>
>>>> However, if this is the new direction, I will format as you suggest.
>>>> Please confirm.
>>>
>>> Hello Jon,
>>>
>>> Without properly documenting macros we make the live of developers more
>>> difficult. Yes, we still have a lot of missing code documentation. But
>>> we should not follow poor example.
>>>
>>> Best regards
>>>
>>> Heinrich
>>>
>>>>
>>>> thanks
>>>> Jon
>>>>
>>>>>> +#define K3_TIBOOT3_IMAGE_GUID \
>>>>>> +   EFI_GUID(0xe672b518, 0x7cd7, 0x4014, 0xbd, 0x8d, \
>>>>>> +            0x40, 0x72, 0x4d, 0x0a, 0xd4, 0xdc)
>>>>>> +
>>>>>> +#define K3_SPL_IMAGE_GUID \
>>>>>> +   EFI_GUID(0x86f710ad, 0x10cf, 0x46ea, 0xac, 0x67, \
>>>>>> +            0x85, 0x6a, 0xe0, 0x6e, 0xfa, 0xd2)
>>>>>> +
>>>>>> +#define K3_UBOOT_IMAGE_GUID \
>>>>>> +   EFI_GUID(0x81b58fb0, 0x3b00, 0x4add, 0xa2, 0x0a, \
>>>>>> +            0xc1, 0x85, 0xbb, 0xac, 0xa1, 0xed)
>>>>>> +
>>>>>> +#define K3_SYSFW_IMAGE_GUID \
>>>>>> +   EFI_GUID(0x6fd10680, 0x361b, 0x431f, 0x80, 0xaa, \
>>>>>> +            0x89, 0x94, 0x55, 0x81, 0x9e, 0x11)
>>>>>> +
>>>>>>    /*
>>>>>>     * We setup defaults based on constraints from the Linux kernel, which should
>>>>>>     * also be safe elsewhere.  We have the default load at 32MB into DDR (for
>>>

-- 
// Caleb (they/them)