Re: [ptxdist] [PATCH v3 3/3] barebox: add integration of firmware blobs

[Michael Riesch <michael.riesch@wolfvision.net>]

Hello Michael,

On 12/10/21 4:03 PM, Michael Olbrich wrote:
> On Thu, Dec 09, 2021 at 07:10:49AM +0100, Michael Riesch wrote:
>> In some cases barebox requires firmware blobs, which may be
>> provided in binary form by the vendor or compiled in a
>> preceding step. Add the possibility to specify files which
>> are injected in the barebox source directory during
>> preparation.
> 
> So, I've been thinking about this some more. And I think this needs a
> different approach.
> 
> With SDRAM calibration and TF-A and stuff like that, there will be a lot of
> blobs that need to be integrated into barebox.
> And it will be different for each vendor and possibly SOC family. And maybe
> a upstream and a downstream version.
> 
> Modifying barebox.in for each of them will not scale. Especially because in
> most cases we'll carry a modified version until it's merged into ptxdist
> master and the BSP is updated to the new version.
> 
> And from what I've seen with TF-A, it's not uncommon to start with some
> ugly downstream stuff and switch to upstream later.
> 
> So I'd like this to be more flexible. More below.

Agreed. While I don't fully understand yet your alternative approach
(questions below), it seems way more elegant.

It seems to me that the first two patches are not affected, though.
Provided there are no other objections, could they be merged so we can
focus on the third patch under discussion?

> 
>> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
>> ---
>>  platforms/barebox.in                   | 35 +++++++++++++++++++++
>>  rules/barebox.make                     |  6 ++++
>>  rules/post/ptxd_make_world_inject.make | 19 ++++++++++++
>>  scripts/lib/ptxd_make_world_inject.sh  | 42 ++++++++++++++++++++++++++
>>  4 files changed, 102 insertions(+)
>>  create mode 100644 rules/post/ptxd_make_world_inject.make
>>  create mode 100644 scripts/lib/ptxd_make_world_inject.sh
>>
>> diff --git a/platforms/barebox.in b/platforms/barebox.in
>> index d35d16501..63e9929e5 100644
>> --- a/platforms/barebox.in
>> +++ b/platforms/barebox.in
>> @@ -15,6 +15,7 @@ menuconfig BAREBOX
>>  	select HOST_IMX_CST if BAREBOX_NEEDS_HOST_IMX_CST
>>  	select HOST_LZOP if BAREBOX_NEEDS_HOST_LZOP
>>  	select CODE_SIGNING if BAREBOX_NEEDS_KEYS
>> +	select FIRMWARE_ROCKCHIP if BAREBOX_NEEDS_FIRMWARE_ROCKCHIP
>>  	prompt "barebox                       "
>>  	bool
>>  	help
>> @@ -55,6 +56,32 @@ config BAREBOX_CONFIG
>>  	  This entry specifies the .config file used to compile
>>  	  barebox.
>>  
>> +menuconfig BAREBOX_FIRMWARE
>> +	bool
>> +	prompt "integrate firmware blobs      "
>> +
>> +if BAREBOX_FIRMWARE
>> +config BAREBOX_FIRMWARE_PATH
>> +	string "path(s) to firmware blobs"
>> +	default "${PTXDIST_SYSROOT_TARGET}/usr/lib/firmware"
>> +	help
>> +	  Define path to the firmware blob(s). Multiple directories can
>> +	  be specified separated by ':'. A relative path will be expanded relative
>> +	  to the workspace and all other layers. Only one of the specified paths
>> +	  can be a relative path.
> 
> I think we can just define a fixed directory for this. For exceptions, and
> absolute file path will work as well.
> 
>> +config BAREBOX_FIRMWARE_FILES
>> +	string "firmware blob file(s)"
>> +	default "<vendorblob>.bin:firmware"
> 
> I don't like having this in the config. The problem ist, that the user will
> need to know, which files to copy and where to.
> 
>> +	help
>> +	  Select the firmware blob(s) to be integrated into the barebox
>> +	  source before compilation. Each entry consists of <file>:<target>,
>> +	  where <target> is an optional path relative to the barebox source
>> +	  directory. Multiple entries can be specified, separated by spaces.
>> +
>> +endif
>> +
>>  config BAREBOX_EXTRA_ENV
>>  	prompt "extend the builtin barebox environment"
>>  	bool
>> @@ -146,4 +173,12 @@ config BAREBOX_NEEDS_HOST_LZOP
>>  	  lzop is used in order to compile lzop for your development
>>  	  host.
>>  
>> +config BAREBOX_NEEDS_FIRMWARE_ROCKCHIP
>> +	prompt "barebox needs firmware-rockchip"
>> +	bool
>> +	depends on ARCH_ARM64
>> +	help
>> +	  Select this if barebox needs the non-free Rockchip firmware
>> +	  blobs.
> 
> As I said, I don't want to modify barebox.in for each vendor / SOC family.
> 
> So how about something like this:
> 
> ------------------------------------
> menu "firmware blobs"
> 
> source barebox_firmware.in
> 
> endmenu
> ------------------------------------

Right, this enters barebox.in and provides a slot to plug one or more
barebox-firmware-xyz.in into the config.
The @$(call world/inject, BAREBOX) is added to the barebox.make rule
file and considers BAREBOX_INJECT_FILES.

> And the firmware package will have an extra menu file, e.g.
> barebox-firmware-rockchip.in:
> 
> ------------------------------------
> ## SECTION=barebox_firmware
> 
> config BAREBOX
> 	select FIRMWARE_ROCKCHIP if BAREBOX_NEEDS_FIRMWARE_ROCKCHIP
> 
> config BAREBOX_NEEDS_FIRMWARE_ROCKCHIP
> 	prompt "barebox needs firmware-rockchip"
> 	...
> 
> ------------------------------------
> Maybe some suboptions for individual SoCs / boards.

The barebox-firmware-rockchip.in as example of such a config plugin.

> And an extra rule file, e.g. barebox.rockchip.make:
> ------------------------------------
> 
> BAREBOX_INJECT_FILES += source-file:target-file
> 
> ------------------------------------

Finally, we need a rule file that adjusts the BAREBOX_INJECT_FILES. So
far I get it.

In ptxdist there is a lot of implicit magic going on behind the scenes.
So what guarantees that the barebox.rockchip.make is considered before
barebox.make (or the barebox prepare stage, to be precise)? Is the
naming "barebox.rockchip.make" relevant (it resembles a custom stage of
the barebox rule)?

It seems that usually the rule files have a corresponding *.in file.
Would it make sense to align the names barebox-firmware-rockchip.in and
barebox.rockchip.make in some way?

Do we need some explicit dependencies in the barebox-firmware-rockchip.in?

Best regards,
Michael

> Maybe with some conditions depending on the suboptions.
> And if absolutely necessary, the source-file could expand to an absolute
> path here.
> 
> Once the core support is upstream in ptxdist. These small drop-ins can be
> easily maintained in a BSP but are also easier to upstream because there
> are no conflicting changes to barebox.in.
> 
> And with the latest ptxdist version, <pkg>.<something>.make can be
> overwritten in the BSP just like the regular rule file. So if the upstream
> version of such a drop-in has a bug, fixing that in the BSP is possible.
> 
> Michael
> 
>> +
>>  endif
>> diff --git a/rules/barebox.make b/rules/barebox.make
>> index bea9f3adc..a81fc86b3 100644
>> --- a/rules/barebox.make
>> +++ b/rules/barebox.make
>> @@ -26,6 +26,8 @@ BAREBOX_BUILD_DIR	:= $(BAREBOX_DIR)-build
>>  BAREBOX_LICENSE		:= GPL-2.0-only
>>  BAREBOX_DEVPKG		:= NO
>>  BAREBOX_BUILD_OOT	:= KEEP
>> +BAREBOX_INJECT_PATH	:=$(call remove_quotes,$(PTXCONF_BAREBOX_FIRMWARE_PATH))
>> +BAREBOX_INJECT_FILES	:=$(call remove_quotes,$(PTXCONF_BAREBOX_FIRMWARE_FILES))
>>  
>>  BAREBOX_CONFIG		:= $(call ptx/in-platformconfigdir, \
>>  		$(call remove_quotes, $(PTXCONF_BAREBOX_CONFIG)))
>> @@ -94,6 +96,10 @@ ifdef PTXCONF_BAREBOX_EXTRA_ENV
>>  	@rm -rf $(BAREBOX_BUILD_DIR)/defaultenv/barebox_default_env
>>  endif
>>  
>> +ifdef PTXCONF_BAREBOX_FIRMWARE
>> +	@$(call world/inject, BAREBOX)
>> +endif
>> +
>>  	@$(call touch)
>>  
>>  # ----------------------------------------------------------------------------
>> diff --git a/rules/post/ptxd_make_world_inject.make b/rules/post/ptxd_make_world_inject.make
>> new file mode 100644
>> index 000000000..b7d28e92f
>> --- /dev/null
>> +++ b/rules/post/ptxd_make_world_inject.make
>> @@ -0,0 +1,19 @@
>> +# -*-makefile-*-
>> +#
>> +# Copyright (C) 2021 by Michael Riesch <michael.riesch@wolfvision.net>
>> +#
>> +# For further information about the PTXdist project and license conditions
>> +# see the README file.
>> +#
>> +
>> +world/inject/env = \
>> +	$(call world/env, $(1)) \
>> +	pkg_inject_path="$($(1)_INJECT_PATH)" \
>> +	pkg_inject_files="$($(1)_INJECT_FILES)" \
>> +	pkg_source="$($(1)_DIR)"
>> +
>> +world/inject = \
>> +	$(call world/inject/env,$(strip $(1))) \
>> +	ptxd_make_world_inject
>> +
>> +# vim: syntax=make
>> diff --git a/scripts/lib/ptxd_make_world_inject.sh b/scripts/lib/ptxd_make_world_inject.sh
>> new file mode 100644
>> index 000000000..c2e45ad42
>> --- /dev/null
>> +++ b/scripts/lib/ptxd_make_world_inject.sh
>> @@ -0,0 +1,42 @@
>> +#!/bin/bash
>> +#
>> +# Copyright (C) 2021 by Michael Riesch <michael.riesch@wolfvision.net>
>> +#
>> +# For further information about the PTXdist project and license conditions
>> +# see the README file.
>> +#
>> +
>> +ptxd_make_inject() {
>> +    local source target
>> +
>> +    source="$(echo ${inject_file} | cut -d ":" -f 1)"
>> +    target="${pkg_source}/$(echo ${inject_file} | cut -d ":" -f 2)"
>> +    if [ -z "${target}" ]; then
>> +	target="${source}"
>> +    fi
>> +
>> +    if [[ "${source}" =~ ^/.* ]]; then
>> +	ptxd_bailout "'${source}' must not be an absolute path!" \
>> +	    "Use <PKG>_INJECT_PATH to specify the search path."
>> +    fi
>> +
>> +    if ! ptxd_in_path pkg_inject_path "${source}"; then
>> +	ptxd_bailout "Blob '${source}' not found in '${pkg_inject_path}'."
>> +    fi
>> +    source="${ptxd_reply}"
>> +
>> +    echo -e "\nInject file $(ptxd_print_path ${source}) into" \
>> +	 "$(ptxd_print_path ${target})..."
>> +    cp ${source} ${target}
>> +}
>> +export -f ptxd_make_inject
>> +
>> +
>> +ptxd_make_world_inject() {
>> +    ptxd_make_world_init || break
>> +
>> +    for inject_file in ${pkg_inject_files}; do
>> +	ptxd_make_inject || break
>> +    done
>> +}
>> +export -f ptxd_make_world_inject
>> -- 
>> 2.30.2
>>
>>
>> _______________________________________________
>> ptxdist mailing list
>> ptxdist@pengutronix.de
>> To unsubscribe, send a mail with subject "unsubscribe" to ptxdist-request@pengutronix.de
>>
> 

_______________________________________________
ptxdist mailing list
ptxdist@pengutronix.de
To unsubscribe, send a mail with subject "unsubscribe" to ptxdist-request@pengutronix.de