u-boot.lists.denx.de archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 0/8] migrate u-boot-rockchip.bin to binman and generate an image for SPI
@ 2022-08-26 15:36 Quentin Schulz
  2022-08-26 15:36 ` [PATCH v5 1/8] binman: add support for skipping file concatenation for mkimage Quentin Schulz
                   ` (7 more replies)
  0 siblings, 8 replies; 29+ messages in thread
From: Quentin Schulz @ 2022-08-26 15:36 UTC (permalink / raw)
  Cc: bharat.gooty, rayagonda.kokatanur, sjg, philipp.tomsich,
	kever.yang, jagan, alpernebiyasak, xypron.glpk, heiko.thiery,
	u-boot, Quentin Schulz

From: Quentin Schulz <quentin.schulz@theobroma-systems.com>

This migrates the generation of u-boot-rockchip.bin from Makefile to binman
completely.

This also adds support for generating the same kind of image than
u-boot-rockchip.bin but for SPI flashes (specifically, a different image
type generated by mkimage is necessary, in addition to a different
offset in the storage medium).

This has been tested on Puma RK3399 with patch series from https://lore.kernel.org/u-boot/20220722160655.3904213-1-foss+uboot@0leil.net/#b (plus https://lore.kernel.org/u-boot/20220722113505.3875669-4-foss+uboot@0leil.net/).

Cheers,
Quentin

v5:
 - rebased on latest master,
 - added binman test for mkimage multiple data files,
 - fixed mkimage data files to pass full paths to input files to
 mkimage,

v4:
 - added binman test for mkimage filename,
 - fixed >80 chars line in patch 2/8 binman: allow user-defined filenames for
 mkimage entry,
 - fixed wrong location for endif in patch 6/8 simplify binman image
 dependencies addition to INPUTS,


v3:
 - removed
 https://lore.kernel.org/u-boot/20220722113505.3875669-4-foss+uboot@0leil.net/,
 it'll be added later on in a separate patch series,
 - added "binman: allow user-defined filenames for mkimage entry,"
 - kept idbloader.img binary creation even with binman as requested by
 community,
 - generate idbloader-spi.img binary with binman,
 - added "rockchip: remove binman temporary files when cleaning"

v2:
 - removed patch 4/8 rockchip: pad u-boot-rockchip.bin correctly because
 it would break partitions table,
 - rebased on top of master, changes to patch 3/7 rockchip: remove
 unneeded CONFIG_SPL_PAD_TO compared to the RFC 3/8 rockchip: remove
 unneeded CONFIG_SPL_PAD_TO,

Quentin Schulz (8):
  binman: add support for skipping file concatenation for mkimage
  binman: allow user-defined filenames for mkimage entry
  rockchip: remove binman temporary files when cleaning
  rockchip: generate idbloader.img content for u-boot-rockchip.bin with
    binman for ARM
  rockchip: generate u-boot-rockchip.bin with binman for ARM64 boards
  rockchip: simplify binman image dependencies addition to INPUTS
  rockchip: allow to build SPI images even without HAS_ROM option
  rockchip: add u-boot-rockchip-spi.bin image for booting from SPI-NOR
    flash

 Makefile                                      | 41 +++------------
 arch/arm/Kconfig                              |  2 +-
 arch/arm/dts/rk3288-u-boot.dtsi               |  2 +-
 arch/arm/dts/rk3399-u-boot.dtsi               |  2 +-
 arch/arm/dts/rockchip-u-boot.dtsi             | 46 +++++++++++++++-
 arch/arm/mach-rockchip/Kconfig                |  6 +--
 tools/binman/entries.rst                      | 22 ++++++++
 tools/binman/etype/mkimage.py                 | 52 ++++++++++++++++---
 tools/binman/ftest.py                         | 23 ++++++++
 .../test/241_mkimage_multiple_data_files.dts  | 21 ++++++++
 tools/binman/test/242_mkimage_filename.dts    | 20 +++++++
 11 files changed, 188 insertions(+), 49 deletions(-)
 create mode 100644 tools/binman/test/241_mkimage_multiple_data_files.dts
 create mode 100644 tools/binman/test/242_mkimage_filename.dts

-- 
2.37.2


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

* [PATCH v5 1/8] binman: add support for skipping file concatenation for mkimage
  2022-08-26 15:36 [PATCH v5 0/8] migrate u-boot-rockchip.bin to binman and generate an image for SPI Quentin Schulz
@ 2022-08-26 15:36 ` Quentin Schulz
  2022-08-27  0:21   ` Simon Glass
  2022-08-26 15:36 ` [PATCH v5 2/8] binman: allow user-defined filenames for mkimage entry Quentin Schulz
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 29+ messages in thread
From: Quentin Schulz @ 2022-08-26 15:36 UTC (permalink / raw)
  Cc: bharat.gooty, rayagonda.kokatanur, sjg, philipp.tomsich,
	kever.yang, jagan, alpernebiyasak, xypron.glpk, heiko.thiery,
	u-boot, Quentin Schulz, Quentin Schulz

From: Quentin Schulz <quentin.schulz@theobroma-systems.com>

Some image types handled by mkimage require the datafiles to be passed
independently (-d data1:data2) for specific handling of each. A
concatenation of datafiles prior to passing them to mkimage wouldn't
work.

That is the case for rkspi for example which requires page alignment
and only writing 2KB every 4KB.

This adds the ability to tell binman to pass the datafiles without
prior concatenation to mkimage, by adding the multiple-data-files
boolean property to the mkimage node.

Cc: Quentin Schulz <foss+uboot@0leil.net>
Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
---

v5:
 - changed to use full path from input dir with tools.get_input_filename
 to make it possible to run the unit tests,
 - added unit test,


 tools/binman/entries.rst                      | 22 ++++++++++
 tools/binman/etype/mkimage.py                 | 41 +++++++++++++++++--
 tools/binman/ftest.py                         | 16 ++++++++
 .../test/241_mkimage_multiple_data_files.dts  | 21 ++++++++++
 4 files changed, 96 insertions(+), 4 deletions(-)
 create mode 100644 tools/binman/test/241_mkimage_multiple_data_files.dts

diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
index b3613d7cbd..18bd328c5c 100644
--- a/tools/binman/entries.rst
+++ b/tools/binman/entries.rst
@@ -1175,6 +1175,9 @@ Properties / Entry arguments:
     - args: Arguments to pass
     - data-to-imagename: Indicates that the -d data should be passed in as
       the image name also (-n)
+    - multiple-data-files: boolean to tell binman to pass all files as
+      datafiles to mkimage instead of creating a temporary file the result
+      of datafiles concatenation
 
 The data passed to mkimage via the -d flag is collected from subnodes of the
 mkimage node, e.g.::
@@ -1205,6 +1208,25 @@ a section, or just multiple subnodes like this::
         };
     };
 
+To pass all datafiles untouched to mkimage::
+
+    mkimage {
+        args = "-n rk3399 -T rkspi";
+        multiple-data-files;
+
+        u-boot-tpl {
+        };
+
+        u-boot-spl {
+        };
+    };
+
+This calls mkimage to create a Rockchip RK3399-specific first stage
+bootloader, made of TPL+SPL. Since this first stage bootloader requires to
+align the TPL and SPL but also some weird hacks that is handled by mkimage
+directly, binman is told to not perform the concatenation of datafiles prior
+to passing the data to mkimage.
+
 To use CONFIG options in the arguments, use a string list instead, as in
 this example which also produces four arguments::
 
diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py
index ddbd9cec65..5f4bc6fa3c 100644
--- a/tools/binman/etype/mkimage.py
+++ b/tools/binman/etype/mkimage.py
@@ -18,6 +18,9 @@ class Entry_mkimage(Entry):
         - args: Arguments to pass
         - data-to-imagename: Indicates that the -d data should be passed in as
           the image name also (-n)
+        - multiple-data-files: boolean to tell binman to pass all files as
+          datafiles to mkimage instead of creating a temporary file the result
+          of datafiles concatenation
 
     The data passed to mkimage via the -d flag is collected from subnodes of the
     mkimage node, e.g.::
@@ -51,6 +54,25 @@ class Entry_mkimage(Entry):
     Note that binman places the contents (here SPL and TPL) into a single file
     and passes that to mkimage using the -d option.
 
+	To pass all datafiles untouched to mkimage::
+
+		mkimage {
+			args = "-n rk3399 -T rkspi";
+			multiple-data-files;
+
+			u-boot-tpl {
+			};
+
+			u-boot-spl {
+			};
+		};
+
+	This calls mkimage to create a Rockchip RK3399-specific first stage
+	bootloader, made of TPL+SPL. Since this first stage bootloader requires to
+	align the TPL and SPL but also some weird hacks that is handled by mkimage
+	directly, binman is told to not perform the concatenation of datafiles prior
+	to passing the data to mkimage.
+
     To use CONFIG options in the arguments, use a string list instead, as in
     this example which also produces four arguments::
 
@@ -96,6 +118,7 @@ class Entry_mkimage(Entry):
     """
     def __init__(self, section, etype, node):
         super().__init__(section, etype, node)
+        self._multiple_data_files = fdt_util.GetBool(self._node, 'multiple-data-files')
         self._mkimage_entries = OrderedDict()
         self._imagename = None
         self.align_default = None
@@ -122,10 +145,20 @@ class Entry_mkimage(Entry):
     def ObtainContents(self):
         # Use a non-zero size for any fake files to keep mkimage happy
         # Note that testMkimageImagename() relies on this 'mkimage' parameter
-        data, input_fname, uniq = self.collect_contents_to_file(
-            self._mkimage_entries.values(), 'mkimage', 1024)
-        if data is None:
-            return False
+        fake_size = 1024
+        if self._multiple_data_files:
+            fnames = []
+            uniq = self.GetUniqueName()
+            for entry in self._mkimage_entries.values():
+                if not entry.ObtainContents(fake_size=fake_size):
+                    return False
+                fnames.append(tools.get_input_filename(entry.GetDefaultFilename()))
+            input_fname = ":".join(fnames)
+        else:
+            data, input_fname, uniq = self.collect_contents_to_file(
+                self._mkimage_entries.values(), 'mkimage', fake_size)
+            if data is None:
+                return False
         if self._imagename:
             image_data, imagename_fname, _ = self.collect_contents_to_file(
                 [self._imagename], 'mkimage-n', 1024)
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index 0b1774046f..091692ef93 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -5818,6 +5818,22 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
         expect = U_BOOT_SPL_DATA + U_BOOT_DATA
         self.assertEqual(expect, data[:len(expect)])
 
+    def testMkimageMultipleDataFiles(self):
+        """Test passing multiple files to mkimage in a mkimage entry"""
+        data = self._DoReadFile('241_mkimage_multiple_data_files.dts')
+        # Size of files are packed in their 4B big-endian format
+        expect = struct.pack('>I', len(U_BOOT_TPL_DATA))
+        expect += struct.pack('>I', len(U_BOOT_SPL_DATA))
+        # Size info is always followed by a 4B zero value.
+        expect += tools.get_bytes(0, 4)
+        expect += U_BOOT_TPL_DATA
+        # All but last files are 4B-aligned
+        align_pad = len(U_BOOT_TPL_DATA) % 4
+        if align_pad:
+            expect += tools.get_bytes(0, align_pad)
+        expect += U_BOOT_SPL_DATA
+        self.assertEqual(expect, data[-len(expect):])
+
     def testCompressDtbPrependInvalid(self):
         """Test that invalid header is detected"""
         with self.assertRaises(ValueError) as e:
diff --git a/tools/binman/test/241_mkimage_multiple_data_files.dts b/tools/binman/test/241_mkimage_multiple_data_files.dts
new file mode 100644
index 0000000000..a092bc39bf
--- /dev/null
+++ b/tools/binman/test/241_mkimage_multiple_data_files.dts
@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	binman {
+		mkimage {
+			args = "-T script";
+			multiple-data-files;
+
+			u-boot-tpl {
+			};
+
+			u-boot-spl {
+			};
+		};
+	};
+};
-- 
2.37.2


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

* [PATCH v5 2/8] binman: allow user-defined filenames for mkimage entry
  2022-08-26 15:36 [PATCH v5 0/8] migrate u-boot-rockchip.bin to binman and generate an image for SPI Quentin Schulz
  2022-08-26 15:36 ` [PATCH v5 1/8] binman: add support for skipping file concatenation for mkimage Quentin Schulz
@ 2022-08-26 15:36 ` Quentin Schulz
  2022-08-27  0:21   ` Simon Glass
  2022-08-26 15:36 ` [PATCH v5 3/8] rockchip: remove binman temporary files when cleaning Quentin Schulz
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 29+ messages in thread
From: Quentin Schulz @ 2022-08-26 15:36 UTC (permalink / raw)
  Cc: bharat.gooty, rayagonda.kokatanur, sjg, philipp.tomsich,
	kever.yang, jagan, alpernebiyasak, xypron.glpk, heiko.thiery,
	u-boot, Quentin Schulz, Quentin Schulz

From: Quentin Schulz <quentin.schulz@theobroma-systems.com>

mkimage entry currently creates a file whose name is derived from the
section name containing said entry.

Let's allow the user to define a filename for the mkimage-generated
binary by using the 'filename' DT property.

Cc: Quentin Schulz <foss+uboot@0leil.net>
Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
---

v5:
 - updated unit test filename,
 - removed _testing section in unit test,
 - removed -n arg in unit test,
 - removed size property in binman node,

v4:
 - added binman test,
 - fixed >80 chars-long line,

added in v3

 tools/binman/etype/mkimage.py              | 11 ++++++++---
 tools/binman/ftest.py                      |  7 +++++++
 tools/binman/test/242_mkimage_filename.dts | 18 ++++++++++++++++++
 3 files changed, 33 insertions(+), 3 deletions(-)
 create mode 100644 tools/binman/test/242_mkimage_filename.dts

diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py
index 5f4bc6fa3c..c2288c48ee 100644
--- a/tools/binman/etype/mkimage.py
+++ b/tools/binman/etype/mkimage.py
@@ -21,11 +21,13 @@ class Entry_mkimage(Entry):
         - multiple-data-files: boolean to tell binman to pass all files as
           datafiles to mkimage instead of creating a temporary file the result
           of datafiles concatenation
+        - filename: filename of output binary generated by mkimage
 
     The data passed to mkimage via the -d flag is collected from subnodes of the
     mkimage node, e.g.::
 
         mkimage {
+            filename = "imximage.bin";
             args = "-n test -T imximage";
 
             u-boot-spl {
@@ -38,8 +40,9 @@ class Entry_mkimage(Entry):
         mkimage -d <data_file> -n test -T imximage <output_file>
 
     The output from mkimage then becomes part of the image produced by
-    binman. If you need to put multiple things in the data file, you can use
-    a section, or just multiple subnodes like this::
+    binman but also is written into `imximage.bin` file. If you need to put
+    multiple things in the data file, you can use a section, or just multiple
+    subnodes like this::
 
         mkimage {
             args = "-n test -T imximage";
@@ -121,6 +124,7 @@ class Entry_mkimage(Entry):
         self._multiple_data_files = fdt_util.GetBool(self._node, 'multiple-data-files')
         self._mkimage_entries = OrderedDict()
         self._imagename = None
+        self._filename = fdt_util.GetString(self._node, 'filename')
         self.align_default = None
 
     def ReadNode(self):
@@ -164,7 +168,8 @@ class Entry_mkimage(Entry):
                 [self._imagename], 'mkimage-n', 1024)
             if image_data is None:
                 return False
-        output_fname = tools.get_output_filename('mkimage-out.%s' % uniq)
+        outfile = self._filename if self._filename else 'mkimage-out.%s' % uniq
+        output_fname = tools.get_output_filename(outfile)
 
         args = ['-d', input_fname]
         if self._data_to_imagename:
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index 091692ef93..da0c7299ac 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -5834,6 +5834,13 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
         expect += U_BOOT_SPL_DATA
         self.assertEqual(expect, data[-len(expect):])
 
+    def testMkimageFilename(self):
+        """Test using mkimage to build a binary with a filename"""
+        retcode = self._DoTestFile('242_mkimage_filename.dts')
+        self.assertEqual(0, retcode)
+        fname = tools.get_output_filename('mkimage-test.bin')
+        self.assertTrue(os.path.exists(fname))
+
     def testCompressDtbPrependInvalid(self):
         """Test that invalid header is detected"""
         with self.assertRaises(ValueError) as e:
diff --git a/tools/binman/test/242_mkimage_filename.dts b/tools/binman/test/242_mkimage_filename.dts
new file mode 100644
index 0000000000..4483790ae8
--- /dev/null
+++ b/tools/binman/test/242_mkimage_filename.dts
@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	binman {
+		mkimage {
+			filename = "mkimage-test.bin";
+			args = "-T script";
+
+			u-boot-spl {
+			};
+		};
+	};
+};
-- 
2.37.2


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

* [PATCH v5 3/8] rockchip: remove binman temporary files when cleaning
  2022-08-26 15:36 [PATCH v5 0/8] migrate u-boot-rockchip.bin to binman and generate an image for SPI Quentin Schulz
  2022-08-26 15:36 ` [PATCH v5 1/8] binman: add support for skipping file concatenation for mkimage Quentin Schulz
  2022-08-26 15:36 ` [PATCH v5 2/8] binman: allow user-defined filenames for mkimage entry Quentin Schulz
@ 2022-08-26 15:36 ` Quentin Schulz
  2022-08-27  0:21   ` Simon Glass
  2022-09-01 12:04   ` Kever Yang
  2022-08-26 15:36 ` [PATCH v5 4/8] rockchip: generate idbloader.img content for u-boot-rockchip.bin with binman for ARM Quentin Schulz
                   ` (4 subsequent siblings)
  7 siblings, 2 replies; 29+ messages in thread
From: Quentin Schulz @ 2022-08-26 15:36 UTC (permalink / raw)
  Cc: bharat.gooty, rayagonda.kokatanur, sjg, philipp.tomsich,
	kever.yang, jagan, alpernebiyasak, xypron.glpk, heiko.thiery,
	u-boot, Quentin Schulz, Quentin Schulz, Johan Jonker

From: Quentin Schulz <quentin.schulz@theobroma-systems.com>

Binman mkimage entry generates temporary files so let's remove them
when calling `make clean`.

Fixes: 9b312e26fc77 ("rockchip: Enable building a SPI ROM image on jerry")
Cc: Quentin Schulz <foss+uboot@0leil.net>
Reported-by: Johan Jonker <jbx6244@gmail.com>
Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
---

added in v3

 Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 541e942ed5..5750a9e4b8 100644
--- a/Makefile
+++ b/Makefile
@@ -2225,7 +2225,8 @@ CLEAN_FILES += include/bmp_logo.h include/bmp_logo_data.h \
 	       lpc32xx-* bl31.c bl31.elf bl31_*.bin image.map tispl.bin* \
 	       idbloader.img flash.bin flash.log defconfig keep-syms-lto.c \
 	       mkimage-out.spl.mkimage mkimage.spl.mkimage imx-boot.map \
-	       itb.fit.fit itb.fit.itb itb.map spl.map
+	       itb.fit.fit itb.fit.itb itb.map spl.map mkimage-out.rom.mkimage \
+	       mkimage.rom.mkimage rom.map simple-bin.map
 
 # Directories & files removed with 'make mrproper'
 MRPROPER_DIRS  += include/config include/generated spl tpl \
-- 
2.37.2


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

* [PATCH v5 4/8] rockchip: generate idbloader.img content for u-boot-rockchip.bin with binman for ARM
  2022-08-26 15:36 [PATCH v5 0/8] migrate u-boot-rockchip.bin to binman and generate an image for SPI Quentin Schulz
                   ` (2 preceding siblings ...)
  2022-08-26 15:36 ` [PATCH v5 3/8] rockchip: remove binman temporary files when cleaning Quentin Schulz
@ 2022-08-26 15:36 ` Quentin Schulz
  2022-09-01 12:04   ` Kever Yang
  2022-08-26 15:36 ` [PATCH v5 5/8] rockchip: generate u-boot-rockchip.bin with binman for ARM64 boards Quentin Schulz
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 29+ messages in thread
From: Quentin Schulz @ 2022-08-26 15:36 UTC (permalink / raw)
  Cc: bharat.gooty, rayagonda.kokatanur, sjg, philipp.tomsich,
	kever.yang, jagan, alpernebiyasak, xypron.glpk, heiko.thiery,
	u-boot, Quentin Schulz, Quentin Schulz

From: Quentin Schulz <quentin.schulz@theobroma-systems.com>

idbloader.img content - currently created by way of Makefile - can be
created by binman directly.

So let's do that for Rockchip ARM platforms.

Cc: Quentin Schulz <foss+uboot@0leil.net>
Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
---
v4:
 - added Reviewed-by,

v3:
 - moved spl back into mkimage section,
 - added filename property so that the idbloader.img binary is still
 generated,
 Makefile                          |  2 +-
 arch/arm/dts/rockchip-u-boot.dtsi | 11 ++++++++++-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 5750a9e4b8..dbe1aa254a 100644
--- a/Makefile
+++ b/Makefile
@@ -1011,7 +1011,7 @@ endif
 else
 ifeq ($(CONFIG_SPL),y)
 # Generate these inputs for binman which will create the output files
-INPUTS-y += idbloader.img u-boot.img
+INPUTS-y += u-boot.img
 endif
 endif
 endif
diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
index eae3ee715d..ad72ca9700 100644
--- a/arch/arm/dts/rockchip-u-boot.dtsi
+++ b/arch/arm/dts/rockchip-u-boot.dtsi
@@ -17,8 +17,17 @@
 		filename = "u-boot-rockchip.bin";
 		pad-byte = <0xff>;
 
-		blob {
+		mkimage {
 			filename = "idbloader.img";
+			args = "-n", CONFIG_SYS_SOC, "-T", "rksd";
+#ifdef CONFIG_TPL
+			multiple-data-files;
+
+			u-boot-tpl {
+			};
+#endif
+			u-boot-spl {
+			};
 		};
 
 		u-boot-img {
-- 
2.37.2


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

* [PATCH v5 5/8] rockchip: generate u-boot-rockchip.bin with binman for ARM64 boards
  2022-08-26 15:36 [PATCH v5 0/8] migrate u-boot-rockchip.bin to binman and generate an image for SPI Quentin Schulz
                   ` (3 preceding siblings ...)
  2022-08-26 15:36 ` [PATCH v5 4/8] rockchip: generate idbloader.img content for u-boot-rockchip.bin with binman for ARM Quentin Schulz
@ 2022-08-26 15:36 ` Quentin Schulz
  2022-09-01 12:05   ` Kever Yang
  2022-08-26 15:36 ` [PATCH v5 6/8] rockchip: simplify binman image dependencies addition to INPUTS Quentin Schulz
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 29+ messages in thread
From: Quentin Schulz @ 2022-08-26 15:36 UTC (permalink / raw)
  Cc: bharat.gooty, rayagonda.kokatanur, sjg, philipp.tomsich,
	kever.yang, jagan, alpernebiyasak, xypron.glpk, heiko.thiery,
	u-boot, Quentin Schulz, Quentin Schulz

From: Quentin Schulz <quentin.schulz@theobroma-systems.com>

This allows to build u-boot-rockchip.bin binary with binman for Rockchip
ARM64 boards instead of the legacy Makefile way.

Cc: Quentin Schulz <foss+uboot@0leil.net>
Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
---

v4:
 - added Reviewed-by,

 Makefile                          | 26 +-------------------------
 arch/arm/Kconfig                  |  2 +-
 arch/arm/dts/rockchip-u-boot.dtsi |  5 +++++
 3 files changed, 7 insertions(+), 26 deletions(-)

diff --git a/Makefile b/Makefile
index dbe1aa254a..1dee09eb36 100644
--- a/Makefile
+++ b/Makefile
@@ -1005,8 +1005,7 @@ ifeq ($(CONFIG_ARCH_ROCKCHIP),y)
 # On ARM64 this target is produced by binman so we don't need this dep
 ifeq ($(CONFIG_ARM64),y)
 ifeq ($(CONFIG_SPL),y)
-# TODO: Get binman to generate this too
-INPUTS-y += u-boot-rockchip.bin
+INPUTS-y += u-boot.itb
 endif
 else
 ifeq ($(CONFIG_SPL),y)
@@ -1498,29 +1497,6 @@ OBJCOPYFLAGS_u-boot-with-spl.bin = -I binary -O binary \
 u-boot-with-spl.bin: $(SPL_IMAGE) $(SPL_PAYLOAD) FORCE
 	$(call if_changed,pad_cat)
 
-ifeq ($(CONFIG_ARCH_ROCKCHIP),y)
-
-# TPL + SPL
-ifeq ($(CONFIG_SPL)$(CONFIG_TPL),yy)
-MKIMAGEFLAGS_u-boot-tpl-rockchip.bin = -n $(CONFIG_SYS_SOC) -T rksd
-tpl/u-boot-tpl-rockchip.bin: tpl/u-boot-tpl.bin FORCE
-	$(call if_changed,mkimage)
-idbloader.img: tpl/u-boot-tpl-rockchip.bin spl/u-boot-spl.bin FORCE
-	$(call if_changed,cat)
-else
-MKIMAGEFLAGS_idbloader.img = -n $(CONFIG_SYS_SOC) -T rksd
-idbloader.img: spl/u-boot-spl.bin FORCE
-	$(call if_changed,mkimage)
-endif
-
-ifeq ($(CONFIG_ARM64),y)
-OBJCOPYFLAGS_u-boot-rockchip.bin = -I binary -O binary \
-	--pad-to=$(CONFIG_SPL_PAD_TO) --gap-fill=0xff
-u-boot-rockchip.bin: idbloader.img u-boot.itb FORCE
-	$(call if_changed,pad_cat)
-endif # CONFIG_ARM64
-
-endif # CONFIG_ARCH_ROCKCHIP
 
 ifeq ($(CONFIG_ARCH_LPC32XX)$(CONFIG_SPL),yy)
 MKIMAGEFLAGS_lpc32xx-spl.img = -T lpc32xximage -a $(CONFIG_SPL_TEXT_BASE)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 0b72e4f650..82cd456f51 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1974,7 +1974,7 @@ config ARCH_STM32MP
 config ARCH_ROCKCHIP
 	bool "Support Rockchip SoCs"
 	select BLK
-	select BINMAN if SPL_OPTEE || (SPL && !ARM64)
+	select BINMAN if SPL_OPTEE || SPL
 	select DM
 	select DM_GPIO
 	select DM_I2C
diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
index ad72ca9700..f90a8bf085 100644
--- a/arch/arm/dts/rockchip-u-boot.dtsi
+++ b/arch/arm/dts/rockchip-u-boot.dtsi
@@ -30,7 +30,12 @@
 			};
 		};
 
+#ifdef CONFIG_ARM64
+		blob {
+			filename = "u-boot.itb";
+#else
 		u-boot-img {
+#endif
 			offset = <CONFIG_SPL_PAD_TO>;
 		};
 	};
-- 
2.37.2


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

* [PATCH v5 6/8] rockchip: simplify binman image dependencies addition to INPUTS
  2022-08-26 15:36 [PATCH v5 0/8] migrate u-boot-rockchip.bin to binman and generate an image for SPI Quentin Schulz
                   ` (4 preceding siblings ...)
  2022-08-26 15:36 ` [PATCH v5 5/8] rockchip: generate u-boot-rockchip.bin with binman for ARM64 boards Quentin Schulz
@ 2022-08-26 15:36 ` Quentin Schulz
  2022-08-27  0:21   ` Simon Glass
  2022-09-01 12:08   ` Kever Yang
  2022-08-26 15:36 ` [PATCH v5 7/8] rockchip: allow to build SPI images even without HAS_ROM option Quentin Schulz
  2022-08-26 15:36 ` [PATCH v5 8/8] rockchip: add u-boot-rockchip-spi.bin image for booting from SPI-NOR flash Quentin Schulz
  7 siblings, 2 replies; 29+ messages in thread
From: Quentin Schulz @ 2022-08-26 15:36 UTC (permalink / raw)
  Cc: bharat.gooty, rayagonda.kokatanur, sjg, philipp.tomsich,
	kever.yang, jagan, alpernebiyasak, xypron.glpk, heiko.thiery,
	u-boot, Quentin Schulz, Quentin Schulz

From: Quentin Schulz <quentin.schulz@theobroma-systems.com>

By factoring SPL check in the first condition, this makes the checks a
bit less convoluted and more readable.

Cc: Quentin Schulz <foss+uboot@0leil.net>
Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
---

v4:
 - fixed wrong place for endif for ARM32 boards,

 Makefile | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/Makefile b/Makefile
index 1dee09eb36..736c4ad182 100644
--- a/Makefile
+++ b/Makefile
@@ -1001,19 +1001,14 @@ ifeq ($(CONFIG_MPC85xx)$(CONFIG_OF_SEPARATE),yy)
 INPUTS-y += u-boot-with-dtb.bin
 endif
 
-ifeq ($(CONFIG_ARCH_ROCKCHIP),y)
-# On ARM64 this target is produced by binman so we don't need this dep
+ifeq ($(CONFIG_ARCH_ROCKCHIP)$(CONFIG_SPL),yy)
+# Binman image dependencies
 ifeq ($(CONFIG_ARM64),y)
-ifeq ($(CONFIG_SPL),y)
 INPUTS-y += u-boot.itb
-endif
 else
-ifeq ($(CONFIG_SPL),y)
-# Generate these inputs for binman which will create the output files
 INPUTS-y += u-boot.img
 endif
 endif
-endif
 
 INPUTS-$(CONFIG_X86) += u-boot-x86-start16.bin u-boot-x86-reset16.bin \
 	$(if $(CONFIG_SPL_X86_16BIT_INIT),spl/u-boot-spl.bin) \
-- 
2.37.2


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

* [PATCH v5 7/8] rockchip: allow to build SPI images even without HAS_ROM option
  2022-08-26 15:36 [PATCH v5 0/8] migrate u-boot-rockchip.bin to binman and generate an image for SPI Quentin Schulz
                   ` (5 preceding siblings ...)
  2022-08-26 15:36 ` [PATCH v5 6/8] rockchip: simplify binman image dependencies addition to INPUTS Quentin Schulz
@ 2022-08-26 15:36 ` Quentin Schulz
  2022-08-27  0:21   ` Simon Glass
  2022-09-01 12:08   ` Kever Yang
  2022-08-26 15:36 ` [PATCH v5 8/8] rockchip: add u-boot-rockchip-spi.bin image for booting from SPI-NOR flash Quentin Schulz
  7 siblings, 2 replies; 29+ messages in thread
From: Quentin Schulz @ 2022-08-26 15:36 UTC (permalink / raw)
  Cc: bharat.gooty, rayagonda.kokatanur, sjg, philipp.tomsich,
	kever.yang, jagan, alpernebiyasak, xypron.glpk, heiko.thiery,
	u-boot, Quentin Schulz, Quentin Schulz

From: Quentin Schulz <quentin.schulz@theobroma-systems.com>

This prepares for the creation of a u-boot-rockchip-spi.bin image
similar to u-boot-rockchip.bin to the exception it's destined for
SPI-NOR flashes instead of MMC storage medium.

Cc: Quentin Schulz <foss+uboot@0leil.net>
Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
---
 arch/arm/dts/rk3288-u-boot.dtsi | 2 +-
 arch/arm/dts/rk3399-u-boot.dtsi | 2 +-
 arch/arm/mach-rockchip/Kconfig  | 6 ++----
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/arch/arm/dts/rk3288-u-boot.dtsi b/arch/arm/dts/rk3288-u-boot.dtsi
index 9eb696b141..e411445ed6 100644
--- a/arch/arm/dts/rk3288-u-boot.dtsi
+++ b/arch/arm/dts/rk3288-u-boot.dtsi
@@ -56,7 +56,7 @@
 	};
 };
 
-#ifdef CONFIG_ROCKCHIP_SPI_IMAGE
+#if defined(CONFIG_ROCKCHIP_SPI_IMAGE) && defined(CONFIG_HAS_ROM)
 &binman {
 	rom {
 		filename = "u-boot.rom";
diff --git a/arch/arm/dts/rk3399-u-boot.dtsi b/arch/arm/dts/rk3399-u-boot.dtsi
index 716b9a433a..3c1a15fe51 100644
--- a/arch/arm/dts/rk3399-u-boot.dtsi
+++ b/arch/arm/dts/rk3399-u-boot.dtsi
@@ -60,7 +60,7 @@
 
 };
 
-#ifdef CONFIG_ROCKCHIP_SPI_IMAGE
+#if defined(CONFIG_ROCKCHIP_SPI_IMAGE) && defined(CONFIG_HAS_ROM)
 &binman {
 	rom {
 		filename = "u-boot.rom";
diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
index c561a77e6a..b46cea2f91 100644
--- a/arch/arm/mach-rockchip/Kconfig
+++ b/arch/arm/mach-rockchip/Kconfig
@@ -425,12 +425,10 @@ config SPL_MMC
 
 config ROCKCHIP_SPI_IMAGE
 	bool "Build a SPI image for rockchip"
-	depends on HAS_ROM
 	help
 	  Some Rockchip SoCs support booting from SPI flash. Enable this
-	  option to produce a 4MB SPI-flash image (called u-boot.rom)
-	  containing U-Boot. The image is built by binman. U-Boot sits near
-	  the start of the image.
+	  option to produce a SPI-flash image containing U-Boot. The image
+	  is built by binman. U-Boot sits near the start of the image.
 
 config LNX_KRNL_IMG_TEXT_OFFSET_BASE
 	default SYS_TEXT_BASE
-- 
2.37.2


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

* [PATCH v5 8/8] rockchip: add u-boot-rockchip-spi.bin image for booting from SPI-NOR flash
  2022-08-26 15:36 [PATCH v5 0/8] migrate u-boot-rockchip.bin to binman and generate an image for SPI Quentin Schulz
                   ` (6 preceding siblings ...)
  2022-08-26 15:36 ` [PATCH v5 7/8] rockchip: allow to build SPI images even without HAS_ROM option Quentin Schulz
@ 2022-08-26 15:36 ` Quentin Schulz
  2022-08-27  0:21   ` Simon Glass
  2022-09-01 12:08   ` Kever Yang
  7 siblings, 2 replies; 29+ messages in thread
From: Quentin Schulz @ 2022-08-26 15:36 UTC (permalink / raw)
  Cc: bharat.gooty, rayagonda.kokatanur, sjg, philipp.tomsich,
	kever.yang, jagan, alpernebiyasak, xypron.glpk, heiko.thiery,
	u-boot, Quentin Schulz, Quentin Schulz

From: Quentin Schulz <quentin.schulz@theobroma-systems.com>

This new image is similar to u-boot-rockchip.bin except that it's
destined to be flashed on SPI-NOR flashes.

Cc: Quentin Schulz <foss+uboot@0leil.net>
Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
---

v3:
 - added filename property so that idblaoder-spi.img binary is generated
 by binman, as per community request,
 - added new temporary files to the list of files to clean up on `make
 clean`,

 Makefile                          |  3 ++-
 arch/arm/dts/rockchip-u-boot.dtsi | 30 ++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 736c4ad182..e70e92c947 100644
--- a/Makefile
+++ b/Makefile
@@ -2197,7 +2197,8 @@ CLEAN_FILES += include/bmp_logo.h include/bmp_logo_data.h \
 	       idbloader.img flash.bin flash.log defconfig keep-syms-lto.c \
 	       mkimage-out.spl.mkimage mkimage.spl.mkimage imx-boot.map \
 	       itb.fit.fit itb.fit.itb itb.map spl.map mkimage-out.rom.mkimage \
-	       mkimage.rom.mkimage rom.map simple-bin.map
+	       mkimage.rom.mkimage rom.map simple-bin.map simple-bin-spi.map \
+	       idbloader-spi.img
 
 # Directories & files removed with 'make mrproper'
 MRPROPER_DIRS  += include/config include/generated spl tpl \
diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
index f90a8bf085..584f21eb5b 100644
--- a/arch/arm/dts/rockchip-u-boot.dtsi
+++ b/arch/arm/dts/rockchip-u-boot.dtsi
@@ -39,5 +39,35 @@
 			offset = <CONFIG_SPL_PAD_TO>;
 		};
 	};
+
+#ifdef CONFIG_ROCKCHIP_SPI_IMAGE
+	simple-bin-spi {
+		filename = "u-boot-rockchip-spi.bin";
+		pad-byte = <0xff>;
+
+		mkimage {
+			filename = "idbloader-spi.img";
+			args = "-n", CONFIG_SYS_SOC, "-T", "rkspi";
+#ifdef CONFIG_TPL
+			multiple-data-files;
+
+			u-boot-tpl {
+			};
+#endif
+			u-boot-spl {
+			};
+		};
+
+#ifdef CONFIG_ARM64
+		blob {
+			filename = "u-boot.itb";
+#else
+		u-boot-img {
+#endif
+			/* Sync with u-boot,spl-payload-offset if present */
+			offset = <CONFIG_SYS_SPI_U_BOOT_OFFS>;
+		};
+	};
+#endif
 };
 #endif
-- 
2.37.2


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

* Re: [PATCH v5 2/8] binman: allow user-defined filenames for mkimage entry
  2022-08-26 15:36 ` [PATCH v5 2/8] binman: allow user-defined filenames for mkimage entry Quentin Schulz
@ 2022-08-27  0:21   ` Simon Glass
  0 siblings, 0 replies; 29+ messages in thread
From: Simon Glass @ 2022-08-27  0:21 UTC (permalink / raw)
  To: Quentin Schulz
  Cc: Bharat Gooty, Rayagonda Kokatanur, Philipp Tomsich, Kever Yang,
	Jagan Teki, Alper Nebi Yasak, Heinrich Schuchardt, Heiko Thiery,
	U-Boot Mailing List, Quentin Schulz

On Fri, 26 Aug 2022 at 09:37, Quentin Schulz <foss+uboot@0leil.net> wrote:
>
> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>
> mkimage entry currently creates a file whose name is derived from the
> section name containing said entry.
>
> Let's allow the user to define a filename for the mkimage-generated
> binary by using the 'filename' DT property.
>
> Cc: Quentin Schulz <foss+uboot@0leil.net>
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> ---
>
> v5:
>  - updated unit test filename,
>  - removed _testing section in unit test,
>  - removed -n arg in unit test,
>  - removed size property in binman node,
>
> v4:
>  - added binman test,
>  - fixed >80 chars-long line,
>
> added in v3
>
>  tools/binman/etype/mkimage.py              | 11 ++++++++---
>  tools/binman/ftest.py                      |  7 +++++++
>  tools/binman/test/242_mkimage_filename.dts | 18 ++++++++++++++++++
>  3 files changed, 33 insertions(+), 3 deletions(-)
>  create mode 100644 tools/binman/test/242_mkimage_filename.dts

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

nit below

>
> diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py
> index 5f4bc6fa3c..c2288c48ee 100644
> --- a/tools/binman/etype/mkimage.py
> +++ b/tools/binman/etype/mkimage.py
> @@ -21,11 +21,13 @@ class Entry_mkimage(Entry):
>          - multiple-data-files: boolean to tell binman to pass all files as
>            datafiles to mkimage instead of creating a temporary file the result
>            of datafiles concatenation
> +        - filename: filename of output binary generated by mkimage
>
>      The data passed to mkimage via the -d flag is collected from subnodes of the
>      mkimage node, e.g.::
>
>          mkimage {
> +            filename = "imximage.bin";
>              args = "-n test -T imximage";
>
>              u-boot-spl {
> @@ -38,8 +40,9 @@ class Entry_mkimage(Entry):
>          mkimage -d <data_file> -n test -T imximage <output_file>
>
>      The output from mkimage then becomes part of the image produced by
> -    binman. If you need to put multiple things in the data file, you can use
> -    a section, or just multiple subnodes like this::
> +    binman but also is written into `imximage.bin` file. If you need to put
> +    multiple things in the data file, you can use a section, or just multiple
> +    subnodes like this::
>
>          mkimage {
>              args = "-n test -T imximage";
> @@ -121,6 +124,7 @@ class Entry_mkimage(Entry):
>          self._multiple_data_files = fdt_util.GetBool(self._node, 'multiple-data-files')
>          self._mkimage_entries = OrderedDict()
>          self._imagename = None
> +        self._filename = fdt_util.GetString(self._node, 'filename')
>          self.align_default = None
>
>      def ReadNode(self):
> @@ -164,7 +168,8 @@ class Entry_mkimage(Entry):
>                  [self._imagename], 'mkimage-n', 1024)
>              if image_data is None:
>                  return False
> -        output_fname = tools.get_output_filename('mkimage-out.%s' % uniq)
> +        outfile = self._filename if self._filename else 'mkimage-out.%s' % uniq
> +        output_fname = tools.get_output_filename(outfile)
>
>          args = ['-d', input_fname]
>          if self._data_to_imagename:
> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> index 091692ef93..da0c7299ac 100644
> --- a/tools/binman/ftest.py
> +++ b/tools/binman/ftest.py
> @@ -5834,6 +5834,13 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
>          expect += U_BOOT_SPL_DATA
>          self.assertEqual(expect, data[-len(expect):])
>
> +    def testMkimageFilename(self):
> +        """Test using mkimage to build a binary with a filename"""
> +        retcode = self._DoTestFile('242_mkimage_filename.dts')
> +        self.assertEqual(0, retcode)
> +        fname = tools.get_output_filename('mkimage-test.bin')
> +        self.assertTrue(os.path.exists(fname))
> +

Please put the new test at the end.

>      def testCompressDtbPrependInvalid(self):
>          """Test that invalid header is detected"""
>          with self.assertRaises(ValueError) as e:
> diff --git a/tools/binman/test/242_mkimage_filename.dts b/tools/binman/test/242_mkimage_filename.dts
> new file mode 100644
> index 0000000000..4483790ae8
> --- /dev/null
> +++ b/tools/binman/test/242_mkimage_filename.dts
> @@ -0,0 +1,18 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +/dts-v1/;
> +
> +/ {
> +       #address-cells = <1>;
> +       #size-cells = <1>;
> +
> +       binman {
> +               mkimage {
> +                       filename = "mkimage-test.bin";
> +                       args = "-T script";
> +
> +                       u-boot-spl {
> +                       };
> +               };
> +       };
> +};
> --
> 2.37.2
>

Regards,
Simon

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

* Re: [PATCH v5 1/8] binman: add support for skipping file concatenation for mkimage
  2022-08-26 15:36 ` [PATCH v5 1/8] binman: add support for skipping file concatenation for mkimage Quentin Schulz
@ 2022-08-27  0:21   ` Simon Glass
  2022-08-30  9:57     ` Quentin Schulz
  0 siblings, 1 reply; 29+ messages in thread
From: Simon Glass @ 2022-08-27  0:21 UTC (permalink / raw)
  To: Quentin Schulz
  Cc: Bharat Gooty, Rayagonda Kokatanur, Philipp Tomsich, Kever Yang,
	Jagan Teki, Alper Nebi Yasak, Heinrich Schuchardt, Heiko Thiery,
	U-Boot Mailing List, Quentin Schulz

Hi Quentin,

On Fri, 26 Aug 2022 at 09:37, Quentin Schulz <foss+uboot@0leil.net> wrote:
>
> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>
> Some image types handled by mkimage require the datafiles to be passed
> independently (-d data1:data2) for specific handling of each. A
> concatenation of datafiles prior to passing them to mkimage wouldn't
> work.
>
> That is the case for rkspi for example which requires page alignment
> and only writing 2KB every 4KB.
>
> This adds the ability to tell binman to pass the datafiles without
> prior concatenation to mkimage, by adding the multiple-data-files
> boolean property to the mkimage node.
>
> Cc: Quentin Schulz <foss+uboot@0leil.net>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> ---
>
> v5:
>  - changed to use full path from input dir with tools.get_input_filename
>  to make it possible to run the unit tests,
>  - added unit test,
>
>
>  tools/binman/entries.rst                      | 22 ++++++++++
>  tools/binman/etype/mkimage.py                 | 41 +++++++++++++++++--
>  tools/binman/ftest.py                         | 16 ++++++++

Please put the new test at the end.

>  .../test/241_mkimage_multiple_data_files.dts  | 21 ++++++++++
>  4 files changed, 96 insertions(+), 4 deletions(-)
>  create mode 100644 tools/binman/test/241_mkimage_multiple_data_files.dts

This is pretty close but it still missing a line of test coverage.
Please try 'binman test -T' to see it. I'd also prefer a shorter
filename for the 241 file.

I've pushed a tree containing a suggested fix (updating this patch). I
can update it when applying if you like, otherwise please send a new
version.

Also note that the files have been renumbered, so the latest update is
at u-boot-dm/testing

Regards,
Simon

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

* Re: [PATCH v5 3/8] rockchip: remove binman temporary files when cleaning
  2022-08-26 15:36 ` [PATCH v5 3/8] rockchip: remove binman temporary files when cleaning Quentin Schulz
@ 2022-08-27  0:21   ` Simon Glass
  2022-09-01 12:04   ` Kever Yang
  1 sibling, 0 replies; 29+ messages in thread
From: Simon Glass @ 2022-08-27  0:21 UTC (permalink / raw)
  To: Quentin Schulz
  Cc: Bharat Gooty, Rayagonda Kokatanur, Philipp Tomsich, Kever Yang,
	Jagan Teki, Alper Nebi Yasak, Heinrich Schuchardt, Heiko Thiery,
	U-Boot Mailing List, Quentin Schulz, Johan Jonker

On Fri, 26 Aug 2022 at 09:37, Quentin Schulz <foss+uboot@0leil.net> wrote:
>
> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>
> Binman mkimage entry generates temporary files so let's remove them
> when calling `make clean`.
>
> Fixes: 9b312e26fc77 ("rockchip: Enable building a SPI ROM image on jerry")
> Cc: Quentin Schulz <foss+uboot@0leil.net>
> Reported-by: Johan Jonker <jbx6244@gmail.com>
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> ---
>
> added in v3
>
>  Makefile | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

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


>
> diff --git a/Makefile b/Makefile
> index 541e942ed5..5750a9e4b8 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2225,7 +2225,8 @@ CLEAN_FILES += include/bmp_logo.h include/bmp_logo_data.h \
>                lpc32xx-* bl31.c bl31.elf bl31_*.bin image.map tispl.bin* \
>                idbloader.img flash.bin flash.log defconfig keep-syms-lto.c \
>                mkimage-out.spl.mkimage mkimage.spl.mkimage imx-boot.map \
> -              itb.fit.fit itb.fit.itb itb.map spl.map
> +              itb.fit.fit itb.fit.itb itb.map spl.map mkimage-out.rom.mkimage \
> +              mkimage.rom.mkimage rom.map simple-bin.map
>
>  # Directories & files removed with 'make mrproper'
>  MRPROPER_DIRS  += include/config include/generated spl tpl \
> --
> 2.37.2
>

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

* Re: [PATCH v5 6/8] rockchip: simplify binman image dependencies addition to INPUTS
  2022-08-26 15:36 ` [PATCH v5 6/8] rockchip: simplify binman image dependencies addition to INPUTS Quentin Schulz
@ 2022-08-27  0:21   ` Simon Glass
  2022-09-01 12:08   ` Kever Yang
  1 sibling, 0 replies; 29+ messages in thread
From: Simon Glass @ 2022-08-27  0:21 UTC (permalink / raw)
  To: Quentin Schulz
  Cc: Bharat Gooty, Rayagonda Kokatanur, Philipp Tomsich, Kever Yang,
	Jagan Teki, Alper Nebi Yasak, Heinrich Schuchardt, Heiko Thiery,
	U-Boot Mailing List, Quentin Schulz

On Fri, 26 Aug 2022 at 09:37, Quentin Schulz <foss+uboot@0leil.net> wrote:
>
> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>
> By factoring SPL check in the first condition, this makes the checks a
> bit less convoluted and more readable.
>
> Cc: Quentin Schulz <foss+uboot@0leil.net>
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> ---
>
> v4:
>  - fixed wrong place for endif for ARM32 boards,
>
>  Makefile | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)

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


>
> diff --git a/Makefile b/Makefile
> index 1dee09eb36..736c4ad182 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1001,19 +1001,14 @@ ifeq ($(CONFIG_MPC85xx)$(CONFIG_OF_SEPARATE),yy)
>  INPUTS-y += u-boot-with-dtb.bin
>  endif
>
> -ifeq ($(CONFIG_ARCH_ROCKCHIP),y)
> -# On ARM64 this target is produced by binman so we don't need this dep
> +ifeq ($(CONFIG_ARCH_ROCKCHIP)$(CONFIG_SPL),yy)
> +# Binman image dependencies
>  ifeq ($(CONFIG_ARM64),y)
> -ifeq ($(CONFIG_SPL),y)
>  INPUTS-y += u-boot.itb
> -endif
>  else
> -ifeq ($(CONFIG_SPL),y)
> -# Generate these inputs for binman which will create the output files
>  INPUTS-y += u-boot.img
>  endif
>  endif
> -endif
>
>  INPUTS-$(CONFIG_X86) += u-boot-x86-start16.bin u-boot-x86-reset16.bin \
>         $(if $(CONFIG_SPL_X86_16BIT_INIT),spl/u-boot-spl.bin) \
> --
> 2.37.2
>

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

* Re: [PATCH v5 7/8] rockchip: allow to build SPI images even without HAS_ROM option
  2022-08-26 15:36 ` [PATCH v5 7/8] rockchip: allow to build SPI images even without HAS_ROM option Quentin Schulz
@ 2022-08-27  0:21   ` Simon Glass
  2022-09-01 12:08   ` Kever Yang
  1 sibling, 0 replies; 29+ messages in thread
From: Simon Glass @ 2022-08-27  0:21 UTC (permalink / raw)
  To: Quentin Schulz
  Cc: Bharat Gooty, Rayagonda Kokatanur, Philipp Tomsich, Kever Yang,
	Jagan Teki, Alper Nebi Yasak, Heinrich Schuchardt, Heiko Thiery,
	U-Boot Mailing List, Quentin Schulz

On Fri, 26 Aug 2022 at 09:37, Quentin Schulz <foss+uboot@0leil.net> wrote:
>
> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>
> This prepares for the creation of a u-boot-rockchip-spi.bin image
> similar to u-boot-rockchip.bin to the exception it's destined for
> SPI-NOR flashes instead of MMC storage medium.
>
> Cc: Quentin Schulz <foss+uboot@0leil.net>
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> ---
>  arch/arm/dts/rk3288-u-boot.dtsi | 2 +-
>  arch/arm/dts/rk3399-u-boot.dtsi | 2 +-
>  arch/arm/mach-rockchip/Kconfig  | 6 ++----
>  3 files changed, 4 insertions(+), 6 deletions(-)
>

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

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

* Re: [PATCH v5 8/8] rockchip: add u-boot-rockchip-spi.bin image for booting from SPI-NOR flash
  2022-08-26 15:36 ` [PATCH v5 8/8] rockchip: add u-boot-rockchip-spi.bin image for booting from SPI-NOR flash Quentin Schulz
@ 2022-08-27  0:21   ` Simon Glass
  2022-09-01 12:08   ` Kever Yang
  1 sibling, 0 replies; 29+ messages in thread
From: Simon Glass @ 2022-08-27  0:21 UTC (permalink / raw)
  To: Quentin Schulz
  Cc: Bharat Gooty, Rayagonda Kokatanur, Philipp Tomsich, Kever Yang,
	Jagan Teki, Alper Nebi Yasak, Heinrich Schuchardt, Heiko Thiery,
	U-Boot Mailing List, Quentin Schulz

On Fri, 26 Aug 2022 at 09:37, Quentin Schulz <foss+uboot@0leil.net> wrote:
>
> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>
> This new image is similar to u-boot-rockchip.bin except that it's
> destined to be flashed on SPI-NOR flashes.
>
> Cc: Quentin Schulz <foss+uboot@0leil.net>
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> ---
>
> v3:
>  - added filename property so that idblaoder-spi.img binary is generated
>  by binman, as per community request,
>  - added new temporary files to the list of files to clean up on `make
>  clean`,
>
>  Makefile                          |  3 ++-
>  arch/arm/dts/rockchip-u-boot.dtsi | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 32 insertions(+), 1 deletion(-)

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

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

* Re: [PATCH v5 1/8] binman: add support for skipping file concatenation for mkimage
  2022-08-27  0:21   ` Simon Glass
@ 2022-08-30  9:57     ` Quentin Schulz
  2022-08-30 15:56       ` Simon Glass
  0 siblings, 1 reply; 29+ messages in thread
From: Quentin Schulz @ 2022-08-30  9:57 UTC (permalink / raw)
  To: Simon Glass, Quentin Schulz
  Cc: Bharat Gooty, Rayagonda Kokatanur, Philipp Tomsich, Kever Yang,
	Jagan Teki, Alper Nebi Yasak, Heinrich Schuchardt, Heiko Thiery,
	U-Boot Mailing List

Hi Simon,

On 8/27/22 02:21, Simon Glass wrote:
> Hi Quentin,
> 
> On Fri, 26 Aug 2022 at 09:37, Quentin Schulz <foss+uboot@0leil.net> wrote:
>>
>> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>>
>> Some image types handled by mkimage require the datafiles to be passed
>> independently (-d data1:data2) for specific handling of each. A
>> concatenation of datafiles prior to passing them to mkimage wouldn't
>> work.
>>
>> That is the case for rkspi for example which requires page alignment
>> and only writing 2KB every 4KB.
>>
>> This adds the ability to tell binman to pass the datafiles without
>> prior concatenation to mkimage, by adding the multiple-data-files
>> boolean property to the mkimage node.
>>
>> Cc: Quentin Schulz <foss+uboot@0leil.net>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>> ---
>>
>> v5:
>>   - changed to use full path from input dir with tools.get_input_filename
>>   to make it possible to run the unit tests,
>>   - added unit test,
>>
>>
>>   tools/binman/entries.rst                      | 22 ++++++++++
>>   tools/binman/etype/mkimage.py                 | 41 +++++++++++++++++--
>>   tools/binman/ftest.py                         | 16 ++++++++
> 
> Please put the new test at the end.
> 
>>   .../test/241_mkimage_multiple_data_files.dts  | 21 ++++++++++
>>   4 files changed, 96 insertions(+), 4 deletions(-)
>>   create mode 100644 tools/binman/test/241_mkimage_multiple_data_files.dts
> 
> This is pretty close but it still missing a line of test coverage.
> Please try 'binman test -T' to see it. I'd also prefer a shorter

This does not work on Fedora.
1) there's no python3-coverage binary available,
2) After replacing python3-coverage with just coverage, the tests are 
stuck and never finish, (I have seen the patches to use COVERAGE 
environment variable so I guess the required changes might be tackled 
soon in master),

Any tip on how to identify which test is stuck except going through them 
one by one?

python3-coverage is also not available in the container image built from 
tools/docker/Dockerfile.

> filename for the 241 file.
> 
> I've pushed a tree containing a suggested fix (updating this patch). I
> can update it when applying if you like, otherwise please send a new
> version.
> 

Where did you push the tree?

Cheers,
Quentin

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

* Re: [PATCH v5 1/8] binman: add support for skipping file concatenation for mkimage
  2022-08-30  9:57     ` Quentin Schulz
@ 2022-08-30 15:56       ` Simon Glass
  2022-08-30 17:53         ` Quentin Schulz
  0 siblings, 1 reply; 29+ messages in thread
From: Simon Glass @ 2022-08-30 15:56 UTC (permalink / raw)
  To: Quentin Schulz
  Cc: Quentin Schulz, Bharat Gooty, Rayagonda Kokatanur,
	Philipp Tomsich, Kever Yang, Jagan Teki, Alper Nebi Yasak,
	Heinrich Schuchardt, Heiko Thiery, U-Boot Mailing List

Hi Quentin,

On Tue, 30 Aug 2022 at 03:57, Quentin Schulz
<quentin.schulz@theobroma-systems.com> wrote:
>
> Hi Simon,
>
> On 8/27/22 02:21, Simon Glass wrote:
> > Hi Quentin,
> >
> > On Fri, 26 Aug 2022 at 09:37, Quentin Schulz <foss+uboot@0leil.net> wrote:
> >>
> >> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> >>
> >> Some image types handled by mkimage require the datafiles to be passed
> >> independently (-d data1:data2) for specific handling of each. A
> >> concatenation of datafiles prior to passing them to mkimage wouldn't
> >> work.
> >>
> >> That is the case for rkspi for example which requires page alignment
> >> and only writing 2KB every 4KB.
> >>
> >> This adds the ability to tell binman to pass the datafiles without
> >> prior concatenation to mkimage, by adding the multiple-data-files
> >> boolean property to the mkimage node.
> >>
> >> Cc: Quentin Schulz <foss+uboot@0leil.net>
> >> Reviewed-by: Simon Glass <sjg@chromium.org>
> >> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> >> ---
> >>
> >> v5:
> >>   - changed to use full path from input dir with tools.get_input_filename
> >>   to make it possible to run the unit tests,
> >>   - added unit test,
> >>
> >>
> >>   tools/binman/entries.rst                      | 22 ++++++++++
> >>   tools/binman/etype/mkimage.py                 | 41 +++++++++++++++++--
> >>   tools/binman/ftest.py                         | 16 ++++++++
> >
> > Please put the new test at the end.
> >
> >>   .../test/241_mkimage_multiple_data_files.dts  | 21 ++++++++++
> >>   4 files changed, 96 insertions(+), 4 deletions(-)
> >>   create mode 100644 tools/binman/test/241_mkimage_multiple_data_files.dts
> >
> > This is pretty close but it still missing a line of test coverage.
> > Please try 'binman test -T' to see it. I'd also prefer a shorter
>
> This does not work on Fedora.
> 1) there's no python3-coverage binary available,
> 2) After replacing python3-coverage with just coverage, the tests are
> stuck and never finish, (I have seen the patches to use COVERAGE
> environment variable so I guess the required changes might be tackled
> soon in master),
>
> Any tip on how to identify which test is stuck except going through them
> one by one?

One way is to add comment blocks '''...''' across the ftest.py file,
using a binary chop to identify the problem.

Or, since tests are run in series, you could hack test_util to pass
verbose parameters when it runs the tests - see 'cmd =' in
run_test_coverage().

>
> python3-coverage is also not available in the container image built from
> tools/docker/Dockerfile.

does 'python3 -m coverage' work?

or this:

https://coverage.readthedocs.io/en/6.3.2/install.html

>
> > filename for the 241 file.
> >
> > I've pushed a tree containing a suggested fix (updating this patch). I
> > can update it when applying if you like, otherwise please send a new
> > version.
> >
>
> Where did you push the tree?

Sorry I forgot to mention that:

https://github.com/sjg20/u-boot/tree/try-rk4

Regards,
SImon

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

* Re: [PATCH v5 1/8] binman: add support for skipping file concatenation for mkimage
  2022-08-30 15:56       ` Simon Glass
@ 2022-08-30 17:53         ` Quentin Schulz
  2022-08-31  3:15           ` Simon Glass
  0 siblings, 1 reply; 29+ messages in thread
From: Quentin Schulz @ 2022-08-30 17:53 UTC (permalink / raw)
  To: Simon Glass
  Cc: Quentin Schulz, Bharat Gooty, Rayagonda Kokatanur,
	Philipp Tomsich, Kever Yang, Jagan Teki, Alper Nebi Yasak,
	Heinrich Schuchardt, Heiko Thiery, U-Boot Mailing List

Hi Simon,

On 8/30/22 17:56, Simon Glass wrote:
> Hi Quentin,
> 
> On Tue, 30 Aug 2022 at 03:57, Quentin Schulz
> <quentin.schulz@theobroma-systems.com> wrote:
>>
>> Hi Simon,
>>
>> On 8/27/22 02:21, Simon Glass wrote:
>>> Hi Quentin,
>>>
>>> On Fri, 26 Aug 2022 at 09:37, Quentin Schulz <foss+uboot@0leil.net> wrote:
>>>>
>>>> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>>>>
>>>> Some image types handled by mkimage require the datafiles to be passed
>>>> independently (-d data1:data2) for specific handling of each. A
>>>> concatenation of datafiles prior to passing them to mkimage wouldn't
>>>> work.
>>>>
>>>> That is the case for rkspi for example which requires page alignment
>>>> and only writing 2KB every 4KB.
>>>>
>>>> This adds the ability to tell binman to pass the datafiles without
>>>> prior concatenation to mkimage, by adding the multiple-data-files
>>>> boolean property to the mkimage node.
>>>>
>>>> Cc: Quentin Schulz <foss+uboot@0leil.net>
>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>>>> ---
>>>>
>>>> v5:
>>>>    - changed to use full path from input dir with tools.get_input_filename
>>>>    to make it possible to run the unit tests,
>>>>    - added unit test,
>>>>
>>>>
>>>>    tools/binman/entries.rst                      | 22 ++++++++++
>>>>    tools/binman/etype/mkimage.py                 | 41 +++++++++++++++++--
>>>>    tools/binman/ftest.py                         | 16 ++++++++
>>>
>>> Please put the new test at the end.
>>>
>>>>    .../test/241_mkimage_multiple_data_files.dts  | 21 ++++++++++
>>>>    4 files changed, 96 insertions(+), 4 deletions(-)
>>>>    create mode 100644 tools/binman/test/241_mkimage_multiple_data_files.dts
>>>
>>> This is pretty close but it still missing a line of test coverage.
>>> Please try 'binman test -T' to see it. I'd also prefer a shorter
>>
>> This does not work on Fedora.
>> 1) there's no python3-coverage binary available,
>> 2) After replacing python3-coverage with just coverage, the tests are
>> stuck and never finish, (I have seen the patches to use COVERAGE
>> environment variable so I guess the required changes might be tackled
>> soon in master),
>>
>> Any tip on how to identify which test is stuck except going through them
>> one by one?
> 
> One way is to add comment blocks '''...''' across the ftest.py file,
> using a binary chop to identify the problem.
> 
> Or, since tests are run in series, you could hack test_util to pass
> verbose parameters when it runs the tests - see 'cmd =' in
> run_test_coverage().
> 

I just commented out tests and found the following two are failing on my 
system:
testCompUtilVersions and testListBintools.

After digging a bit it seems that it is stuck here: 
https://source.denx.de/u-boot/u-boot/-/blob/master/tools/patman/command.py#L105 
for bzip2.

Furthermore:
bzip2 -V > /dev/null
bzip2 -V > /dev/null 2>&1
both get stuck which I assume is where the issue lies :)

bzip2 --help is just fine BTW.

I tested on a colleague's PC running Ubuntu 22.04.1, it works as 
intended. I guess I'll have to check if Fedora or Ubuntu has patches on 
top of bzip2 source code that triggers/patches this behavior.

>>
>> python3-coverage is also not available in the container image built from
>> tools/docker/Dockerfile.
> 
> does 'python3 -m coverage' work?
> 

diff --git a/tools/patman/test_util.py b/tools/patman/test_util.py
index 0f6d1aa902..eaa769a564 100644
--- a/tools/patman/test_util.py
+++ b/tools/patman/test_util.py
@@ -58,11 +58,11 @@ def run_test_coverage(prog, filter_fname, 
exclude_list, build_dir, required=None
      prefix = ''
      if build_dir:
          prefix = 'PYTHONPATH=$PYTHONPATH:%s/sandbox_spl/tools ' % 
build_dir
-    cmd = ('%spython3-coverage run '
+    cmd = ('%spython3 -m coverage run '
             '--omit "%s" %s %s %s -P1' % (prefix, ','.join(glob_list),
                                           prog, extra_args or '', 
test_cmd))
      os.system(cmd)
-    stdout = command.output('python3-coverage', 'report')
+    stdout = command.output('python3', '-m', 'coverage', 'report')
      lines = stdout.splitlines()
      if required:
          # Convert '/path/to/name.py' just the module name 'name'
@@ -81,7 +81,7 @@ def run_test_coverage(prog, filter_fname, 
exclude_list, build_dir, required=None
      print(coverage)
      if coverage != '100%':
          print(stdout)
-        print("To get a report in 'htmlcov/index.html', type: 
python3-coverage html")
+        print("To get a report in 'htmlcov/index.html', type: python3 
-m coverage html")
          print('Coverage error: %s, but should be 100%%' % coverage)
          ok = False
      if not ok:

works just fine for me.

Michal Suchánek seems to disagree with me on this one, see 
https://lore.kernel.org/u-boot/20220830101149.GM28810@kitsune.suse.cz/

> or this:
> 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__coverage.readthedocs.io_en_6.3.2_install.html&d=DwIBaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=AH6ijvL2fp8TDrFjgeja0AVykFkzBjRPDOAfg8m_eKMHJW7RzTVA1wMpGn7qBwOe&s=BLW968ZKOcdPWg0s4-4AlA_rqiJCCCKPjP-Y-Fux6oI&e=
> 
>>
>>> filename for the 241 file.
>>>
>>> I've pushed a tree containing a suggested fix (updating this patch). I
>>> can update it when applying if you like, otherwise please send a new
>>> version.
>>>
>>
>> Where did you push the tree?
> 
> Sorry I forgot to mention that:
> 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_sjg20_u-2Dboot_tree_try-2Drk4&d=DwIBaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=AH6ijvL2fp8TDrFjgeja0AVykFkzBjRPDOAfg8m_eKMHJW7RzTVA1wMpGn7qBwOe&s=7LOQoSkcQA52SvFgC_aUR4l2MtMWjdVM-t_bCKUetEs&e=
> 


I do not understand how you found out coverage was not happy about my 
patchset. I have the same percentage reported from your branch or my 
local one. What am I missing?

Regarding the content of the changed commits:
testMkimageMultipleNoContent is not testing what is says it does?
It's using multiple-data-files DT property which only impacts -d 
parameter of mkimage and the comment for the test is """Test using 
mkimage with -n and no data""".

What exactly are you trying to test?

Cheers,
Quentin

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

* Re: [PATCH v5 1/8] binman: add support for skipping file concatenation for mkimage
  2022-08-30 17:53         ` Quentin Schulz
@ 2022-08-31  3:15           ` Simon Glass
  2022-08-31  9:25             ` Quentin Schulz
  0 siblings, 1 reply; 29+ messages in thread
From: Simon Glass @ 2022-08-31  3:15 UTC (permalink / raw)
  To: Quentin Schulz
  Cc: Quentin Schulz, Bharat Gooty, Rayagonda Kokatanur,
	Philipp Tomsich, Kever Yang, Jagan Teki, Alper Nebi Yasak,
	Heinrich Schuchardt, Heiko Thiery, U-Boot Mailing List

Hi Quentin,

On Tue, 30 Aug 2022 at 11:54, Quentin Schulz
<quentin.schulz@theobroma-systems.com> wrote:
>
> Hi Simon,
>
> On 8/30/22 17:56, Simon Glass wrote:
> > Hi Quentin,
> >
> > On Tue, 30 Aug 2022 at 03:57, Quentin Schulz
> > <quentin.schulz@theobroma-systems.com> wrote:
> >>
> >> Hi Simon,
> >>
> >> On 8/27/22 02:21, Simon Glass wrote:
> >>> Hi Quentin,
> >>>
> >>> On Fri, 26 Aug 2022 at 09:37, Quentin Schulz <foss+uboot@0leil.net> wrote:
> >>>>
> >>>> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> >>>>
> >>>> Some image types handled by mkimage require the datafiles to be passed
> >>>> independently (-d data1:data2) for specific handling of each. A
> >>>> concatenation of datafiles prior to passing them to mkimage wouldn't
> >>>> work.
> >>>>
> >>>> That is the case for rkspi for example which requires page alignment
> >>>> and only writing 2KB every 4KB.
> >>>>
> >>>> This adds the ability to tell binman to pass the datafiles without
> >>>> prior concatenation to mkimage, by adding the multiple-data-files
> >>>> boolean property to the mkimage node.
> >>>>
> >>>> Cc: Quentin Schulz <foss+uboot@0leil.net>
> >>>> Reviewed-by: Simon Glass <sjg@chromium.org>
> >>>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> >>>> ---
> >>>>
> >>>> v5:
> >>>>    - changed to use full path from input dir with tools.get_input_filename
> >>>>    to make it possible to run the unit tests,
> >>>>    - added unit test,
> >>>>
> >>>>
> >>>>    tools/binman/entries.rst                      | 22 ++++++++++
> >>>>    tools/binman/etype/mkimage.py                 | 41 +++++++++++++++++--
> >>>>    tools/binman/ftest.py                         | 16 ++++++++
> >>>
> >>> Please put the new test at the end.
> >>>
> >>>>    .../test/241_mkimage_multiple_data_files.dts  | 21 ++++++++++
> >>>>    4 files changed, 96 insertions(+), 4 deletions(-)
> >>>>    create mode 100644 tools/binman/test/241_mkimage_multiple_data_files.dts
> >>>
> >>> This is pretty close but it still missing a line of test coverage.
> >>> Please try 'binman test -T' to see it. I'd also prefer a shorter
> >>
> >> This does not work on Fedora.
> >> 1) there's no python3-coverage binary available,
> >> 2) After replacing python3-coverage with just coverage, the tests are
> >> stuck and never finish, (I have seen the patches to use COVERAGE
> >> environment variable so I guess the required changes might be tackled
> >> soon in master),
> >>
> >> Any tip on how to identify which test is stuck except going through them
> >> one by one?
> >
> > One way is to add comment blocks '''...''' across the ftest.py file,
> > using a binary chop to identify the problem.
> >
> > Or, since tests are run in series, you could hack test_util to pass
> > verbose parameters when it runs the tests - see 'cmd =' in
> > run_test_coverage().
> >
>
> I just commented out tests and found the following two are failing on my
> system:
> testCompUtilVersions and testListBintools.
>
> After digging a bit it seems that it is stuck here:
> https://source.denx.de/u-boot/u-boot/-/blob/master/tools/patman/command.py#L105
> for bzip2.
>
> Furthermore:
> bzip2 -V > /dev/null
> bzip2 -V > /dev/null 2>&1

I wonder why that would hang. Can you try 'bzip2 -V' on the cmdline?

> both get stuck which I assume is where the issue lies :)
>
> bzip2 --help is just fine BTW.
>
> I tested on a colleague's PC running Ubuntu 22.04.1, it works as
> intended. I guess I'll have to check if Fedora or Ubuntu has patches on
> top of bzip2 source code that triggers/patches this behavior.

Very strange!

>
> >>
> >> python3-coverage is also not available in the container image built from
> >> tools/docker/Dockerfile.
> >
> > does 'python3 -m coverage' work?
> >
>
> diff --git a/tools/patman/test_util.py b/tools/patman/test_util.py
> index 0f6d1aa902..eaa769a564 100644
> --- a/tools/patman/test_util.py
> +++ b/tools/patman/test_util.py
> @@ -58,11 +58,11 @@ def run_test_coverage(prog, filter_fname,
> exclude_list, build_dir, required=None
>       prefix = ''
>       if build_dir:
>           prefix = 'PYTHONPATH=$PYTHONPATH:%s/sandbox_spl/tools ' %
> build_dir
> -    cmd = ('%spython3-coverage run '
> +    cmd = ('%spython3 -m coverage run '
>              '--omit "%s" %s %s %s -P1' % (prefix, ','.join(glob_list),
>                                            prog, extra_args or '',
> test_cmd))
>       os.system(cmd)
> -    stdout = command.output('python3-coverage', 'report')
> +    stdout = command.output('python3', '-m', 'coverage', 'report')
>       lines = stdout.splitlines()
>       if required:
>           # Convert '/path/to/name.py' just the module name 'name'
> @@ -81,7 +81,7 @@ def run_test_coverage(prog, filter_fname,
> exclude_list, build_dir, required=None
>       print(coverage)
>       if coverage != '100%':
>           print(stdout)
> -        print("To get a report in 'htmlcov/index.html', type:
> python3-coverage html")
> +        print("To get a report in 'htmlcov/index.html', type: python3
> -m coverage html")
>           print('Coverage error: %s, but should be 100%%' % coverage)
>           ok = False
>       if not ok:
>
> works just fine for me.
>
> Michal Suchánek seems to disagree with me on this one, see
> https://lore.kernel.org/u-boot/20220830101149.GM28810@kitsune.suse.cz/

I don't fully understand that point.

I think it is fine to specify the tool as an env var.

But if -m coverage works in general, let's use it. If not, we'll have
the env var.


>
> > or this:
> >
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__coverage.readthedocs.io_en_6.3.2_install.html&d=DwIBaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=AH6ijvL2fp8TDrFjgeja0AVykFkzBjRPDOAfg8m_eKMHJW7RzTVA1wMpGn7qBwOe&s=BLW968ZKOcdPWg0s4-4AlA_rqiJCCCKPjP-Y-Fux6oI&e=
> >
> >>
> >>> filename for the 241 file.
> >>>
> >>> I've pushed a tree containing a suggested fix (updating this patch). I
> >>> can update it when applying if you like, otherwise please send a new
> >>> version.
> >>>
> >>
> >> Where did you push the tree?
> >
> > Sorry I forgot to mention that:
> >
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_sjg20_u-2Dboot_tree_try-2Drk4&d=DwIBaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=AH6ijvL2fp8TDrFjgeja0AVykFkzBjRPDOAfg8m_eKMHJW7RzTVA1wMpGn7qBwOe&s=7LOQoSkcQA52SvFgC_aUR4l2MtMWjdVM-t_bCKUetEs&e=
> >
>
>
> I do not understand how you found out coverage was not happy about my
> patchset. I have the same percentage reported from your branch or my
> local one. What am I missing?
>
> Regarding the content of the changed commits:
> testMkimageMultipleNoContent is not testing what is says it does?
> It's using multiple-data-files DT property which only impacts -d
> parameter of mkimage and the comment for the test is """Test using
> mkimage with -n and no data""".
>
> What exactly are you trying to test?

'binman test -T'

I pushed your original patches to the try-rk4-orig branch. My changes
are in try-rk4.

With yours I see this:

======================== Running binman tests ========================
........................................................................................................................................................................................................................................................................................................................................................................................................................................................................
----------------------------------------------------------------------
Ran 456 tests in 19.669s

OK

99%
Name                                                    Stmts   Miss  Cover
---------------------------------------------------------------------------
tools/binman/__init__.py                                    0      0   100%
tools/binman/bintool.py                                   254      0   100%
tools/binman/btool/btool_gzip.py                            5      0   100%
tools/binman/btool/bzip2.py                                 5      0   100%
tools/binman/btool/cbfstool.py                             24      0   100%
tools/binman/btool/fiptool.py                              22      0   100%
tools/binman/btool/futility.py                             24      0   100%
tools/binman/btool/ifwitool.py                             22      0   100%
tools/binman/btool/lz4.py                                  28      0   100%
tools/binman/btool/lzma_alone.py                           34      0   100%
tools/binman/btool/lzop.py                                  5      0   100%
tools/binman/btool/mkimage.py                              29      0   100%
tools/binman/btool/xz.py                                    5      0   100%
tools/binman/btool/zstd.py                                  5      0   100%
tools/binman/cbfs_util.py                                 366      0   100%
tools/binman/cmdline.py                                    73      0   100%
tools/binman/control.py                                   342      0   100%
tools/binman/elf.py                                       195      0   100%
tools/binman/entry.py                                     483      0   100%
tools/binman/etype/atf_bl31.py                              5      0   100%
tools/binman/etype/atf_fip.py                              67      0   100%
tools/binman/etype/blob.py                                 39      0   100%
tools/binman/etype/blob_dtb.py                             46      0   100%
tools/binman/etype/blob_ext.py                             11      0   100%
tools/binman/etype/blob_ext_list.py                        32      0   100%
tools/binman/etype/blob_named_by_arg.py                     9      0   100%
tools/binman/etype/blob_phase.py                           16      0   100%
tools/binman/etype/cbfs.py                                101      0   100%
tools/binman/etype/collection.py                           30      0   100%
tools/binman/etype/cros_ec_rw.py                            5      0   100%
tools/binman/etype/fdtmap.py                               62      0   100%
tools/binman/etype/files.py                                35      0   100%
tools/binman/etype/fill.py                                 13      0   100%
tools/binman/etype/fit.py                                 214      0   100%
tools/binman/etype/fmap.py                                 34      0   100%
tools/binman/etype/gbb.py                                  37      0   100%
tools/binman/etype/image_header.py                         53      0   100%
tools/binman/etype/intel_cmc.py                             4      0   100%
tools/binman/etype/intel_descriptor.py                     39      0   100%
tools/binman/etype/intel_fit.py                            12      0   100%
tools/binman/etype/intel_fit_ptr.py                        17      0   100%
tools/binman/etype/intel_fsp.py                             4      0   100%
tools/binman/etype/intel_fsp_m.py                           4      0   100%
tools/binman/etype/intel_fsp_s.py                           4      0   100%
tools/binman/etype/intel_fsp_t.py                           4      0   100%
tools/binman/etype/intel_ifwi.py                           67      0   100%
tools/binman/etype/intel_me.py                              4      0   100%
tools/binman/etype/intel_mrc.py                             6      0   100%
tools/binman/etype/intel_refcode.py                         6      0   100%
tools/binman/etype/intel_vbt.py                             4      0   100%
tools/binman/etype/intel_vga.py                             4      0   100%
tools/binman/etype/mkimage.py                              80      1    99%
tools/binman/etype/opensbi.py                               5      0   100%
tools/binman/etype/powerpc_mpc85xx_bootpg_resetvec.py       6      0   100%
tools/binman/etype/pre_load.py                             77      0   100%
tools/binman/etype/scp.py                                   5      0   100%
tools/binman/etype/section.py                             376      0   100%
tools/binman/etype/tee_os.py                                5      0   100%
tools/binman/etype/text.py                                 21      0   100%
tools/binman/etype/u_boot.py                                7      0   100%
tools/binman/etype/u_boot_dtb.py                            9      0   100%
tools/binman/etype/u_boot_dtb_with_ucode.py                51      0   100%
tools/binman/etype/u_boot_elf.py                           19      0   100%
tools/binman/etype/u_boot_env.py                           27      0   100%
tools/binman/etype/u_boot_expanded.py                       4      0   100%
tools/binman/etype/u_boot_img.py                            7      0   100%
tools/binman/etype/u_boot_nodtb.py                          7      0   100%
tools/binman/etype/u_boot_spl.py                           11      0   100%
tools/binman/etype/u_boot_spl_bss_pad.py                   14      0   100%
tools/binman/etype/u_boot_spl_dtb.py                        9      0   100%
tools/binman/etype/u_boot_spl_elf.py                        7      0   100%
tools/binman/etype/u_boot_spl_expanded.py                  12      0   100%
tools/binman/etype/u_boot_spl_nodtb.py                     11      0   100%
tools/binman/etype/u_boot_spl_with_ucode_ptr.py             8      0   100%
tools/binman/etype/u_boot_tpl.py                           11      0   100%
tools/binman/etype/u_boot_tpl_bss_pad.py                   14      0   100%
tools/binman/etype/u_boot_tpl_dtb.py                        9      0   100%
tools/binman/etype/u_boot_tpl_dtb_with_ucode.py             8      0   100%
tools/binman/etype/u_boot_tpl_elf.py                        7      0   100%
tools/binman/etype/u_boot_tpl_expanded.py                  12      0   100%
tools/binman/etype/u_boot_tpl_nodtb.py                     11      0   100%
tools/binman/etype/u_boot_tpl_with_ucode_ptr.py            12      0   100%
tools/binman/etype/u_boot_ucode.py                         33      0   100%
tools/binman/etype/u_boot_with_ucode_ptr.py                42      0   100%
tools/binman/etype/vblock.py                               38      0   100%
tools/binman/etype/x86_reset16.py                           7      0   100%
tools/binman/etype/x86_reset16_spl.py                       7      0   100%
tools/binman/etype/x86_reset16_tpl.py                       7      0   100%
tools/binman/etype/x86_start16.py                           7      0   100%
tools/binman/etype/x86_start16_spl.py                       7      0   100%
tools/binman/etype/x86_start16_tpl.py                       7      0   100%
tools/binman/fip_util.py                                  202      0   100%
tools/binman/fmap_util.py                                  48      0   100%
tools/binman/image.py                                     164      0   100%
tools/binman/state.py                                     201      0   100%
---------------------------------------------------------------------------
TOTAL                                                    4541      1    99%

To get a report in 'htmlcov/index.html', type: python3-coverage html
Coverage error: 99%, but should be 100%
ValueError: Test coverage failure


It is only a tiny difference! Basically we need to support the
contents of an entry being unavailable, temporarily or permanently, so
I added a test for that.

Regards,
Simon

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

* Re: [PATCH v5 1/8] binman: add support for skipping file concatenation for mkimage
  2022-08-31  3:15           ` Simon Glass
@ 2022-08-31  9:25             ` Quentin Schulz
  2022-08-31 13:46               ` Simon Glass
  0 siblings, 1 reply; 29+ messages in thread
From: Quentin Schulz @ 2022-08-31  9:25 UTC (permalink / raw)
  To: Simon Glass
  Cc: Quentin Schulz, Bharat Gooty, Rayagonda Kokatanur,
	Philipp Tomsich, Kever Yang, Jagan Teki, Alper Nebi Yasak,
	Heinrich Schuchardt, Heiko Thiery, U-Boot Mailing List

Hi Simon,

On 8/31/22 05:15, Simon Glass wrote:
> Hi Quentin,
> 
> On Tue, 30 Aug 2022 at 11:54, Quentin Schulz
> <quentin.schulz@theobroma-systems.com> wrote:
>>
>> Hi Simon,
>>
>> On 8/30/22 17:56, Simon Glass wrote:
>>> Hi Quentin,
>>>
>>> On Tue, 30 Aug 2022 at 03:57, Quentin Schulz
>>> <quentin.schulz@theobroma-systems.com> wrote:
>>>>
>>>> Hi Simon,
>>>>
>>>> On 8/27/22 02:21, Simon Glass wrote:
>>>>> Hi Quentin,
>>>>>
>>>>> On Fri, 26 Aug 2022 at 09:37, Quentin Schulz <foss+uboot@0leil.net> wrote:
>>>>>>
>>>>>> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>>>>>>
>>>>>> Some image types handled by mkimage require the datafiles to be passed
>>>>>> independently (-d data1:data2) for specific handling of each. A
>>>>>> concatenation of datafiles prior to passing them to mkimage wouldn't
>>>>>> work.
>>>>>>
>>>>>> That is the case for rkspi for example which requires page alignment
>>>>>> and only writing 2KB every 4KB.
>>>>>>
>>>>>> This adds the ability to tell binman to pass the datafiles without
>>>>>> prior concatenation to mkimage, by adding the multiple-data-files
>>>>>> boolean property to the mkimage node.
>>>>>>
>>>>>> Cc: Quentin Schulz <foss+uboot@0leil.net>
>>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>>>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>>>>>> ---
>>>>>>
>>>>>> v5:
>>>>>>     - changed to use full path from input dir with tools.get_input_filename
>>>>>>     to make it possible to run the unit tests,
>>>>>>     - added unit test,
>>>>>>
>>>>>>
>>>>>>     tools/binman/entries.rst                      | 22 ++++++++++
>>>>>>     tools/binman/etype/mkimage.py                 | 41 +++++++++++++++++--
>>>>>>     tools/binman/ftest.py                         | 16 ++++++++
>>>>>
>>>>> Please put the new test at the end.
>>>>>
>>>>>>     .../test/241_mkimage_multiple_data_files.dts  | 21 ++++++++++
>>>>>>     4 files changed, 96 insertions(+), 4 deletions(-)
>>>>>>     create mode 100644 tools/binman/test/241_mkimage_multiple_data_files.dts
>>>>>
>>>>> This is pretty close but it still missing a line of test coverage.
>>>>> Please try 'binman test -T' to see it. I'd also prefer a shorter
>>>>
>>>> This does not work on Fedora.
>>>> 1) there's no python3-coverage binary available,
>>>> 2) After replacing python3-coverage with just coverage, the tests are
>>>> stuck and never finish, (I have seen the patches to use COVERAGE
>>>> environment variable so I guess the required changes might be tackled
>>>> soon in master),
>>>>
>>>> Any tip on how to identify which test is stuck except going through them
>>>> one by one?
>>>
>>> One way is to add comment blocks '''...''' across the ftest.py file,
>>> using a binary chop to identify the problem.
>>>
>>> Or, since tests are run in series, you could hack test_util to pass
>>> verbose parameters when it runs the tests - see 'cmd =' in
>>> run_test_coverage().
>>>
>>
>> I just commented out tests and found the following two are failing on my
>> system:
>> testCompUtilVersions and testListBintools.
>>
>> After digging a bit it seems that it is stuck here:
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__source.denx.de_u-2Dboot_u-2Dboot_-2D_blob_master_tools_patman_command.py-23L105&d=DwIFaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=MbdarH_QzLsQMF7mlCoDjHMwaiirTOGXbHZkHpb79AsDp1RElUfGn9NaYl4FQIJw&s=goCAUmylx43E8E8Yf9t6iJdmCGODogQVuiVjJ7qebwc&e=
>> for bzip2.
>>
>> Furthermore:
>> bzip2 -V > /dev/null
>> bzip2 -V > /dev/null 2>&1
> 
> I wonder why that would hang. Can you try 'bzip2 -V' on the cmdline?
> 
>> both get stuck which I assume is where the issue lies :)
>>
>> bzip2 --help is just fine BTW.
>>
>> I tested on a colleague's PC running Ubuntu 22.04.1, it works as
>> intended. I guess I'll have to check if Fedora or Ubuntu has patches on
>> top of bzip2 source code that triggers/patches this behavior.
> 
> Very strange!
> 

OK. Upstream "bug", see:
https://sourceware.org/git/?p=bzip2.git;a=blob;f=bzip2.c;h=1538faf73a8b311f53f0fe608347de761196de90;hb=HEAD#l1902

When you pass the -V or -L option, the program does not exit (unlike 
--help). I guess it tries to compress something from stdin and endlessly 
waits. I cannot explain why:
bzip2 -V
returns -1 directly
but
bzip2 -V > /dev/null
is stuck.

$ strace bzip2 -V > /dev/null
[...]
write(2, "bzip2, a block-sorting file comp"..., 540bzip2, a 
block-sorting file compressor.  Version 1.0.8, 13-Jul-2019.

    Copyright (C) 1996-2019 by Julian Seward.

    This program is free software; you can redistribute it and/or modify
    it under the terms set out in the LICENSE file, which is included
    in the bzip2 source distribution.

    This program is distributed in the hope that it will be useful,
    but WITHOUT ANY WARRANTY; without even the implied warranty of
    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
    LICENSE file for more details.

) = 540
ioctl(1, TCGETS, 0x7ffdddef4390)        = -1 ENOTTY (Inappropriate ioctl 
for device)
mmap(NULL, 3600384, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 
0) = 0x7ff9a1091000
mmap(NULL, 3600384, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 
0) = 0x7ff9a0d22000
mmap(NULL, 266240, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 
0) = 0x7ff9a163f000
newfstatat(0, "", {st_mode=S_IFCHR|0620, st_rdev=makedev(0x88, 0x4), 
...}, AT_EMPTY_PATH) = 0
read(0,

This is "fixed" in Ubuntu with:
https://git.launchpad.net/ubuntu/+source/bzip2/tree/debian/patches/20-legacy.patch?h=ubuntu/jammy

I suggest that we use bzip2 --help instead in binman. It does print the 
version name there so it should be just fine. I'll send a patch for 
binman and open a bug or something on bzip2 ML to find out what exactly 
they are trying to do (if it's on purpose for example).

>>
>>>>
>>>> python3-coverage is also not available in the container image built from
>>>> tools/docker/Dockerfile.
>>>
>>> does 'python3 -m coverage' work?
>>>
>>
>> diff --git a/tools/patman/test_util.py b/tools/patman/test_util.py
>> index 0f6d1aa902..eaa769a564 100644
>> --- a/tools/patman/test_util.py
>> +++ b/tools/patman/test_util.py
>> @@ -58,11 +58,11 @@ def run_test_coverage(prog, filter_fname,
>> exclude_list, build_dir, required=None
>>        prefix = ''
>>        if build_dir:
>>            prefix = 'PYTHONPATH=$PYTHONPATH:%s/sandbox_spl/tools ' %
>> build_dir
>> -    cmd = ('%spython3-coverage run '
>> +    cmd = ('%spython3 -m coverage run '
>>               '--omit "%s" %s %s %s -P1' % (prefix, ','.join(glob_list),
>>                                             prog, extra_args or '',
>> test_cmd))
>>        os.system(cmd)
>> -    stdout = command.output('python3-coverage', 'report')
>> +    stdout = command.output('python3', '-m', 'coverage', 'report')
>>        lines = stdout.splitlines()
>>        if required:
>>            # Convert '/path/to/name.py' just the module name 'name'
>> @@ -81,7 +81,7 @@ def run_test_coverage(prog, filter_fname,
>> exclude_list, build_dir, required=None
>>        print(coverage)
>>        if coverage != '100%':
>>            print(stdout)
>> -        print("To get a report in 'htmlcov/index.html', type:
>> python3-coverage html")
>> +        print("To get a report in 'htmlcov/index.html', type: python3
>> -m coverage html")
>>            print('Coverage error: %s, but should be 100%%' % coverage)
>>            ok = False
>>        if not ok:
>>
>> works just fine for me.
>>
>> Michal Suchánek seems to disagree with me on this one, see
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_u-2Dboot_20220830101149.GM28810-40kitsune.suse.cz_&d=DwIFaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=MbdarH_QzLsQMF7mlCoDjHMwaiirTOGXbHZkHpb79AsDp1RElUfGn9NaYl4FQIJw&s=PIpNEgfpEtiIeShj3dhklIwaomQemLRGI3wo8nKxsr8&e=
> 
> I don't fully understand that point.
> 
> I think it is fine to specify the tool as an env var.
> 
> But if -m coverage works in general, let's use it. If not, we'll have
> the env var.
> 

It works for me and I believe it is better. Installing coverage from pip 
will install a "coverage" binary. In essence, having a COVERAGE 
environment variable is fine, but having it set to python3-coverage by 
default means it works by default only on Debian/Ubuntu-based distros: 
see https://pkgs.org/search/?q=python3-coverage&on=files

Also, installing with pip, one can run python3 -m coverage without 
adding ~/.local/bin to PATH (unlike using python3-coverage or coverage).

I suggest we move this discussion to the patch from Michal :)

> 
>>
>>> or this:
>>>
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__coverage.readthedocs.io_en_6.3.2_install.html&d=DwIBaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=AH6ijvL2fp8TDrFjgeja0AVykFkzBjRPDOAfg8m_eKMHJW7RzTVA1wMpGn7qBwOe&s=BLW968ZKOcdPWg0s4-4AlA_rqiJCCCKPjP-Y-Fux6oI&e=
>>>
>>>>
>>>>> filename for the 241 file.
>>>>>
>>>>> I've pushed a tree containing a suggested fix (updating this patch). I
>>>>> can update it when applying if you like, otherwise please send a new
>>>>> version.
>>>>>
>>>>
>>>> Where did you push the tree?
>>>
>>> Sorry I forgot to mention that:
>>>
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_sjg20_u-2Dboot_tree_try-2Drk4&d=DwIBaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=AH6ijvL2fp8TDrFjgeja0AVykFkzBjRPDOAfg8m_eKMHJW7RzTVA1wMpGn7qBwOe&s=7LOQoSkcQA52SvFgC_aUR4l2MtMWjdVM-t_bCKUetEs&e=
>>>
>>
>>
>> I do not understand how you found out coverage was not happy about my
>> patchset. I have the same percentage reported from your branch or my
>> local one. What am I missing?
>>
>> Regarding the content of the changed commits:
>> testMkimageMultipleNoContent is not testing what is says it does?
>> It's using multiple-data-files DT property which only impacts -d
>> parameter of mkimage and the comment for the test is """Test using
>> mkimage with -n and no data""".
>>
>> What exactly are you trying to test?
> 
> 'binman test -T'
> 
> I pushed your original patches to the try-rk4-orig branch. My changes
> are in try-rk4.
> 
> With yours I see this:
> 
> ======================== Running binman tests ========================
> ........................................................................................................................................................................................................................................................................................................................................................................................................................................................................
> ----------------------------------------------------------------------
> Ran 456 tests in 19.669s
> 
> OK
> 
> 99%
> Name                                                    Stmts   Miss  Cover
> ---------------------------------------------------------------------------
> tools/binman/__init__.py                                    0      0   100%
> tools/binman/bintool.py                                   254      0   100%
> tools/binman/btool/btool_gzip.py                            5      0   100%
> tools/binman/btool/bzip2.py                                 5      0   100%
> tools/binman/btool/cbfstool.py                             24      0   100%
> tools/binman/btool/fiptool.py                              22      0   100%
> tools/binman/btool/futility.py                             24      0   100%
> tools/binman/btool/ifwitool.py                             22      0   100%
> tools/binman/btool/lz4.py                                  28      0   100%
> tools/binman/btool/lzma_alone.py                           34      0   100%
> tools/binman/btool/lzop.py                                  5      0   100%
> tools/binman/btool/mkimage.py                              29      0   100%
> tools/binman/btool/xz.py                                    5      0   100%
> tools/binman/btool/zstd.py                                  5      0   100%
> tools/binman/cbfs_util.py                                 366      0   100%
> tools/binman/cmdline.py                                    73      0   100%
> tools/binman/control.py                                   342      0   100%
> tools/binman/elf.py                                       195      0   100%
> tools/binman/entry.py                                     483      0   100%
> tools/binman/etype/atf_bl31.py                              5      0   100%
> tools/binman/etype/atf_fip.py                              67      0   100%
> tools/binman/etype/blob.py                                 39      0   100%
> tools/binman/etype/blob_dtb.py                             46      0   100%
> tools/binman/etype/blob_ext.py                             11      0   100%
> tools/binman/etype/blob_ext_list.py                        32      0   100%
> tools/binman/etype/blob_named_by_arg.py                     9      0   100%
> tools/binman/etype/blob_phase.py                           16      0   100%
> tools/binman/etype/cbfs.py                                101      0   100%
> tools/binman/etype/collection.py                           30      0   100%
> tools/binman/etype/cros_ec_rw.py                            5      0   100%
> tools/binman/etype/fdtmap.py                               62      0   100%
> tools/binman/etype/files.py                                35      0   100%
> tools/binman/etype/fill.py                                 13      0   100%
> tools/binman/etype/fit.py                                 214      0   100%
> tools/binman/etype/fmap.py                                 34      0   100%
> tools/binman/etype/gbb.py                                  37      0   100%
> tools/binman/etype/image_header.py                         53      0   100%
> tools/binman/etype/intel_cmc.py                             4      0   100%
> tools/binman/etype/intel_descriptor.py                     39      0   100%
> tools/binman/etype/intel_fit.py                            12      0   100%
> tools/binman/etype/intel_fit_ptr.py                        17      0   100%
> tools/binman/etype/intel_fsp.py                             4      0   100%
> tools/binman/etype/intel_fsp_m.py                           4      0   100%
> tools/binman/etype/intel_fsp_s.py                           4      0   100%
> tools/binman/etype/intel_fsp_t.py                           4      0   100%
> tools/binman/etype/intel_ifwi.py                           67      0   100%
> tools/binman/etype/intel_me.py                              4      0   100%
> tools/binman/etype/intel_mrc.py                             6      0   100%
> tools/binman/etype/intel_refcode.py                         6      0   100%
> tools/binman/etype/intel_vbt.py                             4      0   100%
> tools/binman/etype/intel_vga.py                             4      0   100%
> tools/binman/etype/mkimage.py                              80      1    99%
> tools/binman/etype/opensbi.py                               5      0   100%
> tools/binman/etype/powerpc_mpc85xx_bootpg_resetvec.py       6      0   100%
> tools/binman/etype/pre_load.py                             77      0   100%
> tools/binman/etype/scp.py                                   5      0   100%
> tools/binman/etype/section.py                             376      0   100%
> tools/binman/etype/tee_os.py                                5      0   100%
> tools/binman/etype/text.py                                 21      0   100%
> tools/binman/etype/u_boot.py                                7      0   100%
> tools/binman/etype/u_boot_dtb.py                            9      0   100%
> tools/binman/etype/u_boot_dtb_with_ucode.py                51      0   100%
> tools/binman/etype/u_boot_elf.py                           19      0   100%
> tools/binman/etype/u_boot_env.py                           27      0   100%
> tools/binman/etype/u_boot_expanded.py                       4      0   100%
> tools/binman/etype/u_boot_img.py                            7      0   100%
> tools/binman/etype/u_boot_nodtb.py                          7      0   100%
> tools/binman/etype/u_boot_spl.py                           11      0   100%
> tools/binman/etype/u_boot_spl_bss_pad.py                   14      0   100%
> tools/binman/etype/u_boot_spl_dtb.py                        9      0   100%
> tools/binman/etype/u_boot_spl_elf.py                        7      0   100%
> tools/binman/etype/u_boot_spl_expanded.py                  12      0   100%
> tools/binman/etype/u_boot_spl_nodtb.py                     11      0   100%
> tools/binman/etype/u_boot_spl_with_ucode_ptr.py             8      0   100%
> tools/binman/etype/u_boot_tpl.py                           11      0   100%
> tools/binman/etype/u_boot_tpl_bss_pad.py                   14      0   100%
> tools/binman/etype/u_boot_tpl_dtb.py                        9      0   100%
> tools/binman/etype/u_boot_tpl_dtb_with_ucode.py             8      0   100%
> tools/binman/etype/u_boot_tpl_elf.py                        7      0   100%
> tools/binman/etype/u_boot_tpl_expanded.py                  12      0   100%
> tools/binman/etype/u_boot_tpl_nodtb.py                     11      0   100%
> tools/binman/etype/u_boot_tpl_with_ucode_ptr.py            12      0   100%
> tools/binman/etype/u_boot_ucode.py                         33      0   100%
> tools/binman/etype/u_boot_with_ucode_ptr.py                42      0   100%
> tools/binman/etype/vblock.py                               38      0   100%
> tools/binman/etype/x86_reset16.py                           7      0   100%
> tools/binman/etype/x86_reset16_spl.py                       7      0   100%
> tools/binman/etype/x86_reset16_tpl.py                       7      0   100%
> tools/binman/etype/x86_start16.py                           7      0   100%
> tools/binman/etype/x86_start16_spl.py                       7      0   100%
> tools/binman/etype/x86_start16_tpl.py                       7      0   100%
> tools/binman/fip_util.py                                  202      0   100%
> tools/binman/fmap_util.py                                  48      0   100%
> tools/binman/image.py                                     164      0   100%
> tools/binman/state.py                                     201      0   100%
> ---------------------------------------------------------------------------
> TOTAL                                                    4541      1    99%
> 
> To get a report in 'htmlcov/index.html', type: python3-coverage html
> Coverage error: 99%, but should be 100%
> ValueError: Test coverage failure
> 

I get 52% coverage only with that exact same branch, something's 
definitely wrong here in my setup. And I **definitely** do not have 
tools/binman/etype/mkimage.py listed in there.... Mmmmmm.

> 
> It is only a tiny difference! Basically we need to support the
> contents of an entry being unavailable, temporarily or permanently, so
> I added a test for that.
> 

I'll play with binman until I manage to get a coverage percentage equal 
to yours.

Cheers,
Quentin

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

* Re: [PATCH v5 1/8] binman: add support for skipping file concatenation for mkimage
  2022-08-31  9:25             ` Quentin Schulz
@ 2022-08-31 13:46               ` Simon Glass
  2022-08-31 16:39                 ` Quentin Schulz
  0 siblings, 1 reply; 29+ messages in thread
From: Simon Glass @ 2022-08-31 13:46 UTC (permalink / raw)
  To: Quentin Schulz
  Cc: Quentin Schulz, Bharat Gooty, Rayagonda Kokatanur,
	Philipp Tomsich, Kever Yang, Jagan Teki, Alper Nebi Yasak,
	Heinrich Schuchardt, Heiko Thiery, U-Boot Mailing List

Hi Quentin,

On Wed, 31 Aug 2022 at 03:25, Quentin Schulz
<quentin.schulz@theobroma-systems.com> wrote:
>
> Hi Simon,
>
> On 8/31/22 05:15, Simon Glass wrote:
> > Hi Quentin,
> >
> > On Tue, 30 Aug 2022 at 11:54, Quentin Schulz
> > <quentin.schulz@theobroma-systems.com> wrote:
> >>
> >> Hi Simon,
> >>
> >> On 8/30/22 17:56, Simon Glass wrote:
> >>> Hi Quentin,
> >>>
> >>> On Tue, 30 Aug 2022 at 03:57, Quentin Schulz
> >>> <quentin.schulz@theobroma-systems.com> wrote:
> >>>>
> >>>> Hi Simon,
> >>>>
> >>>> On 8/27/22 02:21, Simon Glass wrote:
> >>>>> Hi Quentin,
> >>>>>
> >>>>> On Fri, 26 Aug 2022 at 09:37, Quentin Schulz <foss+uboot@0leil.net> wrote:
> >>>>>>
> >>>>>> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> >>>>>>
> >>>>>> Some image types handled by mkimage require the datafiles to be passed
> >>>>>> independently (-d data1:data2) for specific handling of each. A
> >>>>>> concatenation of datafiles prior to passing them to mkimage wouldn't
> >>>>>> work.
> >>>>>>
> >>>>>> That is the case for rkspi for example which requires page alignment
> >>>>>> and only writing 2KB every 4KB.
> >>>>>>
> >>>>>> This adds the ability to tell binman to pass the datafiles without
> >>>>>> prior concatenation to mkimage, by adding the multiple-data-files
> >>>>>> boolean property to the mkimage node.
> >>>>>>
> >>>>>> Cc: Quentin Schulz <foss+uboot@0leil.net>
> >>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
> >>>>>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> >>>>>> ---
> >>>>>>
> >>>>>> v5:
> >>>>>>     - changed to use full path from input dir with tools.get_input_filename
> >>>>>>     to make it possible to run the unit tests,
> >>>>>>     - added unit test,
> >>>>>>
> >>>>>>
> >>>>>>     tools/binman/entries.rst                      | 22 ++++++++++
> >>>>>>     tools/binman/etype/mkimage.py                 | 41 +++++++++++++++++--
> >>>>>>     tools/binman/ftest.py                         | 16 ++++++++
> >>>>>
> >>>>> Please put the new test at the end.
> >>>>>
> >>>>>>     .../test/241_mkimage_multiple_data_files.dts  | 21 ++++++++++
> >>>>>>     4 files changed, 96 insertions(+), 4 deletions(-)
> >>>>>>     create mode 100644 tools/binman/test/241_mkimage_multiple_data_files.dts
> >>>>>
> >>>>> This is pretty close but it still missing a line of test coverage.
> >>>>> Please try 'binman test -T' to see it. I'd also prefer a shorter
> >>>>
> >>>> This does not work on Fedora.
> >>>> 1) there's no python3-coverage binary available,
> >>>> 2) After replacing python3-coverage with just coverage, the tests are
> >>>> stuck and never finish, (I have seen the patches to use COVERAGE
> >>>> environment variable so I guess the required changes might be tackled
> >>>> soon in master),
> >>>>
> >>>> Any tip on how to identify which test is stuck except going through them
> >>>> one by one?
> >>>
> >>> One way is to add comment blocks '''...''' across the ftest.py file,
> >>> using a binary chop to identify the problem.
> >>>
> >>> Or, since tests are run in series, you could hack test_util to pass
> >>> verbose parameters when it runs the tests - see 'cmd =' in
> >>> run_test_coverage().
> >>>
> >>
> >> I just commented out tests and found the following two are failing on my
> >> system:
> >> testCompUtilVersions and testListBintools.
> >>
> >> After digging a bit it seems that it is stuck here:
> >> https://urldefense.proofpoint.com/v2/url?u=https-3A__source.denx.de_u-2Dboot_u-2Dboot_-2D_blob_master_tools_patman_command.py-23L105&d=DwIFaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=MbdarH_QzLsQMF7mlCoDjHMwaiirTOGXbHZkHpb79AsDp1RElUfGn9NaYl4FQIJw&s=goCAUmylx43E8E8Yf9t6iJdmCGODogQVuiVjJ7qebwc&e=
> >> for bzip2.
> >>
> >> Furthermore:
> >> bzip2 -V > /dev/null
> >> bzip2 -V > /dev/null 2>&1
> >
> > I wonder why that would hang. Can you try 'bzip2 -V' on the cmdline?
> >
> >> both get stuck which I assume is where the issue lies :)
> >>
> >> bzip2 --help is just fine BTW.
> >>
> >> I tested on a colleague's PC running Ubuntu 22.04.1, it works as
> >> intended. I guess I'll have to check if Fedora or Ubuntu has patches on
> >> top of bzip2 source code that triggers/patches this behavior.
> >
> > Very strange!
> >
>
> OK. Upstream "bug", see:
> https://sourceware.org/git/?p=bzip2.git;a=blob;f=bzip2.c;h=1538faf73a8b311f53f0fe608347de761196de90;hb=HEAD#l1902
>
> When you pass the -V or -L option, the program does not exit (unlike
> --help). I guess it tries to compress something from stdin and endlessly
> waits. I cannot explain why:
> bzip2 -V
> returns -1 directly
> but
> bzip2 -V > /dev/null
> is stuck.
>
> $ strace bzip2 -V > /dev/null
> [...]
> write(2, "bzip2, a block-sorting file comp"..., 540bzip2, a
> block-sorting file compressor.  Version 1.0.8, 13-Jul-2019.
>
>     Copyright (C) 1996-2019 by Julian Seward.
>
>     This program is free software; you can redistribute it and/or modify
>     it under the terms set out in the LICENSE file, which is included
>     in the bzip2 source distribution.
>
>     This program is distributed in the hope that it will be useful,
>     but WITHOUT ANY WARRANTY; without even the implied warranty of
>     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>     LICENSE file for more details.
>
> ) = 540
> ioctl(1, TCGETS, 0x7ffdddef4390)        = -1 ENOTTY (Inappropriate ioctl
> for device)
> mmap(NULL, 3600384, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
> 0) = 0x7ff9a1091000
> mmap(NULL, 3600384, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
> 0) = 0x7ff9a0d22000
> mmap(NULL, 266240, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
> 0) = 0x7ff9a163f000
> newfstatat(0, "", {st_mode=S_IFCHR|0620, st_rdev=makedev(0x88, 0x4),
> ...}, AT_EMPTY_PATH) = 0
> read(0,
>
> This is "fixed" in Ubuntu with:
> https://git.launchpad.net/ubuntu/+source/bzip2/tree/debian/patches/20-legacy.patch?h=ubuntu/jammy
>
> I suggest that we use bzip2 --help instead in binman. It does print the
> version name there so it should be just fine. I'll send a patch for
> binman and open a bug or something on bzip2 ML to find out what exactly
> they are trying to do (if it's on purpose for example).
>
> >>
> >>>>
> >>>> python3-coverage is also not available in the container image built from
> >>>> tools/docker/Dockerfile.
> >>>
> >>> does 'python3 -m coverage' work?
> >>>
> >>
> >> diff --git a/tools/patman/test_util.py b/tools/patman/test_util.py
> >> index 0f6d1aa902..eaa769a564 100644
> >> --- a/tools/patman/test_util.py
> >> +++ b/tools/patman/test_util.py
> >> @@ -58,11 +58,11 @@ def run_test_coverage(prog, filter_fname,
> >> exclude_list, build_dir, required=None
> >>        prefix = ''
> >>        if build_dir:
> >>            prefix = 'PYTHONPATH=$PYTHONPATH:%s/sandbox_spl/tools ' %
> >> build_dir
> >> -    cmd = ('%spython3-coverage run '
> >> +    cmd = ('%spython3 -m coverage run '
> >>               '--omit "%s" %s %s %s -P1' % (prefix, ','.join(glob_list),
> >>                                             prog, extra_args or '',
> >> test_cmd))
> >>        os.system(cmd)
> >> -    stdout = command.output('python3-coverage', 'report')
> >> +    stdout = command.output('python3', '-m', 'coverage', 'report')
> >>        lines = stdout.splitlines()
> >>        if required:
> >>            # Convert '/path/to/name.py' just the module name 'name'
> >> @@ -81,7 +81,7 @@ def run_test_coverage(prog, filter_fname,
> >> exclude_list, build_dir, required=None
> >>        print(coverage)
> >>        if coverage != '100%':
> >>            print(stdout)
> >> -        print("To get a report in 'htmlcov/index.html', type:
> >> python3-coverage html")
> >> +        print("To get a report in 'htmlcov/index.html', type: python3
> >> -m coverage html")
> >>            print('Coverage error: %s, but should be 100%%' % coverage)
> >>            ok = False
> >>        if not ok:
> >>
> >> works just fine for me.
> >>
> >> Michal Suchánek seems to disagree with me on this one, see
> >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_u-2Dboot_20220830101149.GM28810-40kitsune.suse.cz_&d=DwIFaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=MbdarH_QzLsQMF7mlCoDjHMwaiirTOGXbHZkHpb79AsDp1RElUfGn9NaYl4FQIJw&s=PIpNEgfpEtiIeShj3dhklIwaomQemLRGI3wo8nKxsr8&e=
> >
> > I don't fully understand that point.
> >
> > I think it is fine to specify the tool as an env var.
> >
> > But if -m coverage works in general, let's use it. If not, we'll have
> > the env var.
> >
>
> It works for me and I believe it is better. Installing coverage from pip
> will install a "coverage" binary. In essence, having a COVERAGE
> environment variable is fine, but having it set to python3-coverage by
> default means it works by default only on Debian/Ubuntu-based distros:
> see https://pkgs.org/search/?q=python3-coverage&on=files
>
> Also, installing with pip, one can run python3 -m coverage without
> adding ~/.local/bin to PATH (unlike using python3-coverage or coverage).
>
> I suggest we move this discussion to the patch from Michal :)
>
> >
> >>
> >>> or this:
> >>>
> >>> https://urldefense.proofpoint.com/v2/url?u=https-3A__coverage.readthedocs.io_en_6.3.2_install.html&d=DwIBaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=AH6ijvL2fp8TDrFjgeja0AVykFkzBjRPDOAfg8m_eKMHJW7RzTVA1wMpGn7qBwOe&s=BLW968ZKOcdPWg0s4-4AlA_rqiJCCCKPjP-Y-Fux6oI&e=
> >>>
> >>>>
> >>>>> filename for the 241 file.
> >>>>>
> >>>>> I've pushed a tree containing a suggested fix (updating this patch). I
> >>>>> can update it when applying if you like, otherwise please send a new
> >>>>> version.
> >>>>>
> >>>>
> >>>> Where did you push the tree?
> >>>
> >>> Sorry I forgot to mention that:
> >>>
> >>> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_sjg20_u-2Dboot_tree_try-2Drk4&d=DwIBaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=AH6ijvL2fp8TDrFjgeja0AVykFkzBjRPDOAfg8m_eKMHJW7RzTVA1wMpGn7qBwOe&s=7LOQoSkcQA52SvFgC_aUR4l2MtMWjdVM-t_bCKUetEs&e=
> >>>
> >>
> >>
> >> I do not understand how you found out coverage was not happy about my
> >> patchset. I have the same percentage reported from your branch or my
> >> local one. What am I missing?
> >>
> >> Regarding the content of the changed commits:
> >> testMkimageMultipleNoContent is not testing what is says it does?
> >> It's using multiple-data-files DT property which only impacts -d
> >> parameter of mkimage and the comment for the test is """Test using
> >> mkimage with -n and no data""".
> >>
> >> What exactly are you trying to test?
> >
> > 'binman test -T'
> >
> > I pushed your original patches to the try-rk4-orig branch. My changes
> > are in try-rk4.
> >
> > With yours I see this:
> >
> > ======================== Running binman tests ========================
> > ........................................................................................................................................................................................................................................................................................................................................................................................................................................................................
> > ----------------------------------------------------------------------
> > Ran 456 tests in 19.669s
> >
> > OK
> >
> > 99%
> > Name                                                    Stmts   Miss  Cover
> > ---------------------------------------------------------------------------
> > tools/binman/__init__.py                                    0      0   100%
> > tools/binman/bintool.py                                   254      0   100%
> > tools/binman/btool/btool_gzip.py                            5      0   100%
> > tools/binman/btool/bzip2.py                                 5      0   100%
> > tools/binman/btool/cbfstool.py                             24      0   100%
> > tools/binman/btool/fiptool.py                              22      0   100%
> > tools/binman/btool/futility.py                             24      0   100%
> > tools/binman/btool/ifwitool.py                             22      0   100%
> > tools/binman/btool/lz4.py                                  28      0   100%
> > tools/binman/btool/lzma_alone.py                           34      0   100%
> > tools/binman/btool/lzop.py                                  5      0   100%
> > tools/binman/btool/mkimage.py                              29      0   100%
> > tools/binman/btool/xz.py                                    5      0   100%
> > tools/binman/btool/zstd.py                                  5      0   100%
> > tools/binman/cbfs_util.py                                 366      0   100%
> > tools/binman/cmdline.py                                    73      0   100%
> > tools/binman/control.py                                   342      0   100%
> > tools/binman/elf.py                                       195      0   100%
> > tools/binman/entry.py                                     483      0   100%
> > tools/binman/etype/atf_bl31.py                              5      0   100%
> > tools/binman/etype/atf_fip.py                              67      0   100%
> > tools/binman/etype/blob.py                                 39      0   100%
> > tools/binman/etype/blob_dtb.py                             46      0   100%
> > tools/binman/etype/blob_ext.py                             11      0   100%
> > tools/binman/etype/blob_ext_list.py                        32      0   100%
> > tools/binman/etype/blob_named_by_arg.py                     9      0   100%
> > tools/binman/etype/blob_phase.py                           16      0   100%
> > tools/binman/etype/cbfs.py                                101      0   100%
> > tools/binman/etype/collection.py                           30      0   100%
> > tools/binman/etype/cros_ec_rw.py                            5      0   100%
> > tools/binman/etype/fdtmap.py                               62      0   100%
> > tools/binman/etype/files.py                                35      0   100%
> > tools/binman/etype/fill.py                                 13      0   100%
> > tools/binman/etype/fit.py                                 214      0   100%
> > tools/binman/etype/fmap.py                                 34      0   100%
> > tools/binman/etype/gbb.py                                  37      0   100%
> > tools/binman/etype/image_header.py                         53      0   100%
> > tools/binman/etype/intel_cmc.py                             4      0   100%
> > tools/binman/etype/intel_descriptor.py                     39      0   100%
> > tools/binman/etype/intel_fit.py                            12      0   100%
> > tools/binman/etype/intel_fit_ptr.py                        17      0   100%
> > tools/binman/etype/intel_fsp.py                             4      0   100%
> > tools/binman/etype/intel_fsp_m.py                           4      0   100%
> > tools/binman/etype/intel_fsp_s.py                           4      0   100%
> > tools/binman/etype/intel_fsp_t.py                           4      0   100%
> > tools/binman/etype/intel_ifwi.py                           67      0   100%
> > tools/binman/etype/intel_me.py                              4      0   100%
> > tools/binman/etype/intel_mrc.py                             6      0   100%
> > tools/binman/etype/intel_refcode.py                         6      0   100%
> > tools/binman/etype/intel_vbt.py                             4      0   100%
> > tools/binman/etype/intel_vga.py                             4      0   100%
> > tools/binman/etype/mkimage.py                              80      1    99%
> > tools/binman/etype/opensbi.py                               5      0   100%
> > tools/binman/etype/powerpc_mpc85xx_bootpg_resetvec.py       6      0   100%
> > tools/binman/etype/pre_load.py                             77      0   100%
> > tools/binman/etype/scp.py                                   5      0   100%
> > tools/binman/etype/section.py                             376      0   100%
> > tools/binman/etype/tee_os.py                                5      0   100%
> > tools/binman/etype/text.py                                 21      0   100%
> > tools/binman/etype/u_boot.py                                7      0   100%
> > tools/binman/etype/u_boot_dtb.py                            9      0   100%
> > tools/binman/etype/u_boot_dtb_with_ucode.py                51      0   100%
> > tools/binman/etype/u_boot_elf.py                           19      0   100%
> > tools/binman/etype/u_boot_env.py                           27      0   100%
> > tools/binman/etype/u_boot_expanded.py                       4      0   100%
> > tools/binman/etype/u_boot_img.py                            7      0   100%
> > tools/binman/etype/u_boot_nodtb.py                          7      0   100%
> > tools/binman/etype/u_boot_spl.py                           11      0   100%
> > tools/binman/etype/u_boot_spl_bss_pad.py                   14      0   100%
> > tools/binman/etype/u_boot_spl_dtb.py                        9      0   100%
> > tools/binman/etype/u_boot_spl_elf.py                        7      0   100%
> > tools/binman/etype/u_boot_spl_expanded.py                  12      0   100%
> > tools/binman/etype/u_boot_spl_nodtb.py                     11      0   100%
> > tools/binman/etype/u_boot_spl_with_ucode_ptr.py             8      0   100%
> > tools/binman/etype/u_boot_tpl.py                           11      0   100%
> > tools/binman/etype/u_boot_tpl_bss_pad.py                   14      0   100%
> > tools/binman/etype/u_boot_tpl_dtb.py                        9      0   100%
> > tools/binman/etype/u_boot_tpl_dtb_with_ucode.py             8      0   100%
> > tools/binman/etype/u_boot_tpl_elf.py                        7      0   100%
> > tools/binman/etype/u_boot_tpl_expanded.py                  12      0   100%
> > tools/binman/etype/u_boot_tpl_nodtb.py                     11      0   100%
> > tools/binman/etype/u_boot_tpl_with_ucode_ptr.py            12      0   100%
> > tools/binman/etype/u_boot_ucode.py                         33      0   100%
> > tools/binman/etype/u_boot_with_ucode_ptr.py                42      0   100%
> > tools/binman/etype/vblock.py                               38      0   100%
> > tools/binman/etype/x86_reset16.py                           7      0   100%
> > tools/binman/etype/x86_reset16_spl.py                       7      0   100%
> > tools/binman/etype/x86_reset16_tpl.py                       7      0   100%
> > tools/binman/etype/x86_start16.py                           7      0   100%
> > tools/binman/etype/x86_start16_spl.py                       7      0   100%
> > tools/binman/etype/x86_start16_tpl.py                       7      0   100%
> > tools/binman/fip_util.py                                  202      0   100%
> > tools/binman/fmap_util.py                                  48      0   100%
> > tools/binman/image.py                                     164      0   100%
> > tools/binman/state.py                                     201      0   100%
> > ---------------------------------------------------------------------------
> > TOTAL                                                    4541      1    99%
> >
> > To get a report in 'htmlcov/index.html', type: python3-coverage html
> > Coverage error: 99%, but should be 100%
> > ValueError: Test coverage failure
> >
>
> I get 52% coverage only with that exact same branch, something's
> definitely wrong here in my setup. And I **definitely** do not have
> tools/binman/etype/mkimage.py listed in there.... Mmmmmm.

You may need to get some bintools with 'binman tool -f missing'. But
in any case, you only need to worry about coverage in mkimage.py which
is what you changed.

>
> >
> > It is only a tiny difference! Basically we need to support the
> > contents of an entry being unavailable, temporarily or permanently, so
> > I added a test for that.
> >
>
> I'll play with binman until I manage to get a coverage percentage equal
> to yours.

OK, I'd appreciate a docs patch if you can produce one from your
efforts, or any feedback on how to make this automatic / easy.

Regards,
Simon

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

* Re: [PATCH v5 1/8] binman: add support for skipping file concatenation for mkimage
  2022-08-31 13:46               ` Simon Glass
@ 2022-08-31 16:39                 ` Quentin Schulz
  2022-08-31 17:44                   ` Simon Glass
  0 siblings, 1 reply; 29+ messages in thread
From: Quentin Schulz @ 2022-08-31 16:39 UTC (permalink / raw)
  To: Simon Glass
  Cc: Quentin Schulz, Bharat Gooty, Rayagonda Kokatanur,
	Philipp Tomsich, Kever Yang, Jagan Teki, Alper Nebi Yasak,
	Heinrich Schuchardt, Heiko Thiery, U-Boot Mailing List

Hi Simon,

On 8/31/22 15:46, Simon Glass wrote:
> Hi Quentin,
> 
> On Wed, 31 Aug 2022 at 03:25, Quentin Schulz
> <quentin.schulz@theobroma-systems.com> wrote:
>>
>> Hi Simon,
>>
>> On 8/31/22 05:15, Simon Glass wrote:
>>> Hi Quentin,
>>>
>>> On Tue, 30 Aug 2022 at 11:54, Quentin Schulz
>>> <quentin.schulz@theobroma-systems.com> wrote:
>>>>
>>>> Hi Simon,
>>>>
>>>> On 8/30/22 17:56, Simon Glass wrote:
>>>>> Hi Quentin,
>>>>>
>>>>> On Tue, 30 Aug 2022 at 03:57, Quentin Schulz
>>>>> <quentin.schulz@theobroma-systems.com> wrote:
[...]
>>>> I do not understand how you found out coverage was not happy about my
>>>> patchset. I have the same percentage reported from your branch or my
>>>> local one. What am I missing?
>>>>
>>>> Regarding the content of the changed commits:
>>>> testMkimageMultipleNoContent is not testing what is says it does?
>>>> It's using multiple-data-files DT property which only impacts -d
>>>> parameter of mkimage and the comment for the test is """Test using
>>>> mkimage with -n and no data""".
>>>>
>>>> What exactly are you trying to test?
>>>
>>> 'binman test -T'
>>>
>>> I pushed your original patches to the try-rk4-orig branch. My changes
>>> are in try-rk4.
>>>

I would change 
https://github.com/sjg20/u-boot/blob/try-rk4/tools/binman/ftest.py#L5917
from
"""Test using mkimage with -n and no data"""
to
"""Test passing multiple data files to mkimage with one data file having 
no content"""
or something like that. Do you want me to send a v6 for this?

Otherwise, looks fine to me :)

With the bzip2 patch I just sent, I could see the line missing from unit 
tests.

However, unit tests aren't happy on my PC.

$ tools/binman/binman tool --list
Name             Version      Description                Path
---------------  -----------  ------------------------- 
------------------------------
gzip             1.11         gzip compression           /usr/bin/gzip
bzip2            1.0.8        bzip2 compression          /usr/bin/bzip2
cbfstool         unknown      Manipulate CBFS files 
/home/qschulz/work/upstream/coreboot/util/cbfstool/cbfstool
fiptool          Unknown vers Manipulate ATF FIP files 
/home/qschulz/work/upstream/trusted-firmware-a/tools/fiptool/fiptool
futility         v0.0.3050-18 Chromium OS firmware utili 
/home/qschulz/work/upstream/coreboot/util/futility/futility
ifwitool         unknown      Manipulate Intel IFWI file 
/home/qschulz/work/upstream/coreboot/util/cbfstool/ifwitool
lz4              v1.9.3       lz4 compression            /usr/bin/lz4
lzma_alone       -            lzma_alone compression     (not found)
lzop             v1.04        lzo compression            /usr/bin/lzop
mkimage          2022.10-rc3- Generate image for U-Boot  /usr/bin/mkimage
xz               5.2.5        xz compression             /usr/bin/xz
zstd             v1.5.2       zstd compression           /usr/bin/zstd
$ tools/binman/binman test -T
======================== Running binman tests ========================
.........................................................sss.............................................F....Fs..............................................................................................F.................................................................................................................................................EEEEFF.................................FF.......FF............F................F.FFFF.................
======================================================================
ERROR: testSymbols (binman.ftest.TestFunctional)
Test binman can assign symbols embedded in U-Boot
----------------------------------------------------------------------
KeyError: '_binman_sym_magic'

======================================================================
ERROR: testSymbolsExpanded (binman.ftest.TestFunctional)
Test binman can assign symbols in expanded entries
----------------------------------------------------------------------
KeyError: '_binman_sym_magic'

======================================================================
ERROR: testSymbolsNoDtb (binman.ftest.TestFunctional)
Test binman can assign symbols embedded in U-Boot SPL
----------------------------------------------------------------------
KeyError: '_binman_sym_magic'

======================================================================
ERROR: testSymbolsSubsection (binman.ftest.TestFunctional)
Test binman can assign symbols from a subsection
----------------------------------------------------------------------
KeyError: '_binman_sym_magic'

======================================================================
FAIL: testExtractAllEntries (binman.ftest.TestFunctional)
Test extracting all entries
----------------------------------------------------------------------
AssertionError: 969 != 0

======================================================================
FAIL: testExtractCbfsCompressed (binman.ftest.TestFunctional)
Test extracting CBFS compressed data
----------------------------------------------------------------------
AssertionError: 969 != 0

======================================================================
FAIL: testListCmd (binman.ftest.TestFunctional)
Test listing the files in an image using an Fdtmap
----------------------------------------------------------------------
AssertionError: Lists differ: ['Nam[463 chars]80   105  u-boot-dtb 
  80          3c9', [184 chars]bf8'] != ['Nam[463 chars]80     0 
u-boot-dtb        80          3c9', [184 chars]bf8']

First differing element 7:
'      u-boot-dtb        180   105  u-boot-dtb        80          3c9'
'      u-boot-dtb        180     0  u-boot-dtb        80          3c9'

Diff is 888 characters long. Set self.maxDiff to None to see it.

======================================================================
FAIL: testSymbolsTplSection (binman.ftest.TestFunctional)
Test binman can assign symbols embedded in U-Boot TPL in a section
----------------------------------------------------------------------
AssertionError: b'\xff\xff\xff\xffBSYM\x04\x00\x00\x00 
\x00\x00\x00\x00\x00[36 chars]0klm' != 
b'\xff\xff\xff\xff56780123456789abcdefghijklm'

======================================================================
FAIL: testSymbolsTplSectionX86 (binman.ftest.TestFunctional)
Test binman can assign symbols in a section with end-at-4gb
----------------------------------------------------------------------
AssertionError: b'\xff\xff\xff\xffBSYM\x04\xff\xff\xff 
\xff\xff\xff\x00\x00[36 chars]0klm' != 
b'\xff\xff\xff\xff56780123456789abcdefghijklm'

======================================================================
FAIL: testBadSymbolSize (binman.elf_test.TestElf)
Test that an attempt to use an 8-bit symbol are detected
----------------------------------------------------------------------
AssertionError: ValueError not raised

======================================================================
FAIL: testDebug (binman.elf_test.TestElf)
Check that enabling debug in the elf module produced debug output
----------------------------------------------------------------------
AssertionError: False is not true

======================================================================
FAIL: testNoValue (binman.elf_test.TestElf)
Test the case where we have no value for the symbol
----------------------------------------------------------------------
AssertionError: b'BSYM\xff\xff\xff\xff\xff\xff\xff\xff\xff\[43 
chars]aaaa' != b'aaaaaaaaaaaaaaaaaaaaaaaaaaaa'

======================================================================
FAIL: testOutsideFile (binman.elf_test.TestElf)
Test a symbol which extends outside the entry area is detected
----------------------------------------------------------------------
AssertionError: ValueError not raised

======================================================================
FAIL: test_cbfs_arch (binman.cbfs_util_test.TestCbfs)
Test on non-x86 architecture
----------------------------------------------------------------------
AssertionError: cbfstool produced a different result

Stdout:
diff -y <(xxd -g1 /tmp/expect) <(xxd -g1 /tmp/actual) | colordiff

======================================================================
FAIL: test_cbfs_offset (binman.cbfs_util_test.TestCbfs)
Test a CBFS with files at particular offsets
----------------------------------------------------------------------
AssertionError: cbfstool produced a different result

Stdout:
diff -y <(xxd -g1 /tmp/expect) <(xxd -g1 /tmp/actual) | colordiff

======================================================================
FAIL: test_cbfs_raw (binman.cbfs_util_test.TestCbfs)
Test base handling of a Coreboot Filesystem (CBFS)
----------------------------------------------------------------------
AssertionError: cbfstool produced a different result

Stdout:
diff -y <(xxd -g1 /tmp/expect) <(xxd -g1 /tmp/actual) | colordiff

======================================================================
FAIL: test_cbfs_raw_compress (binman.cbfs_util_test.TestCbfs)
Test base handling of compressing raw files
----------------------------------------------------------------------
AssertionError: b'' != b'compress xxxxxxxxxxxxxxxxxxxxxx data'

======================================================================
FAIL: test_cbfs_raw_space (binman.cbfs_util_test.TestCbfs)
Test files with unused space in the CBFS
----------------------------------------------------------------------
AssertionError: cbfstool produced a different result

Stdout:
diff -y <(xxd -g1 /tmp/expect) <(xxd -g1 /tmp/actual) | colordiff

======================================================================
FAIL: test_cbfs_stage (binman.cbfs_util_test.TestCbfs)
Tests handling of a Coreboot Filesystem (CBFS)
----------------------------------------------------------------------
AssertionError: cbfstool produced a different result

Stdout:
diff -y <(xxd -g1 /tmp/expect) <(xxd -g1 /tmp/actual) | colordiff

======================================================================
SKIP: testCompUtilCompressions (binman.ftest.TestFunctional)
Test compression algorithms
----------------------------------------------------------------------
lzma_alone not available

======================================================================
SKIP: testCompUtilPadding (binman.ftest.TestFunctional)
Test padding of compression algorithms
----------------------------------------------------------------------
lzma_alone not available

======================================================================
SKIP: testCompUtilVersions (binman.ftest.TestFunctional)
Test tool version of compression algorithms
----------------------------------------------------------------------
lzma_alone not available

======================================================================
SKIP: testExtractCbfsRaw (binman.ftest.TestFunctional)
Test extracting CBFS compressed data without decompressing it
----------------------------------------------------------------------
lzma_alone not available

----------------------------------------------------------------------
Ran 454 tests in 11.738s

FAILED (failures=15, errors=4, skipped=4)

99%
Name                                                    Stmts   Miss  Cover
---------------------------------------------------------------------------
tools/binman/__init__.py                                    0      0   100%
tools/binman/bintool.py                                   254      0   100%
tools/binman/btool/btool_gzip.py                            5      0   100%
tools/binman/btool/bzip2.py                                15      0   100%
tools/binman/btool/cbfstool.py                             24      0   100%
tools/binman/btool/fiptool.py                              22      0   100%
tools/binman/btool/futility.py                             24      0   100%
tools/binman/btool/ifwitool.py                             22      0   100%
tools/binman/btool/lz4.py                                  28      0   100%
tools/binman/btool/lzma_alone.py                           34      3    91%
tools/binman/btool/lzop.py                                  5      0   100%
tools/binman/btool/mkimage.py                              29      0   100%
tools/binman/btool/xz.py                                    5      0   100%
tools/binman/btool/zstd.py                                  5      0   100%
tools/binman/cbfs_util.py                                 366      0   100%
tools/binman/cmdline.py                                    73      0   100%
tools/binman/control.py                                   342      0   100%
tools/binman/elf.py                                       195     18    91%
tools/binman/entry.py                                     483      0   100%
tools/binman/etype/atf_bl31.py                              5      0   100%
tools/binman/etype/atf_fip.py                              67      0   100%
tools/binman/etype/blob.py                                 39      0   100%
tools/binman/etype/blob_dtb.py                             46      0   100%
tools/binman/etype/blob_ext.py                             11      0   100%
tools/binman/etype/blob_ext_list.py                        32      0   100%
tools/binman/etype/blob_named_by_arg.py                     9      0   100%
tools/binman/etype/blob_phase.py                           16      0   100%
tools/binman/etype/cbfs.py                                101      0   100%
tools/binman/etype/collection.py                           30      0   100%
tools/binman/etype/cros_ec_rw.py                            5      0   100%
tools/binman/etype/fdtmap.py                               62      0   100%
tools/binman/etype/files.py                                35      0   100%
tools/binman/etype/fill.py                                 13      0   100%
tools/binman/etype/fit.py                                 214      0   100%
tools/binman/etype/fmap.py                                 34      0   100%
tools/binman/etype/gbb.py                                  37      0   100%
tools/binman/etype/image_header.py                         53      0   100%
tools/binman/etype/intel_cmc.py                             4      0   100%
tools/binman/etype/intel_descriptor.py                     39      0   100%
tools/binman/etype/intel_fit.py                            12      0   100%
tools/binman/etype/intel_fit_ptr.py                        17      0   100%
tools/binman/etype/intel_fsp.py                             4      0   100%
tools/binman/etype/intel_fsp_m.py                           4      0   100%
tools/binman/etype/intel_fsp_s.py                           4      0   100%
tools/binman/etype/intel_fsp_t.py                           4      0   100%
tools/binman/etype/intel_ifwi.py                           67      0   100%
tools/binman/etype/intel_me.py                              4      0   100%
tools/binman/etype/intel_mrc.py                             6      0   100%
tools/binman/etype/intel_refcode.py                         6      0   100%
tools/binman/etype/intel_vbt.py                             4      0   100%
tools/binman/etype/intel_vga.py                             4      0   100%
tools/binman/etype/mkimage.py                              68      0   100%
tools/binman/etype/opensbi.py                               5      0   100%
tools/binman/etype/powerpc_mpc85xx_bootpg_resetvec.py       6      0   100%
tools/binman/etype/pre_load.py                             77      0   100%
tools/binman/etype/scp.py                                   5      0   100%
tools/binman/etype/section.py                             376     12    97%
tools/binman/etype/tee_os.py                                5      0   100%
tools/binman/etype/text.py                                 21      0   100%
tools/binman/etype/u_boot.py                                7      0   100%
tools/binman/etype/u_boot_dtb.py                            9      0   100%
tools/binman/etype/u_boot_dtb_with_ucode.py                51      0   100%
tools/binman/etype/u_boot_elf.py                           19      0   100%
tools/binman/etype/u_boot_env.py                           27      0   100%
tools/binman/etype/u_boot_expanded.py                       4      0   100%
tools/binman/etype/u_boot_img.py                            7      0   100%
tools/binman/etype/u_boot_nodtb.py                          7      0   100%
tools/binman/etype/u_boot_spl.py                           11      0   100%
tools/binman/etype/u_boot_spl_bss_pad.py                   14      0   100%
tools/binman/etype/u_boot_spl_dtb.py                        9      0   100%
tools/binman/etype/u_boot_spl_elf.py                        7      0   100%
tools/binman/etype/u_boot_spl_expanded.py                  12      0   100%
tools/binman/etype/u_boot_spl_nodtb.py                     11      0   100%
tools/binman/etype/u_boot_spl_with_ucode_ptr.py             8      0   100%
tools/binman/etype/u_boot_tpl.py                           11      0   100%
tools/binman/etype/u_boot_tpl_bss_pad.py                   14      0   100%
tools/binman/etype/u_boot_tpl_dtb.py                        9      0   100%
tools/binman/etype/u_boot_tpl_dtb_with_ucode.py             8      0   100%
tools/binman/etype/u_boot_tpl_elf.py                        7      0   100%
tools/binman/etype/u_boot_tpl_expanded.py                  12      0   100%
tools/binman/etype/u_boot_tpl_nodtb.py                     11      0   100%
tools/binman/etype/u_boot_tpl_with_ucode_ptr.py            12      0   100%
tools/binman/etype/u_boot_ucode.py                         33      0   100%
tools/binman/etype/u_boot_with_ucode_ptr.py                42      0   100%
tools/binman/etype/vblock.py                               38      0   100%
tools/binman/etype/x86_reset16.py                           7      0   100%
tools/binman/etype/x86_reset16_spl.py                       7      0   100%
tools/binman/etype/x86_reset16_tpl.py                       7      0   100%
tools/binman/etype/x86_start16.py                           7      0   100%
tools/binman/etype/x86_start16_spl.py                       7      0   100%
tools/binman/etype/x86_start16_tpl.py                       7      0   100%
tools/binman/fip_util.py                                  202      0   100%
tools/binman/fmap_util.py                                  48      0   100%
tools/binman/image.py                                     164      4    98%
tools/binman/state.py                                     201      0   100%
---------------------------------------------------------------------------
TOTAL                                                    4539     37    99%

To get a report in 'htmlcov/index.html', type: python3 -m coverage html
Coverage error: 99%, but should be 100%
ValueError: Test coverage failure

I guess I just should send a new mail so the community can have a look 
at it? I sent two patches for low hanging fruits but I'm afraid I won't 
have time to look at those issues any time soon :/

I'm running Fedora Server 36 x86_64 fully up-to-date if that helps.

[...]
>> I get 52% coverage only with that exact same branch, something's
>> definitely wrong here in my setup. And I **definitely** do not have
>> tools/binman/etype/mkimage.py listed in there.... Mmmmmm.
> 
> You may need to get some bintools with 'binman tool -f missing'. But

It expects apt package manager, I only have dnf :)

Also, tries to download binaries from a Google Drive, no thank you :)

> in any case, you only need to worry about coverage in mkimage.py which
> is what you changed.
> 

That's fair. Thanks for fixing my patches, lemme know if you want a v6 
instead.

[...]
>>
>> I'll play with binman until I manage to get a coverage percentage equal
>> to yours.
> 
> OK, I'd appreciate a docs patch if you can produce one from your
> efforts, or any feedback on how to make this automatic / easy.
> 

I copied the errors I'm having right now a few paragraphs above but the 
biggest issue was bzip2 getting stuck and messing up the whole test 
suite. With it patched, I was able to see the line that was kept 
untested in my patches. Otherwise I believe your advice of running 
binman test -T was enough if that had worked, just bad luck :)

Thanks!
Quentin

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

* Re: [PATCH v5 1/8] binman: add support for skipping file concatenation for mkimage
  2022-08-31 16:39                 ` Quentin Schulz
@ 2022-08-31 17:44                   ` Simon Glass
  0 siblings, 0 replies; 29+ messages in thread
From: Simon Glass @ 2022-08-31 17:44 UTC (permalink / raw)
  To: Quentin Schulz
  Cc: Quentin Schulz, Bharat Gooty, Rayagonda Kokatanur,
	Philipp Tomsich, Kever Yang, Jagan Teki, Alper Nebi Yasak,
	Heinrich Schuchardt, Heiko Thiery, U-Boot Mailing List

Hi Quentin,

On Wed, 31 Aug 2022 at 10:39, Quentin Schulz
<quentin.schulz@theobroma-systems.com> wrote:
>
> Hi Simon,
>
> On 8/31/22 15:46, Simon Glass wrote:
> > Hi Quentin,
> >
> > On Wed, 31 Aug 2022 at 03:25, Quentin Schulz
> > <quentin.schulz@theobroma-systems.com> wrote:
> >>
> >> Hi Simon,
> >>
> >> On 8/31/22 05:15, Simon Glass wrote:
> >>> Hi Quentin,
> >>>
> >>> On Tue, 30 Aug 2022 at 11:54, Quentin Schulz
> >>> <quentin.schulz@theobroma-systems.com> wrote:
> >>>>
> >>>> Hi Simon,
> >>>>
> >>>> On 8/30/22 17:56, Simon Glass wrote:
> >>>>> Hi Quentin,
> >>>>>
> >>>>> On Tue, 30 Aug 2022 at 03:57, Quentin Schulz
> >>>>> <quentin.schulz@theobroma-systems.com> wrote:
> [...]
> >>>> I do not understand how you found out coverage was not happy about my
> >>>> patchset. I have the same percentage reported from your branch or my
> >>>> local one. What am I missing?
> >>>>
> >>>> Regarding the content of the changed commits:
> >>>> testMkimageMultipleNoContent is not testing what is says it does?
> >>>> It's using multiple-data-files DT property which only impacts -d
> >>>> parameter of mkimage and the comment for the test is """Test using
> >>>> mkimage with -n and no data""".
> >>>>
> >>>> What exactly are you trying to test?
> >>>
> >>> 'binman test -T'
> >>>
> >>> I pushed your original patches to the try-rk4-orig branch. My changes
> >>> are in try-rk4.
> >>>
>
> I would change
> https://github.com/sjg20/u-boot/blob/try-rk4/tools/binman/ftest.py#L5917
> from
> """Test using mkimage with -n and no data"""
> to
> """Test passing multiple data files to mkimage with one data file having
> no content"""
> or something like that. Do you want me to send a v6 for this?
>
> Otherwise, looks fine to me :)
>
> With the bzip2 patch I just sent, I could see the line missing from unit
> tests.
>
> However, unit tests aren't happy on my PC.
>
> $ tools/binman/binman tool --list
> Name             Version      Description                Path
> ---------------  -----------  -------------------------
> ------------------------------
> gzip             1.11         gzip compression           /usr/bin/gzip
> bzip2            1.0.8        bzip2 compression          /usr/bin/bzip2
> cbfstool         unknown      Manipulate CBFS files
> /home/qschulz/work/upstream/coreboot/util/cbfstool/cbfstool
> fiptool          Unknown vers Manipulate ATF FIP files
> /home/qschulz/work/upstream/trusted-firmware-a/tools/fiptool/fiptool
> futility         v0.0.3050-18 Chromium OS firmware utili
> /home/qschulz/work/upstream/coreboot/util/futility/futility
> ifwitool         unknown      Manipulate Intel IFWI file
> /home/qschulz/work/upstream/coreboot/util/cbfstool/ifwitool
> lz4              v1.9.3       lz4 compression            /usr/bin/lz4
> lzma_alone       -            lzma_alone compression     (not found)
> lzop             v1.04        lzo compression            /usr/bin/lzop
> mkimage          2022.10-rc3- Generate image for U-Boot  /usr/bin/mkimage
> xz               5.2.5        xz compression             /usr/bin/xz
> zstd             v1.5.2       zstd compression           /usr/bin/zstd
> $ tools/binman/binman test -T
> ======================== Running binman tests ========================
> .........................................................sss.............................................F....Fs..............................................................................................F.................................................................................................................................................EEEEFF.................................FF.......FF............F................F.FFFF.................
> ======================================================================
> ERROR: testSymbols (binman.ftest.TestFunctional)
> Test binman can assign symbols embedded in U-Boot
> ----------------------------------------------------------------------
> KeyError: '_binman_sym_magic'

This could be a naming difference with the bintools on Fedora.

>
> ======================================================================
> ERROR: testSymbolsExpanded (binman.ftest.TestFunctional)
> Test binman can assign symbols in expanded entries
> ----------------------------------------------------------------------
> KeyError: '_binman_sym_magic'
>
> ======================================================================
> ERROR: testSymbolsNoDtb (binman.ftest.TestFunctional)
> Test binman can assign symbols embedded in U-Boot SPL
> ----------------------------------------------------------------------
> KeyError: '_binman_sym_magic'
>
> ======================================================================
> ERROR: testSymbolsSubsection (binman.ftest.TestFunctional)
> Test binman can assign symbols from a subsection
> ----------------------------------------------------------------------
> KeyError: '_binman_sym_magic'
>
> ======================================================================
> FAIL: testExtractAllEntries (binman.ftest.TestFunctional)
> Test extracting all entries
> ----------------------------------------------------------------------
> AssertionError: 969 != 0
>
> ======================================================================
> FAIL: testExtractCbfsCompressed (binman.ftest.TestFunctional)
> Test extracting CBFS compressed data
> ----------------------------------------------------------------------
> AssertionError: 969 != 0

That is a bit of a mystery.

>
> ======================================================================
> FAIL: testListCmd (binman.ftest.TestFunctional)
> Test listing the files in an image using an Fdtmap
> ----------------------------------------------------------------------
> AssertionError: Lists differ: ['Nam[463 chars]80   105  u-boot-dtb
>   80          3c9', [184 chars]bf8'] != ['Nam[463 chars]80     0
> u-boot-dtb        80          3c9', [184 chars]bf8']
>
> First differing element 7:
> '      u-boot-dtb        180   105  u-boot-dtb        80          3c9'
> '      u-boot-dtb        180     0  u-boot-dtb        80          3c9'
>
> Diff is 888 characters long. Set self.maxDiff to None to see it.
>
> ======================================================================
> FAIL: testSymbolsTplSection (binman.ftest.TestFunctional)
> Test binman can assign symbols embedded in U-Boot TPL in a section
> ----------------------------------------------------------------------
> AssertionError: b'\xff\xff\xff\xffBSYM\x04\x00\x00\x00
> \x00\x00\x00\x00\x00[36 chars]0klm' !=
> b'\xff\xff\xff\xff56780123456789abcdefghijklm'
>
> ======================================================================
> FAIL: testSymbolsTplSectionX86 (binman.ftest.TestFunctional)
> Test binman can assign symbols in a section with end-at-4gb
> ----------------------------------------------------------------------
> AssertionError: b'\xff\xff\xff\xffBSYM\x04\xff\xff\xff
> \xff\xff\xff\x00\x00[36 chars]0klm' !=
> b'\xff\xff\xff\xff56780123456789abcdefghijklm'
>
> ======================================================================
> FAIL: testBadSymbolSize (binman.elf_test.TestElf)
> Test that an attempt to use an 8-bit symbol are detected
> ----------------------------------------------------------------------
> AssertionError: ValueError not raised
>
> ======================================================================
> FAIL: testDebug (binman.elf_test.TestElf)
> Check that enabling debug in the elf module produced debug output
> ----------------------------------------------------------------------
> AssertionError: False is not true
>
> ======================================================================
> FAIL: testNoValue (binman.elf_test.TestElf)
> Test the case where we have no value for the symbol
> ----------------------------------------------------------------------
> AssertionError: b'BSYM\xff\xff\xff\xff\xff\xff\xff\xff\xff\[43
> chars]aaaa' != b'aaaaaaaaaaaaaaaaaaaaaaaaaaaa'

Probably the symbol table is not being read correctly in elf.py

>
> ======================================================================
> FAIL: testOutsideFile (binman.elf_test.TestElf)
> Test a symbol which extends outside the entry area is detected
> ----------------------------------------------------------------------
> AssertionError: ValueError not raised
>
> ======================================================================
> FAIL: test_cbfs_arch (binman.cbfs_util_test.TestCbfs)
> Test on non-x86 architecture
> ----------------------------------------------------------------------
> AssertionError: cbfstool produced a different result

There is no particular spec for what cbfstool does, so perhaps you
have a different version. Above it shows you have none at all,
actually.

>
> Stdout:
> diff -y <(xxd -g1 /tmp/expect) <(xxd -g1 /tmp/actual) | colordiff
>
> ======================================================================
> FAIL: test_cbfs_offset (binman.cbfs_util_test.TestCbfs)
> Test a CBFS with files at particular offsets
> ----------------------------------------------------------------------
> AssertionError: cbfstool produced a different result
>
> Stdout:
> diff -y <(xxd -g1 /tmp/expect) <(xxd -g1 /tmp/actual) | colordiff
>
> ======================================================================
> FAIL: test_cbfs_raw (binman.cbfs_util_test.TestCbfs)
> Test base handling of a Coreboot Filesystem (CBFS)
> ----------------------------------------------------------------------
> AssertionError: cbfstool produced a different result
>
> Stdout:
> diff -y <(xxd -g1 /tmp/expect) <(xxd -g1 /tmp/actual) | colordiff
>
> ======================================================================
> FAIL: test_cbfs_raw_compress (binman.cbfs_util_test.TestCbfs)
> Test base handling of compressing raw files
> ----------------------------------------------------------------------
> AssertionError: b'' != b'compress xxxxxxxxxxxxxxxxxxxxxx data'
>
> ======================================================================
> FAIL: test_cbfs_raw_space (binman.cbfs_util_test.TestCbfs)
> Test files with unused space in the CBFS
> ----------------------------------------------------------------------
> AssertionError: cbfstool produced a different result
>
> Stdout:
> diff -y <(xxd -g1 /tmp/expect) <(xxd -g1 /tmp/actual) | colordiff
>
> ======================================================================
> FAIL: test_cbfs_stage (binman.cbfs_util_test.TestCbfs)
> Tests handling of a Coreboot Filesystem (CBFS)
> ----------------------------------------------------------------------
> AssertionError: cbfstool produced a different result
>
> Stdout:
> diff -y <(xxd -g1 /tmp/expect) <(xxd -g1 /tmp/actual) | colordiff
>
> ======================================================================
> SKIP: testCompUtilCompressions (binman.ftest.TestFunctional)
> Test compression algorithms
> ----------------------------------------------------------------------
> lzma_alone not available
>
> ======================================================================
> SKIP: testCompUtilPadding (binman.ftest.TestFunctional)
> Test padding of compression algorithms
> ----------------------------------------------------------------------
> lzma_alone not available
>
> ======================================================================
> SKIP: testCompUtilVersions (binman.ftest.TestFunctional)
> Test tool version of compression algorithms
> ----------------------------------------------------------------------
> lzma_alone not available
>
> ======================================================================
> SKIP: testExtractCbfsRaw (binman.ftest.TestFunctional)
> Test extracting CBFS compressed data without decompressing it
> ----------------------------------------------------------------------
> lzma_alone not available

These are pretty obvious

>
> ----------------------------------------------------------------------
> Ran 454 tests in 11.738s
>
> FAILED (failures=15, errors=4, skipped=4)
>
> 99%
> Name                                                    Stmts   Miss  Cover
> ---------------------------------------------------------------------------
> tools/binman/__init__.py                                    0      0   100%
> tools/binman/bintool.py                                   254      0   100%
> tools/binman/btool/btool_gzip.py                            5      0   100%
> tools/binman/btool/bzip2.py                                15      0   100%
> tools/binman/btool/cbfstool.py                             24      0   100%
> tools/binman/btool/fiptool.py                              22      0   100%
> tools/binman/btool/futility.py                             24      0   100%
> tools/binman/btool/ifwitool.py                             22      0   100%
> tools/binman/btool/lz4.py                                  28      0   100%
> tools/binman/btool/lzma_alone.py                           34      3    91%
> tools/binman/btool/lzop.py                                  5      0   100%
> tools/binman/btool/mkimage.py                              29      0   100%
> tools/binman/btool/xz.py                                    5      0   100%
> tools/binman/btool/zstd.py                                  5      0   100%
> tools/binman/cbfs_util.py                                 366      0   100%
> tools/binman/cmdline.py                                    73      0   100%
> tools/binman/control.py                                   342      0   100%
> tools/binman/elf.py                                       195     18    91%
> tools/binman/entry.py                                     483      0   100%
> tools/binman/etype/atf_bl31.py                              5      0   100%
> tools/binman/etype/atf_fip.py                              67      0   100%
> tools/binman/etype/blob.py                                 39      0   100%
> tools/binman/etype/blob_dtb.py                             46      0   100%
> tools/binman/etype/blob_ext.py                             11      0   100%
> tools/binman/etype/blob_ext_list.py                        32      0   100%
> tools/binman/etype/blob_named_by_arg.py                     9      0   100%
> tools/binman/etype/blob_phase.py                           16      0   100%
> tools/binman/etype/cbfs.py                                101      0   100%
> tools/binman/etype/collection.py                           30      0   100%
> tools/binman/etype/cros_ec_rw.py                            5      0   100%
> tools/binman/etype/fdtmap.py                               62      0   100%
> tools/binman/etype/files.py                                35      0   100%
> tools/binman/etype/fill.py                                 13      0   100%
> tools/binman/etype/fit.py                                 214      0   100%
> tools/binman/etype/fmap.py                                 34      0   100%
> tools/binman/etype/gbb.py                                  37      0   100%
> tools/binman/etype/image_header.py                         53      0   100%
> tools/binman/etype/intel_cmc.py                             4      0   100%
> tools/binman/etype/intel_descriptor.py                     39      0   100%
> tools/binman/etype/intel_fit.py                            12      0   100%
> tools/binman/etype/intel_fit_ptr.py                        17      0   100%
> tools/binman/etype/intel_fsp.py                             4      0   100%
> tools/binman/etype/intel_fsp_m.py                           4      0   100%
> tools/binman/etype/intel_fsp_s.py                           4      0   100%
> tools/binman/etype/intel_fsp_t.py                           4      0   100%
> tools/binman/etype/intel_ifwi.py                           67      0   100%
> tools/binman/etype/intel_me.py                              4      0   100%
> tools/binman/etype/intel_mrc.py                             6      0   100%
> tools/binman/etype/intel_refcode.py                         6      0   100%
> tools/binman/etype/intel_vbt.py                             4      0   100%
> tools/binman/etype/intel_vga.py                             4      0   100%
> tools/binman/etype/mkimage.py                              68      0   100%
> tools/binman/etype/opensbi.py                               5      0   100%
> tools/binman/etype/powerpc_mpc85xx_bootpg_resetvec.py       6      0   100%
> tools/binman/etype/pre_load.py                             77      0   100%
> tools/binman/etype/scp.py                                   5      0   100%
> tools/binman/etype/section.py                             376     12    97%
> tools/binman/etype/tee_os.py                                5      0   100%
> tools/binman/etype/text.py                                 21      0   100%
> tools/binman/etype/u_boot.py                                7      0   100%
> tools/binman/etype/u_boot_dtb.py                            9      0   100%
> tools/binman/etype/u_boot_dtb_with_ucode.py                51      0   100%
> tools/binman/etype/u_boot_elf.py                           19      0   100%
> tools/binman/etype/u_boot_env.py                           27      0   100%
> tools/binman/etype/u_boot_expanded.py                       4      0   100%
> tools/binman/etype/u_boot_img.py                            7      0   100%
> tools/binman/etype/u_boot_nodtb.py                          7      0   100%
> tools/binman/etype/u_boot_spl.py                           11      0   100%
> tools/binman/etype/u_boot_spl_bss_pad.py                   14      0   100%
> tools/binman/etype/u_boot_spl_dtb.py                        9      0   100%
> tools/binman/etype/u_boot_spl_elf.py                        7      0   100%
> tools/binman/etype/u_boot_spl_expanded.py                  12      0   100%
> tools/binman/etype/u_boot_spl_nodtb.py                     11      0   100%
> tools/binman/etype/u_boot_spl_with_ucode_ptr.py             8      0   100%
> tools/binman/etype/u_boot_tpl.py                           11      0   100%
> tools/binman/etype/u_boot_tpl_bss_pad.py                   14      0   100%
> tools/binman/etype/u_boot_tpl_dtb.py                        9      0   100%
> tools/binman/etype/u_boot_tpl_dtb_with_ucode.py             8      0   100%
> tools/binman/etype/u_boot_tpl_elf.py                        7      0   100%
> tools/binman/etype/u_boot_tpl_expanded.py                  12      0   100%
> tools/binman/etype/u_boot_tpl_nodtb.py                     11      0   100%
> tools/binman/etype/u_boot_tpl_with_ucode_ptr.py            12      0   100%
> tools/binman/etype/u_boot_ucode.py                         33      0   100%
> tools/binman/etype/u_boot_with_ucode_ptr.py                42      0   100%
> tools/binman/etype/vblock.py                               38      0   100%
> tools/binman/etype/x86_reset16.py                           7      0   100%
> tools/binman/etype/x86_reset16_spl.py                       7      0   100%
> tools/binman/etype/x86_reset16_tpl.py                       7      0   100%
> tools/binman/etype/x86_start16.py                           7      0   100%
> tools/binman/etype/x86_start16_spl.py                       7      0   100%
> tools/binman/etype/x86_start16_tpl.py                       7      0   100%
> tools/binman/fip_util.py                                  202      0   100%
> tools/binman/fmap_util.py                                  48      0   100%
> tools/binman/image.py                                     164      4    98%
> tools/binman/state.py                                     201      0   100%
> ---------------------------------------------------------------------------
> TOTAL                                                    4539     37    99%
>
> To get a report in 'htmlcov/index.html', type: python3 -m coverage html
> Coverage error: 99%, but should be 100%
> ValueError: Test coverage failure
>
> I guess I just should send a new mail so the community can have a look
> at it? I sent two patches for low hanging fruits but I'm afraid I won't
> have time to look at those issues any time soon :/

Yes please resend your series, just with the extra test code I added.

>
> I'm running Fedora Server 36 x86_64 fully up-to-date if that helps.
>
> [...]
> >> I get 52% coverage only with that exact same branch, something's
> >> definitely wrong here in my setup. And I **definitely** do not have
> >> tools/binman/etype/mkimage.py listed in there.... Mmmmmm.
> >
> > You may need to get some bintools with 'binman tool -f missing'. But
>
> It expects apt package manager, I only have dnf :)

Patches welcome!

>
> Also, tries to download binaries from a Google Drive, no thank you :)

I don't like it either. Perhaps we could use a web site? I would
prefer to build from source, anyway.

>
> > in any case, you only need to worry about coverage in mkimage.py which
> > is what you changed.
> >
>
> That's fair. Thanks for fixing my patches, lemme know if you want a v6
> instead.

Yes please, my things were just an example to help.

>
> [...]
> >>
> >> I'll play with binman until I manage to get a coverage percentage equal
> >> to yours.
> >
> > OK, I'd appreciate a docs patch if you can produce one from your
> > efforts, or any feedback on how to make this automatic / easy.
> >
>
> I copied the errors I'm having right now a few paragraphs above but the
> biggest issue was bzip2 getting stuck and messing up the whole test
> suite. With it patched, I was able to see the line that was kept
> untested in my patches. Otherwise I believe your advice of running
> binman test -T was enough if that had worked, just bad luck :)

OK good.

Regards,
Simon

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

* Re: [PATCH v5 3/8] rockchip: remove binman temporary files when cleaning
  2022-08-26 15:36 ` [PATCH v5 3/8] rockchip: remove binman temporary files when cleaning Quentin Schulz
  2022-08-27  0:21   ` Simon Glass
@ 2022-09-01 12:04   ` Kever Yang
  1 sibling, 0 replies; 29+ messages in thread
From: Kever Yang @ 2022-09-01 12:04 UTC (permalink / raw)
  To: Quentin Schulz
  Cc: bharat.gooty, rayagonda.kokatanur, sjg, philipp.tomsich, jagan,
	alpernebiyasak, xypron.glpk, heiko.thiery, u-boot,
	Quentin Schulz, Johan Jonker


On 2022/8/26 23:36, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>
> Binman mkimage entry generates temporary files so let's remove them
> when calling `make clean`.
>
> Fixes: 9b312e26fc77 ("rockchip: Enable building a SPI ROM image on jerry")
> Cc: Quentin Schulz <foss+uboot@0leil.net>
> Reported-by: Johan Jonker <jbx6244@gmail.com>
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>

Reviewed-by: Kever Yang <kever.yang@rock-chips.com>

Thanks,
- Kever
> ---
>
> added in v3
>
>   Makefile | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index 541e942ed5..5750a9e4b8 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2225,7 +2225,8 @@ CLEAN_FILES += include/bmp_logo.h include/bmp_logo_data.h \
>   	       lpc32xx-* bl31.c bl31.elf bl31_*.bin image.map tispl.bin* \
>   	       idbloader.img flash.bin flash.log defconfig keep-syms-lto.c \
>   	       mkimage-out.spl.mkimage mkimage.spl.mkimage imx-boot.map \
> -	       itb.fit.fit itb.fit.itb itb.map spl.map
> +	       itb.fit.fit itb.fit.itb itb.map spl.map mkimage-out.rom.mkimage \
> +	       mkimage.rom.mkimage rom.map simple-bin.map
>   
>   # Directories & files removed with 'make mrproper'
>   MRPROPER_DIRS  += include/config include/generated spl tpl \

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

* Re: [PATCH v5 4/8] rockchip: generate idbloader.img content for u-boot-rockchip.bin with binman for ARM
  2022-08-26 15:36 ` [PATCH v5 4/8] rockchip: generate idbloader.img content for u-boot-rockchip.bin with binman for ARM Quentin Schulz
@ 2022-09-01 12:04   ` Kever Yang
  0 siblings, 0 replies; 29+ messages in thread
From: Kever Yang @ 2022-09-01 12:04 UTC (permalink / raw)
  To: Quentin Schulz
  Cc: bharat.gooty, rayagonda.kokatanur, sjg, philipp.tomsich, jagan,
	alpernebiyasak, xypron.glpk, heiko.thiery, u-boot,
	Quentin Schulz


On 2022/8/26 23:36, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>
> idbloader.img content - currently created by way of Makefile - can be
> created by binman directly.
>
> So let's do that for Rockchip ARM platforms.
>
> Cc: Quentin Schulz <foss+uboot@0leil.net>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>

Reviewed-by: Kever Yang <kever.yang@rock-chips.com>

Thanks,
- Kever
> ---
> v4:
>   - added Reviewed-by,
>
> v3:
>   - moved spl back into mkimage section,
>   - added filename property so that the idbloader.img binary is still
>   generated,
>   Makefile                          |  2 +-
>   arch/arm/dts/rockchip-u-boot.dtsi | 11 ++++++++++-
>   2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 5750a9e4b8..dbe1aa254a 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1011,7 +1011,7 @@ endif
>   else
>   ifeq ($(CONFIG_SPL),y)
>   # Generate these inputs for binman which will create the output files
> -INPUTS-y += idbloader.img u-boot.img
> +INPUTS-y += u-boot.img
>   endif
>   endif
>   endif
> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
> index eae3ee715d..ad72ca9700 100644
> --- a/arch/arm/dts/rockchip-u-boot.dtsi
> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
> @@ -17,8 +17,17 @@
>   		filename = "u-boot-rockchip.bin";
>   		pad-byte = <0xff>;
>   
> -		blob {
> +		mkimage {
>   			filename = "idbloader.img";
> +			args = "-n", CONFIG_SYS_SOC, "-T", "rksd";
> +#ifdef CONFIG_TPL
> +			multiple-data-files;
> +
> +			u-boot-tpl {
> +			};
> +#endif
> +			u-boot-spl {
> +			};
>   		};
>   
>   		u-boot-img {

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

* Re: [PATCH v5 5/8] rockchip: generate u-boot-rockchip.bin with binman for ARM64 boards
  2022-08-26 15:36 ` [PATCH v5 5/8] rockchip: generate u-boot-rockchip.bin with binman for ARM64 boards Quentin Schulz
@ 2022-09-01 12:05   ` Kever Yang
  0 siblings, 0 replies; 29+ messages in thread
From: Kever Yang @ 2022-09-01 12:05 UTC (permalink / raw)
  To: Quentin Schulz
  Cc: bharat.gooty, rayagonda.kokatanur, sjg, philipp.tomsich, jagan,
	alpernebiyasak, xypron.glpk, heiko.thiery, u-boot,
	Quentin Schulz


On 2022/8/26 23:36, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>
> This allows to build u-boot-rockchip.bin binary with binman for Rockchip
> ARM64 boards instead of the legacy Makefile way.
>
> Cc: Quentin Schulz <foss+uboot@0leil.net>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>

The Reviewed-by Tag usually should be after the Signed-off-by.


Reviewed-by: Kever Yang <kever.yang@rock-chips.com>

Thanks,
- Kever

> ---
>
> v4:
>   - added Reviewed-by,
>
>   Makefile                          | 26 +-------------------------
>   arch/arm/Kconfig                  |  2 +-
>   arch/arm/dts/rockchip-u-boot.dtsi |  5 +++++
>   3 files changed, 7 insertions(+), 26 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index dbe1aa254a..1dee09eb36 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1005,8 +1005,7 @@ ifeq ($(CONFIG_ARCH_ROCKCHIP),y)
>   # On ARM64 this target is produced by binman so we don't need this dep
>   ifeq ($(CONFIG_ARM64),y)
>   ifeq ($(CONFIG_SPL),y)
> -# TODO: Get binman to generate this too
> -INPUTS-y += u-boot-rockchip.bin
> +INPUTS-y += u-boot.itb
>   endif
>   else
>   ifeq ($(CONFIG_SPL),y)
> @@ -1498,29 +1497,6 @@ OBJCOPYFLAGS_u-boot-with-spl.bin = -I binary -O binary \
>   u-boot-with-spl.bin: $(SPL_IMAGE) $(SPL_PAYLOAD) FORCE
>   	$(call if_changed,pad_cat)
>   
> -ifeq ($(CONFIG_ARCH_ROCKCHIP),y)
> -
> -# TPL + SPL
> -ifeq ($(CONFIG_SPL)$(CONFIG_TPL),yy)
> -MKIMAGEFLAGS_u-boot-tpl-rockchip.bin = -n $(CONFIG_SYS_SOC) -T rksd
> -tpl/u-boot-tpl-rockchip.bin: tpl/u-boot-tpl.bin FORCE
> -	$(call if_changed,mkimage)
> -idbloader.img: tpl/u-boot-tpl-rockchip.bin spl/u-boot-spl.bin FORCE
> -	$(call if_changed,cat)
> -else
> -MKIMAGEFLAGS_idbloader.img = -n $(CONFIG_SYS_SOC) -T rksd
> -idbloader.img: spl/u-boot-spl.bin FORCE
> -	$(call if_changed,mkimage)
> -endif
> -
> -ifeq ($(CONFIG_ARM64),y)
> -OBJCOPYFLAGS_u-boot-rockchip.bin = -I binary -O binary \
> -	--pad-to=$(CONFIG_SPL_PAD_TO) --gap-fill=0xff
> -u-boot-rockchip.bin: idbloader.img u-boot.itb FORCE
> -	$(call if_changed,pad_cat)
> -endif # CONFIG_ARM64
> -
> -endif # CONFIG_ARCH_ROCKCHIP
>   
>   ifeq ($(CONFIG_ARCH_LPC32XX)$(CONFIG_SPL),yy)
>   MKIMAGEFLAGS_lpc32xx-spl.img = -T lpc32xximage -a $(CONFIG_SPL_TEXT_BASE)
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 0b72e4f650..82cd456f51 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1974,7 +1974,7 @@ config ARCH_STM32MP
>   config ARCH_ROCKCHIP
>   	bool "Support Rockchip SoCs"
>   	select BLK
> -	select BINMAN if SPL_OPTEE || (SPL && !ARM64)
> +	select BINMAN if SPL_OPTEE || SPL
>   	select DM
>   	select DM_GPIO
>   	select DM_I2C
> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
> index ad72ca9700..f90a8bf085 100644
> --- a/arch/arm/dts/rockchip-u-boot.dtsi
> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
> @@ -30,7 +30,12 @@
>   			};
>   		};
>   
> +#ifdef CONFIG_ARM64
> +		blob {
> +			filename = "u-boot.itb";
> +#else
>   		u-boot-img {
> +#endif
>   			offset = <CONFIG_SPL_PAD_TO>;
>   		};
>   	};

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

* Re: [PATCH v5 6/8] rockchip: simplify binman image dependencies addition to INPUTS
  2022-08-26 15:36 ` [PATCH v5 6/8] rockchip: simplify binman image dependencies addition to INPUTS Quentin Schulz
  2022-08-27  0:21   ` Simon Glass
@ 2022-09-01 12:08   ` Kever Yang
  1 sibling, 0 replies; 29+ messages in thread
From: Kever Yang @ 2022-09-01 12:08 UTC (permalink / raw)
  To: Quentin Schulz
  Cc: bharat.gooty, rayagonda.kokatanur, sjg, philipp.tomsich, jagan,
	alpernebiyasak, xypron.glpk, heiko.thiery, u-boot,
	Quentin Schulz


On 2022/8/26 23:36, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>
> By factoring SPL check in the first condition, this makes the checks a
> bit less convoluted and more readable.
>
> Cc: Quentin Schulz <foss+uboot@0leil.net>
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>

Reviewed-by: Kever Yang <kever.yang@rock-chips.com>

Thanks,
- Kever
> ---
>
> v4:
>   - fixed wrong place for endif for ARM32 boards,
>
>   Makefile | 9 ++-------
>   1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 1dee09eb36..736c4ad182 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1001,19 +1001,14 @@ ifeq ($(CONFIG_MPC85xx)$(CONFIG_OF_SEPARATE),yy)
>   INPUTS-y += u-boot-with-dtb.bin
>   endif
>   
> -ifeq ($(CONFIG_ARCH_ROCKCHIP),y)
> -# On ARM64 this target is produced by binman so we don't need this dep
> +ifeq ($(CONFIG_ARCH_ROCKCHIP)$(CONFIG_SPL),yy)
> +# Binman image dependencies
>   ifeq ($(CONFIG_ARM64),y)
> -ifeq ($(CONFIG_SPL),y)
>   INPUTS-y += u-boot.itb
> -endif
>   else
> -ifeq ($(CONFIG_SPL),y)
> -# Generate these inputs for binman which will create the output files
>   INPUTS-y += u-boot.img
>   endif
>   endif
> -endif
>   
>   INPUTS-$(CONFIG_X86) += u-boot-x86-start16.bin u-boot-x86-reset16.bin \
>   	$(if $(CONFIG_SPL_X86_16BIT_INIT),spl/u-boot-spl.bin) \

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

* Re: [PATCH v5 7/8] rockchip: allow to build SPI images even without HAS_ROM option
  2022-08-26 15:36 ` [PATCH v5 7/8] rockchip: allow to build SPI images even without HAS_ROM option Quentin Schulz
  2022-08-27  0:21   ` Simon Glass
@ 2022-09-01 12:08   ` Kever Yang
  1 sibling, 0 replies; 29+ messages in thread
From: Kever Yang @ 2022-09-01 12:08 UTC (permalink / raw)
  To: Quentin Schulz
  Cc: bharat.gooty, rayagonda.kokatanur, sjg, philipp.tomsich, jagan,
	alpernebiyasak, xypron.glpk, heiko.thiery, u-boot,
	Quentin Schulz


On 2022/8/26 23:36, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>
> This prepares for the creation of a u-boot-rockchip-spi.bin image
> similar to u-boot-rockchip.bin to the exception it's destined for
> SPI-NOR flashes instead of MMC storage medium.
>
> Cc: Quentin Schulz <foss+uboot@0leil.net>
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>

Reviewed-by: Kever Yang <kever.yang@rock-chips.com>

Thanks,
- Kever
> ---
>   arch/arm/dts/rk3288-u-boot.dtsi | 2 +-
>   arch/arm/dts/rk3399-u-boot.dtsi | 2 +-
>   arch/arm/mach-rockchip/Kconfig  | 6 ++----
>   3 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/dts/rk3288-u-boot.dtsi b/arch/arm/dts/rk3288-u-boot.dtsi
> index 9eb696b141..e411445ed6 100644
> --- a/arch/arm/dts/rk3288-u-boot.dtsi
> +++ b/arch/arm/dts/rk3288-u-boot.dtsi
> @@ -56,7 +56,7 @@
>   	};
>   };
>   
> -#ifdef CONFIG_ROCKCHIP_SPI_IMAGE
> +#if defined(CONFIG_ROCKCHIP_SPI_IMAGE) && defined(CONFIG_HAS_ROM)
>   &binman {
>   	rom {
>   		filename = "u-boot.rom";
> diff --git a/arch/arm/dts/rk3399-u-boot.dtsi b/arch/arm/dts/rk3399-u-boot.dtsi
> index 716b9a433a..3c1a15fe51 100644
> --- a/arch/arm/dts/rk3399-u-boot.dtsi
> +++ b/arch/arm/dts/rk3399-u-boot.dtsi
> @@ -60,7 +60,7 @@
>   
>   };
>   
> -#ifdef CONFIG_ROCKCHIP_SPI_IMAGE
> +#if defined(CONFIG_ROCKCHIP_SPI_IMAGE) && defined(CONFIG_HAS_ROM)
>   &binman {
>   	rom {
>   		filename = "u-boot.rom";
> diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
> index c561a77e6a..b46cea2f91 100644
> --- a/arch/arm/mach-rockchip/Kconfig
> +++ b/arch/arm/mach-rockchip/Kconfig
> @@ -425,12 +425,10 @@ config SPL_MMC
>   
>   config ROCKCHIP_SPI_IMAGE
>   	bool "Build a SPI image for rockchip"
> -	depends on HAS_ROM
>   	help
>   	  Some Rockchip SoCs support booting from SPI flash. Enable this
> -	  option to produce a 4MB SPI-flash image (called u-boot.rom)
> -	  containing U-Boot. The image is built by binman. U-Boot sits near
> -	  the start of the image.
> +	  option to produce a SPI-flash image containing U-Boot. The image
> +	  is built by binman. U-Boot sits near the start of the image.
>   
>   config LNX_KRNL_IMG_TEXT_OFFSET_BASE
>   	default SYS_TEXT_BASE

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

* Re: [PATCH v5 8/8] rockchip: add u-boot-rockchip-spi.bin image for booting from SPI-NOR flash
  2022-08-26 15:36 ` [PATCH v5 8/8] rockchip: add u-boot-rockchip-spi.bin image for booting from SPI-NOR flash Quentin Schulz
  2022-08-27  0:21   ` Simon Glass
@ 2022-09-01 12:08   ` Kever Yang
  1 sibling, 0 replies; 29+ messages in thread
From: Kever Yang @ 2022-09-01 12:08 UTC (permalink / raw)
  To: Quentin Schulz
  Cc: bharat.gooty, rayagonda.kokatanur, sjg, philipp.tomsich, jagan,
	alpernebiyasak, xypron.glpk, heiko.thiery, u-boot,
	Quentin Schulz


On 2022/8/26 23:36, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>
> This new image is similar to u-boot-rockchip.bin except that it's
> destined to be flashed on SPI-NOR flashes.
>
> Cc: Quentin Schulz <foss+uboot@0leil.net>
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>

Reviewed-by: Kever Yang <kever.yang@rock-chips.com>

Thanks,
- Kever
> ---
>
> v3:
>   - added filename property so that idblaoder-spi.img binary is generated
>   by binman, as per community request,
>   - added new temporary files to the list of files to clean up on `make
>   clean`,
>
>   Makefile                          |  3 ++-
>   arch/arm/dts/rockchip-u-boot.dtsi | 30 ++++++++++++++++++++++++++++++
>   2 files changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index 736c4ad182..e70e92c947 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2197,7 +2197,8 @@ CLEAN_FILES += include/bmp_logo.h include/bmp_logo_data.h \
>   	       idbloader.img flash.bin flash.log defconfig keep-syms-lto.c \
>   	       mkimage-out.spl.mkimage mkimage.spl.mkimage imx-boot.map \
>   	       itb.fit.fit itb.fit.itb itb.map spl.map mkimage-out.rom.mkimage \
> -	       mkimage.rom.mkimage rom.map simple-bin.map
> +	       mkimage.rom.mkimage rom.map simple-bin.map simple-bin-spi.map \
> +	       idbloader-spi.img
>   
>   # Directories & files removed with 'make mrproper'
>   MRPROPER_DIRS  += include/config include/generated spl tpl \
> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
> index f90a8bf085..584f21eb5b 100644
> --- a/arch/arm/dts/rockchip-u-boot.dtsi
> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
> @@ -39,5 +39,35 @@
>   			offset = <CONFIG_SPL_PAD_TO>;
>   		};
>   	};
> +
> +#ifdef CONFIG_ROCKCHIP_SPI_IMAGE
> +	simple-bin-spi {
> +		filename = "u-boot-rockchip-spi.bin";
> +		pad-byte = <0xff>;
> +
> +		mkimage {
> +			filename = "idbloader-spi.img";
> +			args = "-n", CONFIG_SYS_SOC, "-T", "rkspi";
> +#ifdef CONFIG_TPL
> +			multiple-data-files;
> +
> +			u-boot-tpl {
> +			};
> +#endif
> +			u-boot-spl {
> +			};
> +		};
> +
> +#ifdef CONFIG_ARM64
> +		blob {
> +			filename = "u-boot.itb";
> +#else
> +		u-boot-img {
> +#endif
> +			/* Sync with u-boot,spl-payload-offset if present */
> +			offset = <CONFIG_SYS_SPI_U_BOOT_OFFS>;
> +		};
> +	};
> +#endif
>   };
>   #endif

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

end of thread, other threads:[~2022-09-01 12:08 UTC | newest]

Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-26 15:36 [PATCH v5 0/8] migrate u-boot-rockchip.bin to binman and generate an image for SPI Quentin Schulz
2022-08-26 15:36 ` [PATCH v5 1/8] binman: add support for skipping file concatenation for mkimage Quentin Schulz
2022-08-27  0:21   ` Simon Glass
2022-08-30  9:57     ` Quentin Schulz
2022-08-30 15:56       ` Simon Glass
2022-08-30 17:53         ` Quentin Schulz
2022-08-31  3:15           ` Simon Glass
2022-08-31  9:25             ` Quentin Schulz
2022-08-31 13:46               ` Simon Glass
2022-08-31 16:39                 ` Quentin Schulz
2022-08-31 17:44                   ` Simon Glass
2022-08-26 15:36 ` [PATCH v5 2/8] binman: allow user-defined filenames for mkimage entry Quentin Schulz
2022-08-27  0:21   ` Simon Glass
2022-08-26 15:36 ` [PATCH v5 3/8] rockchip: remove binman temporary files when cleaning Quentin Schulz
2022-08-27  0:21   ` Simon Glass
2022-09-01 12:04   ` Kever Yang
2022-08-26 15:36 ` [PATCH v5 4/8] rockchip: generate idbloader.img content for u-boot-rockchip.bin with binman for ARM Quentin Schulz
2022-09-01 12:04   ` Kever Yang
2022-08-26 15:36 ` [PATCH v5 5/8] rockchip: generate u-boot-rockchip.bin with binman for ARM64 boards Quentin Schulz
2022-09-01 12:05   ` Kever Yang
2022-08-26 15:36 ` [PATCH v5 6/8] rockchip: simplify binman image dependencies addition to INPUTS Quentin Schulz
2022-08-27  0:21   ` Simon Glass
2022-09-01 12:08   ` Kever Yang
2022-08-26 15:36 ` [PATCH v5 7/8] rockchip: allow to build SPI images even without HAS_ROM option Quentin Schulz
2022-08-27  0:21   ` Simon Glass
2022-09-01 12:08   ` Kever Yang
2022-08-26 15:36 ` [PATCH v5 8/8] rockchip: add u-boot-rockchip-spi.bin image for booting from SPI-NOR flash Quentin Schulz
2022-08-27  0:21   ` Simon Glass
2022-09-01 12:08   ` Kever Yang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).