[ptxdist] [PATCH v3 0/3] Add support for Rockchip firmware blobs

[ptxdist] [PATCH v3 0/3] Add support for Rockchip firmware blobs

[Michael Riesch]

Hi all,

The v3 of this series addresses the review by Michael Olbrich
(shell coding style, description in Kconfig).

Best regards,
Michael

Michael Riesch (3):
  platforms: add section for non-free firmware blobs
  add package for rockchip firmware binaries
  barebox: add integration of firmware blobs

 platforms/Kconfig                      | 10 ++++
 platforms/barebox.in                   | 35 ++++++++++++
 platforms/firmware-rockchip.in         | 41 ++++++++++++++
 rules/barebox.make                     |  6 +++
 rules/firmware-rockchip.make           | 74 ++++++++++++++++++++++++++
 rules/post/ptxd_make_world_inject.make | 19 +++++++
 scripts/lib/ptxd_make_world_inject.sh  | 42 +++++++++++++++
 7 files changed, 227 insertions(+)
 create mode 100644 platforms/firmware-rockchip.in
 create mode 100644 rules/firmware-rockchip.make
 create mode 100644 rules/post/ptxd_make_world_inject.make
 create mode 100644 scripts/lib/ptxd_make_world_inject.sh

-- 
2.30.2


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

[ptxdist] [PATCH v3 1/3] platforms: add section for non-free firmware blobs

[Michael Riesch]

Add a dedicated section for non-free firmware binary blobs, such
as SDRAM initialization binaries.

Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
---
 platforms/Kconfig | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/platforms/Kconfig b/platforms/Kconfig
index 0bfe34cdc..047ac2b13 100644
--- a/platforms/Kconfig
+++ b/platforms/Kconfig
@@ -40,3 +40,13 @@ source "generated/hosttools_noprompt.in"
 source "generated/hosttools_platform.in"
 
 source "generated/platform_project_specific.in"
+
+menuconfig NON_FREE_FIRMWARE
+	bool
+	prompt "non-free firmware blobs       "
+	help
+	  Enable integration of non-free firmware blobs.
+
+if NON_FREE_FIRMWARE
+source "generated/non_free_firmware.in"
+endif
-- 
2.30.2


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

[ptxdist] [PATCH v3 2/3] add package for rockchip firmware binaries

[Michael Riesch]

Rockchip provides the SDRAM initialization as well as BL31/BL32
firmware for their SoCs in binary form. Add a package that downloads
those binaries for further use in e.g., barebox.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
[revised and extended Kconfig]
Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
---
 platforms/firmware-rockchip.in | 41 +++++++++++++++++++
 rules/firmware-rockchip.make   | 74 ++++++++++++++++++++++++++++++++++
 2 files changed, 115 insertions(+)
 create mode 100644 platforms/firmware-rockchip.in
 create mode 100644 rules/firmware-rockchip.make

diff --git a/platforms/firmware-rockchip.in b/platforms/firmware-rockchip.in
new file mode 100644
index 000000000..aa702416d
--- /dev/null
+++ b/platforms/firmware-rockchip.in
@@ -0,0 +1,41 @@
+## SECTION=non_free_firmware
+
+menuconfig FIRMWARE_ROCKCHIP
+	bool
+	prompt "Rockchip firmware binaries    "
+	help
+	  Rockchip prebuilt SDRAM initialization and TF-A binaries.
+
+if FIRMWARE_ROCKCHIP
+
+config FIRMWARE_ROCKCHIP_RK3566_SDRAM
+	bool
+	prompt "RK3566 SDRAM init"
+	help
+	  Rockchip RK3566 SDRAM initialization binary.
+	  Compatible with e.g., the Pine64 Quartz64 Model A board.
+
+config FIRMWARE_ROCKCHIP_RK3568_SDRAM
+	bool
+	prompt "RK3568 SDRAM init"
+	help
+	  Rockchip RK3568 SDRAM initialization binary.
+	  Compatible with e.g., the Rockchip RK3568 EVB1 board.
+
+config FIRMWARE_ROCKCHIP_RK356x_BL31
+	bool
+	prompt "RK356x BL31 binary"
+	help
+	  Rockchip RK3566/RK3568 BL31 binary.
+	  Compatible with e.g., the Pine64 Quartz64 Model A board
+	  and the Rockchip RK3568 EVB1 board.
+
+config FIRMWARE_ROCKCHIP_RK356x_BL32
+	bool
+	prompt "RK356x BL32 binary"
+	help
+	  Rockchip RK3566/RK3568 BL32 binary.
+	  Compatible with e.g., the Pine64 Quartz64 Model A board
+	  and the Rockchip RK3568 EVB1 board.
+
+endif
diff --git a/rules/firmware-rockchip.make b/rules/firmware-rockchip.make
new file mode 100644
index 000000000..dd50c865a
--- /dev/null
+++ b/rules/firmware-rockchip.make
@@ -0,0 +1,74 @@
+# -*-makefile-*-
+#
+# Copyright (C) 2021 by Michael Tretter <m.tretter@pengutronix.de>
+#
+# For further information about the PTXdist project and license conditions
+# see the README file.
+#
+
+#
+# We provide this package
+#
+PACKAGES-$(PTXCONF_FIRMWARE_ROCKCHIP) += firmware-rockchip
+
+FIRMWARE_ROCKCHIP_VERSION	:= 2021-06-01-g7d631e0d
+FIRMWARE_ROCKCHIP_MD5		:= 4ca62f76ca75019dc708c4cb7cc31b0a
+FIRMWARE_ROCKCHIP		:= firmware-rockchip-$(FIRMWARE_ROCKCHIP_VERSION)
+FIRMWARE_ROCKCHIP_SUFFIX	:= zip
+FIRMWARE_ROCKCHIP_URL		:= https://github.com/rockchip-linux/rkbin/archive/$(FIRMWARE_ROCKCHIP_VERSION).$(FIRMWARE_ROCKCHIP_SUFFIX)
+FIRMWARE_ROCKCHIP_SOURCE	:= $(SRCDIR)/$(FIRMWARE_ROCKCHIP).$(FIRMWARE_ROCKCHIP_SUFFIX)
+FIRMWARE_ROCKCHIP_DIR		:= $(BUILDDIR)/$(FIRMWARE_ROCKCHIP)
+FIRMWARE_ROCKCHIP_LICENSE	:= proprietary
+
+# ----------------------------------------------------------------------------
+# Prepare
+# ----------------------------------------------------------------------------
+
+FIRMWARE_ROCKCHIP_CONF_TOOL := NO
+
+# ----------------------------------------------------------------------------
+# Compile
+# ----------------------------------------------------------------------------
+
+$(STATEDIR)/firmware-rockchip.compile:
+	@$(call targetinfo)
+	@$(call touch)
+
+# ----------------------------------------------------------------------------
+# Install
+# ----------------------------------------------------------------------------
+
+$(STATEDIR)/firmware-rockchip.install:
+	@$(call targetinfo)
+
+ifdef PTXCONF_FIRMWARE_ROCKCHIP_RK3566_SDRAM
+	install -v -D -m644 $(FIRMWARE_ROCKCHIP_DIR)/bin/rk35/rk3566_ddr_1056MHz_v1.08.bin \
+		$(FIRMWARE_ROCKCHIP_PKGDIR)/usr/lib/firmware/rk3566_ddr_1056MHz_v1.08.bin
+endif
+
+ifdef PTXCONF_FIRMWARE_ROCKCHIP_RK3568_SDRAM
+	install -v -D -m644 $(FIRMWARE_ROCKCHIP_DIR)/bin/rk35/rk3568_ddr_1560MHz_v1.08.bin \
+		$(FIRMWARE_ROCKCHIP_PKGDIR)/usr/lib/firmware/rk3568_ddr_1560MHz_v1.08.bin
+endif
+
+ifdef PTXCONF_FIRMWARE_ROCKCHIP_RK356x_BL31
+	install -v -D -m644 $(FIRMWARE_ROCKCHIP_DIR)/bin/rk35/rk3568_bl31_v1.24.elf \
+		$(FIRMWARE_ROCKCHIP_PKGDIR)/usr/lib/firmware/rk3568_bl31_v1.24.elf
+endif
+
+ifdef PTXCONF_FIRMWARE_ROCKCHIP_RK356x_BL32
+	install -v -D -m644 $(FIRMWARE_ROCKCHIP_DIR)/bin/rk35/rk3568_bl32_v1.05.bin \
+		$(FIRMWARE_ROCKCHIP_PKGDIR)/usr/lib/firmware/rk3568_bl32_v1.05.bin
+endif
+
+	@$(call touch)
+
+# ----------------------------------------------------------------------------
+# Target-Install
+# ----------------------------------------------------------------------------
+
+$(STATEDIR)/firmware-rockchip.targetinstall:
+	@$(call targetinfo)
+	@$(call touch)
+
+# vim: syntax=make
-- 
2.30.2


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

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

[Michael Riesch]

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.

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.
+
+config BAREBOX_FIRMWARE_FILES
+	string "firmware blob file(s)"
+	default "<vendorblob>.bin:firmware"
+	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.
+
 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

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

[Michael Olbrich]

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

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

[Michael Riesch]

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

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

[Michael Olbrich]

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

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

[Michael Riesch]

Hello Michael,

Thanks for the detailed response.

On 12/17/21 8:35 AM, Michael Olbrich wrote:
> 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.

OK! I'll come up with a v4 soon.

Best regards,
Michael

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

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