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

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

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.

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

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.

And an extra rule file, e.g. barebox.rockchip.make:
------------------------------------

BAREBOX_INJECT_FILES += source-file:target-file

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

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