[PATCH 0/9] riscv: Partially move to binman to generate u-boot.itb

[PATCH 0/9] riscv: Partially move to binman to generate u-boot.itb

[Bin Meng]

This series updates binman to handle creation of u-boot.itb image for
RISC-V SiFive Unleashed board.

QEMU RISC-V remains unchanged, as binman uses a dtb to describe the
image format, but for QEMU RISC-V there is no dtb as dtb is passed
to U-Boot via CONFIG_OF_PRIOR_STAGE.

Not sure how such use case could be properly supported by binman?


Bin Meng (9):
  common: kconfig: Correct a typo in SPL_LOAD_FIT
  binman: Correct '-a' description in the doc
  binman: Correct the comment for ATF entry type
  binman: test: Rename 172_fit_fdt.dts to 170_fit_fdt.dts
  binman: test: Correct the name of 170_fit_fdt_missing_prop.dts
  binman: Add support for RISC-V OpenSBI fw_dynamic blob
  makefile: Update clean rule to remove files generated by binman
  makefile: Pass OpenSBI blob to binman make rules
  riscv: sifive: unleashed: Switch to use binman to generate u-boot.itb

 .gitignore                                    |  4 +-
 Makefile                                      |  6 +-
 arch/riscv/dts/binman.dtsi                    | 70 +++++++++++++++++++
 .../dts/hifive-unleashed-a00-u-boot.dtsi      |  1 +
 board/sifive/unleashed/Kconfig                |  1 +
 common/Kconfig.boot                           |  2 +-
 configs/sifive_unleashed_defconfig            |  1 +
 tools/binman/binman.rst                       |  4 +-
 tools/binman/entries.rst                      | 13 ++++
 tools/binman/etype/atf_bl31.py                |  2 +-
 tools/binman/etype/opensbi.py                 | 23 ++++++
 tools/binman/ftest.py                         | 17 +++--
 .../test/{172_fit_fdt.dts => 170_fit_fdt.dts} |  0
 tools/binman/test/201_opensbi.dts             | 16 +++++
 14 files changed, 148 insertions(+), 12 deletions(-)
 create mode 100644 arch/riscv/dts/binman.dtsi
 create mode 100644 tools/binman/etype/opensbi.py
 rename tools/binman/test/{172_fit_fdt.dts => 170_fit_fdt.dts} (100%)
 create mode 100644 tools/binman/test/201_opensbi.dts

-- 
2.25.1

[PATCH 1/9] common: kconfig: Correct a typo in SPL_LOAD_FIT

[Bin Meng]

It should be FDT, not FTD.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 common/Kconfig.boot | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/Kconfig.boot b/common/Kconfig.boot
index 5a18d62d78..94d82c27dd 100644
--- a/common/Kconfig.boot
+++ b/common/Kconfig.boot
@@ -204,7 +204,7 @@ config SPL_LOAD_FIT
 
 	  This path has the following limitations:
 
-	  1. "loadables" images, other than FTDs, which do not have a "load"
+	  1. "loadables" images, other than FDTs, which do not have a "load"
 	     property will not be loaded. This limitation also applies to FPGA
 	     images with the correct "compatible" string.
 	  2. For FPGA images, only the "compatible" = "u-boot,fpga-legacy"
-- 
2.25.1

[PATCH 2/9] binman: Correct '-a' description in the doc

[Bin Meng]

It needs a space around '-a'.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 tools/binman/binman.rst | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst
index 1aa2459d50..b3df3a6428 100644
--- a/tools/binman/binman.rst
+++ b/tools/binman/binman.rst
@@ -322,9 +322,9 @@ Sometimes it is useful to pass binman the value of an entry property from the
 command line. For example some entries need access to files and it is not
 always convenient to put these filenames in the image definition (device tree).
 
-The-a option supports this::
+The -a option supports this::
 
-    -a<prop>=<value>
+    -a <prop>=<value>
 
 where::
 
-- 
2.25.1

[PATCH 3/9] binman: Correct the comment for ATF entry type

[Bin Meng]

This is wrongly referring to Intel ME, which should be ATF.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 tools/binman/etype/atf_bl31.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/binman/etype/atf_bl31.py b/tools/binman/etype/atf_bl31.py
index 163d714184..2041da416c 100644
--- a/tools/binman/etype/atf_bl31.py
+++ b/tools/binman/etype/atf_bl31.py
@@ -2,7 +2,7 @@
 # Copyright 2020 Google LLC
 # Written by Simon Glass <sjg@chromium.org>
 #
-# Entry-type module for Intel Management Engine binary blob
+# Entry-type module for ARM Trusted Firmware binary blob
 #
 
 from binman.etype.blob_named_by_arg import Entry_blob_named_by_arg
-- 
2.25.1

[PATCH 4/9] binman: test: Rename 172_fit_fdt.dts to 170_fit_fdt.dts

[Bin Meng]

Currently there are 2 binman test cases using the same 172 number.
It seems that 172_fit_fdt.dts was originally named as 170_, but
commit c0f1ebe9c1b9 ("binman: Allow selecting default FIT configuration")
changed its name to 172_ for no reason. Let's change it back.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 tools/binman/ftest.py                                | 12 ++++++------
 .../binman/test/{172_fit_fdt.dts => 170_fit_fdt.dts} |  0
 2 files changed, 6 insertions(+), 6 deletions(-)
 rename tools/binman/test/{172_fit_fdt.dts => 170_fit_fdt.dts} (100%)

diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index f36823f51b..08f84cd32d 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -3826,7 +3826,7 @@ class TestFunctional(unittest.TestCase):
             'default-dt': 'test-fdt2',
         }
         data = self._DoReadFileDtb(
-            '172_fit_fdt.dts',
+            '170_fit_fdt.dts',
             entry_args=entry_args,
             extra_indirs=[os.path.join(self._indir, TEST_FDT_SUBDIR)])[0]
         self.assertEqual(U_BOOT_NODTB_DATA, data[-len(U_BOOT_NODTB_DATA):])
@@ -3848,7 +3848,7 @@ class TestFunctional(unittest.TestCase):
     def testFitFdtMissingList(self):
         """Test handling of a missing 'of-list' entry arg"""
         with self.assertRaises(ValueError) as e:
-            self._DoReadFile('172_fit_fdt.dts')
+            self._DoReadFile('170_fit_fdt.dts')
         self.assertIn("Generator node requires 'of-list' entry argument",
                       str(e.exception))
 
@@ -3862,7 +3862,7 @@ class TestFunctional(unittest.TestCase):
     def testFitFdtMissingProp(self):
         """Test handling of a missing 'fit,fdt-list' property"""
         with self.assertRaises(ValueError) as e:
-            self._DoReadFile('171_fit_fdt_missing_prop.dts')
+            self._DoReadFile('170_fit_fdt_missing_prop.dts')
         self.assertIn("Generator node requires 'fit,fdt-list' property",
                       str(e.exception))
 
@@ -3871,7 +3871,7 @@ class TestFunctional(unittest.TestCase):
         entry_args = {
             'of-list': '',
         }
-        data = self._DoReadFileDtb('172_fit_fdt.dts', entry_args=entry_args)[0]
+        data = self._DoReadFileDtb('170_fit_fdt.dts', entry_args=entry_args)[0]
 
     def testFitFdtMissing(self):
         """Test handling of a missing 'default-dt' entry arg"""
@@ -3880,7 +3880,7 @@ class TestFunctional(unittest.TestCase):
         }
         with self.assertRaises(ValueError) as e:
             self._DoReadFileDtb(
-                '172_fit_fdt.dts',
+                '170_fit_fdt.dts',
                 entry_args=entry_args,
                 extra_indirs=[os.path.join(self._indir, TEST_FDT_SUBDIR)])[0]
         self.assertIn("Generated 'default' node requires default-dt entry argument",
@@ -3894,7 +3894,7 @@ class TestFunctional(unittest.TestCase):
         }
         with self.assertRaises(ValueError) as e:
             self._DoReadFileDtb(
-                '172_fit_fdt.dts',
+                '170_fit_fdt.dts',
                 entry_args=entry_args,
                 extra_indirs=[os.path.join(self._indir, TEST_FDT_SUBDIR)])[0]
         self.assertIn("default-dt entry argument 'test-fdt3' not found in fdt list: test-fdt1, test-fdt2",
diff --git a/tools/binman/test/172_fit_fdt.dts b/tools/binman/test/170_fit_fdt.dts
similarity index 100%
rename from tools/binman/test/172_fit_fdt.dts
rename to tools/binman/test/170_fit_fdt.dts
-- 
2.25.1

[PATCH 5/9] binman: test: Correct the name of 170_fit_fdt_missing_prop.dts

[Bin Meng]

It should be 171_fit_fdt_missing_prop.dts.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 tools/binman/ftest.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index 08f84cd32d..b0daccbc3b 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -3862,7 +3862,7 @@ class TestFunctional(unittest.TestCase):
     def testFitFdtMissingProp(self):
         """Test handling of a missing 'fit,fdt-list' property"""
         with self.assertRaises(ValueError) as e:
-            self._DoReadFile('170_fit_fdt_missing_prop.dts')
+            self._DoReadFile('171_fit_fdt_missing_prop.dts')
         self.assertIn("Generator node requires 'fit,fdt-list' property",
                       str(e.exception))
 
-- 
2.25.1

[PATCH 6/9] binman: Add support for RISC-V OpenSBI fw_dynamic blob

[Bin Meng]

Add an entry for RISC-V OpenSBI's 'fw_dynamic' firmware payload.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 tools/binman/entries.rst          | 13 +++++++++++++
 tools/binman/etype/opensbi.py     | 23 +++++++++++++++++++++++
 tools/binman/ftest.py             |  7 +++++++
 tools/binman/test/201_opensbi.dts | 16 ++++++++++++++++
 4 files changed, 59 insertions(+)
 create mode 100644 tools/binman/etype/opensbi.py
 create mode 100644 tools/binman/test/201_opensbi.dts

diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
index f1c3b7de7a..dcac700c46 100644
--- a/tools/binman/entries.rst
+++ b/tools/binman/entries.rst
@@ -761,6 +761,19 @@ binman.
 
 
 
+Entry: opensbi: RISC-V OpenSBI fw_dynamic blob
+----------------------------------------------
+
+Properties / Entry arguments:
+    - opensbi-path: Filename of file to read into entry. This is typically
+        called fw_dynamic.bin
+
+This entry holds the run-time firmware, typically started by U-Boot SPL.
+See the U-Boot README for your architecture or board for how to use it. See
+https://github.com/riscv/opensbi for more information about OpenSBI.
+
+
+
 Entry: powerpc-mpc85xx-bootpg-resetvec: PowerPC mpc85xx bootpg + resetvec code for U-Boot
 -----------------------------------------------------------------------------------------
 
diff --git a/tools/binman/etype/opensbi.py b/tools/binman/etype/opensbi.py
new file mode 100644
index 0000000000..74d473d535
--- /dev/null
+++ b/tools/binman/etype/opensbi.py
@@ -0,0 +1,23 @@
+# SPDX-License-Identifier: GPL-2.0+
+#
+# Copyright (C) 2021, Bin Meng <bmeng.cn@gmail.com>
+#
+# Entry-type module for RISC-V OpenSBI binary blob
+#
+
+from binman.etype.blob_named_by_arg import Entry_blob_named_by_arg
+
+class Entry_opensbi(Entry_blob_named_by_arg):
+    """RISC-V OpenSBI fw_dynamic blob
+
+    Properties / Entry arguments:
+        - opensbi-path: Filename of file to read into entry. This is typically
+            called fw_dynamic.bin
+
+    This entry holds the run-time firmware, typically started by U-Boot SPL.
+    See the U-Boot README for your architecture or board for how to use it. See
+    https://github.com/riscv/opensbi for more information about OpenSBI.
+    """
+    def __init__(self, section, etype, node):
+        super().__init__(section, etype, node, 'opensbi')
+        self.external = True
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index b0daccbc3b..5383eec489 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -76,6 +76,7 @@ FSP_M_DATA            = b'fsp_m'
 FSP_S_DATA            = b'fsp_s'
 FSP_T_DATA            = b'fsp_t'
 ATF_BL31_DATA         = b'bl31'
+OPENSBI_DATA          = b'opensbi'
 SCP_DATA              = b'scp'
 TEST_FDT1_DATA        = b'fdt1'
 TEST_FDT2_DATA        = b'test-fdt2'
@@ -178,6 +179,7 @@ class TestFunctional(unittest.TestCase):
         TestFunctional._MakeInputFile('compress', COMPRESS_DATA)
         TestFunctional._MakeInputFile('compress_big', COMPRESS_DATA_BIG)
         TestFunctional._MakeInputFile('bl31.bin', ATF_BL31_DATA)
+        TestFunctional._MakeInputFile('fw_dynamic.bin', OPENSBI_DATA)
         TestFunctional._MakeInputFile('scp.bin', SCP_DATA)
 
         # Add a few .dtb files for testing
@@ -4535,5 +4537,10 @@ class TestFunctional(unittest.TestCase):
         expected += tools.GetBytes(0, 88 - len(expected)) + U_BOOT_NODTB_DATA
         self.assertEqual(expected, data)
 
+    def testPackOpenSBI(self):
+        """Test that an image with an OpenSBI binary can be created"""
+        data = self._DoReadFile('201_opensbi.dts')
+        self.assertEqual(OPENSBI_DATA, data[:len(OPENSBI_DATA)])
+
 if __name__ == "__main__":
     unittest.main()
diff --git a/tools/binman/test/201_opensbi.dts b/tools/binman/test/201_opensbi.dts
new file mode 100644
index 0000000000..16c01a946a
--- /dev/null
+++ b/tools/binman/test/201_opensbi.dts
@@ -0,0 +1,16 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	binman {
+		size = <16>;
+
+		opensbi {
+			filename = "fw_dynamic.bin";
+		};
+	};
+};
-- 
2.25.1

[PATCH 7/9] makefile: Update clean rule to remove files generated by binman

[Bin Meng]

When using binman to generate an FIT image, these intermediate files
"*.fit.fit" and "*.fit.itb" are generated from mkimage, which should
be cleaned, and git ignored.

While we are here, clean the map file generated by "binman -m" as well.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 .gitignore | 4 +++-
 Makefile   | 5 +++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/.gitignore b/.gitignore
index e66aa864da..b9ebe0dfd7 100644
--- a/.gitignore
+++ b/.gitignore
@@ -40,10 +40,12 @@
 fit-dtb.blob*
 /MLO*
 /SPL*
-/System.map
 /u-boot*
 /boards.cfg
+/*.fit.fit
+/*.fit.itb
 /*.log
+/*.map
 
 #
 # git files that we don't want to ignore even it they are dot-files
diff --git a/Makefile b/Makefile
index 404977efa5..a5701f6f9a 100644
--- a/Makefile
+++ b/Makefile
@@ -1998,9 +1998,10 @@ CLEAN_DIRS  += $(MODVERDIR) \
 			$(filter-out include, $(shell ls -1 $d 2>/dev/null))))
 
 CLEAN_FILES += include/bmp_logo.h include/bmp_logo_data.h tools/version.h \
-	       boot* u-boot* MLO* SPL System.map fit-dtb.blob* \
+	       boot* u-boot* MLO* SPL *.map fit-dtb.blob* \
 	       u-boot-ivt.img.log u-boot-dtb.imx.log SPL.log u-boot.imx.log \
-	       lpc32xx-* bl31.c bl31.elf bl31_*.bin image.map tispl.bin* \
+	       lpc32xx-* bl31.c bl31.elf bl31_*.bin tispl.bin* \
+	       *.fit.fit *.fit.itb \
 	       idbloader.img flash.bin flash.log defconfig
 
 # Directories & files removed with 'make mrproper'
-- 
2.25.1

[PATCH 8/9] makefile: Pass OpenSBI blob to binman make rules

[Bin Meng]

This updates the make rules to pass OpenSBI blob to binman.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Makefile b/Makefile
index a5701f6f9a..c1ddcae22d 100644
--- a/Makefile
+++ b/Makefile
@@ -1287,6 +1287,7 @@ cmd_binman = $(srctree)/tools/binman/binman $(if $(BINMAN_DEBUG),-D) \
 		-I . -I $(srctree) -I $(srctree)/board/$(BOARDDIR) \
 		-I arch/$(ARCH)/dts -a of-list=$(CONFIG_OF_LIST) \
 		-a atf-bl31-path=${BL31} \
+		-a opensbi-path=${OPENSBI} \
 		-a default-dt=$(default_dt) \
 		-a scp-path=$(SCP) \
 		-a spl-bss-pad=$(if $(CONFIG_SPL_SEPARATE_BSS),,1) \
-- 
2.25.1

[PATCH 9/9] riscv: sifive: unleashed: Switch to use binman to generate u-boot.itb

[Bin Meng]

At present SiFive Unleashed board uses the Makefile to create the FIT,
using USE_SPL_FIT_GENERATOR, which is deprecated as per the Makefile
warning. Update to use binman instead.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

---

 arch/riscv/dts/binman.dtsi                    | 70 +++++++++++++++++++
 .../dts/hifive-unleashed-a00-u-boot.dtsi      |  1 +
 board/sifive/unleashed/Kconfig                |  1 +
 configs/sifive_unleashed_defconfig            |  1 +
 4 files changed, 73 insertions(+)
 create mode 100644 arch/riscv/dts/binman.dtsi

diff --git a/arch/riscv/dts/binman.dtsi b/arch/riscv/dts/binman.dtsi
new file mode 100644
index 0000000000..f2f8647b24
--- /dev/null
+++ b/arch/riscv/dts/binman.dtsi
@@ -0,0 +1,70 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2021, Bin Meng <bmeng.cn@gmail.com>
+ */
+
+#include <config.h>
+
+/ {
+	binman: binman {
+		multiple-images;
+	};
+};
+
+&binman {
+	itb {
+		filename = "u-boot.itb";
+
+		fit {
+			description = "Configuration to load OpenSBI before U-Boot";
+			#address-cells = <1>;
+			fit,fdt-list = "of-list";
+
+			images {
+				uboot {
+					description = "U-Boot";
+					type = "standalone";
+					os = "U-Boot";
+					arch = "riscv";
+					compression = "none";
+					load = <CONFIG_SYS_TEXT_BASE>;
+
+					uboot_blob: blob-ext {
+						filename = "u-boot-nodtb.bin";
+					};
+				};
+
+				opensbi {
+					description = "OpenSBI fw_dynamic Firmware";
+					type = "firmware";
+					os = "opensbi";
+					arch = "riscv";
+					compression = "none";
+					load = <CONFIG_SPL_OPENSBI_LOAD_ADDR>;
+					entry = <CONFIG_SPL_OPENSBI_LOAD_ADDR>;
+
+					opensbi_blob: blob-ext {
+						filename = "fw_dynamic.bin";
+					};
+				};
+
+				@fdt-SEQ {
+					description = "NAME";
+					type = "flat_dt";
+					compression = "none";
+				};
+			};
+
+			configurations {
+				default = "conf-1";
+
+				@conf-SEQ {
+					description = "NAME";
+					firmware = "opensbi";
+					loadables = "uboot";
+					fdt = "fdt-SEQ";
+				};
+			};
+		};
+	};
+};
diff --git a/arch/riscv/dts/hifive-unleashed-a00-u-boot.dtsi b/arch/riscv/dts/hifive-unleashed-a00-u-boot.dtsi
index 1996149c95..51b566116d 100644
--- a/arch/riscv/dts/hifive-unleashed-a00-u-boot.dtsi
+++ b/arch/riscv/dts/hifive-unleashed-a00-u-boot.dtsi
@@ -3,6 +3,7 @@
  * Copyright (C) 2019 Jagan Teki <jagan@amarulasolutions.com>
  */
 
+#include "binman.dtsi"
 #include "fu540-c000-u-boot.dtsi"
 #include "fu540-hifive-unleashed-a00-ddr.dtsi"
 
diff --git a/board/sifive/unleashed/Kconfig b/board/sifive/unleashed/Kconfig
index dbffd59c98..b08652365f 100644
--- a/board/sifive/unleashed/Kconfig
+++ b/board/sifive/unleashed/Kconfig
@@ -27,6 +27,7 @@ config BOARD_SPECIFIC_OPTIONS # dummy
 	def_bool y
 	select SIFIVE_FU540
 	select ENV_IS_IN_SPI_FLASH
+	select BINMAN if !USE_SPL_FIT_GENERATOR
 	imply CMD_DHCP
 	imply CMD_EXT2
 	imply CMD_EXT4
diff --git a/configs/sifive_unleashed_defconfig b/configs/sifive_unleashed_defconfig
index 62416a7c1d..dc9313e572 100644
--- a/configs/sifive_unleashed_defconfig
+++ b/configs/sifive_unleashed_defconfig
@@ -14,6 +14,7 @@ CONFIG_RISCV_SMODE=y
 CONFIG_DISTRO_DEFAULTS=y
 CONFIG_FIT=y
 CONFIG_SPL_LOAD_FIT_ADDRESS=0x84000000
+# CONFIG_USE_SPL_FIT_GENERATOR is not set
 CONFIG_DISPLAY_CPUINFO=y
 CONFIG_DISPLAY_BOARDINFO=y
 CONFIG_MISC_INIT_R=y
-- 
2.25.1

[PATCH 0/9] riscv: Partially move to binman to generate u-boot.itb

[Simon Glass]

Hi Bin,

On Wed, 5 May 2021 at 08:16, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> This series updates binman to handle creation of u-boot.itb image for
> RISC-V SiFive Unleashed board.
>
> QEMU RISC-V remains unchanged, as binman uses a dtb to describe the
> image format, but for QEMU RISC-V there is no dtb as dtb is passed
> to U-Boot via CONFIG_OF_PRIOR_STAGE.

That's odd. What software is providing the DTB? Not SPL?

>
> Not sure how such use case could be properly supported by binman?

Perhaps by adding a .dts file in U-Boot, or making it available
manually in some hacky way.

Regards,
Simon

[PATCH 1/9] common: kconfig: Correct a typo in SPL_LOAD_FIT

[Simon Glass]

On Wed, 5 May 2021 at 08:16, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> It should be FDT, not FTD.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  common/Kconfig.boot | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

[PATCH 2/9] binman: Correct '-a' description in the doc

[Simon Glass]

On Wed, 5 May 2021 at 08:16, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> It needs a space around '-a'.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  tools/binman/binman.rst | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

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

[PATCH 3/9] binman: Correct the comment for ATF entry type

[Simon Glass]

On Wed, 5 May 2021 at 08:16, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> This is wrongly referring to Intel ME, which should be ATF.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  tools/binman/etype/atf_bl31.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

[PATCH 4/9] binman: test: Rename 172_fit_fdt.dts to 170_fit_fdt.dts

[Simon Glass]

On Wed, 5 May 2021 at 08:16, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Currently there are 2 binman test cases using the same 172 number.
> It seems that 172_fit_fdt.dts was originally named as 170_, but
> commit c0f1ebe9c1b9 ("binman: Allow selecting default FIT configuration")
> changed its name to 172_ for no reason. Let's change it back.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  tools/binman/ftest.py                                | 12 ++++++------
>  .../binman/test/{172_fit_fdt.dts => 170_fit_fdt.dts} |  0
>  2 files changed, 6 insertions(+), 6 deletions(-)
>  rename tools/binman/test/{172_fit_fdt.dts => 170_fit_fdt.dts} (100%)

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

[PATCH 5/9] binman: test: Correct the name of 170_fit_fdt_missing_prop.dts

[Simon Glass]

Hi Bin,

On Wed, 5 May 2021 at 08:16, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> It should be 171_fit_fdt_missing_prop.dts.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  tools/binman/ftest.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Are you sure you are on upstream/master? Which commit?

CI should stop errors like this. I don't see it in my tree.

>
> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> index 08f84cd32d..b0daccbc3b 100644
> --- a/tools/binman/ftest.py
> +++ b/tools/binman/ftest.py
> @@ -3862,7 +3862,7 @@ class TestFunctional(unittest.TestCase):
>      def testFitFdtMissingProp(self):
>          """Test handling of a missing 'fit,fdt-list' property"""
>          with self.assertRaises(ValueError) as e:
> -            self._DoReadFile('170_fit_fdt_missing_prop.dts')
> +            self._DoReadFile('171_fit_fdt_missing_prop.dts')
>          self.assertIn("Generator node requires 'fit,fdt-list' property",
>                        str(e.exception))

Regards,
Simon

[PATCH 6/9] binman: Add support for RISC-V OpenSBI fw_dynamic blob

[Simon Glass]

Hi Bin,

On Wed, 5 May 2021 at 07:16, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Add an entry for RISC-V OpenSBI's 'fw_dynamic' firmware payload.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  tools/binman/entries.rst          | 13 +++++++++++++
>  tools/binman/etype/opensbi.py     | 23 +++++++++++++++++++++++
>  tools/binman/ftest.py             |  7 +++++++
>  tools/binman/test/201_opensbi.dts | 16 ++++++++++++++++
>  4 files changed, 59 insertions(+)
>  create mode 100644 tools/binman/etype/opensbi.py
>  create mode 100644 tools/binman/test/201_opensbi.dts

Is it possible to use a default filename for this, instead of having
to specify it?

>
> diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
> index f1c3b7de7a..dcac700c46 100644
> --- a/tools/binman/entries.rst
> +++ b/tools/binman/entries.rst
> @@ -761,6 +761,19 @@ binman.
>
>
>
> +Entry: opensbi: RISC-V OpenSBI fw_dynamic blob
> +----------------------------------------------
> +
> +Properties / Entry arguments:
> +    - opensbi-path: Filename of file to read into entry. This is typically
> +        called fw_dynamic.bin
> +
> +This entry holds the run-time firmware, typically started by U-Boot SPL.
> +See the U-Boot README for your architecture or board for how to use it. See
> +https://github.com/riscv/opensbi for more information about OpenSBI.
> +
> +
> +
>  Entry: powerpc-mpc85xx-bootpg-resetvec: PowerPC mpc85xx bootpg + resetvec code for U-Boot
>  -----------------------------------------------------------------------------------------
>
> diff --git a/tools/binman/etype/opensbi.py b/tools/binman/etype/opensbi.py
> new file mode 100644
> index 0000000000..74d473d535
> --- /dev/null
> +++ b/tools/binman/etype/opensbi.py
> @@ -0,0 +1,23 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +#
> +# Copyright (C) 2021, Bin Meng <bmeng.cn@gmail.com>
> +#
> +# Entry-type module for RISC-V OpenSBI binary blob
> +#
> +
> +from binman.etype.blob_named_by_arg import Entry_blob_named_by_arg
> +
> +class Entry_opensbi(Entry_blob_named_by_arg):
> +    """RISC-V OpenSBI fw_dynamic blob
> +
> +    Properties / Entry arguments:
> +        - opensbi-path: Filename of file to read into entry. This is typically
> +            called fw_dynamic.bin
> +
> +    This entry holds the run-time firmware, typically started by U-Boot SPL.
> +    See the U-Boot README for your architecture or board for how to use it. See
> +    https://github.com/riscv/opensbi for more information about OpenSBI.
> +    """
> +    def __init__(self, section, etype, node):
> +        super().__init__(section, etype, node, 'opensbi')
> +        self.external = True
> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> index b0daccbc3b..5383eec489 100644
> --- a/tools/binman/ftest.py
> +++ b/tools/binman/ftest.py
> @@ -76,6 +76,7 @@ FSP_M_DATA            = b'fsp_m'
>  FSP_S_DATA            = b'fsp_s'
>  FSP_T_DATA            = b'fsp_t'
>  ATF_BL31_DATA         = b'bl31'
> +OPENSBI_DATA          = b'opensbi'
>  SCP_DATA              = b'scp'
>  TEST_FDT1_DATA        = b'fdt1'
>  TEST_FDT2_DATA        = b'test-fdt2'
> @@ -178,6 +179,7 @@ class TestFunctional(unittest.TestCase):
>          TestFunctional._MakeInputFile('compress', COMPRESS_DATA)
>          TestFunctional._MakeInputFile('compress_big', COMPRESS_DATA_BIG)
>          TestFunctional._MakeInputFile('bl31.bin', ATF_BL31_DATA)
> +        TestFunctional._MakeInputFile('fw_dynamic.bin', OPENSBI_DATA)
>          TestFunctional._MakeInputFile('scp.bin', SCP_DATA)
>
>          # Add a few .dtb files for testing
> @@ -4535,5 +4537,10 @@ class TestFunctional(unittest.TestCase):
>          expected += tools.GetBytes(0, 88 - len(expected)) + U_BOOT_NODTB_DATA
>          self.assertEqual(expected, data)
>
> +    def testPackOpenSBI(self):
> +        """Test that an image with an OpenSBI binary can be created"""
> +        data = self._DoReadFile('201_opensbi.dts')
> +        self.assertEqual(OPENSBI_DATA, data[:len(OPENSBI_DATA)])
> +
>  if __name__ == "__main__":
>      unittest.main()
> diff --git a/tools/binman/test/201_opensbi.dts b/tools/binman/test/201_opensbi.dts
> new file mode 100644
> index 0000000000..16c01a946a
> --- /dev/null
> +++ b/tools/binman/test/201_opensbi.dts
> @@ -0,0 +1,16 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +/dts-v1/;
> +
> +/ {
> +       #address-cells = <1>;
> +       #size-cells = <1>;
> +
> +       binman {
> +               size = <16>;

Can you drop that?

> +
> +               opensbi {
> +                       filename = "fw_dynamic.bin";
> +               };
> +       };
> +};
> --
> 2.25.1
>

Regards,
Simon

[PATCH 7/9] makefile: Update clean rule to remove files generated by binman

[Simon Glass]

Hi Bin,

On Wed, 5 May 2021 at 07:16, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> When using binman to generate an FIT image, these intermediate files
> "*.fit.fit" and "*.fit.itb" are generated from mkimage, which should
> be cleaned, and git ignored.
>
> While we are here, clean the map file generated by "binman -m" as well.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  .gitignore | 4 +++-
>  Makefile   | 5 +++--
>  2 files changed, 6 insertions(+), 3 deletions(-)

The thing is, binman can emit a lot of files and this is just some of
them. We need a way to handle this in general.

Options I can think of:

- put intermedia files in a subdir
- have a way to ask binman to list the files it would have generated
for any particular board, and 'rm' them in the Makefile
- have binman write out a list of generated files into something like
binman.lst and remove those files

Regards,
Simon

[PATCH 8/9] makefile: Pass OpenSBI blob to binman make rules

[Simon Glass]

On Wed, 5 May 2021 at 08:16, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> This updates the make rules to pass OpenSBI blob to binman.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  Makefile | 1 +
>  1 file changed, 1 insertion(+)

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

I suppose at some point we should separate this into a variable that
we add to throughout the Makefile and then use $(foreach ) to add the
-a flag to each one.

[PATCH 9/9] riscv: sifive: unleashed: Switch to use binman to generate u-boot.itb

[Simon Glass]

On Wed, 5 May 2021 at 08:16, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> At present SiFive Unleashed board uses the Makefile to create the FIT,
> using USE_SPL_FIT_GENERATOR, which is deprecated as per the Makefile
> warning. Update to use binman instead.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>
> ---
>
>  arch/riscv/dts/binman.dtsi                    | 70 +++++++++++++++++++
>  .../dts/hifive-unleashed-a00-u-boot.dtsi      |  1 +
>  board/sifive/unleashed/Kconfig                |  1 +
>  configs/sifive_unleashed_defconfig            |  1 +
>  4 files changed, 73 insertions(+)
>  create mode 100644 arch/riscv/dts/binman.dtsi

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

Is it possible to migrate all the boards so as not to leave two paths in use?

[PATCH 0/9] riscv: Partially move to binman to generate u-boot.itb

[Bin Meng]

Hi Simon,

On Thu, May 6, 2021 at 7:37 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Bin,
>
> On Wed, 5 May 2021 at 08:16, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > This series updates binman to handle creation of u-boot.itb image for
> > RISC-V SiFive Unleashed board.
> >
> > QEMU RISC-V remains unchanged, as binman uses a dtb to describe the
> > image format, but for QEMU RISC-V there is no dtb as dtb is passed
> > to U-Boot via CONFIG_OF_PRIOR_STAGE.
>
> That's odd. What software is providing the DTB? Not SPL?

QEMU itself creates DTB on the fly, based on command parameters passed
to QEMU. The DTB address will be passed to U-Boot by QEMU.

> >
> > Not sure how such use case could be properly supported by binman?
>
> Perhaps by adding a .dts file in U-Boot, or making it available
> manually in some hacky way.

I will see if I can do some hacky way :( Not sure if it brings any
improvements compared to current CONFIG_SPL_FIT_GENERATOR scripts.

Regards,
Bin

[PATCH 0/9] riscv: Partially move to binman to generate u-boot.itb

[Simon Glass]

Hi Bin,

On Wed, 5 May 2021 at 19:42, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Thu, May 6, 2021 at 7:37 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Bin,
> >
> > On Wed, 5 May 2021 at 08:16, Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > This series updates binman to handle creation of u-boot.itb image for
> > > RISC-V SiFive Unleashed board.
> > >
> > > QEMU RISC-V remains unchanged, as binman uses a dtb to describe the
> > > image format, but for QEMU RISC-V there is no dtb as dtb is passed
> > > to U-Boot via CONFIG_OF_PRIOR_STAGE.
> >
> > That's odd. What software is providing the DTB? Not SPL?
>
> QEMU itself creates DTB on the fly, based on command parameters passed
> to QEMU. The DTB address will be passed to U-Boot by QEMU.

OK. So the canonical DTB is in qemu? Then I think we need at least
something in the U-Boot tree too, as you have done.

>
> > >
> > > Not sure how such use case could be properly supported by binman?
> >
> > Perhaps by adding a .dts file in U-Boot, or making it available
> > manually in some hacky way.
>
> I will see if I can do some hacky way :( Not sure if it brings any
> improvements compared to current CONFIG_SPL_FIT_GENERATOR scripts.

We should drop these scripts and use binman, as your patch does.

Regards,
Simon

[PATCH 7/9] makefile: Update clean rule to remove files generated by binman

[Bin Meng]

Hi Simon,

On Thu, May 6, 2021 at 7:38 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Bin,
>
> On Wed, 5 May 2021 at 07:16, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > When using binman to generate an FIT image, these intermediate files
> > "*.fit.fit" and "*.fit.itb" are generated from mkimage, which should
> > be cleaned, and git ignored.
> >
> > While we are here, clean the map file generated by "binman -m" as well.
> >
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> >
> >  .gitignore | 4 +++-
> >  Makefile   | 5 +++--
> >  2 files changed, 6 insertions(+), 3 deletions(-)
>
> The thing is, binman can emit a lot of files and this is just some of
> them. We need a way to handle this in general.
>
> Options I can think of:
>
> - put intermedia files in a subdir
> - have a way to ask binman to list the files it would have generated
> for any particular board, and 'rm' them in the Makefile
> - have binman write out a list of generated files into something like
> binman.lst and remove those files

For binman generated files, these are options we can look at. But I am
not sure if the same applies to mkimage generated ones? Is it worth
changing in this series?

Regards,
Bin

[PATCH 7/9] makefile: Update clean rule to remove files generated by binman

[Simon Glass]

Hi Bin,

On Fri, 7 May 2021 at 08:50, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Thu, May 6, 2021 at 7:38 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Bin,
> >
> > On Wed, 5 May 2021 at 07:16, Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > When using binman to generate an FIT image, these intermediate files
> > > "*.fit.fit" and "*.fit.itb" are generated from mkimage, which should
> > > be cleaned, and git ignored.
> > >
> > > While we are here, clean the map file generated by "binman -m" as well.
> > >
> > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > > ---
> > >
> > >  .gitignore | 4 +++-
> > >  Makefile   | 5 +++--
> > >  2 files changed, 6 insertions(+), 3 deletions(-)
> >
> > The thing is, binman can emit a lot of files and this is just some of
> > them. We need a way to handle this in general.
> >
> > Options I can think of:
> >
> > - put intermedia files in a subdir
> > - have a way to ask binman to list the files it would have generated
> > for any particular board, and 'rm' them in the Makefile
> > - have binman write out a list of generated files into something like
> > binman.lst and remove those files
>
> For binman generated files, these are options we can look at. But I am
> not sure if the same applies to mkimage generated ones? Is it worth
> changing in this series?

But isn't mkimage invoked by binman?

I think this patch is best skipped if we cannot solve the problem
fully. We can return to it another time perhaps.

Regards,
Simon

[PATCH 6/9] binman: Add support for RISC-V OpenSBI fw_dynamic blob

[Bin Meng]

Hi Simon,

On Thu, May 6, 2021 at 7:37 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Bin,
>
> On Wed, 5 May 2021 at 07:16, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Add an entry for RISC-V OpenSBI's 'fw_dynamic' firmware payload.
> >
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> >
> >  tools/binman/entries.rst          | 13 +++++++++++++
> >  tools/binman/etype/opensbi.py     | 23 +++++++++++++++++++++++
> >  tools/binman/ftest.py             |  7 +++++++
> >  tools/binman/test/201_opensbi.dts | 16 ++++++++++++++++
> >  4 files changed, 59 insertions(+)
> >  create mode 100644 tools/binman/etype/opensbi.py
> >  create mode 100644 tools/binman/test/201_opensbi.dts
>
> Is it possible to use a default filename for this, instead of having
> to specify it?

I don't get what you meant. Would you please clarify?

Regards,
Bin

[PATCH 6/9] binman: Add support for RISC-V OpenSBI fw_dynamic blob

[Simon Glass]

Hi Bin,

On Fri, 7 May 2021 at 18:29, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Thu, May 6, 2021 at 7:37 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Bin,
> >
> > On Wed, 5 May 2021 at 07:16, Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > Add an entry for RISC-V OpenSBI's 'fw_dynamic' firmware payload.
> > >
> > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > > ---
> > >
> > >  tools/binman/entries.rst          | 13 +++++++++++++
> > >  tools/binman/etype/opensbi.py     | 23 +++++++++++++++++++++++
> > >  tools/binman/ftest.py             |  7 +++++++
> > >  tools/binman/test/201_opensbi.dts | 16 ++++++++++++++++
> > >  4 files changed, 59 insertions(+)
> > >  create mode 100644 tools/binman/etype/opensbi.py
> > >  create mode 100644 tools/binman/test/201_opensbi.dts
> >
> > Is it possible to use a default filename for this, instead of having
> > to specify it?
>
> I don't get what you meant. Would you please clarify?

I mean adding a GetDefaultFilename() argument. But I see that you are
using a -a parameter, so perhaps there is no point.

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

Regards,
Simon