[PATCH v4 0/3] binman: Further updates for FIT support
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%)
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";
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"; }; }; }; };
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
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.
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!
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"; + }; }; }; };
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 = Nonemsg = ''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 messagefor tag in tags:if tag in missing_blob_help:_ShowBlobHelp(entry._node.path, missing_blob_help[tag])breakdef 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)@@ -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()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 helpReturns: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";}; }; };
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 isused for external blobs. When they are missing it is helpful to showinformation about what needs to be fixed. See missing-blob-help for themessage 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 = Nonemsg = ''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
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 resultYes 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
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 resultYes 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!
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
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
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
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-samu...
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
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-samu...
great.
Thanks, Michal
participants (4)
-
Alper Nebi Yasak -
Michal Simek -
Samuel Holland -
Simon Glass