On Fri, 31 May 2024 at 07:10, Jon Humphreys <[email protected]> wrote: > > Ilias Apalodimas <[email protected]> writes: > > > Hi Jon, > > > > On Fri, 24 May 2024 at 18:38, Jon Humphreys <[email protected]> wrote: > >> > >> Ilias Apalodimas <[email protected]> writes: > >> > >> > Hi Jonathan > >> > > >> > Thanks for working on this > >> > > >> > On Thu, May 09, 2024 at 11:41:19AM -0500, Jonathan Humphreys wrote: > >> >> Define the firmware components updatable via EFI capsule update, > >> >> including > >> >> defining capsule GUIDs for the various firmware components for the > >> >> AM62px > >> >> SK. > >> >> > >> >> Signed-off-by: Jonathan Humphreys <[email protected]> > >> >> --- > >> >> board/ti/am62px/evm.c | 32 ++++++++++++++++++++++++++++++++ > >> >> include/configs/am62px_evm.h | 24 ++++++++++++++++++++++++ > >> >> 2 files changed, 56 insertions(+) > >> >> > >> >> diff --git a/board/ti/am62px/evm.c b/board/ti/am62px/evm.c > >> >> index 97a95ce8cc2..6d0f66e5dc0 100644 > >> >> --- a/board/ti/am62px/evm.c > >> >> +++ b/board/ti/am62px/evm.c > >> >> @@ -6,6 +6,7 @@ > >> >> * > >> >> */ > >> >> > >> >> +#include <efi_loader.h> > >> >> #include <asm/arch/hardware.h> > >> >> #include <asm/io.h> > >> >> #include <dm/uclass.h> > >> >> @@ -13,6 +14,37 @@ > >> >> #include <fdt_support.h> > >> >> #include <spl.h> > >> >> > >> >> +struct efi_fw_image fw_images[] = { > >> > > >> > It's better if we add an > >> > #if IS_ENABLED(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) > >> > for both of the structs that follow (and it applies to all your patches) > >> > > >> > >> Ilias, thanks for the reviews. > >> > >> I had this protected in #if's in an earlier patch set, as you suggest here. > >> However, in those reviews, Roger recommended that we don't do that and put > >> conditions around the use of it in set_dfu_alt_info(). > >> > > > > Hmm but the function prototype itself is on an ifdef. If you want to > > remove the ifdef you got to do it everywhere > > > > Are you referring to set_dfu_alt_info() which is guarded by > CONFIG_SET_DFU_ALT_INFO?
Yes > > If so, that is separate but I can add a CONFIG_SET_DFU_ALT_INFO guard > around the definition, for now. But IMO it is a bit of a mess because it's > use and board specific defs are guarded by CONFIG_SET_DFU_ALT_INFO but the > weak/default definition is guarded by CONFIG_EFI_CAPSULE_FIRMWARE, which > causes problems because the configs are not always the same for all builds. Indeed > I was wanting to fix that too so I might do that as a separate patch and > make that patch a prerequisite for this series, which then allows me to > remove the definitions of set_dfu_alt_info() in this series. > We can clean it up later sure, but for now put it under an IS_ENABLED so we have the same mess everywhere :) Thanks /Ilias > Jon > > > Thanks > > /Ilias > > > >> https://lore.kernel.org/all/[email protected]/ > >> > >> I assume the reasoning is to reduce #if's in the code and rely on the > >> compiler to be smart enough to remove dead data. (Roger, speak up if I > >> misrepresent you.) > >> > >> I'm ok to do either way. What is the preferred way in U-Boot? > >> > >> Thanks > >> Jon > >> > >> >> + { > >> >> + .image_type_id = AM62PX_SK_TIBOOT3_IMAGE_GUID, > >> >> + .fw_name = u"AM62PX_SK_TIBOOT3", > >> >> + .image_index = 1, > >> >> + }, > >> >> + { > >> >> + .image_type_id = AM62PX_SK_SPL_IMAGE_GUID, > >> >> + .fw_name = u"AM62PX_SK_SPL", > >> >> + .image_index = 2, > >> >> + }, > >> >> + { > >> >> + .image_type_id = AM62PX_SK_UBOOT_IMAGE_GUID, > >> >> + .fw_name = u"AM62PX_SK_UBOOT", > >> >> + .image_index = 3, > >> >> + } > >> >> +}; > >> >> + > >> >> +struct efi_capsule_update_info update_info = { > >> >> + .dfu_string = "sf 0:0=tiboot3.bin raw 0 80000;" > >> >> + "tispl.bin raw 80000 200000;u-boot.img raw 280000 400000", > >> >> + .num_images = ARRAY_SIZE(fw_images), > >> >> + .images = fw_images, > >> >> +}; > >> > > >> > I haven't worked on any TI platforms lately so I cant say much about the > >> > naming and the flash regions. The definition seems correct > >> > > >> > > >> >> + > >> >> +void set_dfu_alt_info(char *interface, char *devstr) > >> >> +{ > >> >> + if (IS_ENABLED(CONFIG_EFI_HAVE_CAPSULE_SUPPORT)) > >> >> + env_set("dfu_alt_info", update_info.dfu_string); > >> >> +} > >> > > >> > There's a CONFIG_SET_DFU_ALT_INFO symbol. This better if we add a check > >> > here > >> > as well > >> > > >> >> + > >> >> int board_init(void) > >> >> { > >> >> return 0; > >> >> diff --git a/include/configs/am62px_evm.h b/include/configs/am62px_evm.h > >> >> index 06b12860e21..57a1ba9dc3c 100644 > >> >> --- a/include/configs/am62px_evm.h > >> >> +++ b/include/configs/am62px_evm.h > >> >> @@ -8,6 +8,30 @@ > >> >> #ifndef __CONFIG_AM62PX_EVM_H > >> >> #define __CONFIG_AM62PX_EVM_H > >> >> > >> > [...] > >> > > >> > Regards > >> > /Ilias
