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 ---

(no changes since v1)

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')

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 ---

(no changes since v1)

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"""

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 ---

Changes in v2: - Use a 'done' variable to reduce code duplication

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..f6a9dee0db12 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(): + done = False if name != 'phandle' and name not in self.props: self.props[name] = Prop(self, None, name, src_prop.bytes) + done = True + tout.debug(f" {name}{'' if done else ' - 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'

Allow phandles to be copied over from a template. This can potentially cause duplicate phandles, so detect this and report an error.

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

Changes in v2: - Handle phandle copying property and report duplicates

tools/dtoc/fdt.py | 28 +++++++++++++++++++--------- tools/dtoc/test_fdt.py | 21 +++++++++++++++++---- 2 files changed, 36 insertions(+), 13 deletions(-)

diff --git a/tools/dtoc/fdt.py b/tools/dtoc/fdt.py index f6a9dee0db12..5963925146a5 100644 --- a/tools/dtoc/fdt.py +++ b/tools/dtoc/fdt.py @@ -337,6 +337,11 @@ class Node: self.props = self._fdt.GetProps(self) phandle = fdt_obj.get_phandle(self.Offset()) if phandle: + dup = self._fdt.phandle_to_node.get(phandle) + if dup: + raise ValueError( + f'Duplicate phandle {phandle} in nodes {dup.path} and {self.path}') + self._fdt.phandle_to_node[phandle] = self

offset = fdt_obj.first_subnode(self.Offset(), QUIET_NOTFOUND) @@ -707,11 +712,12 @@ class Node: prop.Sync(auto_resize) return added

- def merge_props(self, src): + def merge_props(self, src, copy_phandles): """Copy missing properties (except 'phandle') from another node

Args: src (Node): Node containing properties to copy + copy_phandles (bool): True to copy phandle properties in nodes

Adds properties which are present in src but not in this node. Any 'phandle' property is not copied since this might result in two nodes @@ -720,21 +726,24 @@ class Node: tout.debug(f'copy to {self.path}: {src.path}') for name, src_prop in src.props.items(): done = False - if name != 'phandle' and name not in self.props: - self.props[name] = Prop(self, None, name, src_prop.bytes) - done = True + if name not in self.props: + if copy_phandles or name != 'phandle': + self.props[name] = Prop(self, None, name, src_prop.bytes) + done = True tout.debug(f" {name}{'' if done else ' - ignored'}")

- def copy_node(self, src): + def copy_node(self, src, copy_phandles=False): """Copy a node and all its subnodes into this node

Args: src (Node): Node to copy + copy_phandles (bool): True to copy phandle properties in nodes

Returns: Node: Resulting destination node

- This works recursively. + This works recursively, with copy_phandles being set to True for the + recursive calls

The new node is put before all other nodes. If the node already exists, just its subnodes and properties are copied, placing them before @@ -746,12 +755,12 @@ class Node: dst.move_to_first() else: dst = self.insert_subnode(src.name) - dst.merge_props(src) + dst.merge_props(src, copy_phandles)

# Process in reverse order so that they appear correctly in the result, # since copy_node() puts the node first in the list for node in reversed(src.subnodes): - dst.copy_node(node) + dst.copy_node(node, True) return dst

def copy_subnodes_from_phandles(self, phandle_list): @@ -774,7 +783,7 @@ class Node: dst = self.copy_node(node)

tout.debug(f'merge props from {parent.path} to {dst.path}') - self.merge_props(parent) + self.merge_props(parent, False)

class Fdt: @@ -835,6 +844,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() diff --git a/tools/dtoc/test_fdt.py b/tools/dtoc/test_fdt.py index f77e48b54eaf..0b01518f3a5b 100755 --- a/tools/dtoc/test_fdt.py +++ b/tools/dtoc/test_fdt.py @@ -340,8 +340,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 +349,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: @@ -385,9 +385,22 @@ class TestNode(unittest.TestCase):

dtb.Sync(auto_resize=True)

- # Now check that the FDT looks correct + # Now check the resulting FDT. It should have duplicate phandles since + # 'over' has been copied to 'dest/base/over' but still exists in its old + # place new_dtb = fdt.Fdt.FromData(dtb.GetContents()) + with self.assertRaises(ValueError) as exc: + new_dtb.Scan() + self.assertIn( + 'Duplicate phandle 1 in nodes /dest/base/over and /base/over', + str(exc.exception)) + + # Remove the source nodes for the copy + new_dtb.GetNode('/base').Delete() + + # Now it should scan OK new_dtb.Scan() + dst = new_dtb.GetNode('/dest') do_copy_checks(new_dtb, dst, second1_ph_val, expect_none=False)