| Message ID | 20240408223134.165877-2-j-humphreys@ti.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | EFI: ti: Enable EFI capsule updates | expand |
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@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. */ Cf. https://docs.kernel.org/doc-guide/kernel-doc.html#object-like-macro-documentation Best regards Heinrich > +#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
Heinrich Schuchardt <xypron.glpk@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@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. > */ > > 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. 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
On 09.04.24 23:05, Jon Humphreys wrote: > Heinrich Schuchardt <xypron.glpk@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@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. >> */ >> >> 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
Hi both On Wed, 10 Apr 2024 at 10:36, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 09.04.24 23:05, Jon Humphreys wrote: > > Heinrich Schuchardt <xypron.glpk@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@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 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 >
Ilias Apalodimas <ilias.apalodimas@linaro.org> writes: > Hi both > > On Wed, 10 Apr 2024 at 10:36, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> >> On 09.04.24 23:05, Jon Humphreys wrote: >> > Heinrich Schuchardt <xypron.glpk@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@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. 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 >>
On 12/04/2024 22:58, Jon Humphreys wrote: > Ilias Apalodimas <ilias.apalodimas@linaro.org> writes: > >> Hi both >> >> On Wed, 10 Apr 2024 at 10:36, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >>> >>> On 09.04.24 23:05, Jon Humphreys wrote: >>>> Heinrich Schuchardt <xypron.glpk@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@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 >>>
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 */ +#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
Define GUIDs for the different firmware images (tiboot3.bin, tispl.bin, u-boot.img, sysfw). Signed-off-by: Jonathan Humphreys <j-humphreys@ti.com> --- include/configs/ti_armv7_common.h | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)