[PATCH v4 0/3] binman: Further updates for FIT support

[PATCH v4 0/3] binman: Further updates for FIT support

[Simon Glass]

This series adds support for help messages when binary blobs are missing,
as well as selecting the default FIT configuration.

It includes the v3 patches from the earlier series that were not applied.

Note: This series is available at u-boot-dm/binman-working and is based
on u-boot-dm/next

Changes in v4:
- Add more documentation for DEFAULT-SEQ
- Drop patches previous applied to u-boot-dm/next

Changes in v3:
- Add a way to show help messages for missing blobs
- Rebase on top of earlier binman series

Changes in v2:
- Add new patch to allow selecting default FIT configuration

Simon Glass (3):
  binman: Allow selecting default FIT configuration
  binman: Support help messages for missing blobs
  binman: sunxi: Add help message for missing sunxi ATF BL31

 Makefile                                      |  2 +
 arch/arm/dts/sunxi-u-boot.dtsi                |  1 +
 tools/binman/README                           |  6 ++
 tools/binman/README.entries                   |  4 ++
 tools/binman/control.py                       | 69 ++++++++++++++++++-
 tools/binman/entry.py                         |  9 +++
 tools/binman/etype/fit.py                     | 26 +++++++
 tools/binman/ftest.py                         | 59 ++++++++++++++--
 tools/binman/missing-blob-help                | 15 ++++
 tools/binman/test/168_fit_missing_blob.dts    |  9 ++-
 .../test/{170_fit_fdt.dts => 172_fit_fdt.dts} |  2 +-
 11 files changed, 195 insertions(+), 7 deletions(-)
 create mode 100644 tools/binman/missing-blob-help
 rename tools/binman/test/{170_fit_fdt.dts => 172_fit_fdt.dts} (95%)

-- 
2.28.0.526.ge36021eeef-goog

[PATCH v4 1/3] binman: Allow selecting default FIT configuration

[Simon Glass]

Add a new entry argument to the fit entry which allows selection of the
default configuration to use. This is the 'default' property in the
'configurations' node.

Update the Makefile to pass in the value of DEVICE_TREE or
CONFIG_DEFAULT_DEVICE_TREE to provide this information.

Signed-off-by: Simon Glass <sjg@chromium.org>
Suggested-by: Michal Simek <michal.simek@xilinx.com>
---

Changes in v4:
- Add more documentation for DEFAULT-SEQ

Changes in v3:
- Rebase on top of earlier binman series

Changes in v2:
- Add new patch to allow selecting default FIT configuration

 Makefile                                      |  2 +
 tools/binman/README.entries                   |  4 ++
 tools/binman/etype/fit.py                     | 26 ++++++++++++
 tools/binman/ftest.py                         | 41 +++++++++++++++++--
 .../test/{170_fit_fdt.dts => 172_fit_fdt.dts} |  2 +-
 5 files changed, 71 insertions(+), 4 deletions(-)
 rename tools/binman/test/{170_fit_fdt.dts => 172_fit_fdt.dts} (95%)

diff --git a/Makefile b/Makefile
index 65024c74089..fcc559ce7fa 100644
--- a/Makefile
+++ b/Makefile
@@ -1321,6 +1321,7 @@ u-boot.ldr:	u-boot
 # binman
 # ---------------------------------------------------------------------------
 # Use 'make BINMAN_DEBUG=1' to enable debugging
+default_dt := $(if $(DEVICE_TREE),$(DEVICE_TREE),$(CONFIG_DEFAULT_DEVICE_TREE))
 quiet_cmd_binman = BINMAN  $@
 cmd_binman = $(srctree)/tools/binman/binman $(if $(BINMAN_DEBUG),-D) \
                 --toolpath $(objtree)/tools \
@@ -1329,6 +1330,7 @@ cmd_binman = $(srctree)/tools/binman/binman $(if $(BINMAN_DEBUG),-D) \
 		-I . -I $(srctree) -I $(srctree)/board/$(BOARDDIR) \
 		-I arch/$(ARCH)/dts -a of-list=$(CONFIG_OF_LIST) \
 		-a atf-bl31-path=${BL31} \
+		-a default-dt=$(default_dt) \
 		$(BINMAN_$(@F))
 
 OBJCOPYFLAGS_u-boot.ldr.hex := -I binary -O ihex
diff --git a/tools/binman/README.entries b/tools/binman/README.entries
index d2628dffd5d..c1d436563e8 100644
--- a/tools/binman/README.entries
+++ b/tools/binman/README.entries
@@ -402,6 +402,10 @@ Available substitutions for '@' nodes are:
 Note that if no devicetree files are provided (with '-a of-list' as above)
 then no nodes will be generated.
 
+The 'default' property, if present, will be automatically set to the name
+if of configuration whose devicetree matches the 'default-dt' entry
+argument, e.g. with '-a default-dt=sun50i-a64-pine64-lts'.
+
 
 Properties (in the 'fit' node itself):
     fit,external-offset: Indicates that the contents of the FIT are external
diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
index c291eb26bad..de4745c5521 100644
--- a/tools/binman/etype/fit.py
+++ b/tools/binman/etype/fit.py
@@ -90,6 +90,14 @@ class Entry_fit(Entry):
     Note that if no devicetree files are provided (with '-a of-list' as above)
     then no nodes will be generated.
 
+    The 'default' property, if present, will be automatically set to the name
+    if of configuration whose devicetree matches the 'default-dt' entry
+    argument, e.g. with '-a default-dt=sun50i-a64-pine64-lts'.
+
+    Available substitutions for '@' property values are:
+
+        DEFAULT-SEQ  Sequence number of the default fdt,as provided by the
+                     'default-dt' entry argument
 
     Properties (in the 'fit' node itself):
         fit,external-offset: Indicates that the contents of the FIT are external
@@ -121,6 +129,8 @@ class Entry_fit(Entry):
                 [EntryArg(self._fit_list_prop.value, str)])
             if fdts is not None:
                 self._fdts = fdts.split()
+        self._fit_default_dt = self.GetEntryArgsOrProps([EntryArg('default-dt',
+                                                                  str)])[0]
 
     def ReadNode(self):
         self._ReadSubnodes()
@@ -142,6 +152,22 @@ class Entry_fit(Entry):
             """
             for pname, prop in node.props.items():
                 if not pname.startswith('fit,'):
+                    if pname == 'default':
+                        val = prop.value
+                        # Handle the 'default' property
+                        if val.startswith('@'):
+                            if not self._fdts:
+                                continue
+                            if not self._fit_default_dt:
+                                self.Raise("Generated 'default' node requires default-dt entry argument")
+                            if self._fit_default_dt not in self._fdts:
+                                self.Raise("default-dt entry argument '%s' not found in fdt list: %s" %
+                                           (self._fit_default_dt,
+                                            ', '.join(self._fdts)))
+                            seq = self._fdts.index(self._fit_default_dt)
+                            val = val[1:].replace('DEFAULT-SEQ', str(seq + 1))
+                            fsw.property_string(pname, val)
+                            continue
                     fsw.property(pname, prop.bytes)
 
             rel_path = node.path[len(base_node.path):]
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index 78d0e9c2b93..a269a7497cb 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -3602,7 +3602,7 @@ class TestFunctional(unittest.TestCase):
             """
             cnode = dtb.GetNode('/configurations')
             self.assertIn('default', cnode.props)
-            self.assertEqual('config-1', cnode.props['default'].value)
+            self.assertEqual('config-2', cnode.props['default'].value)
 
             name = 'config-%d' % seq
             fnode = dtb.GetNode('/configurations/%s' % name)
@@ -3615,9 +3615,10 @@ class TestFunctional(unittest.TestCase):
 
         entry_args = {
             'of-list': 'test-fdt1 test-fdt2',
+            'default-dt': 'test-fdt2',
         }
         data = self._DoReadFileDtb(
-            '170_fit_fdt.dts',
+            '172_fit_fdt.dts',
             entry_args=entry_args,
             extra_indirs=[os.path.join(self._indir, TEST_FDT_SUBDIR)])[0]
         self.assertEqual(U_BOOT_NODTB_DATA, data[-len(U_BOOT_NODTB_DATA):])
@@ -3639,7 +3640,7 @@ class TestFunctional(unittest.TestCase):
     def testFitFdtMissingList(self):
         """Test handling of a missing 'of-list' entry arg"""
         with self.assertRaises(ValueError) as e:
-            self._DoReadFile('170_fit_fdt.dts')
+            self._DoReadFile('172_fit_fdt.dts')
         self.assertIn("Generator node requires 'of-list' entry argument",
                       str(e.exception))
 
@@ -3657,5 +3658,39 @@ class TestFunctional(unittest.TestCase):
         self.assertIn("Generator node requires 'fit,fdt-list' property",
                       str(e.exception))
 
+    def testFitFdtEmptyList(self):
+        """Test handling of an empty 'of-list' entry arg"""
+        entry_args = {
+            'of-list': '',
+        }
+        data = self._DoReadFileDtb('172_fit_fdt.dts', entry_args=entry_args)[0]
+
+    def testFitFdtMissing(self):
+        """Test handling of a missing 'default-dt' entry arg"""
+        entry_args = {
+            'of-list': 'test-fdt1 test-fdt2',
+        }
+        with self.assertRaises(ValueError) as e:
+            self._DoReadFileDtb(
+                '172_fit_fdt.dts',
+                entry_args=entry_args,
+                extra_indirs=[os.path.join(self._indir, TEST_FDT_SUBDIR)])[0]
+        self.assertIn("Generated 'default' node requires default-dt entry argument",
+                      str(e.exception))
+
+    def testFitFdtNotInList(self):
+        """Test handling of a default-dt that is not in the of-list"""
+        entry_args = {
+            'of-list': 'test-fdt1 test-fdt2',
+            'default-dt': 'test-fdt3',
+        }
+        with self.assertRaises(ValueError) as e:
+            self._DoReadFileDtb(
+                '172_fit_fdt.dts',
+                entry_args=entry_args,
+                extra_indirs=[os.path.join(self._indir, TEST_FDT_SUBDIR)])[0]
+        self.assertIn("default-dt entry argument 'test-fdt3' not found in fdt list: test-fdt1, test-fdt2",
+                      str(e.exception))
+
 if __name__ == "__main__":
     unittest.main()
diff --git a/tools/binman/test/170_fit_fdt.dts b/tools/binman/test/172_fit_fdt.dts
similarity index 95%
rename from tools/binman/test/170_fit_fdt.dts
rename to tools/binman/test/172_fit_fdt.dts
index 89142e91017..99d710c57e9 100644
--- a/tools/binman/test/170_fit_fdt.dts
+++ b/tools/binman/test/172_fit_fdt.dts
@@ -40,7 +40,7 @@
 			};
 
 			configurations {
-				default = "config-1";
+				default = "@config-DEFAULT-SEQ";
 				@config-SEQ {
 					description = "conf-NAME.dtb";
 					firmware = "uboot";
-- 
2.28.0.526.ge36021eeef-goog

[PATCH v4 2/3] binman: Support help messages for missing blobs

[Simon Glass]

When an external blob is missing it can be quite confusing for the user.
Add a way to provide a help message that is shown.

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

(no changes since v3)

Changes in v3:
- Add a way to show help messages for missing blobs

 tools/binman/README                        |  6 ++
 tools/binman/control.py                    | 69 +++++++++++++++++++++-
 tools/binman/entry.py                      |  9 +++
 tools/binman/ftest.py                      | 18 +++++-
 tools/binman/missing-blob-help             | 11 ++++
 tools/binman/test/168_fit_missing_blob.dts |  9 ++-
 6 files changed, 119 insertions(+), 3 deletions(-)
 create mode 100644 tools/binman/missing-blob-help

diff --git a/tools/binman/README b/tools/binman/README
index 37ee3fc2d3b..f7bf285a915 100644
--- a/tools/binman/README
+++ b/tools/binman/README
@@ -343,6 +343,12 @@ compress:
 	Sets the compression algortihm to use (for blobs only). See the entry
 	documentation for details.
 
+missing-msg:
+	Sets the tag of the message to show if this entry is missing. This is
+	used for external blobs. When they are missing it is helpful to show
+	information about what needs to be fixed. See missing-blob-help for the
+	message for each tag.
+
 The attributes supported for images and sections are described below. Several
 are similar to those for entries.
 
diff --git a/tools/binman/control.py b/tools/binman/control.py
index 15bfac582a7..ee5771e7292 100644
--- a/tools/binman/control.py
+++ b/tools/binman/control.py
@@ -9,6 +9,7 @@ from collections import OrderedDict
 import glob
 import os
 import pkg_resources
+import re
 
 import sys
 from patman import tools
@@ -22,6 +23,11 @@ from patman import tout
 # Make this global so that it can be referenced from tests
 images = OrderedDict()
 
+# Help text for each type of missing blob, dict:
+#    key: Value of the entry's 'missing-msg' or entry name
+#    value: Text for the help
+missing_blob_help = {}
+
 def _ReadImageDesc(binman_node):
     """Read the image descriptions from the /binman node
 
@@ -54,6 +60,66 @@ def _FindBinmanNode(dtb):
             return node
     return None
 
+def _ReadMissingBlobHelp():
+    """Read the missing-blob-help file
+
+    This file containins help messages explaining what to do when external blobs
+    are missing.
+
+    Returns:
+        Dict:
+            key: Message tag (str)
+            value: Message text (str)
+    """
+
+    def _FinishTag(tag, msg, result):
+        if tag:
+            result[tag] = msg.rstrip()
+            tag = None
+            msg = ''
+        return tag, msg
+
+    my_data = pkg_resources.resource_string(__name__, 'missing-blob-help')
+    re_tag = re.compile('^([-a-z0-9]+):$')
+    result = {}
+    tag = None
+    msg = ''
+    for line in my_data.decode('utf-8').splitlines():
+        if not line.startswith('#'):
+            m_tag = re_tag.match(line)
+            if m_tag:
+                _, msg = _FinishTag(tag, msg, result)
+                tag = m_tag.group(1)
+            elif tag:
+                msg += line + '\n'
+    _FinishTag(tag, msg, result)
+    return result
+
+def _ShowBlobHelp(path, text):
+    tout.Warning('\n%s:' % path)
+    for line in text.splitlines():
+        tout.Warning('   %s' % line)
+
+def _ShowHelpForMissingBlobs(missing_list):
+    """Show help for each missing blob to help the user take action
+
+    Args:
+        missing_list: List of Entry objects to show help for
+    """
+    global missing_blob_help
+
+    if not missing_blob_help:
+        missing_blob_help = _ReadMissingBlobHelp()
+
+    for entry in missing_list:
+        tags = entry.GetHelpTags()
+
+        # Show the first match help message
+        for tag in tags:
+            if tag in missing_blob_help:
+                _ShowBlobHelp(entry._node.path, missing_blob_help[tag])
+                break
+
 def GetEntryModules(include_testing=True):
     """Get a set of entry class implementations
 
@@ -478,6 +544,7 @@ def ProcessImage(image, update_fdt, write_map, get_contents=True,
     if missing_list:
         tout.Warning("Image '%s' is missing external blobs and is non-functional: %s" %
                      (image.name, ' '.join([e.name for e in missing_list])))
+        _ShowHelpForMissingBlobs(missing_list)
     return bool(missing_list)
 
 
@@ -563,7 +630,7 @@ def Binman(args):
                 tools.WriteFile(dtb_item._fname, dtb_item.GetContents())
 
             if missing:
-                tout.Warning("Some images are invalid")
+                tout.Warning("\nSome images are invalid")
         finally:
             tools.FinaliseOutputDir()
     finally:
diff --git a/tools/binman/entry.py b/tools/binman/entry.py
index 0f128c4cf5e..f7adc3b1abb 100644
--- a/tools/binman/entry.py
+++ b/tools/binman/entry.py
@@ -178,6 +178,7 @@ class Entry(object):
         self.align_end = fdt_util.GetInt(self._node, 'align-end')
         self.offset_unset = fdt_util.GetBool(self._node, 'offset-unset')
         self.expand_size = fdt_util.GetBool(self._node, 'expand-size')
+        self.missing_msg = fdt_util.GetString(self._node, 'missing-msg')
 
     def GetDefaultFilename(self):
         return None
@@ -827,3 +828,11 @@ features to produce new behaviours.
             True if allowed, False if not allowed
         """
         return self.allow_missing
+
+    def GetHelpTags(self):
+        """Get the tags use for missing-blob help
+
+        Returns:
+            list of possible tags, most desirable first
+        """
+        return list(filter(None, [self.missing_msg, self.name, self.etype]))
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index a269a7497cb..95b17d0b749 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -3561,7 +3561,7 @@ class TestFunctional(unittest.TestCase):
             self._DoTestFile('168_fit_missing_blob.dts',
                              allow_missing=True)
         err = stderr.getvalue()
-        self.assertRegex(err, "Image 'main-section'.*missing.*: blob-ext")
+        self.assertRegex(err, "Image 'main-section'.*missing.*: atf-bl31")
 
     def testBlobNamedByArgMissing(self):
         """Test handling of a missing entry arg"""
@@ -3692,5 +3692,21 @@ class TestFunctional(unittest.TestCase):
         self.assertIn("default-dt entry argument 'test-fdt3' not found in fdt list: test-fdt1, test-fdt2",
                       str(e.exception))
 
+    def testFitExtblobMissingHelp(self):
+        """Test display of help messages when an external blob is missing"""
+        control.missing_blob_help = control._ReadMissingBlobHelp()
+        control.missing_blob_help['wibble'] = 'Wibble test'
+        control.missing_blob_help['another'] = 'Another test'
+        with test_util.capture_sys_output() as (stdout, stderr):
+            self._DoTestFile('168_fit_missing_blob.dts',
+                             allow_missing=True)
+        err = stderr.getvalue()
+
+        # We can get the tag from the name, the type or the missing-msg
+        # property. Check all three.
+        self.assertIn('You may need to build ARM Trusted', err)
+        self.assertIn('Wibble test', err)
+        self.assertIn('Another test', err)
+
 if __name__ == "__main__":
     unittest.main()
diff --git a/tools/binman/missing-blob-help b/tools/binman/missing-blob-help
new file mode 100644
index 00000000000..3de195c23c8
--- /dev/null
+++ b/tools/binman/missing-blob-help
@@ -0,0 +1,11 @@
+# This file contains help messages for missing external blobs. Each message has
+# a tag (MUST be just lower-case text, digits and hyphens) starting in column 1,
+# followed by a colon (:) to indicate its start. The message can include any
+# number of lines, including blank lines.
+#
+# When looking for a tag, Binman uses the value of 'missing-msg' for the entry,
+# the entry name or the entry type, in that order
+
+atf-bl31:
+See the documentation for your board. You may need to build ARM Trusted
+Firmware and build with BL31=/path/to/bl31.bin
diff --git a/tools/binman/test/168_fit_missing_blob.dts b/tools/binman/test/168_fit_missing_blob.dts
index e007aa41d8a..15f6cc07e5d 100644
--- a/tools/binman/test/168_fit_missing_blob.dts
+++ b/tools/binman/test/168_fit_missing_blob.dts
@@ -29,9 +29,16 @@
 					hash-2 {
 						algo = "sha1";
 					};
-					blob-ext {
+					atf-bl31 {
 						filename = "missing";
 					};
+					cros-ec-rw {
+						type = "atf-bl31";
+						missing-msg = "wibble";
+					};
+					another {
+						type = "atf-bl31";
+					};
 				};
 			};
 		};
-- 
2.28.0.526.ge36021eeef-goog

[PATCH v4 3/3] binman: sunxi: Add help message for missing sunxi ATF BL31

[Simon Glass]

Add a special help message pointing to the relevant README.

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

Changes in v4:
- Drop patches previous applied to u-boot-dm/next

 arch/arm/dts/sunxi-u-boot.dtsi | 1 +
 tools/binman/missing-blob-help | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/arch/arm/dts/sunxi-u-boot.dtsi b/arch/arm/dts/sunxi-u-boot.dtsi
index 1d1c3691099..c97943b3c19 100644
--- a/arch/arm/dts/sunxi-u-boot.dtsi
+++ b/arch/arm/dts/sunxi-u-boot.dtsi
@@ -48,6 +48,7 @@
 					entry = <0x44000>;
 #endif
 					atf-bl31 {
+						missing-msg = "atf-bl31-sunxi";
 					};
 				};
 
diff --git a/tools/binman/missing-blob-help b/tools/binman/missing-blob-help
index 3de195c23c8..7cf1c346101 100644
--- a/tools/binman/missing-blob-help
+++ b/tools/binman/missing-blob-help
@@ -9,3 +9,7 @@
 atf-bl31:
 See the documentation for your board. You may need to build ARM Trusted
 Firmware and build with BL31=/path/to/bl31.bin
+
+atf-bl31-sunxi:
+Please read the section on ARM Trusted Firmware (ATF) in
+board/sunxi/README.sunxi64
-- 
2.28.0.526.ge36021eeef-goog

[PATCH v4 0/3] binman: Further updates for FIT support

[Michal Simek]

Hi Simon,

On 06. 09. 20 18:39, Simon Glass wrote:
> This series adds support for help messages when binary blobs are missing,
> as well as selecting the default FIT configuration.
> 
> It includes the v3 patches from the earlier series that were not applied.
> 
> Note: This series is available at u-boot-dm/binman-working and is based
> on u-boot-dm/next
> 
> Changes in v4:
> - Add more documentation for DEFAULT-SEQ
> - Drop patches previous applied to u-boot-dm/next
> 
> Changes in v3:
> - Add a way to show help messages for missing blobs
> - Rebase on top of earlier binman series
> 
> Changes in v2:
> - Add new patch to allow selecting default FIT configuration
> 
> Simon Glass (3):
>   binman: Allow selecting default FIT configuration
>   binman: Support help messages for missing blobs
>   binman: sunxi: Add help message for missing sunxi ATF BL31
> 
>  Makefile                                      |  2 +
>  arch/arm/dts/sunxi-u-boot.dtsi                |  1 +
>  tools/binman/README                           |  6 ++
>  tools/binman/README.entries                   |  4 ++
>  tools/binman/control.py                       | 69 ++++++++++++++++++-
>  tools/binman/entry.py                         |  9 +++
>  tools/binman/etype/fit.py                     | 26 +++++++
>  tools/binman/ftest.py                         | 59 ++++++++++++++--
>  tools/binman/missing-blob-help                | 15 ++++
>  tools/binman/test/168_fit_missing_blob.dts    |  9 ++-
>  .../test/{170_fit_fdt.dts => 172_fit_fdt.dts} |  2 +-
>  11 files changed, 195 insertions(+), 7 deletions(-)
>  create mode 100644 tools/binman/missing-blob-help
>  rename tools/binman/test/{170_fit_fdt.dts => 172_fit_fdt.dts} (95%)
> 

I just spot one thing and will be good to clearify it.

In sunxi this description is used.
+					firmware = "uboot";
+					loadables = "atf";

But is this right way how this should be described?
I personally use firmware which points to ATF and loadables which points
to uboot. Not sure what sunxi is doing but also in spl_fit.c there is a
comment about it.
	/*
	 * Find the U-Boot image using the following search order:
	 *   - start at 'firmware' (e.g. an ARM Trusted Firmware)
	 *   - fall back 'kernel' (e.g. a Falcon-mode OS boot
	 *   - fall back to using the first 'loadables' entry
	 */

Thanks,
Michal

[PATCH v4 0/3] binman: Further updates for FIT support

[Simon Glass]

Hi Michal,

On Mon, 7 Sep 2020 at 02:44, Michal Simek <michal.simek@xilinx.com> wrote:
>
> Hi Simon,
>
> On 06. 09. 20 18:39, Simon Glass wrote:
> > This series adds support for help messages when binary blobs are missing,
> > as well as selecting the default FIT configuration.
> >
> > It includes the v3 patches from the earlier series that were not applied.
> >
> > Note: This series is available at u-boot-dm/binman-working and is based
> > on u-boot-dm/next
> >
> > Changes in v4:
> > - Add more documentation for DEFAULT-SEQ
> > - Drop patches previous applied to u-boot-dm/next
> >
> > Changes in v3:
> > - Add a way to show help messages for missing blobs
> > - Rebase on top of earlier binman series
> >
> > Changes in v2:
> > - Add new patch to allow selecting default FIT configuration
> >
> > Simon Glass (3):
> >   binman: Allow selecting default FIT configuration
> >   binman: Support help messages for missing blobs
> >   binman: sunxi: Add help message for missing sunxi ATF BL31
> >
> >  Makefile                                      |  2 +
> >  arch/arm/dts/sunxi-u-boot.dtsi                |  1 +
> >  tools/binman/README                           |  6 ++
> >  tools/binman/README.entries                   |  4 ++
> >  tools/binman/control.py                       | 69 ++++++++++++++++++-
> >  tools/binman/entry.py                         |  9 +++
> >  tools/binman/etype/fit.py                     | 26 +++++++
> >  tools/binman/ftest.py                         | 59 ++++++++++++++--
> >  tools/binman/missing-blob-help                | 15 ++++
> >  tools/binman/test/168_fit_missing_blob.dts    |  9 ++-
> >  .../test/{170_fit_fdt.dts => 172_fit_fdt.dts} |  2 +-
> >  11 files changed, 195 insertions(+), 7 deletions(-)
> >  create mode 100644 tools/binman/missing-blob-help
> >  rename tools/binman/test/{170_fit_fdt.dts => 172_fit_fdt.dts} (95%)
> >
>
> I just spot one thing and will be good to clearify it.
>
> In sunxi this description is used.
> +                                       firmware = "uboot";
> +                                       loadables = "atf";
>
> But is this right way how this should be described?
> I personally use firmware which points to ATF and loadables which points
> to uboot. Not sure what sunxi is doing but also in spl_fit.c there is a
> comment about it.
>         /*
>          * Find the U-Boot image using the following search order:
>          *   - start at 'firmware' (e.g. an ARM Trusted Firmware)
>          *   - fall back 'kernel' (e.g. a Falcon-mode OS boot
>          *   - fall back to using the first 'loadables' entry
>          */

It would be better if this could be made deterministic. I don't really
know which is right, but we should not need fallbacks, etc. We know
when we are in Falcon mode, at least, and we should be able to settle
on which one to use for U-Boot.

Regards,
Simon

[PATCH v4 2/3] binman: Support help messages for missing blobs

[Alper Nebi Yasak]

On 06/09/2020 19:39, Simon Glass wrote:
> When an external blob is missing it can be quite confusing for the user.
> Add a way to provide a help message that is shown.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> (no changes since v3)
> 
> Changes in v3:
> - Add a way to show help messages for missing blobs
> 
>  tools/binman/README                        |  6 ++
>  tools/binman/control.py                    | 69 +++++++++++++++++++++-
>  tools/binman/entry.py                      |  9 +++
>  tools/binman/ftest.py                      | 18 +++++-
>  tools/binman/missing-blob-help             | 11 ++++
>  tools/binman/test/168_fit_missing_blob.dts |  9 ++-
>  6 files changed, 119 insertions(+), 3 deletions(-)
>  create mode 100644 tools/binman/missing-blob-help
> 
> diff --git a/tools/binman/README b/tools/binman/README
> index 37ee3fc2d3b..f7bf285a915 100644
> --- a/tools/binman/README
> +++ b/tools/binman/README
> @@ -343,6 +343,12 @@ compress:
>  	Sets the compression algortihm to use (for blobs only). See the entry
>  	documentation for details.
>  
> +missing-msg:
> +	Sets the tag of the message to show if this entry is missing. This is
> +	used for external blobs. When they are missing it is helpful to show
> +	information about what needs to be fixed. See missing-blob-help for the
> +	message for each tag.
> +
>  The attributes supported for images and sections are described below. Several
>  are similar to those for entries.
>  
> diff --git a/tools/binman/control.py b/tools/binman/control.py
> index 15bfac582a7..ee5771e7292 100644
> --- a/tools/binman/control.py
> +++ b/tools/binman/control.py
> @@ -9,6 +9,7 @@ from collections import OrderedDict
>  import glob
>  import os
>  import pkg_resources
> +import re
>  
>  import sys
>  from patman import tools
> @@ -22,6 +23,11 @@ from patman import tout
>  # Make this global so that it can be referenced from tests
>  images = OrderedDict()
>  
> +# Help text for each type of missing blob, dict:
> +#    key: Value of the entry's 'missing-msg' or entry name
> +#    value: Text for the help
> +missing_blob_help = {}
> +
>  def _ReadImageDesc(binman_node):
>      """Read the image descriptions from the /binman node
>  
> @@ -54,6 +60,66 @@ def _FindBinmanNode(dtb):
>              return node
>      return None
>  
> +def _ReadMissingBlobHelp():
> +    """Read the missing-blob-help file
> +
> +    This file containins help messages explaining what to do when external blobs
> +    are missing.
> +
> +    Returns:
> +        Dict:
> +            key: Message tag (str)
> +            value: Message text (str)
> +    """
> +
> +    def _FinishTag(tag, msg, result):
> +        if tag:
> +            result[tag] = msg.rstrip()
> +            tag = None
> +            msg = ''
> +        return tag, msg
> +
> +    my_data = pkg_resources.resource_string(__name__, 'missing-blob-help')
> +    re_tag = re.compile('^([-a-z0-9]+):$')
> +    result = {}
> +    tag = None
> +    msg = ''
> +    for line in my_data.decode('utf-8').splitlines():
> +        if not line.startswith('#'):
> +            m_tag = re_tag.match(line)
> +            if m_tag:
> +                _, msg = _FinishTag(tag, msg, result)
> +                tag = m_tag.group(1)
> +            elif tag:
> +                msg += line + '\n'
> +    _FinishTag(tag, msg, result)
> +    return result

This looks a bit complex, I think something like this would be clearer:

    result = {}
    tag = None
    for line in my_data.decode('utf-8').splitlines():
        m_tag = re_tag.match(line)
        if line.startswith('#'):
            continue
        elif m_tag:
            tag = m_tag.group(1)
            result[tag] = []
        elif tag:
            result[tag].append(line)
    for tag, lines in result.items():
        result[tag] = "".join(lines).rstrip()
    return result

> +
> +def _ShowBlobHelp(path, text):
> +    tout.Warning('\n%s:' % path)
> +    for line in text.splitlines():
> +        tout.Warning('   %s' % line)
> +
> +def _ShowHelpForMissingBlobs(missing_list):
> +    """Show help for each missing blob to help the user take action
> +
> +    Args:
> +        missing_list: List of Entry objects to show help for
> +    """
> +    global missing_blob_help
> +
> +    if not missing_blob_help:
> +        missing_blob_help = _ReadMissingBlobHelp()
> +
> +    for entry in missing_list:
> +        tags = entry.GetHelpTags()
> +
> +        # Show the first match help message
> +        for tag in tags:
> +            if tag in missing_blob_help:
> +                _ShowBlobHelp(entry._node.path, missing_blob_help[tag])
> +                break
> +
>  def GetEntryModules(include_testing=True):
>      """Get a set of entry class implementations
>  
> @@ -478,6 +544,7 @@ def ProcessImage(image, update_fdt, write_map, get_contents=True,
>      if missing_list:
>          tout.Warning("Image '%s' is missing external blobs and is non-functional: %s" %
>                       (image.name, ' '.join([e.name for e in missing_list])))
> +        _ShowHelpForMissingBlobs(missing_list)
>      return bool(missing_list)
>  
>  
> @@ -563,7 +630,7 @@ def Binman(args):
>                  tools.WriteFile(dtb_item._fname, dtb_item.GetContents())
>  
>              if missing:
> -                tout.Warning("Some images are invalid")
> +                tout.Warning("\nSome images are invalid")
>          finally:
>              tools.FinaliseOutputDir()
>      finally:
> diff --git a/tools/binman/entry.py b/tools/binman/entry.py
> index 0f128c4cf5e..f7adc3b1abb 100644
> --- a/tools/binman/entry.py
> +++ b/tools/binman/entry.py
> @@ -178,6 +178,7 @@ class Entry(object):
>          self.align_end = fdt_util.GetInt(self._node, 'align-end')
>          self.offset_unset = fdt_util.GetBool(self._node, 'offset-unset')
>          self.expand_size = fdt_util.GetBool(self._node, 'expand-size')
> +        self.missing_msg = fdt_util.GetString(self._node, 'missing-msg')
>  
>      def GetDefaultFilename(self):
>          return None
> @@ -827,3 +828,11 @@ features to produce new behaviours.
>              True if allowed, False if not allowed
>          """
>          return self.allow_missing
> +
> +    def GetHelpTags(self):
> +        """Get the tags use for missing-blob help
> +
> +        Returns:
> +            list of possible tags, most desirable first
> +        """
> +        return list(filter(None, [self.missing_msg, self.name, self.etype]))
> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> index a269a7497cb..95b17d0b749 100644
> --- a/tools/binman/ftest.py
> +++ b/tools/binman/ftest.py
> @@ -3561,7 +3561,7 @@ class TestFunctional(unittest.TestCase):
>              self._DoTestFile('168_fit_missing_blob.dts',
>                               allow_missing=True)
>          err = stderr.getvalue()
> -        self.assertRegex(err, "Image 'main-section'.*missing.*: blob-ext")
> +        self.assertRegex(err, "Image 'main-section'.*missing.*: atf-bl31")
>  
>      def testBlobNamedByArgMissing(self):
>          """Test handling of a missing entry arg"""
> @@ -3692,5 +3692,21 @@ class TestFunctional(unittest.TestCase):
>          self.assertIn("default-dt entry argument 'test-fdt3' not found in fdt list: test-fdt1, test-fdt2",
>                        str(e.exception))
>  
> +    def testFitExtblobMissingHelp(self):
> +        """Test display of help messages when an external blob is missing"""
> +        control.missing_blob_help = control._ReadMissingBlobHelp()
> +        control.missing_blob_help['wibble'] = 'Wibble test'
> +        control.missing_blob_help['another'] = 'Another test'
> +        with test_util.capture_sys_output() as (stdout, stderr):
> +            self._DoTestFile('168_fit_missing_blob.dts',
> +                             allow_missing=True)
> +        err = stderr.getvalue()
> +
> +        # We can get the tag from the name, the type or the missing-msg
> +        # property. Check all three.
> +        self.assertIn('You may need to build ARM Trusted', err)
> +        self.assertIn('Wibble test', err)
> +        self.assertIn('Another test', err)
> +
>  if __name__ == "__main__":
>      unittest.main()
> diff --git a/tools/binman/missing-blob-help b/tools/binman/missing-blob-help
> new file mode 100644
> index 00000000000..3de195c23c8
> --- /dev/null
> +++ b/tools/binman/missing-blob-help
> @@ -0,0 +1,11 @@
> +# This file contains help messages for missing external blobs. Each message has
> +# a tag (MUST be just lower-case text, digits and hyphens) starting in column 1,
> +# followed by a colon (:) to indicate its start. The message can include any
> +# number of lines, including blank lines.
> +#
> +# When looking for a tag, Binman uses the value of 'missing-msg' for the entry,
> +# the entry name or the entry type, in that order
> +
> +atf-bl31:
> +See the documentation for your board. You may need to build ARM Trusted
> +Firmware and build with BL31=/path/to/bl31.bin
> diff --git a/tools/binman/test/168_fit_missing_blob.dts b/tools/binman/test/168_fit_missing_blob.dts
> index e007aa41d8a..15f6cc07e5d 100644
> --- a/tools/binman/test/168_fit_missing_blob.dts
> +++ b/tools/binman/test/168_fit_missing_blob.dts
> @@ -29,9 +29,16 @@
>  					hash-2 {
>  						algo = "sha1";
>  					};
> -					blob-ext {
> +					atf-bl31 {
>  						filename = "missing";
>  					};
> +					cros-ec-rw {
> +						type = "atf-bl31";
> +						missing-msg = "wibble";
> +					};
> +					another {
> +						type = "atf-bl31";
> +					};
>  				};
>  			};
>  		};
> 

[PATCH v4 1/3] binman: Allow selecting default FIT configuration

[Alper Nebi Yasak]

On 06/09/2020 19:39, Simon Glass wrote:
> Add a new entry argument to the fit entry which allows selection of the
> default configuration to use. This is the 'default' property in the
> 'configurations' node.
> 
> Update the Makefile to pass in the value of DEVICE_TREE or
> CONFIG_DEFAULT_DEVICE_TREE to provide this information.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Suggested-by: Michal Simek <michal.simek@xilinx.com>
> ---
> 
> Changes in v4:
> - Add more documentation for DEFAULT-SEQ

I might be too late to say this but the SEQ thing looks ugly to me.
Maybe there could be some generic control-flow-like nodes that could
generate and insert things in their own place. If it makes sense, I'm
imagining something like:

    fit {
        images {
            __for__ {
                for,variable = "name";
                for,in-args = "of-list";

                fdt-#name {
                    description = "fdt-$name.dtb";
                    type = "flat_dt";
                    compression = "none";
                };
            };
        };

        configurations {
            __for__ {
                for,variable = "name"
                for,in-args = "of-list";

                __if__ {
                    if,arg-equals = "default-dt", "$name";
                    default = "config-#name";
                };

                config-#name {
                    description = "conf-$name.dtb";
                    fdt = "fdt-#name";
                };
            };
        };
    };

[PATCH v4 1/3] binman: Allow selecting default FIT configuration

[Simon Glass]

Hi Alper,

On Tue, 8 Sep 2020 at 11:33, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> On 06/09/2020 19:39, Simon Glass wrote:
> > Add a new entry argument to the fit entry which allows selection of the
> > default configuration to use. This is the 'default' property in the
> > 'configurations' node.
> >
> > Update the Makefile to pass in the value of DEVICE_TREE or
> > CONFIG_DEFAULT_DEVICE_TREE to provide this information.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > Suggested-by: Michal Simek <michal.simek@xilinx.com>
> > ---
> >
> > Changes in v4:
> > - Add more documentation for DEFAULT-SEQ
>
> I might be too late to say this but the SEQ thing looks ugly to me.
> Maybe there could be some generic control-flow-like nodes that could
> generate and insert things in their own place. If it makes sense, I'm
> imagining something like:
>
>     fit {
>         images {
>             __for__ {
>                 for,variable = "name";
>                 for,in-args = "of-list";
>
>                 fdt-#name {
>                     description = "fdt-$name.dtb";
>                     type = "flat_dt";
>                     compression = "none";
>                 };
>             };
>         };
>
>         configurations {
>             __for__ {
>                 for,variable = "name"
>                 for,in-args = "of-list";
>
>                 __if__ {
>                     if,arg-equals = "default-dt", "$name";
>                     default = "config-#name";
>                 };
>
>                 config-#name {
>                     description = "conf-$name.dtb";
>                     fdt = "fdt-#name";
>                 };
>             };
>         };
>     };

I certainly like the flexibility of this and I fiddled with something
similar (without the __if__ as I didn't have 'default support) before
going with a hard-coded approach. I agree what I have is ugly and I'm
pleased to get some feedback.

I think we could use _for instead of __for__.

For $name I avoided that because it only works if there is a
non-character afterwards, e.g. $namex is ambiguous. I briefly played
with ${name} and {name} but ended up with the ugly capitals.

The biggest question in my mind is whether we want our device-tree
templates to be turing-complete. It seems nice but I feel that what
you have above is much harder to understand and get right.

Regards,
Simon

[PATCH v4 2/3] binman: Support help messages for missing blobs

[Simon Glass]

Hi Alper,

On Tue, 8 Sep 2020 at 10:37, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> On 06/09/2020 19:39, Simon Glass wrote:
> > When an external blob is missing it can be quite confusing for the user.
> > Add a way to provide a help message that is shown.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> > (no changes since v3)
> >
> > Changes in v3:
> > - Add a way to show help messages for missing blobs
> >
> >  tools/binman/README                        |  6 ++
> >  tools/binman/control.py                    | 69 +++++++++++++++++++++-
> >  tools/binman/entry.py                      |  9 +++
> >  tools/binman/ftest.py                      | 18 +++++-
> >  tools/binman/missing-blob-help             | 11 ++++
> >  tools/binman/test/168_fit_missing_blob.dts |  9 ++-
> >  6 files changed, 119 insertions(+), 3 deletions(-)
> >  create mode 100644 tools/binman/missing-blob-help
> >
> > diff --git a/tools/binman/README b/tools/binman/README
> > index 37ee3fc2d3b..f7bf285a915 100644
> > --- a/tools/binman/README
> > +++ b/tools/binman/README
> > @@ -343,6 +343,12 @@ compress:
> >       Sets the compression algortihm to use (for blobs only). See the entry
> >       documentation for details.
> >
> > +missing-msg:
> > +     Sets the tag of the message to show if this entry is missing. This is
> > +     used for external blobs. When they are missing it is helpful to show
> > +     information about what needs to be fixed. See missing-blob-help for the
> > +     message for each tag.
> > +
> >  The attributes supported for images and sections are described below. Several
> >  are similar to those for entries.
> >
> > diff --git a/tools/binman/control.py b/tools/binman/control.py
> > index 15bfac582a7..ee5771e7292 100644
> > --- a/tools/binman/control.py
> > +++ b/tools/binman/control.py
> > @@ -9,6 +9,7 @@ from collections import OrderedDict
> >  import glob
> >  import os
> >  import pkg_resources
> > +import re
> >
> >  import sys
> >  from patman import tools
> > @@ -22,6 +23,11 @@ from patman import tout
> >  # Make this global so that it can be referenced from tests
> >  images = OrderedDict()
> >
> > +# Help text for each type of missing blob, dict:
> > +#    key: Value of the entry's 'missing-msg' or entry name
> > +#    value: Text for the help
> > +missing_blob_help = {}
> > +
> >  def _ReadImageDesc(binman_node):
> >      """Read the image descriptions from the /binman node
> >
> > @@ -54,6 +60,66 @@ def _FindBinmanNode(dtb):
> >              return node
> >      return None
> >
> > +def _ReadMissingBlobHelp():
> > +    """Read the missing-blob-help file
> > +
> > +    This file containins help messages explaining what to do when external blobs
> > +    are missing.
> > +
> > +    Returns:
> > +        Dict:
> > +            key: Message tag (str)
> > +            value: Message text (str)
> > +    """
> > +
> > +    def _FinishTag(tag, msg, result):
> > +        if tag:
> > +            result[tag] = msg.rstrip()
> > +            tag = None
> > +            msg = ''
> > +        return tag, msg
> > +
> > +    my_data = pkg_resources.resource_string(__name__, 'missing-blob-help')
> > +    re_tag = re.compile('^([-a-z0-9]+):$')
> > +    result = {}
> > +    tag = None
> > +    msg = ''
> > +    for line in my_data.decode('utf-8').splitlines():
> > +        if not line.startswith('#'):
> > +            m_tag = re_tag.match(line)
> > +            if m_tag:
> > +                _, msg = _FinishTag(tag, msg, result)
> > +                tag = m_tag.group(1)
> > +            elif tag:
> > +                msg += line + '\n'
> > +    _FinishTag(tag, msg, result)
> > +    return result
>
> This looks a bit complex, I think something like this would be clearer:
>
>     result = {}
>     tag = None
>     for line in my_data.decode('utf-8').splitlines():
>         m_tag = re_tag.match(line)
>         if line.startswith('#'):
>             continue
>         elif m_tag:
>             tag = m_tag.group(1)
>             result[tag] = []
>         elif tag:
>             result[tag].append(line)
>     for tag, lines in result.items():
>         result[tag] = "".join(lines).rstrip()
>     return result
>

Yes that is easier, thank you. I'll use this in v4 and perhaps you can
reply with your sign-off.

Regards,
Simon

[PATCH v4 1/3] binman: Allow selecting default FIT configuration

[Alper Nebi Yasak]

On 09/09/2020 02:56, Simon Glass wrote:
> On Tue, 8 Sep 2020 at 11:33, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>> I might be too late to say this but the SEQ thing looks ugly to me.
>> Maybe there could be some generic control-flow-like nodes that could
>> generate and insert things in their own place. If it makes sense, I'm
>> imagining something like:
>>
>>     fit {
>>         images {
>>             __for__ {
>>                 for,variable = "name";
>>                 for,in-args = "of-list";
>>
>>                 fdt-#name {
>>                     description = "fdt-$name.dtb";
>>                     type = "flat_dt";
>>                     compression = "none";
>>                 };
>>             };
>>         };
>>
>>         configurations {
>>             __for__ {
>>                 for,variable = "name"
>>                 for,in-args = "of-list";
>>
>>                 __if__ {
>>                     if,arg-equals = "default-dt", "$name";
>>                     default = "config-#name";
>>                 };
>>
>>                 config-#name {
>>                     description = "conf-$name.dtb";
>>                     fdt = "fdt-#name";
>>                 };
>>             };
>>         };
>>     };
> 
> I certainly like the flexibility of this and I fiddled with something
> similar (without the __if__ as I didn't have 'default support) before
> going with a hard-coded approach. I agree what I have is ugly and I'm
> pleased to get some feedback.
> 
> I think we could use _for instead of __for__.

That works too, I just wanted it to feel very out-of-place as a node
name and that was the first thing to pop into my mind.

> For $name I avoided that because it only works if there is a
> non-character afterwards, e.g. $namex is ambiguous. I briefly played
> with ${name} and {name} but ended up with the ugly capitals.

I was thinking that it'd only substitute the thing in "for,variable" so
it wouldn't be ambiguous, but indeed ${name} is better.

> The biggest question in my mind is whether we want our device-tree
> templates to be turing-complete. It seems nice but I feel that what
> you have above is much harder to understand and get right.

That's true. At least, better to put it off until it's needed in other
places and then attempt a proper design. I'm not sure that something
like this has to be turing-complete to be useful, though it could easily
end up turing-complete by accident.

[PATCH v4 2/3] binman: Support help messages for missing blobs

[Alper Nebi Yasak]

On 09/09/2020 02:56, Simon Glass wrote:
> On Tue, 8 Sep 2020 at 10:37, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>>
>>     result = {}
>>     tag = None
>>     for line in my_data.decode('utf-8').splitlines():
>>         m_tag = re_tag.match(line)
>>         if line.startswith('#'):
>>             continue
>>         elif m_tag:
>>             tag = m_tag.group(1)
>>             result[tag] = []
>>         elif tag:
>>             result[tag].append(line)
>>     for tag, lines in result.items():
>>         result[tag] = "".join(lines).rstrip()
>>     return result
>>
> 
> Yes that is easier, thank you. I'll use this in v4 and perhaps you can
> reply with your sign-off.

The code fragment above is:

Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>

[PATCH v4 0/3] binman: Further updates for FIT support

[Samuel Holland]

On 9/7/20 3:44 AM, Michal Simek wrote:
> Hi Simon,
> 
> On 06. 09. 20 18:39, Simon Glass wrote:
>> This series adds support for help messages when binary blobs are missing,
>> as well as selecting the default FIT configuration.
>>
>> It includes the v3 patches from the earlier series that were not applied.
>>
>> Note: This series is available at u-boot-dm/binman-working and is based
>> on u-boot-dm/next
>>
>> Changes in v4:
>> - Add more documentation for DEFAULT-SEQ
>> - Drop patches previous applied to u-boot-dm/next
>>
>> Changes in v3:
>> - Add a way to show help messages for missing blobs
>> - Rebase on top of earlier binman series
>>
>> Changes in v2:
>> - Add new patch to allow selecting default FIT configuration
>>
>> Simon Glass (3):
>>   binman: Allow selecting default FIT configuration
>>   binman: Support help messages for missing blobs
>>   binman: sunxi: Add help message for missing sunxi ATF BL31
>>
>>  Makefile                                      |  2 +
>>  arch/arm/dts/sunxi-u-boot.dtsi                |  1 +
>>  tools/binman/README                           |  6 ++
>>  tools/binman/README.entries                   |  4 ++
>>  tools/binman/control.py                       | 69 ++++++++++++++++++-
>>  tools/binman/entry.py                         |  9 +++
>>  tools/binman/etype/fit.py                     | 26 +++++++
>>  tools/binman/ftest.py                         | 59 ++++++++++++++--
>>  tools/binman/missing-blob-help                | 15 ++++
>>  tools/binman/test/168_fit_missing_blob.dts    |  9 ++-
>>  .../test/{170_fit_fdt.dts => 172_fit_fdt.dts} |  2 +-
>>  11 files changed, 195 insertions(+), 7 deletions(-)
>>  create mode 100644 tools/binman/missing-blob-help
>>  rename tools/binman/test/{170_fit_fdt.dts => 172_fit_fdt.dts} (95%)
>>
> 
> I just spot one thing and will be good to clearify it.
> 
> In sunxi this description is used.
> +					firmware = "uboot";
> +					loadables = "atf";
> 
> But is this right way how this should be described?

No, this is backwards. I have sent a patch to fix this along with the script it
was copied from:

https://patchwork.ozlabs.org/project/uboot/patch/20200906032615.40448-8-samuel at sholland.org/

Cheers,
Samuel

> I personally use firmware which points to ATF and loadables which points
> to uboot. Not sure what sunxi is doing but also in spl_fit.c there is a
> comment about it.
> 	/*
> 	 * Find the U-Boot image using the following search order:
> 	 *   - start at 'firmware' (e.g. an ARM Trusted Firmware)
> 	 *   - fall back 'kernel' (e.g. a Falcon-mode OS boot
> 	 *   - fall back to using the first 'loadables' entry
> 	 */
> 
> Thanks,
> Michal
> 
> 
> 

[PATCH v4 0/3] binman: Further updates for FIT support

[Michal Simek]

On 12. 09. 20 22:43, Samuel Holland wrote:
> On 9/7/20 3:44 AM, Michal Simek wrote:
>> Hi Simon,
>>
>> On 06. 09. 20 18:39, Simon Glass wrote:
>>> This series adds support for help messages when binary blobs are missing,
>>> as well as selecting the default FIT configuration.
>>>
>>> It includes the v3 patches from the earlier series that were not applied.
>>>
>>> Note: This series is available at u-boot-dm/binman-working and is based
>>> on u-boot-dm/next
>>>
>>> Changes in v4:
>>> - Add more documentation for DEFAULT-SEQ
>>> - Drop patches previous applied to u-boot-dm/next
>>>
>>> Changes in v3:
>>> - Add a way to show help messages for missing blobs
>>> - Rebase on top of earlier binman series
>>>
>>> Changes in v2:
>>> - Add new patch to allow selecting default FIT configuration
>>>
>>> Simon Glass (3):
>>>   binman: Allow selecting default FIT configuration
>>>   binman: Support help messages for missing blobs
>>>   binman: sunxi: Add help message for missing sunxi ATF BL31
>>>
>>>  Makefile                                      |  2 +
>>>  arch/arm/dts/sunxi-u-boot.dtsi                |  1 +
>>>  tools/binman/README                           |  6 ++
>>>  tools/binman/README.entries                   |  4 ++
>>>  tools/binman/control.py                       | 69 ++++++++++++++++++-
>>>  tools/binman/entry.py                         |  9 +++
>>>  tools/binman/etype/fit.py                     | 26 +++++++
>>>  tools/binman/ftest.py                         | 59 ++++++++++++++--
>>>  tools/binman/missing-blob-help                | 15 ++++
>>>  tools/binman/test/168_fit_missing_blob.dts    |  9 ++-
>>>  .../test/{170_fit_fdt.dts => 172_fit_fdt.dts} |  2 +-
>>>  11 files changed, 195 insertions(+), 7 deletions(-)
>>>  create mode 100644 tools/binman/missing-blob-help
>>>  rename tools/binman/test/{170_fit_fdt.dts => 172_fit_fdt.dts} (95%)
>>>
>>
>> I just spot one thing and will be good to clearify it.
>>
>> In sunxi this description is used.
>> +					firmware = "uboot";
>> +					loadables = "atf";
>>
>> But is this right way how this should be described?
> 
> No, this is backwards. I have sent a patch to fix this along with the script it
> was copied from:
> 
> https://patchwork.ozlabs.org/project/uboot/patch/20200906032615.40448-8-samuel at sholland.org/

great.

Thanks,
Michal

[PATCH v4 2/3] binman: Support help messages for missing blobs

[Simon Glass]

On 09/09/2020 02:56, Simon Glass wrote:
> On Tue, 8 Sep 2020 at 10:37, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>>
>>     result = {}
>>     tag = None
>>     for line in my_data.decode('utf-8').splitlines():
>>         m_tag = re_tag.match(line)
>>         if line.startswith('#'):
>>             continue
>>         elif m_tag:
>>             tag = m_tag.group(1)
>>             result[tag] = []
>>         elif tag:
>>             result[tag].append(line)
>>     for tag, lines in result.items():
>>         result[tag] = "".join(lines).rstrip()
>>     return result
>>
>
> Yes that is easier, thank you. I'll use this in v4 and perhaps you can
> reply with your sign-off.

The code fragment above is:

Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>

Applied to u-boot-dm/next, thanks!

[PATCH v4 1/3] binman: Allow selecting default FIT configuration

[Simon Glass]

On 09/09/2020 02:56, Simon Glass wrote:
> On Tue, 8 Sep 2020 at 11:33, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>> I might be too late to say this but the SEQ thing looks ugly to me.
>> Maybe there could be some generic control-flow-like nodes that could
>> generate and insert things in their own place. If it makes sense, I'm
>> imagining something like:
>>
>>     fit {
>>         images {
>>             __for__ {
>>                 for,variable = "name";
>>                 for,in-args = "of-list";
>>
>>                 fdt-#name {
>>                     description = "fdt-$name.dtb";
>>                     type = "flat_dt";
>>                     compression = "none";
>>                 };
>>             };
>>         };
>>
>>         configurations {
>>             __for__ {
>>                 for,variable = "name"
>>                 for,in-args = "of-list";
>>
>>                 __if__ {
>>                     if,arg-equals = "default-dt", "$name";
>>                     default = "config-#name";
>>                 };
>>
>>                 config-#name {
>>                     description = "conf-$name.dtb";
>>                     fdt = "fdt-#name";
>>                 };
>>             };
>>         };
>>     };
>
> I certainly like the flexibility of this and I fiddled with something
> similar (without the __if__ as I didn't have 'default support) before
> going with a hard-coded approach. I agree what I have is ugly and I'm
> pleased to get some feedback.
>
> I think we could use _for instead of __for__.

That works too, I just wanted it to feel very out-of-place as a node
name and that was the first thing to pop into my mind.

> For $name I avoided that because it only works if there is a
> non-character afterwards, e.g. $namex is ambiguous. I briefly played
> with ${name} and {name} but ended up with the ugly capitals.

I was thinking that it'd only substitute the thing in "for,variable" so
it wouldn't be ambiguous, but indeed ${name} is better.

> The biggest question in my mind is whether we want our device-tree
> templates to be turing-complete. It seems nice but I feel that what
> you have above is much harder to understand and get right.

That's true. At least, better to put it off until it's needed in other
places and then attempt a proper design. I'm not sure that something
like this has to be turing-complete to be useful, though it could easily
end up turing-complete by accident.

Applied to u-boot-dm/next, thanks!