Re: [Buildroot] [PATCH v1 2/5] boot/uboot.mk: new zynqmp pmufw prebuilt option

[Brandon Maier via buildroot <buildroot@buildroot.org>]

Hi Neal,

On 2024.06.12 19:28, Frager, Neal via buildroot wrote:
> Hi Brandon,
> 
> > The new BR2_TARGET_UBOOT_ZYNQMP_PMUFW_PREBUILT option will enable u-boot to
> > use the xilinx-prebuilt package for downloading a pmufw.elf that gets included
> > in the generated boot.bin.
> > 
> > If the BR2_TARGET_UBOOT_ZYNQMP_PMUFW_PREBUILT 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>
> > ---
> >  boot/uboot/Config.in | 16 ++++++++++++++++
> >  boot/uboot/uboot.mk  |  5 +++++
> >  2 files changed, 21 insertions(+)
> > 
> > diff --git a/boot/uboot/Config.in b/boot/uboot/Config.in
> > index d0901edc9d..9d275e8611 100644
> > --- a/boot/uboot/Config.in
> > +++ b/boot/uboot/Config.in
> > @@ -512,9 +512,25 @@ config BR2_TARGET_UBOOT_ZYNQMP
> >  
> >  if BR2_TARGET_UBOOT_ZYNQMP
> >  
> > +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
> 
> > Since this is a ZynqMP only option. Shouldn't this depend on the
> > Xilinx-prebuilt ZynqMP choice?
> 
> >  depends on BR2_TARGET_XILINX_PREBUILT_ZYNQMP
> 
> The kria boards also use this u-boot feature as they are part of the zynqmp
> family.  I suppose we could add this?
> 
> depends on BR2_TARGET_XILINX_PREBUILT_ZYNQMP | BR2_TARGET_XILINX_PREBUILT_KRIA

Ah, I assumed this was ZynqMP specific due to the config symbol name. If
it applies to multiple product lines then I don't think it's worth
bothering.

> 
> > +	help
> > +	  Use xilinx-prebuilt boot package for downloading prebuilt zynqmp
> > +	  pmufw.elf from https://github.com/Xilinx/soc-prebuilt-firmware 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.
> > +
> > +	  If this option is selected, the BR2_TARGET_UBOOT_ZYNQMP_PMUFW
> > +	  prebuilt option will be ignored.
> > +
> >  config BR2_TARGET_UBOOT_ZYNQMP_PMUFW
> >  	string "PMU firmware location"
> >  	depends on BR2_TARGET_UBOOT_BUILD_SYSTEM_KCONFIG
> > +	depends on !BR2_TARGET_UBOOT_ZYNQMP_PMUFW_PREBUILT
> >  	help
> >  	  Location of a PMU firmware binary.
> >  
> > diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
> > index b9f165f041..5d12c6283b 100644
> > --- a/boot/uboot/uboot.mk
> > +++ b/boot/uboot/uboot.mk
> > @@ -436,7 +436,12 @@ endef
> >  
> >  ifeq ($(BR2_TARGET_UBOOT_ZYNQMP),y)
> >  
> > +ifeq ($(BR2_TARGET_UBOOT_ZYNQMP_PMUFW_PREBUILT),y)
> > +UBOOT_DEPENDENCIES += xilinx-prebuilt
> > +UBOOT_ZYNQMP_PMUFW = $(BINARIES_DIR)/pmufw.elf
> > +else
> >  UBOOT_ZYNQMP_PMUFW = $(call qstrip,$(BR2_TARGET_UBOOT_ZYNQMP_PMUFW))
> > +endif #BR2_TARGET_UBOOT_ZYNQMP_PMUFW_PREBUILT
> >  
> >  ifneq ($(findstring ://,$(UBOOT_ZYNQMP_PMUFW)),)
> 
> > The `endif` should be moved to cover this ifneq statement. That way when
> > xilinx-prebuilt is enabled it doesn't do the "://" and readlink checks.
> 
> Does this really matter?  If BR2_TARGET_UBOOT_ZYNQMP_PMUFW_PREBUILT is set,
> then UBOOT_ZYNQMP_PMUFW = $(BINARIES_DIR)/pmufw.elf, so there will never be
> a :// in UBOOT_ZYNQMP_PMUFW and the UBOOT_EXTRA_DOWNLOADS will not get set.

My intent is to make the logic easier to understand, by splitting the
custom PMUFW logic from the xilinx-prebuilt logic.

> The problem with moving the endif after the ifneq is that we still need the
> UBOOT_ZYNQMP_PMUFW_PATH to be set for the rest of the functionality to work.
> 
> UBOOT_ZYNQMP_PMUFW_PATH = $(shell readlink -f $(UBOOT_ZYNQMP_PMUFW))
> 
> To avoid duplicating this line of code, keeping the endif before the ifneq
> seemed to be the cleaner solution to me.
> 

The readlink is not needed for xilinx-prebuilt as $(BINARIES_DIR) is
always an absolute path. Instead you could assign the
`$(BINARIES_DIR)/pmufw.elf` directly to `UBOOT_ZYNQMP_PMUFW_PATH`.

But the above is a nit pick, so I'm good with applying.

Reviewed-by: Brandon Maier <brandon.maier@collins.com>

> >  UBOOT_EXTRA_DOWNLOADS += $(UBOOT_ZYNQMP_PMUFW)
> > -- 
> > 2.25.1
> > 
> > _______________________________________________
> > buildroot mailing list
> > buildroot@buildroot.org
> > https://lists.buildroot.org/mailman/listinfo/buildroot
> 
> Best regards,
> Neal Frager
> AMD
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot