Re: [Buildroot] [PATCH v12 1/3] boot/zynqmp-firmware: new boot firmware

["Yann E. MORIN" <yann.morin.1998@free.fr>]

Neal, All,

On 2024-06-07 10:47 +0100, Neal Frager via buildroot spake thusly:
> This patch adds a new boot firmware to buildroot for building the zynqmp pmufw.
> It requires the toolchain-bare-metal package that includes a bare-metal
> binutils, gcc and newlib which can be built for the microblaze architecture.
> 
> Signed-off-by: Neal Frager <neal.frager@amd.com>
> ---
[--SNIP--]
> diff --git a/boot/zynqmp-firmware/Config.in b/boot/zynqmp-firmware/Config.in
> new file mode 100644
> index 0000000000..478d58d552
> --- /dev/null
> +++ b/boot/zynqmp-firmware/Config.in
> @@ -0,0 +1,25 @@
> +config BR2_TARGET_ZYNQMP_FIRMWARE
> +	bool "zynqmp-firmware"
> +	depends on BR2_TOOLCHAIN_BARE_METAL_BUILDROOT_ARCH = "microblazeel-xilinx-elf"
> +	help
> +	  This package builds the PMU Firmware application required
> +	  to run U-Boot and Linux in the Zynq MPSoC devices.
> +
> +if BR2_TARGET_ZYNQMP_FIRMWARE
> +
> +comment "zynqmp-firmware needs a bare metal toolchain for tuple microblazeel-xilinx-elf"
> +	depends on BR2_TOOLCHAIN_BARE_METAL_BUILDROOT_ARCH != "microblazeel-xilinx-elf"

The comment is in the if-endif block, so it will only show when
BR2_TARGET_ZYNQMP_FIRMWARE, but the conditions to show the comment are
the opposite of those to show BR2_TARGET_ZYNQMP_FIRMWARE in the first
place, so the comment will not be visible when BR2_TARGET_ZYNQMP_FIRMWARE
is not already available.

So the comment will never be visible.

So, either place it before the main symnbol, or after the if-endif block
(but not in-between the two).

No need to respin just for that, it can be fixed when applying.

> +config BR2_TARGET_ZYNQMP_FIRMWARE_CUSTOM_CFLAGS
> +	string "custom cflags"
> +	help
> +	  Adds additional CFLAGS for building zynqmp firmware.

What kind of specific CFLAGS would one have to pass?

I see in patch 3 that you are passing -DK26_SOM, so presumably that's
about the CPU or board variant. Shouldn't we have a choice for that:

In the Config.in:

    choice
        bool "Board/CPU variant"

    config BR2_TARGET_ZYNQMP_FIRMWARE_DEFAULT
        bool "default"

    config BR2_TARGET_ZYNQMP_FIRMWARE_K26_SOM
        bool "K26 SOM"

    endchoice

    config BR2_TARGET_ZYNQMP_FIRMWARE_VARIANT
        string
        default "K26_SOM"   if BR2_TARGET_ZYNQMP_FIRMWARE_K26_SOM

In the .mk:

    ZYNQMP_FIRMWARE_VARIANT = $(call qstrip,$(BR2_TARGET_ZYNQMP_FIRMWARE_VARIANT))
    ifneq ($(ZYNQMP_FIRMWARE_VARIANT),)
    ZYNQMP_FIRMWARE_CFLAGS += -D$(ZYNQMP_FIRMWARE_VARIANT)
    endif

> +endif # BR2_TARGET_ZYNQMP_FIRMWARE
> diff --git a/boot/zynqmp-firmware/zynqmp-firmware.mk b/boot/zynqmp-firmware/zynqmp-firmware.mk
> new file mode 100644
> index 0000000000..dafb2dbe55
> --- /dev/null
> +++ b/boot/zynqmp-firmware/zynqmp-firmware.mk
> @@ -0,0 +1,33 @@
> +################################################################################
> +#
> +# zynqmp-firmware
> +#
> +################################################################################
> +
> +ZYNQMP_FIRMWARE_VERSION = $(call qstrip,$(BR2_TARGET_ZYNQMP_FIRMWARE_VERSION))
> +ZYNQMP_FIRMWARE_SITE = \
> +	$(call github,Xilinx,embeddedsw,$(ZYNQMP_FIRMWARE_VERSION))
> +ZYNQMP_FIRMWARE_LICENSE = MIT
> +ZYNQMP_FIRMWARE_LICENSE_FILES = license.txt
> +ZYNQMP_FIRMWARE_INSTALL_IMAGES = YES
> +ZYNQMP_FIRMWARE_INSTALL_TARGET = NO
> +ZYNQMP_FIRMWARE_DEPENDENCIES = toolchain-bare-metal-buildroot
> +
> +CUSTOM_CFLAGS = $(call qstrip,$(BR2_TARGET_ZYNQMP_FIRMWARE_CUSTOM_CFLAGS))

All variables must be scopped in the current package "namespace" [0]:

    ZYNQMP_FIRMWARE_CUSTOM_CFLAGS = ...

No need to respin just for that, it can be fixed when applying...

> +ZYNQMP_FIRMWARE_CFLAGS = "-Os -flto -ffat-lto-objects $(CUSTOM_CFLAGS)"
> +
> +define ZYNQMP_FIRMWARE_BUILD_CMDS
> +	$(MAKE) -C $(@D)/lib/sw_apps/zynqmp_pmufw/src \
> +		COMPILER=$(HOST_DIR)/bin/microblazeel-xilinx-elf-gcc \
> +		ARCHIVER=$(HOST_DIR)/bin/microblazeel-xilinx-elf-gcc-ar \
> +		CC=$(HOST_DIR)/bin/microblazeel-xilinx-elf-gcc \
> +		CFLAGS=$(ZYNQMP_FIRMWARE_CFLAGS)
> +endef
> +
> +ZYNQMP_FIRMWARE = $(@D)/lib/sw_apps/zynqmp_pmufw/src/executable.elf

Ah, I see that the check-package hint is not very explicit. All it says
is that the variable is not properly scopped, and what it reports is
the prefix all variables should have:

    ZYNQMP_FIRMWARE_PMUFW = ...

ZYNQMP_FIRMWARE is the stem all variables should have. Sorry, it was not
clear enough in my previous reply, and the check-package hint is a bit
misleading... I'll send a patch to make it clearer.

[0] https://buildroot.org/downloads/manual/manual.html#_tips_and_tricks

Since ZYNQMP_FIRMWARE_PMUFW is used only once, is there really a need
for an intermediate variable to begin with?

No need to respin just for that, it can be fixed when applying...

Regards,
Yann E. MORIN.

> +define ZYNQMP_FIRMWARE_INSTALL_IMAGES_CMDS
> +	$(INSTALL) -D -m 0755 $(ZYNQMP_FIRMWARE) $(BINARIES_DIR)/pmufw.elf
> +endef
> +
> +$(eval $(generic-package))
> -- 
> 2.25.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot