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

[Michael Olbrich <m.olbrich@pengutronix.de>]

On Sat, Dec 11, 2021 at 08:03:40AM +0100, Michael Riesch wrote:
> 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?

The first one cannot be applied alone. Creating a new section without
anything in it does not work. And I think the second one should should
include the barebox integration once we've decided how exactly that will
work.

> > 
> >> 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.

Yes, exactly.

> > 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.

yes.

> > 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)?

The naming is relevant insofar that this makes it possible to overwrite the
file in the BSP or another layer. Any other makefile that is not directly
linked to a package will always be included.

In general, the order in which the rules are read is random. For
<pkg>.*.make, they are explicitly read immediately after <pkg>.make

Expanding BAREBOX_INJECT_FILES like this relies on how make works: Any
variable used within the build commands of a target is evaluated when the
target is executed. So something like this works just fine:

my-target:
	echo $(message)

message = "Hello World!"

> 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?

Good idea. But the rule file must be called barebox.<something>.make. It
doesn't matter what name the menu file has, so we can align it accordingly.

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

Barebox must select the firmare package to ensure the build order. I've
added that to my example above.

In Kconfig, you can declare the same Symbol multiple times. All instances
will be merged, so you just need to avoid conflicts. So:

config BAREBOX
	tristate

config BAREBOX
	prompt "barebox"

config BAREBOX
	select FIRMWARE_ROCKCHIP

has exactly the same effect as:

config BAREBOX
	tristate
	prompt "barebox"
	select FIRMWARE_ROCKCHIP

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
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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