Re: [Buildroot] [PATCH v2 2/4] boot/uboot.mk: new zynqmp pmufw embeddedsw option

[Luca Ceresoli via buildroot <buildroot@buildroot.org>]

On Mon, 20 Jan 2025 11:34:03 +0000
Neal Frager <neal.frager@amd.com> wrote:

> The new BR2_TARGET_UBOOT_ZYNQMP_PMUFW_EMBEDDEDSW option will enable u-boot to
> use the xilinx-embeddedsw package for building a pmufw.elf that gets included
> in the generated boot.bin.
> 
> If the BR2_TARGET_UBOOT_ZYNQMP_PMUFW_EMBEDDEDSW option is enabled, then the
> BR2_TARGET_UBOOT_ZYNQMP_PMUFW config for downloading a prebuilt pmufw from a
> custom location will be ignored.
> 
> Signed-off-by: Neal Frager <neal.frager@amd.com>
> ---
> V1->V2:
> - edited Config.in help text to fit within 70 characters
> ---
>  boot/uboot/Config.in | 27 +++++++++++++++++++++++++++
>  boot/uboot/uboot.mk  |  5 ++++-
>  2 files changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/boot/uboot/Config.in b/boot/uboot/Config.in
> index b6ac2f9380..430191d213 100644
> --- a/boot/uboot/Config.in
> +++ b/boot/uboot/Config.in
> @@ -572,6 +572,27 @@ config BR2_TARGET_UBOOT_ZYNQMP
>  
>  if BR2_TARGET_UBOOT_ZYNQMP
>  
> +choice
> +	prompt "xilinx-prebuilt pmufw.elf or build pmufw.elf from source"

Not a very clear string IMO, it should not list the options in the
choice title. I'd rather change it to "PMUFW origin".

> +	default BR2_TARGET_UBOOT_ZYNQMP_PMUFW_EMBEDDEDSW
> +	help
> +	  Choose between installing the pmufw.elf from
> +	  xilinx-prebuilt or building the pmufw.elf from
> +	  xilinx-embeddedsw.
> +
> +config BR2_TARGET_UBOOT_ZYNQMP_PMUFW_EMBEDDEDSW
> +	bool "xilinx-embeddedsw build pmufw.elf from source"

And I'd change this to "Build from source via xilinx-embeddedsw"

> +	depends on BR2_TARGET_UBOOT_BUILD_SYSTEM_KCONFIG
> +	depends on BR2_TARGET_XILINX_EMBEDDEDSW_ZYNQMP_PMUFW

Not sure what is best here:

 a) if xilinx-embeddedsw is enabled, show the option in the choice menu
 b) if the choice is selected in the menu, enable xilinx-embeddedsw

a) is what you implemented, b) is what I had in mind before reading
this patch and it would look more intuitive for users I think. The same
applies for the xilinx-prebuilt option.

Opinions from Buildroot maintainers would be welcome here.

> +	help
> +	  Use xilinx-embeddedsw boot package for building
> +	  zynqmp pmufw.elf from
> +	    https://github.com/Xilinx/embeddedsw repo.
> +
> +	  U-Boot build process will generate a boot.bin (to be loaded
> +	  by the ZynqMP boot ROM) containing both the U-Boot SPL and
> +	  the PMU firmware in the Xilinx-specific boot format.
> +
>  config BR2_TARGET_UBOOT_ZYNQMP_PMUFW_PREBUILT
>  	bool "xilinx-prebuilt pmufw.elf"

And this to "Prebuilt via xilinx-prebuilt"

>  	depends on BR2_TARGET_UBOOT_BUILD_SYSTEM_KCONFIG
> @@ -585,9 +606,15 @@ config BR2_TARGET_UBOOT_ZYNQMP_PMUFW_PREBUILT
>  	  by the ZynqMP boot ROM) containing both the U-Boot SPL and the
>  	  PMU firmware in the Xilinx-specific boot format.
>  
> +endchoice
> +
> +comment "If xilinx-embeddedsw or xilinx-prebuilt is selected for pmufw.elf, custom PMU firmware location will be ignored."
> +	depends on BR2_TARGET_UBOOT_ZYNQMP_PMUFW_EMBEDDEDSW || BR2_TARGET_UBOOT_ZYNQMP_PMUFW_PREBUILT

This is not needed. If any of those two is selected,
BR2_TARGET_UBOOT_ZYNQMP_PMUFW will not be visible. The commit message
needs to be updated accordingly. See below however.

>  config BR2_TARGET_UBOOT_ZYNQMP_PMUFW
>  	string "Custom PMU firmware location"
>  	depends on BR2_TARGET_UBOOT_BUILD_SYSTEM_KCONFIG
> +	depends on !BR2_TARGET_UBOOT_ZYNQMP_PMUFW_EMBEDDEDSW
>  	depends on !BR2_TARGET_UBOOT_ZYNQMP_PMUFW_PREBUILT

I think BR2_TARGET_UBOOT_ZYNQMP_PMUFW should become a third option in
the choice you are adding above. In other words we should ask users
"How do you want to get the pmufw? Build from source, download from
xilinx-prebuilt or pre-built at a custom location you provide?".

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot