El Tue, Jul 26, 2022 at 11:08:21AM +0200, Quentin Schulz deia:
>
> You could also have a fit,align = <8>; property instead of hardcoding it.
I'm not sure the fit entry in binman heeds this property, may have
overlooked something. I'd have to try it.
> The issue is that this flag seems to be added only for u-boot.itb and
> fit-dtb.blob. I assume there are usecases outside of those two binaries
> where the user does not want the fit header to be aligned (or don't need
> it).
The commit message said that the device tree spec requires 4 or 8 byte
alignment, so maybe all fits want it because all fits are device trees
? Not sure.
> > > + configurations {
> > > + default = "@config_DEFAULT-SEQ";
> > > +
> > > + @config_SEQ {
> > > + description = "NAME.dtb";
> > > + fdt = "fdt_SEQ";
> > > + firmware = "atf_1";
> > > + loadables = "uboot","atf_2","atf_3";
>
> This section will need some more love with some ifdef for ATF_SPL and TEE.
>
I'm sending a patch below that adds a couple of configuration properties to
binman so that split-elf can fill the properties. How many segments are
in bl31.elf or optee is not something that we have in CONFIGs, I think,
so it may be difficult to catch all cases with ifdefs.
It doesn't have to be this patch, or these properties, but maybe better
something like this than ifdefs ? Also missing tests...
>
> This is unfortunately not possible since binman parallelizes the build of
> images in the binman DT node. This means there is no guarantee the
> u-boot.itb file is generated before this section is worked on by binman. Or
> maybe I misunderstood the docs.
>
> But good progress, I guess this phandle thing "just" needs to be fixed to
> have something nice (both for this patch series and mine).
>
I'm sending another patch below "fixing" the phandle issue, but it's a
dirty hack without too much thought given. It could still fail if threads
try to read data from the image of another thread before it's ready or
something.
Only lightly tested with binman -T 0, not parallel.
It seems to run mkimage -B 8 -E -t -F ./itb.fit.fit SIX times each time
I run binman. Not sure why.
But it boots.
I wonder if we shouldn't just run binman several times from make instead of
once at the end,
have make run it once for each file we want to create, so that we can reuse
u-boot.itb for both
the MMC and the SPI image. We could have one .dts for each image, so when make
want u-boot.itb it runs binman -a of-list="rk3399-rock-pi-4b.dts
rockchip-itb-u-boot.dts"
And in this rockchip-itb-u-boot.dts there's only
/binman {
itb: itb {
filename = "u-boot.itb";
fit {
filename = "u-boot.itb";
...
};
};
}
Then when it wants u-boot-rockchip.bin it runs binman -a
of-list="rk3399-rock-pi-4b.dts rockchip-rksd-u-boot.dts"
And in this rockchip-rksd-u-boot.dts there's only
/binman {
simple-bin {
filename = "u-boot-rockchip.bin";
pad-byte = <0xff>;
mkimage {
args = "-n", CONFIG_SYS_SOC, "-T", "rksd";
...
};
blob {
filename = "u-boot.itb";
...
};
...
};
};
Then when make wants u-boot-rockchip-spi.bin it runs binman -a
of-list="rk3399-rock-pi-4b.dts rockchip-rkspi-u-boot.dts"
And in this rockchip-rkspi-u-boot.dts there's only
/binman {
simple-bin-spi {
filename = "u-boot-rockchip-spi.bin";
pad-byte = <0xff>;
mkimage {
args = "-n", CONFIG_SYS_SOC, "-T", "rkspi";
...
};
blob {
filename = "u-boot.itb";
...
};
...
};
};
I don't know if it's possible or if reading so many .dts so many times would be
slower.
But dependencies between binaries seems more a job for make than binman.
Anyway, what I've tried so far:
rockchip-u-boot.dtsi:
// SPDX-License-Identifier: GPL-2.0+
/*
* Copyright (C) 2019 Jagan Teki <ja...@amarulasolutions.com>
*/
#include <config.h>
/ {
binman: binman {
multiple-images;
};
};
#ifdef CONFIG_SPL
&binman {
#ifndef CONFIG_USE_SPL_FIT_GENERATOR
itb: itb {
filename = "u-boot.itb";
fit {
filename = "u-boot.itb";
description = "U-Boot FIT";
fit,fdt-list = "of-list";
fit,external-offset=<0>;
images {
uboot {
description = "U-Boot (64-bit)";
type = "standalone";
os = "U-Boot";
arch = "arm64";
compression = "none";
load = <CONFIG_SYS_TEXT_BASE>;
u-boot-nodtb {
};
};
#ifdef CONFIG_SPL_ATF
@atf_SEQ {
fit,operation = "split-elf";
description = "ARM Trusted Firmware";
type = "firmware";
arch = "arm64";
os = "arm-trusted-firmware";
compression = "none";
fit,load;
fit,entry;
fit,data;
atf-bl31 {
};
};
#endif
#ifdef CONFIG_TEE
@tee_SEQ {
fit,operation = "split-elf";
description = "TEE";
type = "tee";
arch = "arm64";
os = "tee";
compression = "none";
fit,load;
fit,entry;
fit,data;
tee-os {
};
};
#endif
@fdt_SEQ {
description = "NAME.dtb";
type = "flat_dt";
compression = "none";
};
};
configurations {
default = "@config_DEFAULT-SEQ";
@config_SEQ {
description = "NAME.dtb";
fdt = "fdt_SEQ";
fit,firmware = "atf_1";
fit,prepend-to-loadables = "uboot";
fit,loadables;
};
};
};
};
#endif
simple-bin {
filename = "u-boot-rockchip.bin";
pad-byte = <0xff>;
mkimage {
args = "-n", CONFIG_SYS_SOC, "-T", "rksd";
#ifndef CONFIG_TPL
u-boot-spl {
};
};
#else
u-boot-tpl {
};
};
u-boot-spl {
};
#endif
#ifdef CONFIG_ARM64
#ifdef CONFIG_USE_SPL_FIT_GENERATOR
blob {
filename = "u-boot.itb";
#else
collection {
content = <&itb>;
#endif
#else
u-boot-img {
#endif
offset = <((CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR -
64) * 512)>;
};
};
#ifdef CONFIG_ROCKCHIP_SPI_IMAGE
simple-bin-spi {
filename = "u-boot-rockchip-spi.bin";
pad-byte = <0xff>;
mkimage {
args = "-n", CONFIG_SYS_SOC, "-T", "rkspi";
#ifdef CONFIG_TPL
multiple-data-files;
u-boot-tpl {
};
#endif
u-boot-spl {
};
};
#ifdef CONFIG_ARM64
#ifdef CONFIG_USE_SPL_FIT_GENERATOR
blob {
filename = "u-boot.itb";
#else
collection {
content = <&itb>;
#endif
#else
u-boot-img {
#endif
/* Sync with u-boot,spl-payload-offset if present */
offset = <CONFIG_SYS_SPI_U_BOOT_OFFS>;
};
};
#endif
};
#endif
>From 0ccdd5a27d86a697ae15aaeae2c54bf574928074 Mon Sep 17 00:00:00 2001
From: Xavier Drudis Ferran <xdru...@tinet.cat>
Date: Tue, 26 Jul 2022 15:31:18 +0200
Subject: [PATCH 1/2] Let entry collection pull contents from a different
image.
This is a quick and dirty change to binman to let a section of an
image reference the content of another image or a section of it. Or it
could also reference an entry in a different section of the same
image. The only forbidden use is to reference itself, a descendent
section or an ascendent section, brothers, cousins, and further
relatives are fair game.
It's very little tested, and if it ever is wanted should be better
written possibly.
In my tests it built an MMC image that boots my Rock Pi 4B, but it
seems to build u-boot.itb 3 times with the same content.
I wonder if it wouldn't be better to use binman for a sigle image at
each execution, and let make schedule the calls like it was just a
compiler or linker, so that if the MMC and SPI each include u-boot.itb
we don't have to build u-boot.itb 3 times. The drawback is that we
would be parsing the device tree 3 times.
---
arch/arm/dts/rockchip-u-boot.dtsi | 15 +++++++-
tools/binman/control.py | 10 +++--
tools/binman/etype/collection.py | 64 ++++++++++++++++++++++++++++++-
tools/binman/etype/section.py | 2 +-
4 files changed, 82 insertions(+), 9 deletions(-)
diff --git a/arch/arm/dts/rockchip-u-boot.dtsi
b/arch/arm/dts/rockchip-u-boot.dtsi
index 5a613650f5..cd775ccae8 100644
--- a/arch/arm/dts/rockchip-u-boot.dtsi
+++ b/arch/arm/dts/rockchip-u-boot.dtsi
@@ -13,7 +13,8 @@
#ifdef CONFIG_SPL
&binman {
- itb {
+#ifndef CONFIG_USE_SPL_FIT_GENERATOR
+ itb: itb {
filename = "u-boot.itb";
fit {
filename = "u-boot.itb";
@@ -82,6 +83,7 @@
};
};
};
+#endif
simple-bin {
filename = "u-boot-rockchip.bin";
pad-byte = <0xff>;
@@ -102,8 +104,13 @@
#endif
#ifdef CONFIG_ARM64
+#ifdef CONFIG_USE_SPL_FIT_GENERATOR
blob {
filename = "u-boot.itb";
+#else
+ collection {
+ content = <&itb>;
+#endif
#else
u-boot-img {
#endif
@@ -129,9 +136,13 @@
};
#ifdef CONFIG_ARM64
+#ifdef CONFIG_USE_SPL_FIT_GENERATOR
blob {
filename = "u-boot.itb";
-
+#else
+ collection {
+ content = <&itb>;
+#endif
#else
u-boot-img {
#endif
diff --git a/tools/binman/control.py b/tools/binman/control.py
index ce57dc7efc..bb33199b42 100644
--- a/tools/binman/control.py
+++ b/tools/binman/control.py
@@ -19,6 +19,7 @@ from binman import cbfs_util
from binman import elf
from patman import command
from patman import tout
+from binman.etype import section
# These are imported if needed since they import libfdt
state = None
@@ -47,14 +48,15 @@ def _ReadImageDesc(binman_node, use_expanded):
"""
# For Image()
# pylint: disable=E1102
- images = OrderedDict()
+
binman_section=section.Entry_section.Create(None,binman_node,etype='section')
if 'multiple-images' in binman_node.props:
for node in binman_node.subnodes:
- images[node.name] = Image(node.name, node,
+ binman_section._entries[node.name] = Image(node.name, node,
use_expanded=use_expanded)
+ binman_section._entries[node.name].binman_section = binman_section
else:
- images['image'] = Image('image', binman_node,
use_expanded=use_expanded)
- return images
+ binman_section._entries['image'] = Image('image', binman_node,
use_expanded=use_expanded)
+ return binman_section._entries
def _FindBinmanNode(dtb):
"""Find the 'binman' node in the device tree
diff --git a/tools/binman/etype/collection.py b/tools/binman/etype/collection.py
index 442b40b48b..5149576adb 100644
--- a/tools/binman/etype/collection.py
+++ b/tools/binman/etype/collection.py
@@ -11,6 +11,26 @@ import os
from binman.entry import Entry
from dtoc import fdt_util
+def PathStartsWith(path, start):
+ """Does path start with start regarding whole path components?
+
+ PathStartsWith("/foo/bar","/foo/bar") => True
+ PathStartsWith("/foo/bar/baz","/foo/bar") => True
+ PathStartsWith("/foo/bar/baz","/foo/bar/") => True
+ PathStartsWith("/foo/bar/baz/","/foo/bar") => True
+ PathStartsWith("/foo/bar/baz","/foo/bar/") => True
+ PathStartsWith("/foo/barbaz","/foo/bar") => False
+ PathStartsWith("/foo/bar","") => True
+ PathStartsWith("/foo/bar","/foo/bar") => True
+ PathStartsWith("/foo/bar","/foo/bar/") => True
+ PathStartsWith("/foo/bar","/foo/bar//") => False
+ PathStartsWith("/foo/bar/","/foo/bar") => True
+ """
+ return ((path.startswith(start) and (path == start or
+ path[len(start)] == '/' or
+ start.endswith('/'))
+ ) or (path == start+"/"))
+
class Entry_collection(Entry):
"""An entry which contains a collection of other entries
@@ -28,6 +48,47 @@ class Entry_collection(Entry):
if not self.content:
self.Raise("Collection must have a 'content' property")
+ def GetContentsByPhandle(self, phandle, required):
+ """Get the contents of an entry that may not be a direct sibling
+ Args:
+ required: True if the data must be present, False if it is OK to
+ return None
+
+ Returns:
+ bytes content of the entry
+ """
+ node = self._node.GetFdt().LookupPhandle(phandle)
+ if not node:
+ self.Raise("Cannot find node for phandle %d" % phandle)
+ if PathStartsWith(node.path, self._node.path):
+ self.Raise("Cannot reference self or descendant nodes with phandle
%d for %s"
+ % (phandle, node.path))
+ if PathStartsWith(self._node.path, node.path):
+ self.Raise("Cannot reference ascendant nodes with phandle %d for
%s"
+ % (phandle, node.path))
+ sec = self.section;
+ while sec:
+ if PathStartsWith(node.path, sec._node.path):
+ if node.path == sec._node.path:
+ return sec.GetData(required)
+ else:
+ for entry in sec._entries.values():
+ if entry._node.path == node.path:
+ return entry.GetData(required)
+ else:
+ if (PathStartsWith(node.path, entry._node.path)
+ and entry is Entry_section):
+ sec = entry #try child
+ break
+ else: # exit while if no sibling matches
+ sec = None
+ else: # try parent
+ if sec.section is None and sec.binman_section:
+ sec=sec.binman_section
+ else:
+ sec=sec.section
+ self.Raise("Cannot find entry for phandle %d" % phandle)
+
def GetContents(self, required):
"""Get the contents of this entry
@@ -42,8 +103,7 @@ class Entry_collection(Entry):
self.Info('Getting contents, required=%s' % required)
data = bytearray()
for entry_phandle in self.content:
- entry_data = self.section.GetContentsByPhandle(entry_phandle, self,
- required)
+ entry_data = self.GetContentsByPhandle(entry_phandle, required)
if not required and entry_data is None:
self.Info('Contents not available yet')
# Data not available yet
diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py
index bd67238b91..e943f27a6d 100644
--- a/tools/binman/etype/section.py
+++ b/tools/binman/etype/section.py
@@ -744,7 +744,7 @@ class Entry_section(Entry):
Returns:
Image object containing this section
"""
- if not self.section:
+ if not self.section or self.section._node.path == '/binman':
return self
return self.section.GetImage()
--
2.20.1
>From 084dd2932115421f8171468f687e06314c01fef8 Mon Sep 17 00:00:00 2001
From: Xavier Drudis Ferran <xdru...@tinet.cat>
Date: Tue, 26 Jul 2022 20:10:46 +0200
Subject: [PATCH 2/2] New configuration properties for binman's split-elf
(fit,firmware and fit,prepend-to-loadables).
Allow more flexibility in the configuration entry of the FIT images
generated by binman when using split-elf.
The generated sections listed atf-1 as loadable and u-boot as
firmware, instead of the other way round like make_fit_atf.py
did. With these new properties, the dts can determine the old
behaviour or that of make-fit-atf.py.
The behaviour is still not the same as make_fit_atf.py when
there are TEE segoments. make_fit_atf.py didn't include them
in the loadables and original binman did. Which is right ?
Or do we need another configuration property yet ?
Signed-off-by: Xavier Drudis Ferran <xdru...@tinet.cat>
---
arch/arm/dts/rockchip-u-boot.dtsi | 5 ++--
tools/binman/entries.rst | 38 +++++++++++++++++++++++++++++
tools/binman/etype/fit.py | 40 +++++++++++++++++++++++++++----
3 files changed, 77 insertions(+), 6 deletions(-)
diff --git a/arch/arm/dts/rockchip-u-boot.dtsi
b/arch/arm/dts/rockchip-u-boot.dtsi
index cd775ccae8..f7c5602aee 100644
--- a/arch/arm/dts/rockchip-u-boot.dtsi
+++ b/arch/arm/dts/rockchip-u-boot.dtsi
@@ -77,8 +77,9 @@
@config_SEQ {
description = "NAME.dtb";
fdt = "fdt_SEQ";
- firmware = "atf_1";
- loadables = "uboot","atf_2","atf_3";
+ fit,firmware = "atf_1";
+ fit,prepend-to-loadables = "uboot";
+ fit,loadables;
};
};
};
diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
index 60c89aec59..e261bceb74 100644
--- a/tools/binman/entries.rst
+++ b/tools/binman/entries.rst
@@ -567,6 +567,9 @@ The top-level 'fit' node supports the following special
properties:
fit,external-offset
Indicates that the contents of the FIT are external and provides the
external offset. This is passed to mkimage via the -E and -p flags.
+ It can be 0 to just put the binaries after the header without padding.
+ A 0 value is different from the property being absent (then the
+ binaries would be internal to the fit)
fit,fdt-list
Indicates the entry argument which provides the list of device tree
@@ -684,6 +687,14 @@ split-elf
Generates a `loadable = <...>` property with a list of the generated
nodes (including all nodes if this operation is used multiple times)
+ fit,firmware
+ Generates a `firmware = "..."` property with the value of this property
+ But it removes the value of this property from the loadables list
+ generated by fit,loadables
+
+ fit,prepend-to-loadables
+ Adds the list of values of this property to the beginnig of the list of
+ values generated by the fit,loadables property.
Here is an example showing ATF, TEE and a device tree all combined::
@@ -801,6 +812,33 @@ is::
U-Boot SPL can then load the firmware (U-Boot proper) and all the loadables
(ATF and TEE), then proceed with the boot.
+Note that you may want to use atf-1 as firmware, not u-boot. In this case
+atf-1 doesn't have to be in the loadabbles list, SPL will load it to run it.
+It is u-boot then that should be in the list of loadables. To achieve this
+you can chnage your dts to::
+
+ configurations {
+ default = "@config-DEFAULT-SEQ";
+ @config-SEQ {
+ description = "conf-NAME.dtb";
+ fdt = "fdt-SEQ";
+ fit,firmware = "atf-1";
+ fit,prepend-to-loadables = "u-boot"
+ fit,loadables;
+ };
+ };
+
+To obtain::
+
+ configurations {
+ default = "config-1";
+ config-1 {
+ firmware = "atf-1";
+ loadables = "u-boot", "atf-2", "atf-3", "tee-1", "tee-2";
+ description = "rk3399-firefly.dtb";
+ fdt = "fdt-1";
+ };
+ };
Entry: fmap: An entry which contains an Fmap section
diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
index 7b99b83fa3..42a0e11ecb 100644
--- a/tools/binman/etype/fit.py
+++ b/tools/binman/etype/fit.py
@@ -1,3 +1,4 @@
+
# SPDX-License-Identifier: GPL-2.0+
# Copyright (c) 2016 Google, Inc
# Written by Simon Glass <s...@chromium.org>
@@ -186,6 +187,14 @@ class Entry_fit(Entry_section):
Generates a `loadable = <...>` property with a list of the
generated
nodes (including all nodes if this operation is used multiple
times)
+ fit,firmware
+ Generates a `firmware = "..."` property with the value of this
property
+ But it removes the value of this property from the loadables list
+ generated by fit, loadables
+
+ fit,prepend-to-loadables
+ Adds the list of values of this property to the beginnig of the
list of
+ values generated by the fit,loadables property.
Here is an example showing ATF, TEE and a device tree all combined::
@@ -205,6 +214,19 @@ class Entry_fit(Entry_section):
u-boot-nodtb {
};
};
+
+ my-app {
+ description = "U-Boot App Silly Example";
+ type = "standalone";
+ os = "U-Boot";
+ arch = "arm64";
+ compression = "none";
+ load = <0x5000000>;
+ blob {
+ filename = "my-app.bin";
+ };
+ };
+
@fdt-SEQ {
description = "fdt-NAME.dtb";
type = "flat_dt";
@@ -246,7 +268,8 @@ class Entry_fit(Entry_section):
@config-SEQ {
description = "conf-NAME.dtb";
fdt = "fdt-SEQ";
- firmware = "u-boot";
+ fit,firmware = "atf-1";
+ fit,prepend-to-loadables = "u-boot", "my-app";
fit,loadables;
};
};
@@ -293,10 +316,10 @@ class Entry_fit(Entry_section):
configurations {
default = "config-1";
config-1 {
- loadables = "atf-1", "atf-2", "atf-3", "tee-1", "tee-2";
+ loadables = "u-boot", "my-app", "atf-2", "atf-3", "tee-1",
"tee-2";
description = "rk3399-firefly.dtb";
fdt = "fdt-1";
- firmware = "u-boot";
+ firmware = "atf-1";
};
};
@@ -510,11 +533,20 @@ class Entry_fit(Entry_section):
node_name = node.name[1:].replace('SEQ', str(seq + 1))
fname = tools.get_input_filename(fdt_fname + '.dtb')
with fsw.add_node(node_name):
+ firmware = fdt_util.GetString(node, 'fit,firmware');
+ if not firmware is None:
+ fsw.property("firmware", firmware.encode('utf-8'))
+ if firmware in self._loadables:
+ self._loadables.remove(firmware)
+ prepend = fdt_util.GetStringList(node,
'fit,prepend-to-loadables', [])
+ prepend.extend(self._loadables)
+ self._loadables = prepend
for pname, prop in node.props.items():
if pname == 'fit,loadables':
val = '\0'.join(self._loadables) + '\0'
fsw.property('loadables', val.encode('utf-8'))
- elif pname == 'fit,operation':
+ elif pname in ['fit,operation', 'fit,firmware',
+ 'fit,prepend-to-loadables' ]:
pass
elif pname.startswith('fit,'):
self._raise_subnode(
--
2.20.1
