diff mbox series

[v6,04/10] toolchain/toolchain-bare-metal-buildroot: new toolchain

Message ID 20231020114236.4129636-4-neal.frager@amd.com
State Changes Requested
Headers show
Series [v6,01/10] package/binutils-bare-metal: new package | expand

Commit Message

Neal Frager Oct. 20, 2023, 11:42 a.m. UTC 
This patch adds a new virtual package for adding a bare-metal toolchain to
buildroot.  By default, this package will configure a bare-metal toolchain
for the Xilinx microblaze little endian architecture.  When configured for
the Xilinx microblaze architecture, this toolchain can be used to build the
microblaze firmware applications for zynqmp and versal.

Signed-off-by: Ibai Erkiaga <ibai.erkiaga-elorza@amd.com>
Signed-off-by: Neal Frager <neal.frager@amd.com>
---
V1->V2:
 - adds select option to bring in all packages needed for toolchain-bare-metal
V2->V3:
 - no changes
V3->V4:
 - moved from package to toolchain directory
 - improved menuconfig help comment
V4->V5:
 - no changes
V5->V6:
 - migrated to toolchain-bare-metal-buildroot
---
 DEVELOPERS                                    |  2 ++
 toolchain/Config.in                           |  1 +
 .../toolchain-bare-metal-buildroot/Config.in  | 21 +++++++++++++++++++
 .../toolchain-bare-metal-buildroot.mk         |  7 +++++++
 4 files changed, 31 insertions(+)
 create mode 100644 toolchain/toolchain-bare-metal-buildroot/Config.in
 create mode 100644 toolchain/toolchain-bare-metal-buildroot/toolchain-bare-metal-buildroot.mk

Comments

Luca Ceresoli Oct. 26, 2023, 4:07 p.m. UTC | #1 
On Fri, 20 Oct 2023 12:42:30 +0100
Neal Frager <neal.frager@amd.com> wrote:

> This patch adds a new virtual package for adding a bare-metal toolchain to
> buildroot.  By default, this package will configure a bare-metal toolchain
> for the Xilinx microblaze little endian architecture.  When configured for
> the Xilinx microblaze architecture, this toolchain can be used to build the
> microblaze firmware applications for zynqmp and versal.
> 
> Signed-off-by: Ibai Erkiaga <ibai.erkiaga-elorza@amd.com>
> Signed-off-by: Neal Frager <neal.frager@amd.com>

[Tested on Kria KV260 starter kit]
Tested-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
Thomas Petazzoni Oct. 31, 2023, 12:51 p.m. UTC | #2 
Hello,

On Fri, 20 Oct 2023 12:42:30 +0100
Neal Frager via buildroot <buildroot@buildroot.org> wrote:


> diff --git a/toolchain/Config.in b/toolchain/Config.in
> index d8081f1b9d..b1333f92f0 100644
> --- a/toolchain/Config.in
> +++ b/toolchain/Config.in
> @@ -59,6 +59,7 @@ config BR2_TOOLCHAIN_EXTERNAL
>  
>  endchoice
>  
> +source "toolchain/toolchain-bare-metal-buildroot/Config.in"
>  source "toolchain/toolchain-buildroot/Config.in"
>  source "toolchain/toolchain-external/Config.in"

I'm not really happy with where this ends up showing in menuconfig.
Indeed, in menuconfig, we end up seeing something like this:

    Toolchain type (External toolchain)  --->
[*] host toolchain-bare-metal
-*-   host binutils-bare-metal
-*-   host gcc-bare-metal
-*-   host newlib-bare-metal
    *** Toolchain External Options ***
    Toolchain (Bootlin toolchains)  --->

So basically, right in the middle of the settings for the "normal"
toolchain, we have those options about the bare-metal toolchain.

Maybe something like this instead:

diff --git a/toolchain/Config.in b/toolchain/Config.in
index b1333f92f0..278d1de65c 100644
--- a/toolchain/Config.in
+++ b/toolchain/Config.in
@@ -59,7 +59,6 @@ config BR2_TOOLCHAIN_EXTERNAL
 
 endchoice
 
-source "toolchain/toolchain-bare-metal-buildroot/Config.in"
 source "toolchain/toolchain-buildroot/Config.in"
 source "toolchain/toolchain-external/Config.in"
 
@@ -919,4 +918,8 @@ config BR2_TOOLCHAIN_HAS_LIBQUADMATH
        default y if BR2_i386 || BR2_x86_64
        default y if BR2_POWERPC_CPU_HAS_VSX
 
+comment "Bare metal toolchain"
+
+source "toolchain/toolchain-bare-metal-buildroot/Config.in"
+
 endmenu

Basically, move it at the end of toolchain/Config.in, with a small
Config.in comment separating it from the rest.

>  
> diff --git a/toolchain/toolchain-bare-metal-buildroot/Config.in b/toolchain/toolchain-bare-metal-buildroot/Config.in
> new file mode 100644
> index 0000000000..6ebf2cdf86
> --- /dev/null
> +++ b/toolchain/toolchain-bare-metal-buildroot/Config.in
> @@ -0,0 +1,21 @@
> +config BR2_PACKAGE_HOST_TOOLCHAIN_BARE_METAL_BUILDROOT
> +	bool "host toolchain-bare-metal"
> +	select BR2_PACKAGE_HOST_BINUTILS_BARE_METAL
> +	select BR2_PACKAGE_HOST_GCC_BARE_METAL
> +	select BR2_PACKAGE_HOST_NEWLIB_BARE_METAL

Those selects are no longer needed, as all those options will be gone.

> +	help
> +	  Build a bare-metal toolchain in addition to the main Linux toolchain
> +
> +if BR2_PACKAGE_HOST_TOOLCHAIN_BARE_METAL_BUILDROOT
> +
> +config BR2_PACKAGE_HOST_TOOLCHAIN_BARE_METAL_BUILDROOT_ARCH
> +	string

This option needs to be made visible (with a prompt), otherwise nobody
can chose a different architecture/tuple.

> +	default "microblazeel-xilinx"

Is "xilinx" really relevant here? I don't think it is. I'm also
wondering whether having a default value actually makes sense. It
should probably just stay empty by default, and be defined by the
specific defconfigs that need this?

> +	help
> +	  select architecture for bare-metal toolchain
> +
> +source "package/binutils-bare-metal/Config.in.host"
> +source "package/gcc-bare-metal/Config.in.host"
> +source "package/newlib-bare-metal/Config.in.host"

These file inclusions can be dropped.

> +++ b/toolchain/toolchain-bare-metal-buildroot/toolchain-bare-metal-buildroot.mk
> @@ -0,0 +1,7 @@
> +################################################################################
> +#
> +# toolchain-bare-metal-buildroot
> +#
> +################################################################################
> +
> +(eval $(host-virtual-package))

I'm a bit rusty with virtual packages, but I don't see where this
virtual package pulls in host-newlib-bare-metal as a dependency. Don't
you need:

TOOLCHAIN_BARE_METAL_BUILDROOT_DEPENDENCIES = host-newlib-bare-metal

to make sure you trigger the build of host-newlib-bare-metal ->
host-gcc-bare-metal -> host-binutils-bare-metal ?

Best regards,

Thomas
Michaelis, Adam J Collins via buildroot Nov. 2, 2023, 9:47 a.m. UTC | #3 
Hi Thomas,


> Le 31 oct. 2023 à 06:51, Thomas Petazzoni <thomas.petazzoni@bootlin.com> a écrit :
> 
> Hello,
> 
>> On Fri, 20 Oct 2023 12:42:30 +0100
>> Neal Frager via buildroot <buildroot@buildroot.org> wrote:
>> 
>> 
>> diff --git a/toolchain/Config.in b/toolchain/Config.in
>> index d8081f1b9d..b1333f92f0 100644
>> --- a/toolchain/Config.in
>> +++ b/toolchain/Config.in
>> @@ -59,6 +59,7 @@ config BR2_TOOLCHAIN_EXTERNAL
>> 
>> endchoice
>> 
>> +source "toolchain/toolchain-bare-metal-buildroot/Config.in"
>> source "toolchain/toolchain-buildroot/Config.in"
>> source "toolchain/toolchain-external/Config.in"
> 
> I'm not really happy with where this ends up showing in menuconfig.
> Indeed, in menuconfig, we end up seeing something like this:
> 
>    Toolchain type (External toolchain)  --->
> [*] host toolchain-bare-metal
> -*-   host binutils-bare-metal
> -*-   host gcc-bare-metal
> -*-   host newlib-bare-metal
>    *** Toolchain External Options ***
>    Toolchain (Bootlin toolchains)  --->
> 
> So basically, right in the middle of the settings for the "normal"
> toolchain, we have those options about the bare-metal toolchain.
> 
> Maybe something like this instead:
> 
> diff --git a/toolchain/Config.in b/toolchain/Config.in
> index b1333f92f0..278d1de65c 100644
> --- a/toolchain/Config.in
> +++ b/toolchain/Config.in
> @@ -59,7 +59,6 @@ config BR2_TOOLCHAIN_EXTERNAL
> 
> endchoice
> 
> -source "toolchain/toolchain-bare-metal-buildroot/Config.in"
> source "toolchain/toolchain-buildroot/Config.in"
> source "toolchain/toolchain-external/Config.in"
> 
> @@ -919,4 +918,8 @@ config BR2_TOOLCHAIN_HAS_LIBQUADMATH
>        default y if BR2_i386 || BR2_x86_64
>        default y if BR2_POWERPC_CPU_HAS_VSX
> 
> +comment "Bare metal toolchain"
> +
> +source "toolchain/toolchain-bare-metal-buildroot/Config.in"
> +
> endmenu
> 
> Basically, move it at the end of toolchain/Config.in, with a small
> Config.in comment separating it from the rest.

Good idea.  I am implementing this for v7.

> 
>> 
>> diff --git a/toolchain/toolchain-bare-metal-buildroot/Config.in b/toolchain/toolchain-bare-metal-buildroot/Config.in
>> new file mode 100644
>> index 0000000000..6ebf2cdf86
>> --- /dev/null
>> +++ b/toolchain/toolchain-bare-metal-buildroot/Config.in
>> @@ -0,0 +1,21 @@
>> +config BR2_PACKAGE_HOST_TOOLCHAIN_BARE_METAL_BUILDROOT
>> +    bool "host toolchain-bare-metal"
>> +    select BR2_PACKAGE_HOST_BINUTILS_BARE_METAL
>> +    select BR2_PACKAGE_HOST_GCC_BARE_METAL
>> +    select BR2_PACKAGE_HOST_NEWLIB_BARE_METAL
> 
> Those selects are no longer needed, as all those options will be gone.

Yes, changes done for v7.  Except for the newlib-bare-metal package which will be a target package still requiring a select.

> 
>> +    help
>> +      Build a bare-metal toolchain in addition to the main Linux toolchain
>> +
>> +if BR2_PACKAGE_HOST_TOOLCHAIN_BARE_METAL_BUILDROOT
>> +
>> +config BR2_PACKAGE_HOST_TOOLCHAIN_BARE_METAL_BUILDROOT_ARCH
>> +    string
> 
> This option needs to be made visible (with a prompt), otherwise nobody
> can chose a different architecture/tuple.

Yes, you are right.  I am fixing this for v7.

> 
>> +    default "microblazeel-xilinx"
> 
> Is "xilinx" really relevant here? I don't think it is. I'm also
> wondering whether having a default value actually makes sense. It
> should probably just stay empty by default, and be defined by the
> specific defconfigs that need this?
> 

In my testing, binutils and gcc expect the arch name microblazeel-xilinx-elf with the xilinx included.

I will change the definition to the full tuple using microblazeel-xilinx-elf in v7.

As for the default, I am not sure we should get rid of having a default.  If the toolchain bare metal buildroot gets selected without an architecture defined, we will have build errors with binutils, gcc and newlib.

If it is ok with you, I would prefer to keep a default architecture definition, but making it visible, so users could configure a different architecture for another platform.

>> +    help
>> +      select architecture for bare-metal toolchain
>> +
>> +source "package/binutils-bare-metal/Config.in.host"
>> +source "package/gcc-bare-metal/Config.in.host"
>> +source "package/newlib-bare-metal/Config.in.host"
> 
> These file inclusions can be dropped.

Except for the newlib one, yes, the others will be dropped in v7.

> 
>> +++ b/toolchain/toolchain-bare-metal-buildroot/toolchain-bare-metal-buildroot.mk
>> @@ -0,0 +1,7 @@
>> +################################################################################
>> +#
>> +# toolchain-bare-metal-buildroot
>> +#
>> +################################################################################
>> +
>> +(eval $(host-virtual-package))
> 
> I'm a bit rusty with virtual packages, but I don't see where this
> virtual package pulls in host-newlib-bare-metal as a dependency. Don't
> you need:
> 
> TOOLCHAIN_BARE_METAL_BUILDROOT_DEPENDENCIES = host-newlib-bare-metal
> 
> to make sure you trigger the build of host-newlib-bare-metal ->
> host-gcc-bare-metal -> host-binutils-bare-metal ?

Yes, I am fixing all of the dependencies for v7, so all of the packages build in the correct order.

> 
> Best regards,
> 
> Thomas
> --
> Thomas Petazzoni, co-owner and CEO, Bootlin
> Embedded Linux and Kernel engineering and training
> https://bootlin.com

Thank you for your support!

Best regards,
Neal Frager
AMD
diff mbox series

Patch

diff --git a/DEVELOPERS b/DEVELOPERS
index 677b8f09bd..849a5263b5 100644
--- a/DEVELOPERS
+++ b/DEVELOPERS
@@ -1370,6 +1370,7 @@  N:	Ibai Erkiaga <ibai.erkiaga-elorza@amd.com>
 F:	package/binutils-bare-metal/
 F:	package/gcc-bare-metal/
 F:	package/newlib-bare-metal/
+F:	toolchain/toolchain-bare-metal-buildroot/
 
 N:	Ignacy Gawędzki <ignacy.gawedzki@green-communications.fr>
 F:	package/angularjs/
@@ -2207,6 +2208,7 @@  F:	package/bootgen/
 F:	package/gcc-bare-metal/
 F:	package/newlib-bare-metal/
 F:	package/versal-firmware/
+F:	toolchain/toolchain-bare-metal-buildroot/
 
 N:	Nicola Di Lieto <nicola.dilieto@gmail.com>
 F:	package/uacme/
diff --git a/toolchain/Config.in b/toolchain/Config.in
index d8081f1b9d..b1333f92f0 100644
--- a/toolchain/Config.in
+++ b/toolchain/Config.in
@@ -59,6 +59,7 @@  config BR2_TOOLCHAIN_EXTERNAL
 
 endchoice
 
+source "toolchain/toolchain-bare-metal-buildroot/Config.in"
 source "toolchain/toolchain-buildroot/Config.in"
 source "toolchain/toolchain-external/Config.in"
 
diff --git a/toolchain/toolchain-bare-metal-buildroot/Config.in b/toolchain/toolchain-bare-metal-buildroot/Config.in
new file mode 100644
index 0000000000..6ebf2cdf86
--- /dev/null
+++ b/toolchain/toolchain-bare-metal-buildroot/Config.in
@@ -0,0 +1,21 @@ 
+config BR2_PACKAGE_HOST_TOOLCHAIN_BARE_METAL_BUILDROOT
+	bool "host toolchain-bare-metal"
+	select BR2_PACKAGE_HOST_BINUTILS_BARE_METAL
+	select BR2_PACKAGE_HOST_GCC_BARE_METAL
+	select BR2_PACKAGE_HOST_NEWLIB_BARE_METAL
+	help
+	  Build a bare-metal toolchain in addition to the main Linux toolchain
+
+if BR2_PACKAGE_HOST_TOOLCHAIN_BARE_METAL_BUILDROOT
+
+config BR2_PACKAGE_HOST_TOOLCHAIN_BARE_METAL_BUILDROOT_ARCH
+	string
+	default "microblazeel-xilinx"
+	help
+	  select architecture for bare-metal toolchain
+
+source "package/binutils-bare-metal/Config.in.host"
+source "package/gcc-bare-metal/Config.in.host"
+source "package/newlib-bare-metal/Config.in.host"
+
+endif #BR2_PACKAGE_HOST_TOOLCHAIN_BARE_METAL_BUILDROOT
diff --git a/toolchain/toolchain-bare-metal-buildroot/toolchain-bare-metal-buildroot.mk b/toolchain/toolchain-bare-metal-buildroot/toolchain-bare-metal-buildroot.mk
new file mode 100644
index 0000000000..170c73aa31
--- /dev/null
+++ b/toolchain/toolchain-bare-metal-buildroot/toolchain-bare-metal-buildroot.mk
@@ -0,0 +1,7 @@ 
+################################################################################
+#
+# toolchain-bare-metal-buildroot
+#
+################################################################################
+
+(eval $(host-virtual-package))