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

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

On Fri, Jan 21, 2022 at 09:43:41AM +0100, Sascha Hauer wrote:
> 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.

Ok, makes sense. I just wasn't sure if they are board specific.

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

But the TF-A is always board specific, right? Anyways, I'd say separate
location for each board make more sense here. A symlink or copy is easy
when you actually want to share the same file. But a patch is needed when
you need different files and two boards use the same path.

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

We're looking at what to do in PTXdist right now. Currently, the idea is
that one package dumps all files somewhere in sysroot. Then we have some
minimal barebox integration that adds the individual files with source and
destination to a list. And in the barebox prepare stage all files in that
list are copied into the barebox source tree.

Just dumping all files from a tarball into the barebox source tree would be
easier, but I don't think its feasible. Just look at the current case: We
have an upstream Tarball with fixed names that include versions. So we need
to do some mapping somewhere anyways.

But having consistent locations where to put the files would make writing
the mappings easier.

Michael

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