Hi Jonas, On Fri, 20 Jan 2023 at 13:47, Jonas Karlman <jo...@kwiboo.se> wrote: > > Hi Simon, > > On 2023-01-20 20:19, Simon Glass wrote: > > Hi Jonas, > > > > On Fri, 20 Jan 2023 at 01:26, Jonas Karlman <jo...@kwiboo.se> wrote: > >> > >> In some cases it is desired for SPL to start TF-A instead of U-Boot > >> proper. Add support to prepend a list of strings to the loadables list > >> generated by the split-elf generator. > >> > >> Signed-off-by: Jonas Karlman <jo...@kwiboo.se> > >> --- > >> v2: > >> - New patch > >> > >> tools/binman/entries.rst | 5 +- > >> tools/binman/etype/fit.py | 13 +++- > >> tools/binman/ftest.py | 37 +++++++++++ > >> tools/binman/test/276_fit_loadables.dts | 87 +++++++++++++++++++++++++ > >> 4 files changed, 137 insertions(+), 5 deletions(-) > >> create mode 100644 tools/binman/test/276_fit_loadables.dts > >> > >> diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst > >> index 78f95dae1a..0ffffd60f2 100644 > >> --- a/tools/binman/entries.rst > >> +++ b/tools/binman/entries.rst > >> @@ -724,6 +724,7 @@ split-elf > >> fit,loadables > >> Generates a `loadable = <...>` property with a list of the > >> generated > >> nodes (including all nodes if this operation is used multiple > >> times) > >> + Optional property value is prepended to the generated list value > >> > >> > >> Here is an example showing ATF, TEE and a device tree all combined:: > >> @@ -791,8 +792,8 @@ Here is an example showing ATF, TEE and a device tree > >> all combined:: > >> @config-SEQ { > >> description = "conf-NAME.dtb"; > >> fdt = "fdt-SEQ"; > >> - firmware = "u-boot"; > >> - fit,loadables; > >> + firmware = "atf-1"; > >> + fit,loadables = "u-boot"; > >> }; > >> }; > >> }; > >> diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py > >> index bcb606f3f9..3c90b50b7e 100644 > >> --- a/tools/binman/etype/fit.py > >> +++ b/tools/binman/etype/fit.py > >> @@ -190,6 +190,7 @@ class Entry_fit(Entry_section): > >> fit,loadables > >> Generates a `loadable = <...>` property with a list of the > >> generated > >> nodes (including all nodes if this operation is used multiple > >> times) > >> + Optional property value is prepended to the generated list > >> value > >> > >> > >> Here is an example showing ATF, TEE and a device tree all combined:: > >> @@ -257,8 +258,8 @@ class Entry_fit(Entry_section): > >> @config-SEQ { > >> description = "conf-NAME.dtb"; > >> fdt = "fdt-SEQ"; > >> - firmware = "u-boot"; > >> - fit,loadables; > >> + firmware = "atf-1"; > >> + fit,loadables = "u-boot"; > >> }; > >> }; > >> }; > >> @@ -533,7 +534,13 @@ class Entry_fit(Entry_section): > >> with fsw.add_node(node_name): > >> for pname, prop in node.props.items(): > >> if pname == 'fit,loadables': > >> - val = '\0'.join(self._loadables) + '\0' > >> + if type(prop.value) is str: > >> + val = [prop.value] > >> + elif type(prop.value) is list: > >> + val = prop.value > >> + else: > >> + val = [] > >> + val = '\0'.join(val + self._loadables) + > >> '\0' > >> fsw.property('loadables', > >> val.encode('utf-8')) > >> elif pname == 'fit,operation': > >> pass > >> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py > >> index cd27572571..053ae99bee 100644 > >> --- a/tools/binman/ftest.py > >> +++ b/tools/binman/ftest.py > >> @@ -6337,6 +6337,43 @@ fdt fdtmap Extract the > >> devicetree blob from the fdtmap > >> } > >> self.assertEqual(expected, props) > >> > >> + def testFitLoadables(self): > >> + """Test an image with an FIT with prepended loadables""" > >> + if not elf.ELF_TOOLS: > >> + self.skipTest('Python elftools not available') > >> + entry_args = { > >> + 'of-list': 'test-fdt1', > >> + 'default-dt': 'test-fdt1', > >> + 'atf-bl31-path': 'bl31.elf', > >> + 'tee-os-path': 'tee.elf', > >> + } > >> + test_subdir = os.path.join(self._indir, TEST_FDT_SUBDIR) > >> + data = self._DoReadFileDtb( > >> + '276_fit_loadables.dts', > >> + entry_args=entry_args, > >> + extra_indirs=[test_subdir])[0] > >> + > >> + dtb = fdt.Fdt.FromData(data) > >> + dtb.Scan() > >> + > >> + node = dtb.GetNode('/configurations/conf-uboot-1') > >> + self.assertEqual('u-boot', node.props['firmware'].value) > >> + self.assertEqual( > >> + ['atf-1', 'atf-2'], > >> + fdt_util.GetStringList(node, 'loadables')) > >> + > >> + node = dtb.GetNode('/configurations/conf-atf-1') > >> + self.assertEqual('atf-1', node.props['firmware'].value) > >> + self.assertEqual( > >> + ['u-boot', 'atf-1', 'atf-2'], > >> + fdt_util.GetStringList(node, 'loadables')) > >> + > >> + node = dtb.GetNode('/configurations/conf-tee-1') > >> + self.assertEqual('atf-1', node.props['firmware'].value) > >> + self.assertEqual( > >> + ['u-boot', 'tee', 'atf-1', 'atf-2'], > >> + fdt_util.GetStringList(node, 'loadables')) > >> + > >> > >> if __name__ == "__main__": > >> unittest.main() > >> diff --git a/tools/binman/test/276_fit_loadables.dts > >> b/tools/binman/test/276_fit_loadables.dts > >> new file mode 100644 > >> index 0000000000..66dbc1fdf6 > >> --- /dev/null > >> +++ b/tools/binman/test/276_fit_loadables.dts > >> @@ -0,0 +1,87 @@ > >> +// SPDX-License-Identifier: GPL-2.0+ > >> + > >> +/dts-v1/; > >> + > >> +/ { > >> + #address-cells = <1>; > >> + #size-cells = <1>; > >> + > >> + binman { > >> + fit { > >> + description = "test desc"; > >> + #address-cells = <1>; > >> + fit,fdt-list = "of-list"; > >> + > >> + images { > >> + u-boot { > >> + description = "test u-boot"; > >> + type = "standalone"; > >> + arch = "arm64"; > >> + os = "u-boot"; > >> + compression = "none"; > >> + load = <0x00000000>; > >> + entry = <0x00000000>; > >> + > >> + u-boot-nodtb { > >> + }; > >> + }; > >> + > >> + tee { > >> + description = "TEE"; > >> + type = "tee"; > >> + arch = "arm64"; > >> + os = "tee"; > >> + compression = "none"; > >> + load = <0x00200000>; > >> + > >> + tee-os { > >> + optional; > >> + }; > >> + }; > >> + > >> + @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 { > >> + }; > >> + }; > >> + > >> + @fdt-SEQ { > >> + description = "test fdt"; > >> + type = "flat_dt"; > >> + compression = "none"; > >> + }; > >> + }; > >> + > >> + configurations { > >> + default = "@conf-uboot-DEFAULT-SEQ"; > >> + @conf-uboot-SEQ { > >> + description = "test config"; > >> + fdt = "fdt-SEQ"; > >> + firmware = "u-boot"; > >> + fit,loadables; > >> + }; > >> + @conf-atf-SEQ { > >> + description = "test config"; > >> + fdt = "fdt-SEQ"; > >> + firmware = "atf-1"; > >> + fit,loadables = "u-boot"; > >> + }; > >> + @conf-tee-SEQ { > >> + description = "test config"; > >> + fdt = "fdt-SEQ"; > >> + firmware = "atf-1"; > >> + fit,loadables = "u-boot", "tee"; > >> + }; > >> + }; > >> + }; > >> + }; > >> +}; > >> -- > >> 2.39.1 > >> > > > > The problem with this is that aft-1 can be missing, in which case it > > is still referenced in the 'firmware' property. > > > > I suspect we need a way to say that the firmware should be something > > other than U-Boot, if it exists. > > > > How about: > > > > atf-SEQ { > > fit,operation = "split-elf"; > > fit,firmware-next; /* bumps 'u-boot' out of the 'firmware' spot > > if atf is present */ > > description = "ARM Trusted Firmware"; > > type = "firmware"; > > } > > > > fit,firmware = "u-boot"; /* default value for 'firmware' if no atf */ > > fit,loadables; /* ends up holding 'u-boot' too, if it is spilled by atf */ > > This looks reasonable, I do however wonder if it will be more flexible > if we can skip the fit,firmware-next prop altogether and just handle it > with a fit,firmware prop alone, if we treat it like a string list. > > fit,firmware: List of possible entries, the first existing entry is used > for the 'firmware' property. > > fit,loadables: Adds 'loadables' property with a list of all remaining existing > entries in 'fit,firmware' and remaining generated loadables. > > That way we do not create a dependency between the images and configurations > and make it possible to generate configs with different 'firmware' like in > the test dts. > > Example for known entries 'atf-1', 'atf-2' and 'u-boot': > > fit,firmware = "u-boot"; firmware = "u-boot"; > fit,loadables; loadables = "atf-1", "atf-2"; > > fit,firmware = "atf-1", "u-boot"; firmware = "atf-1"; > fit,loadables; loadables = "u-boot", "atf-2"; > > fit,firmware = "fw-1", "u-boot"; firmware = "u-boot"; > fit,loadables; loadables = "atf-1", "atf-2";
Yes that seems better, thanks. So things in fit,firmware are omitted if they are 'missing'. Regards, Simon > > Regards, > Jonas > > > > > I also think the 'atf-1' should not appear in 'loadables' if it is in > > 'firmware'. > > > > Regards, > > Simon >