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"; Regards, Jonas > > I also think the 'atf-1' should not appear in 'loadables' if it is in > 'firmware'. > > Regards, > Simon