Re: [PATCH v2 5/6] binman: Add support for prepending loadables with split-elf

Hi Simon,

On 2023-01-20 20:19, Simon Glass wrote:

Hi Jonas,

On Fri, 20 Jan 2023 at 01:26, Jonas Karlman jonas@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 jonas@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