Apologies for responding to myself here, but clarifying things a bit more...
On Fri, 13 Sept 2024 at 14:15, Ilias Apalodimas <[email protected]> wrote: > > Hi Matthias > > On Fri, 13 Sept 2024 at 13:40, Matthias Brugger <[email protected]> wrote: > > > > > > > > On 11/09/2024 12:05, Ilias Apalodimas wrote: > > > On Wed, 11 Sept 2024 at 12:50, Peter Robinson <[email protected]> > > > wrote: > > >> > > >> On Tue, 10 Sept 2024 at 08:29, Sughosh Ganu <[email protected]> > > >> wrote: > > >>> > > >>> On Tue, 10 Sept 2024 at 12:09, Ilias Apalodimas > > >>> <[email protected]> wrote: > > >>>> > > >>>> Since RPI works well using EFI and has no size limitations with regards > > >>>> to U-Boot, add the needed structures and Kconfig options needed to > > >>>> enable capsule updates > > >>>> --- > > >>>> board/raspberrypi/rpi/rpi.c | 22 ++++++++++++++++++++++ > > >>>> configs/rpi_4_defconfig | 2 ++ > > >>>> 2 files changed, 24 insertions(+) > > >>> > > >>> Tested-by: Sughosh Ganu <[email protected]> > > >>> > > >>> A couple of nits below. > > >>> > > >>>> > > >>>> diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c > > >>>> index ab5ea85cf9f8..1f342eee12b2 100644 > > >>>> --- a/board/raspberrypi/rpi/rpi.c > > >>>> +++ b/board/raspberrypi/rpi/rpi.c > > >>>> @@ -63,6 +63,28 @@ struct msg_get_clock_rate { > > >>>> u32 end_tag; > > >>>> }; > > >>>> > > >>>> +struct efi_fw_image fw_images[] = { > > >>>> + { > > >>>> + .fw_name = u"RPI_UBOOT", > > >>>> + .image_index = 1, > > >>>> + }, > > >>>> +}; > > >>>> + > > >>>> +struct efi_capsule_update_info update_info = { > > >>>> + .dfu_string = "mmc 0=u-boot.bin fat 0 1", > > >>>> + .num_images = ARRAY_SIZE(fw_images), > > >>>> + .images = fw_images, > > >>>> +}; > > >>>> + > > >>>> +#if IS_ENABLED(CONFIG_SET_DFU_ALT_INFO) > > >>>> +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); > > >>>> +} > > >>>> +#endif > > >>> > > >>> Is this really needed? We have a weak function in efi_firrmware.c > > >>> which is doing exactly this. > > >>> > > >>>> + > > >>>> + > > >>>> #ifdef CONFIG_ARM64 > > >>>> #define DTB_DIR "broadcom/" > > >>>> #else > > >>>> diff --git a/configs/rpi_4_defconfig b/configs/rpi_4_defconfig > > >>>> index f5fb322aa8fc..c70414e6fcaf 100644 > > >>>> --- a/configs/rpi_4_defconfig > > >>>> +++ b/configs/rpi_4_defconfig > > >>>> @@ -65,3 +65,5 @@ CONFIG_SYS_WHITE_ON_BLACK=y > > >>>> CONFIG_VIDEO_BCM2835=y > > >>>> CONFIG_CONSOLE_SCROLL_LINES=10 > > >>>> CONFIG_PHYS_TO_BUS=y > > >>>> +CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y > > >>>> +CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y > > >>> > > >>> Can we also add the efidebug and efi nvedit commands here. They are > > >>> pretty handy especially when it comes to capsule updates. Thanks. > > >> > > >> Are they pretty handy for capsule updates for general users when > > >> things should "just work", as a user applies them from Linux with > > >> fwupdmgr and reboots, or for firmwre developers trying to debug > > >> things? If it's the later I don't think we should be enabling them by > > >> default :) > > > > > > nvedit lets you print and see EFI variables, which is pretty basic if > > > you want to boot with efi. > > > efidebug is supposed to be a debug tool mostly, but unfortunately, we > > > havent plugged in EFI HTTP boot into the 'eficonfig' command yet. So > > > the only way you can add a boot option for HTTP is via efidebug > > > > > > > I don't really understand why you are mentioning EFI HTTP. > > > Because I misread Peters's question and assumed he was asking if it > was for capsules only or useful overall as a tool. > > > From what I can see > > we need efidebug to be able to do capsule updates from U-Boot command line. > > Updating via the command line is for debugging only. The capsule > updates execute automatically after a reboot if a variable > (OsIndications) is set properly or you enable > CONFIG_EFI_IGNORE_OSINDICATIONS and have a boot entry pointing to the > same ESP where the capsule is located. But that's a bit complicated > atm, look below. The above is only true if CONFIG_EFI_CAPSULE_ON_DISK, which I've intentionally left out. Only runtime capsule update is selected, which requires efidebug or an EFI app. > > > It is also needed for EFI HTTP boot but this needs CONFIG_EFI_HTTP_BOOT > > which we > > don't enable (and I think is out-of-scope for capsule update). > > > > /me is puzzled :) > > Fair enough. We could make it default tbh. It depends on how much > 'efi' you prefer in the defconfig. But that belongs on a different > patchset. > > Something completely irrelevant to efidebug, but related to capsule updates. > Capsule updates, in order to work with tools like fwupd etc, either > need commits 00da8d65a3ba and bc3dd2493ef8 and c28d32f946f0 and > https://github.com/rhboot/efivar/pull/267 or enable > CONFIG_EFI_IGNORE_OSINDICATIONS. Since the latter is a hack I was > going to enable SetVariable at Runtime once the userspace patches get > merged and sorted. Keep in mind there's a v3 out which I managed to > test with rpi5. FWIW still think efidebug is useful and needs to be > around regardless of capsules or not. > What we could do in v4 is enable CONFIG_EFI_IGNORE_OSINDICATIONS, but > I would prefer to wait for the efivar pathes to merge and just enable > setvariable at runtime. > Also, all these apply to CONFIG_EFI_CAPSULE_ON_DISK only, which I will enable once the efivar are merged Sorry for the confusion! /Ilias > Thanks > /Ilias > > > > > Regards, > > Matthias
