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

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

Hi Neal,

On Tue, 21 Jan 2025 08:40:33 +0000
"Frager, Neal" <neal.frager@amd.com> wrote:

> Hello Luca,
> 
> > 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".  
> 
> Agreed, thanks!
> 
> > +	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"  
> 
> Ok, works for me.
> 
> > +	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.  
> 
> I thought about this as well.  Unfortunately, option b is more complicated
> than just enabling xilinx-embeddedsw or xilinx-prebuilt.  Not only do the
> zynqmp options in either package also need to be enabled, but in the case
> of xilinx-embeddedsw, the toolchain-bare-metal-buildroot also needs to be
> enabled along with configuring the tupple to microblaze.

That's true.

> Since I do not think enabling all these options in other packages
> automatically from the uboot config is very clean, I think it is best we
> stay with option a.
> 
> To go along with option a, I like your idea of making the choice three
> options as you speak of later on in this email.  For options not enabled,
> I would like to add a comment in the choice menu informing the user of
> what they need to enable to make the option available.  I think this is
> a better solution.

Adding the comments looks like a good idea, e.g. "Building the PMUFW
from source needs xilinx-embeddedsw"

> And since the custom pmufw location option is the only one not dependent
> on another Buildroot package, I would recommend having it being the
> default choice, since it will always be available.
> 
> > +	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"  
> 
> Ok, sounds good to me.
> 
> >  	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.  
> 
> Agreed.  Making this a three way choice is the best option.
> 
> >  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?".  
> 
> I agree with the three option choice idea.  I had not thought of it earlier
> because choice options need to be boolean as far as I am aware.
> 
> To make this possible, I am thinking of adding the third choice as:
> BR2_TARGET_UBOOT_ZYNQMP_PMUFW_CUSTOM
>     bool "Prebuilt from custom location"
> 
> And then the BR2_TARGET_UBOOT_ZYNQMP_PMUFW would stay the same as before
> with just having a dependency on BR2_TARGET_UBOOT_ZYNQMP_PMUFW_CUSTOM.
> 
> What do you think?

I think it is fine.

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