Re: [Buildroot] [PATCH v3 1/6] package/binutils-bare-metal: new package

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

Hi Neal,

On Fri, 22 Sep 2023 13:34:58 +0000
"Frager, Neal" <neal.frager@amd.com> wrote:

> Hi Luca,
> 
> > Hello Neal, Ibai,  
> 
> > thank you for your persistence in working on this!  
> 
> > The overall patch set appears pretty clean, except for a few remarks as you can read in this and the other replies.   
> 
> Thank you for the kind words.
> 
> > I had a small hiccup while trying to apply your patches using 'git am' from my inbox:  
> 
> >   error: cannot convert from y to UTF-8  
> 
> > This is probably due to this weird header value:  
> 
> >   Content-Type: text/plain; charset="y"  
> 
> > Probably some dirt in your git config. However the mbox file as downloaded from patchwork did apply without issues. I don't think you need to resend the series just for this.  
> 
> I will create a clean patch set with v4, so this should hopefully disappear.
> 
> 
> > This patch adds a new package for building binutils for a bare-metal toolchain.
> > The cpu architecture is defined by a toolchain-bare-metal virtual package.
> > While any cpu architecture could be used, the default configuration 
> > will be a Xilinx microblaze little endian architecture, so that 
> > buildroot will be able to build the microblaze firmware applications for zynqmp and versal.
> > 
> > When configured for the Xilinx microblaze architecture, all of the 
> > binutils patches that are applied to the Xilinx distributed toolchain 
> > will be applied in order to generate a toolchain that is equivalent to what Xilinx distributes.
> > 
> > Signed-off-by: Ibai Erkiaga-Elorza <ibai.erkiaga-elorza@amd.com>
> > Signed-off-by: Neal Frager <neal.frager@amd.com>  
> 
> ...
> 
> > diff --git a/package/binutils-bare-metal/Config.in.host 
> > b/package/binutils-bare-metal/Config.in.host
> > new file mode 100644
> > index 0000000000..036698d418
> > --- /dev/null
> > +++ b/package/binutils-bare-metal/Config.in.host
> > @@ -0,0 +1,19 @@
> > +config BR2_PACKAGE_HOST_BINUTILS_BARE_METAL
> > +	bool "host binutils-bare-metal"
> > +	help
> > +	  binutils-bare-metal is a host utility for a
> > +	  bare-metal toolchain  
> 
> > "a host utility seems a bit of an understatement, I'd rather say "Build to GNU binutils for a bare-metal toolchain" to clarify this is building no less than the GNU binutils.  
> 
> I agree.  I will change this in v4.
> 
> > +
> > +if BR2_PACKAGE_HOST_BINUTILS_BARE_METAL
> > +
> > +config BR2_PACKAGE_HOST_BINUTILS_BARE_METAL_VERSION
> > +	string
> > +	default "2.39"
> > +
> > +config BR2_PACKAGE_HOST_BINUTILS_BARE_METAL_EXTRA_CONFIG_OPTIONS
> > +	string "Additional binutils options"
> > +	default ""
> > +	help
> > +	  Any additional binutils options you may want to include  
> 
> > Do we really want this without a valid use case?  
> 
> > The same question could apply to the _VERSION setting, however I feel it's reasonable to keep it...  
> 
> > This applies to patches 2 and 3 as well.  
> 
> My thought was that someone might want to add additional bare-metal options aside from the Xilinx microblaze target.
> 
> If not building binutils, gcc or newlib for microblaze, users might want to change the version and the config options used.
> 
> > diff --git a/package/binutils-bare-metal/binutils-bare-metal.mk 
> > b/package/binutils-bare-metal/binutils-bare-metal.mk
> > new file mode 100644
> > index 0000000000..fd983abc93
> > --- /dev/null
> > +++ b/package/binutils-bare-metal/binutils-bare-metal.mk
> > @@ -0,0 +1,56 @@
> > +#####################################################################
> > +###########
> > +#
> > +# binutils-bare-metal
> > +#
> > +#####################################################################
> > +###########
> > +
> > +HOST_BINUTILS_BARE_METAL_VERSION = $(call 
> > +qstrip,$(BR2_PACKAGE_HOST_BINUTILS_BARE_METAL_VERSION))
> > +ifeq ($(HOST_BINUTILS_BARE_METAL_VERSION),)
> > +HOST_BINUTILS_BARE_METAL_VERSION = 2.39 endif # BINUTILS_VERSION
> > +
> > +HOST_BINUTILS_BARE_METAL_SITE ?= $(BR2_GNU_MIRROR)/binutils 
> > +HOST_BINUTILS_BARE_METAL_SOURCE ?= 
> > +binutils-$(HOST_BINUTILS_BARE_METAL_VERSION).tar.xz
> > +
> > +HOST_BINUTILS_BARE_METAL_LICENSE = GPL-3.0+, libiberty LGPL-2.1+ 
> > +HOST_BINUTILS_BARE_METAL_LICENSE_FILES = COPYING3 COPYING.LIB 
> > +HOST_BINUTILS_BARE_METAL_CPE_ID_VENDOR = gnu
> > +
> > +HOST_BINUTILS_BARE_METAL_DEPENDENCIES = host-zlib
> > +
> > +# if toolchain is for microblazeel-xilinx, apply Xilinx patch set 
> > +ifeq 
> > +($(BR2_PACKAGE_HOST_TOOLCHAIN_BARE_METAL_ARCH),"microblazeel-xilinx")  
> 
> > The toolchain-bare-metal is added in patch 4, thus this series as-is is not bisectable. I don't see any obvious solutions as toolchain-bare-metal selects binutils-bare-metal so I guess some maintainer can provide some hints on how to handle this.  
> 
> > The same issue applies to patches 2 and 3.  
> 
> Ok, I am not sure what to do with this feedback.

I'd say waiting for Buildroot maintainers feedback.

> > +HOST_BINUTILS_BARE_METAL_EXTRA_DOWNLOADS = 
> > +https://github.com/Xilinx/meta-xilinx/archive/refs/tags/xlnx-rel-v202
> > +3.1.tar.gz  
> 
> > This is not a huge download (500 kB compressed, 5.6 MB  uncompressed), but we use the binutils patches only, which account for 10% of the whole archive. I wonder whether there is a way to download only a subdirectory from github.  
> 
> > And looking at the patches themselves, I wonder how many are actually needed. At a cursory look, some don't really look like production code.
> > Are those changes being mainlined?  
> 
> Our initial plan was to make binutils and gcc versions that match the Xilinx software release as closely as possible.  This is why we just took the patch set blindly.
> 
> From my view, it is easier for me to maintain this if we either take all of the patches as is or none at all.

Agreed it is simpler to maintain, so this boils down to the question:
are those patches being actively mainlined? I have a sad feeling about
it as the number and overall quality of those patches seems the same I
saw more than an year ago.

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