[PATCH 0/6] binman: Template fixes and improvements

newer
[PATCH 1/1] spl: boot always fail...

Simon Glass

21 Jul 2023 21 Jul '23

6:07 p.m.

With the basic template feature in place, some problems have come to light.

Firstly, keeping the template around while processing entries seems unnecessary and perhaps confusing, so this is removed.

Secondly this series aims to support phandles in a more intuitive way, rather than just ignoring them in templates. It includes an experimental patch to copy phandles from template so that it is possible to so something like:

template { some_node: some-node { }; };

image { insert-template = <&template>; };

with the some_node phandle being copied to the 'image' node, to result in:

image { insert-template = <&template>;

some_node: some-node { }; };

Simon Glass (6): binman: Produce a template-file after processing dtoc: Make properties dirty when purging them dtoc: Add some debugging when copying nodes fdt: Allow copying phandles into templates binman: Remove templates after use WIP: binman: Support templates containing phandles

tools/binman/binman.rst | 8 +++- tools/binman/control.py | 32 ++++++++++++-- tools/binman/ftest.py | 31 +++++++++++++ tools/binman/test/291_template_phandle.dts | 51 ++++++++++++++++++++++ tools/dtoc/fdt.py | 9 +++- tools/dtoc/test/dtoc_test_copy.dts | 6 ++- tools/dtoc/test_fdt.py | 34 ++++++++++----- 7 files changed, 153 insertions(+), 18 deletions(-) create mode 100644 tools/binman/test/291_template_phandle.dts

-- 2.41.0.487.g6d72f3e995-goog

Show replies by date

Simon Glass

21 Jul 21 Jul

6:07 p.m.

New subject: [PATCH 1/6] binman: Produce a template-file after processing

This file aids debugging when binman fails to get far enough to write out the final devicetree file. Write it immediate after template processing.

Signed-off-by: Simon Glass sjg@chromium.org ---

tools/binman/binman.rst | 4 ++++ tools/binman/control.py | 14 ++++++++++++-- tools/binman/ftest.py | 9 +++++++++ 3 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst index 8f57b6cfc76f..67bc3e87531b 100644 --- a/tools/binman/binman.rst +++ b/tools/binman/binman.rst @@ -1256,6 +1256,10 @@ Properties in the template node are inserted into the destination node if they do not exist there. In the example above, `some-property` is added to each of `spi-image` and `mmc-image`.

+The initial devicetree produced by the templating process is written to the +`u-boot.dtb.tmpl1` file. This can be useful to see what is going on if there is +a failure before the final `u-boot.dtb.out` file is written. + Note that template nodes are not removed from the binman description at present.

diff --git a/tools/binman/control.py b/tools/binman/control.py index 25e66814837d..9a9a4817eeaf 100644 --- a/tools/binman/control.py +++ b/tools/binman/control.py @@ -486,6 +486,9 @@ def _ProcessTemplates(parent): Args: parent: Binman node to process (typically /binman)

+ Returns: + bool: True if any templates were processed + Search though each target node looking for those with an 'insert-template' property. Use that as a list of references to template nodes to use to adjust the target node. @@ -498,11 +501,15 @@ def _ProcessTemplates(parent):

See 'Templates' in the Binman documnentation for details. """ + found = False for node in parent.subnodes: tmpl = fdt_util.GetPhandleList(node, 'insert-template') if tmpl: node.copy_subnodes_from_phandles(tmpl) - _ProcessTemplates(node) + found = True + + found |= _ProcessTemplates(node) + return found

def PrepareImagesAndDtbs(dtb_fname, select_images, update_fdt, use_expanded): """Prepare the images to be processed and select the device tree @@ -546,7 +553,10 @@ def PrepareImagesAndDtbs(dtb_fname, select_images, update_fdt, use_expanded): raise ValueError("Device tree '%s' does not have a 'binman' " "node" % dtb_fname)

- _ProcessTemplates(node) + if _ProcessTemplates(node): + dtb.Sync(True) + fname = tools.get_output_filename('u-boot.dtb.tmpl1') + tools.write_file(fname, dtb.GetContents())

images = _ReadImageDesc(node, use_expanded)

diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index e53181afb78a..80d41cf5c35f 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -6793,6 +6793,15 @@ fdt fdtmap Extract the devicetree blob from the fdtmap second = U_BOOT_DATA + b'#' + VGA_DATA + U_BOOT_DTB_DATA self.assertEqual(U_BOOT_IMG_DATA + first + second, data)

+ dtb_fname1 = tools.get_output_filename('u-boot.dtb.tmpl1') + self.assertTrue(os.path.exists(dtb_fname1)) + dtb = fdt.Fdt.FromData(tools.read_file(dtb_fname1)) + dtb.Scan() + node1 = dtb.GetNode('/binman/template') + self.assertTrue(node1) + vga = dtb.GetNode('/binman/first/intel-vga') + self.assertTrue(vga) + def testTemplateBlobMulti(self): """Test using a template with 'multiple-images' enabled""" TestFunctional._MakeInputFile('my-blob.bin', b'blob')

-- 2.41.0.487.g6d72f3e995-goog

Simon Glass

6:07 p.m.

New subject: [PATCH 2/6] dtoc: Make properties dirty when purging them

Without the 'dirty' flag properties are not written back to the devicetree when synced. This means that new properties copied over to a node are not always written out.

Fix this and add a test.

Signed-off-by: Simon Glass sjg@chromium.org ---

tools/dtoc/fdt.py | 1 + tools/dtoc/test/dtoc_test_copy.dts | 6 ++++-- tools/dtoc/test_fdt.py | 16 +++++++++++++--- 3 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/tools/dtoc/fdt.py b/tools/dtoc/fdt.py index fd0f3e94f5c0..2589be990ae9 100644 --- a/tools/dtoc/fdt.py +++ b/tools/dtoc/fdt.py @@ -272,6 +272,7 @@ class Prop: the FDT is synced """ self._offset = None + self.dirty = True

class Node: """A device tree node diff --git a/tools/dtoc/test/dtoc_test_copy.dts b/tools/dtoc/test/dtoc_test_copy.dts index 36faa9b72b56..8e50c7565928 100644 --- a/tools/dtoc/test/dtoc_test_copy.dts +++ b/tools/dtoc/test/dtoc_test_copy.dts @@ -37,11 +37,12 @@ new-prop; };

- second1 { + second1: second1 { new-prop; };

second4 { + use_second1 = <&second1>; }; }; }; @@ -65,12 +66,13 @@ };

second: second { - second1 { + second_1_bad: second1 { some-prop; };

second2 { some-prop; + use_second1_bad = <&second_1_bad>; }; }; }; diff --git a/tools/dtoc/test_fdt.py b/tools/dtoc/test_fdt.py index 3e54694eec9f..84dcd8b5caa5 100755 --- a/tools/dtoc/test_fdt.py +++ b/tools/dtoc/test_fdt.py @@ -308,7 +308,7 @@ class TestNode(unittest.TestCase):

def test_copy_node(self): """Test copy_node() function""" - def do_copy_checks(dtb, dst, expect_none): + def do_copy_checks(dtb, dst, second1_ph_val, expect_none): self.assertEqual( ['/dest/base', '/dest/first@0', '/dest/existing'], [n.path for n in dst.subnodes]) @@ -365,12 +365,22 @@ class TestNode(unittest.TestCase): ['second1', 'second2', 'second3', 'second4'], [n.name for n in second.subnodes])

+ # Check the 'second_1_bad' phandle is not copied over + second1 = second.FindNode('second1') + self.assertTrue(second1) + sph = second1.props.get('phandle') + self.assertTrue(sph) + self.assertEqual(second1_ph_val, sph.bytes) + + dtb = fdt.FdtScan(find_dtb_file('dtoc_test_copy.dts')) tmpl = dtb.GetNode('/base') dst = dtb.GetNode('/dest') + second1_ph_val = (dtb.GetNode('/dest/base/second/second1'). + props['phandle'].bytes) dst.copy_node(tmpl)

- do_copy_checks(dtb, dst, expect_none=True) + do_copy_checks(dtb, dst, second1_ph_val, expect_none=True)

dtb.Sync(auto_resize=True)

@@ -378,7 +388,7 @@ class TestNode(unittest.TestCase): new_dtb = fdt.Fdt.FromData(dtb.GetContents()) new_dtb.Scan() dst = new_dtb.GetNode('/dest') - do_copy_checks(new_dtb, dst, expect_none=False) + do_copy_checks(new_dtb, dst, second1_ph_val, expect_none=False)

def test_copy_subnodes_from_phandles(self): """Test copy_node() function"""

-- 2.41.0.487.g6d72f3e995-goog

Simon Glass

6:07 p.m.

New subject: [PATCH 3/6] dtoc: Add some debugging when copying nodes

Show the operations being performed, when debugging is enabled.

Convert a mistaken 'print' in test_copy_subnodes_from_phandles() while we are here.

Signed-off-by: Simon Glass sjg@chromium.org ---

tools/dtoc/fdt.py | 5 +++++ tools/dtoc/test_fdt.py | 3 ++- 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/dtoc/fdt.py b/tools/dtoc/fdt.py index 2589be990ae9..a0b92d43e8a1 100644 --- a/tools/dtoc/fdt.py +++ b/tools/dtoc/fdt.py @@ -249,6 +249,7 @@ class Prop: """ if self.dirty: node = self._node + tout.debug(f'sync {node.path}: {self.name}') fdt_obj = node._fdt._fdt_obj node_name = fdt_obj.get_name(node._offset) if node_name and node_name != node.name: @@ -716,9 +717,13 @@ class Node: 'phandle' property is not copied since this might result in two nodes with the same phandle, thus making phandle references ambiguous. """ + tout.debug(f'copy to {self.path}: {src.path}') for name, src_prop in src.props.items(): if name != 'phandle' and name not in self.props: self.props[name] = Prop(self, None, name, src_prop.bytes) + tout.debug(f' {name}') + else: + tout.debug(f' {name} - ignored')

def copy_node(self, src): """Copy a node and all its subnodes into this node diff --git a/tools/dtoc/test_fdt.py b/tools/dtoc/test_fdt.py index 84dcd8b5caa5..f77e48b54eaf 100755 --- a/tools/dtoc/test_fdt.py +++ b/tools/dtoc/test_fdt.py @@ -32,6 +32,7 @@ from dtoc.fdt import Type, BytesToValue import libfdt from u_boot_pylib import test_util from u_boot_pylib import tools +from u_boot_pylib import tout

#pylint: disable=protected-access

@@ -414,7 +415,7 @@ class TestNode(unittest.TestCase):

# Make sure that the phandle for 'over' is not copied over = dst.FindNode('over') - print('keys', over.props.keys()) + tout.debug(f'keys: {over.props.keys()}') self.assertNotIn('phandle', over.props.keys())

# Check the merged properties, first the base ones in '/dest'

-- 2.41.0.487.g6d72f3e995-goog

Simon Glass

6:07 p.m.

New subject: [PATCH 4/6] fdt: Allow copying phandles into templates

Allow phandles to be copied over from a template. This can potentially cause duplicate phandles, but we can deal with that later.

Signed-off-by: Simon Glass sjg@chromium.org ---

tools/dtoc/fdt.py | 2 +- tools/dtoc/test_fdt.py | 15 ++++++++------- 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/tools/dtoc/fdt.py b/tools/dtoc/fdt.py index a0b92d43e8a1..917088041cc4 100644 --- a/tools/dtoc/fdt.py +++ b/tools/dtoc/fdt.py @@ -719,7 +719,7 @@ class Node: """ tout.debug(f'copy to {self.path}: {src.path}') for name, src_prop in src.props.items(): - if name != 'phandle' and name not in self.props: + if name not in self.props: self.props[name] = Prop(self, None, name, src_prop.bytes) tout.debug(f' {name}') else: diff --git a/tools/dtoc/test_fdt.py b/tools/dtoc/test_fdt.py index f77e48b54eaf..4772071d59dd 100755 --- a/tools/dtoc/test_fdt.py +++ b/tools/dtoc/test_fdt.py @@ -317,7 +317,8 @@ class TestNode(unittest.TestCase): chk = dtb.GetNode('/dest/base') self.assertTrue(chk) self.assertEqual( - {'compatible', 'bootph-all', '#address-cells', '#size-cells'}, + {'compatible', 'bootph-all', '#address-cells', '#size-cells', + 'phandle'}, chk.props.keys())

# Check the first property @@ -340,8 +341,8 @@ class TestNode(unittest.TestCase): over = dtb.GetNode('/dest/base/over') self.assertTrue(over)

- # Make sure that the phandle for 'over' is not copied - self.assertNotIn('phandle', over.props.keys()) + # Make sure that the phandle for 'over' is copied + self.assertIn('phandle', over.props.keys())

second = dtb.GetNode('/dest/base/second') self.assertTrue(second) @@ -349,7 +350,7 @@ class TestNode(unittest.TestCase): [n.name for n in chk.subnodes]) self.assertEqual(chk, over.parent) self.assertEqual( - {'bootph-all', 'compatible', 'reg', 'low-power'}, + {'bootph-all', 'compatible', 'reg', 'low-power', 'phandle'}, over.props.keys())

if expect_none: @@ -413,14 +414,14 @@ class TestNode(unittest.TestCase): '/dest/second', '/dest/existing', '/dest/base'], [n.path for n in dst.subnodes])

- # Make sure that the phandle for 'over' is not copied + # Make sure that the phandle for 'over' is copied over = dst.FindNode('over') tout.debug(f'keys: {over.props.keys()}') - self.assertNotIn('phandle', over.props.keys()) + self.assertIn('phandle', over.props.keys())

# Check the merged properties, first the base ones in '/dest' expect = {'bootph-all', 'compatible', 'stringarray', 'longbytearray', - 'maybe-empty-int'} + 'maybe-empty-int', 'phandle'}

# Properties from 'base' expect.update({'#address-cells', '#size-cells'})

-- 2.41.0.487.g6d72f3e995-goog

Simon Glass

6:07 p.m.

New subject: [PATCH 5/6] binman: Remove templates after use

It is not necessary to keep templates around after they have been processed. They can cause confusion and potentially duplicate phandles.

Remove them.

Use the same means of detecting a template node in _ReadImageDesc so that the two places are consistent.

Signed-off-by: Simon Glass sjg@chromium.org ---

tools/binman/binman.rst | 8 +++++--- tools/binman/control.py | 14 +++++++++++++- tools/binman/ftest.py | 7 +++++++ 3 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst index 67bc3e87531b..147fbc5ff1b9 100644 --- a/tools/binman/binman.rst +++ b/tools/binman/binman.rst @@ -1256,11 +1256,13 @@ Properties in the template node are inserted into the destination node if they do not exist there. In the example above, `some-property` is added to each of `spi-image` and `mmc-image`.

+Note that template nodes are removed from the binman description after +processing and before binman builds the image descriptions. + The initial devicetree produced by the templating process is written to the `u-boot.dtb.tmpl1` file. This can be useful to see what is going on if there is -a failure before the final `u-boot.dtb.out` file is written. - -Note that template nodes are not removed from the binman description at present. +a failure before the final `u-boot.dtb.out` file is written. A second +`u-boot.dtb.tmpl2` file is written when the templates themselves are removed.

Updating an ELF file diff --git a/tools/binman/control.py b/tools/binman/control.py index 9a9a4817eeaf..3857f50e6436 100644 --- a/tools/binman/control.py +++ b/tools/binman/control.py @@ -57,7 +57,7 @@ def _ReadImageDesc(binman_node, use_expanded): images = OrderedDict() if 'multiple-images' in binman_node.props: for node in binman_node.subnodes: - if 'template' not in node.name: + if not node.name.startswith('template'): images[node.name] = Image(node.name, node, use_expanded=use_expanded) else: @@ -511,6 +511,13 @@ def _ProcessTemplates(parent): found |= _ProcessTemplates(node) return found

+def _RemoveTemplates(parent): + """Remove any templates in the binman description + """ + for node in parent.subnodes: + if node.name.startswith('template'): + node.Delete() + def PrepareImagesAndDtbs(dtb_fname, select_images, update_fdt, use_expanded): """Prepare the images to be processed and select the device tree

@@ -558,6 +565,11 @@ def PrepareImagesAndDtbs(dtb_fname, select_images, update_fdt, use_expanded): fname = tools.get_output_filename('u-boot.dtb.tmpl1') tools.write_file(fname, dtb.GetContents())

+ _RemoveTemplates(node) + dtb.Sync(True) + fname = tools.get_output_filename('u-boot.dtb.tmpl2') + tools.write_file(fname, dtb.GetContents()) + images = _ReadImageDesc(node, use_expanded)

if select_images: diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 80d41cf5c35f..7ea4797ad6ac 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -6802,6 +6802,13 @@ fdt fdtmap Extract the devicetree blob from the fdtmap vga = dtb.GetNode('/binman/first/intel-vga') self.assertTrue(vga)

+ dtb_fname2 = tools.get_output_filename('u-boot.dtb.tmpl2') + self.assertTrue(os.path.exists(dtb_fname2)) + dtb2 = fdt.Fdt.FromData(tools.read_file(dtb_fname2)) + dtb2.Scan() + node2 = dtb2.GetNode('/binman/template') + self.assertFalse(node2) + def testTemplateBlobMulti(self): """Test using a template with 'multiple-images' enabled""" TestFunctional._MakeInputFile('my-blob.bin', b'blob')

-- 2.41.0.487.g6d72f3e995-goog

Simon Glass

6:07 p.m.

New subject: [PATCH 6/6] WIP: binman: Support templates containing phandles

This provides support for phandles to be copied over from templates. This is not quite safe, since if the template is instantiated twice (i.e. in two different nodes), then duplicate phandles will be found.

This patch is provided for some initial experimentation.

Signed-off-by: Simon Glass sjg@chromium.org ---

tools/binman/control.py | 4 ++ tools/binman/ftest.py | 15 +++++++ tools/binman/test/291_template_phandle.dts | 51 ++++++++++++++++++++++ tools/dtoc/fdt.py | 1 + 4 files changed, 71 insertions(+) create mode 100644 tools/binman/test/291_template_phandle.dts

diff --git a/tools/binman/control.py b/tools/binman/control.py index 3857f50e6436..6a6d247e6969 100644 --- a/tools/binman/control.py +++ b/tools/binman/control.py @@ -567,6 +567,10 @@ def PrepareImagesAndDtbs(dtb_fname, select_images, update_fdt, use_expanded):

_RemoveTemplates(node) dtb.Sync(True) + + # Rescan the dtb to pick up the new phandles + dtb.Scan() + node = _FindBinmanNode(dtb) fname = tools.get_output_filename('u-boot.dtb.tmpl2') tools.write_file(fname, dtb.GetContents())

diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 7ea4797ad6ac..5f3f56e07736 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -6900,6 +6900,21 @@ fdt fdtmap Extract the devicetree blob from the fdtmap # Move to next spl_data = content[:0x18]

+ def testTemplatePhandle(self): + """Test using a template in a node containing a phandle""" + entry_args = { + 'atf-bl31-path': 'bl31.elf', + } + data = self._DoReadFileDtb('291_template_phandle.dts', + entry_args=entry_args) + fname = tools.get_output_filename('image.bin') + out = tools.run('dumpimage', '-l', fname) + + # We should see the FIT description and one for each of the two images + lines = out.splitlines() + descs = [line.split()[-1] for line in lines if 'escription' in line] + self.assertEqual(['test-desc', 'atf', 'fdt'], descs) +

if __name__ == "__main__": unittest.main() diff --git a/tools/binman/test/291_template_phandle.dts b/tools/binman/test/291_template_phandle.dts new file mode 100644 index 000000000000..c4ec1dd41bed --- /dev/null +++ b/tools/binman/test/291_template_phandle.dts @@ -0,0 +1,51 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + multiple-images; + + ti_spl_template: template-1 { + fit { + description = "test-desc"; + #address-cells = <1>; + images { + atf { + description = "atf"; + ti-secure { + type = "collection"; + content = <&atf>; + keyfile = "key.pem"; + }; + atf: atf-bl31 { + description = "atf"; + }; + }; + }; + }; + }; + + image { + insert-template = <&ti_spl_template>; + fit { + images { + fdt-0 { + description = "fdt"; + ti-secure { + type = "collection"; + content = <&foo_dtb>; + keyfile = "key.pem"; + }; + foo_dtb: blob-ext { + filename = "vga.bin"; + }; + }; + }; + }; + }; + }; +}; diff --git a/tools/dtoc/fdt.py b/tools/dtoc/fdt.py index 917088041cc4..216c068c5fb5 100644 --- a/tools/dtoc/fdt.py +++ b/tools/dtoc/fdt.py @@ -835,6 +835,7 @@ class Fdt:

TODO(sjg@chromium.org): Implement the 'root' parameter """ + self.phandle_to_node = {} self._cached_offsets = True self._root = self.Node(self, None, 0, '/', '/') self._root.Scan()

-- 2.41.0.487.g6d72f3e995-goog

Neha Malcom Francis

6:47 p.m.

Hi Simon

On 21/07/23 21:37, Simon Glass wrote:

...

With the basic template feature in place, some problems have come to light.

Firstly, keeping the template around while processing entries seems unnecessary and perhaps confusing, so this is removed.

Secondly this series aims to support phandles in a more intuitive way, rather than just ignoring them in templates. It includes an experimental patch to copy phandles from template so that it is possible to so something like:
template {
   some_node: some-node {
   };
};

image {
   insert-template = <&template>;
};
with the some_node phandle being copied to the 'image' node, to result in:
image {
   insert-template = <&template>;

   some_node: some-node {
   };
};

Thanks for this patch series! I will try using this.

...

Simon Glass (6): binman: Produce a template-file after processing dtoc: Make properties dirty when purging them dtoc: Add some debugging when copying nodes fdt: Allow copying phandles into templates binman: Remove templates after use WIP: binman: Support templates containing phandles

tools/binman/binman.rst | 8 +++- tools/binman/control.py | 32 ++++++++++++-- tools/binman/ftest.py | 31 +++++++++++++ tools/binman/test/291_template_phandle.dts | 51 ++++++++++++++++++++++ tools/dtoc/fdt.py | 9 +++- tools/dtoc/test/dtoc_test_copy.dts | 6 ++- tools/dtoc/test_fdt.py | 34 ++++++++++----- 7 files changed, 153 insertions(+), 18 deletions(-) create mode 100644 tools/binman/test/291_template_phandle.dts

-- Thanking You Neha Malcom Francis

659

Age (days ago)

659

Last active (days ago)

List overview

Download

7 comments

2 participants

tags (0)

participants (2)

Neha Malcom Francis
Simon Glass