[PATCH 0/6] Attempt to enforce standard extensions for build output

[PATCH 0/6] Attempt to enforce standard extensions for build output

[Simon Glass]

In this early stage of using binman to produce output files, we are mostly
seeing people using common extensions such as '.bin' and '.rom'

But unusual extensions appear in some places.

We would like 'buildman -k' to keep the build outputs, but this is hard if
there is no consistency as to the extension used.

This series adjusts binman to enforce just 4 extensions for output images:

   .bin
   .rom
   .itb
   .img

Other extensions will produce an error. With this rule observed, buildman
can keep the required files.

Some patches are included to fix up some easy problems. But the following
boards generate 'custMpk.pem' and it is not clear how to fix this, so I am
asking for help from the maintainers:

   am62ax_evm_r5 am62x_evm_r5 am64x_evm_r5 am65x_evm_r5
   am65x_evm_r5_usbdfu am65x_evm_r5_usbmsc am65x_hs_evm_r5
   j7200_evm_r5 j721e_evm_r5 j721s2_evm_r5 verdin-am62_r5
   am62ax_evm_a53 am62x_evm_a53 am64x_evm_a53 am65x_evm_a53
   am65x_hs_evm_a53 j7200_evm_a72 j721e_evm_a72 j721s2_evm_a72
   verdin-am62_a53

It looks like the .pem files are listed as top-level images, e.g.:

   &custmpk_pem {
      filename = "../../ti/keys/custMpk.pem";
   };

but if the only objective is to pick up an existing file, it is better to
set BINMAN_INDIRS to include that directory. Also we should only have
simple leafnames in the 'filename' property, so the '../../ti/keys' is not
correct. It makes it harder for people to get the files from other places.
Making this easier is one of binman's goals.

Most likely the custmpk_pem node can be removed and the .pem file can be
included directly in the place that needs it, e.g. by adjusting the
ti-secure-rom etype (or ex509_cert) to use tools.get_input_filename() when
reading the key file.

For example, this:

   custMpk {
      filename = "custMpk.pem";
      custmpk_pem: blob-ext {
         filename = "../keys/custMpk.pem";
      };
   };

is really just copying a file from '../keys/custMpk.pem' to 'custMpk.pem'
so is equivalent to:

   custMpk {
      type = "blob";
      filename = "custMpk.pem";
   }

(note that blob-ext implies that the blob may be missing, so blob is a
better choice, since the key cannot be missing)

The fact that the .pem files are at the top level means that they are
output images, which surely is not intended. They should be buried in the
image description, at a lower level, as part of something else.

So please take a loke at the above and see if the binman descriptions can
be reworked slightly to follow these new rules.


Simon Glass (6):
  binman: Require image filenames to have certain extensions
  buildman: Keep all permitted output files
  buildman: Show progress when regenerating the board.cfg file
  buildman: Start the clock when the build starts
  kontron_sl28: Use u-boot-update.bin instead of u-boot.update
  stm32mp15: Use u-boot-spl-stm32.bin instead of u-boot-spl.stm32

 .../dts/fsl-ls1028a-kontron-sl28-u-boot.dtsi   |  2 +-
 arch/arm/dts/stm32mp15-u-boot.dtsi             |  2 +-
 doc/board/kontron/sl28.rst                     |  4 ++--
 doc/board/st/stm32mp1.rst                      | 18 +++++++++---------
 include/configs/stm32mp15_dh_dhsom.h           |  2 +-
 tools/binman/binman.rst                        |  5 +++++
 tools/binman/etype/section.py                  |  3 +--
 tools/binman/ftest.py                          | 12 ++++++++++--
 tools/binman/image.py                          |  9 +++++++++
 tools/binman/test/022_image_name.dts           |  4 ++--
 tools/binman/test/311_bad_image_name.dts       | 17 +++++++++++++++++
 tools/buildman/boards.py                       | 15 ++++++++++++---
 tools/buildman/builder.py                      |  3 ++-
 tools/buildman/builderthread.py                | 11 +++++++----
 tools/buildman/control.py                      |  3 ++-
 15 files changed, 81 insertions(+), 29 deletions(-)
 create mode 100644 tools/binman/test/311_bad_image_name.dts

-- 
2.42.0.rc1.204.g551eb34607-goog

[PATCH 1/6] binman: Require image filenames to have certain extensions

[Simon Glass]

It is helpful to be able to distinguish output files from other files when
building U-Boot. This is useful for buildman, for example, which can
preserve output files when requested.

Most images use extensions like .bin or .rom but some use other ones.

Introduce a check and produce an error if an image filename does not have
an allowed extension.

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

 tools/binman/binman.rst                  |  5 +++++
 tools/binman/etype/section.py            |  3 +--
 tools/binman/ftest.py                    | 12 ++++++++++--
 tools/binman/image.py                    |  9 +++++++++
 tools/binman/test/022_image_name.dts     |  4 ++--
 tools/binman/test/311_bad_image_name.dts | 17 +++++++++++++++++
 6 files changed, 44 insertions(+), 6 deletions(-)
 create mode 100644 tools/binman/test/311_bad_image_name.dts

diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst
index aeea33fddb95..3c24a8313c47 100644
--- a/tools/binman/binman.rst
+++ b/tools/binman/binman.rst
@@ -678,6 +678,11 @@ filename:
     put into the entry. If binman knows about the entry type (like
     u-boot-bin), then there is no need to specify this.
 
+    Note that image filenames must have one of a limited set of extensions:
+    `.bin`, `.rom`, `.itb` or `.img`. Others will generate an error. This is
+    so that it is clear which output files are images. It allows buildman to
+    preserve them when its `-k` option is used, for example.
+
 type:
     Sets the type of an entry. This defaults to the entry name, but it is
     possible to use any name, and then add (for example) 'type = "u-boot"'
diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py
index fb49e85a7634..8074f181ea58 100644
--- a/tools/binman/etype/section.py
+++ b/tools/binman/etype/section.py
@@ -10,6 +10,7 @@ images to be created.
 
 from collections import OrderedDict
 import concurrent.futures
+import os
 import re
 import sys
 
@@ -20,7 +21,6 @@ from u_boot_pylib import tools
 from u_boot_pylib import tout
 from u_boot_pylib.tools import to_hex_size
 
-
 class Entry_section(Entry):
     """Entry that contains other entries
 
@@ -203,7 +203,6 @@ class Entry_section(Entry):
         self.align_default = fdt_util.GetInt(self._node, 'align-default', 0)
         self._filename = fdt_util.GetString(self._node, 'filename',
                                             self._filename)
-
         self.ReadEntries()
 
     def ReadEntries(self):
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index 1293e9dbf423..5b54239827d4 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -1107,11 +1107,11 @@ class TestFunctional(unittest.TestCase):
         retcode = self._DoTestFile('022_image_name.dts')
         self.assertEqual(0, retcode)
         image = control.images['image1']
-        fname = tools.get_output_filename('test-name')
+        fname = tools.get_output_filename('test-name.bin')
         self.assertTrue(os.path.exists(fname))
 
         image = control.images['image2']
-        fname = tools.get_output_filename('test-name.xx')
+        fname = tools.get_output_filename('test-name.img')
         self.assertTrue(os.path.exists(fname))
 
     def testBlobFilename(self):
@@ -7216,5 +7216,13 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
         self.assertRegex(err,
                          "Image 'image'.*missing bintools.*: bootgen")
 
+    def testBadImageName(self):
+        """Test that image files can be named"""
+        with self.assertRaises(ValueError) as e:
+            self._DoTestFile('311_bad_image_name.dts')
+        self.assertIn(
+            "Image filename 'test-name.what' must use a permitted extension: .bin, .rom, .itb, .img",
+            str(e.exception))
+
 if __name__ == "__main__":
     unittest.main()
diff --git a/tools/binman/image.py b/tools/binman/image.py
index e77b5d0d97cd..71dc28854412 100644
--- a/tools/binman/image.py
+++ b/tools/binman/image.py
@@ -21,6 +21,9 @@ from dtoc import fdt_util
 from u_boot_pylib import tools
 from u_boot_pylib import tout
 
+# Extensions allowed for images (keep in sync with buildman/builderthread.py)
+ALLOWED_EXTS = ['.bin', '.rom', '.itb', '.img']
+
 class Image(section.Entry_section):
     """A Image, representing an output from binman
 
@@ -97,6 +100,12 @@ class Image(section.Entry_section):
         self.allow_repack = fdt_util.GetBool(self._node, 'allow-repack')
         self._symlink = fdt_util.GetString(self._node, 'symlink')
 
+        if self.section is None:
+            ext = os.path.splitext(self._filename)[1]
+            if ext not in ALLOWED_EXTS:
+                self.Raise(
+                    f"Image filename '{self._filename}' must use a permitted extension: {', '.join(ALLOWED_EXTS)}")
+
     @classmethod
     def FromFile(cls, fname):
         """Convert an image file into an Image for use in binman
diff --git a/tools/binman/test/022_image_name.dts b/tools/binman/test/022_image_name.dts
index 94fc069c1764..2da68ab06921 100644
--- a/tools/binman/test/022_image_name.dts
+++ b/tools/binman/test/022_image_name.dts
@@ -7,13 +7,13 @@
 	binman {
 		multiple-images;
 		image1 {
-			filename = "test-name";
+			filename = "test-name.bin";
 			u-boot {
 			};
 		};
 
 		image2 {
-			filename = "test-name.xx";
+			filename = "test-name.img";
 			u-boot {
 			};
 		};
diff --git a/tools/binman/test/311_bad_image_name.dts b/tools/binman/test/311_bad_image_name.dts
new file mode 100644
index 000000000000..30192957c447
--- /dev/null
+++ b/tools/binman/test/311_bad_image_name.dts
@@ -0,0 +1,17 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	binman {
+		multiple-images;
+		image1 {
+			filename = "test-name.what";
+			u-boot {
+			};
+		};
+	};
+};
-- 
2.42.0.rc1.204.g551eb34607-goog

[PATCH 2/6] buildman: Keep all permitted output files

[Simon Glass]

Now that we have a list of permitted output extensions, use it to ensure
that the -k option preserves all of these.

Signed-off-by: Simon Glass <sjg@chromium.org>
Suggested-by: Tom Rini <trini@konsulko.com>
---

 tools/buildman/builderthread.py | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
index 25f460c207db..d8374e68eb7f 100644
--- a/tools/buildman/builderthread.py
+++ b/tools/buildman/builderthread.py
@@ -23,6 +23,9 @@ from u_boot_pylib import command
 RETURN_CODE_RETRY = -1
 BASE_ELF_FILENAMES = ['u-boot', 'spl/u-boot-spl', 'tpl/u-boot-tpl']
 
+# Extensions allowed for images (keep in sync with binman/image.py, README.md)
+ALLOWED_EXTS = ['.bin', '.rom', '.itb', '.img']
+
 def mkdir(dirname, parents=False):
     """Make a directory if it doesn't already exist.
 
@@ -636,10 +639,10 @@ class BuilderThread(threading.Thread):
 
             # Now write the actual build output
             if keep_outputs:
-                copy_files(
-                    result.out_dir, build_dir, '',
-                    ['u-boot*', '*.bin', '*.map', '*.img', 'MLO', 'SPL',
-                     'include/autoconf.mk', 'spl/u-boot-spl*'])
+                to_copy = ['u-boot*', '*.map', 'MLO', 'SPL',
+                           'include/autoconf.mk', 'spl/u-boot-spl*']
+                to_copy += [f'*{ext}' for ext in ALLOWED_EXTS]
+                copy_files(result.out_dir, build_dir, '', to_copy)
 
     def _send_result(self, result):
         """Send a result to the builder for processing
-- 
2.42.0.rc1.204.g551eb34607-goog

[PATCH 3/6] buildman: Show progress when regenerating the board.cfg file

[Simon Glass]

This can take a while, so show a message when starting.

Signed-off-by: Simon Glass <sjg@chromium.org>
Reported-by Tom Rini <trini@konsulko.com>
---

 tools/buildman/boards.py  | 15 ++++++++++++---
 tools/buildman/control.py |  3 ++-
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/tools/buildman/boards.py b/tools/buildman/boards.py
index eef3f19f7ad6..341a5056dfd2 100644
--- a/tools/buildman/boards.py
+++ b/tools/buildman/boards.py
@@ -19,6 +19,7 @@ import time
 from buildman import board
 from buildman import kconfiglib
 
+from u_boot_pylib.terminal import print_clear, tprint
 
 ### constant variables ###
 OUTPUT_FILE = 'boards.cfg'
@@ -863,11 +864,19 @@ class Boards:
         Returns:
             bool: True if all is well, False if there were warnings
         """
-        if not force and output_is_new(output, CONFIG_DIR, '.'):
+        if not force:
             if not quiet:
-                print(f'{output} is up to date. Nothing to do.')
-            return True
+                tprint('\rChecking for Kconfig changes...', newline=False)
+            is_new = output_is_new(output, CONFIG_DIR, '.')
+            print_clear()
+            if is_new:
+                if not quiet:
+                    print(f'{output} is up to date. Nothing to do.')
+                return True
+        if not quiet:
+            tprint('\rGenerating board list...', newline=False)
         params_list, warnings = self.build_board_list(CONFIG_DIR, '.', jobs)
+        print_clear()
         for warn in warnings:
             print(warn, file=sys.stderr)
         self.format_and_output(params_list, output)
diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index f2ffb7f5b4aa..8f6850c52113 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -621,7 +621,8 @@ def do_buildman(args, toolchains=None, make_func=None, brds=None,
     if not brds:
         brds = get_boards_obj(output_dir, args.regen_board_list,
                               args.maintainer_check, args.full_check,
-                              args.threads, args.verbose)
+                              args.threads, args.verbose and
+                              not args.print_arch and not args.print_prefix)
         if isinstance(brds, int):
             return brds
 
-- 
2.42.0.rc1.204.g551eb34607-goog

[PATCH 4/6] buildman: Start the clock when the build starts

[Simon Glass]

The Kconfig and maintainer processing can take a while, sometimes 5
seconds or more. This skews the timing printed by buildmand when the build
completes. Start the clock when the threads start to avoid this problem.

Signed-off-by: Simon Glass <sjg@chromium.org>
Suggested-by: Tom Rini <trini@konsulko.com>
---

 tools/buildman/builder.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/buildman/builder.py b/tools/buildman/builder.py
index ecbd368c47a5..5305477c5be6 100644
--- a/tools/buildman/builder.py
+++ b/tools/buildman/builder.py
@@ -328,7 +328,7 @@ class Builder:
         self._build_period_us = None
         self._complete_delay = None
         self._next_delay_update = datetime.now()
-        self._start_time = datetime.now()
+        self._start_time = None
         self._step = step
         self._error_lines = 0
         self.no_subdirs = no_subdirs
@@ -1778,6 +1778,7 @@ class Builder:
         self._prepare_output_space()
         if not self._ide:
             tprint('\rStarting build...', newline=False)
+        self._start_time = datetime.now()
         self.setup_build(board_selected, commits)
         self.process_result(None)
         self.thread_exceptions = []
-- 
2.42.0.rc1.204.g551eb34607-goog

[PATCH 5/6] kontron_sl28: Use u-boot-update.bin instead of u-boot.update

[Simon Glass]

A '.update' extension is not allowed anymore, so change it.

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

 arch/arm/dts/fsl-ls1028a-kontron-sl28-u-boot.dtsi | 2 +-
 doc/board/kontron/sl28.rst                        | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/dts/fsl-ls1028a-kontron-sl28-u-boot.dtsi b/arch/arm/dts/fsl-ls1028a-kontron-sl28-u-boot.dtsi
index 83750ab001b2..aacf181e2dd0 100644
--- a/arch/arm/dts/fsl-ls1028a-kontron-sl28-u-boot.dtsi
+++ b/arch/arm/dts/fsl-ls1028a-kontron-sl28-u-boot.dtsi
@@ -64,7 +64,7 @@
 
 &binman {
 	u-boot-update {
-		filename = "u-boot.update";
+		filename = "u-boot-update.bin";
 
 		fit {
 			description = "FIT update image";
diff --git a/doc/board/kontron/sl28.rst b/doc/board/kontron/sl28.rst
index 44435d90c624..2cb8ec62be42 100644
--- a/doc/board/kontron/sl28.rst
+++ b/doc/board/kontron/sl28.rst
@@ -39,12 +39,12 @@ Update image
 ------------
 
 After the build finished, there will be an update image called
-u-boot.update. This can either be used in the DFU mode (which isn't
+u-boot-update.bin. This can either be used in the DFU mode (which isn't
 supported yet) or encapsulated in an EFI UpdateCapsule.
 
 To build the capsule use the following command
 
- $ tools/mkeficapsule -f u-boot.update -i 1 UpdateUboot
+ $ tools/mkeficapsule -f u-boot-update.bin -i 1 UpdateUboot
 
 Afterward you can copy this file to your ESP into the /EFI/UpdateCapsule/
 folder. On the next EFI boot this will automatically update your
-- 
2.42.0.rc1.204.g551eb34607-goog

[PATCH 6/6] stm32mp15: Use u-boot-spl-stm32.bin instead of u-boot-spl.stm32

[Simon Glass]

A '.stm32' extension is not allowed anymore, so change it.

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

 arch/arm/dts/stm32mp15-u-boot.dtsi   |  2 +-
 doc/board/st/stm32mp1.rst            | 18 +++++++++---------
 include/configs/stm32mp15_dh_dhsom.h |  2 +-
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/arch/arm/dts/stm32mp15-u-boot.dtsi b/arch/arm/dts/stm32mp15-u-boot.dtsi
index 573dd4d3ed56..717eb96a517c 100644
--- a/arch/arm/dts/stm32mp15-u-boot.dtsi
+++ b/arch/arm/dts/stm32mp15-u-boot.dtsi
@@ -222,7 +222,7 @@
 #if defined(CONFIG_SPL)
 &binman {
 	spl-stm32 {
-		filename = "u-boot-spl.stm32";
+		filename = "u-boot-spl-stm32.bin";
 		mkimage {
 			args = "-T stm32image -a 0x2ffc2500 -e 0x2ffc2500";
 			u-boot-spl {
diff --git a/doc/board/st/stm32mp1.rst b/doc/board/st/stm32mp1.rst
index 63b44776ffc1..01ba817aa80d 100644
--- a/doc/board/st/stm32mp1.rst
+++ b/doc/board/st/stm32mp1.rst
@@ -318,7 +318,7 @@ Build Procedure
 
      - stm32mp15_basic_defconfig
 
-       - FSBL = spl/u-boot-spl.stm32
+       - FSBL = spl/u-boot-spl-stm32.bin
 
        - SSBL = u-boot.img (without CONFIG_SPL_LOAD_FIT) or
                 u-boot.itb (with CONFIG_SPL_LOAD_FIT=y)
@@ -361,7 +361,7 @@ Build Procedure
 8. The bootloaders files
 
 + The **ROM code** expects FSBL binaries with STM32 image header =
-  tf-a.stm32 or u-boot-spl.stm32
+  tf-a.stm32 or u-boot-spl-stm32.bin
 
 According the FSBL / the boot mode:
 
@@ -468,9 +468,9 @@ or:
   +-------+--------+---------+------------------------+------------------------+
   | *Num* | *Name* | *Size*  | *Trusted boot content* | *Basic boot content*   |
   +=======+========+=========+========================+========================+
-  | 1     | fsbl1  | 256 KiB | TF-A_ BL2 (tf-a.stm32) | SPL (u-boot-spl.stm32) |
+  | 1     | fsbl1  | 256 KiB | TF-A_ BL2 (tf-a.stm32) | SPL (u-boot-spl-stm32.bin) |
   +-------+--------+---------+------------------------+------------------------+
-  | 2     | fsbl2  | 256 KiB | TF-A_ BL2 (tf-a.stm32) | SPL (u-boot-spl.stm32) |
+  | 2     | fsbl2  | 256 KiB | TF-A_ BL2 (tf-a.stm32) | SPL (u-boot-spl-stm32.bin) |
   +-------+--------+---------+------------------------+------------------------+
   | 3     | ssbl   | 2MB     | U-Boot (u-boot.stm32)  | U-Boot (u-boot.img)    |
   +-------+--------+---------+------------------------+------------------------+
@@ -528,8 +528,8 @@ c) copy the FSBL (2 times) and SSBL file on the correct partition.
 
    for basic boot mode : <SD card dev> = /dev/mmcblk0::
 
-    # dd if=u-boot-spl.stm32 of=/dev/mmcblk0p1
-    # dd if=u-boot-spl.stm32 of=/dev/mmcblk0p2
+    # dd if=u-boot-spl-stm32.bin of=/dev/mmcblk0p1
+    # dd if=u-boot-spl-stm32.bin of=/dev/mmcblk0p2
     # dd if=u-boot.img of=/dev/mmcblk0p3 # Without CONFIG_SPL_LOAD_FIT
       OR
       dd if=u-boot.itb of=/dev/mmcblk0p3 # With CONFIG_SPL_LOAD_FIT=y
@@ -542,7 +542,7 @@ Prepare eMMC
 You can use U-Boot to copy binary in eMMC.
 
 In the next example, you need to boot from SD card and the images
-(tf-a.stm32, fip.bin / u-boot-spl.stm32, u-boot.img for systems without
+(tf-a.stm32, fip.bin / u-boot-spl-stm32.bin, u-boot.img for systems without
 CONFIG_SPL_LOAD_FIT or u-boot.itb for systems with CONFIG_SPL_LOAD_FIT=y) are
 presents on SD card (mmc 0) in ext4 partition 4 (bootfs)
 
@@ -561,7 +561,7 @@ b) copy FSBL, TF-A_ or SPL, on first eMMC boot partition
 
     # ext4load mmc 0:4 0xC0000000 tf-a.stm32
     or
-    # ext4load mmc 0:4 0xC0000000 u-boot-spl.stm32
+    # ext4load mmc 0:4 0xC0000000 u-boot-spl-stm32.bin
 
     # mmc dev 1
     # mmc partconf 1 1 1 1
@@ -838,4 +838,4 @@ Arm TrustZone technology
   + https://www.op-tee.org/
   + https://optee.readthedocs.io/en/latest/
   + https://optee.readthedocs.io/en/latest/building/devices/stm32mp1.html
-  + https://github.com/OP-TEE/optee_os
\ No newline at end of file
+  + https://github.com/OP-TEE/optee_os
diff --git a/include/configs/stm32mp15_dh_dhsom.h b/include/configs/stm32mp15_dh_dhsom.h
index 919216906249..ad4e93805ded 100644
--- a/include/configs/stm32mp15_dh_dhsom.h
+++ b/include/configs/stm32mp15_dh_dhsom.h
@@ -22,7 +22,7 @@
 	"usb_pgood_delay=1000\0" \
 	"update_sf=" /* Erase SPI NOR and install U-Boot from SD */	\
 		"setexpr loadaddr1 ${loadaddr} + 0x1000000 && "		\
-		"load mmc 0:4 ${loadaddr1} /boot/u-boot-spl.stm32 && "	\
+		"load mmc 0:4 ${loadaddr1} /boot/u-boot-spl-stm32.bin && "	\
 		"env set filesize1 ${filesize} && "			\
 		"load mmc 0:4 ${loadaddr} /boot/u-boot.itb && "		\
 		"sf probe && sf erase 0 0x200000 && "			\
-- 
2.42.0.rc1.204.g551eb34607-goog

Re: [PATCH 6/6] stm32mp15: Use u-boot-spl-stm32.bin instead of u-boot-spl.stm32

[Marek Vasut]

On 8/24/23 05:02, Simon Glass wrote:
> A '.stm32' extension is not allowed anymore, so change it.

Why?

This will likely break a huge amount of scripts, I'm tempted to NAK it 
unless there is a very good reason.

Re: [PATCH 0/6] Attempt to enforce standard extensions for build output

[Michael Walle]

Hi,

> This series adjusts binman to enforce just 4 extensions for output 
> images:
> 
>    .bin
>    .rom
>    .itb
>    .img
> 
> Other extensions will produce an error. With this rule observed, 
> buildman
> can keep the required files.

How does this work? I didn't get all the patches from this series, which 
makes
it really hard to review or ack the individual patches.

Are we just whitelisting any files with these extension? Honestly, this
sounds like an arbitrary restriction to me. But maybe I'm missing some
context.

-michael

Re: [PATCH 5/6] kontron_sl28: Use u-boot-update.bin instead of u-boot.update

[Michael Walle]

Am 2023-08-24 05:02, schrieb Simon Glass:
> A '.update' extension is not allowed anymore, so change it.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Looks good to me, as it is just an intermediate binary.

Acked-by: Michael Walle <michael@walle.cc>

Re: [PATCH 0/6] Attempt to enforce standard extensions for build output

[Neha Malcom Francis]

Hi Simon

+ Vignesh, Nishanth to comment on this as well

On 24/08/23 08:32, Simon Glass wrote:
> In this early stage of using binman to produce output files, we are mostly
> seeing people using common extensions such as '.bin' and '.rom'

[...]

> The fact that the .pem files are at the top level means that they are
> output images, which surely is not intended. They should be buried in the
> image description, at a lower level, as part of something else.
> 
> So please take a loke at the above and see if the binman descriptions can
> be reworked slightly to follow these new rules.
> 

I think this can work... but few concerns.

1. Our output binaries also include <image>.bin_unsigned, would extensions that 
aren't "standard" be added to the list if they are truly output binaries? If 
not, changing names for them may be a long stretch (pinging Vignesh and Nishanth 
to comment here).

2. Can it be an option to not enforce this, to support users that may make use 
of the intermediate binaries?

Also if we do move forward with this, we will need to sync to make sure that the 
changes to the affected boards come through as well.

-- 
Thanking You
Neha Malcom Francis

Re: [PATCH 0/6] Attempt to enforce standard extensions for build output

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4485 bytes --]

On Wed, Aug 23, 2023 at 09:02:53PM -0600, Simon Glass wrote:

> In this early stage of using binman to produce output files, we are mostly
> seeing people using common extensions such as '.bin' and '.rom'
> 
> But unusual extensions appear in some places.
> 
> We would like 'buildman -k' to keep the build outputs, but this is hard if
> there is no consistency as to the extension used.
> 
> This series adjusts binman to enforce just 4 extensions for output images:
> 
>    .bin
>    .rom
>    .itb
>    .img
> 
> Other extensions will produce an error. With this rule observed, buildman
> can keep the required files.
> 
> Some patches are included to fix up some easy problems. But the following
> boards generate 'custMpk.pem' and it is not clear how to fix this, so I am
> asking for help from the maintainers:
> 
>    am62ax_evm_r5 am62x_evm_r5 am64x_evm_r5 am65x_evm_r5
>    am65x_evm_r5_usbdfu am65x_evm_r5_usbmsc am65x_hs_evm_r5
>    j7200_evm_r5 j721e_evm_r5 j721s2_evm_r5 verdin-am62_r5
>    am62ax_evm_a53 am62x_evm_a53 am64x_evm_a53 am65x_evm_a53
>    am65x_hs_evm_a53 j7200_evm_a72 j721e_evm_a72 j721s2_evm_a72
>    verdin-am62_a53
> 
> It looks like the .pem files are listed as top-level images, e.g.:
> 
>    &custmpk_pem {
>       filename = "../../ti/keys/custMpk.pem";
>    };
> 
> but if the only objective is to pick up an existing file, it is better to
> set BINMAN_INDIRS to include that directory. Also we should only have
> simple leafnames in the 'filename' property, so the '../../ti/keys' is not
> correct. It makes it harder for people to get the files from other places.
> Making this easier is one of binman's goals.
> 
> Most likely the custmpk_pem node can be removed and the .pem file can be
> included directly in the place that needs it, e.g. by adjusting the
> ti-secure-rom etype (or ex509_cert) to use tools.get_input_filename() when
> reading the key file.
> 
> For example, this:
> 
>    custMpk {
>       filename = "custMpk.pem";
>       custmpk_pem: blob-ext {
>          filename = "../keys/custMpk.pem";
>       };
>    };
> 
> is really just copying a file from '../keys/custMpk.pem' to 'custMpk.pem'
> so is equivalent to:
> 
>    custMpk {
>       type = "blob";
>       filename = "custMpk.pem";
>    }
> 
> (note that blob-ext implies that the blob may be missing, so blob is a
> better choice, since the key cannot be missing)
> 
> The fact that the .pem files are at the top level means that they are
> output images, which surely is not intended. They should be buried in the
> image description, at a lower level, as part of something else.
> 
> So please take a loke at the above and see if the binman descriptions can
> be reworked slightly to follow these new rules.
> 
> 
> Simon Glass (6):
>   binman: Require image filenames to have certain extensions
>   buildman: Keep all permitted output files
>   buildman: Show progress when regenerating the board.cfg file
>   buildman: Start the clock when the build starts
>   kontron_sl28: Use u-boot-update.bin instead of u-boot.update
>   stm32mp15: Use u-boot-spl-stm32.bin instead of u-boot-spl.stm32
> 
>  .../dts/fsl-ls1028a-kontron-sl28-u-boot.dtsi   |  2 +-
>  arch/arm/dts/stm32mp15-u-boot.dtsi             |  2 +-
>  doc/board/kontron/sl28.rst                     |  4 ++--
>  doc/board/st/stm32mp1.rst                      | 18 +++++++++---------
>  include/configs/stm32mp15_dh_dhsom.h           |  2 +-
>  tools/binman/binman.rst                        |  5 +++++
>  tools/binman/etype/section.py                  |  3 +--
>  tools/binman/ftest.py                          | 12 ++++++++++--
>  tools/binman/image.py                          |  9 +++++++++
>  tools/binman/test/022_image_name.dts           |  4 ++--
>  tools/binman/test/311_bad_image_name.dts       | 17 +++++++++++++++++
>  tools/buildman/boards.py                       | 15 ++++++++++++---
>  tools/buildman/builder.py                      |  3 ++-
>  tools/buildman/builderthread.py                | 11 +++++++----
>  tools/buildman/control.py                      |  3 ++-
>  15 files changed, 81 insertions(+), 29 deletions(-)
>  create mode 100644 tools/binman/test/311_bad_image_name.dts

This doesn't seem to address the ones I pointed out on IRC as being the
first order (to me anyhow) problem of needing the _unsigned files on the
platforms you list above for PEM files.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 6/6] stm32mp15: Use u-boot-spl-stm32.bin instead of u-boot-spl.stm32

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 723 bytes --]

On Thu, Aug 24, 2023 at 05:12:45AM +0200, Marek Vasut wrote:
> On 8/24/23 05:02, Simon Glass wrote:
> > A '.stm32' extension is not allowed anymore, so change it.
> 
> Why?
> 
> This will likely break a huge amount of scripts, I'm tempted to NAK it
> unless there is a very good reason.

This is in the cover letter.  Today, buildman --keep-outputs doesn't
actually keep the needed for booting outputs from a build for a number
of platforms.  Simon's response is to stop having a free-form list of
outputs. With I guess the caveat being ROM-defined names (for example,
we still keep "MLO" because that is the literal filename TI ROM looks
for on FAT partitions, on mos of their 32bit platforms).

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/6] buildman: Keep all permitted output files

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2048 bytes --]

On Wed, Aug 23, 2023 at 09:02:55PM -0600, Simon Glass wrote:

> Now that we have a list of permitted output extensions, use it to ensure
> that the -k option preserves all of these.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Suggested-by: Tom Rini <trini@konsulko.com>
> ---
> 
>  tools/buildman/builderthread.py | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
> index 25f460c207db..d8374e68eb7f 100644
> --- a/tools/buildman/builderthread.py
> +++ b/tools/buildman/builderthread.py
> @@ -23,6 +23,9 @@ from u_boot_pylib import command
>  RETURN_CODE_RETRY = -1
>  BASE_ELF_FILENAMES = ['u-boot', 'spl/u-boot-spl', 'tpl/u-boot-tpl']
>  
> +# Extensions allowed for images (keep in sync with binman/image.py, README.md)
> +ALLOWED_EXTS = ['.bin', '.rom', '.itb', '.img']
> +
>  def mkdir(dirname, parents=False):
>      """Make a directory if it doesn't already exist.
>  
> @@ -636,10 +639,10 @@ class BuilderThread(threading.Thread):
>  
>              # Now write the actual build output
>              if keep_outputs:
> -                copy_files(
> -                    result.out_dir, build_dir, '',
> -                    ['u-boot*', '*.bin', '*.map', '*.img', 'MLO', 'SPL',
> -                     'include/autoconf.mk', 'spl/u-boot-spl*'])
> +                to_copy = ['u-boot*', '*.map', 'MLO', 'SPL',
> +                           'include/autoconf.mk', 'spl/u-boot-spl*']
> +                to_copy += [f'*{ext}' for ext in ALLOWED_EXTS]
> +                copy_files(result.out_dir, build_dir, '', to_copy)
>  
>      def _send_result(self, result):
>          """Send a result to the builder for processing

The alternative here is to just add to the list "*.stm32" and
"*_unsigned", and move along.  We already have other special cases here,
of "MLO" and I guess "SPL" too.  And of course there's many other things
being caught with "u-boot*" such as ".imx".

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 0/6] Attempt to enforce standard extensions for build output

[Simon Glass]

Hi Neha,

On Thu, 24 Aug 2023 at 06:26, Neha Malcom Francis <n-francis@ti.com> wrote:
>
> Hi Simon
>
> + Vignesh, Nishanth to comment on this as well
>
> On 24/08/23 08:32, Simon Glass wrote:
> > In this early stage of using binman to produce output files, we are mostly
> > seeing people using common extensions such as '.bin' and '.rom'
>
> [...]
>
> > The fact that the .pem files are at the top level means that they are
> > output images, which surely is not intended. They should be buried in the
> > image description, at a lower level, as part of something else.
> >
> > So please take a loke at the above and see if the binman descriptions can
> > be reworked slightly to follow these new rules.
> >
>
> I think this can work... but few concerns.
>
> 1. Our output binaries also include <image>.bin_unsigned, would extensions that
> aren't "standard" be added to the list if they are truly output binaries? If
> not, changing names for them may be a long stretch (pinging Vignesh and Nishanth
> to comment here).

How about changing them to image-unsigned.bin ? That is a convention I
am trying to follow...so we have the file type last.

>
> 2. Can it be an option to not enforce this, to support users that may make use
> of the intermediate binaries?

This does not apply to sections within an image, which can still
produce files. But those files won't be preserved by buildman unless
they have a supported extension.

>
> Also if we do move forward with this, we will need to sync to make sure that the
> changes to the affected boards come through as well.

Yes.

Regards,
Simon

Re: [PATCH 0/6] Attempt to enforce standard extensions for build output

[Simon Glass]

Hi Tom,

On Thu, 24 Aug 2023 at 07:38, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Aug 23, 2023 at 09:02:53PM -0600, Simon Glass wrote:
>
> > In this early stage of using binman to produce output files, we are mostly
> > seeing people using common extensions such as '.bin' and '.rom'
> >
> > But unusual extensions appear in some places.
> >
> > We would like 'buildman -k' to keep the build outputs, but this is hard if
> > there is no consistency as to the extension used.
> >
> > This series adjusts binman to enforce just 4 extensions for output images:
> >
> >    .bin
> >    .rom
> >    .itb
> >    .img
> >
> > Other extensions will produce an error. With this rule observed, buildman
> > can keep the required files.
> >
> > Some patches are included to fix up some easy problems. But the following
> > boards generate 'custMpk.pem' and it is not clear how to fix this, so I am
> > asking for help from the maintainers:
> >
> >    am62ax_evm_r5 am62x_evm_r5 am64x_evm_r5 am65x_evm_r5
> >    am65x_evm_r5_usbdfu am65x_evm_r5_usbmsc am65x_hs_evm_r5
> >    j7200_evm_r5 j721e_evm_r5 j721s2_evm_r5 verdin-am62_r5
> >    am62ax_evm_a53 am62x_evm_a53 am64x_evm_a53 am65x_evm_a53
> >    am65x_hs_evm_a53 j7200_evm_a72 j721e_evm_a72 j721s2_evm_a72
> >    verdin-am62_a53
> >
> > It looks like the .pem files are listed as top-level images, e.g.:
> >
> >    &custmpk_pem {
> >       filename = "../../ti/keys/custMpk.pem";
> >    };
> >
> > but if the only objective is to pick up an existing file, it is better to
> > set BINMAN_INDIRS to include that directory. Also we should only have
> > simple leafnames in the 'filename' property, so the '../../ti/keys' is not
> > correct. It makes it harder for people to get the files from other places.
> > Making this easier is one of binman's goals.
> >
> > Most likely the custmpk_pem node can be removed and the .pem file can be
> > included directly in the place that needs it, e.g. by adjusting the
> > ti-secure-rom etype (or ex509_cert) to use tools.get_input_filename() when
> > reading the key file.
> >
> > For example, this:
> >
> >    custMpk {
> >       filename = "custMpk.pem";
> >       custmpk_pem: blob-ext {
> >          filename = "../keys/custMpk.pem";
> >       };
> >    };
> >
> > is really just copying a file from '../keys/custMpk.pem' to 'custMpk.pem'
> > so is equivalent to:
> >
> >    custMpk {
> >       type = "blob";
> >       filename = "custMpk.pem";
> >    }
> >
> > (note that blob-ext implies that the blob may be missing, so blob is a
> > better choice, since the key cannot be missing)
> >
> > The fact that the .pem files are at the top level means that they are
> > output images, which surely is not intended. They should be buried in the
> > image description, at a lower level, as part of something else.
> >
> > So please take a loke at the above and see if the binman descriptions can
> > be reworked slightly to follow these new rules.
> >
> >
> > Simon Glass (6):
> >   binman: Require image filenames to have certain extensions
> >   buildman: Keep all permitted output files
> >   buildman: Show progress when regenerating the board.cfg file
> >   buildman: Start the clock when the build starts
> >   kontron_sl28: Use u-boot-update.bin instead of u-boot.update
> >   stm32mp15: Use u-boot-spl-stm32.bin instead of u-boot-spl.stm32
> >
> >  .../dts/fsl-ls1028a-kontron-sl28-u-boot.dtsi   |  2 +-
> >  arch/arm/dts/stm32mp15-u-boot.dtsi             |  2 +-
> >  doc/board/kontron/sl28.rst                     |  4 ++--
> >  doc/board/st/stm32mp1.rst                      | 18 +++++++++---------
> >  include/configs/stm32mp15_dh_dhsom.h           |  2 +-
> >  tools/binman/binman.rst                        |  5 +++++
> >  tools/binman/etype/section.py                  |  3 +--
> >  tools/binman/ftest.py                          | 12 ++++++++++--
> >  tools/binman/image.py                          |  9 +++++++++
> >  tools/binman/test/022_image_name.dts           |  4 ++--
> >  tools/binman/test/311_bad_image_name.dts       | 17 +++++++++++++++++
> >  tools/buildman/boards.py                       | 15 ++++++++++++---
> >  tools/buildman/builder.py                      |  3 ++-
> >  tools/buildman/builderthread.py                | 11 +++++++----
> >  tools/buildman/control.py                      |  3 ++-
> >  15 files changed, 81 insertions(+), 29 deletions(-)
> >  create mode 100644 tools/binman/test/311_bad_image_name.dts
>
> This doesn't seem to address the ones I pointed out on IRC as being the
> first order (to me anyhow) problem of needing the _unsigned files on the
> platforms you list above for PEM files.

I think it should be xxx-unsigned.bin instead of xxx.bin_unsigned

Regards,
Simon

Re: [PATCH 0/6] Attempt to enforce standard extensions for build output

[Simon Glass]

Hi Michael,

On Thu, 24 Aug 2023 at 01:32, Michael Walle <michael@walle.cc> wrote:
>
> Hi,
>
> > This series adjusts binman to enforce just 4 extensions for output
> > images:
> >
> >    .bin
> >    .rom
> >    .itb
> >    .img
> >
> > Other extensions will produce an error. With this rule observed,
> > buildman
> > can keep the required files.
>
> How does this work? I didn't get all the patches from this series, which
> makes
> it really hard to review or ack the individual patches.
>
> Are we just whitelisting any files with these extension? Honestly, this
> sounds like an arbitrary restriction to me. But maybe I'm missing some
> context.

You can see the full series here:

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

Regards,
Simon

Re: [PATCH 0/6] Attempt to enforce standard extensions for build output

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 5388 bytes --]

On Thu, Aug 24, 2023 at 08:41:21AM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Thu, 24 Aug 2023 at 07:38, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Aug 23, 2023 at 09:02:53PM -0600, Simon Glass wrote:
> >
> > > In this early stage of using binman to produce output files, we are mostly
> > > seeing people using common extensions such as '.bin' and '.rom'
> > >
> > > But unusual extensions appear in some places.
> > >
> > > We would like 'buildman -k' to keep the build outputs, but this is hard if
> > > there is no consistency as to the extension used.
> > >
> > > This series adjusts binman to enforce just 4 extensions for output images:
> > >
> > >    .bin
> > >    .rom
> > >    .itb
> > >    .img
> > >
> > > Other extensions will produce an error. With this rule observed, buildman
> > > can keep the required files.
> > >
> > > Some patches are included to fix up some easy problems. But the following
> > > boards generate 'custMpk.pem' and it is not clear how to fix this, so I am
> > > asking for help from the maintainers:
> > >
> > >    am62ax_evm_r5 am62x_evm_r5 am64x_evm_r5 am65x_evm_r5
> > >    am65x_evm_r5_usbdfu am65x_evm_r5_usbmsc am65x_hs_evm_r5
> > >    j7200_evm_r5 j721e_evm_r5 j721s2_evm_r5 verdin-am62_r5
> > >    am62ax_evm_a53 am62x_evm_a53 am64x_evm_a53 am65x_evm_a53
> > >    am65x_hs_evm_a53 j7200_evm_a72 j721e_evm_a72 j721s2_evm_a72
> > >    verdin-am62_a53
> > >
> > > It looks like the .pem files are listed as top-level images, e.g.:
> > >
> > >    &custmpk_pem {
> > >       filename = "../../ti/keys/custMpk.pem";
> > >    };
> > >
> > > but if the only objective is to pick up an existing file, it is better to
> > > set BINMAN_INDIRS to include that directory. Also we should only have
> > > simple leafnames in the 'filename' property, so the '../../ti/keys' is not
> > > correct. It makes it harder for people to get the files from other places.
> > > Making this easier is one of binman's goals.
> > >
> > > Most likely the custmpk_pem node can be removed and the .pem file can be
> > > included directly in the place that needs it, e.g. by adjusting the
> > > ti-secure-rom etype (or ex509_cert) to use tools.get_input_filename() when
> > > reading the key file.
> > >
> > > For example, this:
> > >
> > >    custMpk {
> > >       filename = "custMpk.pem";
> > >       custmpk_pem: blob-ext {
> > >          filename = "../keys/custMpk.pem";
> > >       };
> > >    };
> > >
> > > is really just copying a file from '../keys/custMpk.pem' to 'custMpk.pem'
> > > so is equivalent to:
> > >
> > >    custMpk {
> > >       type = "blob";
> > >       filename = "custMpk.pem";
> > >    }
> > >
> > > (note that blob-ext implies that the blob may be missing, so blob is a
> > > better choice, since the key cannot be missing)
> > >
> > > The fact that the .pem files are at the top level means that they are
> > > output images, which surely is not intended. They should be buried in the
> > > image description, at a lower level, as part of something else.
> > >
> > > So please take a loke at the above and see if the binman descriptions can
> > > be reworked slightly to follow these new rules.
> > >
> > >
> > > Simon Glass (6):
> > >   binman: Require image filenames to have certain extensions
> > >   buildman: Keep all permitted output files
> > >   buildman: Show progress when regenerating the board.cfg file
> > >   buildman: Start the clock when the build starts
> > >   kontron_sl28: Use u-boot-update.bin instead of u-boot.update
> > >   stm32mp15: Use u-boot-spl-stm32.bin instead of u-boot-spl.stm32
> > >
> > >  .../dts/fsl-ls1028a-kontron-sl28-u-boot.dtsi   |  2 +-
> > >  arch/arm/dts/stm32mp15-u-boot.dtsi             |  2 +-
> > >  doc/board/kontron/sl28.rst                     |  4 ++--
> > >  doc/board/st/stm32mp1.rst                      | 18 +++++++++---------
> > >  include/configs/stm32mp15_dh_dhsom.h           |  2 +-
> > >  tools/binman/binman.rst                        |  5 +++++
> > >  tools/binman/etype/section.py                  |  3 +--
> > >  tools/binman/ftest.py                          | 12 ++++++++++--
> > >  tools/binman/image.py                          |  9 +++++++++
> > >  tools/binman/test/022_image_name.dts           |  4 ++--
> > >  tools/binman/test/311_bad_image_name.dts       | 17 +++++++++++++++++
> > >  tools/buildman/boards.py                       | 15 ++++++++++++---
> > >  tools/buildman/builder.py                      |  3 ++-
> > >  tools/buildman/builderthread.py                | 11 +++++++----
> > >  tools/buildman/control.py                      |  3 ++-
> > >  15 files changed, 81 insertions(+), 29 deletions(-)
> > >  create mode 100644 tools/binman/test/311_bad_image_name.dts
> >
> > This doesn't seem to address the ones I pointed out on IRC as being the
> > first order (to me anyhow) problem of needing the _unsigned files on the
> > platforms you list above for PEM files.
> 
> I think it should be xxx-unsigned.bin instead of xxx.bin_unsigned

As I was saying on IRC, you need to make that change then, if we're
going to go down this path, before v2023.10 is out.

And to repeat what I said inside another part of the thread, the u-boot*
glob is hiding other "arbitrary" extensions that we've had in-use for
forever.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 0/6] Attempt to enforce standard extensions for build output

[Simon Glass]

Hi Tom,

On Thu, 24 Aug 2023 at 08:45, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Aug 24, 2023 at 08:41:21AM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Thu, 24 Aug 2023 at 07:38, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Wed, Aug 23, 2023 at 09:02:53PM -0600, Simon Glass wrote:
> > >
> > > > In this early stage of using binman to produce output files, we are mostly
> > > > seeing people using common extensions such as '.bin' and '.rom'
> > > >
> > > > But unusual extensions appear in some places.
> > > >
> > > > We would like 'buildman -k' to keep the build outputs, but this is hard if
> > > > there is no consistency as to the extension used.
> > > >
> > > > This series adjusts binman to enforce just 4 extensions for output images:
> > > >
> > > >    .bin
> > > >    .rom
> > > >    .itb
> > > >    .img
> > > >
> > > > Other extensions will produce an error. With this rule observed, buildman
> > > > can keep the required files.
> > > >
> > > > Some patches are included to fix up some easy problems. But the following
> > > > boards generate 'custMpk.pem' and it is not clear how to fix this, so I am
> > > > asking for help from the maintainers:
> > > >
> > > >    am62ax_evm_r5 am62x_evm_r5 am64x_evm_r5 am65x_evm_r5
> > > >    am65x_evm_r5_usbdfu am65x_evm_r5_usbmsc am65x_hs_evm_r5
> > > >    j7200_evm_r5 j721e_evm_r5 j721s2_evm_r5 verdin-am62_r5
> > > >    am62ax_evm_a53 am62x_evm_a53 am64x_evm_a53 am65x_evm_a53
> > > >    am65x_hs_evm_a53 j7200_evm_a72 j721e_evm_a72 j721s2_evm_a72
> > > >    verdin-am62_a53
> > > >
> > > > It looks like the .pem files are listed as top-level images, e.g.:
> > > >
> > > >    &custmpk_pem {
> > > >       filename = "../../ti/keys/custMpk.pem";
> > > >    };
> > > >
> > > > but if the only objective is to pick up an existing file, it is better to
> > > > set BINMAN_INDIRS to include that directory. Also we should only have
> > > > simple leafnames in the 'filename' property, so the '../../ti/keys' is not
> > > > correct. It makes it harder for people to get the files from other places.
> > > > Making this easier is one of binman's goals.
> > > >
> > > > Most likely the custmpk_pem node can be removed and the .pem file can be
> > > > included directly in the place that needs it, e.g. by adjusting the
> > > > ti-secure-rom etype (or ex509_cert) to use tools.get_input_filename() when
> > > > reading the key file.
> > > >
> > > > For example, this:
> > > >
> > > >    custMpk {
> > > >       filename = "custMpk.pem";
> > > >       custmpk_pem: blob-ext {
> > > >          filename = "../keys/custMpk.pem";
> > > >       };
> > > >    };
> > > >
> > > > is really just copying a file from '../keys/custMpk.pem' to 'custMpk.pem'
> > > > so is equivalent to:
> > > >
> > > >    custMpk {
> > > >       type = "blob";
> > > >       filename = "custMpk.pem";
> > > >    }
> > > >
> > > > (note that blob-ext implies that the blob may be missing, so blob is a
> > > > better choice, since the key cannot be missing)
> > > >
> > > > The fact that the .pem files are at the top level means that they are
> > > > output images, which surely is not intended. They should be buried in the
> > > > image description, at a lower level, as part of something else.
> > > >
> > > > So please take a loke at the above and see if the binman descriptions can
> > > > be reworked slightly to follow these new rules.
> > > >
> > > >
> > > > Simon Glass (6):
> > > >   binman: Require image filenames to have certain extensions
> > > >   buildman: Keep all permitted output files
> > > >   buildman: Show progress when regenerating the board.cfg file
> > > >   buildman: Start the clock when the build starts
> > > >   kontron_sl28: Use u-boot-update.bin instead of u-boot.update
> > > >   stm32mp15: Use u-boot-spl-stm32.bin instead of u-boot-spl.stm32
> > > >
> > > >  .../dts/fsl-ls1028a-kontron-sl28-u-boot.dtsi   |  2 +-
> > > >  arch/arm/dts/stm32mp15-u-boot.dtsi             |  2 +-
> > > >  doc/board/kontron/sl28.rst                     |  4 ++--
> > > >  doc/board/st/stm32mp1.rst                      | 18 +++++++++---------
> > > >  include/configs/stm32mp15_dh_dhsom.h           |  2 +-
> > > >  tools/binman/binman.rst                        |  5 +++++
> > > >  tools/binman/etype/section.py                  |  3 +--
> > > >  tools/binman/ftest.py                          | 12 ++++++++++--
> > > >  tools/binman/image.py                          |  9 +++++++++
> > > >  tools/binman/test/022_image_name.dts           |  4 ++--
> > > >  tools/binman/test/311_bad_image_name.dts       | 17 +++++++++++++++++
> > > >  tools/buildman/boards.py                       | 15 ++++++++++++---
> > > >  tools/buildman/builder.py                      |  3 ++-
> > > >  tools/buildman/builderthread.py                | 11 +++++++----
> > > >  tools/buildman/control.py                      |  3 ++-
> > > >  15 files changed, 81 insertions(+), 29 deletions(-)
> > > >  create mode 100644 tools/binman/test/311_bad_image_name.dts
> > >
> > > This doesn't seem to address the ones I pointed out on IRC as being the
> > > first order (to me anyhow) problem of needing the _unsigned files on the
> > > platforms you list above for PEM files.
> >
> > I think it should be xxx-unsigned.bin instead of xxx.bin_unsigned
>
> As I was saying on IRC, you need to make that change then, if we're
> going to go down this path, before v2023.10 is out.

Yes I can do that one...but I am stuck on the PEM files so need the
maintainer there to figure out what is up, as described in detail in
the cover letter.

>
> And to repeat what I said inside another part of the thread, the u-boot*
> glob is hiding other "arbitrary" extensions that we've had in-use for
> forever.

Yes, understood. Do you think that is a problem? I should improve the
docs for buildman -k too.

Regrads,
Simon

Re: [PATCH 0/6] Attempt to enforce standard extensions for build output

[Michael Walle]

Hi,

>> > This series adjusts binman to enforce just 4 extensions for output
>> > images:
>> >
>> >    .bin
>> >    .rom
>> >    .itb
>> >    .img
>> >
>> > Other extensions will produce an error. With this rule observed,
>> > buildman
>> > can keep the required files.
>> 
>> How does this work? I didn't get all the patches from this series, 
>> which
>> makes
>> it really hard to review or ack the individual patches.
>> 
>> Are we just whitelisting any files with these extension? Honestly, 
>> this
>> sounds like an arbitrary restriction to me. But maybe I'm missing some
>> context.
> 
> You can see the full series here:
> 
> https://patchwork.ozlabs.org/project/uboot/list/?series=370121

I know the archives/patchwork. All I was saying is that sending just 
parts
of a patch series to specific ppl doesn't make any sense, because you 
always
miss the context and for that you have to look at the archives.

-michael

Re: [PATCH 6/6] stm32mp15: Use u-boot-spl-stm32.bin instead of u-boot-spl.stm32

[Marek Vasut]

On 8/24/23 16:25, Tom Rini wrote:
> On Thu, Aug 24, 2023 at 05:12:45AM +0200, Marek Vasut wrote:
>> On 8/24/23 05:02, Simon Glass wrote:
>>> A '.stm32' extension is not allowed anymore, so change it.
>>
>> Why?
>>
>> This will likely break a huge amount of scripts, I'm tempted to NAK it
>> unless there is a very good reason.
> 
> This is in the cover letter.  Today, buildman --keep-outputs doesn't
> actually keep the needed for booting outputs from a build for a number
> of platforms.  Simon's response is to stop having a free-form list of
> outputs. With I guess the caveat being ROM-defined names (for example,
> we still keep "MLO" because that is the literal filename TI ROM looks
> for on FAT partitions, on mos of their 32bit platforms).

Why not just place the free-form files into some output/ directory and 
be done with it ? Then they can have whatever extension they want, as 
long as the output/ directory name is stable.

Re: [PATCH 6/6] stm32mp15: Use u-boot-spl-stm32.bin instead of u-boot-spl.stm32

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1167 bytes --]

On Thu, Aug 24, 2023 at 05:09:07PM +0200, Marek Vasut wrote:
> On 8/24/23 16:25, Tom Rini wrote:
> > On Thu, Aug 24, 2023 at 05:12:45AM +0200, Marek Vasut wrote:
> > > On 8/24/23 05:02, Simon Glass wrote:
> > > > A '.stm32' extension is not allowed anymore, so change it.
> > > 
> > > Why?
> > > 
> > > This will likely break a huge amount of scripts, I'm tempted to NAK it
> > > unless there is a very good reason.
> > 
> > This is in the cover letter.  Today, buildman --keep-outputs doesn't
> > actually keep the needed for booting outputs from a build for a number
> > of platforms.  Simon's response is to stop having a free-form list of
> > outputs. With I guess the caveat being ROM-defined names (for example,
> > we still keep "MLO" because that is the literal filename TI ROM looks
> > for on FAT partitions, on mos of their 32bit platforms).
> 
> Why not just place the free-form files into some output/ directory and be
> done with it ? Then they can have whatever extension they want, as long as
> the output/ directory name is stable.

Yes, an alternative here is to just extend the list that's removed in
patch 2/6.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 0/6] Attempt to enforce standard extensions for build output

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 6626 bytes --]

On Thu, Aug 24, 2023 at 08:49:36AM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Thu, 24 Aug 2023 at 08:45, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Thu, Aug 24, 2023 at 08:41:21AM -0600, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Thu, 24 Aug 2023 at 07:38, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Wed, Aug 23, 2023 at 09:02:53PM -0600, Simon Glass wrote:
> > > >
> > > > > In this early stage of using binman to produce output files, we are mostly
> > > > > seeing people using common extensions such as '.bin' and '.rom'
> > > > >
> > > > > But unusual extensions appear in some places.
> > > > >
> > > > > We would like 'buildman -k' to keep the build outputs, but this is hard if
> > > > > there is no consistency as to the extension used.
> > > > >
> > > > > This series adjusts binman to enforce just 4 extensions for output images:
> > > > >
> > > > >    .bin
> > > > >    .rom
> > > > >    .itb
> > > > >    .img
> > > > >
> > > > > Other extensions will produce an error. With this rule observed, buildman
> > > > > can keep the required files.
> > > > >
> > > > > Some patches are included to fix up some easy problems. But the following
> > > > > boards generate 'custMpk.pem' and it is not clear how to fix this, so I am
> > > > > asking for help from the maintainers:
> > > > >
> > > > >    am62ax_evm_r5 am62x_evm_r5 am64x_evm_r5 am65x_evm_r5
> > > > >    am65x_evm_r5_usbdfu am65x_evm_r5_usbmsc am65x_hs_evm_r5
> > > > >    j7200_evm_r5 j721e_evm_r5 j721s2_evm_r5 verdin-am62_r5
> > > > >    am62ax_evm_a53 am62x_evm_a53 am64x_evm_a53 am65x_evm_a53
> > > > >    am65x_hs_evm_a53 j7200_evm_a72 j721e_evm_a72 j721s2_evm_a72
> > > > >    verdin-am62_a53
> > > > >
> > > > > It looks like the .pem files are listed as top-level images, e.g.:
> > > > >
> > > > >    &custmpk_pem {
> > > > >       filename = "../../ti/keys/custMpk.pem";
> > > > >    };
> > > > >
> > > > > but if the only objective is to pick up an existing file, it is better to
> > > > > set BINMAN_INDIRS to include that directory. Also we should only have
> > > > > simple leafnames in the 'filename' property, so the '../../ti/keys' is not
> > > > > correct. It makes it harder for people to get the files from other places.
> > > > > Making this easier is one of binman's goals.
> > > > >
> > > > > Most likely the custmpk_pem node can be removed and the .pem file can be
> > > > > included directly in the place that needs it, e.g. by adjusting the
> > > > > ti-secure-rom etype (or ex509_cert) to use tools.get_input_filename() when
> > > > > reading the key file.
> > > > >
> > > > > For example, this:
> > > > >
> > > > >    custMpk {
> > > > >       filename = "custMpk.pem";
> > > > >       custmpk_pem: blob-ext {
> > > > >          filename = "../keys/custMpk.pem";
> > > > >       };
> > > > >    };
> > > > >
> > > > > is really just copying a file from '../keys/custMpk.pem' to 'custMpk.pem'
> > > > > so is equivalent to:
> > > > >
> > > > >    custMpk {
> > > > >       type = "blob";
> > > > >       filename = "custMpk.pem";
> > > > >    }
> > > > >
> > > > > (note that blob-ext implies that the blob may be missing, so blob is a
> > > > > better choice, since the key cannot be missing)
> > > > >
> > > > > The fact that the .pem files are at the top level means that they are
> > > > > output images, which surely is not intended. They should be buried in the
> > > > > image description, at a lower level, as part of something else.
> > > > >
> > > > > So please take a loke at the above and see if the binman descriptions can
> > > > > be reworked slightly to follow these new rules.
> > > > >
> > > > >
> > > > > Simon Glass (6):
> > > > >   binman: Require image filenames to have certain extensions
> > > > >   buildman: Keep all permitted output files
> > > > >   buildman: Show progress when regenerating the board.cfg file
> > > > >   buildman: Start the clock when the build starts
> > > > >   kontron_sl28: Use u-boot-update.bin instead of u-boot.update
> > > > >   stm32mp15: Use u-boot-spl-stm32.bin instead of u-boot-spl.stm32
> > > > >
> > > > >  .../dts/fsl-ls1028a-kontron-sl28-u-boot.dtsi   |  2 +-
> > > > >  arch/arm/dts/stm32mp15-u-boot.dtsi             |  2 +-
> > > > >  doc/board/kontron/sl28.rst                     |  4 ++--
> > > > >  doc/board/st/stm32mp1.rst                      | 18 +++++++++---------
> > > > >  include/configs/stm32mp15_dh_dhsom.h           |  2 +-
> > > > >  tools/binman/binman.rst                        |  5 +++++
> > > > >  tools/binman/etype/section.py                  |  3 +--
> > > > >  tools/binman/ftest.py                          | 12 ++++++++++--
> > > > >  tools/binman/image.py                          |  9 +++++++++
> > > > >  tools/binman/test/022_image_name.dts           |  4 ++--
> > > > >  tools/binman/test/311_bad_image_name.dts       | 17 +++++++++++++++++
> > > > >  tools/buildman/boards.py                       | 15 ++++++++++++---
> > > > >  tools/buildman/builder.py                      |  3 ++-
> > > > >  tools/buildman/builderthread.py                | 11 +++++++----
> > > > >  tools/buildman/control.py                      |  3 ++-
> > > > >  15 files changed, 81 insertions(+), 29 deletions(-)
> > > > >  create mode 100644 tools/binman/test/311_bad_image_name.dts
> > > >
> > > > This doesn't seem to address the ones I pointed out on IRC as being the
> > > > first order (to me anyhow) problem of needing the _unsigned files on the
> > > > platforms you list above for PEM files.
> > >
> > > I think it should be xxx-unsigned.bin instead of xxx.bin_unsigned
> >
> > As I was saying on IRC, you need to make that change then, if we're
> > going to go down this path, before v2023.10 is out.
> 
> Yes I can do that one...but I am stuck on the PEM files so need the
> maintainer there to figure out what is up, as described in detail in
> the cover letter.
> 
> >
> > And to repeat what I said inside another part of the thread, the u-boot*
> > glob is hiding other "arbitrary" extensions that we've had in-use for
> > forever.
> 
> Yes, understood. Do you think that is a problem? I should improve the
> docs for buildman -k too.

Well, I'm not convinced that just extending the list in 2/6 isn't
better.  What this series does is say "anything that's not prefixed with
u-boot can only have a few suffixes, for CI".  Since most people still
aren't going to use buildman instead of make, to build things.  It'll
just be maybe-feasible for some other cases now.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 6/6] stm32mp15: Use u-boot-spl-stm32.bin instead of u-boot-spl.stm32

[Marek Vasut]

On 8/24/23 17:14, Tom Rini wrote:
> On Thu, Aug 24, 2023 at 05:09:07PM +0200, Marek Vasut wrote:
>> On 8/24/23 16:25, Tom Rini wrote:
>>> On Thu, Aug 24, 2023 at 05:12:45AM +0200, Marek Vasut wrote:
>>>> On 8/24/23 05:02, Simon Glass wrote:
>>>>> A '.stm32' extension is not allowed anymore, so change it.
>>>>
>>>> Why?
>>>>
>>>> This will likely break a huge amount of scripts, I'm tempted to NAK it
>>>> unless there is a very good reason.
>>>
>>> This is in the cover letter.  Today, buildman --keep-outputs doesn't
>>> actually keep the needed for booting outputs from a build for a number
>>> of platforms.  Simon's response is to stop having a free-form list of
>>> outputs. With I guess the caveat being ROM-defined names (for example,
>>> we still keep "MLO" because that is the literal filename TI ROM looks
>>> for on FAT partitions, on mos of their 32bit platforms).
>>
>> Why not just place the free-form files into some output/ directory and be
>> done with it ? Then they can have whatever extension they want, as long as
>> the output/ directory name is stable.
> 
> Yes, an alternative here is to just extend the list that's removed in
> patch 2/6.

I ... am not on CC of that patch ...

It would be really good if Simon CCed everyone involved on all the patches.

Re: [PATCH 0/6] Attempt to enforce standard extensions for build output

[Alper Nebi Yasak]

On 2023-08-24 06:02 +03:00, Simon Glass wrote:
> In this early stage of using binman to produce output files, we are mostly
> seeing people using common extensions such as '.bin' and '.rom'
> 
> But unusual extensions appear in some places.
> 
> We would like 'buildman -k' to keep the build outputs, but this is hard if
> there is no consistency as to the extension used.
> 
> This series adjusts binman to enforce just 4 extensions for output images:
> 
>    .bin
>    .rom
>    .itb
>    .img
> 
> Other extensions will produce an error. With this rule observed, buildman
> can keep the required files.

I dislike this limitation. We know what files we will generate, they are
listed in binman dtb, so we can add something like `binman build --ls`
to print their names/paths for buildman to preserve them.

Regarding the output directory suggestion, I think the binman outputs
(not temporary/intermediate files) should be in the same directory as
make outputs, and the Makefile should default to O=build to achieve the
"output dir". I'm not sure if that's going to happen.

Re: [PATCH 0/6] Attempt to enforce standard extensions for build output

[Simon Glass]

Hi Alper,

On Sun, 27 Aug 2023 at 13:17, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> On 2023-08-24 06:02 +03:00, Simon Glass wrote:
> > In this early stage of using binman to produce output files, we are mostly
> > seeing people using common extensions such as '.bin' and '.rom'
> >
> > But unusual extensions appear in some places.
> >
> > We would like 'buildman -k' to keep the build outputs, but this is hard if
> > there is no consistency as to the extension used.
> >
> > This series adjusts binman to enforce just 4 extensions for output images:
> >
> >    .bin
> >    .rom
> >    .itb
> >    .img
> >
> > Other extensions will produce an error. With this rule observed, buildman
> > can keep the required files.
>
> I dislike this limitation. We know what files we will generate, they are
> listed in binman dtb, so we can add something like `binman build --ls`
> to print their names/paths for buildman to preserve them.

Yes, it would be good to have that...

But why do you dislike the limitation? Do you think extensions provide
useful information? I suppose one problem is that *.bin might pick up
private blobs that happen to be in the source directory?

>
> Regarding the output directory suggestion, I think the binman outputs
> (not temporary/intermediate files) should be in the same directory as
> make outputs

Agreed

>, and the Makefile should default to O=build to achieve the
> "output dir". I'm not sure if that's going to happen.

I would quite like the 'non-output' file (i.e. things that are not a
binman image) to appear in a 'binman-work' subdir of the output dir.
What do you think?

Regards,
Simon

Re: [PATCH 0/6] Attempt to enforce standard extensions for build output

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1463 bytes --]

On Mon, Aug 28, 2023 at 11:54:55AM -0600, Simon Glass wrote:
> Hi Alper,
> 
> On Sun, 27 Aug 2023 at 13:17, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
> >
> > On 2023-08-24 06:02 +03:00, Simon Glass wrote:
> > > In this early stage of using binman to produce output files, we are mostly
> > > seeing people using common extensions such as '.bin' and '.rom'
> > >
> > > But unusual extensions appear in some places.
> > >
> > > We would like 'buildman -k' to keep the build outputs, but this is hard if
> > > there is no consistency as to the extension used.
> > >
> > > This series adjusts binman to enforce just 4 extensions for output images:
> > >
> > >    .bin
> > >    .rom
> > >    .itb
> > >    .img
> > >
> > > Other extensions will produce an error. With this rule observed, buildman
> > > can keep the required files.
> >
> > I dislike this limitation. We know what files we will generate, they are
> > listed in binman dtb, so we can add something like `binman build --ls`
> > to print their names/paths for buildman to preserve them.
> 
> Yes, it would be good to have that...
> 
> But why do you dislike the limitation? Do you think extensions provide
> useful information? I suppose one problem is that *.bin might pick up
> private blobs that happen to be in the source directory?

I think all of the complaints thus far have shown the problem with
trying to enforce an arbitrary limit here.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 6/6] stm32mp15: Use u-boot-spl-stm32.bin instead of u-boot-spl.stm32

[Patrick DELAUNAY]

Hi Simon,

On 8/24/23 17:14, Tom Rini wrote:
> On Thu, Aug 24, 2023 at 05:09:07PM +0200, Marek Vasut wrote:
>> On 8/24/23 16:25, Tom Rini wrote:
>>> On Thu, Aug 24, 2023 at 05:12:45AM +0200, Marek Vasut wrote:
>>>> On 8/24/23 05:02, Simon Glass wrote:
>>>>> A '.stm32' extension is not allowed anymore, so change it.
>>>> Why?
>>>>
>>>> This will likely break a huge amount of scripts, I'm tempted to NAK it
>>>> unless there is a very good reason.
>>> This is in the cover letter.  Today, buildman --keep-outputs doesn't
>>> actually keep the needed for booting outputs from a build for a number
>>> of platforms.  Simon's response is to stop having a free-form list of
>>> outputs. With I guess the caveat being ROM-defined names (for example,
>>> we still keep "MLO" because that is the literal filename TI ROM looks
>>> for on FAT partitions, on mos of their 32bit platforms).
>> Why not just place the free-form files into some output/ directory and be
>> done with it ? Then they can have whatever extension they want, as long as
>> the output/ directory name is stable.
> Yes, an alternative here is to just extend the list that's removed in
> patch 2/6.
>

The ".stm32" was choosen on output on mkimage to be aligned with:

- all STMicroelectonics documentation  (for example 
https://wiki.st.com/stm32mpu/wiki/STM32_header_for_binary_files)

- the proposed scripts or files, in particular in the YOCTO generated 
flashlayout files.

- this extension list expected by our tools: STM CubeProgrammer 
(https://wiki.st.com/stm32mpu/wiki/STM32CubeProgrammer)

and Signing tools (https://wiki.st.com/stm32mpu/wiki/Signing_tool)


So I prefer to kept the ".stm32" extension here:

	filename = "u-boot-spl.stm32"


NB: the justification for buildman '-k' option seens not fully relevant here

        because in patch 2/6 you kept not only the ALLOWED extension but 
also some particular files

+                to_copy = ['u-boot*', '*.map', 'MLO', 'SPL',
+                           'include/autoconf.mk', 'spl/u-boot-spl*']
+                to_copy += [f'*{ext}' for ext in ALLOWED_EXTS]


so all the files "u-boot*" are kept with buildman -k even if it is not a 
allowed extension.


I propose to change the patch 1/6 if you are agree

and allow binman to generate the file with same rules than buildman -k 
option in patch 2/6


The filename is valid if

- the file is named with the allowed prefix 'u-boot' => 'u-boot*' so 
"u-boot-spl.stm32" is allowed

- the file is with allowed extension =>.bin, .rom, .itb, .img


Regards


Patrick

Re: [PATCH 0/6] Attempt to enforce standard extensions for build output

[Alper Nebi Yasak]

On 2023-08-28 20:54 +03:00, Simon Glass wrote:
> Hi Alper,
> 
> On Sun, 27 Aug 2023 at 13:17, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>>
>> On 2023-08-24 06:02 +03:00, Simon Glass wrote:
>>> In this early stage of using binman to produce output files, we are mostly
>>> seeing people using common extensions such as '.bin' and '.rom'
>>>
>>> But unusual extensions appear in some places.
>>>
>>> We would like 'buildman -k' to keep the build outputs, but this is hard if
>>> there is no consistency as to the extension used.
>>>
>>> This series adjusts binman to enforce just 4 extensions for output images:
>>>
>>>    .bin
>>>    .rom
>>>    .itb
>>>    .img
>>>
>>> Other extensions will produce an error. With this rule observed, buildman
>>> can keep the required files.
>>
>> I dislike this limitation. We know what files we will generate, they are
>> listed in binman dtb, so we can add something like `binman build --ls`
>> to print their names/paths for buildman to preserve them.
> 
> Yes, it would be good to have that...
> 
> But why do you dislike the limitation? Do you think extensions provide
> useful information? I suppose one problem is that *.bin might pick up
> private blobs that happen to be in the source directory?

I guess my strongest argument is that people already/will have workflows
that depend on certain filenames or extensions, and shouldn't have to
modify binman code (consider that people may be using the PyPI wheels)
to accommodate those when we are already putting the name in the dtb.

I do want some standardization to the U-Boot build outputs though, but
more like a global binman.dts with like u-boot.rom, u-boot-sdmmc.img
images with well-defined TPL, SPL, U-Boot sections that arch-dtsi files
can fill in.

>> Regarding the output directory suggestion, I think the binman outputs
>> (not temporary/intermediate files) should be in the same directory as
>> make outputs
> 
> Agreed
> 
>> , and the Makefile should default to O=build to achieve the
>> "output dir". I'm not sure if that's going to happen.
> 
> I would quite like the 'non-output' file (i.e. things that are not a
> binman image) to appear in a 'binman-work' subdir of the output dir.
> What do you think?

I'd prefer them to go in a tempfile.TemporaryDirectory() and discarded
at the end of a binman run, and a "--tmpdir <path>" option to use a
given directory instead and preserve things for debugging.

Re: [PATCH 0/6] Attempt to enforce standard extensions for build output

[Simon Glass]

Hi Alper,

On Thu, 31 Aug 2023 at 04:20, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> On 2023-08-28 20:54 +03:00, Simon Glass wrote:
> > Hi Alper,
> >
> > On Sun, 27 Aug 2023 at 13:17, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
> >>
> >> On 2023-08-24 06:02 +03:00, Simon Glass wrote:
> >>> In this early stage of using binman to produce output files, we are mostly
> >>> seeing people using common extensions such as '.bin' and '.rom'
> >>>
> >>> But unusual extensions appear in some places.
> >>>
> >>> We would like 'buildman -k' to keep the build outputs, but this is hard if
> >>> there is no consistency as to the extension used.
> >>>
> >>> This series adjusts binman to enforce just 4 extensions for output images:
> >>>
> >>>    .bin
> >>>    .rom
> >>>    .itb
> >>>    .img
> >>>
> >>> Other extensions will produce an error. With this rule observed, buildman
> >>> can keep the required files.
> >>
> >> I dislike this limitation. We know what files we will generate, they are
> >> listed in binman dtb, so we can add something like `binman build --ls`
> >> to print their names/paths for buildman to preserve them.
> >
> > Yes, it would be good to have that...
> >
> > But why do you dislike the limitation? Do you think extensions provide
> > useful information? I suppose one problem is that *.bin might pick up
> > private blobs that happen to be in the source directory?
>
> I guess my strongest argument is that people already/will have workflows
> that depend on certain filenames or extensions, and shouldn't have to
> modify binman code (consider that people may be using the PyPI wheels)
> to accommodate those when we are already putting the name in the dtb.
>
> I do want some standardization to the U-Boot build outputs though, but
> more like a global binman.dts with like u-boot.rom, u-boot-sdmmc.img
> images with well-defined TPL, SPL, U-Boot sections that arch-dtsi files
> can fill in.
>
> >> Regarding the output directory suggestion, I think the binman outputs
> >> (not temporary/intermediate files) should be in the same directory as
> >> make outputs
> >
> > Agreed
> >
> >> , and the Makefile should default to O=build to achieve the
> >> "output dir". I'm not sure if that's going to happen.
> >
> > I would quite like the 'non-output' file (i.e. things that are not a
> > binman image) to appear in a 'binman-work' subdir of the output dir.
> > What do you think?
>
> I'd prefer them to go in a tempfile.TemporaryDirectory() and discarded
> at the end of a binman run, and a "--tmpdir <path>" option to use a
> given directory instead and preserve things for debugging.

Actually, that was the original purpose of the output directory in the
u_boot_pylib.tools module. But with all the files binman creates, that
has been lost. I think we should restore that purpose (with an output
dir as a temp dir) and then I think what you have written here will
work.

Regards,
Simon

Re: [PATCH 0/6] Attempt to enforce standard extensions for build output

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3283 bytes --]

On Thu, Aug 31, 2023 at 01:01:59PM -0600, Simon Glass wrote:
> Hi Alper,
> 
> On Thu, 31 Aug 2023 at 04:20, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
> >
> > On 2023-08-28 20:54 +03:00, Simon Glass wrote:
> > > Hi Alper,
> > >
> > > On Sun, 27 Aug 2023 at 13:17, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
> > >>
> > >> On 2023-08-24 06:02 +03:00, Simon Glass wrote:
> > >>> In this early stage of using binman to produce output files, we are mostly
> > >>> seeing people using common extensions such as '.bin' and '.rom'
> > >>>
> > >>> But unusual extensions appear in some places.
> > >>>
> > >>> We would like 'buildman -k' to keep the build outputs, but this is hard if
> > >>> there is no consistency as to the extension used.
> > >>>
> > >>> This series adjusts binman to enforce just 4 extensions for output images:
> > >>>
> > >>>    .bin
> > >>>    .rom
> > >>>    .itb
> > >>>    .img
> > >>>
> > >>> Other extensions will produce an error. With this rule observed, buildman
> > >>> can keep the required files.
> > >>
> > >> I dislike this limitation. We know what files we will generate, they are
> > >> listed in binman dtb, so we can add something like `binman build --ls`
> > >> to print their names/paths for buildman to preserve them.
> > >
> > > Yes, it would be good to have that...
> > >
> > > But why do you dislike the limitation? Do you think extensions provide
> > > useful information? I suppose one problem is that *.bin might pick up
> > > private blobs that happen to be in the source directory?
> >
> > I guess my strongest argument is that people already/will have workflows
> > that depend on certain filenames or extensions, and shouldn't have to
> > modify binman code (consider that people may be using the PyPI wheels)
> > to accommodate those when we are already putting the name in the dtb.
> >
> > I do want some standardization to the U-Boot build outputs though, but
> > more like a global binman.dts with like u-boot.rom, u-boot-sdmmc.img
> > images with well-defined TPL, SPL, U-Boot sections that arch-dtsi files
> > can fill in.
> >
> > >> Regarding the output directory suggestion, I think the binman outputs
> > >> (not temporary/intermediate files) should be in the same directory as
> > >> make outputs
> > >
> > > Agreed
> > >
> > >> , and the Makefile should default to O=build to achieve the
> > >> "output dir". I'm not sure if that's going to happen.
> > >
> > > I would quite like the 'non-output' file (i.e. things that are not a
> > > binman image) to appear in a 'binman-work' subdir of the output dir.
> > > What do you think?
> >
> > I'd prefer them to go in a tempfile.TemporaryDirectory() and discarded
> > at the end of a binman run, and a "--tmpdir <path>" option to use a
> > given directory instead and preserve things for debugging.
> 
> Actually, that was the original purpose of the output directory in the
> u_boot_pylib.tools module. But with all the files binman creates, that
> has been lost. I think we should restore that purpose (with an output
> dir as a temp dir) and then I think what you have written here will
> work.

Just please note the rest of the feedback that's been given to this
series too.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 0/6] Attempt to enforce standard extensions for build output

[Simon Glass]

Hi Tom,

On Thu, 31 Aug 2023 at 13:07, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Aug 31, 2023 at 01:01:59PM -0600, Simon Glass wrote:
> > Hi Alper,
> >
> > On Thu, 31 Aug 2023 at 04:20, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
> > >
> > > On 2023-08-28 20:54 +03:00, Simon Glass wrote:
> > > > Hi Alper,
> > > >
> > > > On Sun, 27 Aug 2023 at 13:17, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
> > > >>
> > > >> On 2023-08-24 06:02 +03:00, Simon Glass wrote:
> > > >>> In this early stage of using binman to produce output files, we are mostly
> > > >>> seeing people using common extensions such as '.bin' and '.rom'
> > > >>>
> > > >>> But unusual extensions appear in some places.
> > > >>>
> > > >>> We would like 'buildman -k' to keep the build outputs, but this is hard if
> > > >>> there is no consistency as to the extension used.
> > > >>>
> > > >>> This series adjusts binman to enforce just 4 extensions for output images:
> > > >>>
> > > >>>    .bin
> > > >>>    .rom
> > > >>>    .itb
> > > >>>    .img
> > > >>>
> > > >>> Other extensions will produce an error. With this rule observed, buildman
> > > >>> can keep the required files.
> > > >>
> > > >> I dislike this limitation. We know what files we will generate, they are
> > > >> listed in binman dtb, so we can add something like `binman build --ls`
> > > >> to print their names/paths for buildman to preserve them.
> > > >
> > > > Yes, it would be good to have that...
> > > >
> > > > But why do you dislike the limitation? Do you think extensions provide
> > > > useful information? I suppose one problem is that *.bin might pick up
> > > > private blobs that happen to be in the source directory?
> > >
> > > I guess my strongest argument is that people already/will have workflows
> > > that depend on certain filenames or extensions, and shouldn't have to
> > > modify binman code (consider that people may be using the PyPI wheels)
> > > to accommodate those when we are already putting the name in the dtb.
> > >
> > > I do want some standardization to the U-Boot build outputs though, but
> > > more like a global binman.dts with like u-boot.rom, u-boot-sdmmc.img
> > > images with well-defined TPL, SPL, U-Boot sections that arch-dtsi files
> > > can fill in.
> > >
> > > >> Regarding the output directory suggestion, I think the binman outputs
> > > >> (not temporary/intermediate files) should be in the same directory as
> > > >> make outputs
> > > >
> > > > Agreed
> > > >
> > > >> , and the Makefile should default to O=build to achieve the
> > > >> "output dir". I'm not sure if that's going to happen.
> > > >
> > > > I would quite like the 'non-output' file (i.e. things that are not a
> > > > binman image) to appear in a 'binman-work' subdir of the output dir.
> > > > What do you think?
> > >
> > > I'd prefer them to go in a tempfile.TemporaryDirectory() and discarded
> > > at the end of a binman run, and a "--tmpdir <path>" option to use a
> > > given directory instead and preserve things for debugging.
> >
> > Actually, that was the original purpose of the output directory in the
> > u_boot_pylib.tools module. But with all the files binman creates, that
> > has been lost. I think we should restore that purpose (with an output
> > dir as a temp dir) and then I think what you have written here will
> > work.
>
> Just please note the rest of the feedback that's been given to this
> series too.

Yes. It is a bit hard to know what to do. I suppose we need binman to
publish a list of the image files it writes, so buildman can use that
to save them. Or we could just add some more special cases to the rule
in buildman?

Regards,
Simon

Re: [PATCH 0/6] Attempt to enforce standard extensions for build output

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4121 bytes --]

On Thu, Aug 31, 2023 at 01:22:13PM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Thu, 31 Aug 2023 at 13:07, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Thu, Aug 31, 2023 at 01:01:59PM -0600, Simon Glass wrote:
> > > Hi Alper,
> > >
> > > On Thu, 31 Aug 2023 at 04:20, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
> > > >
> > > > On 2023-08-28 20:54 +03:00, Simon Glass wrote:
> > > > > Hi Alper,
> > > > >
> > > > > On Sun, 27 Aug 2023 at 13:17, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
> > > > >>
> > > > >> On 2023-08-24 06:02 +03:00, Simon Glass wrote:
> > > > >>> In this early stage of using binman to produce output files, we are mostly
> > > > >>> seeing people using common extensions such as '.bin' and '.rom'
> > > > >>>
> > > > >>> But unusual extensions appear in some places.
> > > > >>>
> > > > >>> We would like 'buildman -k' to keep the build outputs, but this is hard if
> > > > >>> there is no consistency as to the extension used.
> > > > >>>
> > > > >>> This series adjusts binman to enforce just 4 extensions for output images:
> > > > >>>
> > > > >>>    .bin
> > > > >>>    .rom
> > > > >>>    .itb
> > > > >>>    .img
> > > > >>>
> > > > >>> Other extensions will produce an error. With this rule observed, buildman
> > > > >>> can keep the required files.
> > > > >>
> > > > >> I dislike this limitation. We know what files we will generate, they are
> > > > >> listed in binman dtb, so we can add something like `binman build --ls`
> > > > >> to print their names/paths for buildman to preserve them.
> > > > >
> > > > > Yes, it would be good to have that...
> > > > >
> > > > > But why do you dislike the limitation? Do you think extensions provide
> > > > > useful information? I suppose one problem is that *.bin might pick up
> > > > > private blobs that happen to be in the source directory?
> > > >
> > > > I guess my strongest argument is that people already/will have workflows
> > > > that depend on certain filenames or extensions, and shouldn't have to
> > > > modify binman code (consider that people may be using the PyPI wheels)
> > > > to accommodate those when we are already putting the name in the dtb.
> > > >
> > > > I do want some standardization to the U-Boot build outputs though, but
> > > > more like a global binman.dts with like u-boot.rom, u-boot-sdmmc.img
> > > > images with well-defined TPL, SPL, U-Boot sections that arch-dtsi files
> > > > can fill in.
> > > >
> > > > >> Regarding the output directory suggestion, I think the binman outputs
> > > > >> (not temporary/intermediate files) should be in the same directory as
> > > > >> make outputs
> > > > >
> > > > > Agreed
> > > > >
> > > > >> , and the Makefile should default to O=build to achieve the
> > > > >> "output dir". I'm not sure if that's going to happen.
> > > > >
> > > > > I would quite like the 'non-output' file (i.e. things that are not a
> > > > > binman image) to appear in a 'binman-work' subdir of the output dir.
> > > > > What do you think?
> > > >
> > > > I'd prefer them to go in a tempfile.TemporaryDirectory() and discarded
> > > > at the end of a binman run, and a "--tmpdir <path>" option to use a
> > > > given directory instead and preserve things for debugging.
> > >
> > > Actually, that was the original purpose of the output directory in the
> > > u_boot_pylib.tools module. But with all the files binman creates, that
> > > has been lost. I think we should restore that purpose (with an output
> > > dir as a temp dir) and then I think what you have written here will
> > > work.
> >
> > Just please note the rest of the feedback that's been given to this
> > series too.
> 
> Yes. It is a bit hard to know what to do. I suppose we need binman to
> publish a list of the image files it writes, so buildman can use that
> to save them. Or we could just add some more special cases to the rule
> in buildman?

Extend the list of files that we do keep today, so that at least more
cases could work, set aside the "everything else is an error" notion for
now.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 0/6] Attempt to enforce standard extensions for build output

[Simon Glass]

Hi Tom,

On Thu, 31 Aug 2023 at 13:24, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Aug 31, 2023 at 01:22:13PM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Thu, 31 Aug 2023 at 13:07, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Thu, Aug 31, 2023 at 01:01:59PM -0600, Simon Glass wrote:
> > > > Hi Alper,
> > > >
> > > > On Thu, 31 Aug 2023 at 04:20, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
> > > > >
> > > > > On 2023-08-28 20:54 +03:00, Simon Glass wrote:
> > > > > > Hi Alper,
> > > > > >
> > > > > > On Sun, 27 Aug 2023 at 13:17, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
> > > > > >>
> > > > > >> On 2023-08-24 06:02 +03:00, Simon Glass wrote:
> > > > > >>> In this early stage of using binman to produce output files, we are mostly
> > > > > >>> seeing people using common extensions such as '.bin' and '.rom'
> > > > > >>>
> > > > > >>> But unusual extensions appear in some places.
> > > > > >>>
> > > > > >>> We would like 'buildman -k' to keep the build outputs, but this is hard if
> > > > > >>> there is no consistency as to the extension used.
> > > > > >>>
> > > > > >>> This series adjusts binman to enforce just 4 extensions for output images:
> > > > > >>>
> > > > > >>>    .bin
> > > > > >>>    .rom
> > > > > >>>    .itb
> > > > > >>>    .img
> > > > > >>>
> > > > > >>> Other extensions will produce an error. With this rule observed, buildman
> > > > > >>> can keep the required files.
> > > > > >>
> > > > > >> I dislike this limitation. We know what files we will generate, they are
> > > > > >> listed in binman dtb, so we can add something like `binman build --ls`
> > > > > >> to print their names/paths for buildman to preserve them.
> > > > > >
> > > > > > Yes, it would be good to have that...
> > > > > >
> > > > > > But why do you dislike the limitation? Do you think extensions provide
> > > > > > useful information? I suppose one problem is that *.bin might pick up
> > > > > > private blobs that happen to be in the source directory?
> > > > >
> > > > > I guess my strongest argument is that people already/will have workflows
> > > > > that depend on certain filenames or extensions, and shouldn't have to
> > > > > modify binman code (consider that people may be using the PyPI wheels)
> > > > > to accommodate those when we are already putting the name in the dtb.
> > > > >
> > > > > I do want some standardization to the U-Boot build outputs though, but
> > > > > more like a global binman.dts with like u-boot.rom, u-boot-sdmmc.img
> > > > > images with well-defined TPL, SPL, U-Boot sections that arch-dtsi files
> > > > > can fill in.
> > > > >
> > > > > >> Regarding the output directory suggestion, I think the binman outputs
> > > > > >> (not temporary/intermediate files) should be in the same directory as
> > > > > >> make outputs
> > > > > >
> > > > > > Agreed
> > > > > >
> > > > > >> , and the Makefile should default to O=build to achieve the
> > > > > >> "output dir". I'm not sure if that's going to happen.
> > > > > >
> > > > > > I would quite like the 'non-output' file (i.e. things that are not a
> > > > > > binman image) to appear in a 'binman-work' subdir of the output dir.
> > > > > > What do you think?
> > > > >
> > > > > I'd prefer them to go in a tempfile.TemporaryDirectory() and discarded
> > > > > at the end of a binman run, and a "--tmpdir <path>" option to use a
> > > > > given directory instead and preserve things for debugging.
> > > >
> > > > Actually, that was the original purpose of the output directory in the
> > > > u_boot_pylib.tools module. But with all the files binman creates, that
> > > > has been lost. I think we should restore that purpose (with an output
> > > > dir as a temp dir) and then I think what you have written here will
> > > > work.
> > >
> > > Just please note the rest of the feedback that's been given to this
> > > series too.
> >
> > Yes. It is a bit hard to know what to do. I suppose we need binman to
> > publish a list of the image files it writes, so buildman can use that
> > to save them. Or we could just add some more special cases to the rule
> > in buildman?
>
> Extend the list of files that we do keep today, so that at least more
> cases could work, set aside the "everything else is an error" notion for
> now.

OK got it, thanks.


- Simon

Re: [PATCH 6/6] stm32mp15: Use u-boot-spl-stm32.bin instead of u-boot-spl.stm32

[Simon Glass]

Hi,

On Tue, 29 Aug 2023 at 10:29, Patrick DELAUNAY
<patrick.delaunay@foss.st.com> wrote:
>
> Hi Simon,
>
> On 8/24/23 17:14, Tom Rini wrote:
> > On Thu, Aug 24, 2023 at 05:09:07PM +0200, Marek Vasut wrote:
> >> On 8/24/23 16:25, Tom Rini wrote:
> >>> On Thu, Aug 24, 2023 at 05:12:45AM +0200, Marek Vasut wrote:
> >>>> On 8/24/23 05:02, Simon Glass wrote:
> >>>>> A '.stm32' extension is not allowed anymore, so change it.
> >>>> Why?
> >>>>
> >>>> This will likely break a huge amount of scripts, I'm tempted to NAK it
> >>>> unless there is a very good reason.
> >>> This is in the cover letter.  Today, buildman --keep-outputs doesn't
> >>> actually keep the needed for booting outputs from a build for a number
> >>> of platforms.  Simon's response is to stop having a free-form list of
> >>> outputs. With I guess the caveat being ROM-defined names (for example,
> >>> we still keep "MLO" because that is the literal filename TI ROM looks
> >>> for on FAT partitions, on mos of their 32bit platforms).
> >> Why not just place the free-form files into some output/ directory and be
> >> done with it ? Then they can have whatever extension they want, as long as
> >> the output/ directory name is stable.
> > Yes, an alternative here is to just extend the list that's removed in
> > patch 2/6.
> >
>
> The ".stm32" was choosen on output on mkimage to be aligned with:
>
> - all STMicroelectonics documentation  (for example
> https://wiki.st.com/stm32mpu/wiki/STM32_header_for_binary_files)
>
> - the proposed scripts or files, in particular in the YOCTO generated
> flashlayout files.
>
> - this extension list expected by our tools: STM CubeProgrammer
> (https://wiki.st.com/stm32mpu/wiki/STM32CubeProgrammer)
>
> and Signing tools (https://wiki.st.com/stm32mpu/wiki/Signing_tool)
>
>
> So I prefer to kept the ".stm32" extension here:
>
>         filename = "u-boot-spl.stm32"
>
>
> NB: the justification for buildman '-k' option seens not fully relevant here
>
>         because in patch 2/6 you kept not only the ALLOWED extension but
> also some particular files
>
> +                to_copy = ['u-boot*', '*.map', 'MLO', 'SPL',
> +                           'include/autoconf.mk', 'spl/u-boot-spl*']
> +                to_copy += [f'*{ext}' for ext in ALLOWED_EXTS]
>
>
> so all the files "u-boot*" are kept with buildman -k even if it is not a
> allowed extension.
>
>
> I propose to change the patch 1/6 if you are agree
>
> and allow binman to generate the file with same rules than buildman -k
> option in patch 2/6
>
>
> The filename is valid if
>
> - the file is named with the allowed prefix 'u-boot' => 'u-boot*' so
> "u-boot-spl.stm32" is allowed
>
> - the file is with allowed extension =>.bin, .rom, .itb, .img

Yes OK, it seems this won't affect you in any case.

But as Tom says, we are going to try another path...basically just opt
in those files we need.

Regards,
Simon