Re: [ptxdist] [PATCH v4 4/4] barebox: add integration of firmware blobs

[Sascha Hauer <sha@pengutronix.de>]

On Fri, Jan 21, 2022 at 08:54:49AM +0100, Michael Olbrich wrote:
> On Mon, Jan 10, 2022 at 07:39:20AM +0100, Michael Riesch wrote:
> > On 1/7/22 3:09 PM, Michael Olbrich wrote:
> > > On Mon, Dec 20, 2021 at 01:08:57PM +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 (in
> > >> separate rule files) which are injected in the barebox source
> > >> directory during preparation.
> > >>
> > >> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
> > >> ---
> > >>  platforms/barebox.in          |  8 ++++++++
> > >>  platforms/barebox.rockchip.in | 10 ++++++++++
> > >>  rules/barebox.make            |  4 ++++
> > >>  rules/barebox.rockchip.make   | 16 ++++++++++++++++
> > >>  4 files changed, 38 insertions(+)
> > >>  create mode 100644 platforms/barebox.rockchip.in
> > >>  create mode 100644 rules/barebox.rockchip.make
> > >>
> > >> diff --git a/platforms/barebox.in b/platforms/barebox.in
> > >> index d35d16501..8a81be40f 100644
> > >> --- a/platforms/barebox.in
> > >> +++ b/platforms/barebox.in
> > >> @@ -55,6 +55,14 @@ config BAREBOX_CONFIG
> > >>  	  This entry specifies the .config file used to compile
> > >>  	  barebox.
> > >>  
> > >> +menuconfig BAREBOX_FIRMWARE
> > >> +	bool
> > >> +	prompt "integrate firmware blobs      "
> > >> +
> > >> +if BAREBOX_FIRMWARE
> > >> +source "generated/barebox_firmware.in"
> > >> +endif
> > > 
> > > If you split out the generic parts and add empty files with just the
> > > "## SECTION=" header (for both new sections) then we could maybe get that
> > > merged while we figure out how to handle the rockchip part.
> > 
> > OK, I'll consider this in v5.
> > 
> > >> +
> > >>  config BAREBOX_EXTRA_ENV
> > >>  	prompt "extend the builtin barebox environment"
> > >>  	bool
> > >> diff --git a/platforms/barebox.rockchip.in b/platforms/barebox.rockchip.in
> > >> new file mode 100644
> > >> index 000000000..238798ce1
> > >> --- /dev/null
> > >> +++ b/platforms/barebox.rockchip.in
> > >> @@ -0,0 +1,10 @@
> > >> +## SECTION=barebox_firmware
> > >> +
> > >> +config BAREBOX_NEEDS_FIRMWARE_ROCKCHIP
> > >> +	prompt "barebox needs firmware-rockchip"
> > >> +	bool
> > >> +	depends on ARCH_ARM64
> > >> +	select FIRMWARE_ROCKCHIP if BAREBOX_NEEDS_FIRMWARE_ROCKCHIP
> > >> +	help
> > >> +	  Select this if barebox needs the non-free Rockchip firmware
> > >> +	  blobs.
> > >> diff --git a/rules/barebox.make b/rules/barebox.make
> > >> index bea9f3adc..cc70a6ff5 100644
> > >> --- a/rules/barebox.make
> > >> +++ b/rules/barebox.make
> > >> @@ -94,6 +94,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/barebox.rockchip.make b/rules/barebox.rockchip.make
> > >> new file mode 100644
> > >> index 000000000..33795bee4
> > >> --- /dev/null
> > >> +++ b/rules/barebox.rockchip.make
> > >> @@ -0,0 +1,16 @@
> > >> +# -*-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.
> > >> +#
> > >> +
> > >> +ifdef PTXCONF_BAREBOX_NEEDS_FIRMWARE_ROCKCHIP
> > >> +
> > >> +BAREBOX_INJECT_PATH	+= ${PTXDIST_SYSROOT_TARGET}/usr/lib/firmware
> > > 
> > > This should be in barebox.make. BAREBOX_INJECT_PATH is a ':' separated
> > > path, so expanding it like this doesn't work anyways.
> > 
> > Will fix this in v5!
> > 
> > > So in general I've been thinking which parts make sense in PTXdist upstream
> > > and which parts should be in the BSPs. I mean it doesn't really scale, if
> > > we add firmware files for all boards into PTXdist upstream. So I had the
> > > vague idea that we could add files that are SoC specific to PTXdist and
> > > leave the board specific files in the BSP. 
> > 
> > Not sure whether this is a SoC-specific/board-specific issue, but I
> > agree that there are a manifold of possibilities here and naturally we
> > cannot cover everything. I would say the aim here is to provide some
> > generic means to integrate binary blobs into packages and (optionally)
> > provide some reasonable defaults. As you stated previously, the defaults
> > can be overwritten if desired.
> > 
> > > Then I saw this:
> > > 
> > >> +BAREBOX_INJECT_FILES	+= rk3568_ddr_1560MHz_v1.08.bin:arch/arm/boards/rockchip-rk3568-evb/sdram-init.bin
> > > 
> > > My understanding was that this file is the code that does the SDRAM timing
> > > calibration. So I expected it to be SoC specific and therefore in a more
> > > generic path, not the board directory.
> > > Am I missing something here?
> > 
> > As far as I understand, the binary contains the SDRAM setup (rows, cols,
> > etc.) which may be board specific. But you may want to talk to Sascha
> > for more details.
> 
> Sascha, can comment on this? I think it would be nice to provide any binary
> blobs that can be used by multiple board in PTXdist upstream. However, for
> that to work we would need a generic location in the barebox source tree.
> Any reason why a board specific location is used here?

The files are board specific so it felt natural to put them in a board
specific location.

Anyway, having a single directory where such files are located seems to
be a good idea. Not only that it helps external build systems like
ptxdist, but we could also create tarballs or repositories with binary
blobs which could be taken as-is and the files wouldn't have to be
copied to special locations in the barebox tree.

> 
> > >> +BAREBOX_INJECT_FILES	+= rk3568_bl31_v1.24.elf:firmware/rk3568-bl31.bin
> > > 
> > > That's TF-A right?
> > 
> > Yes.
> > 
> > >> +BAREBOX_INJECT_FILES	+= rk3568_bl32_v1.05.bin:firmware/rk3568-op-tee.bin
> > > 
> > > I expected these files to be board specific, but they are in a generic
> > > directory? Also, I would expect those to be built from source.
> > 
> > Again, Sascha is the expert here. For all I know those are SoC-specific
> > and barebox expects them to reside at this particular location.
> 
> Sascha, same here, just the reverse.

OP-TEE files are not necessarily board specific. Often enough they are,
because things like serial ports are hardcoded, but in theory OP-TEE
could take a device tree and may not even be SoC specific. I think the
current OP-TEE codebase makes it hard to actually create board or SoC
agnostic images, but who knows how things develop in the future.

> 
> > Indeed one may want to generate these binaries from the mainline
> > sources. However, at the moment those sources are not available for
> > RK356x. Rockchip tends to provide only a binary blob when a new SoC is
> > released, the sources follow about 1-2 years later.
> 
> And if you need to make board specific changes? :-/

I guess you know the answer :)

Generally I like the idea of having a single directory with binary blobs
and other firmware files. This would also allow us to let barebox take
board specific blobs which could point to more generic files using
symlinks.
I think ptxdist could provide ways to either extract specific files from
a tarball or to let the user choose to build certain things like OP-TEE
or TF-A himself.

Sounds like some work for cleaning that up in barebox, but doable and
worthwile.

Sascha

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