Re: [Buildroot] [PATCH v3 3/4] boot/uboot.mk: new zynqmp pmufw source option

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

On Tue, 18 Jun 2024 08:49:21 +0100
Neal Frager <neal.frager@amd.com> wrote:

> The new BR2_TARGET_UBOOT_ZYNQMP_PMUFW_SOURCE option will enable u-boot to
> use the xilinx-source package for building a pmufw.elf that gets included
> in the generated boot.bin.
> 
> If the BR2_TARGET_UBOOT_ZYNQMP_PMUFW_SOURCE option is enabled, then the
> BR2_TARGET_UBOOT_ZYNQMP_PMUFW config for downloading a prebuilt pmufw will
> be ignored.
> 
> Signed-off-by: Neal Frager <neal.frager@amd.com>
> ---
> V1->V3:
> - no changes
> ---
>  boot/uboot/Config.in | 20 +++++++++++++++++++-
>  boot/uboot/uboot.mk  |  3 +++
>  2 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/boot/uboot/Config.in b/boot/uboot/Config.in
> index f37040a28a..3aef222772 100644
> --- a/boot/uboot/Config.in
> +++ b/boot/uboot/Config.in
> @@ -512,9 +512,12 @@ config BR2_TARGET_UBOOT_ZYNQMP
>  
>  if BR2_TARGET_UBOOT_ZYNQMP
>  
> +choice
> +	bool "pmufw.elf prebuilt or source"

I find this string not very clear. It should rather look more like
"PMUFW origin", or "Origin of pmufw.elf".

But what is more confusing is the available choice options. With some
combinations of BR2_TARGET_XILINX_PREBUILT, BR2_TARGET_XILINX_SOURCE
and the family variant, we get no selectable options for this choice,
which is very awkward.

If the choice has no selectable options, then it should just disappear.
In its place you might instead put something like:

  comment "both xilinx-source and xilinx-prebuilt disabled, you need to
    pass a valid pmufw"

just before BR2_TARGET_UBOOT_ZYNQMP_PMUFW.

> +	depends on BR2_TARGET_UBOOT_BUILD_SYSTEM_KCONFIG
> +
>  config BR2_TARGET_UBOOT_ZYNQMP_PMUFW_PREBUILT
>  	bool "xilinx-prebuilt pmufw.elf"
> -	depends on BR2_TARGET_UBOOT_BUILD_SYSTEM_KCONFIG
>  	depends on BR2_TARGET_XILINX_PREBUILT_ZYNQMP || BR2_TARGET_XILINX_PREBUILT_KRIA
>  	help
>  	  Use xilinx-prebuilt boot package for downloading prebuilt
> @@ -525,10 +528,25 @@ 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.
>  
> +config BR2_TARGET_UBOOT_ZYNQMP_PMUFW_SOURCE
> +	bool "xilinx-source pmufw.elf"
> +	depends on BR2_TARGET_XILINX_SOURCE_ZYNQMP || BR2_TARGET_XILINX_SOURCE_KRIA
> +	help
> +	  Use xilinx-source 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
> +	  by the ZynqMP boot ROM) containing both the U-Boot SPL and the
> +	  PMU firmware in the Xilinx-specific boot format.	  
> +
> +endchoice
> +
>  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_PREBUILT
> +	depends on !BR2_TARGET_UBOOT_ZYNQMP_PMUFW_SOURCE

Very strange: with this patch, when both
BR2_TARGET_UBOOT_ZYNQMP_PMUFW_PREBUILT and
BR2_TARGET_UBOOT_ZYNQMP_PMUFW_SOURCE are =y,
BR2_TARGET_UBOOT_ZYNQMP_PMUFW becomes visible. This looks like a
kconfig/menuconfig bug and certainly not a bug in your patch, but maybe
you can have a deeper look?

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