All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] rockchip: Align FIT images to SD/MMC block length
@ 2023-01-17 22:54 Jonas Karlman
  2023-01-17 22:54 ` [PATCH 1/4] binman: Add support for align argument to mkimage tool Jonas Karlman
                   ` (4 more replies)
  0 siblings, 5 replies; 39+ messages in thread
From: Jonas Karlman @ 2023-01-17 22:54 UTC (permalink / raw)
  To: Simon Glass, Philipp Tomsich, Kever Yang, Alper Nebi Yasak
  Cc: Quentin Schulz, u-boot, Jonas Karlman

When I was trying to run mainline U-Boot on my new Rockchip RK3568 board
I discovered that one segment of vendor TF-A could not successfully be
loaded into SRAM, validation of the image sha256 hash failed.

The issue with loading the data turned out to be because of how SPL load
FIT images. It reads data aligned to block length. Aligned image data is
read directly to the load address. Unaligned image data is written to an
offset of the load address and then memcpy to the load address.

The segment of the TF-A that failed to load is a 8KiB segment that
should be loaded into the 8KiB PMU SRAM. Because this segment was
unaligned to the SD/MMC block length the segment was written to and
memcpy from beyond the SRAM boundary, in the end this results in invalid
data in SRAM.

Vendor u-boot has worked around this issue by using a bounce buffer for
any load address that is not in the gd->ram_base - gd->ram_top range.

However, by passing -B 200 to mkimage we can work around this issue
becuase the FIT and its external data ends up being aligned to SD/MMC
block length.

This series adds support for a fit,align property in binman and makes
use of this new property in rockchip-u-boot.dtsi.
It also adds image hash to the FIT images when configured with
CONFIG_SPL_FIT_SIGNATURE=y.

This series builds on top of Simon Glass "binman: rockchip:
Migrate from rockchip SPL_FIT_GENERATOR script" v9 series at [1].

[1] https://patchwork.ozlabs.org/project/uboot/list/?series=335487

Jonas Karlman (4):
  binman: Add support for align argument to mkimage tool
  rockchip: Align FIT image data to SD/MMC block length
  binman: Add subnodes to the nodes generated by split-elf
  rockchip: Add sha256 hash to FIT images

 arch/arm/dts/rockchip-u-boot.dtsi | 21 +++++++++++++++++++++
 tools/binman/btool/mkimage.py     |  5 ++++-
 tools/binman/etype/fit.py         | 17 +++++++++++++++--
 3 files changed, 40 insertions(+), 3 deletions(-)

-- 
2.39.0


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

* [PATCH 1/4] binman: Add support for align argument to mkimage tool
  2023-01-17 22:54 [PATCH 0/4] rockchip: Align FIT images to SD/MMC block length Jonas Karlman
@ 2023-01-17 22:54 ` Jonas Karlman
  2023-01-18 19:42   ` Simon Glass
  2023-01-17 22:54 ` [PATCH 2/4] rockchip: Align FIT image data to SD/MMC block length Jonas Karlman
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 39+ messages in thread
From: Jonas Karlman @ 2023-01-17 22:54 UTC (permalink / raw)
  To: Simon Glass, Philipp Tomsich, Kever Yang, Alper Nebi Yasak
  Cc: Quentin Schulz, u-boot, Jonas Karlman

Add support to indicate what alignment to use for the FIT and its
external data. Pass the alignment to mkimage via the -B flag.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 tools/binman/btool/mkimage.py | 5 ++++-
 tools/binman/etype/fit.py     | 8 ++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/tools/binman/btool/mkimage.py b/tools/binman/btool/mkimage.py
index da5f344162..663ed16b65 100644
--- a/tools/binman/btool/mkimage.py
+++ b/tools/binman/btool/mkimage.py
@@ -22,7 +22,7 @@ class Bintoolmkimage(bintool.Bintool):
 
     # pylint: disable=R0913
     def run(self, reset_timestamp=False, output_fname=None, external=False,
-            pad=None):
+            pad=None, align=None):
         """Run mkimage
 
         Args:
@@ -33,6 +33,7 @@ class Bintoolmkimage(bintool.Bintool):
             pad: Bytes to use for padding the FIT devicetree output. This allows
                 other things to be easily added later, if required, such as
                 signatures
+            align: Bytes to use for alignment of the FIT and its external data
             version: True to get the mkimage version
         """
         args = []
@@ -40,6 +41,8 @@ class Bintoolmkimage(bintool.Bintool):
             args.append('-E')
         if pad:
             args += ['-p', f'{pad:x}']
+        if align:
+            args += ['-B', f'{align:x}']
         if reset_timestamp:
             args.append('-t')
         if output_fname:
diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
index f0e3fd1a09..deec27bee3 100644
--- a/tools/binman/etype/fit.py
+++ b/tools/binman/etype/fit.py
@@ -70,6 +70,11 @@ class Entry_fit(Entry_section):
             Indicates that the contents of the FIT are external and provides the
             external offset. This is passed to mkimage via the -E and -p flags.
 
+        fit,align
+            Indicates what alignment to use for the FIT and its external data,
+            and provides the alignment to use. This is passed to mkimage via
+            the -B flag.
+
         fit,fdt-list
             Indicates the entry argument which provides the list of device tree
             files for the gen-fdt-nodes operation (as below). This is often
@@ -423,6 +428,9 @@ class Entry_fit(Entry_section):
                 'external': True,
                 'pad': fdt_util.fdt32_to_cpu(ext_offset.value)
                 }
+        align = self._fit_props.get('fit,align')
+        if align is not None:
+            args.update({'align': fdt_util.fdt32_to_cpu(align.value)})
         if self.mkimage.run(reset_timestamp=True, output_fname=output_fname,
                             **args) is None:
             # Bintool is missing; just use empty data as the output
-- 
2.39.0


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

* [PATCH 2/4] rockchip: Align FIT image data to SD/MMC block length
  2023-01-17 22:54 [PATCH 0/4] rockchip: Align FIT images to SD/MMC block length Jonas Karlman
  2023-01-17 22:54 ` [PATCH 1/4] binman: Add support for align argument to mkimage tool Jonas Karlman
@ 2023-01-17 22:54 ` Jonas Karlman
  2023-01-18 19:42   ` Simon Glass
  2023-01-17 22:54 ` [PATCH 3/4] binman: Add subnodes to the nodes generated by split-elf Jonas Karlman
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 39+ messages in thread
From: Jonas Karlman @ 2023-01-17 22:54 UTC (permalink / raw)
  To: Simon Glass, Philipp Tomsich, Kever Yang, Alper Nebi Yasak
  Cc: Quentin Schulz, u-boot, Jonas Karlman

SPL load FIT images by reading the data aligned to block length.
Block length aligned image data is read directly to the load address.
Unaligned image data is written to an offset of the load address and
then the data is memcpy to the load address.

This adds a small overhead of having to memcpy unaligned data, something
that normally is not an issue.

However, TF-A may have a segment that should be loaded into SRAM, e.g.
vendor TF-A for RK3568 has a 8KiB segment that should be loaded into the
8KiB PMU SRAM. Having the image data for such segment unaligned result
in segment being written to and memcpy from beyond the SRAM boundary, in
the end this results in invalid data in SRAM.

Aligning the FIT and its external data to MMC block length to work
around such issue.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 arch/arm/dts/rockchip-u-boot.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
index 234fc5df43..63c8da456b 100644
--- a/arch/arm/dts/rockchip-u-boot.dtsi
+++ b/arch/arm/dts/rockchip-u-boot.dtsi
@@ -37,6 +37,7 @@
 			fit,fdt-list = "of-list";
 			filename = "u-boot.itb";
 			fit,external-offset = <CONFIG_FIT_EXTERNAL_OFFSET>;
+			fit,align = <512>;
 			offset = <CONFIG_SPL_PAD_TO>;
 			images {
 				u-boot {
-- 
2.39.0


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

* [PATCH 3/4] binman: Add subnodes to the nodes generated by split-elf
  2023-01-17 22:54 [PATCH 0/4] rockchip: Align FIT images to SD/MMC block length Jonas Karlman
  2023-01-17 22:54 ` [PATCH 1/4] binman: Add support for align argument to mkimage tool Jonas Karlman
  2023-01-17 22:54 ` [PATCH 2/4] rockchip: Align FIT image data to SD/MMC block length Jonas Karlman
@ 2023-01-17 22:54 ` Jonas Karlman
  2023-01-18 19:42   ` Simon Glass
  2023-01-17 22:55 ` [PATCH 4/4] rockchip: Add sha256 hash to FIT images Jonas Karlman
  2023-01-20  8:26 ` [PATCH v2 0/6] rockchip: Align FIT images to SD/MMC block length Jonas Karlman
  4 siblings, 1 reply; 39+ messages in thread
From: Jonas Karlman @ 2023-01-17 22:54 UTC (permalink / raw)
  To: Simon Glass, Philipp Tomsich, Kever Yang, Alper Nebi Yasak
  Cc: Quentin Schulz, u-boot, Jonas Karlman

Add hash and signature nodes to generated nodes by split-elf operation.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 tools/binman/etype/fit.py | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
index deec27bee3..fb27c8877e 100644
--- a/tools/binman/etype/fit.py
+++ b/tools/binman/etype/fit.py
@@ -548,12 +548,13 @@ class Entry_fit(Entry_section):
                     else:
                         self.Raise("Generator node requires 'fit,fdt-list' property")
 
-        def _gen_split_elf(base_node, node, segments, entry_addr):
+        def _gen_split_elf(base_node, node, depth, segments, entry_addr):
             """Add nodes for the ELF file, one per group of contiguous segments
 
             Args:
                 base_node (Node): Template node from the binman definition
                 node (Node): Node to replace (in the FIT being built)
+                depth: Current node depth (0 is the base 'fit' node)
                 segments (list): list of segments, each:
                     int: Segment number (0 = first)
                     int: Start address of segment in memory
@@ -578,6 +579,10 @@ class Entry_fit(Entry_section):
                             self._raise_subnode(
                                 node, f"Unknown directive '{pname}'")
 
+                    for subnode in node.subnodes:
+                        with fsw.add_node(subnode.name):
+                            _add_node(node, depth + 1, subnode)
+
         def _gen_node(base_node, node, depth, in_images, entry):
             """Generate nodes from a template
 
@@ -631,7 +636,7 @@ class Entry_fit(Entry_section):
                             self._raise_subnode(
                                 node, f'Failed to read ELF file: {str(exc)}')
 
-                    _gen_split_elf(base_node, node, segments, entry_addr)
+                    _gen_split_elf(base_node, node, depth, segments, entry_addr)
 
         def _add_node(base_node, depth, node):
             """Add nodes to the output FIT
-- 
2.39.0


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

* [PATCH 4/4] rockchip: Add sha256 hash to FIT images
  2023-01-17 22:54 [PATCH 0/4] rockchip: Align FIT images to SD/MMC block length Jonas Karlman
                   ` (2 preceding siblings ...)
  2023-01-17 22:54 ` [PATCH 3/4] binman: Add subnodes to the nodes generated by split-elf Jonas Karlman
@ 2023-01-17 22:55 ` Jonas Karlman
  2023-01-18 19:42   ` Simon Glass
  2023-01-20  8:26 ` [PATCH v2 0/6] rockchip: Align FIT images to SD/MMC block length Jonas Karlman
  4 siblings, 1 reply; 39+ messages in thread
From: Jonas Karlman @ 2023-01-17 22:55 UTC (permalink / raw)
  To: Simon Glass, Philipp Tomsich, Kever Yang, Alper Nebi Yasak
  Cc: Quentin Schulz, u-boot, Jonas Karlman

Add sha256 hash to FIT images when CONFIG_SPL_FIT_SIGNATURE=y.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 arch/arm/dts/rockchip-u-boot.dtsi | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
index 63c8da456b..e35902bb63 100644
--- a/arch/arm/dts/rockchip-u-boot.dtsi
+++ b/arch/arm/dts/rockchip-u-boot.dtsi
@@ -50,6 +50,11 @@
 					entry = <CONFIG_TEXT_BASE>;
 					u-boot-nodtb {
 					};
+#ifdef CONFIG_SPL_FIT_SIGNATURE
+					hash {
+						algo = "sha256";
+					};
+#endif
 				};
 
 				@atf-SEQ {
@@ -65,6 +70,11 @@
 
 					atf-bl31 {
 					};
+#ifdef CONFIG_SPL_FIT_SIGNATURE
+					hash {
+						algo = "sha256";
+					};
+#endif
 				};
 				@tee-SEQ {
 					fit,operation = "split-elf";
@@ -80,12 +90,22 @@
 					tee-os {
 						optional;
 					};
+#ifdef CONFIG_SPL_FIT_SIGNATURE
+					hash {
+						algo = "sha256";
+					};
+#endif
 				};
 
 				@fdt-SEQ {
 					description = "fdt-NAME";
 					compression = "none";
 					type = "flat_dt";
+#ifdef CONFIG_SPL_FIT_SIGNATURE
+					hash {
+						algo = "sha256";
+					};
+#endif
 				};
 			};
 
-- 
2.39.0


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

* Re: [PATCH 1/4] binman: Add support for align argument to mkimage tool
  2023-01-17 22:54 ` [PATCH 1/4] binman: Add support for align argument to mkimage tool Jonas Karlman
@ 2023-01-18 19:42   ` Simon Glass
  2023-01-19 12:40     ` Jonas Karlman
  0 siblings, 1 reply; 39+ messages in thread
From: Simon Glass @ 2023-01-18 19:42 UTC (permalink / raw)
  To: Jonas Karlman
  Cc: Philipp Tomsich, Kever Yang, Alper Nebi Yasak, Quentin Schulz, u-boot

Hi Jonas,

On Tue, 17 Jan 2023 at 15:54, Jonas Karlman <jonas@kwiboo.se> wrote:
>
> Add support to indicate what alignment to use for the FIT and its
> external data. Pass the alignment to mkimage via the -B flag.
>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>  tools/binman/btool/mkimage.py | 5 ++++-
>  tools/binman/etype/fit.py     | 8 ++++++++
>  2 files changed, 12 insertions(+), 1 deletion(-)

This looks good to me but please update docs at the top of fit.py,
regenerate entries.rst  and add a test to ftest.py here, as this
causes a test-coverage failure:

$ binman test -T
...
tools/binman/etype/fit.py                                 224      1    99%
tools/binman/btool/mkimage.py                              23      1    96%

Regards,
Simon

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

* Re: [PATCH 2/4] rockchip: Align FIT image data to SD/MMC block length
  2023-01-17 22:54 ` [PATCH 2/4] rockchip: Align FIT image data to SD/MMC block length Jonas Karlman
@ 2023-01-18 19:42   ` Simon Glass
  0 siblings, 0 replies; 39+ messages in thread
From: Simon Glass @ 2023-01-18 19:42 UTC (permalink / raw)
  To: Jonas Karlman
  Cc: Philipp Tomsich, Kever Yang, Alper Nebi Yasak, Quentin Schulz, u-boot

On Tue, 17 Jan 2023 at 15:54, Jonas Karlman <jonas@kwiboo.se> wrote:
>
> SPL load FIT images by reading the data aligned to block length.
> Block length aligned image data is read directly to the load address.
> Unaligned image data is written to an offset of the load address and
> then the data is memcpy to the load address.
>
> This adds a small overhead of having to memcpy unaligned data, something
> that normally is not an issue.
>
> However, TF-A may have a segment that should be loaded into SRAM, e.g.
> vendor TF-A for RK3568 has a 8KiB segment that should be loaded into the
> 8KiB PMU SRAM. Having the image data for such segment unaligned result
> in segment being written to and memcpy from beyond the SRAM boundary, in
> the end this results in invalid data in SRAM.
>
> Aligning the FIT and its external data to MMC block length to work
> around such issue.
>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>  arch/arm/dts/rockchip-u-boot.dtsi | 1 +
>  1 file changed, 1 insertion(+)
>

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

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

* Re: [PATCH 3/4] binman: Add subnodes to the nodes generated by split-elf
  2023-01-17 22:54 ` [PATCH 3/4] binman: Add subnodes to the nodes generated by split-elf Jonas Karlman
@ 2023-01-18 19:42   ` Simon Glass
  0 siblings, 0 replies; 39+ messages in thread
From: Simon Glass @ 2023-01-18 19:42 UTC (permalink / raw)
  To: Jonas Karlman
  Cc: Philipp Tomsich, Kever Yang, Alper Nebi Yasak, Quentin Schulz, u-boot

Hi Jonas,

On Tue, 17 Jan 2023 at 15:55, Jonas Karlman <jonas@kwiboo.se> wrote:
>
> Add hash and signature nodes to generated nodes by split-elf operation.
>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>  tools/binman/etype/fit.py | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
> index deec27bee3..fb27c8877e 100644
> --- a/tools/binman/etype/fit.py
> +++ b/tools/binman/etype/fit.py
> @@ -548,12 +548,13 @@ class Entry_fit(Entry_section):
>                      else:
>                          self.Raise("Generator node requires 'fit,fdt-list' property")
>
> -        def _gen_split_elf(base_node, node, segments, entry_addr):
> +        def _gen_split_elf(base_node, node, depth, segments, entry_addr):
>              """Add nodes for the ELF file, one per group of contiguous segments
>
>              Args:
>                  base_node (Node): Template node from the binman definition
>                  node (Node): Node to replace (in the FIT being built)
> +                depth: Current node depth (0 is the base 'fit' node)
>                  segments (list): list of segments, each:
>                      int: Segment number (0 = first)
>                      int: Start address of segment in memory
> @@ -578,6 +579,10 @@ class Entry_fit(Entry_section):
>                              self._raise_subnode(
>                                  node, f"Unknown directive '{pname}'")
>
> +                    for subnode in node.subnodes:
> +                        with fsw.add_node(subnode.name):
> +                            _add_node(node, depth + 1, subnode)
> +
>          def _gen_node(base_node, node, depth, in_images, entry):
>              """Generate nodes from a template
>
> @@ -631,7 +636,7 @@ class Entry_fit(Entry_section):
>                              self._raise_subnode(
>                                  node, f'Failed to read ELF file: {str(exc)}')
>
> -                    _gen_split_elf(base_node, node, segments, entry_addr)
> +                    _gen_split_elf(base_node, node, depth, segments, entry_addr)
>
>          def _add_node(base_node, depth, node):
>              """Add nodes to the output FIT
> --
> 2.39.0
>

Please can you also update the docs at the top of the file to indicate
what happened (and regen entries.rst).

Also, can you update an existing test to check that at least one
subnode is created? Perhaps testFitSplitElf() ?

Regards,
Simon

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

* Re: [PATCH 4/4] rockchip: Add sha256 hash to FIT images
  2023-01-17 22:55 ` [PATCH 4/4] rockchip: Add sha256 hash to FIT images Jonas Karlman
@ 2023-01-18 19:42   ` Simon Glass
  0 siblings, 0 replies; 39+ messages in thread
From: Simon Glass @ 2023-01-18 19:42 UTC (permalink / raw)
  To: Jonas Karlman
  Cc: Philipp Tomsich, Kever Yang, Alper Nebi Yasak, Quentin Schulz, u-boot

On Tue, 17 Jan 2023 at 15:55, Jonas Karlman <jonas@kwiboo.se> wrote:
>
> Add sha256 hash to FIT images when CONFIG_SPL_FIT_SIGNATURE=y.
>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>  arch/arm/dts/rockchip-u-boot.dtsi | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>

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

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

* Re: [PATCH 1/4] binman: Add support for align argument to mkimage tool
  2023-01-18 19:42   ` Simon Glass
@ 2023-01-19 12:40     ` Jonas Karlman
  2023-01-19 16:20       ` Simon Glass
  0 siblings, 1 reply; 39+ messages in thread
From: Jonas Karlman @ 2023-01-19 12:40 UTC (permalink / raw)
  To: Simon Glass
  Cc: Philipp Tomsich, Kever Yang, Alper Nebi Yasak, Quentin Schulz, u-boot

Hi Simon,

On 2023-01-18 20:42, Simon Glass wrote:
> Hi Jonas,
> 
> On Tue, 17 Jan 2023 at 15:54, Jonas Karlman <jonas@kwiboo.se> wrote:
>>
>> Add support to indicate what alignment to use for the FIT and its
>> external data. Pass the alignment to mkimage via the -B flag.
>>
>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>> ---
>>  tools/binman/btool/mkimage.py | 5 ++++-
>>  tools/binman/etype/fit.py     | 8 ++++++++
>>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> This looks good to me but please update docs at the top of fit.py,
> regenerate entries.rst  and add a test to ftest.py here, as this
> causes a test-coverage failure:

I will take a look and send a v2 series that includes tests,
updated entries.rst and is rebased on top of dm-pull-18jan23.


I have noticed that the generated FIT configuration node is not fully
matching the old script, it uses firmware = "u-boot" instead of "atf-1".
So TF-A is never initialized with the new FIT.

I made a quick attempt of a fix using the the diff below.

Now with binman:
firmware = "u-boot";
loadables = "atf-1", "atf-2", ...;

With old script:
firmware = "atf-1";
loadables = "u-boot", "atf-2", ...;

With the diff below:
firmware = "atf-1";
loadables = "u-boot", "atf-1", "atf-2", ...;

Should I send patches with something like this? Or do you have something else in mind?

Regards,
Jonas


diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
index 1449657e0a..1e8333753e 100644
--- a/arch/arm/dts/rockchip-u-boot.dtsi
+++ b/arch/arm/dts/rockchip-u-boot.dtsi
@@ -116,8 +116,8 @@
                                @config-SEQ {
                                        description = "NAME.dtb";
                                        fdt = "fdt-SEQ";
-                                       firmware = "u-boot";
-                                       fit,loadables;
+                                       firmware = "atf-1";
+                                       fit,loadables = "u-boot";
                                };
                        };
                };
diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
index 51b122962f..21ec9e5cc7 100644
--- a/tools/binman/etype/fit.py
+++ b/tools/binman/etype/fit.py
@@ -525,7 +525,13 @@ class Entry_fit(Entry_section):
                     with fsw.add_node(node_name):
                         for pname, prop in node.props.items():
                             if pname == 'fit,loadables':
-                                val = '\0'.join(self._loadables) + '\0'
+                                if type(prop.value) is str:
+                                    val = [prop.value]
+                                elif type(prop.value) is list:
+                                    val = prop.value
+                                else:
+                                    val = []
+                                val = '\0'.join(val + self._loadables) + '\0'
                                 fsw.property('loadables', val.encode('utf-8'))
                             elif pname == 'fit,operation':
                                 pass



> 
> $ binman test -T
> ...
> tools/binman/etype/fit.py                                 224      1    99%
> tools/binman/btool/mkimage.py                              23      1    96%
> 
> Regards,
> Simon


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

* Re: [PATCH 1/4] binman: Add support for align argument to mkimage tool
  2023-01-19 12:40     ` Jonas Karlman
@ 2023-01-19 16:20       ` Simon Glass
  0 siblings, 0 replies; 39+ messages in thread
From: Simon Glass @ 2023-01-19 16:20 UTC (permalink / raw)
  To: Jonas Karlman
  Cc: Philipp Tomsich, Kever Yang, Alper Nebi Yasak, Quentin Schulz, u-boot

Hi Jonas,

On Thu, 19 Jan 2023 at 05:40, Jonas Karlman <jonas@kwiboo.se> wrote:
>
> Hi Simon,
>
> On 2023-01-18 20:42, Simon Glass wrote:
> > Hi Jonas,
> >
> > On Tue, 17 Jan 2023 at 15:54, Jonas Karlman <jonas@kwiboo.se> wrote:
> >>
> >> Add support to indicate what alignment to use for the FIT and its
> >> external data. Pass the alignment to mkimage via the -B flag.
> >>
> >> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> >> ---
> >>  tools/binman/btool/mkimage.py | 5 ++++-
> >>  tools/binman/etype/fit.py     | 8 ++++++++
> >>  2 files changed, 12 insertions(+), 1 deletion(-)
> >
> > This looks good to me but please update docs at the top of fit.py,
> > regenerate entries.rst  and add a test to ftest.py here, as this
> > causes a test-coverage failure:
>
> I will take a look and send a v2 series that includes tests,
> updated entries.rst and is rebased on top of dm-pull-18jan23.
>
>
> I have noticed that the generated FIT configuration node is not fully
> matching the old script, it uses firmware = "u-boot" instead of "atf-1".
> So TF-A is never initialized with the new FIT.
>
> I made a quick attempt of a fix using the the diff below.
>
> Now with binman:
> firmware = "u-boot";
> loadables = "atf-1", "atf-2", ...;
>
> With old script:
> firmware = "atf-1";
> loadables = "u-boot", "atf-2", ...;
>
> With the diff below:
> firmware = "atf-1";
> loadables = "u-boot", "atf-1", "atf-2", ...;
>
> Should I send patches with something like this? Or do you have something else in mind?

Yes please!

>
> Regards,
> Jonas
>
>
> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
> index 1449657e0a..1e8333753e 100644
> --- a/arch/arm/dts/rockchip-u-boot.dtsi
> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
> @@ -116,8 +116,8 @@
>                                 @config-SEQ {
>                                         description = "NAME.dtb";
>                                         fdt = "fdt-SEQ";
> -                                       firmware = "u-boot";
> -                                       fit,loadables;
> +                                       firmware = "atf-1";
> +                                       fit,loadables = "u-boot";
>                                 };
>                         };
>                 };
> diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
> index 51b122962f..21ec9e5cc7 100644
> --- a/tools/binman/etype/fit.py
> +++ b/tools/binman/etype/fit.py
> @@ -525,7 +525,13 @@ class Entry_fit(Entry_section):
>                      with fsw.add_node(node_name):
>                          for pname, prop in node.props.items():
>                              if pname == 'fit,loadables':
> -                                val = '\0'.join(self._loadables) + '\0'
> +                                if type(prop.value) is str:
> +                                    val = [prop.value]
> +                                elif type(prop.value) is list:
> +                                    val = prop.value
> +                                else:
> +                                    val = []
> +                                val = '\0'.join(val + self._loadables) + '\0'
>                                  fsw.property('loadables', val.encode('utf-8'))
>                              elif pname == 'fit,operation':
>                                  pass
>
>
>
> >
> > $ binman test -T
> > ...
> > tools/binman/etype/fit.py                                 224      1    99%
> > tools/binman/btool/mkimage.py                              23      1    96%
> >
> > Regards,
> > Simon
>

Regards,
Simon

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

* [PATCH v2 0/6] rockchip: Align FIT images to SD/MMC block length
  2023-01-17 22:54 [PATCH 0/4] rockchip: Align FIT images to SD/MMC block length Jonas Karlman
                   ` (3 preceding siblings ...)
  2023-01-17 22:55 ` [PATCH 4/4] rockchip: Add sha256 hash to FIT images Jonas Karlman
@ 2023-01-20  8:26 ` Jonas Karlman
  2023-01-20  8:26   ` [PATCH v2 2/6] rockchip: Align FIT image data " Jonas Karlman
                     ` (6 more replies)
  4 siblings, 7 replies; 39+ messages in thread
From: Jonas Karlman @ 2023-01-20  8:26 UTC (permalink / raw)
  To: Simon Glass, Philipp Tomsich, Kever Yang, Alper Nebi Yasak
  Cc: Quentin Schulz, u-boot, Jonas Karlman

When I was trying to run mainline U-Boot on my new Rockchip RK3568 board
I discovered that one segment of vendor TF-A could not successfully be
loaded into SRAM, validation of the image sha256 hash failed.

The issue with loading the data turned out to be because of how SPL load
FIT images. It reads data aligned to block length. Aligned image data is
read directly to the load address. Unaligned image data is written to an
offset of the load address and then memcpy to the load address.

The segment of the TF-A that failed to load is a 8KiB segment that
should be loaded into the 8KiB PMU SRAM. Because this segment was
unaligned to the SD/MMC block length the segment was written to and
memcpy from beyond the SRAM boundary, in the end this results in invalid
data in SRAM.

Vendor u-boot has worked around this issue by using a bounce buffer for
any load address that is not in the gd->ram_base - gd->ram_top range.

However, by passing -B 200 to mkimage we can work around this issue
because the FIT and its external data ends up being aligned to SD/MMC
block length.

This series adds support for a fit,align property in binman and makes
use of this new property in rockchip-u-boot.dtsi.
It also adds image hash to the FIT images when configured with
CONFIG_SPL_FIT_SIGNATURE=y.

Changes in v2:
- Added tests
- Updated entries.rst
- Collect r-b tags
- Rebased on top of dm-pull-18jan23
- New patch to fix initializing of TF-A

Jonas Karlman (6):
  binman: Add support for align argument to mkimage tool
  rockchip: Align FIT image data to SD/MMC block length
  binman: Add special subnodes to the nodes generated by split-elf
  rockchip: Add sha256 hash to FIT images
  binman: Add support for prepend loadables with split-elf
  rockchip: Use atf as firmware and move u-boot to loadables in FIT

 arch/arm/dts/rockchip-u-boot.dtsi       | 25 ++++++-
 tools/binman/btool/mkimage.py           |  5 +-
 tools/binman/entries.rst                | 24 ++++++-
 tools/binman/etype/fit.py               | 44 +++++++++++--
 tools/binman/ftest.py                   | 65 ++++++++++++++++++
 tools/binman/test/226_fit_split_elf.dts |  6 ++
 tools/binman/test/275_fit_align.dts     | 59 +++++++++++++++++
 tools/binman/test/276_fit_loadables.dts | 87 +++++++++++++++++++++++++
 8 files changed, 305 insertions(+), 10 deletions(-)
 create mode 100644 tools/binman/test/275_fit_align.dts
 create mode 100644 tools/binman/test/276_fit_loadables.dts

-- 
2.39.1


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

* [PATCH v2 1/6] binman: Add support for align argument to mkimage tool
  2023-01-20  8:26 ` [PATCH v2 0/6] rockchip: Align FIT images to SD/MMC block length Jonas Karlman
  2023-01-20  8:26   ` [PATCH v2 2/6] rockchip: Align FIT image data " Jonas Karlman
@ 2023-01-20  8:26   ` Jonas Karlman
  2023-01-20 19:19     ` Simon Glass
  2023-01-20  8:26   ` [PATCH v2 3/6] binman: Add special subnodes to the nodes generated by split-elf Jonas Karlman
                     ` (4 subsequent siblings)
  6 siblings, 1 reply; 39+ messages in thread
From: Jonas Karlman @ 2023-01-20  8:26 UTC (permalink / raw)
  To: Simon Glass, Philipp Tomsich, Kever Yang, Alper Nebi Yasak
  Cc: Quentin Schulz, u-boot, Jonas Karlman

Add support to indicate what alignment to use for the FIT and its
external data. Pass the alignment to mkimage via the -B flag.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
v2:
- Add test
- Update entries.rst

 tools/binman/btool/mkimage.py       |  5 ++-
 tools/binman/entries.rst            |  5 +++
 tools/binman/etype/fit.py           |  8 ++++
 tools/binman/ftest.py               | 16 ++++++++
 tools/binman/test/275_fit_align.dts | 59 +++++++++++++++++++++++++++++
 5 files changed, 92 insertions(+), 1 deletion(-)
 create mode 100644 tools/binman/test/275_fit_align.dts

diff --git a/tools/binman/btool/mkimage.py b/tools/binman/btool/mkimage.py
index da5f344162..d5b407c554 100644
--- a/tools/binman/btool/mkimage.py
+++ b/tools/binman/btool/mkimage.py
@@ -22,7 +22,7 @@ class Bintoolmkimage(bintool.Bintool):
 
     # pylint: disable=R0913
     def run(self, reset_timestamp=False, output_fname=None, external=False,
-            pad=None):
+            pad=None, align=None):
         """Run mkimage
 
         Args:
@@ -33,6 +33,7 @@ class Bintoolmkimage(bintool.Bintool):
             pad: Bytes to use for padding the FIT devicetree output. This allows
                 other things to be easily added later, if required, such as
                 signatures
+            align: Bytes to use for alignment of the FIT and its external data
             version: True to get the mkimage version
         """
         args = []
@@ -40,6 +41,8 @@ class Bintoolmkimage(bintool.Bintool):
             args.append('-E')
         if pad:
             args += ['-p', f'{pad:x}']
+        if align:
+            args += ['-B', f'{align:x}']
         if reset_timestamp:
             args.append('-t')
         if output_fname:
diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
index 2b32c131ed..8f11189b7b 100644
--- a/tools/binman/entries.rst
+++ b/tools/binman/entries.rst
@@ -604,6 +604,11 @@ The top-level 'fit' node supports the following special properties:
         Indicates that the contents of the FIT are external and provides the
         external offset. This is passed to mkimage via the -E and -p flags.
 
+    fit,align
+        Indicates what alignment to use for the FIT and its external data,
+        and provides the alignment to use. This is passed to mkimage via
+        the -B flag.
+
     fit,fdt-list
         Indicates the entry argument which provides the list of device tree
         files for the gen-fdt-nodes operation (as below). This is often
diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
index 0e9d81b9e8..df1ce81f9c 100644
--- a/tools/binman/etype/fit.py
+++ b/tools/binman/etype/fit.py
@@ -70,6 +70,11 @@ class Entry_fit(Entry_section):
             Indicates that the contents of the FIT are external and provides the
             external offset. This is passed to mkimage via the -E and -p flags.
 
+        fit,align
+            Indicates what alignment to use for the FIT and its external data,
+            and provides the alignment to use. This is passed to mkimage via
+            the -B flag.
+
         fit,fdt-list
             Indicates the entry argument which provides the list of device tree
             files for the gen-fdt-nodes operation (as below). This is often
@@ -423,6 +428,9 @@ class Entry_fit(Entry_section):
                 'external': True,
                 'pad': fdt_util.fdt32_to_cpu(ext_offset.value)
                 }
+        align = self._fit_props.get('fit,align')
+        if align is not None:
+            args.update({'align': fdt_util.fdt32_to_cpu(align.value)})
         if self.mkimage.run(reset_timestamp=True, output_fname=output_fname,
                             **args) is None:
             # Bintool is missing; just use empty data as the output
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index be0aea49ce..f0d0afd5b8 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -6309,6 +6309,22 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
         self.assertEqual(base + 8, inset.image_pos);
         self.assertEqual(4, inset.size);
 
+    def testFitAlign(self):
+        """Test an image with an FIT with aligned external data"""
+        data = self._DoReadFile('275_fit_align.dts')
+        self.assertEqual(4096, len(data))
+
+        dtb = fdt.Fdt.FromData(data)
+        dtb.Scan()
+
+        props = self._GetPropTree(dtb, ['data-position'])
+        expected = {
+            'u-boot:data-position': 1024,
+            'fdt-1:data-position': 2048,
+            'fdt-2:data-position': 3072,
+        }
+        self.assertEqual(expected, props)
+
 
 if __name__ == "__main__":
     unittest.main()
diff --git a/tools/binman/test/275_fit_align.dts b/tools/binman/test/275_fit_align.dts
new file mode 100644
index 0000000000..c7b06e390f
--- /dev/null
+++ b/tools/binman/test/275_fit_align.dts
@@ -0,0 +1,59 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	binman {
+		fit {
+			description = "test desc";
+			#address-cells = <1>;
+			fit,external-offset = <1024>;
+			fit,align = <1024>;
+
+			images {
+				u-boot {
+					description = "test u-boot";
+					type = "standalone";
+					arch = "arm64";
+					os = "u-boot";
+					compression = "none";
+					load = <00000000>;
+					entry = <00000000>;
+
+					u-boot-nodtb {
+					};
+				};
+
+				fdt-1 {
+					description = "test fdt";
+					type = "flat_dt";
+					compression = "none";
+
+					u-boot-dtb {
+					};
+				};
+
+				fdt-2 {
+					description = "test fdt";
+					type = "flat_dt";
+					compression = "none";
+
+					u-boot-dtb {
+					};
+				};
+			};
+
+			configurations {
+				default = "config-1";
+				config-1 {
+					description = "test config";
+					fdt = "fdt-1";
+					firmware = "u-boot";
+				};
+			};
+		};
+	};
+};
-- 
2.39.1


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

* [PATCH v2 2/6] rockchip: Align FIT image data to SD/MMC block length
  2023-01-20  8:26 ` [PATCH v2 0/6] rockchip: Align FIT images to SD/MMC block length Jonas Karlman
@ 2023-01-20  8:26   ` Jonas Karlman
  2023-01-20  8:26   ` [PATCH v2 1/6] binman: Add support for align argument to mkimage tool Jonas Karlman
                     ` (5 subsequent siblings)
  6 siblings, 0 replies; 39+ messages in thread
From: Jonas Karlman @ 2023-01-20  8:26 UTC (permalink / raw)
  To: Simon Glass, Philipp Tomsich, Kever Yang, Alper Nebi Yasak
  Cc: Quentin Schulz, u-boot, Jonas Karlman

SPL load FIT images by reading the data aligned to block length.
Block length aligned image data is read directly to the load address.
Unaligned image data is written to an offset of the load address and
then the data is memcpy to the load address.

This adds a small overhead of having to memcpy unaligned data, something
that normally is not an issue.

However, TF-A may have a segment that should be loaded into SRAM, e.g.
vendor TF-A for RK3568 has a 8KiB segment that should be loaded into the
8KiB PMU SRAM. Having the image data for such segment unaligned result
in segment being written to and memcpy from beyond the SRAM boundary, in
the end this results in invalid data in SRAM.

Aligning the FIT and its external data to MMC block length to work
around such issue.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
v2:
- Collect r-b tag

 arch/arm/dts/rockchip-u-boot.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
index 234fc5df43..63c8da456b 100644
--- a/arch/arm/dts/rockchip-u-boot.dtsi
+++ b/arch/arm/dts/rockchip-u-boot.dtsi
@@ -37,6 +37,7 @@
 			fit,fdt-list = "of-list";
 			filename = "u-boot.itb";
 			fit,external-offset = <CONFIG_FIT_EXTERNAL_OFFSET>;
+			fit,align = <512>;
 			offset = <CONFIG_SPL_PAD_TO>;
 			images {
 				u-boot {
-- 
2.39.1


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

* [PATCH v2 3/6] binman: Add special subnodes to the nodes generated by split-elf
  2023-01-20  8:26 ` [PATCH v2 0/6] rockchip: Align FIT images to SD/MMC block length Jonas Karlman
  2023-01-20  8:26   ` [PATCH v2 2/6] rockchip: Align FIT image data " Jonas Karlman
  2023-01-20  8:26   ` [PATCH v2 1/6] binman: Add support for align argument to mkimage tool Jonas Karlman
@ 2023-01-20  8:26   ` Jonas Karlman
  2023-01-20 19:19     ` Simon Glass
  2023-01-20  8:26   ` [PATCH v2 4/6] rockchip: Add sha256 hash to FIT images Jonas Karlman
                     ` (3 subsequent siblings)
  6 siblings, 1 reply; 39+ messages in thread
From: Jonas Karlman @ 2023-01-20  8:26 UTC (permalink / raw)
  To: Simon Glass, Philipp Tomsich, Kever Yang, Alper Nebi Yasak
  Cc: Quentin Schulz, u-boot, Jonas Karlman

Special nodes, hash and signature, is not being added to the nodes
generated for each segment in split-elf operation.

Copy the subnode logic used in _gen_fdt_nodes to _gen_split_elf to
ensure special nodes are added to the generated nodes.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
v2:
- Add test
- Update commit message and documentation
- Update entries.rst

 tools/binman/entries.rst                | 14 ++++++++++++++
 tools/binman/etype/fit.py               | 23 +++++++++++++++++++++--
 tools/binman/ftest.py                   | 12 ++++++++++++
 tools/binman/test/226_fit_split_elf.dts |  6 ++++++
 4 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
index 8f11189b7b..78f95dae1a 100644
--- a/tools/binman/entries.rst
+++ b/tools/binman/entries.rst
@@ -762,6 +762,9 @@ Here is an example showing ATF, TEE and a device tree all combined::
 
                 atf-bl31 {
                 };
+                hash {
+                    algo = "sha256";
+                };
             };
 
             @tee-SEQ {
@@ -777,6 +780,9 @@ Here is an example showing ATF, TEE and a device tree all combined::
 
                 tee-os {
                 };
+                hash {
+                    algo = "sha256";
+                };
             };
         };
 
@@ -805,6 +811,10 @@ ELF file, for example::
             arch = "arm64";
             type = "firmware";
             description = "ARM Trusted Firmware";
+            hash {
+                algo = "sha256";
+                value = <...hash of first segment...>;
+            };
         };
         atf-2 {
             data = <...contents of second segment...>;
@@ -814,6 +824,10 @@ ELF file, for example::
             arch = "arm64";
             type = "firmware";
             description = "ARM Trusted Firmware";
+            hash {
+                algo = "sha256";
+                value = <...hash of second segment...>;
+            };
         };
     };
 
diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
index df1ce81f9c..bcb606f3f9 100644
--- a/tools/binman/etype/fit.py
+++ b/tools/binman/etype/fit.py
@@ -228,6 +228,9 @@ class Entry_fit(Entry_section):
 
                     atf-bl31 {
                     };
+                    hash {
+                        algo = "sha256";
+                    };
                 };
 
                 @tee-SEQ {
@@ -243,6 +246,9 @@ class Entry_fit(Entry_section):
 
                     tee-os {
                     };
+                    hash {
+                        algo = "sha256";
+                    };
                 };
             };
 
@@ -271,6 +277,10 @@ class Entry_fit(Entry_section):
                 arch = "arm64";
                 type = "firmware";
                 description = "ARM Trusted Firmware";
+                hash {
+                    algo = "sha256";
+                    value = <...hash of first segment...>;
+                };
             };
             atf-2 {
                 data = <...contents of second segment...>;
@@ -280,6 +290,10 @@ class Entry_fit(Entry_section):
                 arch = "arm64";
                 type = "firmware";
                 description = "ARM Trusted Firmware";
+                hash {
+                    algo = "sha256";
+                    value = <...hash of second segment...>;
+                };
             };
         };
 
@@ -548,12 +562,13 @@ class Entry_fit(Entry_section):
                     else:
                         self.Raise("Generator node requires 'fit,fdt-list' property")
 
-        def _gen_split_elf(base_node, node, segments, entry_addr):
+        def _gen_split_elf(base_node, node, depth, segments, entry_addr):
             """Add nodes for the ELF file, one per group of contiguous segments
 
             Args:
                 base_node (Node): Template node from the binman definition
                 node (Node): Node to replace (in the FIT being built)
+                depth: Current node depth (0 is the base 'fit' node)
                 segments (list): list of segments, each:
                     int: Segment number (0 = first)
                     int: Start address of segment in memory
@@ -578,6 +593,10 @@ class Entry_fit(Entry_section):
                             self._raise_subnode(
                                 node, f"Unknown directive '{pname}'")
 
+                    for subnode in node.subnodes:
+                        with fsw.add_node(subnode.name):
+                            _add_node(node, depth + 1, subnode)
+
         def _gen_node(base_node, node, depth, in_images, entry):
             """Generate nodes from a template
 
@@ -631,7 +650,7 @@ class Entry_fit(Entry_section):
                             self._raise_subnode(
                                 node, f'Failed to read ELF file: {str(exc)}')
 
-                    _gen_split_elf(base_node, node, segments, entry_addr)
+                    _gen_split_elf(base_node, node, depth, segments, entry_addr)
 
         def _add_node(base_node, depth, node):
             """Add nodes to the output FIT
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index f0d0afd5b8..cd27572571 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -5439,6 +5439,10 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
                          fdt_util.fdt32_to_cpu(atf1.props['load'].value))
         self.assertEqual(data, atf1.props['data'].bytes)
 
+        hash_node = atf1.FindNode('hash')
+        self.assertIsNotNone(hash_node)
+        self.assertEqual({'algo', 'value'}, hash_node.props.keys())
+
         atf2 = dtb.GetNode('/images/atf-2')
         self.assertEqual(base_keys, atf2.props.keys())
         _, start, data = segments[1]
@@ -5446,6 +5450,14 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
                          fdt_util.fdt32_to_cpu(atf2.props['load'].value))
         self.assertEqual(data, atf2.props['data'].bytes)
 
+        hash_node = atf2.FindNode('hash')
+        self.assertIsNotNone(hash_node)
+        self.assertEqual({'algo', 'value'}, hash_node.props.keys())
+
+        hash_node = dtb.GetNode('/images/tee-1/hash-1')
+        self.assertIsNotNone(hash_node)
+        self.assertEqual({'algo', 'value'}, hash_node.props.keys())
+
         conf = dtb.GetNode('/configurations')
         self.assertEqual({'default'}, conf.props.keys())
 
diff --git a/tools/binman/test/226_fit_split_elf.dts b/tools/binman/test/226_fit_split_elf.dts
index fab15338b2..22c453e603 100644
--- a/tools/binman/test/226_fit_split_elf.dts
+++ b/tools/binman/test/226_fit_split_elf.dts
@@ -33,6 +33,9 @@
 
 					atf-bl31 {
 					};
+					hash {
+						algo = "sha256";
+					};
 				};
 
 				@tee-SEQ {
@@ -48,6 +51,9 @@
 
 					tee-os {
 					};
+					hash-1 {
+						algo = "sha256";
+					};
 				};
 			};
 
-- 
2.39.1


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

* [PATCH v2 4/6] rockchip: Add sha256 hash to FIT images
  2023-01-20  8:26 ` [PATCH v2 0/6] rockchip: Align FIT images to SD/MMC block length Jonas Karlman
                     ` (2 preceding siblings ...)
  2023-01-20  8:26   ` [PATCH v2 3/6] binman: Add special subnodes to the nodes generated by split-elf Jonas Karlman
@ 2023-01-20  8:26   ` Jonas Karlman
  2023-01-20  8:26   ` [PATCH v2 5/6] binman: Add support for prepending loadables with split-elf Jonas Karlman
                     ` (2 subsequent siblings)
  6 siblings, 0 replies; 39+ messages in thread
From: Jonas Karlman @ 2023-01-20  8:26 UTC (permalink / raw)
  To: Simon Glass, Philipp Tomsich, Kever Yang, Alper Nebi Yasak
  Cc: Quentin Schulz, u-boot, Jonas Karlman

Add sha256 hash to FIT images when CONFIG_SPL_FIT_SIGNATURE=y.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
v2:
- Collect r-b tag

 arch/arm/dts/rockchip-u-boot.dtsi | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
index 63c8da456b..e35902bb63 100644
--- a/arch/arm/dts/rockchip-u-boot.dtsi
+++ b/arch/arm/dts/rockchip-u-boot.dtsi
@@ -50,6 +50,11 @@
 					entry = <CONFIG_TEXT_BASE>;
 					u-boot-nodtb {
 					};
+#ifdef CONFIG_SPL_FIT_SIGNATURE
+					hash {
+						algo = "sha256";
+					};
+#endif
 				};
 
 				@atf-SEQ {
@@ -65,6 +70,11 @@
 
 					atf-bl31 {
 					};
+#ifdef CONFIG_SPL_FIT_SIGNATURE
+					hash {
+						algo = "sha256";
+					};
+#endif
 				};
 				@tee-SEQ {
 					fit,operation = "split-elf";
@@ -80,12 +90,22 @@
 					tee-os {
 						optional;
 					};
+#ifdef CONFIG_SPL_FIT_SIGNATURE
+					hash {
+						algo = "sha256";
+					};
+#endif
 				};
 
 				@fdt-SEQ {
 					description = "fdt-NAME";
 					compression = "none";
 					type = "flat_dt";
+#ifdef CONFIG_SPL_FIT_SIGNATURE
+					hash {
+						algo = "sha256";
+					};
+#endif
 				};
 			};
 
-- 
2.39.1


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

* [PATCH v2 5/6] binman: Add support for prepending loadables with split-elf
  2023-01-20  8:26 ` [PATCH v2 0/6] rockchip: Align FIT images to SD/MMC block length Jonas Karlman
                     ` (3 preceding siblings ...)
  2023-01-20  8:26   ` [PATCH v2 4/6] rockchip: Add sha256 hash to FIT images Jonas Karlman
@ 2023-01-20  8:26   ` Jonas Karlman
  2023-01-20 19:19     ` Simon Glass
  2023-01-20  8:27   ` [PATCH v2 6/6] rockchip: Use atf as firmware and move u-boot to loadables in FIT Jonas Karlman
  2023-01-21 19:01   ` [PATCH v3 0/6] rockchip: Align FIT images to SD/MMC block length Jonas Karlman
  6 siblings, 1 reply; 39+ messages in thread
From: Jonas Karlman @ 2023-01-20  8:26 UTC (permalink / raw)
  To: Simon Glass, Philipp Tomsich, Kever Yang, Alper Nebi Yasak
  Cc: Quentin Schulz, u-boot, Jonas Karlman

In some cases it is desired for SPL to start TF-A instead of U-Boot
proper. Add support to prepend a list of strings to the loadables list
generated by the split-elf generator.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
v2:
- New patch

 tools/binman/entries.rst                |  5 +-
 tools/binman/etype/fit.py               | 13 +++-
 tools/binman/ftest.py                   | 37 +++++++++++
 tools/binman/test/276_fit_loadables.dts | 87 +++++++++++++++++++++++++
 4 files changed, 137 insertions(+), 5 deletions(-)
 create mode 100644 tools/binman/test/276_fit_loadables.dts

diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
index 78f95dae1a..0ffffd60f2 100644
--- a/tools/binman/entries.rst
+++ b/tools/binman/entries.rst
@@ -724,6 +724,7 @@ split-elf
     fit,loadables
         Generates a `loadable = <...>` property with a list of the generated
         nodes (including all nodes if this operation is used multiple times)
+        Optional property value is prepended to the generated list value
 
 
 Here is an example showing ATF, TEE and a device tree all combined::
@@ -791,8 +792,8 @@ Here is an example showing ATF, TEE and a device tree all combined::
             @config-SEQ {
                 description = "conf-NAME.dtb";
                 fdt = "fdt-SEQ";
-                firmware = "u-boot";
-                fit,loadables;
+                firmware = "atf-1";
+                fit,loadables = "u-boot";
             };
         };
     };
diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
index bcb606f3f9..3c90b50b7e 100644
--- a/tools/binman/etype/fit.py
+++ b/tools/binman/etype/fit.py
@@ -190,6 +190,7 @@ class Entry_fit(Entry_section):
         fit,loadables
             Generates a `loadable = <...>` property with a list of the generated
             nodes (including all nodes if this operation is used multiple times)
+            Optional property value is prepended to the generated list value
 
 
     Here is an example showing ATF, TEE and a device tree all combined::
@@ -257,8 +258,8 @@ class Entry_fit(Entry_section):
                 @config-SEQ {
                     description = "conf-NAME.dtb";
                     fdt = "fdt-SEQ";
-                    firmware = "u-boot";
-                    fit,loadables;
+                    firmware = "atf-1";
+                    fit,loadables = "u-boot";
                 };
             };
         };
@@ -533,7 +534,13 @@ class Entry_fit(Entry_section):
                     with fsw.add_node(node_name):
                         for pname, prop in node.props.items():
                             if pname == 'fit,loadables':
-                                val = '\0'.join(self._loadables) + '\0'
+                                if type(prop.value) is str:
+                                    val = [prop.value]
+                                elif type(prop.value) is list:
+                                    val = prop.value
+                                else:
+                                    val = []
+                                val = '\0'.join(val + self._loadables) + '\0'
                                 fsw.property('loadables', val.encode('utf-8'))
                             elif pname == 'fit,operation':
                                 pass
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index cd27572571..053ae99bee 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -6337,6 +6337,43 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
         }
         self.assertEqual(expected, props)
 
+    def testFitLoadables(self):
+        """Test an image with an FIT with prepended loadables"""
+        if not elf.ELF_TOOLS:
+            self.skipTest('Python elftools not available')
+        entry_args = {
+            'of-list': 'test-fdt1',
+            'default-dt': 'test-fdt1',
+            'atf-bl31-path': 'bl31.elf',
+            'tee-os-path': 'tee.elf',
+        }
+        test_subdir = os.path.join(self._indir, TEST_FDT_SUBDIR)
+        data = self._DoReadFileDtb(
+            '276_fit_loadables.dts',
+            entry_args=entry_args,
+            extra_indirs=[test_subdir])[0]
+
+        dtb = fdt.Fdt.FromData(data)
+        dtb.Scan()
+
+        node = dtb.GetNode('/configurations/conf-uboot-1')
+        self.assertEqual('u-boot', node.props['firmware'].value)
+        self.assertEqual(
+            ['atf-1', 'atf-2'],
+            fdt_util.GetStringList(node, 'loadables'))
+
+        node = dtb.GetNode('/configurations/conf-atf-1')
+        self.assertEqual('atf-1', node.props['firmware'].value)
+        self.assertEqual(
+            ['u-boot', 'atf-1', 'atf-2'],
+            fdt_util.GetStringList(node, 'loadables'))
+
+        node = dtb.GetNode('/configurations/conf-tee-1')
+        self.assertEqual('atf-1', node.props['firmware'].value)
+        self.assertEqual(
+            ['u-boot', 'tee', 'atf-1', 'atf-2'],
+            fdt_util.GetStringList(node, 'loadables'))
+
 
 if __name__ == "__main__":
     unittest.main()
diff --git a/tools/binman/test/276_fit_loadables.dts b/tools/binman/test/276_fit_loadables.dts
new file mode 100644
index 0000000000..66dbc1fdf6
--- /dev/null
+++ b/tools/binman/test/276_fit_loadables.dts
@@ -0,0 +1,87 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	binman {
+		fit {
+			description = "test desc";
+			#address-cells = <1>;
+			fit,fdt-list = "of-list";
+
+			images {
+				u-boot {
+					description = "test u-boot";
+					type = "standalone";
+					arch = "arm64";
+					os = "u-boot";
+					compression = "none";
+					load = <0x00000000>;
+					entry = <0x00000000>;
+
+					u-boot-nodtb {
+					};
+				};
+
+				tee {
+					description = "TEE";
+					type = "tee";
+					arch = "arm64";
+					os = "tee";
+					compression = "none";
+					load = <0x00200000>;
+
+					tee-os {
+						optional;
+					};
+				};
+
+				@atf-SEQ {
+					fit,operation = "split-elf";
+					description = "ARM Trusted Firmware";
+					type = "firmware";
+					arch = "arm64";
+					os = "arm-trusted-firmware";
+					compression = "none";
+					fit,load;
+					fit,entry;
+					fit,data;
+
+					atf-bl31 {
+					};
+				};
+
+				@fdt-SEQ {
+					description = "test fdt";
+					type = "flat_dt";
+					compression = "none";
+				};
+			};
+
+			configurations {
+				default = "@conf-uboot-DEFAULT-SEQ";
+				@conf-uboot-SEQ {
+					description = "test config";
+					fdt = "fdt-SEQ";
+					firmware = "u-boot";
+					fit,loadables;
+				};
+				@conf-atf-SEQ {
+					description = "test config";
+					fdt = "fdt-SEQ";
+					firmware = "atf-1";
+					fit,loadables = "u-boot";
+				};
+				@conf-tee-SEQ {
+					description = "test config";
+					fdt = "fdt-SEQ";
+					firmware = "atf-1";
+					fit,loadables = "u-boot", "tee";
+				};
+			};
+		};
+	};
+};
-- 
2.39.1


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

* [PATCH v2 6/6] rockchip: Use atf as firmware and move u-boot to loadables in FIT
  2023-01-20  8:26 ` [PATCH v2 0/6] rockchip: Align FIT images to SD/MMC block length Jonas Karlman
                     ` (4 preceding siblings ...)
  2023-01-20  8:26   ` [PATCH v2 5/6] binman: Add support for prepending loadables with split-elf Jonas Karlman
@ 2023-01-20  8:27   ` Jonas Karlman
  2023-01-21 19:01   ` [PATCH v3 0/6] rockchip: Align FIT images to SD/MMC block length Jonas Karlman
  6 siblings, 0 replies; 39+ messages in thread
From: Jonas Karlman @ 2023-01-20  8:27 UTC (permalink / raw)
  To: Simon Glass, Philipp Tomsich, Kever Yang, Alper Nebi Yasak
  Cc: Quentin Schulz, u-boot, Jonas Karlman

The FIT generated after the switch to using binman is using different
values for firmware and loadables properties compared to the old script.

With the old script:
 firmware = "atf-1";
 loadables = "u-boot", "atf-2", ...;

After switch to binman:
 firmware = "u-boot";
 loadables = "atf-1", "atf-2", ...;

This change result in SPL jumping directly into U-Boot proper instead of
initializing TF-A.

With this patch the properties changes to:
 firmware = "atf-1";
 loatables = "u-boot", "atf-1", "atf-2", ...;

This result in "atf-1" being listed in both properties. However, SPL is
smart enough not to load "atf-1" twice.

Fixes: e0c0efff2a02 ("rockchip: Support building the all output files in binman")
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
v2:
- New patch

 arch/arm/dts/rockchip-u-boot.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
index e35902bb63..a24fddfca3 100644
--- a/arch/arm/dts/rockchip-u-boot.dtsi
+++ b/arch/arm/dts/rockchip-u-boot.dtsi
@@ -114,8 +114,8 @@
 				@config-SEQ {
 					description = "NAME.dtb";
 					fdt = "fdt-SEQ";
-					firmware = "u-boot";
-					fit,loadables;
+					firmware = "atf-1";
+					fit,loadables = "u-boot";
 				};
 			};
 		};
-- 
2.39.1


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

* Re: [PATCH v2 3/6] binman: Add special subnodes to the nodes generated by split-elf
  2023-01-20  8:26   ` [PATCH v2 3/6] binman: Add special subnodes to the nodes generated by split-elf Jonas Karlman
@ 2023-01-20 19:19     ` Simon Glass
  0 siblings, 0 replies; 39+ messages in thread
From: Simon Glass @ 2023-01-20 19:19 UTC (permalink / raw)
  To: Jonas Karlman
  Cc: Philipp Tomsich, Kever Yang, Alper Nebi Yasak, Quentin Schulz, u-boot

On Fri, 20 Jan 2023 at 01:26, Jonas Karlman <jonas@kwiboo.se> wrote:
>
> Special nodes, hash and signature, is not being added to the nodes
> generated for each segment in split-elf operation.
>
> Copy the subnode logic used in _gen_fdt_nodes to _gen_split_elf to
> ensure special nodes are added to the generated nodes.
>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
> v2:
> - Add test
> - Update commit message and documentation
> - Update entries.rst
>
>  tools/binman/entries.rst                | 14 ++++++++++++++
>  tools/binman/etype/fit.py               | 23 +++++++++++++++++++++--
>  tools/binman/ftest.py                   | 12 ++++++++++++
>  tools/binman/test/226_fit_split_elf.dts |  6 ++++++
>  4 files changed, 53 insertions(+), 2 deletions(-)

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

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

* Re: [PATCH v2 1/6] binman: Add support for align argument to mkimage tool
  2023-01-20  8:26   ` [PATCH v2 1/6] binman: Add support for align argument to mkimage tool Jonas Karlman
@ 2023-01-20 19:19     ` Simon Glass
  0 siblings, 0 replies; 39+ messages in thread
From: Simon Glass @ 2023-01-20 19:19 UTC (permalink / raw)
  To: Jonas Karlman
  Cc: Philipp Tomsich, Kever Yang, Alper Nebi Yasak, Quentin Schulz, u-boot

On Fri, 20 Jan 2023 at 01:26, Jonas Karlman <jonas@kwiboo.se> wrote:
>
> Add support to indicate what alignment to use for the FIT and its
> external data. Pass the alignment to mkimage via the -B flag.
>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
> v2:
> - Add test
> - Update entries.rst
>
>  tools/binman/btool/mkimage.py       |  5 ++-
>  tools/binman/entries.rst            |  5 +++
>  tools/binman/etype/fit.py           |  8 ++++
>  tools/binman/ftest.py               | 16 ++++++++
>  tools/binman/test/275_fit_align.dts | 59 +++++++++++++++++++++++++++++
>  5 files changed, 92 insertions(+), 1 deletion(-)
>  create mode 100644 tools/binman/test/275_fit_align.dts

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

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

* Re: [PATCH v2 5/6] binman: Add support for prepending loadables with split-elf
  2023-01-20  8:26   ` [PATCH v2 5/6] binman: Add support for prepending loadables with split-elf Jonas Karlman
@ 2023-01-20 19:19     ` Simon Glass
  2023-01-20 20:46       ` Jonas Karlman
  0 siblings, 1 reply; 39+ messages in thread
From: Simon Glass @ 2023-01-20 19:19 UTC (permalink / raw)
  To: Jonas Karlman
  Cc: Philipp Tomsich, Kever Yang, Alper Nebi Yasak, Quentin Schulz, u-boot

Hi Jonas,

On Fri, 20 Jan 2023 at 01:26, Jonas Karlman <jonas@kwiboo.se> wrote:
>
> In some cases it is desired for SPL to start TF-A instead of U-Boot
> proper. Add support to prepend a list of strings to the loadables list
> generated by the split-elf generator.
>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
> v2:
> - New patch
>
>  tools/binman/entries.rst                |  5 +-
>  tools/binman/etype/fit.py               | 13 +++-
>  tools/binman/ftest.py                   | 37 +++++++++++
>  tools/binman/test/276_fit_loadables.dts | 87 +++++++++++++++++++++++++
>  4 files changed, 137 insertions(+), 5 deletions(-)
>  create mode 100644 tools/binman/test/276_fit_loadables.dts
>
> diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
> index 78f95dae1a..0ffffd60f2 100644
> --- a/tools/binman/entries.rst
> +++ b/tools/binman/entries.rst
> @@ -724,6 +724,7 @@ split-elf
>      fit,loadables
>          Generates a `loadable = <...>` property with a list of the generated
>          nodes (including all nodes if this operation is used multiple times)
> +        Optional property value is prepended to the generated list value
>
>
>  Here is an example showing ATF, TEE and a device tree all combined::
> @@ -791,8 +792,8 @@ Here is an example showing ATF, TEE and a device tree all combined::
>              @config-SEQ {
>                  description = "conf-NAME.dtb";
>                  fdt = "fdt-SEQ";
> -                firmware = "u-boot";
> -                fit,loadables;
> +                firmware = "atf-1";
> +                fit,loadables = "u-boot";
>              };
>          };
>      };
> diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
> index bcb606f3f9..3c90b50b7e 100644
> --- a/tools/binman/etype/fit.py
> +++ b/tools/binman/etype/fit.py
> @@ -190,6 +190,7 @@ class Entry_fit(Entry_section):
>          fit,loadables
>              Generates a `loadable = <...>` property with a list of the generated
>              nodes (including all nodes if this operation is used multiple times)
> +            Optional property value is prepended to the generated list value
>
>
>      Here is an example showing ATF, TEE and a device tree all combined::
> @@ -257,8 +258,8 @@ class Entry_fit(Entry_section):
>                  @config-SEQ {
>                      description = "conf-NAME.dtb";
>                      fdt = "fdt-SEQ";
> -                    firmware = "u-boot";
> -                    fit,loadables;
> +                    firmware = "atf-1";
> +                    fit,loadables = "u-boot";
>                  };
>              };
>          };
> @@ -533,7 +534,13 @@ class Entry_fit(Entry_section):
>                      with fsw.add_node(node_name):
>                          for pname, prop in node.props.items():
>                              if pname == 'fit,loadables':
> -                                val = '\0'.join(self._loadables) + '\0'
> +                                if type(prop.value) is str:
> +                                    val = [prop.value]
> +                                elif type(prop.value) is list:
> +                                    val = prop.value
> +                                else:
> +                                    val = []
> +                                val = '\0'.join(val + self._loadables) + '\0'
>                                  fsw.property('loadables', val.encode('utf-8'))
>                              elif pname == 'fit,operation':
>                                  pass
> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> index cd27572571..053ae99bee 100644
> --- a/tools/binman/ftest.py
> +++ b/tools/binman/ftest.py
> @@ -6337,6 +6337,43 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
>          }
>          self.assertEqual(expected, props)
>
> +    def testFitLoadables(self):
> +        """Test an image with an FIT with prepended loadables"""
> +        if not elf.ELF_TOOLS:
> +            self.skipTest('Python elftools not available')
> +        entry_args = {
> +            'of-list': 'test-fdt1',
> +            'default-dt': 'test-fdt1',
> +            'atf-bl31-path': 'bl31.elf',
> +            'tee-os-path': 'tee.elf',
> +        }
> +        test_subdir = os.path.join(self._indir, TEST_FDT_SUBDIR)
> +        data = self._DoReadFileDtb(
> +            '276_fit_loadables.dts',
> +            entry_args=entry_args,
> +            extra_indirs=[test_subdir])[0]
> +
> +        dtb = fdt.Fdt.FromData(data)
> +        dtb.Scan()
> +
> +        node = dtb.GetNode('/configurations/conf-uboot-1')
> +        self.assertEqual('u-boot', node.props['firmware'].value)
> +        self.assertEqual(
> +            ['atf-1', 'atf-2'],
> +            fdt_util.GetStringList(node, 'loadables'))
> +
> +        node = dtb.GetNode('/configurations/conf-atf-1')
> +        self.assertEqual('atf-1', node.props['firmware'].value)
> +        self.assertEqual(
> +            ['u-boot', 'atf-1', 'atf-2'],
> +            fdt_util.GetStringList(node, 'loadables'))
> +
> +        node = dtb.GetNode('/configurations/conf-tee-1')
> +        self.assertEqual('atf-1', node.props['firmware'].value)
> +        self.assertEqual(
> +            ['u-boot', 'tee', 'atf-1', 'atf-2'],
> +            fdt_util.GetStringList(node, 'loadables'))
> +
>
>  if __name__ == "__main__":
>      unittest.main()
> diff --git a/tools/binman/test/276_fit_loadables.dts b/tools/binman/test/276_fit_loadables.dts
> new file mode 100644
> index 0000000000..66dbc1fdf6
> --- /dev/null
> +++ b/tools/binman/test/276_fit_loadables.dts
> @@ -0,0 +1,87 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +/dts-v1/;
> +
> +/ {
> +       #address-cells = <1>;
> +       #size-cells = <1>;
> +
> +       binman {
> +               fit {
> +                       description = "test desc";
> +                       #address-cells = <1>;
> +                       fit,fdt-list = "of-list";
> +
> +                       images {
> +                               u-boot {
> +                                       description = "test u-boot";
> +                                       type = "standalone";
> +                                       arch = "arm64";
> +                                       os = "u-boot";
> +                                       compression = "none";
> +                                       load = <0x00000000>;
> +                                       entry = <0x00000000>;
> +
> +                                       u-boot-nodtb {
> +                                       };
> +                               };
> +
> +                               tee {
> +                                       description = "TEE";
> +                                       type = "tee";
> +                                       arch = "arm64";
> +                                       os = "tee";
> +                                       compression = "none";
> +                                       load = <0x00200000>;
> +
> +                                       tee-os {
> +                                               optional;
> +                                       };
> +                               };
> +
> +                               @atf-SEQ {
> +                                       fit,operation = "split-elf";
> +                                       description = "ARM Trusted Firmware";
> +                                       type = "firmware";
> +                                       arch = "arm64";
> +                                       os = "arm-trusted-firmware";
> +                                       compression = "none";
> +                                       fit,load;
> +                                       fit,entry;
> +                                       fit,data;
> +
> +                                       atf-bl31 {
> +                                       };
> +                               };
> +
> +                               @fdt-SEQ {
> +                                       description = "test fdt";
> +                                       type = "flat_dt";
> +                                       compression = "none";
> +                               };
> +                       };
> +
> +                       configurations {
> +                               default = "@conf-uboot-DEFAULT-SEQ";
> +                               @conf-uboot-SEQ {
> +                                       description = "test config";
> +                                       fdt = "fdt-SEQ";
> +                                       firmware = "u-boot";
> +                                       fit,loadables;
> +                               };
> +                               @conf-atf-SEQ {
> +                                       description = "test config";
> +                                       fdt = "fdt-SEQ";
> +                                       firmware = "atf-1";
> +                                       fit,loadables = "u-boot";
> +                               };
> +                               @conf-tee-SEQ {
> +                                       description = "test config";
> +                                       fdt = "fdt-SEQ";
> +                                       firmware = "atf-1";
> +                                       fit,loadables = "u-boot", "tee";
> +                               };
> +                       };
> +               };
> +       };
> +};
> --
> 2.39.1
>

The problem with this is that aft-1 can be missing, in which case it
is still referenced in the 'firmware' property.

I suspect we need a way to say that the firmware should be something
other than U-Boot, if it exists.

How about:

atf-SEQ {
   fit,operation = "split-elf";
   fit,firmware-next;    /* bumps 'u-boot' out of the 'firmware' spot
if atf is present */
   description = "ARM Trusted Firmware";
   type = "firmware";
}

fit,firmware = "u-boot";   /* default value for 'firmware' if no atf */
fit,loadables;   /* ends up holding 'u-boot' too, if it is spilled by atf */

I also think the 'atf-1' should not appear in 'loadables' if it is in
'firmware'.

Regards,
Simon

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

* Re: [PATCH v2 5/6] binman: Add support for prepending loadables with split-elf
  2023-01-20 19:19     ` Simon Glass
@ 2023-01-20 20:46       ` Jonas Karlman
  2023-01-20 21:11         ` Simon Glass
  0 siblings, 1 reply; 39+ messages in thread
From: Jonas Karlman @ 2023-01-20 20:46 UTC (permalink / raw)
  To: Simon Glass
  Cc: Philipp Tomsich, Kever Yang, Alper Nebi Yasak, Quentin Schulz, u-boot

Hi Simon,

On 2023-01-20 20:19, Simon Glass wrote:
> Hi Jonas,
> 
> On Fri, 20 Jan 2023 at 01:26, Jonas Karlman <jonas@kwiboo.se> wrote:
>>
>> In some cases it is desired for SPL to start TF-A instead of U-Boot
>> proper. Add support to prepend a list of strings to the loadables list
>> generated by the split-elf generator.
>>
>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>> ---
>> v2:
>> - New patch
>>
>>  tools/binman/entries.rst                |  5 +-
>>  tools/binman/etype/fit.py               | 13 +++-
>>  tools/binman/ftest.py                   | 37 +++++++++++
>>  tools/binman/test/276_fit_loadables.dts | 87 +++++++++++++++++++++++++
>>  4 files changed, 137 insertions(+), 5 deletions(-)
>>  create mode 100644 tools/binman/test/276_fit_loadables.dts
>>
>> diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
>> index 78f95dae1a..0ffffd60f2 100644
>> --- a/tools/binman/entries.rst
>> +++ b/tools/binman/entries.rst
>> @@ -724,6 +724,7 @@ split-elf
>>      fit,loadables
>>          Generates a `loadable = <...>` property with a list of the generated
>>          nodes (including all nodes if this operation is used multiple times)
>> +        Optional property value is prepended to the generated list value
>>
>>
>>  Here is an example showing ATF, TEE and a device tree all combined::
>> @@ -791,8 +792,8 @@ Here is an example showing ATF, TEE and a device tree all combined::
>>              @config-SEQ {
>>                  description = "conf-NAME.dtb";
>>                  fdt = "fdt-SEQ";
>> -                firmware = "u-boot";
>> -                fit,loadables;
>> +                firmware = "atf-1";
>> +                fit,loadables = "u-boot";
>>              };
>>          };
>>      };
>> diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
>> index bcb606f3f9..3c90b50b7e 100644
>> --- a/tools/binman/etype/fit.py
>> +++ b/tools/binman/etype/fit.py
>> @@ -190,6 +190,7 @@ class Entry_fit(Entry_section):
>>          fit,loadables
>>              Generates a `loadable = <...>` property with a list of the generated
>>              nodes (including all nodes if this operation is used multiple times)
>> +            Optional property value is prepended to the generated list value
>>
>>
>>      Here is an example showing ATF, TEE and a device tree all combined::
>> @@ -257,8 +258,8 @@ class Entry_fit(Entry_section):
>>                  @config-SEQ {
>>                      description = "conf-NAME.dtb";
>>                      fdt = "fdt-SEQ";
>> -                    firmware = "u-boot";
>> -                    fit,loadables;
>> +                    firmware = "atf-1";
>> +                    fit,loadables = "u-boot";
>>                  };
>>              };
>>          };
>> @@ -533,7 +534,13 @@ class Entry_fit(Entry_section):
>>                      with fsw.add_node(node_name):
>>                          for pname, prop in node.props.items():
>>                              if pname == 'fit,loadables':
>> -                                val = '\0'.join(self._loadables) + '\0'
>> +                                if type(prop.value) is str:
>> +                                    val = [prop.value]
>> +                                elif type(prop.value) is list:
>> +                                    val = prop.value
>> +                                else:
>> +                                    val = []
>> +                                val = '\0'.join(val + self._loadables) + '\0'
>>                                  fsw.property('loadables', val.encode('utf-8'))
>>                              elif pname == 'fit,operation':
>>                                  pass
>> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
>> index cd27572571..053ae99bee 100644
>> --- a/tools/binman/ftest.py
>> +++ b/tools/binman/ftest.py
>> @@ -6337,6 +6337,43 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
>>          }
>>          self.assertEqual(expected, props)
>>
>> +    def testFitLoadables(self):
>> +        """Test an image with an FIT with prepended loadables"""
>> +        if not elf.ELF_TOOLS:
>> +            self.skipTest('Python elftools not available')
>> +        entry_args = {
>> +            'of-list': 'test-fdt1',
>> +            'default-dt': 'test-fdt1',
>> +            'atf-bl31-path': 'bl31.elf',
>> +            'tee-os-path': 'tee.elf',
>> +        }
>> +        test_subdir = os.path.join(self._indir, TEST_FDT_SUBDIR)
>> +        data = self._DoReadFileDtb(
>> +            '276_fit_loadables.dts',
>> +            entry_args=entry_args,
>> +            extra_indirs=[test_subdir])[0]
>> +
>> +        dtb = fdt.Fdt.FromData(data)
>> +        dtb.Scan()
>> +
>> +        node = dtb.GetNode('/configurations/conf-uboot-1')
>> +        self.assertEqual('u-boot', node.props['firmware'].value)
>> +        self.assertEqual(
>> +            ['atf-1', 'atf-2'],
>> +            fdt_util.GetStringList(node, 'loadables'))
>> +
>> +        node = dtb.GetNode('/configurations/conf-atf-1')
>> +        self.assertEqual('atf-1', node.props['firmware'].value)
>> +        self.assertEqual(
>> +            ['u-boot', 'atf-1', 'atf-2'],
>> +            fdt_util.GetStringList(node, 'loadables'))
>> +
>> +        node = dtb.GetNode('/configurations/conf-tee-1')
>> +        self.assertEqual('atf-1', node.props['firmware'].value)
>> +        self.assertEqual(
>> +            ['u-boot', 'tee', 'atf-1', 'atf-2'],
>> +            fdt_util.GetStringList(node, 'loadables'))
>> +
>>
>>  if __name__ == "__main__":
>>      unittest.main()
>> diff --git a/tools/binman/test/276_fit_loadables.dts b/tools/binman/test/276_fit_loadables.dts
>> new file mode 100644
>> index 0000000000..66dbc1fdf6
>> --- /dev/null
>> +++ b/tools/binman/test/276_fit_loadables.dts
>> @@ -0,0 +1,87 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +
>> +/dts-v1/;
>> +
>> +/ {
>> +       #address-cells = <1>;
>> +       #size-cells = <1>;
>> +
>> +       binman {
>> +               fit {
>> +                       description = "test desc";
>> +                       #address-cells = <1>;
>> +                       fit,fdt-list = "of-list";
>> +
>> +                       images {
>> +                               u-boot {
>> +                                       description = "test u-boot";
>> +                                       type = "standalone";
>> +                                       arch = "arm64";
>> +                                       os = "u-boot";
>> +                                       compression = "none";
>> +                                       load = <0x00000000>;
>> +                                       entry = <0x00000000>;
>> +
>> +                                       u-boot-nodtb {
>> +                                       };
>> +                               };
>> +
>> +                               tee {
>> +                                       description = "TEE";
>> +                                       type = "tee";
>> +                                       arch = "arm64";
>> +                                       os = "tee";
>> +                                       compression = "none";
>> +                                       load = <0x00200000>;
>> +
>> +                                       tee-os {
>> +                                               optional;
>> +                                       };
>> +                               };
>> +
>> +                               @atf-SEQ {
>> +                                       fit,operation = "split-elf";
>> +                                       description = "ARM Trusted Firmware";
>> +                                       type = "firmware";
>> +                                       arch = "arm64";
>> +                                       os = "arm-trusted-firmware";
>> +                                       compression = "none";
>> +                                       fit,load;
>> +                                       fit,entry;
>> +                                       fit,data;
>> +
>> +                                       atf-bl31 {
>> +                                       };
>> +                               };
>> +
>> +                               @fdt-SEQ {
>> +                                       description = "test fdt";
>> +                                       type = "flat_dt";
>> +                                       compression = "none";
>> +                               };
>> +                       };
>> +
>> +                       configurations {
>> +                               default = "@conf-uboot-DEFAULT-SEQ";
>> +                               @conf-uboot-SEQ {
>> +                                       description = "test config";
>> +                                       fdt = "fdt-SEQ";
>> +                                       firmware = "u-boot";
>> +                                       fit,loadables;
>> +                               };
>> +                               @conf-atf-SEQ {
>> +                                       description = "test config";
>> +                                       fdt = "fdt-SEQ";
>> +                                       firmware = "atf-1";
>> +                                       fit,loadables = "u-boot";
>> +                               };
>> +                               @conf-tee-SEQ {
>> +                                       description = "test config";
>> +                                       fdt = "fdt-SEQ";
>> +                                       firmware = "atf-1";
>> +                                       fit,loadables = "u-boot", "tee";
>> +                               };
>> +                       };
>> +               };
>> +       };
>> +};
>> --
>> 2.39.1
>>
> 
> The problem with this is that aft-1 can be missing, in which case it
> is still referenced in the 'firmware' property.
> 
> I suspect we need a way to say that the firmware should be something
> other than U-Boot, if it exists.
> 
> How about:
> 
> atf-SEQ {
>    fit,operation = "split-elf";
>    fit,firmware-next;    /* bumps 'u-boot' out of the 'firmware' spot
> if atf is present */
>    description = "ARM Trusted Firmware";
>    type = "firmware";
> }
> 
> fit,firmware = "u-boot";   /* default value for 'firmware' if no atf */
> fit,loadables;   /* ends up holding 'u-boot' too, if it is spilled by atf */

This looks reasonable, I do however wonder if it will be more flexible
if we can skip the fit,firmware-next prop altogether and just handle it
with a fit,firmware prop alone, if we treat it like a string list.

fit,firmware:  List of possible entries, the first existing entry is used
               for the 'firmware' property.

fit,loadables: Adds 'loadables' property with a list of all remaining existing
               entries in 'fit,firmware' and remaining generated loadables.

That way we do not create a dependency between the images and configurations
and make it possible to generate configs with different 'firmware' like in
the test dts.

Example for known entries 'atf-1', 'atf-2' and 'u-boot':

 fit,firmware = "u-boot";               firmware = "u-boot";
 fit,loadables;                         loadables = "atf-1", "atf-2";

 fit,firmware = "atf-1", "u-boot";      firmware = "atf-1";
 fit,loadables;                         loadables = "u-boot", "atf-2";

 fit,firmware = "fw-1", "u-boot";       firmware = "u-boot";
 fit,loadables;                         loadables = "atf-1", "atf-2";

Regards,
Jonas

> 
> I also think the 'atf-1' should not appear in 'loadables' if it is in
> 'firmware'.
> 
> Regards,
> Simon


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

* Re: [PATCH v2 5/6] binman: Add support for prepending loadables with split-elf
  2023-01-20 20:46       ` Jonas Karlman
@ 2023-01-20 21:11         ` Simon Glass
  0 siblings, 0 replies; 39+ messages in thread
From: Simon Glass @ 2023-01-20 21:11 UTC (permalink / raw)
  To: Jonas Karlman
  Cc: Philipp Tomsich, Kever Yang, Alper Nebi Yasak, Quentin Schulz, u-boot

Hi Jonas,

On Fri, 20 Jan 2023 at 13:47, Jonas Karlman <jonas@kwiboo.se> wrote:
>
> Hi Simon,
>
> On 2023-01-20 20:19, Simon Glass wrote:
> > Hi Jonas,
> >
> > On Fri, 20 Jan 2023 at 01:26, Jonas Karlman <jonas@kwiboo.se> wrote:
> >>
> >> In some cases it is desired for SPL to start TF-A instead of U-Boot
> >> proper. Add support to prepend a list of strings to the loadables list
> >> generated by the split-elf generator.
> >>
> >> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> >> ---
> >> v2:
> >> - New patch
> >>
> >>  tools/binman/entries.rst                |  5 +-
> >>  tools/binman/etype/fit.py               | 13 +++-
> >>  tools/binman/ftest.py                   | 37 +++++++++++
> >>  tools/binman/test/276_fit_loadables.dts | 87 +++++++++++++++++++++++++
> >>  4 files changed, 137 insertions(+), 5 deletions(-)
> >>  create mode 100644 tools/binman/test/276_fit_loadables.dts
> >>
> >> diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
> >> index 78f95dae1a..0ffffd60f2 100644
> >> --- a/tools/binman/entries.rst
> >> +++ b/tools/binman/entries.rst
> >> @@ -724,6 +724,7 @@ split-elf
> >>      fit,loadables
> >>          Generates a `loadable = <...>` property with a list of the generated
> >>          nodes (including all nodes if this operation is used multiple times)
> >> +        Optional property value is prepended to the generated list value
> >>
> >>
> >>  Here is an example showing ATF, TEE and a device tree all combined::
> >> @@ -791,8 +792,8 @@ Here is an example showing ATF, TEE and a device tree all combined::
> >>              @config-SEQ {
> >>                  description = "conf-NAME.dtb";
> >>                  fdt = "fdt-SEQ";
> >> -                firmware = "u-boot";
> >> -                fit,loadables;
> >> +                firmware = "atf-1";
> >> +                fit,loadables = "u-boot";
> >>              };
> >>          };
> >>      };
> >> diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
> >> index bcb606f3f9..3c90b50b7e 100644
> >> --- a/tools/binman/etype/fit.py
> >> +++ b/tools/binman/etype/fit.py
> >> @@ -190,6 +190,7 @@ class Entry_fit(Entry_section):
> >>          fit,loadables
> >>              Generates a `loadable = <...>` property with a list of the generated
> >>              nodes (including all nodes if this operation is used multiple times)
> >> +            Optional property value is prepended to the generated list value
> >>
> >>
> >>      Here is an example showing ATF, TEE and a device tree all combined::
> >> @@ -257,8 +258,8 @@ class Entry_fit(Entry_section):
> >>                  @config-SEQ {
> >>                      description = "conf-NAME.dtb";
> >>                      fdt = "fdt-SEQ";
> >> -                    firmware = "u-boot";
> >> -                    fit,loadables;
> >> +                    firmware = "atf-1";
> >> +                    fit,loadables = "u-boot";
> >>                  };
> >>              };
> >>          };
> >> @@ -533,7 +534,13 @@ class Entry_fit(Entry_section):
> >>                      with fsw.add_node(node_name):
> >>                          for pname, prop in node.props.items():
> >>                              if pname == 'fit,loadables':
> >> -                                val = '\0'.join(self._loadables) + '\0'
> >> +                                if type(prop.value) is str:
> >> +                                    val = [prop.value]
> >> +                                elif type(prop.value) is list:
> >> +                                    val = prop.value
> >> +                                else:
> >> +                                    val = []
> >> +                                val = '\0'.join(val + self._loadables) + '\0'
> >>                                  fsw.property('loadables', val.encode('utf-8'))
> >>                              elif pname == 'fit,operation':
> >>                                  pass
> >> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> >> index cd27572571..053ae99bee 100644
> >> --- a/tools/binman/ftest.py
> >> +++ b/tools/binman/ftest.py
> >> @@ -6337,6 +6337,43 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
> >>          }
> >>          self.assertEqual(expected, props)
> >>
> >> +    def testFitLoadables(self):
> >> +        """Test an image with an FIT with prepended loadables"""
> >> +        if not elf.ELF_TOOLS:
> >> +            self.skipTest('Python elftools not available')
> >> +        entry_args = {
> >> +            'of-list': 'test-fdt1',
> >> +            'default-dt': 'test-fdt1',
> >> +            'atf-bl31-path': 'bl31.elf',
> >> +            'tee-os-path': 'tee.elf',
> >> +        }
> >> +        test_subdir = os.path.join(self._indir, TEST_FDT_SUBDIR)
> >> +        data = self._DoReadFileDtb(
> >> +            '276_fit_loadables.dts',
> >> +            entry_args=entry_args,
> >> +            extra_indirs=[test_subdir])[0]
> >> +
> >> +        dtb = fdt.Fdt.FromData(data)
> >> +        dtb.Scan()
> >> +
> >> +        node = dtb.GetNode('/configurations/conf-uboot-1')
> >> +        self.assertEqual('u-boot', node.props['firmware'].value)
> >> +        self.assertEqual(
> >> +            ['atf-1', 'atf-2'],
> >> +            fdt_util.GetStringList(node, 'loadables'))
> >> +
> >> +        node = dtb.GetNode('/configurations/conf-atf-1')
> >> +        self.assertEqual('atf-1', node.props['firmware'].value)
> >> +        self.assertEqual(
> >> +            ['u-boot', 'atf-1', 'atf-2'],
> >> +            fdt_util.GetStringList(node, 'loadables'))
> >> +
> >> +        node = dtb.GetNode('/configurations/conf-tee-1')
> >> +        self.assertEqual('atf-1', node.props['firmware'].value)
> >> +        self.assertEqual(
> >> +            ['u-boot', 'tee', 'atf-1', 'atf-2'],
> >> +            fdt_util.GetStringList(node, 'loadables'))
> >> +
> >>
> >>  if __name__ == "__main__":
> >>      unittest.main()
> >> diff --git a/tools/binman/test/276_fit_loadables.dts b/tools/binman/test/276_fit_loadables.dts
> >> new file mode 100644
> >> index 0000000000..66dbc1fdf6
> >> --- /dev/null
> >> +++ b/tools/binman/test/276_fit_loadables.dts
> >> @@ -0,0 +1,87 @@
> >> +// SPDX-License-Identifier: GPL-2.0+
> >> +
> >> +/dts-v1/;
> >> +
> >> +/ {
> >> +       #address-cells = <1>;
> >> +       #size-cells = <1>;
> >> +
> >> +       binman {
> >> +               fit {
> >> +                       description = "test desc";
> >> +                       #address-cells = <1>;
> >> +                       fit,fdt-list = "of-list";
> >> +
> >> +                       images {
> >> +                               u-boot {
> >> +                                       description = "test u-boot";
> >> +                                       type = "standalone";
> >> +                                       arch = "arm64";
> >> +                                       os = "u-boot";
> >> +                                       compression = "none";
> >> +                                       load = <0x00000000>;
> >> +                                       entry = <0x00000000>;
> >> +
> >> +                                       u-boot-nodtb {
> >> +                                       };
> >> +                               };
> >> +
> >> +                               tee {
> >> +                                       description = "TEE";
> >> +                                       type = "tee";
> >> +                                       arch = "arm64";
> >> +                                       os = "tee";
> >> +                                       compression = "none";
> >> +                                       load = <0x00200000>;
> >> +
> >> +                                       tee-os {
> >> +                                               optional;
> >> +                                       };
> >> +                               };
> >> +
> >> +                               @atf-SEQ {
> >> +                                       fit,operation = "split-elf";
> >> +                                       description = "ARM Trusted Firmware";
> >> +                                       type = "firmware";
> >> +                                       arch = "arm64";
> >> +                                       os = "arm-trusted-firmware";
> >> +                                       compression = "none";
> >> +                                       fit,load;
> >> +                                       fit,entry;
> >> +                                       fit,data;
> >> +
> >> +                                       atf-bl31 {
> >> +                                       };
> >> +                               };
> >> +
> >> +                               @fdt-SEQ {
> >> +                                       description = "test fdt";
> >> +                                       type = "flat_dt";
> >> +                                       compression = "none";
> >> +                               };
> >> +                       };
> >> +
> >> +                       configurations {
> >> +                               default = "@conf-uboot-DEFAULT-SEQ";
> >> +                               @conf-uboot-SEQ {
> >> +                                       description = "test config";
> >> +                                       fdt = "fdt-SEQ";
> >> +                                       firmware = "u-boot";
> >> +                                       fit,loadables;
> >> +                               };
> >> +                               @conf-atf-SEQ {
> >> +                                       description = "test config";
> >> +                                       fdt = "fdt-SEQ";
> >> +                                       firmware = "atf-1";
> >> +                                       fit,loadables = "u-boot";
> >> +                               };
> >> +                               @conf-tee-SEQ {
> >> +                                       description = "test config";
> >> +                                       fdt = "fdt-SEQ";
> >> +                                       firmware = "atf-1";
> >> +                                       fit,loadables = "u-boot", "tee";
> >> +                               };
> >> +                       };
> >> +               };
> >> +       };
> >> +};
> >> --
> >> 2.39.1
> >>
> >
> > The problem with this is that aft-1 can be missing, in which case it
> > is still referenced in the 'firmware' property.
> >
> > I suspect we need a way to say that the firmware should be something
> > other than U-Boot, if it exists.
> >
> > How about:
> >
> > atf-SEQ {
> >    fit,operation = "split-elf";
> >    fit,firmware-next;    /* bumps 'u-boot' out of the 'firmware' spot
> > if atf is present */
> >    description = "ARM Trusted Firmware";
> >    type = "firmware";
> > }
> >
> > fit,firmware = "u-boot";   /* default value for 'firmware' if no atf */
> > fit,loadables;   /* ends up holding 'u-boot' too, if it is spilled by atf */
>
> This looks reasonable, I do however wonder if it will be more flexible
> if we can skip the fit,firmware-next prop altogether and just handle it
> with a fit,firmware prop alone, if we treat it like a string list.
>
> fit,firmware:  List of possible entries, the first existing entry is used
>                for the 'firmware' property.
>
> fit,loadables: Adds 'loadables' property with a list of all remaining existing
>                entries in 'fit,firmware' and remaining generated loadables.
>
> That way we do not create a dependency between the images and configurations
> and make it possible to generate configs with different 'firmware' like in
> the test dts.
>
> Example for known entries 'atf-1', 'atf-2' and 'u-boot':
>
>  fit,firmware = "u-boot";               firmware = "u-boot";
>  fit,loadables;                         loadables = "atf-1", "atf-2";
>
>  fit,firmware = "atf-1", "u-boot";      firmware = "atf-1";
>  fit,loadables;                         loadables = "u-boot", "atf-2";
>
>  fit,firmware = "fw-1", "u-boot";       firmware = "u-boot";
>  fit,loadables;                         loadables = "atf-1", "atf-2";

Yes that seems better, thanks. So things in fit,firmware are omitted
if they are 'missing'.

Regards,
Simon

>
> Regards,
> Jonas
>
> >
> > I also think the 'atf-1' should not appear in 'loadables' if it is in
> > 'firmware'.
> >
> > Regards,
> > Simon
>

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

* [PATCH v3 1/6] binman: Add support for align argument to mkimage tool
  2023-01-21 19:01   ` [PATCH v3 0/6] rockchip: Align FIT images to SD/MMC block length Jonas Karlman
@ 2023-01-21 19:01     ` Jonas Karlman
  2023-01-21 19:01     ` [PATCH v3 2/6] rockchip: Align FIT image data to SD/MMC block length Jonas Karlman
                       ` (8 subsequent siblings)
  9 siblings, 0 replies; 39+ messages in thread
From: Jonas Karlman @ 2023-01-21 19:01 UTC (permalink / raw)
  To: Simon Glass, Philipp Tomsich, Kever Yang, Alper Nebi Yasak
  Cc: Quentin Schulz, u-boot, Jonas Karlman

Add support to indicate what alignment to use for the FIT and its
external data. Pass the alignment to mkimage via the -B flag.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
v3:
- Collect r-b tag
v2:
- Add test
- Update entries.rst

 tools/binman/btool/mkimage.py       |  5 ++-
 tools/binman/entries.rst            |  5 +++
 tools/binman/etype/fit.py           |  8 ++++
 tools/binman/ftest.py               | 16 ++++++++
 tools/binman/test/275_fit_align.dts | 59 +++++++++++++++++++++++++++++
 5 files changed, 92 insertions(+), 1 deletion(-)
 create mode 100644 tools/binman/test/275_fit_align.dts

diff --git a/tools/binman/btool/mkimage.py b/tools/binman/btool/mkimage.py
index da5f3441624d..d5b407c55471 100644
--- a/tools/binman/btool/mkimage.py
+++ b/tools/binman/btool/mkimage.py
@@ -22,7 +22,7 @@ class Bintoolmkimage(bintool.Bintool):
 
     # pylint: disable=R0913
     def run(self, reset_timestamp=False, output_fname=None, external=False,
-            pad=None):
+            pad=None, align=None):
         """Run mkimage
 
         Args:
@@ -33,6 +33,7 @@ class Bintoolmkimage(bintool.Bintool):
             pad: Bytes to use for padding the FIT devicetree output. This allows
                 other things to be easily added later, if required, such as
                 signatures
+            align: Bytes to use for alignment of the FIT and its external data
             version: True to get the mkimage version
         """
         args = []
@@ -40,6 +41,8 @@ class Bintoolmkimage(bintool.Bintool):
             args.append('-E')
         if pad:
             args += ['-p', f'{pad:x}']
+        if align:
+            args += ['-B', f'{align:x}']
         if reset_timestamp:
             args.append('-t')
         if output_fname:
diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
index 2b32c131ede4..8f11189b7bf0 100644
--- a/tools/binman/entries.rst
+++ b/tools/binman/entries.rst
@@ -604,6 +604,11 @@ The top-level 'fit' node supports the following special properties:
         Indicates that the contents of the FIT are external and provides the
         external offset. This is passed to mkimage via the -E and -p flags.
 
+    fit,align
+        Indicates what alignment to use for the FIT and its external data,
+        and provides the alignment to use. This is passed to mkimage via
+        the -B flag.
+
     fit,fdt-list
         Indicates the entry argument which provides the list of device tree
         files for the gen-fdt-nodes operation (as below). This is often
diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
index 0e9d81b9e84a..df1ce81f9c07 100644
--- a/tools/binman/etype/fit.py
+++ b/tools/binman/etype/fit.py
@@ -70,6 +70,11 @@ class Entry_fit(Entry_section):
             Indicates that the contents of the FIT are external and provides the
             external offset. This is passed to mkimage via the -E and -p flags.
 
+        fit,align
+            Indicates what alignment to use for the FIT and its external data,
+            and provides the alignment to use. This is passed to mkimage via
+            the -B flag.
+
         fit,fdt-list
             Indicates the entry argument which provides the list of device tree
             files for the gen-fdt-nodes operation (as below). This is often
@@ -423,6 +428,9 @@ class Entry_fit(Entry_section):
                 'external': True,
                 'pad': fdt_util.fdt32_to_cpu(ext_offset.value)
                 }
+        align = self._fit_props.get('fit,align')
+        if align is not None:
+            args.update({'align': fdt_util.fdt32_to_cpu(align.value)})
         if self.mkimage.run(reset_timestamp=True, output_fname=output_fname,
                             **args) is None:
             # Bintool is missing; just use empty data as the output
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index be0aea49ce9b..f0d0afd5b808 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -6309,6 +6309,22 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
         self.assertEqual(base + 8, inset.image_pos);
         self.assertEqual(4, inset.size);
 
+    def testFitAlign(self):
+        """Test an image with an FIT with aligned external data"""
+        data = self._DoReadFile('275_fit_align.dts')
+        self.assertEqual(4096, len(data))
+
+        dtb = fdt.Fdt.FromData(data)
+        dtb.Scan()
+
+        props = self._GetPropTree(dtb, ['data-position'])
+        expected = {
+            'u-boot:data-position': 1024,
+            'fdt-1:data-position': 2048,
+            'fdt-2:data-position': 3072,
+        }
+        self.assertEqual(expected, props)
+
 
 if __name__ == "__main__":
     unittest.main()
diff --git a/tools/binman/test/275_fit_align.dts b/tools/binman/test/275_fit_align.dts
new file mode 100644
index 000000000000..c7b06e390fae
--- /dev/null
+++ b/tools/binman/test/275_fit_align.dts
@@ -0,0 +1,59 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	binman {
+		fit {
+			description = "test desc";
+			#address-cells = <1>;
+			fit,external-offset = <1024>;
+			fit,align = <1024>;
+
+			images {
+				u-boot {
+					description = "test u-boot";
+					type = "standalone";
+					arch = "arm64";
+					os = "u-boot";
+					compression = "none";
+					load = <00000000>;
+					entry = <00000000>;
+
+					u-boot-nodtb {
+					};
+				};
+
+				fdt-1 {
+					description = "test fdt";
+					type = "flat_dt";
+					compression = "none";
+
+					u-boot-dtb {
+					};
+				};
+
+				fdt-2 {
+					description = "test fdt";
+					type = "flat_dt";
+					compression = "none";
+
+					u-boot-dtb {
+					};
+				};
+			};
+
+			configurations {
+				default = "config-1";
+				config-1 {
+					description = "test config";
+					fdt = "fdt-1";
+					firmware = "u-boot";
+				};
+			};
+		};
+	};
+};
-- 
2.39.1


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

* [PATCH v3 2/6] rockchip: Align FIT image data to SD/MMC block length
  2023-01-21 19:01   ` [PATCH v3 0/6] rockchip: Align FIT images to SD/MMC block length Jonas Karlman
  2023-01-21 19:01     ` [PATCH v3 1/6] binman: Add support for align argument to mkimage tool Jonas Karlman
@ 2023-01-21 19:01     ` Jonas Karlman
  2023-01-21 19:01     ` [PATCH v3 3/6] binman: Add special subnodes to the nodes generated by split-elf Jonas Karlman
                       ` (7 subsequent siblings)
  9 siblings, 0 replies; 39+ messages in thread
From: Jonas Karlman @ 2023-01-21 19:01 UTC (permalink / raw)
  To: Simon Glass, Philipp Tomsich, Kever Yang, Alper Nebi Yasak
  Cc: Quentin Schulz, u-boot, Jonas Karlman

SPL load FIT images by reading the data aligned to block length.
Block length aligned image data is read directly to the load address.
Unaligned image data is written to an offset of the load address and
then the data is memcpy to the load address.

This adds a small overhead of having to memcpy unaligned data, something
that normally is not an issue.

However, TF-A may have a segment that should be loaded into SRAM, e.g.
vendor TF-A for RK3568 has a 8KiB segment that should be loaded into the
8KiB PMU SRAM. Having the image data for such segment unaligned result
in segment being written to and memcpy from beyond the SRAM boundary, in
the end this results in invalid data in SRAM.

Aligning the FIT and its external data to MMC block length to work
around such issue.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
v2:
- Collect r-b tag

 arch/arm/dts/rockchip-u-boot.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
index 234fc5df4332..63c8da456b4f 100644
--- a/arch/arm/dts/rockchip-u-boot.dtsi
+++ b/arch/arm/dts/rockchip-u-boot.dtsi
@@ -37,6 +37,7 @@
 			fit,fdt-list = "of-list";
 			filename = "u-boot.itb";
 			fit,external-offset = <CONFIG_FIT_EXTERNAL_OFFSET>;
+			fit,align = <512>;
 			offset = <CONFIG_SPL_PAD_TO>;
 			images {
 				u-boot {
-- 
2.39.1


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

* [PATCH v3 0/6] rockchip: Align FIT images to SD/MMC block length
  2023-01-20  8:26 ` [PATCH v2 0/6] rockchip: Align FIT images to SD/MMC block length Jonas Karlman
                     ` (5 preceding siblings ...)
  2023-01-20  8:27   ` [PATCH v2 6/6] rockchip: Use atf as firmware and move u-boot to loadables in FIT Jonas Karlman
@ 2023-01-21 19:01   ` Jonas Karlman
  2023-01-21 19:01     ` [PATCH v3 1/6] binman: Add support for align argument to mkimage tool Jonas Karlman
                       ` (9 more replies)
  6 siblings, 10 replies; 39+ messages in thread
From: Jonas Karlman @ 2023-01-21 19:01 UTC (permalink / raw)
  To: Simon Glass, Philipp Tomsich, Kever Yang, Alper Nebi Yasak
  Cc: Quentin Schulz, u-boot, Jonas Karlman

When I was trying to run mainline U-Boot on my new Rockchip RK3568 board
I discovered that one segment of vendor TF-A could not successfully be
loaded into SRAM, validation of the image sha256 hash failed.

The issue with loading the data turned out to be because of how SPL load
FIT images. It reads data aligned to block length. Aligned image data is
read directly to the load address. Unaligned image data is written to an
offset of the load address and then memcpy to the load address.

The segment of the TF-A that failed to load is a 8KiB segment that
should be loaded into the 8KiB PMU SRAM. Because this segment was
unaligned to the SD/MMC block length the segment was written to and
memcpy from beyond the SRAM boundary, in the end this results in invalid
data in SRAM.

Vendor u-boot has worked around this issue by using a bounce buffer for
any load address that is not in the gd->ram_base - gd->ram_top range.

However, by passing -B 200 to mkimage we can work around this issue
because the FIT and its external data ends up being aligned to SD/MMC
block length.

This series adds support for a fit,align property in binman and makes
use of this new property in rockchip-u-boot.dtsi.
It also adds image hash to the FIT images when configured with
CONFIG_SPL_FIT_SIGNATURE=y.

Changes in v3:
- Introduce new fit,firmware property to handle firmware selection
- Use fit,firmware property to fix initialization of TF-A
- Collect r-b tags

Changes in v2:
- Added tests
- Updated entries.rst
- Collect r-b tags
- Rebased on top of dm-pull-18jan23
- New patch to fix initialization of TF-A

Jonas Karlman (6):
  binman: Add support for align argument to mkimage tool
  rockchip: Align FIT image data to SD/MMC block length
  binman: Add special subnodes to the nodes generated by split-elf
  rockchip: Add sha256 hash to FIT images
  binman: Add support for selecting firmware to use with split-elf
  rockchip: Use atf as firmware and move u-boot to loadables in FIT

 arch/arm/dts/rockchip-u-boot.dtsi             | 23 ++++-
 tools/binman/btool/mkimage.py                 |  5 +-
 tools/binman/entries.rst                      | 35 ++++++-
 tools/binman/etype/fit.py                     | 93 ++++++++++++++++--
 tools/binman/ftest.py                         | 72 ++++++++++++++
 tools/binman/test/226_fit_split_elf.dts       |  6 ++
 tools/binman/test/275_fit_align.dts           | 59 ++++++++++++
 .../test/276_fit_firmware_loadables.dts       | 96 +++++++++++++++++++
 8 files changed, 372 insertions(+), 17 deletions(-)
 create mode 100644 tools/binman/test/275_fit_align.dts
 create mode 100644 tools/binman/test/276_fit_firmware_loadables.dts

-- 
2.39.1


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

* [PATCH v3 3/6] binman: Add special subnodes to the nodes generated by split-elf
  2023-01-21 19:01   ` [PATCH v3 0/6] rockchip: Align FIT images to SD/MMC block length Jonas Karlman
  2023-01-21 19:01     ` [PATCH v3 1/6] binman: Add support for align argument to mkimage tool Jonas Karlman
  2023-01-21 19:01     ` [PATCH v3 2/6] rockchip: Align FIT image data to SD/MMC block length Jonas Karlman
@ 2023-01-21 19:01     ` Jonas Karlman
  2023-01-21 19:01     ` [PATCH v3 4/6] rockchip: Add sha256 hash to FIT images Jonas Karlman
                       ` (6 subsequent siblings)
  9 siblings, 0 replies; 39+ messages in thread
From: Jonas Karlman @ 2023-01-21 19:01 UTC (permalink / raw)
  To: Simon Glass, Philipp Tomsich, Kever Yang, Alper Nebi Yasak
  Cc: Quentin Schulz, u-boot, Jonas Karlman

Special nodes, hash and signature, is not being added to the nodes
generated for each segment in split-elf operation.

Copy the subnode logic used in _gen_fdt_nodes to _gen_split_elf to
ensure special nodes are added to the generated nodes.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
v3:
- Collect r-b tag
v2:
- Add test
- Update commit message and documentation
- Update entries.rst

 tools/binman/entries.rst                | 14 ++++++++++++++
 tools/binman/etype/fit.py               | 23 +++++++++++++++++++++--
 tools/binman/ftest.py                   | 12 ++++++++++++
 tools/binman/test/226_fit_split_elf.dts |  6 ++++++
 4 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
index 8f11189b7bf0..78f95dae1a4e 100644
--- a/tools/binman/entries.rst
+++ b/tools/binman/entries.rst
@@ -762,6 +762,9 @@ Here is an example showing ATF, TEE and a device tree all combined::
 
                 atf-bl31 {
                 };
+                hash {
+                    algo = "sha256";
+                };
             };
 
             @tee-SEQ {
@@ -777,6 +780,9 @@ Here is an example showing ATF, TEE and a device tree all combined::
 
                 tee-os {
                 };
+                hash {
+                    algo = "sha256";
+                };
             };
         };
 
@@ -805,6 +811,10 @@ ELF file, for example::
             arch = "arm64";
             type = "firmware";
             description = "ARM Trusted Firmware";
+            hash {
+                algo = "sha256";
+                value = <...hash of first segment...>;
+            };
         };
         atf-2 {
             data = <...contents of second segment...>;
@@ -814,6 +824,10 @@ ELF file, for example::
             arch = "arm64";
             type = "firmware";
             description = "ARM Trusted Firmware";
+            hash {
+                algo = "sha256";
+                value = <...hash of second segment...>;
+            };
         };
     };
 
diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
index df1ce81f9c07..bcb606f3f94c 100644
--- a/tools/binman/etype/fit.py
+++ b/tools/binman/etype/fit.py
@@ -228,6 +228,9 @@ class Entry_fit(Entry_section):
 
                     atf-bl31 {
                     };
+                    hash {
+                        algo = "sha256";
+                    };
                 };
 
                 @tee-SEQ {
@@ -243,6 +246,9 @@ class Entry_fit(Entry_section):
 
                     tee-os {
                     };
+                    hash {
+                        algo = "sha256";
+                    };
                 };
             };
 
@@ -271,6 +277,10 @@ class Entry_fit(Entry_section):
                 arch = "arm64";
                 type = "firmware";
                 description = "ARM Trusted Firmware";
+                hash {
+                    algo = "sha256";
+                    value = <...hash of first segment...>;
+                };
             };
             atf-2 {
                 data = <...contents of second segment...>;
@@ -280,6 +290,10 @@ class Entry_fit(Entry_section):
                 arch = "arm64";
                 type = "firmware";
                 description = "ARM Trusted Firmware";
+                hash {
+                    algo = "sha256";
+                    value = <...hash of second segment...>;
+                };
             };
         };
 
@@ -548,12 +562,13 @@ class Entry_fit(Entry_section):
                     else:
                         self.Raise("Generator node requires 'fit,fdt-list' property")
 
-        def _gen_split_elf(base_node, node, segments, entry_addr):
+        def _gen_split_elf(base_node, node, depth, segments, entry_addr):
             """Add nodes for the ELF file, one per group of contiguous segments
 
             Args:
                 base_node (Node): Template node from the binman definition
                 node (Node): Node to replace (in the FIT being built)
+                depth: Current node depth (0 is the base 'fit' node)
                 segments (list): list of segments, each:
                     int: Segment number (0 = first)
                     int: Start address of segment in memory
@@ -578,6 +593,10 @@ class Entry_fit(Entry_section):
                             self._raise_subnode(
                                 node, f"Unknown directive '{pname}'")
 
+                    for subnode in node.subnodes:
+                        with fsw.add_node(subnode.name):
+                            _add_node(node, depth + 1, subnode)
+
         def _gen_node(base_node, node, depth, in_images, entry):
             """Generate nodes from a template
 
@@ -631,7 +650,7 @@ class Entry_fit(Entry_section):
                             self._raise_subnode(
                                 node, f'Failed to read ELF file: {str(exc)}')
 
-                    _gen_split_elf(base_node, node, segments, entry_addr)
+                    _gen_split_elf(base_node, node, depth, segments, entry_addr)
 
         def _add_node(base_node, depth, node):
             """Add nodes to the output FIT
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index f0d0afd5b808..cd2757257178 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -5439,6 +5439,10 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
                          fdt_util.fdt32_to_cpu(atf1.props['load'].value))
         self.assertEqual(data, atf1.props['data'].bytes)
 
+        hash_node = atf1.FindNode('hash')
+        self.assertIsNotNone(hash_node)
+        self.assertEqual({'algo', 'value'}, hash_node.props.keys())
+
         atf2 = dtb.GetNode('/images/atf-2')
         self.assertEqual(base_keys, atf2.props.keys())
         _, start, data = segments[1]
@@ -5446,6 +5450,14 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
                          fdt_util.fdt32_to_cpu(atf2.props['load'].value))
         self.assertEqual(data, atf2.props['data'].bytes)
 
+        hash_node = atf2.FindNode('hash')
+        self.assertIsNotNone(hash_node)
+        self.assertEqual({'algo', 'value'}, hash_node.props.keys())
+
+        hash_node = dtb.GetNode('/images/tee-1/hash-1')
+        self.assertIsNotNone(hash_node)
+        self.assertEqual({'algo', 'value'}, hash_node.props.keys())
+
         conf = dtb.GetNode('/configurations')
         self.assertEqual({'default'}, conf.props.keys())
 
diff --git a/tools/binman/test/226_fit_split_elf.dts b/tools/binman/test/226_fit_split_elf.dts
index fab15338b204..22c453e6037b 100644
--- a/tools/binman/test/226_fit_split_elf.dts
+++ b/tools/binman/test/226_fit_split_elf.dts
@@ -33,6 +33,9 @@
 
 					atf-bl31 {
 					};
+					hash {
+						algo = "sha256";
+					};
 				};
 
 				@tee-SEQ {
@@ -48,6 +51,9 @@
 
 					tee-os {
 					};
+					hash-1 {
+						algo = "sha256";
+					};
 				};
 			};
 
-- 
2.39.1


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

* [PATCH v3 4/6] rockchip: Add sha256 hash to FIT images
  2023-01-21 19:01   ` [PATCH v3 0/6] rockchip: Align FIT images to SD/MMC block length Jonas Karlman
                       ` (2 preceding siblings ...)
  2023-01-21 19:01     ` [PATCH v3 3/6] binman: Add special subnodes to the nodes generated by split-elf Jonas Karlman
@ 2023-01-21 19:01     ` Jonas Karlman
  2023-01-21 19:02     ` [PATCH v3 5/6] binman: Add support for selecting firmware to use with split-elf Jonas Karlman
                       ` (5 subsequent siblings)
  9 siblings, 0 replies; 39+ messages in thread
From: Jonas Karlman @ 2023-01-21 19:01 UTC (permalink / raw)
  To: Simon Glass, Philipp Tomsich, Kever Yang, Alper Nebi Yasak
  Cc: Quentin Schulz, u-boot, Jonas Karlman

Add sha256 hash to FIT images when CONFIG_SPL_FIT_SIGNATURE=y.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
v2:
- Collect r-b tag

 arch/arm/dts/rockchip-u-boot.dtsi | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
index 63c8da456b4f..e35902bb63a8 100644
--- a/arch/arm/dts/rockchip-u-boot.dtsi
+++ b/arch/arm/dts/rockchip-u-boot.dtsi
@@ -50,6 +50,11 @@
 					entry = <CONFIG_TEXT_BASE>;
 					u-boot-nodtb {
 					};
+#ifdef CONFIG_SPL_FIT_SIGNATURE
+					hash {
+						algo = "sha256";
+					};
+#endif
 				};
 
 				@atf-SEQ {
@@ -65,6 +70,11 @@
 
 					atf-bl31 {
 					};
+#ifdef CONFIG_SPL_FIT_SIGNATURE
+					hash {
+						algo = "sha256";
+					};
+#endif
 				};
 				@tee-SEQ {
 					fit,operation = "split-elf";
@@ -80,12 +90,22 @@
 					tee-os {
 						optional;
 					};
+#ifdef CONFIG_SPL_FIT_SIGNATURE
+					hash {
+						algo = "sha256";
+					};
+#endif
 				};
 
 				@fdt-SEQ {
 					description = "fdt-NAME";
 					compression = "none";
 					type = "flat_dt";
+#ifdef CONFIG_SPL_FIT_SIGNATURE
+					hash {
+						algo = "sha256";
+					};
+#endif
 				};
 			};
 
-- 
2.39.1


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

* [PATCH v3 5/6] binman: Add support for selecting firmware to use with split-elf
  2023-01-21 19:01   ` [PATCH v3 0/6] rockchip: Align FIT images to SD/MMC block length Jonas Karlman
                       ` (3 preceding siblings ...)
  2023-01-21 19:01     ` [PATCH v3 4/6] rockchip: Add sha256 hash to FIT images Jonas Karlman
@ 2023-01-21 19:02     ` Jonas Karlman
  2023-01-23 18:50       ` Simon Glass
  2023-01-21 19:02     ` [PATCH v3 6/6] rockchip: Use atf as firmware and move u-boot to loadables in FIT Jonas Karlman
                       ` (4 subsequent siblings)
  9 siblings, 1 reply; 39+ messages in thread
From: Jonas Karlman @ 2023-01-21 19:02 UTC (permalink / raw)
  To: Simon Glass, Philipp Tomsich, Kever Yang, Alper Nebi Yasak
  Cc: Quentin Schulz, u-boot, Jonas Karlman

In some cases it is desired for SPL to start TF-A instead of U-Boot
proper. Add support for a new property fit,firmware that picks a
valid entry and prepends the remaining valid entries to the
loadables list generated by the split-elf generator.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
v3:
- Introduce new fit,firmware property to handle firmware selection
v2:
- New patch

 tools/binman/entries.rst                      | 16 +++-
 tools/binman/etype/fit.py                     | 62 ++++++++++--
 tools/binman/ftest.py                         | 44 +++++++++
 .../test/276_fit_firmware_loadables.dts       | 96 +++++++++++++++++++
 4 files changed, 205 insertions(+), 13 deletions(-)
 create mode 100644 tools/binman/test/276_fit_firmware_loadables.dts

diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
index 78f95dae1a4e..7a04a613992d 100644
--- a/tools/binman/entries.rst
+++ b/tools/binman/entries.rst
@@ -721,6 +721,12 @@ split-elf
     fit,data
         Generates a `data = <...>` property with the contents of the segment
 
+    fit,firmware
+        Generates a `firmware = <...>` property. Provides a list of possible
+        nodes to be used as the `firmware` property value. The first valid
+        node is picked as the firmware. Any remaining valid nodes is
+        prepended to the `loadable` property generated by `fit,loadables`
+
     fit,loadables
         Generates a `loadable = <...>` property with a list of the generated
         nodes (including all nodes if this operation is used multiple times)
@@ -791,7 +797,7 @@ Here is an example showing ATF, TEE and a device tree all combined::
             @config-SEQ {
                 description = "conf-NAME.dtb";
                 fdt = "fdt-SEQ";
-                firmware = "u-boot";
+                fit,firmware = "atf-1", "u-boot";
                 fit,loadables;
             };
         };
@@ -846,15 +852,15 @@ is::
     configurations {
         default = "config-1";
         config-1 {
-            loadables = "atf-1", "atf-2", "atf-3", "tee-1", "tee-2";
+            loadables = "u-boot", "atf-2", "atf-3", "tee-1", "tee-2";
             description = "rk3399-firefly.dtb";
             fdt = "fdt-1";
-            firmware = "u-boot";
+            firmware = "atf-1";
         };
     };
 
-U-Boot SPL can then load the firmware (U-Boot proper) and all the loadables
-(ATF and TEE), then proceed with the boot.
+U-Boot SPL can then load the firmware (ATF) and all the loadables (U-Boot
+proper, ATF and TEE), then proceed with the boot.
 
 
 
diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
index bcb606f3f94c..cd2943533ce2 100644
--- a/tools/binman/etype/fit.py
+++ b/tools/binman/etype/fit.py
@@ -187,6 +187,12 @@ class Entry_fit(Entry_section):
         fit,data
             Generates a `data = <...>` property with the contents of the segment
 
+        fit,firmware
+            Generates a `firmware = <...>` property. Provides a list of possible
+            nodes to be used as the `firmware` property value. The first valid
+            node is picked as the firmware. Any remaining valid nodes is
+            prepended to the `loadable` property generated by `fit,loadables`
+
         fit,loadables
             Generates a `loadable = <...>` property with a list of the generated
             nodes (including all nodes if this operation is used multiple times)
@@ -257,7 +263,7 @@ class Entry_fit(Entry_section):
                 @config-SEQ {
                     description = "conf-NAME.dtb";
                     fdt = "fdt-SEQ";
-                    firmware = "u-boot";
+                    fit,firmware = "atf-1", "u-boot";
                     fit,loadables;
                 };
             };
@@ -312,15 +318,15 @@ class Entry_fit(Entry_section):
         configurations {
             default = "config-1";
             config-1 {
-                loadables = "atf-1", "atf-2", "atf-3", "tee-1", "tee-2";
+                loadables = "u-boot", "atf-2", "atf-3", "tee-1", "tee-2";
                 description = "rk3399-firefly.dtb";
                 fdt = "fdt-1";
-                firmware = "u-boot";
+                firmware = "atf-1";
             };
         };
 
-    U-Boot SPL can then load the firmware (U-Boot proper) and all the loadables
-    (ATF and TEE), then proceed with the boot.
+    U-Boot SPL can then load the firmware (ATF) and all the loadables (U-Boot
+    proper, ATF and TEE), then proceed with the boot.
     """
     def __init__(self, section, etype, node):
         """
@@ -510,6 +516,42 @@ class Entry_fit(Entry_section):
                 return
             fsw.property(pname, prop.bytes)
 
+        def _process_firmware_prop(node):
+            """Process optional fit,firmware property
+
+            Picks the first valid entry for use as the firmware, remaining valid
+            entries is prepended to loadables
+
+            Args:
+                node (Node): Generator node to process
+
+            Returns:
+                firmware (str): Firmware or None
+                result (list): List of remaining loadables
+            """
+            val = fdt_util.GetStringList(node, 'fit,firmware')
+            if val is None:
+                return None, self._loadables
+            valid_entries = list(self._loadables)
+            for name, entry in self.GetEntries().items():
+                missing = []
+                entry.CheckMissing(missing)
+                entry.CheckOptional(missing)
+                if not missing:
+                    valid_entries.append(name)
+            firmware = None
+            result = []
+            for name in val:
+                if name in valid_entries:
+                    if not firmware:
+                        firmware = name
+                    elif name not in result:
+                        result.append(name)
+            for name in self._loadables:
+                if name != firmware and name not in result:
+                    result.append(name)
+            return firmware, result
+
         def _gen_fdt_nodes(base_node, node, depth, in_images):
             """Generate FDT nodes
 
@@ -520,20 +562,24 @@ class Entry_fit(Entry_section):
             first.
 
             Args:
-                node (None): Generator node to process
+                node (Node): Generator node to process
                 depth: Current node depth (0 is the base 'fit' node)
                 in_images: True if this is inside the 'images' node, so that
                     'data' properties should be generated
             """
             if self._fdts:
+                firmware, fit_loadables = _process_firmware_prop(node)
                 # Generate nodes for each FDT
                 for seq, fdt_fname in enumerate(self._fdts):
                     node_name = node.name[1:].replace('SEQ', str(seq + 1))
                     fname = tools.get_input_filename(fdt_fname + '.dtb')
                     with fsw.add_node(node_name):
                         for pname, prop in node.props.items():
-                            if pname == 'fit,loadables':
-                                val = '\0'.join(self._loadables) + '\0'
+                            if pname == 'fit,firmware':
+                                if firmware:
+                                    fsw.property_string('firmware', firmware)
+                            elif pname == 'fit,loadables':
+                                val = '\0'.join(fit_loadables) + '\0'
                                 fsw.property('loadables', val.encode('utf-8'))
                             elif pname == 'fit,operation':
                                 pass
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index cd2757257178..0c4d34dfe480 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -6337,6 +6337,50 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
         }
         self.assertEqual(expected, props)
 
+    def testFitFirmwareLoadables(self):
+        """Test an image with an FIT that use fit,firmware"""
+        if not elf.ELF_TOOLS:
+            self.skipTest('Python elftools not available')
+        entry_args = {
+            'of-list': 'test-fdt1',
+            'default-dt': 'test-fdt1',
+            'atf-bl31-path': 'bl31.elf',
+            'tee-os-path': 'missing.bin',
+        }
+        test_subdir = os.path.join(self._indir, TEST_FDT_SUBDIR)
+        data = self._DoReadFileDtb(
+            '276_fit_firmware_loadables.dts',
+            entry_args=entry_args,
+            extra_indirs=[test_subdir])[0]
+
+        dtb = fdt.Fdt.FromData(data)
+        dtb.Scan()
+
+        node = dtb.GetNode('/configurations/conf-uboot-1')
+        self.assertEqual('u-boot', node.props['firmware'].value)
+        self.assertEqual(['atf-1', 'atf-2'],
+                         fdt_util.GetStringList(node, 'loadables'))
+
+        node = dtb.GetNode('/configurations/conf-atf-1')
+        self.assertEqual('atf-1', node.props['firmware'].value)
+        self.assertEqual(['u-boot', 'atf-2'],
+                         fdt_util.GetStringList(node, 'loadables'))
+
+        node = dtb.GetNode('/configurations/conf-missing-uboot-1')
+        self.assertEqual('u-boot', node.props['firmware'].value)
+        self.assertEqual(['atf-1', 'atf-2'],
+                         fdt_util.GetStringList(node, 'loadables'))
+
+        node = dtb.GetNode('/configurations/conf-missing-atf-1')
+        self.assertEqual('atf-1', node.props['firmware'].value)
+        self.assertEqual(['u-boot', 'atf-2'],
+                         fdt_util.GetStringList(node, 'loadables'))
+
+        node = dtb.GetNode('/configurations/conf-missing-tee-1')
+        self.assertEqual('atf-1', node.props['firmware'].value)
+        self.assertEqual(['u-boot', 'atf-2'],
+                         fdt_util.GetStringList(node, 'loadables'))
+
 
 if __name__ == "__main__":
     unittest.main()
diff --git a/tools/binman/test/276_fit_firmware_loadables.dts b/tools/binman/test/276_fit_firmware_loadables.dts
new file mode 100644
index 000000000000..4428e6a7e8e8
--- /dev/null
+++ b/tools/binman/test/276_fit_firmware_loadables.dts
@@ -0,0 +1,96 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	binman {
+		fit {
+			description = "test desc";
+			#address-cells = <1>;
+			fit,fdt-list = "of-list";
+
+			images {
+				u-boot {
+					description = "test u-boot";
+					type = "standalone";
+					arch = "arm64";
+					os = "u-boot";
+					compression = "none";
+					load = <0x00000000>;
+					entry = <0x00000000>;
+
+					u-boot-nodtb {
+					};
+				};
+				tee {
+					description = "test tee";
+					type = "tee";
+					arch = "arm64";
+					os = "tee";
+					compression = "none";
+					load = <0x00200000>;
+
+					tee-os {
+						optional;
+					};
+				};
+				@atf-SEQ {
+					fit,operation = "split-elf";
+					description = "test tf-a";
+					type = "firmware";
+					arch = "arm64";
+					os = "arm-trusted-firmware";
+					compression = "none";
+					fit,load;
+					fit,entry;
+					fit,data;
+
+					atf-bl31 {
+					};
+				};
+				@fdt-SEQ {
+					description = "test fdt";
+					type = "flat_dt";
+					compression = "none";
+				};
+			};
+
+			configurations {
+				default = "@conf-uboot-DEFAULT-SEQ";
+				@conf-uboot-SEQ {
+					description = "uboot config";
+					fdt = "fdt-SEQ";
+					fit,firmware = "u-boot";
+					fit,loadables;
+				};
+				@conf-atf-SEQ {
+					description = "atf config";
+					fdt = "fdt-SEQ";
+					fit,firmware = "atf-1", "u-boot";
+					fit,loadables;
+				};
+				@conf-missing-uboot-SEQ {
+					description = "missing uboot config";
+					fdt = "fdt-SEQ";
+					fit,firmware = "missing-1", "u-boot";
+					fit,loadables;
+				};
+				@conf-missing-atf-SEQ {
+					description = "missing atf config";
+					fdt = "fdt-SEQ";
+					fit,firmware = "missing-1", "atf-1", "u-boot";
+					fit,loadables;
+				};
+				@conf-missing-tee-SEQ {
+					description = "missing tee config";
+					fdt = "fdt-SEQ";
+					fit,firmware = "atf-1", "u-boot", "tee";
+					fit,loadables;
+				};
+			};
+		};
+	};
+};
-- 
2.39.1


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

* [PATCH v3 6/6] rockchip: Use atf as firmware and move u-boot to loadables in FIT
  2023-01-21 19:01   ` [PATCH v3 0/6] rockchip: Align FIT images to SD/MMC block length Jonas Karlman
                       ` (4 preceding siblings ...)
  2023-01-21 19:02     ` [PATCH v3 5/6] binman: Add support for selecting firmware to use with split-elf Jonas Karlman
@ 2023-01-21 19:02     ` Jonas Karlman
  2023-01-23 18:50       ` Simon Glass
  2023-01-26 17:48       ` Simon Glass
  2023-01-26 17:48     ` [PATCH v3 4/6] rockchip: Add sha256 hash to FIT images Simon Glass
                       ` (3 subsequent siblings)
  9 siblings, 2 replies; 39+ messages in thread
From: Jonas Karlman @ 2023-01-21 19:02 UTC (permalink / raw)
  To: Simon Glass, Philipp Tomsich, Kever Yang, Alper Nebi Yasak
  Cc: Quentin Schulz, u-boot, Jonas Karlman

The FIT generated after the switch to using binman is using different
values for firmware and loadables properties compared to the old script.

With the old script:
 firmware = "atf-1";
 loadables = "u-boot", "atf-2", ...;

After switch to binman:
 firmware = "u-boot";
 loadables = "atf-1", "atf-2", ...;

This change result in SPL jumping directly into U-Boot proper instead of
initializing TF-A.

With this patch the properties change back to:
 firmware = "atf-1";
 loatables = "u-boot", "atf-2", ...;

Fixes: e0c0efff2a02 ("rockchip: Support building the all output files in binman")
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
v3:
- Use fit,firmware property
v2:
- New patch

 arch/arm/dts/rockchip-u-boot.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
index e35902bb63a8..f147dc2066a0 100644
--- a/arch/arm/dts/rockchip-u-boot.dtsi
+++ b/arch/arm/dts/rockchip-u-boot.dtsi
@@ -114,7 +114,7 @@
 				@config-SEQ {
 					description = "NAME.dtb";
 					fdt = "fdt-SEQ";
-					firmware = "u-boot";
+					fit,firmware = "atf-1", "u-boot";
 					fit,loadables;
 				};
 			};
-- 
2.39.1


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

* Re: [PATCH v3 5/6] binman: Add support for selecting firmware to use with split-elf
  2023-01-21 19:02     ` [PATCH v3 5/6] binman: Add support for selecting firmware to use with split-elf Jonas Karlman
@ 2023-01-23 18:50       ` Simon Glass
  2023-01-24  8:53         ` Jonas Karlman
  2023-01-26 17:48         ` Simon Glass
  0 siblings, 2 replies; 39+ messages in thread
From: Simon Glass @ 2023-01-23 18:50 UTC (permalink / raw)
  To: Jonas Karlman
  Cc: Philipp Tomsich, Kever Yang, Alper Nebi Yasak, Quentin Schulz, u-boot

On Sat, 21 Jan 2023 at 12:02, Jonas Karlman <jonas@kwiboo.se> wrote:
>
> In some cases it is desired for SPL to start TF-A instead of U-Boot
> proper. Add support for a new property fit,firmware that picks a
> valid entry and prepends the remaining valid entries to the
> loadables list generated by the split-elf generator.
>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
> v3:
> - Introduce new fit,firmware property to handle firmware selection
> v2:
> - New patch
>
>  tools/binman/entries.rst                      | 16 +++-
>  tools/binman/etype/fit.py                     | 62 ++++++++++--
>  tools/binman/ftest.py                         | 44 +++++++++
>  .../test/276_fit_firmware_loadables.dts       | 96 +++++++++++++++++++
>  4 files changed, 205 insertions(+), 13 deletions(-)
>  create mode 100644 tools/binman/test/276_fit_firmware_loadables.dts

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

I think this is a good solution.

It does need one change, though. When using 'missing-1' this should
produce an error. We cannot silently skip things. If the
'allow-missing' flag is enabled, then it should collect them and
report them, while continuing execution. But they cannot be ignored.

Do you think you could fix that? It could be a follow-on patch though.

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

Regards,
Simon

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

* Re: [PATCH v3 6/6] rockchip: Use atf as firmware and move u-boot to loadables in FIT
  2023-01-21 19:02     ` [PATCH v3 6/6] rockchip: Use atf as firmware and move u-boot to loadables in FIT Jonas Karlman
@ 2023-01-23 18:50       ` Simon Glass
  2023-01-26 17:48       ` Simon Glass
  1 sibling, 0 replies; 39+ messages in thread
From: Simon Glass @ 2023-01-23 18:50 UTC (permalink / raw)
  To: Jonas Karlman
  Cc: Philipp Tomsich, Kever Yang, Alper Nebi Yasak, Quentin Schulz, u-boot

On Sat, 21 Jan 2023 at 12:02, Jonas Karlman <jonas@kwiboo.se> wrote:
>
> The FIT generated after the switch to using binman is using different
> values for firmware and loadables properties compared to the old script.
>
> With the old script:
>  firmware = "atf-1";
>  loadables = "u-boot", "atf-2", ...;
>
> After switch to binman:
>  firmware = "u-boot";
>  loadables = "atf-1", "atf-2", ...;
>
> This change result in SPL jumping directly into U-Boot proper instead of
> initializing TF-A.
>
> With this patch the properties change back to:
>  firmware = "atf-1";
>  loatables = "u-boot", "atf-2", ...;
>
> Fixes: e0c0efff2a02 ("rockchip: Support building the all output files in binman")
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
> v3:
> - Use fit,firmware property
> v2:
> - New patch
>
>  arch/arm/dts/rockchip-u-boot.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

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

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

* Re: [PATCH v3 5/6] binman: Add support for selecting firmware to use with split-elf
  2023-01-23 18:50       ` Simon Glass
@ 2023-01-24  8:53         ` Jonas Karlman
  2023-01-26 17:48         ` Simon Glass
  1 sibling, 0 replies; 39+ messages in thread
From: Jonas Karlman @ 2023-01-24  8:53 UTC (permalink / raw)
  To: Simon Glass
  Cc: Philipp Tomsich, Kever Yang, Alper Nebi Yasak, Quentin Schulz, u-boot

Hi Simon,

On 2023-01-23 19:50, Simon Glass wrote:
> On Sat, 21 Jan 2023 at 12:02, Jonas Karlman <jonas@kwiboo.se> wrote:
>>
>> In some cases it is desired for SPL to start TF-A instead of U-Boot
>> proper. Add support for a new property fit,firmware that picks a
>> valid entry and prepends the remaining valid entries to the
>> loadables list generated by the split-elf generator.
>>
>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>> ---
>> v3:
>> - Introduce new fit,firmware property to handle firmware selection
>> v2:
>> - New patch
>>
>>  tools/binman/entries.rst                      | 16 +++-
>>  tools/binman/etype/fit.py                     | 62 ++++++++++--
>>  tools/binman/ftest.py                         | 44 +++++++++
>>  .../test/276_fit_firmware_loadables.dts       | 96 +++++++++++++++++++
>>  4 files changed, 205 insertions(+), 13 deletions(-)
>>  create mode 100644 tools/binman/test/276_fit_firmware_loadables.dts
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> I think this is a good solution.
> 
> It does need one change, though. When using 'missing-1' this should
> produce an error. We cannot silently skip things. If the
> 'allow-missing' flag is enabled, then it should collect them and
> report them, while continuing execution. But they cannot be ignored.
> 
> Do you think you could fix that? It could be a follow-on patch though.

Sure, I can take a look at a follow-up patch next week.

Regards,
Jonas

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


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

* Re: [PATCH v3 6/6] rockchip: Use atf as firmware and move u-boot to loadables in FIT
  2023-01-21 19:02     ` [PATCH v3 6/6] rockchip: Use atf as firmware and move u-boot to loadables in FIT Jonas Karlman
  2023-01-23 18:50       ` Simon Glass
@ 2023-01-26 17:48       ` Simon Glass
  1 sibling, 0 replies; 39+ messages in thread
From: Simon Glass @ 2023-01-26 17:48 UTC (permalink / raw)
  To: Simon Glass
  Cc: Philipp Tomsich, Kever Yang, Alper Nebi Yasak, Quentin Schulz,
	u-boot, Jonas Karlman

On Sat, 21 Jan 2023 at 12:02, Jonas Karlman <jonas@kwiboo.se> wrote:
>
> The FIT generated after the switch to using binman is using different
> values for firmware and loadables properties compared to the old script.
>
> With the old script:
>  firmware = "atf-1";
>  loadables = "u-boot", "atf-2", ...;
>
> After switch to binman:
>  firmware = "u-boot";
>  loadables = "atf-1", "atf-2", ...;
>
> This change result in SPL jumping directly into U-Boot proper instead of
> initializing TF-A.
>
> With this patch the properties change back to:
>  firmware = "atf-1";
>  loatables = "u-boot", "atf-2", ...;
>
> Fixes: e0c0efff2a02 ("rockchip: Support building the all output files in binman")
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
> v3:
> - Use fit,firmware property
> v2:
> - New patch
>
>  arch/arm/dts/rockchip-u-boot.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

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

Applied to u-boot-dm, thanks!

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

* Re: [PATCH v3 5/6] binman: Add support for selecting firmware to use with split-elf
  2023-01-23 18:50       ` Simon Glass
  2023-01-24  8:53         ` Jonas Karlman
@ 2023-01-26 17:48         ` Simon Glass
  1 sibling, 0 replies; 39+ messages in thread
From: Simon Glass @ 2023-01-26 17:48 UTC (permalink / raw)
  To: Jonas Karlman
  Cc: Philipp Tomsich, Kever Yang, Alper Nebi Yasak, Quentin Schulz,
	u-boot, Simon Glass

Hi Simon,

On 2023-01-23 19:50, Simon Glass wrote:
> On Sat, 21 Jan 2023 at 12:02, Jonas Karlman <jonas@kwiboo.se> wrote:
>>
>> In some cases it is desired for SPL to start TF-A instead of U-Boot
>> proper. Add support for a new property fit,firmware that picks a
>> valid entry and prepends the remaining valid entries to the
>> loadables list generated by the split-elf generator.
>>
>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>> ---
>> v3:
>> - Introduce new fit,firmware property to handle firmware selection
>> v2:
>> - New patch
>>
>>  tools/binman/entries.rst                      | 16 +++-
>>  tools/binman/etype/fit.py                     | 62 ++++++++++--
>>  tools/binman/ftest.py                         | 44 +++++++++
>>  .../test/276_fit_firmware_loadables.dts       | 96 +++++++++++++++++++
>>  4 files changed, 205 insertions(+), 13 deletions(-)
>>  create mode 100644 tools/binman/test/276_fit_firmware_loadables.dts
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> I think this is a good solution.
>
> It does need one change, though. When using 'missing-1' this should
> produce an error. We cannot silently skip things. If the
> 'allow-missing' flag is enabled, then it should collect them and
> report them, while continuing execution. But they cannot be ignored.
>
> Do you think you could fix that? It could be a follow-on patch though.

Sure, I can take a look at a follow-up patch next week.

Regards,
Jonas

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


Applied to u-boot-dm, thanks!

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

* Re: [PATCH v3 4/6] rockchip: Add sha256 hash to FIT images
  2023-01-21 19:01   ` [PATCH v3 0/6] rockchip: Align FIT images to SD/MMC block length Jonas Karlman
                       ` (5 preceding siblings ...)
  2023-01-21 19:02     ` [PATCH v3 6/6] rockchip: Use atf as firmware and move u-boot to loadables in FIT Jonas Karlman
@ 2023-01-26 17:48     ` Simon Glass
  2023-01-26 17:48     ` [PATCH v3 3/6] binman: Add special subnodes to the nodes generated by split-elf Simon Glass
                       ` (2 subsequent siblings)
  9 siblings, 0 replies; 39+ messages in thread
From: Simon Glass @ 2023-01-26 17:48 UTC (permalink / raw)
  To: Jonas Karlman
  Cc: Quentin Schulz, u-boot, Simon Glass, Philipp Tomsich, Kever Yang,
	Alper Nebi Yasak

Add sha256 hash to FIT images when CONFIG_SPL_FIT_SIGNATURE=y.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
v2:
- Collect r-b tag

 arch/arm/dts/rockchip-u-boot.dtsi | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Applied to u-boot-dm, thanks!

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

* Re: [PATCH v3 3/6] binman: Add special subnodes to the nodes generated by split-elf
  2023-01-21 19:01   ` [PATCH v3 0/6] rockchip: Align FIT images to SD/MMC block length Jonas Karlman
                       ` (6 preceding siblings ...)
  2023-01-26 17:48     ` [PATCH v3 4/6] rockchip: Add sha256 hash to FIT images Simon Glass
@ 2023-01-26 17:48     ` Simon Glass
  2023-01-26 17:48     ` [PATCH v3 1/6] binman: Add support for align argument to mkimage tool Simon Glass
  2023-01-26 17:48     ` [PATCH v3 2/6] rockchip: Align FIT image data to SD/MMC block length Simon Glass
  9 siblings, 0 replies; 39+ messages in thread
From: Simon Glass @ 2023-01-26 17:48 UTC (permalink / raw)
  To: Jonas Karlman
  Cc: Quentin Schulz, u-boot, Simon Glass, Philipp Tomsich, Kever Yang,
	Alper Nebi Yasak

Special nodes, hash and signature, is not being added to the nodes
generated for each segment in split-elf operation.

Copy the subnode logic used in _gen_fdt_nodes to _gen_split_elf to
ensure special nodes are added to the generated nodes.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
v3:
- Collect r-b tag
v2:
- Add test
- Update commit message and documentation
- Update entries.rst

 tools/binman/entries.rst                | 14 ++++++++++++++
 tools/binman/etype/fit.py               | 23 +++++++++++++++++++++--
 tools/binman/ftest.py                   | 12 ++++++++++++
 tools/binman/test/226_fit_split_elf.dts |  6 ++++++
 4 files changed, 53 insertions(+), 2 deletions(-)

Applied to u-boot-dm, thanks!

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

* Re: [PATCH v3 1/6] binman: Add support for align argument to mkimage tool
  2023-01-21 19:01   ` [PATCH v3 0/6] rockchip: Align FIT images to SD/MMC block length Jonas Karlman
                       ` (7 preceding siblings ...)
  2023-01-26 17:48     ` [PATCH v3 3/6] binman: Add special subnodes to the nodes generated by split-elf Simon Glass
@ 2023-01-26 17:48     ` Simon Glass
  2023-01-26 17:48     ` [PATCH v3 2/6] rockchip: Align FIT image data to SD/MMC block length Simon Glass
  9 siblings, 0 replies; 39+ messages in thread
From: Simon Glass @ 2023-01-26 17:48 UTC (permalink / raw)
  To: Jonas Karlman
  Cc: Quentin Schulz, u-boot, Simon Glass, Philipp Tomsich, Kever Yang,
	Alper Nebi Yasak

Add support to indicate what alignment to use for the FIT and its
external data. Pass the alignment to mkimage via the -B flag.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
v3:
- Collect r-b tag
v2:
- Add test
- Update entries.rst

 tools/binman/btool/mkimage.py       |  5 ++-
 tools/binman/entries.rst            |  5 +++
 tools/binman/etype/fit.py           |  8 ++++
 tools/binman/ftest.py               | 16 ++++++++
 tools/binman/test/275_fit_align.dts | 59 +++++++++++++++++++++++++++++
 5 files changed, 92 insertions(+), 1 deletion(-)
 create mode 100644 tools/binman/test/275_fit_align.dts

Applied to u-boot-dm, thanks!

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

* Re: [PATCH v3 2/6] rockchip: Align FIT image data to SD/MMC block length
  2023-01-21 19:01   ` [PATCH v3 0/6] rockchip: Align FIT images to SD/MMC block length Jonas Karlman
                       ` (8 preceding siblings ...)
  2023-01-26 17:48     ` [PATCH v3 1/6] binman: Add support for align argument to mkimage tool Simon Glass
@ 2023-01-26 17:48     ` Simon Glass
  9 siblings, 0 replies; 39+ messages in thread
From: Simon Glass @ 2023-01-26 17:48 UTC (permalink / raw)
  To: Jonas Karlman
  Cc: Quentin Schulz, u-boot, Simon Glass, Philipp Tomsich, Kever Yang,
	Alper Nebi Yasak

SPL load FIT images by reading the data aligned to block length.
Block length aligned image data is read directly to the load address.
Unaligned image data is written to an offset of the load address and
then the data is memcpy to the load address.

This adds a small overhead of having to memcpy unaligned data, something
that normally is not an issue.

However, TF-A may have a segment that should be loaded into SRAM, e.g.
vendor TF-A for RK3568 has a 8KiB segment that should be loaded into the
8KiB PMU SRAM. Having the image data for such segment unaligned result
in segment being written to and memcpy from beyond the SRAM boundary, in
the end this results in invalid data in SRAM.

Aligning the FIT and its external data to MMC block length to work
around such issue.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
v2:
- Collect r-b tag

 arch/arm/dts/rockchip-u-boot.dtsi | 1 +
 1 file changed, 1 insertion(+)

Applied to u-boot-dm, thanks!

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

end of thread, other threads:[~2023-01-26 17:50 UTC | newest]

Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-17 22:54 [PATCH 0/4] rockchip: Align FIT images to SD/MMC block length Jonas Karlman
2023-01-17 22:54 ` [PATCH 1/4] binman: Add support for align argument to mkimage tool Jonas Karlman
2023-01-18 19:42   ` Simon Glass
2023-01-19 12:40     ` Jonas Karlman
2023-01-19 16:20       ` Simon Glass
2023-01-17 22:54 ` [PATCH 2/4] rockchip: Align FIT image data to SD/MMC block length Jonas Karlman
2023-01-18 19:42   ` Simon Glass
2023-01-17 22:54 ` [PATCH 3/4] binman: Add subnodes to the nodes generated by split-elf Jonas Karlman
2023-01-18 19:42   ` Simon Glass
2023-01-17 22:55 ` [PATCH 4/4] rockchip: Add sha256 hash to FIT images Jonas Karlman
2023-01-18 19:42   ` Simon Glass
2023-01-20  8:26 ` [PATCH v2 0/6] rockchip: Align FIT images to SD/MMC block length Jonas Karlman
2023-01-20  8:26   ` [PATCH v2 2/6] rockchip: Align FIT image data " Jonas Karlman
2023-01-20  8:26   ` [PATCH v2 1/6] binman: Add support for align argument to mkimage tool Jonas Karlman
2023-01-20 19:19     ` Simon Glass
2023-01-20  8:26   ` [PATCH v2 3/6] binman: Add special subnodes to the nodes generated by split-elf Jonas Karlman
2023-01-20 19:19     ` Simon Glass
2023-01-20  8:26   ` [PATCH v2 4/6] rockchip: Add sha256 hash to FIT images Jonas Karlman
2023-01-20  8:26   ` [PATCH v2 5/6] binman: Add support for prepending loadables with split-elf Jonas Karlman
2023-01-20 19:19     ` Simon Glass
2023-01-20 20:46       ` Jonas Karlman
2023-01-20 21:11         ` Simon Glass
2023-01-20  8:27   ` [PATCH v2 6/6] rockchip: Use atf as firmware and move u-boot to loadables in FIT Jonas Karlman
2023-01-21 19:01   ` [PATCH v3 0/6] rockchip: Align FIT images to SD/MMC block length Jonas Karlman
2023-01-21 19:01     ` [PATCH v3 1/6] binman: Add support for align argument to mkimage tool Jonas Karlman
2023-01-21 19:01     ` [PATCH v3 2/6] rockchip: Align FIT image data to SD/MMC block length Jonas Karlman
2023-01-21 19:01     ` [PATCH v3 3/6] binman: Add special subnodes to the nodes generated by split-elf Jonas Karlman
2023-01-21 19:01     ` [PATCH v3 4/6] rockchip: Add sha256 hash to FIT images Jonas Karlman
2023-01-21 19:02     ` [PATCH v3 5/6] binman: Add support for selecting firmware to use with split-elf Jonas Karlman
2023-01-23 18:50       ` Simon Glass
2023-01-24  8:53         ` Jonas Karlman
2023-01-26 17:48         ` Simon Glass
2023-01-21 19:02     ` [PATCH v3 6/6] rockchip: Use atf as firmware and move u-boot to loadables in FIT Jonas Karlman
2023-01-23 18:50       ` Simon Glass
2023-01-26 17:48       ` Simon Glass
2023-01-26 17:48     ` [PATCH v3 4/6] rockchip: Add sha256 hash to FIT images Simon Glass
2023-01-26 17:48     ` [PATCH v3 3/6] binman: Add special subnodes to the nodes generated by split-elf Simon Glass
2023-01-26 17:48     ` [PATCH v3 1/6] binman: Add support for align argument to mkimage tool Simon Glass
2023-01-26 17:48     ` [PATCH v3 2/6] rockchip: Align FIT image data to SD/MMC block length Simon Glass

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