[PATCH] board: rpi: Enable capsule updates

[Ilias Apalodimas]

Hi Matthias

On Fri, 13 Sept 2024 at 13:40, Matthias Brugger <mbrugger at suse.com> wrote:
>
>
>
> On 11/09/2024 12:05, Ilias Apalodimas wrote:
> > On Wed, 11 Sept 2024 at 12:50, Peter Robinson <pbrobinson at gmail.com> wrote:
> >>
> >> On Tue, 10 Sept 2024 at 08:29, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> >>>
> >>> On Tue, 10 Sept 2024 at 12:09, Ilias Apalodimas
> >>> <ilias.apalodimas at linaro.org> 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 <sughosh.ganu at linaro.org>
> >>>
> >>> 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.

> 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.

Thanks
/Ilias

>
> Regards,
> Matthias