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

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

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