Hi Jon,
On Fri, 24 May 2024 at 18:38, Jon Humphreys j-humphreys@ti.com wrote:
Ilias Apalodimas ilias.apalodimas@linaro.org 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 j-humphreys@ti.com
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
Thanks /Ilias
https://lore.kernel.org/all/b19f02e0-a80a-46d6-8296-5d516577766a@kernel.org/
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