All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/5] binman: Improvements to FIT entry type
@ 2022-02-07 22:08 Alper Nebi Yasak
  2022-02-07 22:08 ` [PATCH v2 1/5] binman: Fix subentry expansion for " Alper Nebi Yasak
                   ` (7 more replies)
  0 siblings, 8 replies; 27+ messages in thread
From: Alper Nebi Yasak @ 2022-02-07 22:08 UTC (permalink / raw)
  To: u-boot; +Cc: Heiko Thiery, Jan Kiszka, Simon Glass, Alper Nebi Yasak

I've managed to build images like in doc/chromium/chainload.rst wtih
binman, but ran into an issue with entry expansion in FIT and worked on
it a bit. I also added SetImagePos() because that documentation asks for
precise placement of u-boot.bin inside the FIT and I felt like doing it
as an easier way to know the positions.

I would try to refactor and experiment with FIT things more, but I know
Simon's currently working on converting SPL_FIT_GENERATOR to binman.
Instead I'm just sending things I have already done with some tests
added, hopefully without too many conflicts.

Changes in v2:
- Split reused testSimpleFit code into a helper function
- Check missing_bintools list instead of catching Fdt exceptions
- Add tag: "Reviewed-by: Simon Glass <sjg@chromium.org>"

v1: https://patchwork.ozlabs.org/project/uboot/list/?series=284714

Alper Nebi Yasak (5):
  binman: Fix subentry expansion for FIT entry type
  binman: Register and check bintools from FIT subentries
  binman: Check missing bintools of Section subclasses
  binman: Convert FIT entry type to a subclass of Section entry type
  binman: Update image positions of FIT subentries

 tools/binman/etype/fit.py                     |  90 ++++++---
 tools/binman/etype/section.py                 |   1 +
 tools/binman/ftest.py                         | 171 +++++++++++++++++-
 .../binman/test/220_fit_subentry_bintool.dts  |  39 ++++
 4 files changed, 266 insertions(+), 35 deletions(-)
 create mode 100644 tools/binman/test/220_fit_subentry_bintool.dts

-- 
2.34.1


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v2 1/5] binman: Fix subentry expansion for FIT entry type
  2022-02-07 22:08 [PATCH v2 0/5] binman: Improvements to FIT entry type Alper Nebi Yasak
@ 2022-02-07 22:08 ` Alper Nebi Yasak
  2022-02-08 15:05   ` Simon Glass
  2022-02-08 20:39   ` Simon Glass
  2022-02-07 22:08 ` [PATCH v2 2/5] binman: Register and check bintools from FIT subentries Alper Nebi Yasak
                   ` (6 subsequent siblings)
  7 siblings, 2 replies; 27+ messages in thread
From: Alper Nebi Yasak @ 2022-02-07 22:08 UTC (permalink / raw)
  To: u-boot; +Cc: Heiko Thiery, Jan Kiszka, Simon Glass, Alper Nebi Yasak

Binman tries to expand some entries into parts that make it up, e.g.
'u-boot' into a 'u-boot-expanded' section that contains 'u-boot-nodtb'
and 'u-boot-dtb'. Entries with child entries must call ExpandEntries()
on them to build a correct image, as it's possible that unexpanded child
entries have no data of their own. The FIT entry type doesn't currently
do this, which means putting a "u-boot" entry inside it doesn't work as
expected.

Implement ExpandEntries() for FIT and add a copy of a simple FIT image
test that checks subentry expansion in FIT entries.

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

Changes in v2:
- Split reused testSimpleFit code into a helper function

 tools/binman/etype/fit.py |  5 +++++
 tools/binman/ftest.py     | 33 ++++++++++++++++++++++++---------
 2 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
index 6ad4a686df55..38bc2a2d784e 100644
--- a/tools/binman/etype/fit.py
+++ b/tools/binman/etype/fit.py
@@ -237,6 +237,11 @@ def _AddNode(base_node, depth, node):
         self._fdt = Fdt.FromData(fdt.as_bytearray())
         self._fdt.Scan()
 
+    def ExpandEntries(self):
+        super().ExpandEntries()
+        for section in self._fit_sections.values():
+            section.ExpandEntries()
+
     def ObtainContents(self):
         """Obtain the contents of the FIT
 
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index 5400f76c676a..626df786c8c9 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -61,6 +61,9 @@
 U_BOOT_NODTB_DATA     = b'nodtb with microcode pointer somewhere in here'
 U_BOOT_SPL_NODTB_DATA = b'splnodtb with microcode pointer somewhere in here'
 U_BOOT_TPL_NODTB_DATA = b'tplnodtb with microcode pointer somewhere in here'
+U_BOOT_EXP_DATA       = U_BOOT_NODTB_DATA + U_BOOT_DTB_DATA
+U_BOOT_SPL_EXP_DATA   = U_BOOT_SPL_NODTB_DATA + U_BOOT_SPL_DTB_DATA
+U_BOOT_TPL_EXP_DATA   = U_BOOT_TPL_NODTB_DATA + U_BOOT_TPL_DTB_DATA
 FSP_DATA              = b'fsp'
 CMC_DATA              = b'cmc'
 VBT_DATA              = b'vbt'
@@ -3713,13 +3716,7 @@ def testPackOverlap(self):
         """Test that zero-size overlapping regions are ignored"""
         self._DoTestFile('160_pack_overlap_zero.dts')
 
-    def testSimpleFit(self):
-        """Test an image with a FIT inside"""
-        data = self._DoReadFile('161_fit.dts')
-        self.assertEqual(U_BOOT_DATA, data[:len(U_BOOT_DATA)])
-        self.assertEqual(U_BOOT_NODTB_DATA, data[-len(U_BOOT_NODTB_DATA):])
-        fit_data = data[len(U_BOOT_DATA):-len(U_BOOT_NODTB_DATA)]
-
+    def _CheckSimpleFitData(self, fit_data, kernel_data, fdt1_data):
         # The data should be inside the FIT
         dtb = fdt.Fdt.FromData(fit_data)
         dtb.Scan()
@@ -3752,8 +3749,26 @@ def testSimpleFit(self):
         self.assertIsNotNone(data_sizes)
         self.assertEqual(2, len(data_sizes))
         # Format is "4 Bytes = 0.00 KiB = 0.00 MiB" so take the first word
-        self.assertEqual(len(U_BOOT_DATA), int(data_sizes[0].split()[0]))
-        self.assertEqual(len(U_BOOT_SPL_DTB_DATA), int(data_sizes[1].split()[0]))
+        self.assertEqual(len(kernel_data), int(data_sizes[0].split()[0]))
+        self.assertEqual(len(fdt1_data), int(data_sizes[1].split()[0]))
+
+    def testSimpleFit(self):
+        """Test an image with a FIT inside"""
+        data = self._DoReadFile('161_fit.dts')
+        self.assertEqual(U_BOOT_DATA, data[:len(U_BOOT_DATA)])
+        self.assertEqual(U_BOOT_NODTB_DATA, data[-len(U_BOOT_NODTB_DATA):])
+        fit_data = data[len(U_BOOT_DATA):-len(U_BOOT_NODTB_DATA)]
+
+        self._CheckSimpleFitData(fit_data, U_BOOT_DATA, U_BOOT_SPL_DTB_DATA)
+
+    def testSimpleFitExpandsSubentries(self):
+        """Test that FIT images expand their subentries"""
+        data = self._DoReadFileDtb('161_fit.dts', use_expanded=True)[0]
+        self.assertEqual(U_BOOT_EXP_DATA, data[:len(U_BOOT_EXP_DATA)])
+        self.assertEqual(U_BOOT_NODTB_DATA, data[-len(U_BOOT_NODTB_DATA):])
+        fit_data = data[len(U_BOOT_EXP_DATA):-len(U_BOOT_NODTB_DATA)]
+
+        self._CheckSimpleFitData(fit_data, U_BOOT_EXP_DATA, U_BOOT_SPL_DTB_DATA)
 
     def testFitExternal(self):
         """Test an image with an FIT with external images"""
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v2 2/5] binman: Register and check bintools from FIT subentries
  2022-02-07 22:08 [PATCH v2 0/5] binman: Improvements to FIT entry type Alper Nebi Yasak
  2022-02-07 22:08 ` [PATCH v2 1/5] binman: Fix subentry expansion for " Alper Nebi Yasak
@ 2022-02-07 22:08 ` Alper Nebi Yasak
  2022-02-07 22:08 ` [PATCH v2 3/5] binman: Check missing bintools of Section subclasses Alper Nebi Yasak
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 27+ messages in thread
From: Alper Nebi Yasak @ 2022-02-07 22:08 UTC (permalink / raw)
  To: u-boot; +Cc: Heiko Thiery, Jan Kiszka, Simon Glass, Alper Nebi Yasak

Binman keeps track of binary tools each entry wants to use. The
implementation of this for the FIT entry only adds "mkimage", but not
the tools that would be used by its subentries.

Register the binary tools that FIT subentries will use in addition to
the one FIT itself uses, and check their existence by copying the
appropriate method from Section entry type. Also add tests that check if
these subentries can use and warn about binary tools.

Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---

Changes in v2:
- Add tag: "Reviewed-by: Simon Glass <sjg@chromium.org>"

 tools/binman/etype/fit.py                     | 14 +++++++
 tools/binman/ftest.py                         | 25 ++++++++++++
 .../binman/test/220_fit_subentry_bintool.dts  | 39 +++++++++++++++++++
 3 files changed, 78 insertions(+)
 create mode 100644 tools/binman/test/220_fit_subentry_bintool.dts

diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
index 38bc2a2d784e..9dffcd5adbd6 100644
--- a/tools/binman/etype/fit.py
+++ b/tools/binman/etype/fit.py
@@ -311,4 +311,18 @@ def SetAllowMissing(self, allow_missing):
             section.SetAllowMissing(allow_missing)
 
     def AddBintools(self, tools):
+        for section in self._fit_sections.values():
+            section.AddBintools(tools)
         self.mkimage = self.AddBintool(tools, 'mkimage')
+
+    def check_missing_bintools(self, missing_list):
+        """Check if any entries in this section have missing bintools
+
+        If there are missing bintools, these are added to the list
+
+        Args:
+            missing_list: List of Bintool objects to be added to
+        """
+        super().check_missing_bintools(missing_list)
+        for entry in self._fit_sections.values():
+            entry.check_missing_bintools(missing_list)
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index 626df786c8c9..bc073587cc07 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -5133,6 +5133,31 @@ def testListWithGenNode(self):
         finally:
             shutil.rmtree(tmpdir)
 
+    def testFitSubentryUsesBintool(self):
+        """Test that binman FIT subentries can use bintools"""
+        command.test_result = self._HandleGbbCommand
+        entry_args = {
+            'keydir': 'devkeys',
+            'bmpblk': 'bmpblk.bin',
+        }
+        data, _, _, _ = self._DoReadFileDtb('220_fit_subentry_bintool.dts',
+                entry_args=entry_args)
+
+        expected = (GBB_DATA + GBB_DATA + tools.GetBytes(0, 8) +
+                    tools.GetBytes(0, 0x2180 - 16))
+        self.assertIn(expected, data)
+
+    def testFitSubentryMissingBintool(self):
+        """Test that binman reports missing bintools for FIT subentries"""
+        entry_args = {
+            'keydir': 'devkeys',
+        }
+        with test_util.capture_sys_output() as (_, stderr):
+            self._DoTestFile('220_fit_subentry_bintool.dts',
+                    force_missing_bintools='futility', entry_args=entry_args)
+        err = stderr.getvalue()
+        self.assertRegex(err,
+                         "Image 'main-section'.*missing bintools.*: futility")
 
 if __name__ == "__main__":
     unittest.main()
diff --git a/tools/binman/test/220_fit_subentry_bintool.dts b/tools/binman/test/220_fit_subentry_bintool.dts
new file mode 100644
index 000000000000..6e29d41eeb33
--- /dev/null
+++ b/tools/binman/test/220_fit_subentry_bintool.dts
@@ -0,0 +1,39 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	binman {
+		fit {
+			description = "test-desc";
+			#address-cells = <1>;
+
+			images {
+				test {
+					description = "Something using a bintool";
+					type = "kernel";
+					arch = "arm";
+					os = "linux";
+					compression = "gzip";
+					load = <00000000>;
+					entry = <00000000>;
+
+					gbb {
+						size = <0x2180>;
+					};
+				};
+			};
+
+			configurations {
+				default = "conf-1";
+				conf-1 {
+					description = "Boot bintool output";
+					kernel = "kernel";
+				};
+			};
+		};
+	};
+};
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v2 3/5] binman: Check missing bintools of Section subclasses
  2022-02-07 22:08 [PATCH v2 0/5] binman: Improvements to FIT entry type Alper Nebi Yasak
  2022-02-07 22:08 ` [PATCH v2 1/5] binman: Fix subentry expansion for " Alper Nebi Yasak
  2022-02-07 22:08 ` [PATCH v2 2/5] binman: Register and check bintools from FIT subentries Alper Nebi Yasak
@ 2022-02-07 22:08 ` Alper Nebi Yasak
  2022-02-07 22:08 ` [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type Alper Nebi Yasak
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 27+ messages in thread
From: Alper Nebi Yasak @ 2022-02-07 22:08 UTC (permalink / raw)
  To: u-boot; +Cc: Heiko Thiery, Jan Kiszka, Simon Glass, Alper Nebi Yasak

Binman can check for missing binary tools and prints warnings if
anything required for an image is missing. The implementation of this
for the Section entry only checks the subentries, presumably because
Section does not use any binary tools itself. However, this means the
check is also skipped for subclasses of Section which might need binary
tools.

Make sure missing binary tools are checked for subclasses of the Section
entry type as well, by calling the parent class' implementation in
the relevant Section method.

Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
This starts being tested by testFitMissing in the next patch when FIT
becomes a Section subclass.

Changes in v2:
- Add tag: "Reviewed-by: Simon Glass <sjg@chromium.org>"

 tools/binman/etype/section.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py
index bb375e9063df..213a510a8d5b 100644
--- a/tools/binman/etype/section.py
+++ b/tools/binman/etype/section.py
@@ -840,6 +840,7 @@ def check_missing_bintools(self, missing_list):
         Args:
             missing_list: List of Bintool objects to be added to
         """
+        super().check_missing_bintools(missing_list)
         for entry in self._entries.values():
             entry.check_missing_bintools(missing_list)
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type
  2022-02-07 22:08 [PATCH v2 0/5] binman: Improvements to FIT entry type Alper Nebi Yasak
                   ` (2 preceding siblings ...)
  2022-02-07 22:08 ` [PATCH v2 3/5] binman: Check missing bintools of Section subclasses Alper Nebi Yasak
@ 2022-02-07 22:08 ` Alper Nebi Yasak
  2022-02-14  9:09   ` Jan Kiszka
  2022-02-07 22:08 ` [PATCH v2 5/5] binman: Update image positions of FIT subentries Alper Nebi Yasak
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 27+ messages in thread
From: Alper Nebi Yasak @ 2022-02-07 22:08 UTC (permalink / raw)
  To: u-boot; +Cc: Heiko Thiery, Jan Kiszka, Simon Glass, Alper Nebi Yasak

The binman FIT entry type shares some code with the Section entry type.
This shared code is bound to grow, since FIT entries are conceptually a
variation of Section entries.

Make FIT entry type a subclass of Section entry type, simplifying it a
bit and providing us the features that Section implements. Also fix the
subentry alignment test which now attempts to write symbols to a
nonexistent SPL ELF test file by creating it first.

Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---

Changes in v2:
- Add tag: "Reviewed-by: Simon Glass <sjg@chromium.org>"

 tools/binman/etype/fit.py | 70 +++++++++++----------------------------
 tools/binman/ftest.py     |  1 +
 2 files changed, 20 insertions(+), 51 deletions(-)

diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
index 9dffcd5adbd6..0f8c88a9720e 100644
--- a/tools/binman/etype/fit.py
+++ b/tools/binman/etype/fit.py
@@ -9,11 +9,12 @@
 import libfdt
 
 from binman.entry import Entry, EntryArg
+from binman.etype.section import Entry_section
 from dtoc import fdt_util
 from dtoc.fdt import Fdt
 from patman import tools
 
-class Entry_fit(Entry):
+class Entry_fit(Entry_section):
     """Flat Image Tree (FIT)
 
     This calls mkimage to create a FIT (U-Boot Flat Image Tree) based on the
@@ -112,15 +113,15 @@ def __init__(self, section, etype, node):
         """
         Members:
             _fit: FIT file being built
-            _fit_sections: dict:
+            _entries: dict from Entry_section:
                 key: relative path to entry Node (from the base of the FIT)
                 value: Entry_section object comprising the contents of this
                     node
         """
         super().__init__(section, etype, node)
         self._fit = None
-        self._fit_sections = {}
         self._fit_props = {}
+
         for pname, prop in self._node.props.items():
             if pname.startswith('fit,'):
                 self._fit_props[pname] = prop
@@ -185,7 +186,7 @@ def _AddNode(base_node, depth, node):
                 # 'data' property later.
                 entry = Entry.Create(self.section, node, etype='section')
                 entry.ReadNode()
-                self._fit_sections[rel_path] = entry
+                self._entries[rel_path] = entry
 
             for subnode in node.subnodes:
                 if has_images and not (subnode.name.startswith('hash') or
@@ -237,18 +238,19 @@ def _AddNode(base_node, depth, node):
         self._fdt = Fdt.FromData(fdt.as_bytearray())
         self._fdt.Scan()
 
-    def ExpandEntries(self):
-        super().ExpandEntries()
-        for section in self._fit_sections.values():
-            section.ExpandEntries()
-
-    def ObtainContents(self):
-        """Obtain the contents of the FIT
+    def BuildSectionData(self, required):
+        """Build FIT entry contents
 
         This adds the 'data' properties to the input ITB (Image-tree Binary)
         then runs mkimage to process it.
+
+        Args:
+            required: True if the data must be present, False if it is OK to
+                return None
+
+        Returns:
+            Contents of the section (bytes)
         """
-        # self._BuildInput() either returns bytes or raises an exception.
         data = self._BuildInput(self._fdt)
         uniq = self.GetUniqueName()
         input_fname = tools.GetOutputFilename('%s.itb' % uniq)
@@ -264,14 +266,12 @@ def ObtainContents(self):
                 'pad': fdt_util.fdt32_to_cpu(ext_offset.value)
                 }
         if self.mkimage.run(reset_timestamp=True, output_fname=output_fname,
-                            **args) is not None:
-            self.SetContents(tools.ReadFile(output_fname))
-        else:
+                            **args) is None:
             # Bintool is missing; just use empty data as the output
             self.record_missing_bintool(self.mkimage)
-            self.SetContents(tools.GetBytes(0, 1024))
+            return tools.GetBytes(0, 1024)
 
-        return True
+        return tools.ReadFile(output_fname)
 
     def _BuildInput(self, fdt):
         """Finish the FIT by adding the 'data' properties to it
@@ -282,12 +282,8 @@ def _BuildInput(self, fdt):
         Returns:
             New fdt contents (bytes)
         """
-        for path, section in self._fit_sections.items():
+        for path, section in self._entries.items():
             node = fdt.GetNode(path)
-            # Entry_section.ObtainContents() either returns True or
-            # raises an exception.
-            section.ObtainContents()
-            section.Pack(0)
             data = section.GetData()
             node.AddData('data', data)
 
@@ -295,34 +291,6 @@ def _BuildInput(self, fdt):
         data = fdt.GetContents()
         return data
 
-    def CheckMissing(self, missing_list):
-        """Check if any entries in this FIT have missing external blobs
-
-        If there are missing blobs, the entries are added to the list
-
-        Args:
-            missing_list: List of Entry objects to be added to
-        """
-        for path, section in self._fit_sections.items():
-            section.CheckMissing(missing_list)
-
-    def SetAllowMissing(self, allow_missing):
-        for section in self._fit_sections.values():
-            section.SetAllowMissing(allow_missing)
-
     def AddBintools(self, tools):
-        for section in self._fit_sections.values():
-            section.AddBintools(tools)
+        super().AddBintools(tools)
         self.mkimage = self.AddBintool(tools, 'mkimage')
-
-    def check_missing_bintools(self, missing_list):
-        """Check if any entries in this section have missing bintools
-
-        If there are missing bintools, these are added to the list
-
-        Args:
-            missing_list: List of Bintool objects to be added to
-        """
-        super().check_missing_bintools(missing_list)
-        for entry in self._fit_sections.values():
-            entry.check_missing_bintools(missing_list)
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index bc073587cc07..f16798960b84 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -3850,6 +3850,7 @@ def testPadInSections(self):
 
     def testFitImageSubentryAlignment(self):
         """Test relative alignability of FIT image subentries"""
+        self._SetupSplElf()
         entry_args = {
             'test-id': TEXT_DATA,
         }
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v2 5/5] binman: Update image positions of FIT subentries
  2022-02-07 22:08 [PATCH v2 0/5] binman: Improvements to FIT entry type Alper Nebi Yasak
                   ` (3 preceding siblings ...)
  2022-02-07 22:08 ` [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type Alper Nebi Yasak
@ 2022-02-07 22:08 ` Alper Nebi Yasak
  2022-02-08 20:43   ` Simon Glass
  2022-02-23  2:35   ` Simon Glass
  2022-02-08 20:39 ` [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type Simon Glass
                   ` (2 subsequent siblings)
  7 siblings, 2 replies; 27+ messages in thread
From: Alper Nebi Yasak @ 2022-02-07 22:08 UTC (permalink / raw)
  To: u-boot; +Cc: Heiko Thiery, Jan Kiszka, Simon Glass, Alper Nebi Yasak

Binman keeps track of positions of each entry in the final image, but
currently this data is wrong for things included in FIT entries,
especially since a previous patch makes FIT a subclass of Section and
inherit its implementation.

There are three ways to put data into a FIT image. It can be directly
included as a "data" property, or it can be external to the FIT image
represented by an offset-size pair of properties. This external offset
is either "data-position" from the start of the FIT or "data-offset"
from the end of the FIT, and the size is "data-size" for both. However,
binman doesn't use the "data-offset" method while building FIT entries.

According to the Section docstring, its subclasses should calculate and
set the correct offsets and sizes in SetImagePos() method. Do this for
FIT subentries for the three ways mentioned above, and add tests for the
two ways binman can pack them in.

Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---

Changes in v2:
- Check missing_bintools list instead of catching Fdt exceptions
- Add tag: "Reviewed-by: Simon Glass <sjg@chromium.org>"

 tools/binman/etype/fit.py |  51 +++++++++++++++++
 tools/binman/ftest.py     | 112 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 163 insertions(+)

diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
index 0f8c88a9720e..5497f036a26d 100644
--- a/tools/binman/etype/fit.py
+++ b/tools/binman/etype/fit.py
@@ -291,6 +291,57 @@ def _BuildInput(self, fdt):
         data = fdt.GetContents()
         return data
 
+    def SetImagePos(self, image_pos):
+        """Set the position in the image
+
+        This sets each subentry's offsets, sizes and positions-in-image
+        according to where they ended up in the packed FIT file.
+
+        Args:
+            image_pos: Position of this entry in the image
+        """
+        super().SetImagePos(image_pos)
+
+        # If mkimage is missing we'll have empty data,
+        # which will cause a FDT_ERR_BADMAGIC error
+        if self.mkimage in self.missing_bintools:
+            return
+
+        fdt = Fdt.FromData(self.GetData())
+        fdt.Scan()
+
+        for path, section in self._entries.items():
+            node = fdt.GetNode(path)
+
+            data_prop = node.props.get("data")
+            data_pos = fdt_util.GetInt(node, "data-position")
+            data_offset = fdt_util.GetInt(node, "data-offset")
+            data_size = fdt_util.GetInt(node, "data-size")
+
+            # Contents are inside the FIT
+            if data_prop is not None:
+                # GetOffset() returns offset of a fdt_property struct,
+                # which has 3 fdt32_t members before the actual data.
+                offset = data_prop.GetOffset() + 12
+                size = len(data_prop.bytes)
+
+            # External offset from the base of the FIT
+            elif data_pos is not None:
+                offset = data_pos
+                size = data_size
+
+            # External offset from the end of the FIT, not used in binman
+            elif data_offset is not None: # pragma: no cover
+                offset = fdt.GetFdtObj().totalsize() + data_offset
+                size = data_size
+
+            # This should never happen
+            else: # pragma: no cover
+                self.Raise("%s: missing data properties" % (path))
+
+            section.SetOffsetSize(offset, size)
+            section.SetImagePos(self.image_pos)
+
     def AddBintools(self, tools):
         super().AddBintools(tools)
         self.mkimage = self.AddBintool(tools, 'mkimage')
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index f16798960b84..ab988565c335 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -3770,6 +3770,62 @@ def testSimpleFitExpandsSubentries(self):
 
         self._CheckSimpleFitData(fit_data, U_BOOT_EXP_DATA, U_BOOT_SPL_DTB_DATA)
 
+    def testSimpleFitImagePos(self):
+        """Test that we have correct image-pos for FIT subentries"""
+        data, _, _, out_dtb_fname = self._DoReadFileDtb('161_fit.dts',
+                                                        update_dtb=True)
+        dtb = fdt.Fdt(out_dtb_fname)
+        dtb.Scan()
+        props = self._GetPropTree(dtb, BASE_DTB_PROPS + REPACK_DTB_PROPS)
+
+        self.assertEqual({
+            'image-pos': 0,
+            'offset': 0,
+            'size': 1890,
+
+            'u-boot:image-pos': 0,
+            'u-boot:offset': 0,
+            'u-boot:size': 4,
+
+            'fit:image-pos': 4,
+            'fit:offset': 4,
+            'fit:size': 1840,
+
+            'fit/images/kernel:image-pos': 160,
+            'fit/images/kernel:offset': 156,
+            'fit/images/kernel:size': 4,
+
+            'fit/images/kernel/u-boot:image-pos': 160,
+            'fit/images/kernel/u-boot:offset': 0,
+            'fit/images/kernel/u-boot:size': 4,
+
+            'fit/images/fdt-1:image-pos': 456,
+            'fit/images/fdt-1:offset': 452,
+            'fit/images/fdt-1:size': 6,
+
+            'fit/images/fdt-1/u-boot-spl-dtb:image-pos': 456,
+            'fit/images/fdt-1/u-boot-spl-dtb:offset': 0,
+            'fit/images/fdt-1/u-boot-spl-dtb:size': 6,
+
+            'u-boot-nodtb:image-pos': 1844,
+            'u-boot-nodtb:offset': 1844,
+            'u-boot-nodtb:size': 46,
+        }, props)
+
+        # Actually check the data is where we think it is
+        for node, expected in [
+            ("u-boot", U_BOOT_DATA),
+            ("fit/images/kernel", U_BOOT_DATA),
+            ("fit/images/kernel/u-boot", U_BOOT_DATA),
+            ("fit/images/fdt-1", U_BOOT_SPL_DTB_DATA),
+            ("fit/images/fdt-1/u-boot-spl-dtb", U_BOOT_SPL_DTB_DATA),
+            ("u-boot-nodtb", U_BOOT_NODTB_DATA),
+        ]:
+            image_pos = props[f"{node}:image-pos"]
+            size = props[f"{node}:size"]
+            self.assertEqual(len(expected), size)
+            self.assertEqual(expected, data[image_pos:image_pos+size])
+
     def testFitExternal(self):
         """Test an image with an FIT with external images"""
         data = self._DoReadFile('162_fit_external.dts')
@@ -3798,6 +3854,62 @@ def testFitExternal(self):
         self.assertEqual(U_BOOT_DATA + b'aa',
                          data[actual_pos:actual_pos + external_data_size])
 
+    def testFitExternalImagePos(self):
+        """Test that we have correct image-pos for external FIT subentries"""
+        data, _, _, out_dtb_fname = self._DoReadFileDtb('162_fit_external.dts',
+                                                        update_dtb=True)
+        dtb = fdt.Fdt(out_dtb_fname)
+        dtb.Scan()
+        props = self._GetPropTree(dtb, BASE_DTB_PROPS + REPACK_DTB_PROPS)
+
+        self.assertEqual({
+            'image-pos': 0,
+            'offset': 0,
+            'size': 1082,
+
+            'u-boot:image-pos': 0,
+            'u-boot:offset': 0,
+            'u-boot:size': 4,
+
+            'fit:size': 1032,
+            'fit:offset': 4,
+            'fit:image-pos': 4,
+
+            'fit/images/kernel:size': 4,
+            'fit/images/kernel:offset': 1024,
+            'fit/images/kernel:image-pos': 1028,
+
+            'fit/images/kernel/u-boot:size': 4,
+            'fit/images/kernel/u-boot:offset': 0,
+            'fit/images/kernel/u-boot:image-pos': 1028,
+
+            'fit/images/fdt-1:size': 2,
+            'fit/images/fdt-1:offset': 1028,
+            'fit/images/fdt-1:image-pos': 1032,
+
+            'fit/images/fdt-1/_testing:size': 2,
+            'fit/images/fdt-1/_testing:offset': 0,
+            'fit/images/fdt-1/_testing:image-pos': 1032,
+
+            'u-boot-nodtb:image-pos': 1036,
+            'u-boot-nodtb:offset': 1036,
+            'u-boot-nodtb:size': 46,
+         }, props)
+
+        # Actually check the data is where we think it is
+        for node, expected in [
+            ("u-boot", U_BOOT_DATA),
+            ("fit/images/kernel", U_BOOT_DATA),
+            ("fit/images/kernel/u-boot", U_BOOT_DATA),
+            ("fit/images/fdt-1", b'aa'),
+            ("fit/images/fdt-1/_testing", b'aa'),
+            ("u-boot-nodtb", U_BOOT_NODTB_DATA),
+        ]:
+            image_pos = props[f"{node}:image-pos"]
+            size = props[f"{node}:size"]
+            self.assertEqual(len(expected), size)
+            self.assertEqual(expected, data[image_pos:image_pos+size])
+
     def testFitMissing(self):
         """Test that binman still produces a FIT image if mkimage is missing"""
         with test_util.capture_sys_output() as (_, stderr):
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 1/5] binman: Fix subentry expansion for FIT entry type
  2022-02-07 22:08 ` [PATCH v2 1/5] binman: Fix subentry expansion for " Alper Nebi Yasak
@ 2022-02-08 15:05   ` Simon Glass
  2022-02-08 20:39   ` Simon Glass
  1 sibling, 0 replies; 27+ messages in thread
From: Simon Glass @ 2022-02-08 15:05 UTC (permalink / raw)
  To: Alper Nebi Yasak; +Cc: U-Boot Mailing List, Heiko Thiery, Jan Kiszka

On Mon, 7 Feb 2022 at 15:08, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> Binman tries to expand some entries into parts that make it up, e.g.
> 'u-boot' into a 'u-boot-expanded' section that contains 'u-boot-nodtb'
> and 'u-boot-dtb'. Entries with child entries must call ExpandEntries()
> on them to build a correct image, as it's possible that unexpanded child
> entries have no data of their own. The FIT entry type doesn't currently
> do this, which means putting a "u-boot" entry inside it doesn't work as
> expected.
>
> Implement ExpandEntries() for FIT and add a copy of a simple FIT image
> test that checks subentry expansion in FIT entries.
>
> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> ---
>
> Changes in v2:
> - Split reused testSimpleFit code into a helper function
>
>  tools/binman/etype/fit.py |  5 +++++
>  tools/binman/ftest.py     | 33 ++++++++++++++++++++++++---------
>  2 files changed, 29 insertions(+), 9 deletions(-)

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

I missed the duplicate number on the dts, but will fix that when applying.

>
> diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
> index 6ad4a686df55..38bc2a2d784e 100644
> --- a/tools/binman/etype/fit.py
> +++ b/tools/binman/etype/fit.py
> @@ -237,6 +237,11 @@ def _AddNode(base_node, depth, node):
>          self._fdt = Fdt.FromData(fdt.as_bytearray())
>          self._fdt.Scan()
>
> +    def ExpandEntries(self):
> +        super().ExpandEntries()
> +        for section in self._fit_sections.values():
> +            section.ExpandEntries()
> +
>      def ObtainContents(self):
>          """Obtain the contents of the FIT
>
> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> index 5400f76c676a..626df786c8c9 100644
> --- a/tools/binman/ftest.py
> +++ b/tools/binman/ftest.py
> @@ -61,6 +61,9 @@
>  U_BOOT_NODTB_DATA     = b'nodtb with microcode pointer somewhere in here'
>  U_BOOT_SPL_NODTB_DATA = b'splnodtb with microcode pointer somewhere in here'
>  U_BOOT_TPL_NODTB_DATA = b'tplnodtb with microcode pointer somewhere in here'
> +U_BOOT_EXP_DATA       = U_BOOT_NODTB_DATA + U_BOOT_DTB_DATA
> +U_BOOT_SPL_EXP_DATA   = U_BOOT_SPL_NODTB_DATA + U_BOOT_SPL_DTB_DATA
> +U_BOOT_TPL_EXP_DATA   = U_BOOT_TPL_NODTB_DATA + U_BOOT_TPL_DTB_DATA
>  FSP_DATA              = b'fsp'
>  CMC_DATA              = b'cmc'
>  VBT_DATA              = b'vbt'
> @@ -3713,13 +3716,7 @@ def testPackOverlap(self):
>          """Test that zero-size overlapping regions are ignored"""
>          self._DoTestFile('160_pack_overlap_zero.dts')
>
> -    def testSimpleFit(self):
> -        """Test an image with a FIT inside"""
> -        data = self._DoReadFile('161_fit.dts')
> -        self.assertEqual(U_BOOT_DATA, data[:len(U_BOOT_DATA)])
> -        self.assertEqual(U_BOOT_NODTB_DATA, data[-len(U_BOOT_NODTB_DATA):])
> -        fit_data = data[len(U_BOOT_DATA):-len(U_BOOT_NODTB_DATA)]
> -
> +    def _CheckSimpleFitData(self, fit_data, kernel_data, fdt1_data):
>          # The data should be inside the FIT
>          dtb = fdt.Fdt.FromData(fit_data)
>          dtb.Scan()
> @@ -3752,8 +3749,26 @@ def testSimpleFit(self):
>          self.assertIsNotNone(data_sizes)
>          self.assertEqual(2, len(data_sizes))
>          # Format is "4 Bytes = 0.00 KiB = 0.00 MiB" so take the first word
> -        self.assertEqual(len(U_BOOT_DATA), int(data_sizes[0].split()[0]))
> -        self.assertEqual(len(U_BOOT_SPL_DTB_DATA), int(data_sizes[1].split()[0]))
> +        self.assertEqual(len(kernel_data), int(data_sizes[0].split()[0]))
> +        self.assertEqual(len(fdt1_data), int(data_sizes[1].split()[0]))
> +
> +    def testSimpleFit(self):
> +        """Test an image with a FIT inside"""
> +        data = self._DoReadFile('161_fit.dts')
> +        self.assertEqual(U_BOOT_DATA, data[:len(U_BOOT_DATA)])
> +        self.assertEqual(U_BOOT_NODTB_DATA, data[-len(U_BOOT_NODTB_DATA):])
> +        fit_data = data[len(U_BOOT_DATA):-len(U_BOOT_NODTB_DATA)]
> +
> +        self._CheckSimpleFitData(fit_data, U_BOOT_DATA, U_BOOT_SPL_DTB_DATA)
> +
> +    def testSimpleFitExpandsSubentries(self):
> +        """Test that FIT images expand their subentries"""
> +        data = self._DoReadFileDtb('161_fit.dts', use_expanded=True)[0]
> +        self.assertEqual(U_BOOT_EXP_DATA, data[:len(U_BOOT_EXP_DATA)])
> +        self.assertEqual(U_BOOT_NODTB_DATA, data[-len(U_BOOT_NODTB_DATA):])
> +        fit_data = data[len(U_BOOT_EXP_DATA):-len(U_BOOT_NODTB_DATA)]
> +
> +        self._CheckSimpleFitData(fit_data, U_BOOT_EXP_DATA, U_BOOT_SPL_DTB_DATA)
>
>      def testFitExternal(self):
>          """Test an image with an FIT with external images"""
> --
> 2.34.1
>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type
  2022-02-07 22:08 [PATCH v2 0/5] binman: Improvements to FIT entry type Alper Nebi Yasak
                   ` (4 preceding siblings ...)
  2022-02-07 22:08 ` [PATCH v2 5/5] binman: Update image positions of FIT subentries Alper Nebi Yasak
@ 2022-02-08 20:39 ` Simon Glass
  2022-02-08 20:39 ` [PATCH v2 3/5] binman: Check missing bintools of Section subclasses Simon Glass
  2022-02-08 20:39 ` [PATCH v2 2/5] binman: Register and check bintools from FIT subentries Simon Glass
  7 siblings, 0 replies; 27+ messages in thread
From: Simon Glass @ 2022-02-08 20:39 UTC (permalink / raw)
  To: Alper Nebi Yasak; +Cc: Heiko Thiery, Jan Kiszka, Simon Glass, u-boot

The binman FIT entry type shares some code with the Section entry type.
This shared code is bound to grow, since FIT entries are conceptually a
variation of Section entries.

Make FIT entry type a subclass of Section entry type, simplifying it a
bit and providing us the features that Section implements. Also fix the
subentry alignment test which now attempts to write symbols to a
nonexistent SPL ELF test file by creating it first.

Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---

Changes in v2:
- Add tag: "Reviewed-by: Simon Glass <sjg@chromium.org>"

 tools/binman/etype/fit.py | 70 +++++++++++----------------------------
 tools/binman/ftest.py     |  1 +
 2 files changed, 20 insertions(+), 51 deletions(-)

Applied to u-boot-dm, thanks!

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 3/5] binman: Check missing bintools of Section subclasses
  2022-02-07 22:08 [PATCH v2 0/5] binman: Improvements to FIT entry type Alper Nebi Yasak
                   ` (5 preceding siblings ...)
  2022-02-08 20:39 ` [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type Simon Glass
@ 2022-02-08 20:39 ` Simon Glass
  2022-02-08 20:39 ` [PATCH v2 2/5] binman: Register and check bintools from FIT subentries Simon Glass
  7 siblings, 0 replies; 27+ messages in thread
From: Simon Glass @ 2022-02-08 20:39 UTC (permalink / raw)
  To: Alper Nebi Yasak; +Cc: Heiko Thiery, Jan Kiszka, Simon Glass, u-boot

Binman can check for missing binary tools and prints warnings if
anything required for an image is missing. The implementation of this
for the Section entry only checks the subentries, presumably because
Section does not use any binary tools itself. However, this means the
check is also skipped for subclasses of Section which might need binary
tools.

Make sure missing binary tools are checked for subclasses of the Section
entry type as well, by calling the parent class' implementation in
the relevant Section method.

Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
This starts being tested by testFitMissing in the next patch when FIT
becomes a Section subclass.

Changes in v2:
- Add tag: "Reviewed-by: Simon Glass <sjg@chromium.org>"

 tools/binman/etype/section.py | 1 +
 1 file changed, 1 insertion(+)

Applied to u-boot-dm, thanks!

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 2/5] binman: Register and check bintools from FIT subentries
  2022-02-07 22:08 [PATCH v2 0/5] binman: Improvements to FIT entry type Alper Nebi Yasak
                   ` (6 preceding siblings ...)
  2022-02-08 20:39 ` [PATCH v2 3/5] binman: Check missing bintools of Section subclasses Simon Glass
@ 2022-02-08 20:39 ` Simon Glass
  7 siblings, 0 replies; 27+ messages in thread
From: Simon Glass @ 2022-02-08 20:39 UTC (permalink / raw)
  To: Alper Nebi Yasak; +Cc: Heiko Thiery, Jan Kiszka, Simon Glass, u-boot

Binman keeps track of binary tools each entry wants to use. The
implementation of this for the FIT entry only adds "mkimage", but not
the tools that would be used by its subentries.

Register the binary tools that FIT subentries will use in addition to
the one FIT itself uses, and check their existence by copying the
appropriate method from Section entry type. Also add tests that check if
these subentries can use and warn about binary tools.

Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---

Changes in v2:
- Add tag: "Reviewed-by: Simon Glass <sjg@chromium.org>"

 tools/binman/etype/fit.py                     | 14 +++++++
 tools/binman/ftest.py                         | 25 ++++++++++++
 .../binman/test/220_fit_subentry_bintool.dts  | 39 +++++++++++++++++++
 3 files changed, 78 insertions(+)
 create mode 100644 tools/binman/test/220_fit_subentry_bintool.dts

Applied to u-boot-dm, thanks!

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 1/5] binman: Fix subentry expansion for FIT entry type
  2022-02-07 22:08 ` [PATCH v2 1/5] binman: Fix subentry expansion for " Alper Nebi Yasak
  2022-02-08 15:05   ` Simon Glass
@ 2022-02-08 20:39   ` Simon Glass
  1 sibling, 0 replies; 27+ messages in thread
From: Simon Glass @ 2022-02-08 20:39 UTC (permalink / raw)
  To: Simon Glass
  Cc: U-Boot Mailing List, Heiko Thiery, Jan Kiszka, Alper Nebi Yasak

On Mon, 7 Feb 2022 at 15:08, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> Binman tries to expand some entries into parts that make it up, e.g.
> 'u-boot' into a 'u-boot-expanded' section that contains 'u-boot-nodtb'
> and 'u-boot-dtb'. Entries with child entries must call ExpandEntries()
> on them to build a correct image, as it's possible that unexpanded child
> entries have no data of their own. The FIT entry type doesn't currently
> do this, which means putting a "u-boot" entry inside it doesn't work as
> expected.
>
> Implement ExpandEntries() for FIT and add a copy of a simple FIT image
> test that checks subentry expansion in FIT entries.
>
> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> ---
>
> Changes in v2:
> - Split reused testSimpleFit code into a helper function
>
>  tools/binman/etype/fit.py |  5 +++++
>  tools/binman/ftest.py     | 33 ++++++++++++++++++++++++---------
>  2 files changed, 29 insertions(+), 9 deletions(-)

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

I missed the duplicate number on the dts, but will fix that when applying.

>
Applied to u-boot-dm, thanks!

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 5/5] binman: Update image positions of FIT subentries
  2022-02-07 22:08 ` [PATCH v2 5/5] binman: Update image positions of FIT subentries Alper Nebi Yasak
@ 2022-02-08 20:43   ` Simon Glass
  2022-02-23  2:35   ` Simon Glass
  1 sibling, 0 replies; 27+ messages in thread
From: Simon Glass @ 2022-02-08 20:43 UTC (permalink / raw)
  To: Alper Nebi Yasak; +Cc: U-Boot Mailing List, Heiko Thiery, Jan Kiszka

Hi Alper,

On Mon, 7 Feb 2022 at 15:08, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> Binman keeps track of positions of each entry in the final image, but
> currently this data is wrong for things included in FIT entries,
> especially since a previous patch makes FIT a subclass of Section and
> inherit its implementation.
>
> There are three ways to put data into a FIT image. It can be directly
> included as a "data" property, or it can be external to the FIT image
> represented by an offset-size pair of properties. This external offset
> is either "data-position" from the start of the FIT or "data-offset"
> from the end of the FIT, and the size is "data-size" for both. However,
> binman doesn't use the "data-offset" method while building FIT entries.
>
> According to the Section docstring, its subclasses should calculate and
> set the correct offsets and sizes in SetImagePos() method. Do this for
> FIT subentries for the three ways mentioned above, and add tests for the
> two ways binman can pack them in.
>
> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes in v2:
> - Check missing_bintools list instead of catching Fdt exceptions
> - Add tag: "Reviewed-by: Simon Glass <sjg@chromium.org>"
>
>  tools/binman/etype/fit.py |  51 +++++++++++++++++
>  tools/binman/ftest.py     | 112 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 163 insertions(+)

As mentioned I had to change the previous patch in a minor way to get
it to apply.

I'd really like to get this in if possible, too. The issue is the
handling of hash nodes in a FIT, as I mentioned.

If you are able to rework this, please let me know.

I've gone ahead sent my fit series but will rebase it onto this patch
if you are able to fix it up.

Regards,
Simon

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type
  2022-02-07 22:08 ` [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type Alper Nebi Yasak
@ 2022-02-14  9:09   ` Jan Kiszka
  2022-02-15 12:27     ` Alper Nebi Yasak
  0 siblings, 1 reply; 27+ messages in thread
From: Jan Kiszka @ 2022-02-14  9:09 UTC (permalink / raw)
  To: Alper Nebi Yasak, u-boot; +Cc: Heiko Thiery, Simon Glass

On 07.02.22 23:08, Alper Nebi Yasak wrote:
> The binman FIT entry type shares some code with the Section entry type.
> This shared code is bound to grow, since FIT entries are conceptually a
> variation of Section entries.
> 
> Make FIT entry type a subclass of Section entry type, simplifying it a
> bit and providing us the features that Section implements. Also fix the
> subentry alignment test which now attempts to write symbols to a
> nonexistent SPL ELF test file by creating it first.
> 
> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v2:
> - Add tag: "Reviewed-by: Simon Glass <sjg@chromium.org>"
> 
>  tools/binman/etype/fit.py | 70 +++++++++++----------------------------
>  tools/binman/ftest.py     |  1 +
>  2 files changed, 20 insertions(+), 51 deletions(-)
> 
> diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
> index 9dffcd5adbd6..0f8c88a9720e 100644
> --- a/tools/binman/etype/fit.py
> +++ b/tools/binman/etype/fit.py
> @@ -9,11 +9,12 @@
>  import libfdt
>  
>  from binman.entry import Entry, EntryArg
> +from binman.etype.section import Entry_section
>  from dtoc import fdt_util
>  from dtoc.fdt import Fdt
>  from patman import tools
>  
> -class Entry_fit(Entry):
> +class Entry_fit(Entry_section):
>      """Flat Image Tree (FIT)
>  
>      This calls mkimage to create a FIT (U-Boot Flat Image Tree) based on the
> @@ -112,15 +113,15 @@ def __init__(self, section, etype, node):
>          """
>          Members:
>              _fit: FIT file being built
> -            _fit_sections: dict:
> +            _entries: dict from Entry_section:
>                  key: relative path to entry Node (from the base of the FIT)
>                  value: Entry_section object comprising the contents of this
>                      node
>          """
>          super().__init__(section, etype, node)
>          self._fit = None
> -        self._fit_sections = {}
>          self._fit_props = {}
> +
>          for pname, prop in self._node.props.items():
>              if pname.startswith('fit,'):
>                  self._fit_props[pname] = prop
> @@ -185,7 +186,7 @@ def _AddNode(base_node, depth, node):
>                  # 'data' property later.
>                  entry = Entry.Create(self.section, node, etype='section')
>                  entry.ReadNode()
> -                self._fit_sections[rel_path] = entry
> +                self._entries[rel_path] = entry
>  
>              for subnode in node.subnodes:
>                  if has_images and not (subnode.name.startswith('hash') or
> @@ -237,18 +238,19 @@ def _AddNode(base_node, depth, node):
>          self._fdt = Fdt.FromData(fdt.as_bytearray())
>          self._fdt.Scan()
>  
> -    def ExpandEntries(self):
> -        super().ExpandEntries()
> -        for section in self._fit_sections.values():
> -            section.ExpandEntries()
> -
> -    def ObtainContents(self):
> -        """Obtain the contents of the FIT
> +    def BuildSectionData(self, required):
> +        """Build FIT entry contents
>  
>          This adds the 'data' properties to the input ITB (Image-tree Binary)
>          then runs mkimage to process it.
> +
> +        Args:
> +            required: True if the data must be present, False if it is OK to
> +                return None
> +
> +        Returns:
> +            Contents of the section (bytes)
>          """
> -        # self._BuildInput() either returns bytes or raises an exception.
>          data = self._BuildInput(self._fdt)
>          uniq = self.GetUniqueName()
>          input_fname = tools.GetOutputFilename('%s.itb' % uniq)
> @@ -264,14 +266,12 @@ def ObtainContents(self):
>                  'pad': fdt_util.fdt32_to_cpu(ext_offset.value)
>                  }
>          if self.mkimage.run(reset_timestamp=True, output_fname=output_fname,
> -                            **args) is not None:
> -            self.SetContents(tools.ReadFile(output_fname))
> -        else:
> +                            **args) is None:
>              # Bintool is missing; just use empty data as the output
>              self.record_missing_bintool(self.mkimage)
> -            self.SetContents(tools.GetBytes(0, 1024))
> +            return tools.GetBytes(0, 1024)
>  
> -        return True
> +        return tools.ReadFile(output_fname)
>  
>      def _BuildInput(self, fdt):
>          """Finish the FIT by adding the 'data' properties to it
> @@ -282,12 +282,8 @@ def _BuildInput(self, fdt):
>          Returns:
>              New fdt contents (bytes)
>          """
> -        for path, section in self._fit_sections.items():
> +        for path, section in self._entries.items():
>              node = fdt.GetNode(path)
> -            # Entry_section.ObtainContents() either returns True or
> -            # raises an exception.
> -            section.ObtainContents()
> -            section.Pack(0)
>              data = section.GetData()
>              node.AddData('data', data)
>  
> @@ -295,34 +291,6 @@ def _BuildInput(self, fdt):
>          data = fdt.GetContents()
>          return data
>  
> -    def CheckMissing(self, missing_list):
> -        """Check if any entries in this FIT have missing external blobs
> -
> -        If there are missing blobs, the entries are added to the list
> -
> -        Args:
> -            missing_list: List of Entry objects to be added to
> -        """
> -        for path, section in self._fit_sections.items():
> -            section.CheckMissing(missing_list)
> -
> -    def SetAllowMissing(self, allow_missing):
> -        for section in self._fit_sections.values():
> -            section.SetAllowMissing(allow_missing)
> -
>      def AddBintools(self, tools):
> -        for section in self._fit_sections.values():
> -            section.AddBintools(tools)
> +        super().AddBintools(tools)
>          self.mkimage = self.AddBintool(tools, 'mkimage')
> -
> -    def check_missing_bintools(self, missing_list):
> -        """Check if any entries in this section have missing bintools
> -
> -        If there are missing bintools, these are added to the list
> -
> -        Args:
> -            missing_list: List of Bintool objects to be added to
> -        """
> -        super().check_missing_bintools(missing_list)
> -        for entry in self._fit_sections.values():
> -            entry.check_missing_bintools(missing_list)
> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> index bc073587cc07..f16798960b84 100644
> --- a/tools/binman/ftest.py
> +++ b/tools/binman/ftest.py
> @@ -3850,6 +3850,7 @@ def testPadInSections(self):
>  
>      def testFitImageSubentryAlignment(self):
>          """Test relative alignability of FIT image subentries"""
> +        self._SetupSplElf()
>          entry_args = {
>              'test-id': TEXT_DATA,
>          }

Only came to testing this now, and it causes a regression.

Before this commit (I've added fdtmap to
arch/arm/dts/k3-am65-iot2050-boot-image.dtsi for this):

$ source/tools/binman/binman -D ls -i flash.bin
Name                 Image-pos  Size     Entry-type         Offset   Uncomp-size
----------------------------------------------------------------------------------
main-section                 0   8c0000  section                  0
  blob-ext@0x000000          0    37373  blob-ext@0x000000        0
  blob@0x080000          80000    84484  blob@0x080000        80000
  fit@0x280000          280000    f445f  fit@0x280000        280000
  fdtmap                37445f      cf9  fdtmap              37445f
  fill@0x680000         680000    20000  fill@0x680000       680000
  fill@0x6a0000         6a0000    20000  fill@0x6a0000       6a0000
  blob-ext@0x6c0000     6c0000    415de  blob-ext@0x6c0000   6c0000
  blob-ext@0x740000     740000    43952  blob-ext@0x740000   740000
  blob-ext@0x7c0000     7c0000    415e2  blob-ext@0x7c0000   7c0000
  blob-ext@0x840000     840000    4395a  blob-ext@0x840000   840000

With this commit:

$ source/tools/binman/binman -D ls -i flash.bin
Name                          Image-pos  Size     Entry-type         Offset   Uncomp-size
-------------------------------------------------------------------------------------------
main-section                          0   8c0000  section                  0
  blob-ext@0x000000                   0    37373  blob-ext@0x000000        0
  blob@0x080000                   80000    84484  blob@0x080000        80000
  fit@0x280000                                    fit@0x280000        280000
    u-boot                                        section
      u-boot-nodtb                                u-boot-nodtb
    fdt-iot2050-basic                             section
      blob                                        blob
    fdt-iot2050-basic-pg2                         section
      blob                                        blob
    fdt-iot2050-advanced                          section
      blob                                        blob
    fdt-iot2050-advanced-pg2                      section
      blob                                        blob
    k3-rti-wdt-firmware                           section
      blob-ext                                    blob-ext
  fdtmap                         37445f      cd9  fdtmap              37445f
  fill@0x680000                  680000    20000  fill@0x680000       680000
  fill@0x6a0000                  6a0000    20000  fill@0x6a0000       6a0000
  blob-ext@0x6c0000              6c0000    415de  blob-ext@0x6c0000   6c0000
  blob-ext@0x740000              740000    43952  blob-ext@0x740000   740000
  blob-ext@0x7c0000              7c0000    415e2  blob-ext@0x7c0000   7c0000
  blob-ext@0x840000              840000    4395a  blob-ext@0x840000   840000

The fit is nicely shown but its Image-pos column is now empty. And that 
leads to:

$ source/tools/binman/binman -D extract -i flash.bin -f extr.bin fit@0x280000
binman: unsupported operand type(s) for +: 'int' and 'NoneType'

Traceback (most recent call last):
  File "source/tools/binman/binman", line 141, in RunBinman
    ret_code = control.Binman(args)
  File "/data/u-boot/tools/binman/control.py", line 639, in Binman
    not args.uncompressed, args.format)
  File "/data/u-boot/tools/binman/control.py", line 260, in ExtractEntries
    data = entry.ReadData(decomp, alt_format)
  File "/data/u-boot/tools/binman/etype/section.py", line 763, in ReadData
    data = parent_data[offset:offset + self.size]
TypeError: unsupported operand type(s) for +: 'int' and 'NoneType'

Similar breakages for "replace" and likely more sub-commands.

I have to revert this locally for now - no time to debug.

Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type
  2022-02-14  9:09   ` Jan Kiszka
@ 2022-02-15 12:27     ` Alper Nebi Yasak
  2022-02-15 16:50       ` Jan Kiszka
  0 siblings, 1 reply; 27+ messages in thread
From: Alper Nebi Yasak @ 2022-02-15 12:27 UTC (permalink / raw)
  To: Jan Kiszka, u-boot; +Cc: Heiko Thiery, Simon Glass

On 14/02/2022 12:09, Jan Kiszka wrote:
> On 07.02.22 23:08, Alper Nebi Yasak wrote:
>> The binman FIT entry type shares some code with the Section entry type.
>> This shared code is bound to grow, since FIT entries are conceptually a
>> variation of Section entries.
>>
>> Make FIT entry type a subclass of Section entry type, simplifying it a
>> bit and providing us the features that Section implements. Also fix the
>> subentry alignment test which now attempts to write symbols to a
>> nonexistent SPL ELF test file by creating it first.
>>
>> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>> ---
> 
> Only came to testing this now, and it causes a regression.
> 
> Before this commit (I've added fdtmap to
> arch/arm/dts/k3-am65-iot2050-boot-image.dtsi for this):
> 
> $ source/tools/binman/binman -D ls -i flash.bin
> Name                 Image-pos  Size     Entry-type         Offset   Uncomp-size
> ----------------------------------------------------------------------------------
> main-section                 0   8c0000  section                  0
>   blob-ext@0x000000          0    37373  blob-ext@0x000000        0
>   blob@0x080000          80000    84484  blob@0x080000        80000
>   fit@0x280000          280000    f445f  fit@0x280000        280000
>   fdtmap                37445f      cf9  fdtmap              37445f
>   fill@0x680000         680000    20000  fill@0x680000       680000
>   fill@0x6a0000         6a0000    20000  fill@0x6a0000       6a0000
>   blob-ext@0x6c0000     6c0000    415de  blob-ext@0x6c0000   6c0000
>   blob-ext@0x740000     740000    43952  blob-ext@0x740000   740000
>   blob-ext@0x7c0000     7c0000    415e2  blob-ext@0x7c0000   7c0000
>   blob-ext@0x840000     840000    4395a  blob-ext@0x840000   840000
> 
> With this commit:
> 
> $ source/tools/binman/binman -D ls -i flash.bin
> Name                          Image-pos  Size     Entry-type         Offset   Uncomp-size
> -------------------------------------------------------------------------------------------
> main-section                          0   8c0000  section                  0
>   blob-ext@0x000000                   0    37373  blob-ext@0x000000        0
>   blob@0x080000                   80000    84484  blob@0x080000        80000
>   fit@0x280000                                    fit@0x280000        280000
>     u-boot                                        section
>       u-boot-nodtb                                u-boot-nodtb
>     fdt-iot2050-basic                             section
>       blob                                        blob
>     fdt-iot2050-basic-pg2                         section
>       blob                                        blob
>     fdt-iot2050-advanced                          section
>       blob                                        blob
>     fdt-iot2050-advanced-pg2                      section
>       blob                                        blob
>     k3-rti-wdt-firmware                           section
>       blob-ext                                    blob-ext
>   fdtmap                         37445f      cd9  fdtmap              37445f
>   fill@0x680000                  680000    20000  fill@0x680000       680000
>   fill@0x6a0000                  6a0000    20000  fill@0x6a0000       6a0000
>   blob-ext@0x6c0000              6c0000    415de  blob-ext@0x6c0000   6c0000
>   blob-ext@0x740000              740000    43952  blob-ext@0x740000   740000
>   blob-ext@0x7c0000              7c0000    415e2  blob-ext@0x7c0000   7c0000
>   blob-ext@0x840000              840000    4395a  blob-ext@0x840000   840000
> 

The AddMissingProperties() and SetCalculatedProperties() methods were
disabled for FIT as a fixup to this patch, that's why image-pos etc.
aren't available. See comments at [1] for some context.

Hopefully my two patches [2][3] fix things, can you test with them?

[1] "binman: Correct the error message for a bad hash algorithm"
https://patchwork.ozlabs.org/project/uboot/patch/20220208105941.1.I8f212a1150defebaf8b7b15a79f7a2fc62c276b2@changeid/

[2] "binman: Skip processing "hash" subnodes of FIT subsections"
https://patchwork.ozlabs.org/project/uboot/patch/20220209190236.26479-1-alpernebiyasak@gmail.com/

[3] "binman: Update image positions of FIT subentries"
https://patchwork.ozlabs.org/project/uboot/patch/20220207220809.4497-6-alpernebiyasak@gmail.com/

> The fit is nicely shown but its Image-pos column is now empty. And that 
> leads to:
> 
> $ source/tools/binman/binman -D extract -i flash.bin -f extr.bin fit@0x280000
> binman: unsupported operand type(s) for +: 'int' and 'NoneType'
> 
> Traceback (most recent call last):
>   File "source/tools/binman/binman", line 141, in RunBinman
>     ret_code = control.Binman(args)
>   File "/data/u-boot/tools/binman/control.py", line 639, in Binman
>     not args.uncompressed, args.format)
>   File "/data/u-boot/tools/binman/control.py", line 260, in ExtractEntries
>     data = entry.ReadData(decomp, alt_format)
>   File "/data/u-boot/tools/binman/etype/section.py", line 763, in ReadData
>     data = parent_data[offset:offset + self.size]
> TypeError: unsupported operand type(s) for +: 'int' and 'NoneType'
> 
> Similar breakages for "replace" and likely more sub-commands.

OTOH, Entry_section docstring does say subentry offsets can be None, so
it might be a good idea to fix these TypeErrors regardless.

> 
> I have to revert this locally for now - no time to debug.
> 
> Jan
> 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type
  2022-02-15 12:27     ` Alper Nebi Yasak
@ 2022-02-15 16:50       ` Jan Kiszka
  2022-02-15 17:06         ` Jan Kiszka
  0 siblings, 1 reply; 27+ messages in thread
From: Jan Kiszka @ 2022-02-15 16:50 UTC (permalink / raw)
  To: Alper Nebi Yasak, u-boot; +Cc: Heiko Thiery, Simon Glass

On 15.02.22 13:27, Alper Nebi Yasak wrote:
> On 14/02/2022 12:09, Jan Kiszka wrote:
>> On 07.02.22 23:08, Alper Nebi Yasak wrote:
>>> The binman FIT entry type shares some code with the Section entry type.
>>> This shared code is bound to grow, since FIT entries are conceptually a
>>> variation of Section entries.
>>>
>>> Make FIT entry type a subclass of Section entry type, simplifying it a
>>> bit and providing us the features that Section implements. Also fix the
>>> subentry alignment test which now attempts to write symbols to a
>>> nonexistent SPL ELF test file by creating it first.
>>>
>>> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>> ---
>>
>> Only came to testing this now, and it causes a regression.
>>
>> Before this commit (I've added fdtmap to
>> arch/arm/dts/k3-am65-iot2050-boot-image.dtsi for this):
>>
>> $ source/tools/binman/binman -D ls -i flash.bin
>> Name                 Image-pos  Size     Entry-type         Offset   Uncomp-size
>> ----------------------------------------------------------------------------------
>> main-section                 0   8c0000  section                  0
>>   blob-ext@0x000000          0    37373  blob-ext@0x000000        0
>>   blob@0x080000          80000    84484  blob@0x080000        80000
>>   fit@0x280000          280000    f445f  fit@0x280000        280000
>>   fdtmap                37445f      cf9  fdtmap              37445f
>>   fill@0x680000         680000    20000  fill@0x680000       680000
>>   fill@0x6a0000         6a0000    20000  fill@0x6a0000       6a0000
>>   blob-ext@0x6c0000     6c0000    415de  blob-ext@0x6c0000   6c0000
>>   blob-ext@0x740000     740000    43952  blob-ext@0x740000   740000
>>   blob-ext@0x7c0000     7c0000    415e2  blob-ext@0x7c0000   7c0000
>>   blob-ext@0x840000     840000    4395a  blob-ext@0x840000   840000
>>
>> With this commit:
>>
>> $ source/tools/binman/binman -D ls -i flash.bin
>> Name                          Image-pos  Size     Entry-type         Offset   Uncomp-size
>> -------------------------------------------------------------------------------------------
>> main-section                          0   8c0000  section                  0
>>   blob-ext@0x000000                   0    37373  blob-ext@0x000000        0
>>   blob@0x080000                   80000    84484  blob@0x080000        80000
>>   fit@0x280000                                    fit@0x280000        280000
>>     u-boot                                        section
>>       u-boot-nodtb                                u-boot-nodtb
>>     fdt-iot2050-basic                             section
>>       blob                                        blob
>>     fdt-iot2050-basic-pg2                         section
>>       blob                                        blob
>>     fdt-iot2050-advanced                          section
>>       blob                                        blob
>>     fdt-iot2050-advanced-pg2                      section
>>       blob                                        blob
>>     k3-rti-wdt-firmware                           section
>>       blob-ext                                    blob-ext
>>   fdtmap                         37445f      cd9  fdtmap              37445f
>>   fill@0x680000                  680000    20000  fill@0x680000       680000
>>   fill@0x6a0000                  6a0000    20000  fill@0x6a0000       6a0000
>>   blob-ext@0x6c0000              6c0000    415de  blob-ext@0x6c0000   6c0000
>>   blob-ext@0x740000              740000    43952  blob-ext@0x740000   740000
>>   blob-ext@0x7c0000              7c0000    415e2  blob-ext@0x7c0000   7c0000
>>   blob-ext@0x840000              840000    4395a  blob-ext@0x840000   840000
>>
> 
> The AddMissingProperties() and SetCalculatedProperties() methods were
> disabled for FIT as a fixup to this patch, that's why image-pos etc.
> aren't available. See comments at [1] for some context.
> 
> Hopefully my two patches [2][3] fix things, can you test with them?
> 
> [1] "binman: Correct the error message for a bad hash algorithm"
> https://patchwork.ozlabs.org/project/uboot/patch/20220208105941.1.I8f212a1150defebaf8b7b15a79f7a2fc62c276b2@changeid/
> 
> [2] "binman: Skip processing "hash" subnodes of FIT subsections"
> https://patchwork.ozlabs.org/project/uboot/patch/20220209190236.26479-1-alpernebiyasak@gmail.com/
> 

This one helped, indeed.

Thanks,
Jan

> [3] "binman: Update image positions of FIT subentries"
> https://patchwork.ozlabs.org/project/uboot/patch/20220207220809.4497-6-alpernebiyasak@gmail.com/
> 
>> The fit is nicely shown but its Image-pos column is now empty. And that 
>> leads to:
>>
>> $ source/tools/binman/binman -D extract -i flash.bin -f extr.bin fit@0x280000
>> binman: unsupported operand type(s) for +: 'int' and 'NoneType'
>>
>> Traceback (most recent call last):
>>   File "source/tools/binman/binman", line 141, in RunBinman
>>     ret_code = control.Binman(args)
>>   File "/data/u-boot/tools/binman/control.py", line 639, in Binman
>>     not args.uncompressed, args.format)
>>   File "/data/u-boot/tools/binman/control.py", line 260, in ExtractEntries
>>     data = entry.ReadData(decomp, alt_format)
>>   File "/data/u-boot/tools/binman/etype/section.py", line 763, in ReadData
>>     data = parent_data[offset:offset + self.size]
>> TypeError: unsupported operand type(s) for +: 'int' and 'NoneType'
>>
>> Similar breakages for "replace" and likely more sub-commands.
> 
> OTOH, Entry_section docstring does say subentry offsets can be None, so
> it might be a good idea to fix these TypeErrors regardless.
> 
>>
>> I have to revert this locally for now - no time to debug.
>>
>> Jan
>>

-- 
Siemens AG, Technology
Competence Center Embedded Linux

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type
  2022-02-15 16:50       ` Jan Kiszka
@ 2022-02-15 17:06         ` Jan Kiszka
  2022-02-18 16:50           ` Jan Kiszka
  0 siblings, 1 reply; 27+ messages in thread
From: Jan Kiszka @ 2022-02-15 17:06 UTC (permalink / raw)
  To: Alper Nebi Yasak, u-boot; +Cc: Heiko Thiery, Simon Glass

On 15.02.22 17:50, Jan Kiszka wrote:
> On 15.02.22 13:27, Alper Nebi Yasak wrote:
>> On 14/02/2022 12:09, Jan Kiszka wrote:
>>> On 07.02.22 23:08, Alper Nebi Yasak wrote:
>>>> The binman FIT entry type shares some code with the Section entry type.
>>>> This shared code is bound to grow, since FIT entries are conceptually a
>>>> variation of Section entries.
>>>>
>>>> Make FIT entry type a subclass of Section entry type, simplifying it a
>>>> bit and providing us the features that Section implements. Also fix the
>>>> subentry alignment test which now attempts to write symbols to a
>>>> nonexistent SPL ELF test file by creating it first.
>>>>
>>>> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>> ---
>>>
>>> Only came to testing this now, and it causes a regression.
>>>
>>> Before this commit (I've added fdtmap to
>>> arch/arm/dts/k3-am65-iot2050-boot-image.dtsi for this):
>>>
>>> $ source/tools/binman/binman -D ls -i flash.bin
>>> Name                 Image-pos  Size     Entry-type         Offset   Uncomp-size
>>> ----------------------------------------------------------------------------------
>>> main-section                 0   8c0000  section                  0
>>>   blob-ext@0x000000          0    37373  blob-ext@0x000000        0
>>>   blob@0x080000          80000    84484  blob@0x080000        80000
>>>   fit@0x280000          280000    f445f  fit@0x280000        280000
>>>   fdtmap                37445f      cf9  fdtmap              37445f
>>>   fill@0x680000         680000    20000  fill@0x680000       680000
>>>   fill@0x6a0000         6a0000    20000  fill@0x6a0000       6a0000
>>>   blob-ext@0x6c0000     6c0000    415de  blob-ext@0x6c0000   6c0000
>>>   blob-ext@0x740000     740000    43952  blob-ext@0x740000   740000
>>>   blob-ext@0x7c0000     7c0000    415e2  blob-ext@0x7c0000   7c0000
>>>   blob-ext@0x840000     840000    4395a  blob-ext@0x840000   840000
>>>
>>> With this commit:
>>>
>>> $ source/tools/binman/binman -D ls -i flash.bin
>>> Name                          Image-pos  Size     Entry-type         Offset   Uncomp-size
>>> -------------------------------------------------------------------------------------------
>>> main-section                          0   8c0000  section                  0
>>>   blob-ext@0x000000                   0    37373  blob-ext@0x000000        0
>>>   blob@0x080000                   80000    84484  blob@0x080000        80000
>>>   fit@0x280000                                    fit@0x280000        280000
>>>     u-boot                                        section
>>>       u-boot-nodtb                                u-boot-nodtb
>>>     fdt-iot2050-basic                             section
>>>       blob                                        blob
>>>     fdt-iot2050-basic-pg2                         section
>>>       blob                                        blob
>>>     fdt-iot2050-advanced                          section
>>>       blob                                        blob
>>>     fdt-iot2050-advanced-pg2                      section
>>>       blob                                        blob
>>>     k3-rti-wdt-firmware                           section
>>>       blob-ext                                    blob-ext
>>>   fdtmap                         37445f      cd9  fdtmap              37445f
>>>   fill@0x680000                  680000    20000  fill@0x680000       680000
>>>   fill@0x6a0000                  6a0000    20000  fill@0x6a0000       6a0000
>>>   blob-ext@0x6c0000              6c0000    415de  blob-ext@0x6c0000   6c0000
>>>   blob-ext@0x740000              740000    43952  blob-ext@0x740000   740000
>>>   blob-ext@0x7c0000              7c0000    415e2  blob-ext@0x7c0000   7c0000
>>>   blob-ext@0x840000              840000    4395a  blob-ext@0x840000   840000
>>>
>>
>> The AddMissingProperties() and SetCalculatedProperties() methods were
>> disabled for FIT as a fixup to this patch, that's why image-pos etc.
>> aren't available. See comments at [1] for some context.
>>
>> Hopefully my two patches [2][3] fix things, can you test with them?
>>
>> [1] "binman: Correct the error message for a bad hash algorithm"
>> https://patchwork.ozlabs.org/project/uboot/patch/20220208105941.1.I8f212a1150defebaf8b7b15a79f7a2fc62c276b2@changeid/
>>
>> [2] "binman: Skip processing "hash" subnodes of FIT subsections"
>> https://patchwork.ozlabs.org/project/uboot/patch/20220209190236.26479-1-alpernebiyasak@gmail.com/
>>
> 
> This one helped, indeed.
> 

...not completely:

$ source/tools/binman/binman replace -i flash.bin -f fit@0x380000.fit
fit@0x380000binman: [Errno 13] Permission denied: '/.fit@0x380000.itb'

Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type
  2022-02-15 17:06         ` Jan Kiszka
@ 2022-02-18 16:50           ` Jan Kiszka
  2022-02-18 17:34             ` Alper Nebi Yasak
  0 siblings, 1 reply; 27+ messages in thread
From: Jan Kiszka @ 2022-02-18 16:50 UTC (permalink / raw)
  To: Alper Nebi Yasak, u-boot; +Cc: Heiko Thiery, Simon Glass


On 15.02.22 18:06, Jan Kiszka wrote:
> On 15.02.22 17:50, Jan Kiszka wrote:
>> On 15.02.22 13:27, Alper Nebi Yasak wrote:
>>> On 14/02/2022 12:09, Jan Kiszka wrote:
>>>> On 07.02.22 23:08, Alper Nebi Yasak wrote:
>>>>> The binman FIT entry type shares some code with the Section entry type.
>>>>> This shared code is bound to grow, since FIT entries are conceptually a
>>>>> variation of Section entries.
>>>>>
>>>>> Make FIT entry type a subclass of Section entry type, simplifying it a
>>>>> bit and providing us the features that Section implements. Also fix the
>>>>> subentry alignment test which now attempts to write symbols to a
>>>>> nonexistent SPL ELF test file by creating it first.
>>>>>
>>>>> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>>> ---
>>>>
>>>> Only came to testing this now, and it causes a regression.
>>>>
>>>> Before this commit (I've added fdtmap to
>>>> arch/arm/dts/k3-am65-iot2050-boot-image.dtsi for this):
>>>>
>>>> $ source/tools/binman/binman -D ls -i flash.bin
>>>> Name                 Image-pos  Size     Entry-type         Offset   Uncomp-size
>>>> ----------------------------------------------------------------------------------
>>>> main-section                 0   8c0000  section                  0
>>>>   blob-ext@0x000000          0    37373  blob-ext@0x000000        0
>>>>   blob@0x080000          80000    84484  blob@0x080000        80000
>>>>   fit@0x280000          280000    f445f  fit@0x280000        280000
>>>>   fdtmap                37445f      cf9  fdtmap              37445f
>>>>   fill@0x680000         680000    20000  fill@0x680000       680000
>>>>   fill@0x6a0000         6a0000    20000  fill@0x6a0000       6a0000
>>>>   blob-ext@0x6c0000     6c0000    415de  blob-ext@0x6c0000   6c0000
>>>>   blob-ext@0x740000     740000    43952  blob-ext@0x740000   740000
>>>>   blob-ext@0x7c0000     7c0000    415e2  blob-ext@0x7c0000   7c0000
>>>>   blob-ext@0x840000     840000    4395a  blob-ext@0x840000   840000
>>>>
>>>> With this commit:
>>>>
>>>> $ source/tools/binman/binman -D ls -i flash.bin
>>>> Name                          Image-pos  Size     Entry-type         Offset   Uncomp-size
>>>> -------------------------------------------------------------------------------------------
>>>> main-section                          0   8c0000  section                  0
>>>>   blob-ext@0x000000                   0    37373  blob-ext@0x000000        0
>>>>   blob@0x080000                   80000    84484  blob@0x080000        80000
>>>>   fit@0x280000                                    fit@0x280000        280000
>>>>     u-boot                                        section
>>>>       u-boot-nodtb                                u-boot-nodtb
>>>>     fdt-iot2050-basic                             section
>>>>       blob                                        blob
>>>>     fdt-iot2050-basic-pg2                         section
>>>>       blob                                        blob
>>>>     fdt-iot2050-advanced                          section
>>>>       blob                                        blob
>>>>     fdt-iot2050-advanced-pg2                      section
>>>>       blob                                        blob
>>>>     k3-rti-wdt-firmware                           section
>>>>       blob-ext                                    blob-ext
>>>>   fdtmap                         37445f      cd9  fdtmap              37445f
>>>>   fill@0x680000                  680000    20000  fill@0x680000       680000
>>>>   fill@0x6a0000                  6a0000    20000  fill@0x6a0000       6a0000
>>>>   blob-ext@0x6c0000              6c0000    415de  blob-ext@0x6c0000   6c0000
>>>>   blob-ext@0x740000              740000    43952  blob-ext@0x740000   740000
>>>>   blob-ext@0x7c0000              7c0000    415e2  blob-ext@0x7c0000   7c0000
>>>>   blob-ext@0x840000              840000    4395a  blob-ext@0x840000   840000
>>>>
>>>
>>> The AddMissingProperties() and SetCalculatedProperties() methods were
>>> disabled for FIT as a fixup to this patch, that's why image-pos etc.
>>> aren't available. See comments at [1] for some context.
>>>
>>> Hopefully my two patches [2][3] fix things, can you test with them?
>>>
>>> [1] "binman: Correct the error message for a bad hash algorithm"
>>> https://patchwork.ozlabs.org/project/uboot/patch/20220208105941.1.I8f212a1150defebaf8b7b15a79f7a2fc62c276b2@changeid/
>>>
>>> [2] "binman: Skip processing "hash" subnodes of FIT subsections"
>>> https://patchwork.ozlabs.org/project/uboot/patch/20220209190236.26479-1-alpernebiyasak@gmail.com/
>>>
>>
>> This one helped, indeed.
>>
> 
> ...not completely:
> 
> $ source/tools/binman/binman replace -i flash.bin -f fit@0x380000.fit
> fit@0x380000binman: [Errno 13] Permission denied: '/.fit@0x380000.itb'
> 

Ping.

$ source/tools/binman/binman -D replace -i flash.bin -f fit@0x380000.fit fit@0x380000
binman: [Errno 13] Permission denied: '/.fit@0x380000.itb'

Traceback (most recent call last):
  File "source/tools/binman/binman", line 141, in RunBinman
    ret_code = control.Binman(args)
  File "u-boot/tools/binman/control.py", line 644, in Binman
    allow_resize=not args.fix_size, write_map=args.map)
  File "u-boot/tools/binman/control.py", line 404, in ReplaceEntries
    allow_resize=allow_resize, write_map=write_map)
  File "u-boot/tools/binman/control.py", line 341, in WriteEntryToImage
    AfterReplace(image, allow_resize=allow_resize, write_map=write_map)
  File "u-boot/tools/binman/control.py", line 333, in AfterReplace
    get_contents=False, allow_resize=allow_resize)
  File "u-boot/tools/binman/control.py", line 560, in ProcessImage
    image.PackEntries()
  File "u-boot/tools/binman/image.py", line 155, in PackEntries
    super().Pack(0)
  File "u-boot/tools/binman/etype/section.py", line 385, in Pack
    self._PackEntries()
  File "u-boot/tools/binman/etype/section.py", line 403, in _PackEntries
    offset = entry.Pack(offset)
  File "u-boot/tools/binman/etype/section.py", line 390, in Pack
    data = self.BuildSectionData(True)
  File "u-boot/tools/binman/etype/fit.py", line 265, in BuildSectionData
    tools.write_file(input_fname, data)
  File "u-boot/tools/patman/tools.py", line 482, in write_file
    with open(filename(fname), binary and 'wb' or 'w') as fd:
PermissionError: [Errno 13] Permission denied: '/.fit@0x380000.itb'

Something seems fairly broken here. That '/.' does not come from the 
output directory name, it's generated by Entry.GetUniqueName. Looks like 
this path should not been taken under these conditions.

Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type
  2022-02-18 16:50           ` Jan Kiszka
@ 2022-02-18 17:34             ` Alper Nebi Yasak
  2022-02-19 15:53               ` Simon Glass
  0 siblings, 1 reply; 27+ messages in thread
From: Alper Nebi Yasak @ 2022-02-18 17:34 UTC (permalink / raw)
  To: Jan Kiszka, u-boot; +Cc: Heiko Thiery, Simon Glass

On 18/02/2022 19:50, Jan Kiszka wrote:
> On 15.02.22 18:06, Jan Kiszka wrote:
>> On 15.02.22 17:50, Jan Kiszka wrote:
>>> On 15.02.22 13:27, Alper Nebi Yasak wrote:
>>>> The AddMissingProperties() and SetCalculatedProperties() methods were
>>>> disabled for FIT as a fixup to this patch, that's why image-pos etc.
>>>> aren't available. See comments at [1] for some context.
>>>>
>>>> Hopefully my two patches [2][3] fix things, can you test with them?
>>>>
>>>> [1] "binman: Correct the error message for a bad hash algorithm"
>>>> https://patchwork.ozlabs.org/project/uboot/patch/20220208105941.1.I8f212a1150defebaf8b7b15a79f7a2fc62c276b2@changeid/
>>>>
>>>> [2] "binman: Skip processing "hash" subnodes of FIT subsections"
>>>> https://patchwork.ozlabs.org/project/uboot/patch/20220209190236.26479-1-alpernebiyasak@gmail.com/
>>>>
>>>
>>> This one helped, indeed.
>>>
>>
>> ...not completely:
>>
>> $ source/tools/binman/binman replace -i flash.bin -f fit@0x380000.fit
>> fit@0x380000binman: [Errno 13] Permission denied: '/.fit@0x380000.itb'
>>
> 
> Ping.
> 
> $ source/tools/binman/binman -D replace -i flash.bin -f fit@0x380000.fit fit@0x380000
> binman: [Errno 13] Permission denied: '/.fit@0x380000.itb'
> 
> Traceback (most recent call last):
>   File "source/tools/binman/binman", line 141, in RunBinman
>     ret_code = control.Binman(args)
>   File "u-boot/tools/binman/control.py", line 644, in Binman
>     allow_resize=not args.fix_size, write_map=args.map)
>   File "u-boot/tools/binman/control.py", line 404, in ReplaceEntries
>     allow_resize=allow_resize, write_map=write_map)
>   File "u-boot/tools/binman/control.py", line 341, in WriteEntryToImage
>     AfterReplace(image, allow_resize=allow_resize, write_map=write_map)
>   File "u-boot/tools/binman/control.py", line 333, in AfterReplace
>     get_contents=False, allow_resize=allow_resize)
>   File "u-boot/tools/binman/control.py", line 560, in ProcessImage
>     image.PackEntries()
>   File "u-boot/tools/binman/image.py", line 155, in PackEntries
>     super().Pack(0)
>   File "u-boot/tools/binman/etype/section.py", line 385, in Pack
>     self._PackEntries()
>   File "u-boot/tools/binman/etype/section.py", line 403, in _PackEntries
>     offset = entry.Pack(offset)
>   File "u-boot/tools/binman/etype/section.py", line 390, in Pack
>     data = self.BuildSectionData(True)
>   File "u-boot/tools/binman/etype/fit.py", line 265, in BuildSectionData
>     tools.write_file(input_fname, data)
>   File "u-boot/tools/patman/tools.py", line 482, in write_file
>     with open(filename(fname), binary and 'wb' or 'w') as fd:
> PermissionError: [Errno 13] Permission denied: '/.fit@0x380000.itb'
> 
> Something seems fairly broken here. That '/.' does not come from the 
> output directory name, it's generated by Entry.GetUniqueName. Looks like 
> this path should not been taken under these conditions.

I can reproduce this and tried a few things, but more issues just kept
popping up (outside u-boot as well). I got it to a point where the
command re-packs the FIT and the image but quite wrongly. The offset and
image-pos properties get added in the FIT, and the image main-section
just concatenates all entries without regard to set offsets. I'll
need more time to work those out, then to add tests and send patches.

Here's the diff I got so far in case it helps:

diff --git a/tools/binman/control.py b/tools/binman/control.py
index a179f7812988..24ec89b26302 100644
--- a/tools/binman/control.py
+++ b/tools/binman/control.py
@@ -390,6 +390,7 @@ def ReplaceEntries(image_fname, input_fname, indir,
entry_paths,
     """
     image_fname = os.path.abspath(image_fname)
     image = Image.FromFile(image_fname)
+    image.CollectBintools()

     # Replace an entry from a single file, as a special case
     if input_fname:
diff --git a/tools/binman/entry.py b/tools/binman/entry.py
index 631215dfc88a..8173e91af96a 100644
--- a/tools/binman/entry.py
+++ b/tools/binman/entry.py
@@ -767,7 +767,7 @@ def GetUniqueName(self):
         node = self._node
         while node.parent:
             node = node.parent
-            if node.name == 'binman':
+            if node.name == 'binman' or node.name == '/':
                 break
             name = '%s.%s' % (node.name, name)
         return name
diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
index 1e957023f354..9b2c33bc2b34 100644
--- a/tools/binman/etype/fit.py
+++ b/tools/binman/etype/fit.py
@@ -137,10 +137,6 @@ def __init__(self, section, etype, node):
                                                                   str)])[0]
         self.mkimage = None

-    def ReadNode(self):
-        self.ReadEntries()
-        super().ReadNode()
-
     def ReadEntries(self):
         def _AddNode(base_node, depth, node):
             """Add a node to the FIT
@@ -184,11 +180,12 @@ def _AddNode(base_node, depth, node):
                 # section entries for them here to merge the content
subnodes
                 # together and put the merged contents in the subimage
node's
                 # 'data' property later.
-                entry = Entry.Create(self.section, node, etype='section')
+                entry = Entry.Create(self, node, etype='section')
                 entry.ReadNode()
                 # The hash subnodes here are for mkimage, not binman.
                 entry.SetUpdateHash(False)
-                self._entries[rel_path] = entry
+                image_name = rel_path[len('/images/'):]
+                self._entries[image_name] = entry

             for subnode in node.subnodes:
                 if has_images and not (subnode.name.startswith('hash') or
@@ -284,7 +281,8 @@ def _BuildInput(self, fdt):
         Returns:
             New fdt contents (bytes)
         """
-        for path, section in self._entries.items():
+        for image_name, section in self._entries.items():
+            path = f"/images/{image_name}"
             node = fdt.GetNode(path)
             data = section.GetData()
             node.AddData('data', data)
@@ -312,7 +310,8 @@ def SetImagePos(self, image_pos):
         fdt = Fdt.FromData(self.GetData())
         fdt.Scan()

-        for path, section in self._entries.items():
+        for image_name, section in self._entries.items():
+            path = f"/images/{image_name}"
             node = fdt.GetNode(path)

             data_prop = node.props.get("data")

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type
  2022-02-18 17:34             ` Alper Nebi Yasak
@ 2022-02-19 15:53               ` Simon Glass
  2022-02-21  4:40                 ` Simon Glass
  0 siblings, 1 reply; 27+ messages in thread
From: Simon Glass @ 2022-02-19 15:53 UTC (permalink / raw)
  To: Alper Nebi Yasak; +Cc: Jan Kiszka, U-Boot Mailing List, Heiko Thiery

Hi Alper,

On Fri, 18 Feb 2022 at 10:34, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> On 18/02/2022 19:50, Jan Kiszka wrote:
> > On 15.02.22 18:06, Jan Kiszka wrote:
> >> On 15.02.22 17:50, Jan Kiszka wrote:
> >>> On 15.02.22 13:27, Alper Nebi Yasak wrote:
> >>>> The AddMissingProperties() and SetCalculatedProperties() methods were
> >>>> disabled for FIT as a fixup to this patch, that's why image-pos etc.
> >>>> aren't available. See comments at [1] for some context.
> >>>>
> >>>> Hopefully my two patches [2][3] fix things, can you test with them?
> >>>>
> >>>> [1] "binman: Correct the error message for a bad hash algorithm"
> >>>> https://patchwork.ozlabs.org/project/uboot/patch/20220208105941.1.I8f212a1150defebaf8b7b15a79f7a2fc62c276b2@changeid/
> >>>>
> >>>> [2] "binman: Skip processing "hash" subnodes of FIT subsections"
> >>>> https://patchwork.ozlabs.org/project/uboot/patch/20220209190236.26479-1-alpernebiyasak@gmail.com/
> >>>>
> >>>
> >>> This one helped, indeed.
> >>>
> >>
> >> ...not completely:
> >>
> >> $ source/tools/binman/binman replace -i flash.bin -f fit@0x380000.fit
> >> fit@0x380000binman: [Errno 13] Permission denied: '/.fit@0x380000.itb'
> >>
> >
> > Ping.
> >
> > $ source/tools/binman/binman -D replace -i flash.bin -f fit@0x380000.fit fit@0x380000
> > binman: [Errno 13] Permission denied: '/.fit@0x380000.itb'
> >
> > Traceback (most recent call last):
> >   File "source/tools/binman/binman", line 141, in RunBinman
> >     ret_code = control.Binman(args)
> >   File "u-boot/tools/binman/control.py", line 644, in Binman
> >     allow_resize=not args.fix_size, write_map=args.map)
> >   File "u-boot/tools/binman/control.py", line 404, in ReplaceEntries
> >     allow_resize=allow_resize, write_map=write_map)
> >   File "u-boot/tools/binman/control.py", line 341, in WriteEntryToImage
> >     AfterReplace(image, allow_resize=allow_resize, write_map=write_map)
> >   File "u-boot/tools/binman/control.py", line 333, in AfterReplace
> >     get_contents=False, allow_resize=allow_resize)
> >   File "u-boot/tools/binman/control.py", line 560, in ProcessImage
> >     image.PackEntries()
> >   File "u-boot/tools/binman/image.py", line 155, in PackEntries
> >     super().Pack(0)
> >   File "u-boot/tools/binman/etype/section.py", line 385, in Pack
> >     self._PackEntries()
> >   File "u-boot/tools/binman/etype/section.py", line 403, in _PackEntries
> >     offset = entry.Pack(offset)
> >   File "u-boot/tools/binman/etype/section.py", line 390, in Pack
> >     data = self.BuildSectionData(True)
> >   File "u-boot/tools/binman/etype/fit.py", line 265, in BuildSectionData
> >     tools.write_file(input_fname, data)
> >   File "u-boot/tools/patman/tools.py", line 482, in write_file
> >     with open(filename(fname), binary and 'wb' or 'w') as fd:
> > PermissionError: [Errno 13] Permission denied: '/.fit@0x380000.itb'
> >
> > Something seems fairly broken here. That '/.' does not come from the
> > output directory name, it's generated by Entry.GetUniqueName. Looks like
> > this path should not been taken under these conditions.
>
> I can reproduce this and tried a few things, but more issues just kept
> popping up (outside u-boot as well). I got it to a point where the
> command re-packs the FIT and the image but quite wrongly. The offset and
> image-pos properties get added in the FIT, and the image main-section
> just concatenates all entries without regard to set offsets. I'll
> need more time to work those out, then to add tests and send patches.

I am going to try to merge my fit generator series today.

One issue I notice is that the conversion to use entry_Section changes
the contents of the self._fit_entries dict. Before it was keyed by
relative path, but entry_section keys self._entries by node name.

We may need to split it up. I will see if I can at least merge my
series, which should not make things any worse, then see if I can come
up with ideas.

Thanks for the diff.

Regards,
Simon

>
> Here's the diff I got so far in case it helps:
>
> diff --git a/tools/binman/control.py b/tools/binman/control.py
> index a179f7812988..24ec89b26302 100644
> --- a/tools/binman/control.py
> +++ b/tools/binman/control.py
> @@ -390,6 +390,7 @@ def ReplaceEntries(image_fname, input_fname, indir,
> entry_paths,
>      """
>      image_fname = os.path.abspath(image_fname)
>      image = Image.FromFile(image_fname)
> +    image.CollectBintools()
>
>      # Replace an entry from a single file, as a special case
>      if input_fname:
> diff --git a/tools/binman/entry.py b/tools/binman/entry.py
> index 631215dfc88a..8173e91af96a 100644
> --- a/tools/binman/entry.py
> +++ b/tools/binman/entry.py
> @@ -767,7 +767,7 @@ def GetUniqueName(self):
>          node = self._node
>          while node.parent:
>              node = node.parent
> -            if node.name == 'binman':
> +            if node.name == 'binman' or node.name == '/':
>                  break
>              name = '%s.%s' % (node.name, name)
>          return name
> diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
> index 1e957023f354..9b2c33bc2b34 100644
> --- a/tools/binman/etype/fit.py
> +++ b/tools/binman/etype/fit.py
> @@ -137,10 +137,6 @@ def __init__(self, section, etype, node):
>                                                                    str)])[0]
>          self.mkimage = None
>
> -    def ReadNode(self):
> -        self.ReadEntries()
> -        super().ReadNode()
> -
>      def ReadEntries(self):
>          def _AddNode(base_node, depth, node):
>              """Add a node to the FIT
> @@ -184,11 +180,12 @@ def _AddNode(base_node, depth, node):
>                  # section entries for them here to merge the content
> subnodes
>                  # together and put the merged contents in the subimage
> node's
>                  # 'data' property later.
> -                entry = Entry.Create(self.section, node, etype='section')
> +                entry = Entry.Create(self, node, etype='section')
>                  entry.ReadNode()
>                  # The hash subnodes here are for mkimage, not binman.
>                  entry.SetUpdateHash(False)
> -                self._entries[rel_path] = entry
> +                image_name = rel_path[len('/images/'):]
> +                self._entries[image_name] = entry
>
>              for subnode in node.subnodes:
>                  if has_images and not (subnode.name.startswith('hash') or
> @@ -284,7 +281,8 @@ def _BuildInput(self, fdt):
>          Returns:
>              New fdt contents (bytes)
>          """
> -        for path, section in self._entries.items():
> +        for image_name, section in self._entries.items():
> +            path = f"/images/{image_name}"
>              node = fdt.GetNode(path)
>              data = section.GetData()
>              node.AddData('data', data)
> @@ -312,7 +310,8 @@ def SetImagePos(self, image_pos):
>          fdt = Fdt.FromData(self.GetData())
>          fdt.Scan()
>
> -        for path, section in self._entries.items():
> +        for image_name, section in self._entries.items():
> +            path = f"/images/{image_name}"
>              node = fdt.GetNode(path)
>
>              data_prop = node.props.get("data")

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type
  2022-02-19 15:53               ` Simon Glass
@ 2022-02-21  4:40                 ` Simon Glass
  2022-02-22 18:58                   ` Alper Nebi Yasak
  0 siblings, 1 reply; 27+ messages in thread
From: Simon Glass @ 2022-02-21  4:40 UTC (permalink / raw)
  To: Alper Nebi Yasak; +Cc: Jan Kiszka, U-Boot Mailing List, Heiko Thiery

Hi again,

On Sat, 19 Feb 2022 at 08:53, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Alper,
>
> On Fri, 18 Feb 2022 at 10:34, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
> >
> > On 18/02/2022 19:50, Jan Kiszka wrote:
> > > On 15.02.22 18:06, Jan Kiszka wrote:
> > >> On 15.02.22 17:50, Jan Kiszka wrote:
> > >>> On 15.02.22 13:27, Alper Nebi Yasak wrote:
> > >>>> The AddMissingProperties() and SetCalculatedProperties() methods were
> > >>>> disabled for FIT as a fixup to this patch, that's why image-pos etc.
> > >>>> aren't available. See comments at [1] for some context.
> > >>>>
> > >>>> Hopefully my two patches [2][3] fix things, can you test with them?
> > >>>>
> > >>>> [1] "binman: Correct the error message for a bad hash algorithm"
> > >>>> https://patchwork.ozlabs.org/project/uboot/patch/20220208105941.1.I8f212a1150defebaf8b7b15a79f7a2fc62c276b2@changeid/
> > >>>>
> > >>>> [2] "binman: Skip processing "hash" subnodes of FIT subsections"
> > >>>> https://patchwork.ozlabs.org/project/uboot/patch/20220209190236.26479-1-alpernebiyasak@gmail.com/
> > >>>>
> > >>>
> > >>> This one helped, indeed.
> > >>>
> > >>
> > >> ...not completely:
> > >>
> > >> $ source/tools/binman/binman replace -i flash.bin -f fit@0x380000.fit
> > >> fit@0x380000binman: [Errno 13] Permission denied: '/.fit@0x380000.itb'
> > >>
> > >
> > > Ping.
> > >
> > > $ source/tools/binman/binman -D replace -i flash.bin -f fit@0x380000.fit fit@0x380000
> > > binman: [Errno 13] Permission denied: '/.fit@0x380000.itb'
> > >
> > > Traceback (most recent call last):
> > >   File "source/tools/binman/binman", line 141, in RunBinman
> > >     ret_code = control.Binman(args)
> > >   File "u-boot/tools/binman/control.py", line 644, in Binman
> > >     allow_resize=not args.fix_size, write_map=args.map)
> > >   File "u-boot/tools/binman/control.py", line 404, in ReplaceEntries
> > >     allow_resize=allow_resize, write_map=write_map)
> > >   File "u-boot/tools/binman/control.py", line 341, in WriteEntryToImage
> > >     AfterReplace(image, allow_resize=allow_resize, write_map=write_map)
> > >   File "u-boot/tools/binman/control.py", line 333, in AfterReplace
> > >     get_contents=False, allow_resize=allow_resize)
> > >   File "u-boot/tools/binman/control.py", line 560, in ProcessImage
> > >     image.PackEntries()
> > >   File "u-boot/tools/binman/image.py", line 155, in PackEntries
> > >     super().Pack(0)
> > >   File "u-boot/tools/binman/etype/section.py", line 385, in Pack
> > >     self._PackEntries()
> > >   File "u-boot/tools/binman/etype/section.py", line 403, in _PackEntries
> > >     offset = entry.Pack(offset)
> > >   File "u-boot/tools/binman/etype/section.py", line 390, in Pack
> > >     data = self.BuildSectionData(True)
> > >   File "u-boot/tools/binman/etype/fit.py", line 265, in BuildSectionData
> > >     tools.write_file(input_fname, data)
> > >   File "u-boot/tools/patman/tools.py", line 482, in write_file
> > >     with open(filename(fname), binary and 'wb' or 'w') as fd:
> > > PermissionError: [Errno 13] Permission denied: '/.fit@0x380000.itb'
> > >
> > > Something seems fairly broken here. That '/.' does not come from the
> > > output directory name, it's generated by Entry.GetUniqueName. Looks like
> > > this path should not been taken under these conditions.
> >
> > I can reproduce this and tried a few things, but more issues just kept
> > popping up (outside u-boot as well). I got it to a point where the
> > command re-packs the FIT and the image but quite wrongly. The offset and
> > image-pos properties get added in the FIT, and the image main-section
> > just concatenates all entries without regard to set offsets. I'll
> > need more time to work those out, then to add tests and send patches.
>
> I am going to try to merge my fit generator series today.
>
> One issue I notice is that the conversion to use entry_Section changes
> the contents of the self._fit_entries dict. Before it was keyed by
> relative path, but entry_section keys self._entries by node name.
>
> We may need to split it up. I will see if I can at least merge my
> series, which should not make things any worse, then see if I can come
> up with ideas.
>
> Thanks for the diff.

I did a bit more fiddling and pushed a tree to u-boot-dm/fit-working

It refactors the fit implementation to separate scanning from emitting
the tree and I think this might help quite a bit. I'll send out the
series when I get a chance in the next few days or so.

Regards,
Simon

[..]

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type
  2022-02-21  4:40                 ` Simon Glass
@ 2022-02-22 18:58                   ` Alper Nebi Yasak
  2022-02-23 22:59                     ` Simon Glass
  0 siblings, 1 reply; 27+ messages in thread
From: Alper Nebi Yasak @ 2022-02-22 18:58 UTC (permalink / raw)
  To: Simon Glass; +Cc: Jan Kiszka, U-Boot Mailing List, Heiko Thiery

On 21/02/2022 07:40, Simon Glass wrote:
> On Sat, 19 Feb 2022 at 08:53, Simon Glass <sjg@chromium.org> wrote:
>> On Fri, 18 Feb 2022 at 10:34, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>>> I can reproduce this and tried a few things, but more issues just kept
>>> popping up (outside u-boot as well). I got it to a point where the
>>> command re-packs the FIT and the image but quite wrongly. The offset and
>>> image-pos properties get added in the FIT, and the image main-section
>>> just concatenates all entries without regard to set offsets. I'll
>>> need more time to work those out, then to add tests and send patches.
>>
>> I am going to try to merge my fit generator series today.
>>
>> One issue I notice is that the conversion to use entry_Section changes
>> the contents of the self._fit_entries dict. Before it was keyed by
>> relative path, but entry_section keys self._entries by node name.

Yeah, this causes an error in image.FindEntryPath() while trying to
replace e.g. "/fit@0x280000/images/u-boot" since there is no "images"
entry in the FIT. Changing the key to the node name works, but then the
"binman replace" invocation needs to use e.g. "/fit@0x280000/u-boot".

>>
>> We may need to split it up. I will see if I can at least merge my
>> series, which should not make things any worse, then see if I can come
>> up with ideas.
>>
>> Thanks for the diff.
> 
> I did a bit more fiddling and pushed a tree to u-boot-dm/fit-working
> 
> It refactors the fit implementation to separate scanning from emitting
> the tree and I think this might help quite a bit. I'll send out the
> series when I get a chance in the next few days or so.

I've also managed to somewhat fix the rest of the issues I wrote, so now
I can replace a FIT entry with a modified one (having a different u-boot
file), or replace a subentry of the FIT with an arbitrary file.

I couldn't look at your new version much but I'll try to see how good my
fixes apply on top of it, will probably take me longer to patchify things.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 5/5] binman: Update image positions of FIT subentries
  2022-02-07 22:08 ` [PATCH v2 5/5] binman: Update image positions of FIT subentries Alper Nebi Yasak
  2022-02-08 20:43   ` Simon Glass
@ 2022-02-23  2:35   ` Simon Glass
  1 sibling, 0 replies; 27+ messages in thread
From: Simon Glass @ 2022-02-23  2:35 UTC (permalink / raw)
  To: Simon Glass
  Cc: U-Boot Mailing List, Heiko Thiery, Jan Kiszka, Alper Nebi Yasak

Hi Alper,

On Mon, 7 Feb 2022 at 15:08, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> Binman keeps track of positions of each entry in the final image, but
> currently this data is wrong for things included in FIT entries,
> especially since a previous patch makes FIT a subclass of Section and
> inherit its implementation.
>
> There are three ways to put data into a FIT image. It can be directly
> included as a "data" property, or it can be external to the FIT image
> represented by an offset-size pair of properties. This external offset
> is either "data-position" from the start of the FIT or "data-offset"
> from the end of the FIT, and the size is "data-size" for both. However,
> binman doesn't use the "data-offset" method while building FIT entries.
>
> According to the Section docstring, its subclasses should calculate and
> set the correct offsets and sizes in SetImagePos() method. Do this for
> FIT subentries for the three ways mentioned above, and add tests for the
> two ways binman can pack them in.
>
> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes in v2:
> - Check missing_bintools list instead of catching Fdt exceptions
> - Add tag: "Reviewed-by: Simon Glass <sjg@chromium.org>"
>
>  tools/binman/etype/fit.py |  51 +++++++++++++++++
>  tools/binman/ftest.py     | 112 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 163 insertions(+)

As mentioned I had to change the previous patch in a minor way to get
it to apply.

I'd really like to get this in if possible, too. The issue is the
handling of hash nodes in a FIT, as I mentioned.

If you are able to rework this, please let me know.

I've gone ahead sent my fit series but will rebase it onto this patch
if you are able to fix it up.

Regards,
Simon

Applied to u-boot-dm, thanks!

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type
  2022-02-22 18:58                   ` Alper Nebi Yasak
@ 2022-02-23 22:59                     ` Simon Glass
  2022-02-28 11:48                       ` Jan Kiszka
  0 siblings, 1 reply; 27+ messages in thread
From: Simon Glass @ 2022-02-23 22:59 UTC (permalink / raw)
  To: Alper Nebi Yasak; +Cc: Jan Kiszka, U-Boot Mailing List, Heiko Thiery

Hi Alper,

On Tue, 22 Feb 2022 at 11:58, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> On 21/02/2022 07:40, Simon Glass wrote:
> > On Sat, 19 Feb 2022 at 08:53, Simon Glass <sjg@chromium.org> wrote:
> >> On Fri, 18 Feb 2022 at 10:34, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
> >>> I can reproduce this and tried a few things, but more issues just kept
> >>> popping up (outside u-boot as well). I got it to a point where the
> >>> command re-packs the FIT and the image but quite wrongly. The offset and
> >>> image-pos properties get added in the FIT, and the image main-section
> >>> just concatenates all entries without regard to set offsets. I'll
> >>> need more time to work those out, then to add tests and send patches.
> >>
> >> I am going to try to merge my fit generator series today.
> >>
> >> One issue I notice is that the conversion to use entry_Section changes
> >> the contents of the self._fit_entries dict. Before it was keyed by
> >> relative path, but entry_section keys self._entries by node name.
>
> Yeah, this causes an error in image.FindEntryPath() while trying to
> replace e.g. "/fit@0x280000/images/u-boot" since there is no "images"
> entry in the FIT. Changing the key to the node name works, but then the
> "binman replace" invocation needs to use e.g. "/fit@0x280000/u-boot".
>
> >>
> >> We may need to split it up. I will see if I can at least merge my
> >> series, which should not make things any worse, then see if I can come
> >> up with ideas.
> >>
> >> Thanks for the diff.
> >
> > I did a bit more fiddling and pushed a tree to u-boot-dm/fit-working
> >
> > It refactors the fit implementation to separate scanning from emitting
> > the tree and I think this might help quite a bit. I'll send out the
> > series when I get a chance in the next few days or so.
>
> I've also managed to somewhat fix the rest of the issues I wrote, so now
> I can replace a FIT entry with a modified one (having a different u-boot
> file), or replace a subentry of the FIT with an arbitrary file.
>
> I couldn't look at your new version much but I'll try to see how good my
> fixes apply on top of it, will probably take me longer to patchify things.

OK I'm going to send a new series with (most of) your suggested fixes
a new patches, then my refactoring. Just need to get things through
CI.

Regards,
Simon

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type
  2022-02-23 22:59                     ` Simon Glass
@ 2022-02-28 11:48                       ` Jan Kiszka
  2022-02-28 13:51                         ` Alper Nebi Yasak
  2022-02-28 13:56                         ` Simon Glass
  0 siblings, 2 replies; 27+ messages in thread
From: Jan Kiszka @ 2022-02-28 11:48 UTC (permalink / raw)
  To: Simon Glass, Alper Nebi Yasak; +Cc: U-Boot Mailing List, Heiko Thiery

On 23.02.22 23:59, Simon Glass wrote:
> Hi Alper,
> 
> On Tue, 22 Feb 2022 at 11:58, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>>
>> On 21/02/2022 07:40, Simon Glass wrote:
>>> On Sat, 19 Feb 2022 at 08:53, Simon Glass <sjg@chromium.org> wrote:
>>>> On Fri, 18 Feb 2022 at 10:34, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>>>>> I can reproduce this and tried a few things, but more issues just kept
>>>>> popping up (outside u-boot as well). I got it to a point where the
>>>>> command re-packs the FIT and the image but quite wrongly. The offset and
>>>>> image-pos properties get added in the FIT, and the image main-section
>>>>> just concatenates all entries without regard to set offsets. I'll
>>>>> need more time to work those out, then to add tests and send patches.
>>>>
>>>> I am going to try to merge my fit generator series today.
>>>>
>>>> One issue I notice is that the conversion to use entry_Section changes
>>>> the contents of the self._fit_entries dict. Before it was keyed by
>>>> relative path, but entry_section keys self._entries by node name.
>>
>> Yeah, this causes an error in image.FindEntryPath() while trying to
>> replace e.g. "/fit@0x280000/images/u-boot" since there is no "images"
>> entry in the FIT. Changing the key to the node name works, but then the
>> "binman replace" invocation needs to use e.g. "/fit@0x280000/u-boot".
>>
>>>>
>>>> We may need to split it up. I will see if I can at least merge my
>>>> series, which should not make things any worse, then see if I can come
>>>> up with ideas.
>>>>
>>>> Thanks for the diff.
>>>
>>> I did a bit more fiddling and pushed a tree to u-boot-dm/fit-working
>>>
>>> It refactors the fit implementation to separate scanning from emitting
>>> the tree and I think this might help quite a bit. I'll send out the
>>> series when I get a chance in the next few days or so.
>>
>> I've also managed to somewhat fix the rest of the issues I wrote, so now
>> I can replace a FIT entry with a modified one (having a different u-boot
>> file), or replace a subentry of the FIT with an arbitrary file.
>>
>> I couldn't look at your new version much but I'll try to see how good my
>> fixes apply on top of it, will probably take me longer to patchify things.
> 
> OK I'm going to send a new series with (most of) your suggested fixes
> a new patches, then my refactoring. Just need to get things through
> CI.
> 

What's the status here? I've just rebased over master, a simple revert
of this commit no longer works, and the regression is still present. Are
there any pending patches that fixes this and I should pick locally in
order to rebase/test my pending things?

Thanks,
Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type
  2022-02-28 11:48                       ` Jan Kiszka
@ 2022-02-28 13:51                         ` Alper Nebi Yasak
  2022-02-28 13:56                         ` Simon Glass
  1 sibling, 0 replies; 27+ messages in thread
From: Alper Nebi Yasak @ 2022-02-28 13:51 UTC (permalink / raw)
  To: Jan Kiszka, Simon Glass; +Cc: U-Boot Mailing List, Heiko Thiery

On 28/02/2022 14:48, Jan Kiszka wrote:
> On 23.02.22 23:59, Simon Glass wrote:
>> On Tue, 22 Feb 2022 at 11:58, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>>> I've also managed to somewhat fix the rest of the issues I wrote, so now
>>> I can replace a FIT entry with a modified one (having a different u-boot
>>> file), or replace a subentry of the FIT with an arbitrary file.
>>>
>>> I couldn't look at your new version much but I'll try to see how good my
>>> fixes apply on top of it, will probably take me longer to patchify things.
>>
>> OK I'm going to send a new series with (most of) your suggested fixes
>> a new patches, then my refactoring. Just need to get things through
>> CI.
> 
> What's the status here? I've just rebased over master, a simple revert
> of this commit no longer works, and the regression is still present. Are
> there any pending patches that fixes this and I should pick locally in
> order to rebase/test my pending things?

Sorry, I didn't get enough time to work on this at all. I'm drafting up
replies to Simon's series [1] now, after I'm done with those I'll try to
finish and send fixes for this.

[1] "binman: rockchip: Migrate from rockchip SPL_FIT_GENERATOR script"
https://lore.kernel.org/u-boot/20220223230040.159317-1-sjg@chromium.org/T/

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type
  2022-02-28 11:48                       ` Jan Kiszka
  2022-02-28 13:51                         ` Alper Nebi Yasak
@ 2022-02-28 13:56                         ` Simon Glass
  2022-02-28 14:14                           ` Jan Kiszka
  1 sibling, 1 reply; 27+ messages in thread
From: Simon Glass @ 2022-02-28 13:56 UTC (permalink / raw)
  To: Jan Kiszka; +Cc: Alper Nebi Yasak, U-Boot Mailing List, Heiko Thiery

Hi Jan,

On Mon, 28 Feb 2022 at 04:48, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
> On 23.02.22 23:59, Simon Glass wrote:
> > Hi Alper,
> >
> > On Tue, 22 Feb 2022 at 11:58, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
> >>
> >> On 21/02/2022 07:40, Simon Glass wrote:
> >>> On Sat, 19 Feb 2022 at 08:53, Simon Glass <sjg@chromium.org> wrote:
> >>>> On Fri, 18 Feb 2022 at 10:34, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
> >>>>> I can reproduce this and tried a few things, but more issues just kept
> >>>>> popping up (outside u-boot as well). I got it to a point where the
> >>>>> command re-packs the FIT and the image but quite wrongly. The offset and
> >>>>> image-pos properties get added in the FIT, and the image main-section
> >>>>> just concatenates all entries without regard to set offsets. I'll
> >>>>> need more time to work those out, then to add tests and send patches.
> >>>>
> >>>> I am going to try to merge my fit generator series today.
> >>>>
> >>>> One issue I notice is that the conversion to use entry_Section changes
> >>>> the contents of the self._fit_entries dict. Before it was keyed by
> >>>> relative path, but entry_section keys self._entries by node name.
> >>
> >> Yeah, this causes an error in image.FindEntryPath() while trying to
> >> replace e.g. "/fit@0x280000/images/u-boot" since there is no "images"
> >> entry in the FIT. Changing the key to the node name works, but then the
> >> "binman replace" invocation needs to use e.g. "/fit@0x280000/u-boot".
> >>
> >>>>
> >>>> We may need to split it up. I will see if I can at least merge my
> >>>> series, which should not make things any worse, then see if I can come
> >>>> up with ideas.
> >>>>
> >>>> Thanks for the diff.
> >>>
> >>> I did a bit more fiddling and pushed a tree to u-boot-dm/fit-working
> >>>
> >>> It refactors the fit implementation to separate scanning from emitting
> >>> the tree and I think this might help quite a bit. I'll send out the
> >>> series when I get a chance in the next few days or so.
> >>
> >> I've also managed to somewhat fix the rest of the issues I wrote, so now
> >> I can replace a FIT entry with a modified one (having a different u-boot
> >> file), or replace a subentry of the FIT with an arbitrary file.
> >>
> >> I couldn't look at your new version much but I'll try to see how good my
> >> fixes apply on top of it, will probably take me longer to patchify things.
> >
> > OK I'm going to send a new series with (most of) your suggested fixes
> > a new patches, then my refactoring. Just need to get things through
> > CI.
> >
>
> What's the status here? I've just rebased over master, a simple revert
> of this commit no longer works, and the regression is still present. Are
> there any pending patches that fixes this and I should pick locally in
> order to rebase/test my pending things?

Please see this series and review if you can:

https://patchwork.ozlabs.org/user/todo/uboot/?series=287681

I did not add a test for your issue though. Can you take a look?

Regards,
Simon

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type
  2022-02-28 13:56                         ` Simon Glass
@ 2022-02-28 14:14                           ` Jan Kiszka
  0 siblings, 0 replies; 27+ messages in thread
From: Jan Kiszka @ 2022-02-28 14:14 UTC (permalink / raw)
  To: Simon Glass; +Cc: Alper Nebi Yasak, U-Boot Mailing List, Heiko Thiery

On 28.02.22 14:56, Simon Glass wrote:
> Hi Jan,
> 
> On Mon, 28 Feb 2022 at 04:48, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>
>> On 23.02.22 23:59, Simon Glass wrote:
>>> Hi Alper,
>>>
>>> On Tue, 22 Feb 2022 at 11:58, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>>>>
>>>> On 21/02/2022 07:40, Simon Glass wrote:
>>>>> On Sat, 19 Feb 2022 at 08:53, Simon Glass <sjg@chromium.org> wrote:
>>>>>> On Fri, 18 Feb 2022 at 10:34, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>>>>>>> I can reproduce this and tried a few things, but more issues just kept
>>>>>>> popping up (outside u-boot as well). I got it to a point where the
>>>>>>> command re-packs the FIT and the image but quite wrongly. The offset and
>>>>>>> image-pos properties get added in the FIT, and the image main-section
>>>>>>> just concatenates all entries without regard to set offsets. I'll
>>>>>>> need more time to work those out, then to add tests and send patches.
>>>>>>
>>>>>> I am going to try to merge my fit generator series today.
>>>>>>
>>>>>> One issue I notice is that the conversion to use entry_Section changes
>>>>>> the contents of the self._fit_entries dict. Before it was keyed by
>>>>>> relative path, but entry_section keys self._entries by node name.
>>>>
>>>> Yeah, this causes an error in image.FindEntryPath() while trying to
>>>> replace e.g. "/fit@0x280000/images/u-boot" since there is no "images"
>>>> entry in the FIT. Changing the key to the node name works, but then the
>>>> "binman replace" invocation needs to use e.g. "/fit@0x280000/u-boot".
>>>>
>>>>>>
>>>>>> We may need to split it up. I will see if I can at least merge my
>>>>>> series, which should not make things any worse, then see if I can come
>>>>>> up with ideas.
>>>>>>
>>>>>> Thanks for the diff.
>>>>>
>>>>> I did a bit more fiddling and pushed a tree to u-boot-dm/fit-working
>>>>>
>>>>> It refactors the fit implementation to separate scanning from emitting
>>>>> the tree and I think this might help quite a bit. I'll send out the
>>>>> series when I get a chance in the next few days or so.
>>>>
>>>> I've also managed to somewhat fix the rest of the issues I wrote, so now
>>>> I can replace a FIT entry with a modified one (having a different u-boot
>>>> file), or replace a subentry of the FIT with an arbitrary file.
>>>>
>>>> I couldn't look at your new version much but I'll try to see how good my
>>>> fixes apply on top of it, will probably take me longer to patchify things.
>>>
>>> OK I'm going to send a new series with (most of) your suggested fixes
>>> a new patches, then my refactoring. Just need to get things through
>>> CI.
>>>
>>
>> What's the status here? I've just rebased over master, a simple revert
>> of this commit no longer works, and the regression is still present. Are
>> there any pending patches that fixes this and I should pick locally in
>> order to rebase/test my pending things?
> 
> Please see this series and review if you can:
> 
> https://patchwork.ozlabs.org/user/todo/uboot/?series=287681

https://patchwork.ozlabs.org/project/uboot/list/?series=287681

> 
> I did not add a test for your issue though. Can you take a look?
> 

I cannot comment on details. However, a quick test did not indicate that
the series by itself changes the situation: same error.

Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux

^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2022-02-28 14:15 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-07 22:08 [PATCH v2 0/5] binman: Improvements to FIT entry type Alper Nebi Yasak
2022-02-07 22:08 ` [PATCH v2 1/5] binman: Fix subentry expansion for " Alper Nebi Yasak
2022-02-08 15:05   ` Simon Glass
2022-02-08 20:39   ` Simon Glass
2022-02-07 22:08 ` [PATCH v2 2/5] binman: Register and check bintools from FIT subentries Alper Nebi Yasak
2022-02-07 22:08 ` [PATCH v2 3/5] binman: Check missing bintools of Section subclasses Alper Nebi Yasak
2022-02-07 22:08 ` [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type Alper Nebi Yasak
2022-02-14  9:09   ` Jan Kiszka
2022-02-15 12:27     ` Alper Nebi Yasak
2022-02-15 16:50       ` Jan Kiszka
2022-02-15 17:06         ` Jan Kiszka
2022-02-18 16:50           ` Jan Kiszka
2022-02-18 17:34             ` Alper Nebi Yasak
2022-02-19 15:53               ` Simon Glass
2022-02-21  4:40                 ` Simon Glass
2022-02-22 18:58                   ` Alper Nebi Yasak
2022-02-23 22:59                     ` Simon Glass
2022-02-28 11:48                       ` Jan Kiszka
2022-02-28 13:51                         ` Alper Nebi Yasak
2022-02-28 13:56                         ` Simon Glass
2022-02-28 14:14                           ` Jan Kiszka
2022-02-07 22:08 ` [PATCH v2 5/5] binman: Update image positions of FIT subentries Alper Nebi Yasak
2022-02-08 20:43   ` Simon Glass
2022-02-23  2:35   ` Simon Glass
2022-02-08 20:39 ` [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type Simon Glass
2022-02-08 20:39 ` [PATCH v2 3/5] binman: Check missing bintools of Section subclasses Simon Glass
2022-02-08 20:39 ` [PATCH v2 2/5] binman: Register and check bintools from FIT subentries Simon Glass

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.