Re: [Buildroot] [PATCH v2 2/4] boot/ti-k3-r5-loader: add new package

[Thomas Petazzoni via buildroot <buildroot@buildroot.org>]

Hello,

On Sun, 31 Jul 2022 02:54:09 +0200
Giulio Benetti <giulio.benetti@benettiengineering.com> wrote:


> > +config BR2_TARGET_TI_K3_R5_LOADER_BOARD
> > +	string "Board to configure for"
> > +	depends on BR2_TARGET_TI_K3_R5_LOADER
> > +	help
> > +	  Specify the board to configure the bootloader for.
> > +	  This should be the name of a board under board/ti
> > +	  For example, "am64x_evm".  
> 
> Here ^^^ I would substitute "config" with "choice", this way everything
> is easier from the user point of view. On patch 4/4 you're adding 2
> boards, so I think it makes sense to add every possible choice(2 for the
> moment).

I am afraid I will disagree here. For this type of option, we
definitely prefer to have a free-form string rather than an exhaustive
list of possible options. See U-Boot/Linux/Barebox and other packages
that have configurable defconfigs.

So please keep the string option.


> > +TI_K3_R5_LOADER_VERSION = 2022.10-rc1  
> 
> -rc1 version is the possibly buggiest version you can pick. There are
> other 2 possible solutions:
> 1. use 2022.07 and backport all needed patches on a dedicated
> repository instead of using official u-boot repository
> 2. wait a bit for at least rc2/3(soon) and later when 2022.10 is
> released, bump it

I'd say it's fine for the time being to have -rc1, with the assumption
that as soon as 2022.10 is out, we bump to it. After all, this package
is very specific to TI boards, so if this -rc1 has been tested as
working in this particular context, that's fine.

> > +TI_K3_R5_LOADER_SITE = https://ftp.denx.de/pub/u-boot
> > +TI_K3_R5_LOADER_SOURCE = u-boot-$(TI_K3_R5_LOADER_VERSION).tar.bz2
> > +TI_K3_R5_LOADER_LICENSE = GPL-2.0+
> > +TI_K3_R5_LOADER_LICENSE_FILES = Licenses/gpl-2.0.txt
> > +TI_K3_R5_LOADER_CPE_ID_VENDOR = denx
> > +TI_K3_R5_LOADER_CPE_ID_PRODUCT = u-boot
> > +TI_K3_R5_LOADER_INSTALL_IMAGES = YES
> > +TI_K3_R5_LOADER_DEPENDENCIES = \
> > +	host-pkgconf \
> > +	$(BR2_MAKE_HOST_DEPENDENCY) \  
> 
> What is this ^^^ needed for?

I guess this is mainly because it's copy/pasted from uboot.mk, and the
explanation is:

# u-boot 2020.01+ needs make 4.0+

> > +	host-arm-gnu-toolchain
> > +
> > +TI_K3_R5_LOADER_MAKE = $(BR2_MAKE)  
> 
> This ^^^ looks superflous, you can directly use $(BR2_MAKE) below

This is also modeled after uboot.mk, though I would agree with you.

> > +TI_K3_R5_LOADER_KCONFIG_DEPENDENCIES = \
> > +	toolchain \
> > +	$(BR2_MAKE_HOST_DEPENDENCY) \
> > +	$(BR2_BISON_HOST_DEPENDENCY) \
> > +	$(BR2_FLEX_HOST_DEPENDENCY)  
> 
> "toolchain" should imply all above _HOST_DEPENDENCY. But here you're
> using host-arm-gnu-toolchain, so toolchain shouldn't be needed, or yes?

Again, this is modeled after uboot.mk. But indeed, here the toolchain
dependency does not make sense, since CROSS_COMPILE points to the
toolchain installed by host-arm-gnu-toolchain. So here, we should
replace "toolchain" by "host-arm-gnu-toolchain".

> > +TI_K3_R5_LOADER_BOARD = $(call qstrip,$(BR2_TARGET_TI_K3_R5_LOADER_BOARD))  
> 
> This ^^^ can be avoided too since you use it one line below with a
> suffix only

I think it's fine, as it makes the following line more readable.

> > +TI_K3_R5_LOADER_KCONFIG_DEFCONFIG = $(TI_K3_R5_LOADER_BOARD)_r5_defconfig
> > +TI_K3_R5_LOADER_MAKE_OPTS += \
> > +	CROSS_COMPILE=$(HOST_ARM_GNU_TOOLCHAIN_INSTALL_DIR)/bin/arm-none-eabi- \
> > +	ARCH=arm  
> 
> What is the reason why you need to use arm-gnu-toolchain to build u-boot 
> SPL? Can you please explain it in commit log?

This has been explained already in previous iterations of the patch
series.

This package is about building a special U-Boot, which targets a
Cortex-R5 core, that acts as a kind of "co-processor". Since the main
processor is an ARM64 core, and an ARM64 toolchain can only be 64-bit
code, we need a separate toolchain to be able to build ARM 32-bit code
for the Cortex-R5 core.

> > +define TI_K3_R5_LOADER_BUILD_CMDS
> > +	$(TI_K3_R5_LOADER_MAKE) -C $(@D) $(TI_K3_R5_LOADER_MAKE_OPTS)
> > +endef
> > +
> > +define TI_K3_R5_LOADER_INSTALL_IMAGES_CMDS
> > +	cp $(@D)/spl/u-boot-spl.bin $(BINARIES_DIR)/r5-u-boot-spl.bin
> > +endef
> > +
> > +$(eval $(kconfig-package))  
> 
> Why do you use kconfig-package? You reimplement anyway BUILD_CMDS and
> INSTALL_IMAGES_CMDS, so generic-package should be fine.

kconfig-package is special, it does not implement BUILD_CMDS,
INSTALL_TARGET_CMDS or INSTALL_IMAGES_CMDS. Look at uboot.mk, linux.mk,
busybox.mk and others that use kconfig-package. kconfig-package only
provides logic for the configuration step, with xxx-menuconfig,
xxx-xconfig, xxx-savedefconfig targets and all, but implementing the
BUILD_CMDS and others CMDS variables is left to the package .mk file.

Best regards,

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot