Re: [Buildroot] [PATCH v11 1/6] boot/zynqmp-firmware: new boot firmware

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

Neal, All,

I had a quick look, and there is something I don;t really like...

On 2024-03-04 07:41 +0000, 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.
[--SNIP--]
> diff --git a/boot/zynqmp-firmware/Config.in b/boot/zynqmp-firmware/Config.in
> new file mode 100644
> index 0000000000..d68ec2cf6a
> --- /dev/null
> +++ b/boot/zynqmp-firmware/Config.in
> @@ -0,0 +1,22 @@
> +config BR2_TARGET_ZYNQMP_FIRMWARE
> +	bool "zynqmp-firmware"
> +	select BR2_TOOLCHAIN_BARE_METAL_BUILDROOT

This and [0]...

[--SNIP--]
> diff --git a/boot/zynqmp-firmware/zynqmp-firmware.mk b/boot/zynqmp-firmware/zynqmp-firmware.mk
> new file mode 100644
> index 0000000000..383d888fba
> --- /dev/null
> +++ b/boot/zynqmp-firmware/zynqmp-firmware.mk
> @@ -0,0 +1,33 @@
[--SNIP--]
> +ZYNQMP_PMUFW = $(@D)/lib/sw_apps/zynqmp_pmufw/src/executable.elf

    $ ./utils/docker-run make check-package
    boot/zynqmp-firmware/zynqmp-firmware.mk:27: possible typo: ZYNQMP_PMUFW -> *ZYNQMP_FIRMWARE*

[--SNIP--]
> diff --git a/toolchain/toolchain-bare-metal-buildroot/Config.in b/toolchain/toolchain-bare-metal-buildroot/Config.in
> index 6fe969357f..7d33a68c5c 100644
> --- a/toolchain/toolchain-bare-metal-buildroot/Config.in
> +++ b/toolchain/toolchain-bare-metal-buildroot/Config.in
> @@ -2,6 +2,7 @@ if BR2_TOOLCHAIN_BARE_METAL_BUILDROOT
>  
>  config BR2_TOOLCHAIN_BARE_METAL_BUILDROOT_ARCH
>  	string "architecture tuple"
> +	default "microblazeel-xilinx-elf" if BR2_TARGET_ZYNQMP_FIRMWARE

...[0] this are smelly to me.

It wokrs in your specific case, because all you do is target a
microblazeel-xilinx-elf and zynqmp-firmware is the first package that
depends on a bare-metal toolchain to be built [2]

But nothign prevents other situations:

  - user manually changes BR2_TOOLCHAIN_BARE_METAL_BUILDROOT_ARCH to
    another tuple, e.g. arm-gnu

  - another package does sopmething similar with another tuple, like
    riscv64-elf

The bare-metal toolchain can currently be built only for one and only
one tuple, so this can't work nicely.

Instead, I think we should go another rooute:

    config BR2_TARGET_ZYNQMP_FIRMWARE
        bool "zynqmp-firmware"
        depends on BR2_TOOLCHAIN_BARE_METAL_BUILDROOT_TUPLE = "microblazeel-xilinx-elf"

    comment "zynqmp-firmware needs a bare metal toolchain for tuple microblazeel-xilinx-elf"
        depends on BR2_TOOLCHAIN_BARE_METAL_BUILDROOT_TUPLE != "microblazeel-xilinx-elf"

[2] Note that we already have a few pre-built bare-metal toolchains:
    - package/arm-gnu-toolchain/
    package/riscv64-elf-toolchain/
    So it is not unconceivalbe that at some point we will want to be
    able to actually build suhc toolchains...

Regards,
Yann E. MORIN.

>  	help
>  	  This option allows to define the architecture tuple for the
>  	  bare-metal toolchain.
> -- 
> 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