Re: [Buildroot] [PATCH v1 1/2] package/zynqmp-firmware: new package

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

Hi Neal,

On Mon, 27 Mar 2023 05:45:45 +0000
"Frager, Neal" <neal.frager@amd.com> wrote:

> Hi Thomas,
> 
> > Le 26 mars 2023 à 22:05, Thomas Petazzoni <thomas.petazzoni@bootlin.com> a écrit :
> > 
> > Hello Neal,
> >   
> >> On Sun, 26 Mar 2023 15:09:48 +0100
> >> Neal Frager via buildroot <buildroot@buildroot.org> wrote:
> >> 
> >> This patch adds a new package to buildroot for building the zynqmp pmufw
> >> with the requirement that the user must provide an external microblaze
> >> toolchain.  
> > 
> > This is not really nice :-/ Was this the conclusion of the discussion
> > we had on this topic?  
> 
> I would not say this is the conclusion of our discussion, but rather a next step.  Once we have a microblaze compiler package, we can update the zynqmp-firmware package to use the included compiler by default or allow users to select an external one, in case they prefer to use the Xilinx distributed compiler.
> 
> >   
> >> +config BR2_PACKAGE_ZYNQMP_FIRMWARE_KRIA_K26
> >> +    bool "kria-k26"
> >> +    help
> >> +      Adds additional CFLAGS for Kria K26 SOMs.  
> > 
> > This doesn't look super extensible. Why are these CFLAGS needed in
> > particular for this platform? Is this a per-SoC or per-board
> > configuration? Do we expect more platforms to need custom CFLAGS?  
> 
> These CFLAGS are needed for the k26 SOMs, so we will need to include them for the kv260 and kr260 defconfigs.  I agree with you that custom flags are not so nice, and hopefully Xilinx will find a better solution for this in future versions.
> 
> For now, I think we have to live with it.
>
> >   
> >> +ZYNQMP_FIRMWARE_VERSION = $(call qstrip,$(BR2_PACKAGE_ZYNQMP_FIRMWARE_VERSION))
> >> +ZYNQMP_FIRMWARE_SOURCE = xilinx_$(ZYNQMP_FIRMWARE_VERSION).tar.gz
> >> +ZYNQMP_FIRMWARE_SITE = https://github.com/Xilinx/embeddedsw/archive/refs/tags  
> > 
> > Use $(call github,...) instead
> >   
> 
> Ok
> 
> >> +ifeq ($(BR2_PACKAGE_ZYNQMP_FIRMWARE_KRIA_K26),y)
> >> +ZYNQMP_CFLAGS = "-Os -flto -ffat-lto-objects -DBOARD_SHUTDOWN_PIN=2 -DBOARD_SHUTDOWN_PIN_STATE=0 \
> >> +        -DENABLE_EM -DENABLE_MOD_OVERTEMP -DENABLE_DYNAMIC_MIO_CONFIG -DENABLE_IOCTL \
> >> +        -DCONNECT_PMU_GPO_2_VAL=0"  
> > 
> > All these CFLAGS are really weird... why isn't this part of the
> > zynqmp-firmware build system itself?

Given you (Neal) are working on this, and you are already active in fixing
some issues in the embeddedsw repository (which is greatly appreciated
here!), why not trying to have a proper solution in there in the first
place?

Fixing things upstream can possibly take time, but in the end it will avoid
adding cruft&hacks in various places, of which I can count 3 now: yocto,
zynqmp-pmufw-builder, and hopefully soon Buildroot.

> >> +else
> >> +ZYNQMP_CFLAGS = "-Os -flto -ffat-lto-objects"
> >> +endif  
> > 
> > In any case, this should be:
> > 
> > ZYNQMP_CFLAGS = -Os -flto -ffat-lto-objects
> > 
> > ifeq ($(BR2_PACKAGE_ZYNQMP_FIRMWARE_KRIA_K26),y)
> > ZYNQMP_CFLAGS += \
> >    -DBOARD_SHUTDOWN_PIN=2 \
> >    -DBOARD_SHUTDOWN_PIN_STATE=0 \
> >    -DENABLE_EM \
> >    -DENABLE_MOD_OVERTEMP \
> >    -DENABLE_DYNAMIC_MIO_CONFIG \
> >    -DENABLE_IOCTL \
> >    -DCONNECT_PMU_GPO_2_VAL=0
> > endif
> >   
> 
> Ok
> 
> >> +
> >> +define ZYNQMP_FIRMWARE_BUILD_CMDS
> >> +    cd $(@D)/lib/sw_apps/zynqmp_pmufw/src && make \
> >> +        COMPILER=$(ZYNQMP_FIRMWARE_TOOLCHAIN_PATH)/$(ZYNQMP_FIRMWARE_TOOLCHAIN_PREFIX)gcc \
> >> +        ARCHIVER=$(ZYNQMP_FIRMWARE_TOOLCHAIN_PATH)/$(ZYNQMP_FIRMWARE_TOOLCHAIN_PREFIX)gcc-ar \
> >> +        CC=$(ZYNQMP_FIRMWARE_TOOLCHAIN_PATH)/$(ZYNQMP_FIRMWARE_TOOLCHAIN_PREFIX)gcc \
> >> +        CFLAGS=$(ZYNQMP_CFLAGS)  
> > 
> > Please use:
> > 
> >    $(MAKE) -C $(@D)/lib/sw_apps/zynqmp_pmufw/src \
> >        ...
> >   

Even better: should you use $(MAKE1) here, would it be possible to get
rid of the 'make -j1' patch?

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot