diff mbox series

board: rpi: Enable capsule updates

Message ID 20240910063933.141283-1-ilias.apalodimas@linaro.org
State New
Headers show
Series board: rpi: Enable capsule updates | expand

Commit Message

Ilias Apalodimas Sept. 10, 2024, 6:39 a.m. UTC 
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(+)

Comments

Ilias Apalodimas Sept. 10, 2024, 6:41 a.m. UTC | #1 
Apologies for the noise, forgot to mention this needs to be applied on
top of https://lore.kernel.org/u-boot/20240830-b4-dynamic-uuid-v8-0-79b31b199bee@linaro.org/,
once that lands

/Ilias

On Tue, 10 Sept 2024 at 09:39, Ilias Apalodimas
<ilias.apalodimas@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(+)
>
> 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
> +
> +
>  #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
> --
> 2.45.2
>
Sughosh Ganu Sept. 10, 2024, 7:29 a.m. UTC | #2 
On Tue, 10 Sept 2024 at 12:09, Ilias Apalodimas
<ilias.apalodimas@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@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.

-sughosh
Ilias Apalodimas Sept. 10, 2024, 7:31 a.m. UTC | #3 
Thanks for testing

On Tue, 10 Sept 2024 at 10:29, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> On Tue, 10 Sept 2024 at 12:09, Ilias Apalodimas
> <ilias.apalodimas@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@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.

Good point, I'll get rid of 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.

Sure, I'll add those


/Ilias
>
> -sughosh
Matthias Brugger Sept. 10, 2024, 1:50 p.m. UTC | #4 
On 10/09/2024 08:39, Ilias Apalodimas 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(+)
> 
> 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
> +
> +
>   #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

Would you mind to enable this in all corresponding configs.

That would be at least: rpi_3_b_plus_defconfig, rpi_3_defconfig, 
rpi_arm64_defconfig.

Thanks,
Matthias
Ilias Apalodimas Sept. 10, 2024, 1:54 p.m. UTC | #5 
Hi Matthias,

On Tue, 10 Sept 2024 at 16:50, Matthias Brugger <mbrugger@suse.com> wrote:
>
>
>
> On 10/09/2024 08:39, Ilias Apalodimas 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(+)
> >
> > 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
> > +
> > +
> >   #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
>
> Would you mind to enable this in all corresponding configs.
>
> That would be at least: rpi_3_b_plus_defconfig, rpi_3_defconfig,
> rpi_arm64_defconfig.

No I don't, I just didn't have the hardware to test it. In practice
different boards should have different GUIDS -- so you dont end up
updating and RPI4 with an RPI3 firmware. I am about to send a PR that
enables a dynamic GUID generation (which this patch depends on). That
patch reads the root compatible node and uses it to derive unique
UUIDs. I assume that all those boards have a different compatible node
and we wont have clashes?

Thanks
/Ilias
>
> Thanks,
> Matthias
Peter Robinson Sept. 11, 2024, 9:49 a.m. UTC | #6 
On Tue, 10 Sept 2024 at 08:29, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> On Tue, 10 Sept 2024 at 12:09, Ilias Apalodimas
> <ilias.apalodimas@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@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 :)
Ilias Apalodimas Sept. 11, 2024, 10:05 a.m. UTC | #7 
On Wed, 11 Sept 2024 at 12:50, Peter Robinson <pbrobinson@gmail.com> wrote:
>
> On Tue, 10 Sept 2024 at 08:29, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > On Tue, 10 Sept 2024 at 12:09, Ilias Apalodimas
> > <ilias.apalodimas@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@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

Cheers
/Ilias
Matthias Brugger Sept. 13, 2024, 10:40 a.m. UTC | #8 
On 11/09/2024 12:05, Ilias Apalodimas wrote:
> On Wed, 11 Sept 2024 at 12:50, Peter Robinson <pbrobinson@gmail.com> wrote:
>>
>> On Tue, 10 Sept 2024 at 08:29, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>>>
>>> On Tue, 10 Sept 2024 at 12:09, Ilias Apalodimas
>>> <ilias.apalodimas@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@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. From what I can see 
we need efidebug to be able to do capsule updates from U-Boot command line. 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 :)

Regards,
Matthias
Ilias Apalodimas Sept. 13, 2024, 11:15 a.m. UTC | #9 
Hi Matthias

On Fri, 13 Sept 2024 at 13:40, Matthias Brugger <mbrugger@suse.com> wrote:
>
>
>
> On 11/09/2024 12:05, Ilias Apalodimas wrote:
> > On Wed, 11 Sept 2024 at 12:50, Peter Robinson <pbrobinson@gmail.com> wrote:
> >>
> >> On Tue, 10 Sept 2024 at 08:29, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >>>
> >>> On Tue, 10 Sept 2024 at 12:09, Ilias Apalodimas
> >>> <ilias.apalodimas@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@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
Ilias Apalodimas Sept. 13, 2024, 11:54 a.m. UTC | #10 
Apologies for responding to myself here, but clarifying things a bit more...

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

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
diff mbox series

Patch

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
+
+
 #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