[PATCH 00/24] binman: rockchip: Migrate from rockchip SPL_FIT_GENERATOR script

[PATCH 00/24] binman: rockchip: Migrate from rockchip SPL_FIT_GENERATOR script

[Simon Glass]

At present rockchip 64-bit boards make use of a FIT-generator script
written in Python. The script supports splitting an ELF file into several
'loadable' nodes in the FIT. Binman does not current support this feature.

This series adds binman support for ELF splitting. This works by adding a
new 'fit,operation' property to the FIT subnodes, allowing this new way of
generating nodes.

Some other fixes and improvements are needed along the way.

A new, common binman description is added for 64-bit boards which includes
the required u-boot.itb file.

The existing script is removed, so that only a few zynq boards are now
using a SPL_FIT_GENERATOR script:

   avnet_ultrazedev_cc_v1_0_ultrazedev_som_v1_0
   xilinx_zynqmp_virt

Migration of those is hopefully in progress.

Note however that tools/k3_fit_atf.sh remains, used by a few boards that
enable CONFIG_TI_SECURE_DEVICE so this series is copied there too:

    am335x_hs_evm
    am335x_hs_evm_uart
    am43xx_hs_evm
    am57xx_hs_evm
    am57xx_hs_evm_usb
    am65x_hs_evm_a53
    am65x_hs_evm_r5
    dra7xx_hs_evm
    dra7xx_hs_evm_usb
    j721e_hs_evm_a72
    j721e_hs_evm_r5
    k2e_hs_evm
    k2g_hs_evm
    k2hk_hs_evm
    k2l_hs_evm

Ivan Mikhaylov has sent a patch to help with these, but I need to take a
look at the testing side. In any case they should really be using binman
for the image generation.

This series also includes some moveconfig improvements that were used to
create board lists in this cover letter.


Simon Glass (24):
  moveconfig: Show the config name rather than the defconfig
  moveconfig: Allow regex matches when finding combinations
  spl: x86: Correct the binman symbols for SPL
  spl: Allow disabling binman symbols in SPL
  rockchip: evb-rk3288: Drop raw-image support
  dtoc: Support adding a string list to a device tree
  dtoc: Support deleting a node
  dtoc: Allow deleting nodes and adding them in the same sync
  dtoc: Support reading a list of arguments
  binman: Update docs to indicate mkimage is supported
  elf: Add a way to read segment information from an ELF file
  WIP: binman: Add support for OP-TEE
  binman: Add to the TODO
  binman: Support a list of strings with the mkimage etype
  binman: Add a ELF test file with disjoint text sections
  binman: Move entry-data collection into a Entry method
  binman: fit: Refactor to reduce function size
  binman: Tidy up the docs a little with fit
  binman: Allow different operations in FIT generator nodes
  binman: Support splitting an ELF file into multiple nodes
  rockchip: Include binman script in 64-bit boards
  rockchip: Support building the all output files in binman
  rockchip: Convert all boards to use binman
  rockchip: Drop the FIT generator script

 Makefile                                     |  43 +-
 arch/arm/dts/px30-u-boot.dtsi                |   2 +
 arch/arm/dts/rk3308-u-boot.dtsi              |   2 +
 arch/arm/dts/rk3326-odroid-go2-u-boot.dtsi   |   2 +
 arch/arm/dts/rk3328-u-boot.dtsi              |   2 +
 arch/arm/dts/rk3368-u-boot.dtsi              |   1 +
 arch/arm/dts/rk3399-u-boot.dtsi              |   5 +-
 arch/arm/dts/rk3568-u-boot.dtsi              |   2 +
 arch/arm/dts/rockchip-u-boot.dtsi            |  84 +++-
 arch/arm/mach-rockchip/Kconfig               |   6 +
 arch/arm/mach-rockchip/make_fit_atf.py       | 240 ---------
 arch/x86/dts/u-boot.dtsi                     |   2 +-
 boot/Kconfig                                 |   3 +-
 common/spl/Kconfig                           |  24 +
 common/spl/spl.c                             |  12 +-
 configs/evb-rk3288_defconfig                 |   1 +
 include/spl.h                                |   4 +-
 tools/binman/binman.rst                      |  98 +++-
 tools/binman/elf.py                          |  37 ++
 tools/binman/elf_test.py                     |  31 +-
 tools/binman/entries.rst                     | 260 +++++++++-
 tools/binman/entry.py                        |  28 ++
 tools/binman/etype/fit.py                    | 490 ++++++++++++++++---
 tools/binman/etype/mkimage.py                |  27 +-
 tools/binman/etype/op_tee.py                 |  23 +
 tools/binman/ftest.py                        | 123 +++++
 tools/binman/missing-blob-help               |   4 +
 tools/binman/test/220_fit_bad_oper.dts       |  27 +
 tools/binman/test/221_fit_split_elf.dts      |  67 +++
 tools/binman/test/222_fit_bad_dir.dts        |   9 +
 tools/binman/test/223_fit_bad_dir_config.dts |   9 +
 tools/binman/test/Makefile                   |   6 +-
 tools/binman/test/elf_sections.c             |  20 +
 tools/binman/test/elf_sections.lds           |  31 ++
 tools/dtoc/fdt.py                            |  37 ++
 tools/dtoc/fdt_util.py                       |  12 +
 tools/dtoc/test/dtoc_test_simple.dts         |   1 +
 tools/dtoc/test_fdt.py                       |  43 ++
 tools/moveconfig.py                          |  39 +-
 39 files changed, 1450 insertions(+), 407 deletions(-)
 delete mode 100755 arch/arm/mach-rockchip/make_fit_atf.py
 create mode 100644 tools/binman/etype/op_tee.py
 create mode 100644 tools/binman/test/220_fit_bad_oper.dts
 create mode 100644 tools/binman/test/221_fit_split_elf.dts
 create mode 100644 tools/binman/test/222_fit_bad_dir.dts
 create mode 100644 tools/binman/test/223_fit_bad_dir_config.dts
 create mode 100644 tools/binman/test/elf_sections.c
 create mode 100644 tools/binman/test/elf_sections.lds

-- 
2.35.0.263.gb82422642f-goog

[PATCH 01/24] moveconfig: Show the config name rather than the defconfig

[Simon Glass]

The _defconfig suffix is unnecessary when showing matching boards. Drop
it.

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

 tools/moveconfig.py | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/tools/moveconfig.py b/tools/moveconfig.py
index 1bcf58caf1..5ef5a95eb6 100755
--- a/tools/moveconfig.py
+++ b/tools/moveconfig.py
@@ -91,7 +91,20 @@ SIZES = {
     'SZ_4G':  0x100000000
 }
 
+RE_REMOVE_DEFCONFIG = re.compile(r'(.*)_defconfig')
+
 ### helper functions ###
+def remove_defconfig(defc):
+    """Drop the _defconfig suffix on a string
+
+    Args:
+        defc (str): String to convert
+
+    Returns:
+        str: string with the '_defconfig' suffix removed
+    """
+    return RE_REMOVE_DEFCONFIG.match(defc)[1]
+
 def check_top_directory():
     """Exit if we are not at the top of source directory."""
     for fname in 'README', 'Licenses':
@@ -1638,7 +1651,7 @@ def do_find_config(config_list):
         print(f"Error: Not in Kconfig: %s" % ' '.join(adhoc))
     else:
         print(f'{len(out)} matches')
-        print(' '.join(out))
+        print(' '.join([remove_defconfig(item) for item in out]))
 
 
 def prefix_config(cfg):
-- 
2.35.0.263.gb82422642f-goog

Re: [PATCH 01/24] moveconfig: Show the config name rather than the defconfig

[Alper Nebi Yasak]

On 08/02/2022 21:49, Simon Glass wrote:
> The _defconfig suffix is unnecessary when showing matching boards. Drop
> it.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  tools/moveconfig.py | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/moveconfig.py b/tools/moveconfig.py
> index 1bcf58caf1..5ef5a95eb6 100755
> --- a/tools/moveconfig.py
> +++ b/tools/moveconfig.py
> @@ -91,7 +91,20 @@ SIZES = {
>      'SZ_4G':  0x100000000
>  }
>  
> +RE_REMOVE_DEFCONFIG = re.compile(r'(.*)_defconfig')
> +
>  ### helper functions ###
> +def remove_defconfig(defc):
> +    """Drop the _defconfig suffix on a string
> +
> +    Args:
> +        defc (str): String to convert
> +
> +    Returns:
> +        str: string with the '_defconfig' suffix removed
> +    """
> +    return RE_REMOVE_DEFCONFIG.match(defc)[1]
> +

I think defc.split('_defconfig')[0] (or defc.partition) would work good
enough here and this function may be unnecessary.

>  def check_top_directory():
>      """Exit if we are not at the top of source directory."""
>      for fname in 'README', 'Licenses':
> @@ -1638,7 +1651,7 @@ def do_find_config(config_list):
>          print(f"Error: Not in Kconfig: %s" % ' '.join(adhoc))
>      else:
>          print(f'{len(out)} matches')
> -        print(' '.join(out))
> +        print(' '.join([remove_defconfig(item) for item in out]))

The square brackets aren't necessary here, but not really harmful either.

>  
>  
>  def prefix_config(cfg):

Re: [PATCH 01/24] moveconfig: Show the config name rather than the defconfig

[Simon Glass]

On 08/02/2022 21:49, Simon Glass wrote:
> The _defconfig suffix is unnecessary when showing matching boards. Drop
> it.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  tools/moveconfig.py | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
>
Applied to u-boot-dm, thanks!

Re: [PATCH 01/24] moveconfig: Show the config name rather than the defconfig

[Simon Glass]

Hi Alper,

On Tue, 22 Feb 2022 at 19:35, Simon Glass <sjg@chromium.org> wrote:
>
> On 08/02/2022 21:49, Simon Glass wrote:
> > The _defconfig suffix is unnecessary when showing matching boards. Drop
> > it.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  tools/moveconfig.py | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> >
> Applied to u-boot-dm, thanks!

Well I completely missed all your comments. If the pull request
doesn't work I'll give it another go, but for now I will do a few new
patches with your comments addressed. I haven't made much time for
things lately.

Regards,
Simon

[PATCH 02/24] moveconfig: Allow regex matches when finding combinations

[Simon Glass]

It is useful to be able to search for CONFIG options that match a regex,
such as this, which lists boards which define SPL_FIT_GENERATOR and
anything not starting with ROCKCHIP:

   ./tools/moveconfig.py -f SPL_FIT_GENERATOR ~ROCKCHIP.*

Add support for this.

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

 tools/moveconfig.py | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/tools/moveconfig.py b/tools/moveconfig.py
index 5ef5a95eb6..cff1e30658 100755
--- a/tools/moveconfig.py
+++ b/tools/moveconfig.py
@@ -1606,12 +1606,31 @@ def do_imply_config(config_list, add_imply, imply_flags, skip_added,
             for linenum in sorted(linenums, reverse=True):
                 add_imply_rule(config[CONFIG_LEN:], fname, linenum)
 
+def defconfig_matches(configs, re_match):
+    """Check if any CONFIG option matches a regex
+
+    The match must be complete, i.e. from the start to end of the CONFIG option.
+
+    Args:
+        configs (dict): Dict of CONFIG options:
+            key: CONFIG option
+            value: Value of option
+        re_match (re.Pattern): Match to check
+
+    Returns:
+        bool: True if any CONFIG matches the regex
+    """
+    for cfg in configs:
+        m_cfg = re_match.match(cfg)
+        if m_cfg and m_cfg.span()[1] == len(cfg):
+            return True
+    return False
 
 def do_find_config(config_list):
     """Find boards with a given combination of CONFIGs
 
     Params:
-        config_list: List of CONFIG options to check (each a string consisting
+        config_list: List of CONFIG options to check (each a regex consisting
             of a config option, with or without a CONFIG_ prefix. If an option
             is preceded by a tilde (~) then it must be false, otherwise it must
             be true)
@@ -1643,8 +1662,9 @@ def do_find_config(config_list):
         # running for the next stage
         in_list = out
         out = set()
+        re_match = re.compile(cfg)
         for defc in in_list:
-            has_cfg = cfg in config_db[defc]
+            has_cfg = defconfig_matches(config_db[defc], re_match)
             if has_cfg == want:
                 out.add(defc)
     if adhoc:
-- 
2.35.0.263.gb82422642f-goog

Re: [PATCH 02/24] moveconfig: Allow regex matches when finding combinations

[Alper Nebi Yasak]

On 08/02/2022 21:49, Simon Glass wrote:
> It is useful to be able to search for CONFIG options that match a regex,
> such as this, which lists boards which define SPL_FIT_GENERATOR and
> anything not starting with ROCKCHIP:
> 
>    ./tools/moveconfig.py -f SPL_FIT_GENERATOR ~ROCKCHIP.*
> 
> Add support for this.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  tools/moveconfig.py | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/moveconfig.py b/tools/moveconfig.py
> index 5ef5a95eb6..cff1e30658 100755
> --- a/tools/moveconfig.py
> +++ b/tools/moveconfig.py
> @@ -1606,12 +1606,31 @@ def do_imply_config(config_list, add_imply, imply_flags, skip_added,
>              for linenum in sorted(linenums, reverse=True):
>                  add_imply_rule(config[CONFIG_LEN:], fname, linenum)
>  
> +def defconfig_matches(configs, re_match):
> +    """Check if any CONFIG option matches a regex
> +
> +    The match must be complete, i.e. from the start to end of the CONFIG option.
> +
> +    Args:
> +        configs (dict): Dict of CONFIG options:
> +            key: CONFIG option
> +            value: Value of option
> +        re_match (re.Pattern): Match to check
> +
> +    Returns:
> +        bool: True if any CONFIG matches the regex
> +    """
> +    for cfg in configs:
> +        m_cfg = re_match.match(cfg)
> +        if m_cfg and m_cfg.span()[1] == len(cfg):
> +            return True
> +    return False

Consider re_match.fullmatch() to match the entire string. I think this
can be further reduced into any(map(re_match.fullmatch, configs)) and
used directly below instead of adding this extra function.

>  
>  def do_find_config(config_list):
>      """Find boards with a given combination of CONFIGs
>  
>      Params:
> -        config_list: List of CONFIG options to check (each a string consisting
> +        config_list: List of CONFIG options to check (each a regex consisting
>              of a config option, with or without a CONFIG_ prefix. If an option
>              is preceded by a tilde (~) then it must be false, otherwise it must
>              be true)
> @@ -1643,8 +1662,9 @@ def do_find_config(config_list):
>          # running for the next stage
>          in_list = out
>          out = set()
> +        re_match = re.compile(cfg)
>          for defc in in_list:
> -            has_cfg = cfg in config_db[defc]
> +            has_cfg = defconfig_matches(config_db[defc], re_match)
>              if has_cfg == want:
>                  out.add(defc)
>      if adhoc:

Re: [PATCH 02/24] moveconfig: Allow regex matches when finding combinations

[Simon Glass]

On 08/02/2022 21:49, Simon Glass wrote:
> It is useful to be able to search for CONFIG options that match a regex,
> such as this, which lists boards which define SPL_FIT_GENERATOR and
> anything not starting with ROCKCHIP:
>
>    ./tools/moveconfig.py -f SPL_FIT_GENERATOR ~ROCKCHIP.*
>
> Add support for this.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  tools/moveconfig.py | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
>
Applied to u-boot-dm, thanks!

[PATCH 03/24] spl: x86: Correct the binman symbols for SPL

[Simon Glass]

These symbols are incorrect, meaning that binman cannot find the
associated entry. This leads to errors like:

binman: Section '/binman/simple-bin': Symbol '_binman_spl_prop_size'
   in entry '/binman/simple-bin/u-boot-spl/u-boot-spl-nodtb':
   Entry 'spl' not found in list (mkimage,u-boot-spl-nodtb,
   u-boot-spl-bss-pad,u-boot-spl-dtb,u-boot-spl,u-boot-img,main-section)

Fix it.

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

 arch/x86/dts/u-boot.dtsi | 2 +-
 common/spl/spl.c         | 8 ++++----
 include/spl.h            | 4 ++--
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/x86/dts/u-boot.dtsi b/arch/x86/dts/u-boot.dtsi
index ca84d18ad9..24e692f988 100644
--- a/arch/x86/dts/u-boot.dtsi
+++ b/arch/x86/dts/u-boot.dtsi
@@ -37,7 +37,7 @@
 	u-boot-tpl-dtb {
 	};
 #endif
-	spl {
+	u-boot-spl {
 		type = "u-boot-spl";
 		offset = <CONFIG_X86_OFFSET_SPL>;
 	};
diff --git a/common/spl/spl.c b/common/spl/spl.c
index 884102bdea..444907432c 100644
--- a/common/spl/spl.c
+++ b/common/spl/spl.c
@@ -54,8 +54,8 @@ binman_sym_declare(ulong, u_boot_any, image_pos);
 binman_sym_declare(ulong, u_boot_any, size);
 
 #ifdef CONFIG_TPL
-binman_sym_declare(ulong, spl, image_pos);
-binman_sym_declare(ulong, spl, size);
+binman_sym_declare(ulong, u_boot_spl, image_pos);
+binman_sym_declare(ulong, u_boot_spl, size);
 #endif
 
 /* Define board data structure */
@@ -143,14 +143,14 @@ void spl_fixup_fdt(void *fdt_blob)
 ulong spl_get_image_pos(void)
 {
 	return spl_phase() == PHASE_TPL ?
-		binman_sym(ulong, spl, image_pos) :
+		binman_sym(ulong, u_boot_spl, image_pos) :
 		binman_sym(ulong, u_boot_any, image_pos);
 }
 
 ulong spl_get_image_size(void)
 {
 	return spl_phase() == PHASE_TPL ?
-		binman_sym(ulong, spl, size) :
+		binman_sym(ulong, u_boot_spl, size) :
 		binman_sym(ulong, u_boot_any, size);
 }
 
diff --git a/include/spl.h b/include/spl.h
index bb92bc6ec6..8ceb3c0f09 100644
--- a/include/spl.h
+++ b/include/spl.h
@@ -269,8 +269,8 @@ struct spl_load_info {
  */
 binman_sym_extern(ulong, u_boot_any, image_pos);
 binman_sym_extern(ulong, u_boot_any, size);
-binman_sym_extern(ulong, spl, image_pos);
-binman_sym_extern(ulong, spl, size);
+binman_sym_extern(ulong, u_boot_spl, image_pos);
+binman_sym_extern(ulong, u_boot_spl, size);
 
 /**
  * spl_get_image_pos() - get the image position of the next phase
-- 
2.35.0.263.gb82422642f-goog

Re: [PATCH 03/24] spl: x86: Correct the binman symbols for SPL

[Alper Nebi Yasak]

On 08/02/2022 21:49, Simon Glass wrote:
> These symbols are incorrect, meaning that binman cannot find the
> associated entry. This leads to errors like:
> 
> binman: Section '/binman/simple-bin': Symbol '_binman_spl_prop_size'
>    in entry '/binman/simple-bin/u-boot-spl/u-boot-spl-nodtb':
>    Entry 'spl' not found in list (mkimage,u-boot-spl-nodtb,
>    u-boot-spl-bss-pad,u-boot-spl-dtb,u-boot-spl,u-boot-img,main-section)

I can't help but feel like this is a bug with entry expansion where the
name of the expanded node is ignored (and replaced by its type?) when it
comes to the symbols.

> 
> Fix it.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  arch/x86/dts/u-boot.dtsi | 2 +-
>  common/spl/spl.c         | 8 ++++----
>  include/spl.h            | 4 ++--
>  3 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/dts/u-boot.dtsi b/arch/x86/dts/u-boot.dtsi
> index ca84d18ad9..24e692f988 100644
> --- a/arch/x86/dts/u-boot.dtsi
> +++ b/arch/x86/dts/u-boot.dtsi
> @@ -37,7 +37,7 @@
>  	u-boot-tpl-dtb {
>  	};
>  #endif
> -	spl {
> +	u-boot-spl {
>  		type = "u-boot-spl";

I guess the type can be removed now that it's the same as the node name.

>  		offset = <CONFIG_X86_OFFSET_SPL>;
>  	};
> diff --git a/common/spl/spl.c b/common/spl/spl.c
> index 884102bdea..444907432c 100644
> --- a/common/spl/spl.c
> +++ b/common/spl/spl.c
> @@ -54,8 +54,8 @@ binman_sym_declare(ulong, u_boot_any, image_pos);
>  binman_sym_declare(ulong, u_boot_any, size);
>  
>  #ifdef CONFIG_TPL
> -binman_sym_declare(ulong, spl, image_pos);
> -binman_sym_declare(ulong, spl, size);
> +binman_sym_declare(ulong, u_boot_spl, image_pos);
> +binman_sym_declare(ulong, u_boot_spl, size);
>  #endif
>  
>  /* Define board data structure */
> @@ -143,14 +143,14 @@ void spl_fixup_fdt(void *fdt_blob)
>  ulong spl_get_image_pos(void)
>  {
>  	return spl_phase() == PHASE_TPL ?
> -		binman_sym(ulong, spl, image_pos) :
> +		binman_sym(ulong, u_boot_spl, image_pos) :
>  		binman_sym(ulong, u_boot_any, image_pos);
>  }
>  
>  ulong spl_get_image_size(void)
>  {
>  	return spl_phase() == PHASE_TPL ?
> -		binman_sym(ulong, spl, size) :
> +		binman_sym(ulong, u_boot_spl, size) :
>  		binman_sym(ulong, u_boot_any, size);
>  }
>  
> diff --git a/include/spl.h b/include/spl.h
> index bb92bc6ec6..8ceb3c0f09 100644
> --- a/include/spl.h
> +++ b/include/spl.h
> @@ -269,8 +269,8 @@ struct spl_load_info {
>   */
>  binman_sym_extern(ulong, u_boot_any, image_pos);
>  binman_sym_extern(ulong, u_boot_any, size);
> -binman_sym_extern(ulong, spl, image_pos);
> -binman_sym_extern(ulong, spl, size);
> +binman_sym_extern(ulong, u_boot_spl, image_pos);
> +binman_sym_extern(ulong, u_boot_spl, size);
>  
>  /**
>   * spl_get_image_pos() - get the image position of the next phase

Re: [PATCH 03/24] spl: x86: Correct the binman symbols for SPL

[Simon Glass]

Hi Alper,

On Tue, 15 Feb 2022 at 04:52, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> On 08/02/2022 21:49, Simon Glass wrote:
> > These symbols are incorrect, meaning that binman cannot find the
> > associated entry. This leads to errors like:
> >
> > binman: Section '/binman/simple-bin': Symbol '_binman_spl_prop_size'
> >    in entry '/binman/simple-bin/u-boot-spl/u-boot-spl-nodtb':
> >    Entry 'spl' not found in list (mkimage,u-boot-spl-nodtb,
> >    u-boot-spl-bss-pad,u-boot-spl-dtb,u-boot-spl,u-boot-img,main-section)
>
> I can't help but feel like this is a bug with entry expansion where the
> name of the expanded node is ignored (and replaced by its type?) when it
> comes to the symbols.

The problem is that there is only really one value for a symbol. E.g.
U-Boot has an image-pos and it doesn't matter what you call it; it is
the same value.

So does it make sense to disallow different names for the same thing?
See testSymbol() which actually creates two SPLs and checks that both
are updated. That is the opposite to what you are talking about, of
course, since it is the properties of the 'u-boot' entry which are
used to write into the SPL entries.

If we move to using the name instead, we could have two different
copies of U-Boot in the image and each SPL could refer to a different
one. At present this is done by puting the pairs into their own
section.

I think this needs more discussion....what do you think?

Regards,
Simon


>
> >
> > Fix it.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  arch/x86/dts/u-boot.dtsi | 2 +-
> >  common/spl/spl.c         | 8 ++++----
> >  include/spl.h            | 4 ++--
> >  3 files changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/x86/dts/u-boot.dtsi b/arch/x86/dts/u-boot.dtsi
> > index ca84d18ad9..24e692f988 100644
> > --- a/arch/x86/dts/u-boot.dtsi
> > +++ b/arch/x86/dts/u-boot.dtsi
> > @@ -37,7 +37,7 @@
> >       u-boot-tpl-dtb {
> >       };
> >  #endif
> > -     spl {
> > +     u-boot-spl {
> >               type = "u-boot-spl";
>
> I guess the type can be removed now that it's the same as the node name.
>
> >               offset = <CONFIG_X86_OFFSET_SPL>;
> >       };
> > diff --git a/common/spl/spl.c b/common/spl/spl.c
> > index 884102bdea..444907432c 100644
> > --- a/common/spl/spl.c
> > +++ b/common/spl/spl.c
> > @@ -54,8 +54,8 @@ binman_sym_declare(ulong, u_boot_any, image_pos);
> >  binman_sym_declare(ulong, u_boot_any, size);
> >
> >  #ifdef CONFIG_TPL
> > -binman_sym_declare(ulong, spl, image_pos);
> > -binman_sym_declare(ulong, spl, size);
> > +binman_sym_declare(ulong, u_boot_spl, image_pos);
> > +binman_sym_declare(ulong, u_boot_spl, size);
> >  #endif
> >
> >  /* Define board data structure */
> > @@ -143,14 +143,14 @@ void spl_fixup_fdt(void *fdt_blob)
> >  ulong spl_get_image_pos(void)
> >  {
> >       return spl_phase() == PHASE_TPL ?
> > -             binman_sym(ulong, spl, image_pos) :
> > +             binman_sym(ulong, u_boot_spl, image_pos) :
> >               binman_sym(ulong, u_boot_any, image_pos);
> >  }
> >
> >  ulong spl_get_image_size(void)
> >  {
> >       return spl_phase() == PHASE_TPL ?
> > -             binman_sym(ulong, spl, size) :
> > +             binman_sym(ulong, u_boot_spl, size) :
> >               binman_sym(ulong, u_boot_any, size);
> >  }
> >
> > diff --git a/include/spl.h b/include/spl.h
> > index bb92bc6ec6..8ceb3c0f09 100644
> > --- a/include/spl.h
> > +++ b/include/spl.h
> > @@ -269,8 +269,8 @@ struct spl_load_info {
> >   */
> >  binman_sym_extern(ulong, u_boot_any, image_pos);
> >  binman_sym_extern(ulong, u_boot_any, size);
> > -binman_sym_extern(ulong, spl, image_pos);
> > -binman_sym_extern(ulong, spl, size);
> > +binman_sym_extern(ulong, u_boot_spl, image_pos);
> > +binman_sym_extern(ulong, u_boot_spl, size);
> >
> >  /**
> >   * spl_get_image_pos() - get the image position of the next phase

Re: [PATCH 03/24] spl: x86: Correct the binman symbols for SPL

[Alper Nebi Yasak]

On 24/02/2022 01:58, Simon Glass wrote:
> Hi Alper,
> 
> On Tue, 15 Feb 2022 at 04:52, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>>
>> On 08/02/2022 21:49, Simon Glass wrote:
>>> These symbols are incorrect, meaning that binman cannot find the
>>> associated entry. This leads to errors like:
>>>
>>> binman: Section '/binman/simple-bin': Symbol '_binman_spl_prop_size'
>>>    in entry '/binman/simple-bin/u-boot-spl/u-boot-spl-nodtb':
>>>    Entry 'spl' not found in list (mkimage,u-boot-spl-nodtb,
>>>    u-boot-spl-bss-pad,u-boot-spl-dtb,u-boot-spl,u-boot-img,main-section)
>>
>> I can't help but feel like this is a bug with entry expansion where the
>> name of the expanded node is ignored (and replaced by its type?) when it
>> comes to the symbols.

I think I misunderstood that error message. I thought this error was for
a board with the x86 dtsi where it wasn't finding an "spl" entry that
actually existed. Not seeing "spl" in the list I assumed its name was
being lost for some reason.

Now I realize it must be for a board where "u-boot-spl" exists instead
of "spl" (which the symbol was looking for), and you were correcting the
symbol to match that and furthermore fixing the x86 dtsi to match the
corrected symbol.

(Perhaps it was a Rockchip board, related to the rest of the series :P )

> 
> The problem is that there is only really one value for a symbol. E.g.
> U-Boot has an image-pos and it doesn't matter what you call it; it is
> the same value.
> 
> So does it make sense to disallow different names for the same thing?
> See testSymbol() which actually creates two SPLs and checks that both
> are updated. That is the opposite to what you are talking about, of
> course, since it is the properties of the 'u-boot' entry which are
> used to write into the SPL entries.
> 
> If we move to using the name instead, we could have two different
> copies of U-Boot in the image and each SPL could refer to a different
> one. At present this is done by puting the pairs into their own
> section.
> 
> I think this needs more discussion....what do you think?

I think it's better to use the names, since there are reasonable cases
where an image would have multiple entries of the same type: A/B updates
and read-only recovery copies (like in Chrome OS firmware, or I guess
eventually with your VPL series?).

>From what I can tell, the symbols are indeed set based on the entry
names (not entry types), so multiple entries of the same type but with
different names should already be working fine -- except no symbols are
declared on the C side for arbitrary names. I guess putting multiple
copies in different sections with different "name-prefix" values works
fine the same way.

However I lightly suspect this might be breaking down a bit with entry
expansion, since the nodes generated in differently-named sections could
have the same name (the desired entry type), but didn't test it or
anything. I guess it works since the symbol would be declared for the
node-to-expand anyway, with the correctly unique name?

(Maybe the symbols might be based on the path instead, but that could be
very verbose. I see an idea in binman.rst for a C library that can read
binman things from device tree, which sounds nice for this as well.)

Re: [PATCH 03/24] spl: x86: Correct the binman symbols for SPL

[Simon Glass]

Hi Alper,

On Thu, 3 Mar 2022 at 14:14, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> On 24/02/2022 01:58, Simon Glass wrote:
> > Hi Alper,
> >
> > On Tue, 15 Feb 2022 at 04:52, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
> >>
> >> On 08/02/2022 21:49, Simon Glass wrote:
> >>> These symbols are incorrect, meaning that binman cannot find the
> >>> associated entry. This leads to errors like:
> >>>
> >>> binman: Section '/binman/simple-bin': Symbol '_binman_spl_prop_size'
> >>>    in entry '/binman/simple-bin/u-boot-spl/u-boot-spl-nodtb':
> >>>    Entry 'spl' not found in list (mkimage,u-boot-spl-nodtb,
> >>>    u-boot-spl-bss-pad,u-boot-spl-dtb,u-boot-spl,u-boot-img,main-section)
> >>
> >> I can't help but feel like this is a bug with entry expansion where the
> >> name of the expanded node is ignored (and replaced by its type?) when it
> >> comes to the symbols.
>
> I think I misunderstood that error message. I thought this error was for
> a board with the x86 dtsi where it wasn't finding an "spl" entry that
> actually existed. Not seeing "spl" in the list I assumed its name was
> being lost for some reason.
>
> Now I realize it must be for a board where "u-boot-spl" exists instead
> of "spl" (which the symbol was looking for), and you were correcting the
> symbol to match that and furthermore fixing the x86 dtsi to match the
> corrected symbol.
>
> (Perhaps it was a Rockchip board, related to the rest of the series :P )
>
> >
> > The problem is that there is only really one value for a symbol. E.g.
> > U-Boot has an image-pos and it doesn't matter what you call it; it is
> > the same value.
> >
> > So does it make sense to disallow different names for the same thing?
> > See testSymbol() which actually creates two SPLs and checks that both
> > are updated. That is the opposite to what you are talking about, of
> > course, since it is the properties of the 'u-boot' entry which are
> > used to write into the SPL entries.
> >
> > If we move to using the name instead, we could have two different
> > copies of U-Boot in the image and each SPL could refer to a different
> > one. At present this is done by puting the pairs into their own
> > section.
> >
> > I think this needs more discussion....what do you think?
>
> I think it's better to use the names, since there are reasonable cases
> where an image would have multiple entries of the same type: A/B updates
> and read-only recovery copies (like in Chrome OS firmware, or I guess
> eventually with your VPL series?).

Well that is already supported to some extent, they just need to go
into their own section. In fact I think we want things in their own
section anyway. For the Elf symbols, it picks the the value the entry
in the same section, so it is possible to have an A section and a B
section, each with its own SPL and U-Boot.

>
> >From what I can tell, the symbols are indeed set based on the entry
> names (not entry types), so multiple entries of the same type but with
> different names should already be working fine -- except no symbols are
> declared on the C side for arbitrary names. I guess putting multiple
> copies in different sections with different "name-prefix" values works
> fine the same way.

I think this could use a few tests to make this clear.

>
> However I lightly suspect this might be breaking down a bit with entry
> expansion, since the nodes generated in differently-named sections could
> have the same name (the desired entry type), but didn't test it or
> anything. I guess it works since the symbol would be declared for the
> node-to-expand anyway, with the correctly unique name?

Yes and often the node-to-expand is the thing you want anyway, since
you want to load the whole section.

>
> (Maybe the symbols might be based on the path instead, but that could be
> very verbose. I see an idea in binman.rst for a C library that can read
> binman things from device tree, which sounds nice for this as well.)

Yes we can do that but best done in the C library as you say. We have
a binman.c but it has very little functionality so far.

Regards,
Simon

Re: [PATCH 03/24] spl: x86: Correct the binman symbols for SPL

[Simon Glass]

On 08/02/2022 21:49, Simon Glass wrote:
> These symbols are incorrect, meaning that binman cannot find the
> associated entry. This leads to errors like:
>
> binman: Section '/binman/simple-bin': Symbol '_binman_spl_prop_size'
>    in entry '/binman/simple-bin/u-boot-spl/u-boot-spl-nodtb':
>    Entry 'spl' not found in list (mkimage,u-boot-spl-nodtb,
>    u-boot-spl-bss-pad,u-boot-spl-dtb,u-boot-spl,u-boot-img,main-section)

I can't help but feel like this is a bug with entry expansion where the
name of the expanded node is ignored (and replaced by its type?) when it
comes to the symbols.

>
> Fix it.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  arch/x86/dts/u-boot.dtsi | 2 +-
>  common/spl/spl.c         | 8 ++++----
>  include/spl.h            | 4 ++--
>  3 files changed, 7 insertions(+), 7 deletions(-)
>
Applied to u-boot-dm, thanks!

[PATCH 04/24] spl: Allow disabling binman symbols in SPL

[Simon Glass]

When CONFIG_SPL_FIT is enabled we do not access U-Boot directly in
the image, since it is embedded in a FIT which is parsed at runtime.

Provide a CONFIG option to drop the symbols in this case.

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

 common/spl/Kconfig | 24 ++++++++++++++++++++++++
 common/spl/spl.c   |  4 ++++
 2 files changed, 28 insertions(+)

diff --git a/common/spl/Kconfig b/common/spl/Kconfig
index e0d0a6f77b..9418d37b2e 100644
--- a/common/spl/Kconfig
+++ b/common/spl/Kconfig
@@ -101,6 +101,18 @@ config SPL_SHOW_ERRORS
 
 	  This adds a small amount to SPL code size, perhaps 100 bytes.
 
+config SPL_BINMAN_SYMBOLS
+	bool "Declare binman symbols in SPL"
+	depends on SPL_FRAMEWORK && BINMAN
+	default y
+	help
+	  This enables use of symbols in SPL which refer to U-Boot, enabling SPL
+	  to obtain the location of U-Boot simply by calling spl_get_image_pos()
+	  and spl_get_image_size().
+
+	  For this to work, you must have a U-Boot image in the binman image, so
+	  binman can update SPL with the location of it.
+
 menu "PowerPC and LayerScape SPL Boot options"
 
 config SPL_NAND_BOOT
@@ -1321,6 +1333,18 @@ config TPL_SIZE_LIMIT
 	  Specifies the maximum length of the U-Boot TPL image.
 	  If this value is zero, it is ignored.
 
+config TPL_BINMAN_SYMBOLS
+	bool "Declare binman symbols in SPL"
+	depends on SPL_FRAMEWORK && BINMAN
+	default y
+	help
+	  This enables use of symbols in TPL which refer to U-Boot, enabling SPL
+	  to obtain the location of U-Boot simply by calling spl_get_image_pos()
+	  and spl_get_image_size().
+
+	  For this to work, you must have a U-Boot image in the binman image, so
+	  binman can update SPL with the location of it.
+
 config TPL_FRAMEWORK
 	bool "Support TPL based upon the common SPL framework"
 	default y if SPL_FRAMEWORK
diff --git a/common/spl/spl.c b/common/spl/spl.c
index 444907432c..b452d4feeb 100644
--- a/common/spl/spl.c
+++ b/common/spl/spl.c
@@ -49,9 +49,11 @@ DECLARE_GLOBAL_DATA_PTR;
 
 u32 *boot_params_ptr = NULL;
 
+#if CONFIG_IS_ENABLED(BINMAN_SYMBOLS)
 /* See spl.h for information about this */
 binman_sym_declare(ulong, u_boot_any, image_pos);
 binman_sym_declare(ulong, u_boot_any, size);
+#endif
 
 #ifdef CONFIG_TPL
 binman_sym_declare(ulong, u_boot_spl, image_pos);
@@ -140,6 +142,7 @@ void spl_fixup_fdt(void *fdt_blob)
 #endif
 }
 
+#if CONFIG_IS_ENABLED(BINMAN_SYMBOLS)
 ulong spl_get_image_pos(void)
 {
 	return spl_phase() == PHASE_TPL ?
@@ -153,6 +156,7 @@ ulong spl_get_image_size(void)
 		binman_sym(ulong, u_boot_spl, size) :
 		binman_sym(ulong, u_boot_any, size);
 }
+#endif /* BINMAN_SYMBOLS */
 
 ulong spl_get_image_text_base(void)
 {
-- 
2.35.0.263.gb82422642f-goog

Re: [PATCH 04/24] spl: Allow disabling binman symbols in SPL

[Alper Nebi Yasak]

On 08/02/2022 21:49, Simon Glass wrote:
> When CONFIG_SPL_FIT is enabled we do not access U-Boot directly in
> the image, since it is embedded in a FIT which is parsed at runtime.
> 
> Provide a CONFIG option to drop the symbols in this case.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  common/spl/Kconfig | 24 ++++++++++++++++++++++++
>  common/spl/spl.c   |  4 ++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/common/spl/Kconfig b/common/spl/Kconfig
> index e0d0a6f77b..9418d37b2e 100644
> --- a/common/spl/Kconfig
> +++ b/common/spl/Kconfig
> @@ -101,6 +101,18 @@ config SPL_SHOW_ERRORS
>  
>  	  This adds a small amount to SPL code size, perhaps 100 bytes.
>  
> +config SPL_BINMAN_SYMBOLS
> +	bool "Declare binman symbols in SPL"
> +	depends on SPL_FRAMEWORK && BINMAN
> +	default y
> +	help
> +	  This enables use of symbols in SPL which refer to U-Boot, enabling SPL
> +	  to obtain the location of U-Boot simply by calling spl_get_image_pos()
> +	  and spl_get_image_size().
> +
> +	  For this to work, you must have a U-Boot image in the binman image, so
> +	  binman can update SPL with the location of it.
> +
>  menu "PowerPC and LayerScape SPL Boot options"
>  
>  config SPL_NAND_BOOT
> @@ -1321,6 +1333,18 @@ config TPL_SIZE_LIMIT
>  	  Specifies the maximum length of the U-Boot TPL image.
>  	  If this value is zero, it is ignored.
>  
> +config TPL_BINMAN_SYMBOLS
> +	bool "Declare binman symbols in SPL"

SPL -> TPL here and two more in the help text.

> +	depends on SPL_FRAMEWORK && BINMAN
> +	default y
> +	help
> +	  This enables use of symbols in TPL which refer to U-Boot, enabling SPL
> +	  to obtain the location of U-Boot simply by calling spl_get_image_pos()
> +	  and spl_get_image_size().
> +
> +	  For this to work, you must have a U-Boot image in the binman image, so
> +	  binman can update SPL with the location of it.
> +
>  config TPL_FRAMEWORK
>  	bool "Support TPL based upon the common SPL framework"
>  	default y if SPL_FRAMEWORK
> diff --git a/common/spl/spl.c b/common/spl/spl.c
> index 444907432c..b452d4feeb 100644
> --- a/common/spl/spl.c
> +++ b/common/spl/spl.c
> @@ -49,9 +49,11 @@ DECLARE_GLOBAL_DATA_PTR;
>  
>  u32 *boot_params_ptr = NULL;
>  
> +#if CONFIG_IS_ENABLED(BINMAN_SYMBOLS)
>  /* See spl.h for information about this */
>  binman_sym_declare(ulong, u_boot_any, image_pos);
>  binman_sym_declare(ulong, u_boot_any, size);
> +#endif
>  
>  #ifdef CONFIG_TPL
>  binman_sym_declare(ulong, u_boot_spl, image_pos);
> @@ -140,6 +142,7 @@ void spl_fixup_fdt(void *fdt_blob)
>  #endif
>  }
>  
> +#if CONFIG_IS_ENABLED(BINMAN_SYMBOLS)
>  ulong spl_get_image_pos(void)
>  {
>  	return spl_phase() == PHASE_TPL ?
> @@ -153,6 +156,7 @@ ulong spl_get_image_size(void)
>  		binman_sym(ulong, u_boot_spl, size) :
>  		binman_sym(ulong, u_boot_any, size);
>  }
> +#endif /* BINMAN_SYMBOLS */
>  
>  ulong spl_get_image_text_base(void)
>  {

Re: [PATCH 04/24] spl: Allow disabling binman symbols in SPL

[Simon Glass]

On 08/02/2022 21:49, Simon Glass wrote:
> When CONFIG_SPL_FIT is enabled we do not access U-Boot directly in
> the image, since it is embedded in a FIT which is parsed at runtime.
>
> Provide a CONFIG option to drop the symbols in this case.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  common/spl/Kconfig | 24 ++++++++++++++++++++++++
>  common/spl/spl.c   |  4 ++++
>  2 files changed, 28 insertions(+)
>
Applied to u-boot-dm, thanks!

[PATCH 05/24] rockchip: evb-rk3288: Drop raw-image support

[Simon Glass]

This boards uses SPL_FIT so does not need to support loading a raw image.
Drop it to avoid binman trying to insert a symbol which has no value.

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

 configs/evb-rk3288_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configs/evb-rk3288_defconfig b/configs/evb-rk3288_defconfig
index 97d4c14f65..6143c4d989 100644
--- a/configs/evb-rk3288_defconfig
+++ b/configs/evb-rk3288_defconfig
@@ -24,6 +24,7 @@ CONFIG_USE_PREBOOT=y
 CONFIG_DEFAULT_FDT_FILE="rk3288-evb-rk808.dtb"
 CONFIG_SILENT_CONSOLE=y
 CONFIG_DISPLAY_BOARDINFO_LATE=y
+# CONFIG_SPL_RAW_IMAGE_SUPPORT is not set
 CONFIG_SPL_STACK_R=y
 CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x10000
 CONFIG_SPL_OPTEE_IMAGE=y
-- 
2.35.0.263.gb82422642f-goog

[PATCH 06/24] dtoc: Support adding a string list to a device tree

[Simon Glass]

Add a new function to add a string list.

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

 tools/dtoc/fdt.py      | 18 ++++++++++++++++++
 tools/dtoc/test_fdt.py |  8 ++++++++
 2 files changed, 26 insertions(+)

diff --git a/tools/dtoc/fdt.py b/tools/dtoc/fdt.py
index 32a7aa9829..e7197f8f12 100644
--- a/tools/dtoc/fdt.py
+++ b/tools/dtoc/fdt.py
@@ -501,6 +501,24 @@ class Node:
         val = bytes(val, 'utf-8')
         return self.AddData(prop_name, val + b'\0')
 
+    def AddStringList(self, prop_name, val):
+        """Add a new string-list property to a node
+
+        The device tree is marked dirty so that the value will be written to
+        the blob on the next sync.
+
+        Args:
+            prop_name: Name of property to add
+            val (list of str): List of strings to add
+
+        Returns:
+            Prop added
+        """
+        out = b''
+        for string in val:
+            out += bytes(string, 'utf-8') + b'\0'
+        return self.AddData(prop_name, out)
+
     def AddInt(self, prop_name, val):
         """Add a new integer property to a node
 
diff --git a/tools/dtoc/test_fdt.py b/tools/dtoc/test_fdt.py
index 55b70e9876..1a7e73ffce 100755
--- a/tools/dtoc/test_fdt.py
+++ b/tools/dtoc/test_fdt.py
@@ -531,6 +531,14 @@ class TestProp(unittest.TestCase):
         self.node.AddData('data', tools.GetBytes(65, 20000))
         self.dtb.Sync(auto_resize=True)
 
+    def test_string_list(self):
+        """Test adding string-list property to a node"""
+        val = ['123', '456']
+        self.node.AddStringList('stringlist', val)
+        self.dtb.Sync(auto_resize=True)
+        data = self.fdt.getprop(self.node.Offset(), 'stringlist')
+        self.assertEqual(b'123\x00456\0', data)
+
     def testFromData(self):
         dtb2 = fdt.Fdt.FromData(self.dtb.GetContents())
         self.assertEqual(dtb2.GetContents(), self.dtb.GetContents())
-- 
2.35.0.263.gb82422642f-goog

Re: [PATCH 06/24] dtoc: Support adding a string list to a device tree

[Alper Nebi Yasak]

On 08/02/2022 21:49, Simon Glass wrote:
> Add a new function to add a string list.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  tools/dtoc/fdt.py      | 18 ++++++++++++++++++
>  tools/dtoc/test_fdt.py |  8 ++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/tools/dtoc/fdt.py b/tools/dtoc/fdt.py
> index 32a7aa9829..e7197f8f12 100644
> --- a/tools/dtoc/fdt.py
> +++ b/tools/dtoc/fdt.py
> @@ -501,6 +501,24 @@ class Node:
>          val = bytes(val, 'utf-8')
>          return self.AddData(prop_name, val + b'\0')
>  
> +    def AddStringList(self, prop_name, val):
> +        """Add a new string-list property to a node
> +
> +        The device tree is marked dirty so that the value will be written to
> +        the blob on the next sync.
> +
> +        Args:
> +            prop_name: Name of property to add
> +            val (list of str): List of strings to add
> +
> +        Returns:
> +            Prop added
> +        """
> +        out = b''
> +        for string in val:
> +            out += bytes(string, 'utf-8') + b'\0'
> +        return self.AddData(prop_name, out)

If val is an empty list this would try to set a zero-byte data, but then
the Prop class considers that a boolean True.

Perhaps use "s" as the variable name as there's a "string" module
(though not imported here).

Also, b'\0'.join() works just like in strings if you prefer that to a loop.

> +
>      def AddInt(self, prop_name, val):
>          """Add a new integer property to a node
>  
> diff --git a/tools/dtoc/test_fdt.py b/tools/dtoc/test_fdt.py
> index 55b70e9876..1a7e73ffce 100755
> --- a/tools/dtoc/test_fdt.py
> +++ b/tools/dtoc/test_fdt.py
> @@ -531,6 +531,14 @@ class TestProp(unittest.TestCase):
>          self.node.AddData('data', tools.GetBytes(65, 20000))
>          self.dtb.Sync(auto_resize=True)
>  
> +    def test_string_list(self):
> +        """Test adding string-list property to a node"""
> +        val = ['123', '456']
> +        self.node.AddStringList('stringlist', val)
> +        self.dtb.Sync(auto_resize=True)
> +        data = self.fdt.getprop(self.node.Offset(), 'stringlist')
> +        self.assertEqual(b'123\x00456\0', data)
> +
>      def testFromData(self):
>          dtb2 = fdt.Fdt.FromData(self.dtb.GetContents())
>          self.assertEqual(dtb2.GetContents(), self.dtb.GetContents())

Re: [PATCH 06/24] dtoc: Support adding a string list to a device tree

[Simon Glass]

Hi Alper,

On Tue, 15 Feb 2022 at 04:53, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> On 08/02/2022 21:49, Simon Glass wrote:
> > Add a new function to add a string list.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  tools/dtoc/fdt.py      | 18 ++++++++++++++++++
> >  tools/dtoc/test_fdt.py |  8 ++++++++
> >  2 files changed, 26 insertions(+)
> >
> > diff --git a/tools/dtoc/fdt.py b/tools/dtoc/fdt.py
> > index 32a7aa9829..e7197f8f12 100644
> > --- a/tools/dtoc/fdt.py
> > +++ b/tools/dtoc/fdt.py
> > @@ -501,6 +501,24 @@ class Node:
> >          val = bytes(val, 'utf-8')
> >          return self.AddData(prop_name, val + b'\0')
> >
> > +    def AddStringList(self, prop_name, val):
> > +        """Add a new string-list property to a node
> > +
> > +        The device tree is marked dirty so that the value will be written to
> > +        the blob on the next sync.
> > +
> > +        Args:
> > +            prop_name: Name of property to add
> > +            val (list of str): List of strings to add
> > +
> > +        Returns:
> > +            Prop added
> > +        """
> > +        out = b''
> > +        for string in val:
> > +            out += bytes(string, 'utf-8') + b'\0'
> > +        return self.AddData(prop_name, out)
>
> If val is an empty list this would try to set a zero-byte data, but then
> the Prop class considers that a boolean True.

Yes, but I believe that is correct, isn't it? I did propose a change
to DT bool props but it has not got anywhere.

>
> Perhaps use "s" as the variable name as there's a "string" module
> (though not imported here).
>
> Also, b'\0'.join() works just like in strings if you prefer that to a loop.

OK, will update both of those.

Regards,
Simon

Re: [PATCH 06/24] dtoc: Support adding a string list to a device tree

[Alper Nebi Yasak]

On 24/02/2022 01:58, Simon Glass wrote:
> On Tue, 15 Feb 2022 at 04:53, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>> On 08/02/2022 21:49, Simon Glass wrote:
>>> diff --git a/tools/dtoc/fdt.py b/tools/dtoc/fdt.py
>>> index 32a7aa9829..e7197f8f12 100644
>>> --- a/tools/dtoc/fdt.py
>>> +++ b/tools/dtoc/fdt.py
>>> @@ -501,6 +501,24 @@ class Node:
>>>          val = bytes(val, 'utf-8')
>>>          return self.AddData(prop_name, val + b'\0')
>>>
>>> +    def AddStringList(self, prop_name, val):
>>> +        """Add a new string-list property to a node
>>> +
>>> +        The device tree is marked dirty so that the value will be written to
>>> +        the blob on the next sync.
>>> +
>>> +        Args:
>>> +            prop_name: Name of property to add
>>> +            val (list of str): List of strings to add
>>> +
>>> +        Returns:
>>> +            Prop added
>>> +        """
>>> +        out = b''
>>> +        for string in val:
>>> +            out += bytes(string, 'utf-8') + b'\0'
>>> +        return self.AddData(prop_name, out)
>>
>> If val is an empty list this would try to set a zero-byte data, but then
>> the Prop class considers that a boolean True.
> 
> Yes, but I believe that is correct, isn't it? I did propose a change
> to DT bool props but it has not got anywhere.

I just wasn't sure about the semantics and wanted to highlight what
might be an edge case. Looking again, I think this is binary-wise
correct, but GetString and GetStringList would return True and [True]
for these boolean props where I'd want None and [].

Re: [PATCH 06/24] dtoc: Support adding a string list to a device tree

[Simon Glass]

Hi Alper,

On Thu, 3 Mar 2022 at 14:14, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> On 24/02/2022 01:58, Simon Glass wrote:
> > On Tue, 15 Feb 2022 at 04:53, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
> >> On 08/02/2022 21:49, Simon Glass wrote:
> >>> diff --git a/tools/dtoc/fdt.py b/tools/dtoc/fdt.py
> >>> index 32a7aa9829..e7197f8f12 100644
> >>> --- a/tools/dtoc/fdt.py
> >>> +++ b/tools/dtoc/fdt.py
> >>> @@ -501,6 +501,24 @@ class Node:
> >>>          val = bytes(val, 'utf-8')
> >>>          return self.AddData(prop_name, val + b'\0')
> >>>
> >>> +    def AddStringList(self, prop_name, val):
> >>> +        """Add a new string-list property to a node
> >>> +
> >>> +        The device tree is marked dirty so that the value will be written to
> >>> +        the blob on the next sync.
> >>> +
> >>> +        Args:
> >>> +            prop_name: Name of property to add
> >>> +            val (list of str): List of strings to add
> >>> +
> >>> +        Returns:
> >>> +            Prop added
> >>> +        """
> >>> +        out = b''
> >>> +        for string in val:
> >>> +            out += bytes(string, 'utf-8') + b'\0'
> >>> +        return self.AddData(prop_name, out)
> >>
> >> If val is an empty list this would try to set a zero-byte data, but then
> >> the Prop class considers that a boolean True.
> >
> > Yes, but I believe that is correct, isn't it? I did propose a change
> > to DT bool props but it has not got anywhere.
>
> I just wasn't sure about the semantics and wanted to highlight what
> might be an edge case. Looking again, I think this is binary-wise
> correct, but GetString and GetStringList would return True and [True]
> for these boolean props where I'd want None and [].

I'll add a test for that in v3.

Regards,
Simon

Re: [PATCH 06/24] dtoc: Support adding a string list to a device tree

[Simon Glass]

On 08/02/2022 21:49, Simon Glass wrote:
> Add a new function to add a string list.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  tools/dtoc/fdt.py      | 18 ++++++++++++++++++
>  tools/dtoc/test_fdt.py |  8 ++++++++
>  2 files changed, 26 insertions(+)
>
Applied to u-boot-dm, thanks!

[PATCH 07/24] dtoc: Support deleting a node

[Simon Glass]

Add a function to delete a node. This is synced to the tree when
requested.

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

 tools/dtoc/fdt.py      | 17 +++++++++++++++++
 tools/dtoc/test_fdt.py |  9 +++++++++
 2 files changed, 26 insertions(+)

diff --git a/tools/dtoc/fdt.py b/tools/dtoc/fdt.py
index e7197f8f12..8108a86c09 100644
--- a/tools/dtoc/fdt.py
+++ b/tools/dtoc/fdt.py
@@ -548,6 +548,23 @@ class Node:
         self.subnodes.append(subnode)
         return subnode
 
+    def Delete(self):
+        """Delete a node
+
+        The node is deleted and the offset cache is invalidated.
+
+        Args:
+            node (Node): Node to delete
+
+        Raises:
+            ValueError if the node does not exist
+        """
+        CheckErr(self._fdt._fdt_obj.del_node(self.Offset()),
+                 "Node '%s': delete" % self.path)
+        parent = self.parent
+        self._fdt.Invalidate()
+        parent.subnodes.remove(self)
+
     def Sync(self, auto_resize=False):
         """Sync node changes back to the device tree
 
diff --git a/tools/dtoc/test_fdt.py b/tools/dtoc/test_fdt.py
index 1a7e73ffce..22e23c40f5 100755
--- a/tools/dtoc/test_fdt.py
+++ b/tools/dtoc/test_fdt.py
@@ -539,6 +539,15 @@ class TestProp(unittest.TestCase):
         data = self.fdt.getprop(self.node.Offset(), 'stringlist')
         self.assertEqual(b'123\x00456\0', data)
 
+    def test_delete_node(self):
+        """Test deleting a node"""
+        old_offset = self.fdt.path_offset('/spl-test')
+        self.assertGreater(old_offset, 0)
+        self.node.Delete()
+        self.dtb.Sync()
+        new_offset = self.fdt.path_offset('/spl-test', libfdt.QUIET_NOTFOUND)
+        self.assertEqual(-libfdt.NOTFOUND, new_offset)
+
     def testFromData(self):
         dtb2 = fdt.Fdt.FromData(self.dtb.GetContents())
         self.assertEqual(dtb2.GetContents(), self.dtb.GetContents())
-- 
2.35.0.263.gb82422642f-goog

[PATCH 08/24] dtoc: Allow deleting nodes and adding them in the same sync

[Simon Glass]

This does not work at present, since the current algorithm assumes that
either there are no nodes or all nodes have an offset. If a node is new,
but an old node is still in the tree, then syncing fails due to this
assumption.

Fix it and add a test.

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

 tools/dtoc/fdt.py      |  2 ++
 tools/dtoc/test_fdt.py | 11 +++++++++++
 2 files changed, 13 insertions(+)

diff --git a/tools/dtoc/fdt.py b/tools/dtoc/fdt.py
index 8108a86c09..98d6e636ea 100644
--- a/tools/dtoc/fdt.py
+++ b/tools/dtoc/fdt.py
@@ -356,6 +356,8 @@ class Node:
 
         offset = fdt_obj.first_subnode(self._offset, QUIET_NOTFOUND)
         for subnode in self.subnodes:
+            if subnode._offset is None:
+                continue
             if subnode.name != fdt_obj.get_name(offset):
                 raise ValueError('Internal error, node name mismatch %s != %s' %
                                  (subnode.name, fdt_obj.get_name(offset)))
diff --git a/tools/dtoc/test_fdt.py b/tools/dtoc/test_fdt.py
index 22e23c40f5..c8fe5fc1de 100755
--- a/tools/dtoc/test_fdt.py
+++ b/tools/dtoc/test_fdt.py
@@ -272,6 +272,17 @@ class TestNode(unittest.TestCase):
 
         self.dtb.Sync(auto_resize=True)
 
+    def testAddOneNode(self):
+        """Testing deleting and adding a subnode before syncing"""
+        subnode = self.node.AddSubnode('subnode')
+        self.node.AddSubnode('subnode2')
+        self.dtb.Sync(auto_resize=True)
+
+        # Delete a node and add a new one
+        subnode.Delete()
+        self.node.AddSubnode('subnode3')
+        self.dtb.Sync()
+
     def testRefreshNameMismatch(self):
         """Test name mismatch when syncing nodes and properties"""
         prop = self.node.AddInt('integer-a', 12)
-- 
2.35.0.263.gb82422642f-goog

[PATCH 09/24] dtoc: Support reading a list of arguments

[Simon Glass]

It is helpful to support a string or stringlist containing a list of
space-separated arguments, for example:

   args = "-n fred", "-a", "123";

This resolves to the list:

   -n fred -a 123

which can be passed to a program as arguments.

Add a helper to do the required processing.

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

 tools/dtoc/fdt_util.py               | 12 ++++++++++++
 tools/dtoc/test/dtoc_test_simple.dts |  1 +
 tools/dtoc/test_fdt.py               | 15 +++++++++++++++
 3 files changed, 28 insertions(+)

diff --git a/tools/dtoc/fdt_util.py b/tools/dtoc/fdt_util.py
index 19eb13aef3..59e065884f 100644
--- a/tools/dtoc/fdt_util.py
+++ b/tools/dtoc/fdt_util.py
@@ -184,6 +184,18 @@ def GetStringList(node, propname, default=None):
         return [strval]
     return value
 
+def GetArgs(node, propname):
+    prop = node.props.get(propname)
+    if not prop:
+        raise ValueError(f"Node '{node.path}': Expected property '{propname}'")
+    if prop.bytes:
+        value = GetStringList(node, propname)
+    else:
+        value = []
+    lists = [v.split() for v in value]
+    args = [x for l in lists for x in l]
+    return args
+
 def GetBool(node, propname, default=False):
     """Get an boolean from a property
 
diff --git a/tools/dtoc/test/dtoc_test_simple.dts b/tools/dtoc/test/dtoc_test_simple.dts
index 4c2c70af22..2d321fb034 100644
--- a/tools/dtoc/test/dtoc_test_simple.dts
+++ b/tools/dtoc/test/dtoc_test_simple.dts
@@ -62,5 +62,6 @@
 
 	orig-node {
 		orig = <1 23 4>;
+		args = "-n first", "second", "-p", "123,456", "-x";
 	};
 };
diff --git a/tools/dtoc/test_fdt.py b/tools/dtoc/test_fdt.py
index c8fe5fc1de..5d46e69b8b 100755
--- a/tools/dtoc/test_fdt.py
+++ b/tools/dtoc/test_fdt.py
@@ -652,6 +652,21 @@ class TestFdtUtil(unittest.TestCase):
         self.assertEqual(['test'],
                          fdt_util.GetStringList(self.node, 'missing', ['test']))
 
+    def testGetArgs(self):
+        node = self.dtb.GetNode('/orig-node')
+        self.assertEqual(['message'], fdt_util.GetArgs(self.node, 'stringval'))
+        self.assertEqual(
+            ['multi-word', 'message'],
+            fdt_util.GetArgs(self.node, 'stringarray'))
+        self.assertEqual([], fdt_util.GetArgs(self.node, 'boolval'))
+        self.assertEqual(['-n', 'first', 'second', '-p', '123,456', '-x'],
+                         fdt_util.GetArgs(node, 'args'))
+        with self.assertRaises(ValueError) as exc:
+            fdt_util.GetArgs(self.node, 'missing')
+        self.assertIn(
+            "Node '/spl-test': Expected property 'missing'",
+            str(exc.exception))
+
     def testGetBool(self):
         self.assertEqual(True, fdt_util.GetBool(self.node, 'boolval'))
         self.assertEqual(False, fdt_util.GetBool(self.node, 'missing'))
-- 
2.35.0.263.gb82422642f-goog

Re: [PATCH 09/24] dtoc: Support reading a list of arguments

[Alper Nebi Yasak]

On 08/02/2022 21:49, Simon Glass wrote:
> It is helpful to support a string or stringlist containing a list of
> space-separated arguments, for example:
> 
>    args = "-n fred", "-a", "123";
> 
> This resolves to the list:
> 
>    -n fred -a 123

Would be clearer as ['-n', 'fred', '-a', '123']

> 
> which can be passed to a program as arguments.
> 
> Add a helper to do the required processing.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  tools/dtoc/fdt_util.py               | 12 ++++++++++++
>  tools/dtoc/test/dtoc_test_simple.dts |  1 +
>  tools/dtoc/test_fdt.py               | 15 +++++++++++++++
>  3 files changed, 28 insertions(+)
> 
> diff --git a/tools/dtoc/fdt_util.py b/tools/dtoc/fdt_util.py
> index 19eb13aef3..59e065884f 100644
> --- a/tools/dtoc/fdt_util.py
> +++ b/tools/dtoc/fdt_util.py
> @@ -184,6 +184,18 @@ def GetStringList(node, propname, default=None):
>          return [strval]
>      return value
>  
> +def GetArgs(node, propname):
> +    prop = node.props.get(propname)
> +    if not prop:
> +        raise ValueError(f"Node '{node.path}': Expected property '{propname}'")
> +    if prop.bytes:
> +        value = GetStringList(node, propname)
> +    else:
> +        value = []

Isn't GetStringList(node, propname, default=[]) enough here, why check
prop.bytes?

> +    lists = [v.split() for v in value]

Use shlex.split() to handle quotes inside the strings, so that we can
pass args with spaces inside them. e.g. mkimage -n "U-Boot v2022.04".
Or each list element could be a single argument with no splitting done.

I also wish mkimage -n "U-Boot $(UBOOTRELEASE) for $(BOARD) board" from
Makefile was possible, but can't think of a great way.

> +    args = [x for l in lists for x in l]
> +    return args
> +

Anyway, I don't think this belongs here as argument lists are not really
a device-tree construct. It would be better in a new binman entry type
("command"?) which mkimage can subclass from.

>  def GetBool(node, propname, default=False):
>      """Get an boolean from a property
>  
> diff --git a/tools/dtoc/test/dtoc_test_simple.dts b/tools/dtoc/test/dtoc_test_simple.dts
> index 4c2c70af22..2d321fb034 100644
> --- a/tools/dtoc/test/dtoc_test_simple.dts
> +++ b/tools/dtoc/test/dtoc_test_simple.dts
> @@ -62,5 +62,6 @@
>  
>  	orig-node {
>  		orig = <1 23 4>;
> +		args = "-n first", "second", "-p", "123,456", "-x";

Could be useful to add an argument with single quotes, and one with
escaped double quotes.

>  	};
>  };
> diff --git a/tools/dtoc/test_fdt.py b/tools/dtoc/test_fdt.py
> index c8fe5fc1de..5d46e69b8b 100755
> --- a/tools/dtoc/test_fdt.py
> +++ b/tools/dtoc/test_fdt.py
> @@ -652,6 +652,21 @@ class TestFdtUtil(unittest.TestCase):
>          self.assertEqual(['test'],
>                           fdt_util.GetStringList(self.node, 'missing', ['test']))
>  
> +    def testGetArgs(self):
> +        node = self.dtb.GetNode('/orig-node')
> +        self.assertEqual(['message'], fdt_util.GetArgs(self.node, 'stringval'))
> +        self.assertEqual(
> +            ['multi-word', 'message'],
> +            fdt_util.GetArgs(self.node, 'stringarray'))
> +        self.assertEqual([], fdt_util.GetArgs(self.node, 'boolval'))
> +        self.assertEqual(['-n', 'first', 'second', '-p', '123,456', '-x'],
> +                         fdt_util.GetArgs(node, 'args'))
> +        with self.assertRaises(ValueError) as exc:
> +            fdt_util.GetArgs(self.node, 'missing')
> +        self.assertIn(
> +            "Node '/spl-test': Expected property 'missing'",
> +            str(exc.exception))
> +
>      def testGetBool(self):
>          self.assertEqual(True, fdt_util.GetBool(self.node, 'boolval'))
>          self.assertEqual(False, fdt_util.GetBool(self.node, 'missing'))

Re: [PATCH 09/24] dtoc: Support reading a list of arguments

[Simon Glass]

Hi Alper,

On Tue, 15 Feb 2022 at 04:53, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> On 08/02/2022 21:49, Simon Glass wrote:
> > It is helpful to support a string or stringlist containing a list of
> > space-separated arguments, for example:
> >
> >    args = "-n fred", "-a", "123";
> >
> > This resolves to the list:
> >
> >    -n fred -a 123
>
> Would be clearer as ['-n', 'fred', '-a', '123']

OK

>
> >
> > which can be passed to a program as arguments.
> >
> > Add a helper to do the required processing.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  tools/dtoc/fdt_util.py               | 12 ++++++++++++
> >  tools/dtoc/test/dtoc_test_simple.dts |  1 +
> >  tools/dtoc/test_fdt.py               | 15 +++++++++++++++
> >  3 files changed, 28 insertions(+)
> >
> > diff --git a/tools/dtoc/fdt_util.py b/tools/dtoc/fdt_util.py
> > index 19eb13aef3..59e065884f 100644
> > --- a/tools/dtoc/fdt_util.py
> > +++ b/tools/dtoc/fdt_util.py
> > @@ -184,6 +184,18 @@ def GetStringList(node, propname, default=None):
> >          return [strval]
> >      return value
> >
> > +def GetArgs(node, propname):
> > +    prop = node.props.get(propname)
> > +    if not prop:
> > +        raise ValueError(f"Node '{node.path}': Expected property '{propname}'")
> > +    if prop.bytes:
> > +        value = GetStringList(node, propname)
> > +    else:
> > +        value = []
>
> Isn't GetStringList(node, propname, default=[]) enough here, why check
> prop.bytes?

Because the default value is for when there is no such property. In
this case there is a property, so the default will not be used.

>
> > +    lists = [v.split() for v in value]
>
> Use shlex.split() to handle quotes inside the strings, so that we can
> pass args with spaces inside them. e.g. mkimage -n "U-Boot v2022.04".
> Or each list element could be a single argument with no splitting done.
>
> I also wish mkimage -n "U-Boot $(UBOOTRELEASE) for $(BOARD) board" from
> Makefile was possible, but can't think of a great way.

That's actually not what I was trying to do. I want "-n first" to mean
two args. It is like normally command-line processing.

Of course this means that it isn't possible to do what you want here.
I am not sure of a good way to support that.

One way would be to only split into args if there is a single string
in the list? I will try that for now.

>
> > +    args = [x for l in lists for x in l]
> > +    return args
> > +
>
> Anyway, I don't think this belongs here as argument lists are not really
> a device-tree construct. It would be better in a new binman entry type
> ("command"?) which mkimage can subclass from.

Well I am trying to keep all typing stuff in here, i.e. everything
that guesses what is meant by a DT property. That way I can have DT
tests and expand as needed. I agree this is a borderline case, but I'm
not sure it is a good to have lots of logic (that depends on the
internal working of Fdt) in a different file. E.g. I hate all of this
code and would like to refactor it to put more stuff in pylibfdt one
day.

>
> >  def GetBool(node, propname, default=False):
> >      """Get an boolean from a property
> >
> > diff --git a/tools/dtoc/test/dtoc_test_simple.dts b/tools/dtoc/test/dtoc_test_simple.dts
> > index 4c2c70af22..2d321fb034 100644
> > --- a/tools/dtoc/test/dtoc_test_simple.dts
> > +++ b/tools/dtoc/test/dtoc_test_simple.dts
> > @@ -62,5 +62,6 @@
> >
> >       orig-node {
> >               orig = <1 23 4>;
> > +             args = "-n first", "second", "-p", "123,456", "-x";
>
> Could be useful to add an argument with single quotes, and one with
> escaped double quotes.

I reverted the shlex change in the end...can we do that in a separate
patch? I think it has interesting implications for the Makefile and we
should think about what tests to add and what the use cases are.

>
> >       };
> >  };
> > diff --git a/tools/dtoc/test_fdt.py b/tools/dtoc/test_fdt.py
> > index c8fe5fc1de..5d46e69b8b 100755
> > --- a/tools/dtoc/test_fdt.py
> > +++ b/tools/dtoc/test_fdt.py
> > @@ -652,6 +652,21 @@ class TestFdtUtil(unittest.TestCase):
> >          self.assertEqual(['test'],
> >                           fdt_util.GetStringList(self.node, 'missing', ['test']))
> >
> > +    def testGetArgs(self):
> > +        node = self.dtb.GetNode('/orig-node')
> > +        self.assertEqual(['message'], fdt_util.GetArgs(self.node, 'stringval'))
> > +        self.assertEqual(
> > +            ['multi-word', 'message'],
> > +            fdt_util.GetArgs(self.node, 'stringarray'))
> > +        self.assertEqual([], fdt_util.GetArgs(self.node, 'boolval'))
> > +        self.assertEqual(['-n', 'first', 'second', '-p', '123,456', '-x'],
> > +                         fdt_util.GetArgs(node, 'args'))
> > +        with self.assertRaises(ValueError) as exc:
> > +            fdt_util.GetArgs(self.node, 'missing')
> > +        self.assertIn(
> > +            "Node '/spl-test': Expected property 'missing'",
> > +            str(exc.exception))
> > +
> >      def testGetBool(self):
> >          self.assertEqual(True, fdt_util.GetBool(self.node, 'boolval'))
> >          self.assertEqual(False, fdt_util.GetBool(self.node, 'missing'))

Regards,
Simon

Re: [PATCH 09/24] dtoc: Support reading a list of arguments

[Alper Nebi Yasak]

On 24/02/2022 01:58, Simon Glass wrote:
> On Tue, 15 Feb 2022 at 04:53, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>>> diff --git a/tools/dtoc/fdt_util.py b/tools/dtoc/fdt_util.py
>>> index 19eb13aef3..59e065884f 100644
>>> --- a/tools/dtoc/fdt_util.py
>>> +++ b/tools/dtoc/fdt_util.py
>>> @@ -184,6 +184,18 @@ def GetStringList(node, propname, default=None):
>>>          return [strval]
>>>      return value
>>>
>>> +def GetArgs(node, propname):
>>> +    prop = node.props.get(propname)
>>> +    if not prop:
>>> +        raise ValueError(f"Node '{node.path}': Expected property '{propname}'")
>>> +    if prop.bytes:
>>> +        value = GetStringList(node, propname)
>>> +    else:
>>> +        value = []
>>
>> Isn't GetStringList(node, propname, default=[]) enough here, why check
>> prop.bytes?
> 
> Because the default value is for when there is no such property. In
> this case there is a property, so the default will not be used.

Ah, it's the same as dict.get(). Don't know why I got confused there.

>>> +    lists = [v.split() for v in value]
>>
>> Use shlex.split() to handle quotes inside the strings, so that we can
>> pass args with spaces inside them. e.g. mkimage -n "U-Boot v2022.04".
>> Or each list element could be a single argument with no splitting done.
>>
>> I also wish mkimage -n "U-Boot $(UBOOTRELEASE) for $(BOARD) board" from
>> Makefile was possible, but can't think of a great way.
> 
> That's actually not what I was trying to do. I want "-n first" to mean
> two args. It is like normally command-line processing.
> 
> Of course this means that it isn't possible to do what you want here.
> I am not sure of a good way to support that.
> 
> One way would be to only split into args if there is a single string
> in the list? I will try that for now.

That sounds like a reasonable middle ground if you really want to do
splitting.

>>> +    args = [x for l in lists for x in l]
>>> +    return args
>>> +
>>
>> Anyway, I don't think this belongs here as argument lists are not really
>> a device-tree construct. It would be better in a new binman entry type
>> ("command"?) which mkimage can subclass from.
> 
> Well I am trying to keep all typing stuff in here, i.e. everything
> that guesses what is meant by a DT property. That way I can have DT
> tests and expand as needed. I agree this is a borderline case, but I'm
> not sure it is a good to have lots of logic (that depends on the
> internal working of Fdt) in a different file. E.g. I hate all of this
> code and would like to refactor it to put more stuff in pylibfdt one
> day.

That's fair, and I like the idea of refactoring things into pylibfdt.

>>
>>>  def GetBool(node, propname, default=False):
>>>      """Get an boolean from a property
>>>
>>> diff --git a/tools/dtoc/test/dtoc_test_simple.dts b/tools/dtoc/test/dtoc_test_simple.dts
>>> index 4c2c70af22..2d321fb034 100644
>>> --- a/tools/dtoc/test/dtoc_test_simple.dts
>>> +++ b/tools/dtoc/test/dtoc_test_simple.dts
>>> @@ -62,5 +62,6 @@
>>>
>>>       orig-node {
>>>               orig = <1 23 4>;
>>> +             args = "-n first", "second", "-p", "123,456", "-x";
>>
>> Could be useful to add an argument with single quotes, and one with
>> escaped double quotes.
> 
> I reverted the shlex change in the end...can we do that in a separate
> patch? I think it has interesting implications for the Makefile and we
> should think about what tests to add and what the use cases are.

OK, but I'll need to focus on other things and might forget to ping
later for this...

Re: [PATCH 09/24] dtoc: Support reading a list of arguments

[Simon Glass]

On 08/02/2022 21:49, Simon Glass wrote:
> It is helpful to support a string or stringlist containing a list of
> space-separated arguments, for example:
>
>    args = "-n fred", "-a", "123";
>
> This resolves to the list:
>
>    -n fred -a 123

Would be clearer as ['-n', 'fred', '-a', '123']

>
> which can be passed to a program as arguments.
>
> Add a helper to do the required processing.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  tools/dtoc/fdt_util.py               | 12 ++++++++++++
>  tools/dtoc/test/dtoc_test_simple.dts |  1 +
>  tools/dtoc/test_fdt.py               | 15 +++++++++++++++
>  3 files changed, 28 insertions(+)
>
Applied to u-boot-dm, thanks!

[PATCH 10/24] binman: Update docs to indicate mkimage is supported

[Simon Glass]

Now that there is a mkimage entry-type, update the docs to remove the
future reference.

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

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

diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst
index ab5a5e06b1..9e39e678a3 100644
--- a/tools/binman/binman.rst
+++ b/tools/binman/binman.rst
@@ -163,8 +163,8 @@ Consider sunxi. It has the following steps:
 
 Binman is intended to replace the last step. The U-Boot build system builds
 u-boot.bin and sunxi-spl.bin. Binman can then take over creation of
-sunxi-spl.bin (by calling mksunxiboot, or hopefully one day mkimage). In any
-case, it would then create the image from the component parts.
+sunxi-spl.bin by calling mksunxiboot or mkimage. In any case, it would then
+create the image from the component parts.
 
 This simplifies the U-Boot Makefile somewhat, since various pieces of logic
 can be replaced by a call to binman.
-- 
2.35.0.263.gb82422642f-goog

[PATCH 11/24] elf: Add a way to read segment information from an ELF file

[Simon Glass]

Add a function which reads the segments and the entry address.

Also fix a comment nit in the tests while we are here.

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

 tools/binman/elf.py      | 37 +++++++++++++++++++++++++++++++++++++
 tools/binman/elf_test.py | 31 +++++++++++++++++++++++++++++--
 2 files changed, 66 insertions(+), 2 deletions(-)

diff --git a/tools/binman/elf.py b/tools/binman/elf.py
index de2bb4651f..2b83ac1876 100644
--- a/tools/binman/elf.py
+++ b/tools/binman/elf.py
@@ -20,6 +20,7 @@ from patman import tout
 ELF_TOOLS = True
 try:
     from elftools.elf.elffile import ELFFile
+    from elftools.elf.elffile import ELFError
     from elftools.elf.sections import SymbolTableSection
 except:  # pragma: no cover
     ELF_TOOLS = False
@@ -369,3 +370,39 @@ def UpdateFile(infile, outfile, start_sym, end_sym, insert):
     newdata += data[syms[end_sym].offset:]
     tools.WriteFile(outfile, newdata)
     tout.Info('Written to offset %#x' % syms[start_sym].offset)
+
+def read_segments(data):
+    """Read segments from an ELF file
+
+    Args:
+        data (bytes): Contents of file
+
+    Returns:
+        tuple:
+            list of segments, each:
+                int: Segment number (0 = first)
+                int: Start address of segment in memory
+                bytes: Contents of segment
+            int: entry address for image
+
+    Raises:
+        ValueError: elftools is not available
+    """
+    if not ELF_TOOLS:
+        raise ValueError('Python elftools package is not available')
+    with io.BytesIO(data) as inf:
+        try:
+            elf = ELFFile(inf)
+        except ELFError as err:
+            raise ValueError(err)
+        entry = elf.header['e_entry']
+        segments = []
+        for i in range(elf.num_segments()):
+            segment = elf.get_segment(i)
+            if segment['p_type'] != 'PT_LOAD' or not segment['p_memsz']:
+                skipped = 1  # To make code-coverage see this line
+                continue
+            start = segment['p_offset']
+            rend = start + segment['p_filesz']
+            segments.append((i, segment['p_paddr'], data[start:rend]))
+    return segments, entry
diff --git a/tools/binman/elf_test.py b/tools/binman/elf_test.py
index f727258487..369260c17a 100644
--- a/tools/binman/elf_test.py
+++ b/tools/binman/elf_test.py
@@ -56,8 +56,8 @@ class FakeSection:
 def BuildElfTestFiles(target_dir):
     """Build ELF files used for testing in binman
 
-    This compiles and links the test files into the specified directory. It the
-    Makefile and source files in the binman test/ directory.
+    This compiles and links the test files into the specified directory. It uses
+    the Makefile and source files in the binman test/ directory.
 
     Args:
         target_dir: Directory to put the files into
@@ -258,6 +258,33 @@ class TestElf(unittest.TestCase):
         offset = elf.GetSymbolFileOffset(fname, ['missing_sym'])
         self.assertEqual({}, offset)
 
+    def test_read_segments(self):
+        """Test for read_segments()"""
+        if not elf.ELF_TOOLS:
+            self.skipTest('Python elftools not available')
+        fname = self.ElfTestFile('embed_data')
+        segments, entry = elf.read_segments(tools.ReadFile(fname))
+
+    def test_read_segments_fail(self):
+        """Test for read_segments() without elftools"""
+        try:
+            old_val = elf.ELF_TOOLS
+            elf.ELF_TOOLS = False
+            fname = self.ElfTestFile('embed_data')
+            with self.assertRaises(ValueError) as e:
+                elf.read_segments(tools.ReadFile(fname))
+            self.assertIn('Python elftools package is not available',
+                          str(e.exception))
+        finally:
+            elf.ELF_TOOLS = old_val
+
+    def test_read_segments_bad_data(self):
+        """Test for read_segments() with an invalid ELF file"""
+        fname = self.ElfTestFile('embed_data')
+        with self.assertRaises(ValueError) as e:
+            elf.read_segments(tools.GetBytes(100, 100))
+        self.assertIn('Magic number does not match', str(e.exception))
+
 
 if __name__ == '__main__':
     unittest.main()
-- 
2.35.0.263.gb82422642f-goog

Re: [PATCH 11/24] elf: Add a way to read segment information from an ELF file

[Alper Nebi Yasak]

On 08/02/2022 21:49, Simon Glass wrote:
> Add a function which reads the segments and the entry address.
> 
> Also fix a comment nit in the tests while we are here.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  tools/binman/elf.py      | 37 +++++++++++++++++++++++++++++++++++++
>  tools/binman/elf_test.py | 31 +++++++++++++++++++++++++++++--
>  2 files changed, 66 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/binman/elf.py b/tools/binman/elf.py
> index de2bb4651f..2b83ac1876 100644
> --- a/tools/binman/elf.py
> +++ b/tools/binman/elf.py
> @@ -20,6 +20,7 @@ from patman import tout
>  ELF_TOOLS = True
>  try:
>      from elftools.elf.elffile import ELFFile
> +    from elftools.elf.elffile import ELFError
>      from elftools.elf.sections import SymbolTableSection
>  except:  # pragma: no cover
>      ELF_TOOLS = False
> @@ -369,3 +370,39 @@ def UpdateFile(infile, outfile, start_sym, end_sym, insert):
>      newdata += data[syms[end_sym].offset:]
>      tools.WriteFile(outfile, newdata)
>      tout.Info('Written to offset %#x' % syms[start_sym].offset)
> +
> +def read_segments(data):
> +    """Read segments from an ELF file
> +
> +    Args:
> +        data (bytes): Contents of file
> +
> +    Returns:
> +        tuple:
> +            list of segments, each:
> +                int: Segment number (0 = first)
> +                int: Start address of segment in memory
> +                bytes: Contents of segment
> +            int: entry address for image
> +
> +    Raises:
> +        ValueError: elftools is not available

... or input data is not a correct ELF file?

> +    """
> +    if not ELF_TOOLS:
> +        raise ValueError('Python elftools package is not available')

I see something like ModuleNotFoundError("No module named 'elftools'")
when I try to import an unavailable module, so maybe this could match that.

> +    with io.BytesIO(data) as inf:
> +        try:
> +            elf = ELFFile(inf)
> +        except ELFError as err:
> +            raise ValueError(err)

Could also be: raise ValueError("Not an ELF file") from err

But I guess you want err's message here to match on it in tests. (It's
also possible but slightly inconvenient with __cause__ when using
raise-from)

> +        entry = elf.header['e_entry']
> +        segments = []
> +        for i in range(elf.num_segments()):
> +            segment = elf.get_segment(i)
> +            if segment['p_type'] != 'PT_LOAD' or not segment['p_memsz']:

I can't say I fully understand ELF details, is it obvious in context
that a function named read_segments() would only return these segments,
or should the name be explicit about it e.g. read_loadable_segments()?

> +                skipped = 1  # To make code-coverage see this line
> +                continue
> +            start = segment['p_offset']
> +            rend = start + segment['p_filesz']
> +            segments.append((i, segment['p_paddr'], data[start:rend]))
> +    return segments, entry
> diff --git a/tools/binman/elf_test.py b/tools/binman/elf_test.py
> index f727258487..369260c17a 100644
> --- a/tools/binman/elf_test.py
> +++ b/tools/binman/elf_test.py
> @@ -56,8 +56,8 @@ class FakeSection:
>  def BuildElfTestFiles(target_dir):
>      """Build ELF files used for testing in binman
>  
> -    This compiles and links the test files into the specified directory. It the
> -    Makefile and source files in the binman test/ directory.
> +    This compiles and links the test files into the specified directory. It uses
> +    the Makefile and source files in the binman test/ directory.
>  
>      Args:
>          target_dir: Directory to put the files into
> @@ -258,6 +258,33 @@ class TestElf(unittest.TestCase):
>          offset = elf.GetSymbolFileOffset(fname, ['missing_sym'])
>          self.assertEqual({}, offset)
>  
> +    def test_read_segments(self):
> +        """Test for read_segments()"""
> +        if not elf.ELF_TOOLS:
> +            self.skipTest('Python elftools not available')
> +        fname = self.ElfTestFile('embed_data')
> +        segments, entry = elf.read_segments(tools.ReadFile(fname))
> +
> +    def test_read_segments_fail(self):
> +        """Test for read_segments() without elftools"""
> +        try:
> +            old_val = elf.ELF_TOOLS
> +            elf.ELF_TOOLS = False
> +            fname = self.ElfTestFile('embed_data')
> +            with self.assertRaises(ValueError) as e:
> +                elf.read_segments(tools.ReadFile(fname))
> +            self.assertIn('Python elftools package is not available',
> +                          str(e.exception))
> +        finally:
> +            elf.ELF_TOOLS = old_val
> +
> +    def test_read_segments_bad_data(self):
> +        """Test for read_segments() with an invalid ELF file"""
> +        fname = self.ElfTestFile('embed_data')
> +        with self.assertRaises(ValueError) as e:
> +            elf.read_segments(tools.GetBytes(100, 100))
> +        self.assertIn('Magic number does not match', str(e.exception))
> +
>  
>  if __name__ == '__main__':
>      unittest.main()

Re: [PATCH 11/24] elf: Add a way to read segment information from an ELF file

[Simon Glass]

Hi Alper,

On Tue, 15 Feb 2022 at 04:53, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> On 08/02/2022 21:49, Simon Glass wrote:
> > Add a function which reads the segments and the entry address.
> >
> > Also fix a comment nit in the tests while we are here.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  tools/binman/elf.py      | 37 +++++++++++++++++++++++++++++++++++++
> >  tools/binman/elf_test.py | 31 +++++++++++++++++++++++++++++--
> >  2 files changed, 66 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/binman/elf.py b/tools/binman/elf.py
> > index de2bb4651f..2b83ac1876 100644
> > --- a/tools/binman/elf.py
> > +++ b/tools/binman/elf.py
> > @@ -20,6 +20,7 @@ from patman import tout
> >  ELF_TOOLS = True
> >  try:
> >      from elftools.elf.elffile import ELFFile
> > +    from elftools.elf.elffile import ELFError
> >      from elftools.elf.sections import SymbolTableSection
> >  except:  # pragma: no cover
> >      ELF_TOOLS = False
> > @@ -369,3 +370,39 @@ def UpdateFile(infile, outfile, start_sym, end_sym, insert):
> >      newdata += data[syms[end_sym].offset:]
> >      tools.WriteFile(outfile, newdata)
> >      tout.Info('Written to offset %#x' % syms[start_sym].offset)
> > +
> > +def read_segments(data):
> > +    """Read segments from an ELF file
> > +
> > +    Args:
> > +        data (bytes): Contents of file
> > +
> > +    Returns:
> > +        tuple:
> > +            list of segments, each:
> > +                int: Segment number (0 = first)
> > +                int: Start address of segment in memory
> > +                bytes: Contents of segment
> > +            int: entry address for image
> > +
> > +    Raises:
> > +        ValueError: elftools is not available
>
> ... or input data is not a correct ELF file?
>
> > +    """
> > +    if not ELF_TOOLS:
> > +        raise ValueError('Python elftools package is not available')
>
> I see something like ModuleNotFoundError("No module named 'elftools'")
> when I try to import an unavailable module, so maybe this could match that.

OK, will update.

>
> > +    with io.BytesIO(data) as inf:
> > +        try:
> > +            elf = ELFFile(inf)
> > +        except ELFError as err:
> > +            raise ValueError(err)
>
> Could also be: raise ValueError("Not an ELF file") from err
>
> But I guess you want err's message here to match on it in tests. (It's
> also possible but slightly inconvenient with __cause__ when using
> raise-from)

It might be a corrupt ELF file, perhaps.


>
> > +        entry = elf.header['e_entry']
> > +        segments = []
> > +        for i in range(elf.num_segments()):
> > +            segment = elf.get_segment(i)
> > +            if segment['p_type'] != 'PT_LOAD' or not segment['p_memsz']:
>
> I can't say I fully understand ELF details, is it obvious in context
> that a function named read_segments() would only return these segments,
> or should the name be explicit about it e.g. read_loadable_segments()?

Yes I agree.

[..]

Regards,
Simon

Re: [PATCH 11/24] elf: Add a way to read segment information from an ELF file

[Simon Glass]

On 08/02/2022 21:49, Simon Glass wrote:
> Add a function which reads the segments and the entry address.
>
> Also fix a comment nit in the tests while we are here.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  tools/binman/elf.py      | 37 +++++++++++++++++++++++++++++++++++++
>  tools/binman/elf_test.py | 31 +++++++++++++++++++++++++++++--
>  2 files changed, 66 insertions(+), 2 deletions(-)
>
Applied to u-boot-dm, thanks!

[PATCH 12/24] WIP: binman: Add support for OP-TEE

[Simon Glass]

Add support for an Open Portable Trusted Execution Environment (OP-TEE)
blob which is used with ARM Trusted Firmware.

Note: This is a placeholder patch as Roger Quadros has already posted
one:

https://patchwork.ozlabs.org/project/uboot/patch/20220204130031.24610-1-rogerq@kernel.org/

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

 Makefile                       |  1 +
 tools/binman/entries.rst       | 13 +++++++++++++
 tools/binman/etype/op_tee.py   | 23 +++++++++++++++++++++++
 tools/binman/missing-blob-help |  4 ++++
 4 files changed, 41 insertions(+)
 create mode 100644 tools/binman/etype/op_tee.py

diff --git a/Makefile b/Makefile
index a69cc088a7..ee1dc11e2f 100644
--- a/Makefile
+++ b/Makefile
@@ -1326,6 +1326,7 @@ cmd_binman = $(srctree)/tools/binman/binman $(if $(BINMAN_DEBUG),-D) \
 		-I arch/$(ARCH)/dts -a of-list=$(CONFIG_OF_LIST) \
 		$(foreach f,$(BINMAN_INDIRS),-I $(f)) \
 		-a atf-bl31-path=${BL31} \
+		-a op-tee-path=${TEE} \
 		-a opensbi-path=${OPENSBI} \
 		-a default-dt=$(default_dt) \
 		-a scp-path=$(SCP) \
diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
index c47f7df098..0fb6d56296 100644
--- a/tools/binman/entries.rst
+++ b/tools/binman/entries.rst
@@ -933,6 +933,19 @@ binman.
 
 
 
+Entry: op-tee: Open Portable Trusted Execution Environment (OP-TEE) blob
+------------------------------------------------------------------------
+
+Properties / Entry arguments:
+    - op-tee-path: Filename of file to read into entry. This is typically
+        called tee.elf
+
+This entry holds the OP-TEE Elf file, typically started by U-Boot SPL.
+See the U-Boot README for your architecture or board for how to use it. See
+https://https://www.op-tee.org/ for more information about OP-TEE.
+
+
+
 Entry: opensbi: RISC-V OpenSBI fw_dynamic blob
 ----------------------------------------------
 
diff --git a/tools/binman/etype/op_tee.py b/tools/binman/etype/op_tee.py
new file mode 100644
index 0000000000..b34f561622
--- /dev/null
+++ b/tools/binman/etype/op_tee.py
@@ -0,0 +1,23 @@
+# SPDX-License-Identifier: GPL-2.0+
+# Copyright 2022 Google LLC
+# Written by Simon Glass <sjg@chromium.org>
+#
+# Entry-type module for Open Portable Trusted Execution Environment (OP-TEE)
+#
+
+from binman.etype.blob_named_by_arg import Entry_blob_named_by_arg
+
+class Entry_op_tee(Entry_blob_named_by_arg):
+    """Open Portable Trusted Execution Environment (OP-TEE) blob
+
+    Properties / Entry arguments:
+        - op-tee-path: Filename of file to read into entry. This is typically
+            called tee.elf
+
+    This entry holds the OP-TEE Elf file, typically started by U-Boot SPL.
+    See the U-Boot README for your architecture or board for how to use it. See
+    https://https://www.op-tee.org/ for more information about OP-TEE.
+    """
+    def __init__(self, section, etype, node):
+        super().__init__(section, etype, node, 'op-tee')
+        self.external = True
diff --git a/tools/binman/missing-blob-help b/tools/binman/missing-blob-help
index 551ca87f6c..c8e6b84882 100644
--- a/tools/binman/missing-blob-help
+++ b/tools/binman/missing-blob-help
@@ -33,3 +33,7 @@ k3-rti-wdt-firmware:
 If CONFIG_WDT_K3_RTI_LOAD_FW is enabled, a firmware image is needed for
 the R5F core(s) to trigger the system reset. One possible source is
 https://github.com/siemens/k3-rti-wdt.
+
+op-tee:
+See the documentation for your board. You may need to build Open Portable
+Trusted Execution Environment (OP-TEE) with TEE=/path/to/tee.elf
-- 
2.35.0.263.gb82422642f-goog

[PATCH 13/24] binman: Add to the TODO

[Simon Glass]

Add some ideas that have come to mind recently.

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

 tools/binman/binman.rst | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst
index 9e39e678a3..d6b95de1f5 100644
--- a/tools/binman/binman.rst
+++ b/tools/binman/binman.rst
@@ -1402,6 +1402,14 @@ Some ideas:
 - Detect invalid properties in nodes
 - Sort the fdtmap by offset
 - Output temporary files to a different directory
+- Rationalise the fdt, fdt_util and pylibfdt modules which currently have some
+  overlapping and confusing functionality
+- Update the fdt library to use a better format for Prop.value (the current one
+  is useful for dtoc but not much else)
+- Figure out how to make Fdt support changing the node order, so that
+  Node.AddSubnode() can support adding a node before another, existing node.
+  Perhaps it should completely regenerate the flat tree?
+
 
 --
 Simon Glass <sjg@chromium.org>
-- 
2.35.0.263.gb82422642f-goog

[PATCH 14/24] binman: Support a list of strings with the mkimage etype

[Simon Glass]

At present the 'args' property of the mkimage entry type is a string. This
makes it difficult to include CONFIG options in that property. In
particular, this does not work:

   args = "-n CONFIG_SYS_SOC -E"

since the preprocessor does not operate within strings, nor does this:

   args = "-n" CONFIG_SYS_SOC" "-E"

since the device tree compiler does not understand string concatenation.

With this new feature, we can do:

   args = "-n", CONFIG_SYS_SOC, "-E";

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

 tools/binman/entries.rst      | 11 +++++++++++
 tools/binman/etype/mkimage.py | 13 ++++++++++++-
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
index 0fb6d56296..b32dc58933 100644
--- a/tools/binman/entries.rst
+++ b/tools/binman/entries.rst
@@ -931,6 +931,17 @@ This calls mkimage to create an imximage with u-boot-spl.bin as the input
 file. The output from mkimage then becomes part of the image produced by
 binman.
 
+To use CONFIG options in the arguments, use a string list instead, as in
+this example which also produces four arguments::
+
+    mkimage {
+        args = "-n", CONFIG_SYS_SOC, "-T imximage";
+
+        u-boot-spl {
+        };
+    };
+
+
 
 
 Entry: op-tee: Open Portable Trusted Execution Environment (OP-TEE) blob
diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py
index 201ee4b569..9ecd1c2548 100644
--- a/tools/binman/etype/mkimage.py
+++ b/tools/binman/etype/mkimage.py
@@ -31,10 +31,21 @@ class Entry_mkimage(Entry):
     This calls mkimage to create an imximage with u-boot-spl.bin as the input
     file. The output from mkimage then becomes part of the image produced by
     binman.
+
+    To use CONFIG options in the arguments, use a string list instead, as in
+    this example which also produces four arguments::
+
+        mkimage {
+            args = "-n", CONFIG_SYS_SOC, "-T imximage";
+
+            u-boot-spl {
+            };
+        };
+
     """
     def __init__(self, section, etype, node):
         super().__init__(section, etype, node)
-        self._args = fdt_util.GetString(self._node, 'args').split(' ')
+        self._args = fdt_util.GetArgs(self._node, 'args')
         self._mkimage_entries = OrderedDict()
         self.align_default = None
         self.ReadEntries()
-- 
2.35.0.263.gb82422642f-goog

Re: [PATCH 14/24] binman: Support a list of strings with the mkimage etype

[Alper Nebi Yasak]

On 08/02/2022 21:49, Simon Glass wrote:
> At present the 'args' property of the mkimage entry type is a string. This
> makes it difficult to include CONFIG options in that property. In
> particular, this does not work:
> 
>    args = "-n CONFIG_SYS_SOC -E"
> 
> since the preprocessor does not operate within strings, nor does this:
> 
>    args = "-n" CONFIG_SYS_SOC" "-E"
> 
> since the device tree compiler does not understand string concatenation.

Fun fact, I got something like this roughly working:

#define _QUOTE(x) #x
#define QUOTE(x) _QUOTE(x)

#define MKIMAGE_ARGS QUOTE( \
       -A arm -T kernel -C none -O linux -n U-Boot \
       -a CONFIG_SYS_TEXT_BASE -e CONFIG_SYS_TEXT_BASE \
)

But obviously a string list is the better choice.

> 
> With this new feature, we can do:
> 
>    args = "-n", CONFIG_SYS_SOC, "-E";
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  tools/binman/entries.rst      | 11 +++++++++++
>  tools/binman/etype/mkimage.py | 13 ++++++++++++-
>  2 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
> index 0fb6d56296..b32dc58933 100644
> --- a/tools/binman/entries.rst
> +++ b/tools/binman/entries.rst
> @@ -931,6 +931,17 @@ This calls mkimage to create an imximage with u-boot-spl.bin as the input
>  file. The output from mkimage then becomes part of the image produced by
>  binman.
>  
> +To use CONFIG options in the arguments, use a string list instead, as in
> +this example which also produces four arguments::
> +
> +    mkimage {
> +        args = "-n", CONFIG_SYS_SOC, "-T imximage";
> +
> +        u-boot-spl {
> +        };
> +    };
> +
> +
>  
>  
>  Entry: op-tee: Open Portable Trusted Execution Environment (OP-TEE) blob
> diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py
> index 201ee4b569..9ecd1c2548 100644
> --- a/tools/binman/etype/mkimage.py
> +++ b/tools/binman/etype/mkimage.py
> @@ -31,10 +31,21 @@ class Entry_mkimage(Entry):
>      This calls mkimage to create an imximage with u-boot-spl.bin as the input
>      file. The output from mkimage then becomes part of the image produced by
>      binman.
> +
> +    To use CONFIG options in the arguments, use a string list instead, as in
> +    this example which also produces four arguments::
> +
> +        mkimage {
> +            args = "-n", CONFIG_SYS_SOC, "-T imximage";
> +
> +            u-boot-spl {
> +            };
> +        };
> +
>      """
>      def __init__(self, section, etype, node):
>          super().__init__(section, etype, node)
> -        self._args = fdt_util.GetString(self._node, 'args').split(' ')
> +        self._args = fdt_util.GetArgs(self._node, 'args')

If you're OK with each string being a single argument, I think you could
use GetStringList(default=[]) and shlex.join() here instead.

>          self._mkimage_entries = OrderedDict()
>          self.align_default = None
>          self.ReadEntries()

Re: [PATCH 14/24] binman: Support a list of strings with the mkimage etype

[Simon Glass]

Hi Alper,

On Tue, 15 Feb 2022 at 04:53, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> On 08/02/2022 21:49, Simon Glass wrote:
> > At present the 'args' property of the mkimage entry type is a string. This
> > makes it difficult to include CONFIG options in that property. In
> > particular, this does not work:
> >
> >    args = "-n CONFIG_SYS_SOC -E"
> >
> > since the preprocessor does not operate within strings, nor does this:
> >
> >    args = "-n" CONFIG_SYS_SOC" "-E"
> >
> > since the device tree compiler does not understand string concatenation.
>
> Fun fact, I got something like this roughly working:
>
> #define _QUOTE(x) #x
> #define QUOTE(x) _QUOTE(x)
>
> #define MKIMAGE_ARGS QUOTE( \
>        -A arm -T kernel -C none -O linux -n U-Boot \
>        -a CONFIG_SYS_TEXT_BASE -e CONFIG_SYS_TEXT_BASE \
> )
>
> But obviously a string list is the better choice.

:-)

>
> >
> > With this new feature, we can do:
> >
> >    args = "-n", CONFIG_SYS_SOC, "-E";
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  tools/binman/entries.rst      | 11 +++++++++++
> >  tools/binman/etype/mkimage.py | 13 ++++++++++++-
> >  2 files changed, 23 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
> > index 0fb6d56296..b32dc58933 100644
> > --- a/tools/binman/entries.rst
> > +++ b/tools/binman/entries.rst
> > @@ -931,6 +931,17 @@ This calls mkimage to create an imximage with u-boot-spl.bin as the input
> >  file. The output from mkimage then becomes part of the image produced by
> >  binman.
> >
> > +To use CONFIG options in the arguments, use a string list instead, as in
> > +this example which also produces four arguments::
> > +
> > +    mkimage {
> > +        args = "-n", CONFIG_SYS_SOC, "-T imximage";
> > +
> > +        u-boot-spl {
> > +        };
> > +    };
> > +
> > +
> >
> >
> >  Entry: op-tee: Open Portable Trusted Execution Environment (OP-TEE) blob
> > diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py
> > index 201ee4b569..9ecd1c2548 100644
> > --- a/tools/binman/etype/mkimage.py
> > +++ b/tools/binman/etype/mkimage.py
> > @@ -31,10 +31,21 @@ class Entry_mkimage(Entry):
> >      This calls mkimage to create an imximage with u-boot-spl.bin as the input
> >      file. The output from mkimage then becomes part of the image produced by
> >      binman.
> > +
> > +    To use CONFIG options in the arguments, use a string list instead, as in
> > +    this example which also produces four arguments::
> > +
> > +        mkimage {
> > +            args = "-n", CONFIG_SYS_SOC, "-T imximage";
> > +
> > +            u-boot-spl {
> > +            };
> > +        };
> > +
> >      """
> >      def __init__(self, section, etype, node):
> >          super().__init__(section, etype, node)
> > -        self._args = fdt_util.GetString(self._node, 'args').split(' ')
> > +        self._args = fdt_util.GetArgs(self._node, 'args')
>
> If you're OK with each string being a single argument, I think you could
> use GetStringList(default=[]) and shlex.join() here instead.

See what you think of what I have come up with here. The arg is not
optional, actually.

>
> >          self._mkimage_entries = OrderedDict()
> >          self.align_default = None
> >          self.ReadEntries()

Regards,
Simon

Re: [PATCH 14/24] binman: Support a list of strings with the mkimage etype

[Simon Glass]

On 08/02/2022 21:49, Simon Glass wrote:
> At present the 'args' property of the mkimage entry type is a string. This
> makes it difficult to include CONFIG options in that property. In
> particular, this does not work:
>
>    args = "-n CONFIG_SYS_SOC -E"
>
> since the preprocessor does not operate within strings, nor does this:
>
>    args = "-n" CONFIG_SYS_SOC" "-E"
>
> since the device tree compiler does not understand string concatenation.

Fun fact, I got something like this roughly working:

#define _QUOTE(x) #x
#define QUOTE(x) _QUOTE(x)

#define MKIMAGE_ARGS QUOTE( \
       -A arm -T kernel -C none -O linux -n U-Boot \
       -a CONFIG_SYS_TEXT_BASE -e CONFIG_SYS_TEXT_BASE \
)

But obviously a string list is the better choice.

>
> With this new feature, we can do:
>
>    args = "-n", CONFIG_SYS_SOC, "-E";
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  tools/binman/entries.rst      | 11 +++++++++++
>  tools/binman/etype/mkimage.py | 13 ++++++++++++-
>  2 files changed, 23 insertions(+), 1 deletion(-)
>
Applied to u-boot-dm, thanks!

[PATCH 15/24] binman: Add a ELF test file with disjoint text sections

[Simon Glass]

Add a file that has two text sections at different addresses, so we can
test this behaviour in binman, once added.

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

 tools/binman/test/Makefile         |  6 +++++-
 tools/binman/test/elf_sections.c   | 20 +++++++++++++++++++
 tools/binman/test/elf_sections.lds | 31 ++++++++++++++++++++++++++++++
 3 files changed, 56 insertions(+), 1 deletion(-)
 create mode 100644 tools/binman/test/elf_sections.c
 create mode 100644 tools/binman/test/elf_sections.lds

diff --git a/tools/binman/test/Makefile b/tools/binman/test/Makefile
index 387ba16335..57057e2d58 100644
--- a/tools/binman/test/Makefile
+++ b/tools/binman/test/Makefile
@@ -29,11 +29,12 @@ LDS_BINMAN := -T $(SRC)u_boot_binman_syms.lds
 LDS_BINMAN_BAD := -T $(SRC)u_boot_binman_syms_bad.lds
 LDS_BINMAN_X86 := -T $(SRC)u_boot_binman_syms_x86.lds
 LDS_BINMAN_EMBED := -T $(SRC)u_boot_binman_embed.lds
+LDS_EFL_SECTIONS := -T $(SRC)elf_sections.lds
 
 TARGETS = u_boot_ucode_ptr u_boot_no_ucode_ptr bss_data \
 	u_boot_binman_syms u_boot_binman_syms.bin u_boot_binman_syms_bad \
 	u_boot_binman_syms_size u_boot_binman_syms_x86 embed_data \
-	u_boot_binman_embed u_boot_binman_embed_sm
+	u_boot_binman_embed u_boot_binman_embed_sm elf_sections
 
 all: $(TARGETS)
 
@@ -70,6 +71,9 @@ u_boot_binman_embed: u_boot_binman_embed.c
 u_boot_binman_embed_sm: CFLAGS += $(LDS_BINMAN_EMBED)
 u_boot_binman_embed_sm: u_boot_binman_embed_sm.c
 
+elf_sections: CFLAGS += $(LDS_EFL_SECTIONS)
+elf_sections: elf_sections.c
+
 clean:
 	rm -f $(TARGETS)
 
diff --git a/tools/binman/test/elf_sections.c b/tools/binman/test/elf_sections.c
new file mode 100644
index 0000000000..9bcce9af02
--- /dev/null
+++ b/tools/binman/test/elf_sections.c
@@ -0,0 +1,20 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2022 Google LLC
+ *
+ * Program containing two text sections
+ */
+
+int __attribute__((section(".sram_data"))) data[29];
+
+int __attribute__((section(".sram_code"))) calculate(int x)
+{
+	data[0] = x;
+
+	return x * x;
+}
+
+int main(void)
+{
+	return calculate(123);
+}
diff --git a/tools/binman/test/elf_sections.lds b/tools/binman/test/elf_sections.lds
new file mode 100644
index 0000000000..7b6e932592
--- /dev/null
+++ b/tools/binman/test/elf_sections.lds
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (c) 2016 Google, Inc
+ */
+
+OUTPUT_FORMAT("elf32-i386", "elf32-i386", "elf32-i386")
+OUTPUT_ARCH(i386)
+ENTRY(_start)
+
+SECTIONS
+{
+	. = 0x00000010;
+	_start = .;
+
+	. = ALIGN(4);
+	.text :
+	{
+		*(.text*)
+	}
+
+	. = 0x00001000;
+	.sram :
+	{
+		*(.sram*)
+	}
+
+	/DISCARD/ : {
+		*(.comment)
+		*(.dyn*)
+	}
+}
-- 
2.35.0.263.gb82422642f-goog

[PATCH 16/24] binman: Move entry-data collection into a Entry method

[Simon Glass]

Collecting the data from a list of entries and putting it in a file is
a useful operation that will be needed by other entry types. Put this into
a method in the Entry class.

Add some documentation about how to collect data for an entry type.

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

 tools/binman/binman.rst       | 86 +++++++++++++++++++++++++++++++++++
 tools/binman/entry.py         | 28 ++++++++++++
 tools/binman/etype/mkimage.py | 14 ++----
 3 files changed, 118 insertions(+), 10 deletions(-)

diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst
index d6b95de1f5..771645380e 100644
--- a/tools/binman/binman.rst
+++ b/tools/binman/binman.rst
@@ -1358,6 +1358,92 @@ development, since dealing with exceptions and problems in threads is more
 difficult. This avoids any use of ThreadPoolExecutor.
 
 
+Collecting data for an entry type
+---------------------------------
+
+Some entry types deal with data obtained from others. For example,
+`Entry_mkimage` calls the `mkimage` tool with data from its subnodes::
+
+    mkimage {
+        args = "-n test -T script";
+
+        u-boot-spl {
+        };
+
+        u-boot {
+        };
+    };
+
+This shows mkimage being passed a file consisting of SPL and U-Boot proper. It
+is create by calling `Entry.collect_contents_to_file()`. Note that in this case,
+the data is passed to mkimage for processing but does not appear separately in
+the image. It may not appear at all, depending on what mkimage does. The
+contents of the `mkimage` entry are entirely dependent on the processing done
+by the entry, with the provided subnodes (`u-boot-spl` and `u-boot`) simply
+providing the input data for that processing.
+
+Note that `Entry.collect_contents_to_file()` simply concatenates the data from
+the different entries together, with no control over alignment, etc. Another
+approach is to subclass `Entry_section` so that those features become available,
+such as `size` and `pad-byte`. Then the contents of the entry can be obtained by
+calling `BuildSectionData()`.
+
+There are other ways to obtain data also, depending on the situation. If the
+entry type is simply signing data which exists elsewhere in the image, then
+you can use `Entry_collection`  as a base class. It lets you use a property
+called `content` which lists the entries containing data to be processed. This
+is used by `Entry_vblock`, for example::
+
+    u_boot: u-boot {
+    };
+    vblock {
+        content = <&u_boot &dtb>;
+        keyblock = "firmware.keyblock";
+        signprivate = "firmware_data_key.vbprivk";
+        version = <1>;
+        kernelkey = "kernel_subkey.vbpubk";
+        preamble-flags = <1>;
+    };
+
+    dtb: u-boot-dtb {
+    };
+
+which shows an image containing `u-boot` and `u-boot-dtb`, with the `vblock`
+image collecting their contents to produce input for its signing process,
+without affecting those entries, which still appear in the final image
+untouched.
+
+Another example is where an entry type needs several independent pieces of input
+to function. For example, `Entry_fip` allows a number of different binary blobs
+to be placed in their own individual places in a custom data structure in the
+output image. To make that work you can add subnodes for each of them and call
+`Entry.Create()` on each subnode, as `Entry_fip` does. Then the data for each
+blob can come from any suitable place, such as an `Entry_u_boot` or an
+`Entry_blob` or anything else::
+
+    atf-fip {
+        fip-hdr-flags = /bits/ 64 <0x123>;
+        soc-fw {
+            fip-flags = /bits/ 64 <0x123456789abcdef>;
+            filename = "bl31.bin";
+        };
+
+        u-boot {
+            fip-uuid = [fc 65 13 92 4a 5b 11 ec
+                    94 35 ff 2d 1c fc 79 9c];
+        };
+    };
+
+The `soc-fw` node is a `blob-ext` (i.e. it reads in a named binary file) whereas
+`u-boot` is a normal entry type. This works because `Entry_fip` selects the
+`blob-ext` entry type if the node name (here `soc-fw`) is recognised as being
+a known blob type.
+
+When adding new entry types you are encouraged to use subnodes to provide the
+data for processing, unless the `content` approach is more suitable. Ad-hoc
+properties and other methods of obtaining data are discouraged, since it adds to
+confusion for users.
+
 History / Credits
 -----------------
 
diff --git a/tools/binman/entry.py b/tools/binman/entry.py
index 08770ec5f0..3eafa078ae 100644
--- a/tools/binman/entry.py
+++ b/tools/binman/entry.py
@@ -1108,3 +1108,31 @@ features to produce new behaviours.
         btool = bintool.Bintool.create(name)
         tools[name] = btool
         return btool
+
+    def collect_contents_to_file(self, entries, prefix):
+        """Put the contents of a list of entries into a file
+
+        Args:
+            entries (list of Entry): Entries to collect
+            prefix (str): Filename prefix of file to write to
+
+        If any entry does not have contents yet, this function returns False
+        for the data.
+
+        Returns:
+            Tuple:
+                bytes: Concatenated data from all the entries (or False)
+                str: Filename of file written (or False if no data)
+                str: Unique portion of filename (or False if no data)
+        """
+        data = b''
+        for entry in entries:
+            # First get the input data and put it in a file. If not available,
+            # try later.
+            if not entry.ObtainContents():
+                return False, False, False
+            data += entry.GetData()
+        uniq = self.GetUniqueName()
+        fname = tools.GetOutputFilename(f'{prefix}.{uniq}')
+        tools.WriteFile(fname, data)
+        return data, fname, uniq
diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py
index 9ecd1c2548..145a50f1aa 100644
--- a/tools/binman/etype/mkimage.py
+++ b/tools/binman/etype/mkimage.py
@@ -51,16 +51,10 @@ class Entry_mkimage(Entry):
         self.ReadEntries()
 
     def ObtainContents(self):
-        data = b''
-        for entry in self._mkimage_entries.values():
-            # First get the input data and put it in a file. If not available,
-            # try later.
-            if not entry.ObtainContents():
-                return False
-            data += entry.GetData()
-        uniq = self.GetUniqueName()
-        input_fname = tools.GetOutputFilename('mkimage.%s' % uniq)
-        tools.WriteFile(input_fname, data)
+        data, input_fname, uniq = self.collect_contents_to_file(
+            self._mkimage_entries.values(), 'mkimage')
+        if data is False:
+            return False
         output_fname = tools.GetOutputFilename('mkimage-out.%s' % uniq)
         if self.mkimage.run_cmd('-d', input_fname, *self._args,
                                 output_fname) is not None:
-- 
2.35.0.263.gb82422642f-goog

Re: [PATCH 16/24] binman: Move entry-data collection into a Entry method

[Alper Nebi Yasak]

On 08/02/2022 21:50, Simon Glass wrote:
> Collecting the data from a list of entries and putting it in a file is
> a useful operation that will be needed by other entry types. Put this into
> a method in the Entry class.
> 
> Add some documentation about how to collect data for an entry type.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  tools/binman/binman.rst       | 86 +++++++++++++++++++++++++++++++++++
>  tools/binman/entry.py         | 28 ++++++++++++
>  tools/binman/etype/mkimage.py | 14 ++----
>  3 files changed, 118 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst
> index d6b95de1f5..771645380e 100644
> --- a/tools/binman/binman.rst
> +++ b/tools/binman/binman.rst
> @@ -1358,6 +1358,92 @@ development, since dealing with exceptions and problems in threads is more
>  difficult. This avoids any use of ThreadPoolExecutor.
>  
>  
> +Collecting data for an entry type
> +---------------------------------
> +
> +Some entry types deal with data obtained from others. For example,
> +`Entry_mkimage` calls the `mkimage` tool with data from its subnodes::
> +
> +    mkimage {
> +        args = "-n test -T script";
> +
> +        u-boot-spl {
> +        };
> +
> +        u-boot {
> +        };
> +    };
> +
> +This shows mkimage being passed a file consisting of SPL and U-Boot proper. It
> +is create by calling `Entry.collect_contents_to_file()`. Note that in this case,

create -> created

> +the data is passed to mkimage for processing but does not appear separately in
> +the image. It may not appear at all, depending on what mkimage does. The
> +contents of the `mkimage` entry are entirely dependent on the processing done
> +by the entry, with the provided subnodes (`u-boot-spl` and `u-boot`) simply
> +providing the input data for that processing.
> +
> +Note that `Entry.collect_contents_to_file()` simply concatenates the data from
> +the different entries together, with no control over alignment, etc. Another
> +approach is to subclass `Entry_section` so that those features become available,
> +such as `size` and `pad-byte`. Then the contents of the entry can be obtained by
> +calling `BuildSectionData()`.

Just saying BuildSectionData() sounds a bit unclear. I think you mean
that subclasses can call super().BuildSectionData() in their
BuildSectionData() implementation to get the input data, then write it
to a file and process it however they like into the final entry data.

After subclassing FIT from Section I've been thinking that any entry
that has subnodes for entries could be likewise made into a Section to
reuse its subentry management code. Anything preventing that for mkimage
and others that might use collect_contents_to_file?

> +
> +There are other ways to obtain data also, depending on the situation. If the
> +entry type is simply signing data which exists elsewhere in the image, then
> +you can use `Entry_collection`  as a base class. It lets you use a property
> +called `content` which lists the entries containing data to be processed. This
> +is used by `Entry_vblock`, for example::
> +
> +    u_boot: u-boot {
> +    };

I like putting spaces between sibling nodes, could use one here and in
atf-fip below for consistency.

> +    vblock {
> +        content = <&u_boot &dtb>;
> +        keyblock = "firmware.keyblock";
> +        signprivate = "firmware_data_key.vbprivk";
> +        version = <1>;
> +        kernelkey = "kernel_subkey.vbpubk";
> +        preamble-flags = <1>;
> +    };
> +
> +    dtb: u-boot-dtb {
> +    };
> +
> +which shows an image containing `u-boot` and `u-boot-dtb`, with the `vblock`
> +image collecting their contents to produce input for its signing process,
> +without affecting those entries, which still appear in the final image
> +untouched.
> +
> +Another example is where an entry type needs several independent pieces of input
> +to function. For example, `Entry_fip` allows a number of different binary blobs
> +to be placed in their own individual places in a custom data structure in the
> +output image. To make that work you can add subnodes for each of them and call
> +`Entry.Create()` on each subnode, as `Entry_fip` does. Then the data for each
> +blob can come from any suitable place, such as an `Entry_u_boot` or an
> +`Entry_blob` or anything else::
> +
> +    atf-fip {
> +        fip-hdr-flags = /bits/ 64 <0x123>;
> +        soc-fw {
> +            fip-flags = /bits/ 64 <0x123456789abcdef>;
> +            filename = "bl31.bin";
> +        };
> +
> +        u-boot {
> +            fip-uuid = [fc 65 13 92 4a 5b 11 ec
> +                    94 35 ff 2d 1c fc 79 9c];
> +        };
> +    };
> +
> +The `soc-fw` node is a `blob-ext` (i.e. it reads in a named binary file) whereas
> +`u-boot` is a normal entry type. This works because `Entry_fip` selects the
> +`blob-ext` entry type if the node name (here `soc-fw`) is recognised as being
> +a known blob type.
> +
> +When adding new entry types you are encouraged to use subnodes to provide the
> +data for processing, unless the `content` approach is more suitable. Ad-hoc
> +properties and other methods of obtaining data are discouraged, since it adds to
> +confusion for users.

I think a good way to make this decision is to consider whether the
input entries are contained within (or consumed by) the entry, vs just
being 'referenced' by the entry.

> +
>  History / Credits
>  -----------------
>  
> diff --git a/tools/binman/entry.py b/tools/binman/entry.py
> index 08770ec5f0..3eafa078ae 100644
> --- a/tools/binman/entry.py
> +++ b/tools/binman/entry.py
> @@ -1108,3 +1108,31 @@ features to produce new behaviours.
>          btool = bintool.Bintool.create(name)
>          tools[name] = btool
>          return btool
> +
> +    def collect_contents_to_file(self, entries, prefix):
> +        """Put the contents of a list of entries into a file
> +
> +        Args:
> +            entries (list of Entry): Entries to collect
> +            prefix (str): Filename prefix of file to write to
> +
> +        If any entry does not have contents yet, this function returns False
> +        for the data.
> +
> +        Returns:
> +            Tuple:
> +                bytes: Concatenated data from all the entries (or False)
> +                str: Filename of file written (or False if no data)
> +                str: Unique portion of filename (or False if no data)
> +        """
> +        data = b''
> +        for entry in entries:
> +            # First get the input data and put it in a file. If not available,
> +            # try later.
> +            if not entry.ObtainContents():
> +                return False, False, False

I'd prefer None over False for return types other than bool.

> +            data += entry.GetData()
> +        uniq = self.GetUniqueName()
> +        fname = tools.GetOutputFilename(f'{prefix}.{uniq}')
> +        tools.WriteFile(fname, data)
> +        return data, fname, uniq
> diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py
> index 9ecd1c2548..145a50f1aa 100644
> --- a/tools/binman/etype/mkimage.py
> +++ b/tools/binman/etype/mkimage.py
> @@ -51,16 +51,10 @@ class Entry_mkimage(Entry):
>          self.ReadEntries()
>  
>      def ObtainContents(self):
> -        data = b''
> -        for entry in self._mkimage_entries.values():
> -            # First get the input data and put it in a file. If not available,
> -            # try later.
> -            if not entry.ObtainContents():
> -                return False
> -            data += entry.GetData()
> -        uniq = self.GetUniqueName()
> -        input_fname = tools.GetOutputFilename('mkimage.%s' % uniq)
> -        tools.WriteFile(input_fname, data)
> +        data, input_fname, uniq = self.collect_contents_to_file(
> +            self._mkimage_entries.values(), 'mkimage')
> +        if data is False:
> +            return False
>          output_fname = tools.GetOutputFilename('mkimage-out.%s' % uniq)
>          if self.mkimage.run_cmd('-d', input_fname, *self._args,
>                                  output_fname) is not None:

Re: [PATCH 16/24] binman: Move entry-data collection into a Entry method

[Simon Glass]

On 08/02/2022 21:50, Simon Glass wrote:
> Collecting the data from a list of entries and putting it in a file is
> a useful operation that will be needed by other entry types. Put this into
> a method in the Entry class.
>
> Add some documentation about how to collect data for an entry type.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  tools/binman/binman.rst       | 86 +++++++++++++++++++++++++++++++++++
>  tools/binman/entry.py         | 28 ++++++++++++
>  tools/binman/etype/mkimage.py | 14 ++----
>  3 files changed, 118 insertions(+), 10 deletions(-)
>
Applied to u-boot-dm, thanks!

[PATCH 17/24] binman: fit: Refactor to reduce function size

[Simon Glass]

Split subnode and property processing into separate functions to make
the _AddNode() function a little smaller. Tweak a few comments.

This does not change any functionality.

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

 tools/binman/etype/fit.py | 116 ++++++++++++++++++++++++--------------
 1 file changed, 73 insertions(+), 43 deletions(-)

diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
index 6ad4a686df..b159844960 100644
--- a/tools/binman/etype/fit.py
+++ b/tools/binman/etype/fit.py
@@ -141,12 +141,82 @@ class Entry_fit(Entry):
         super().ReadNode()
 
     def ReadEntries(self):
+        def _process_prop(pname, prop):
+            """Process special properties
+
+            Handles properties with generated values. At present the only
+            supported property is 'default', i.e. the default device tree in
+            the configurations node.
+
+            Args:
+                pname (str): Name of property
+                prop (Prop): Property to process
+            """
+            if pname == 'default':
+                val = prop.value
+                # Handle the 'default' property
+                if val.startswith('@'):
+                    if not self._fdts:
+                        return
+                    if not self._fit_default_dt:
+                        self.Raise("Generated 'default' node requires default-dt entry argument")
+                    if self._fit_default_dt not in self._fdts:
+                        self.Raise("default-dt entry argument '%s' not found in fdt list: %s" %
+                                   (self._fit_default_dt,
+                                    ', '.join(self._fdts)))
+                    seq = self._fdts.index(self._fit_default_dt)
+                    val = val[1:].replace('DEFAULT-SEQ', str(seq + 1))
+                    fsw.property_string(pname, val)
+                    return
+            fsw.property(pname, prop.bytes)
+
+        def _generate_node(subnode, depth, in_images):
+            """Generate nodes from a template
+
+            This creates one node for each member of self._fdts using the
+            provided template. If a property value contains 'NAME' it is
+            replaced with the filename of the FDT. If a property value contains
+            SEQ it is replaced with the node sequence number, where 1 is the
+            first.
+
+            Args:
+                subnode (None): 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:
+                # Generate nodes for each FDT
+                for seq, fdt_fname in enumerate(self._fdts):
+                    node_name = subnode.name[1:].replace('SEQ',
+                                                         str(seq + 1))
+                    fname = tools.GetInputFilename(fdt_fname + '.dtb')
+                    with fsw.add_node(node_name):
+                        for pname, prop in subnode.props.items():
+                            val = prop.bytes.replace(
+                                b'NAME', tools.ToBytes(fdt_fname))
+                            val = val.replace(
+                                b'SEQ', tools.ToBytes(str(seq + 1)))
+                            fsw.property(pname, val)
+
+                        # Add data for 'images' nodes (but not 'config')
+                        if depth == 1 and in_images:
+                            fsw.property('data',
+                                         tools.ReadFile(fname))
+            else:
+                if self._fdts is None:
+                    if self._fit_list_prop:
+                        self.Raise("Generator node requires '%s' entry argument" %
+                                   self._fit_list_prop.value)
+                    else:
+                        self.Raise("Generator node requires 'fit,fdt-list' property")
+
         def _AddNode(base_node, depth, node):
             """Add a node to the FIT
 
             Args:
                 base_node: Base Node of the FIT (with 'description' property)
-                depth: Current node depth (0 is the base node)
+                depth: Current node depth (0 is the base 'fit' node)
                 node: Current node to process
 
             There are two cases to deal with:
@@ -156,23 +226,7 @@ class Entry_fit(Entry):
             """
             for pname, prop in node.props.items():
                 if not pname.startswith('fit,'):
-                    if pname == 'default':
-                        val = prop.value
-                        # Handle the 'default' property
-                        if val.startswith('@'):
-                            if not self._fdts:
-                                continue
-                            if not self._fit_default_dt:
-                                self.Raise("Generated 'default' node requires default-dt entry argument")
-                            if self._fit_default_dt not in self._fdts:
-                                self.Raise("default-dt entry argument '%s' not found in fdt list: %s" %
-                                           (self._fit_default_dt,
-                                            ', '.join(self._fdts)))
-                            seq = self._fdts.index(self._fit_default_dt)
-                            val = val[1:].replace('DEFAULT-SEQ', str(seq + 1))
-                            fsw.property_string(pname, val)
-                            continue
-                    fsw.property(pname, prop.bytes)
+                    _process_prop(pname, prop)
 
             rel_path = node.path[len(base_node.path):]
             in_images = rel_path.startswith('/images')
@@ -195,31 +249,7 @@ class Entry_fit(Entry):
                     # fsw.add_node() or _AddNode() for it.
                     pass
                 elif self.GetImage().generate and subnode.name.startswith('@'):
-                    if self._fdts:
-                        # Generate notes for each FDT
-                        for seq, fdt_fname in enumerate(self._fdts):
-                            node_name = subnode.name[1:].replace('SEQ',
-                                                                 str(seq + 1))
-                            fname = tools.GetInputFilename(fdt_fname + '.dtb')
-                            with fsw.add_node(node_name):
-                                for pname, prop in subnode.props.items():
-                                    val = prop.bytes.replace(
-                                        b'NAME', tools.ToBytes(fdt_fname))
-                                    val = val.replace(
-                                        b'SEQ', tools.ToBytes(str(seq + 1)))
-                                    fsw.property(pname, val)
-
-                                # Add data for 'fdt' nodes (but not 'config')
-                                if depth == 1 and in_images:
-                                    fsw.property('data',
-                                                 tools.ReadFile(fname))
-                    else:
-                        if self._fdts is None:
-                            if self._fit_list_prop:
-                                self.Raise("Generator node requires '%s' entry argument" %
-                                           self._fit_list_prop.value)
-                            else:
-                                self.Raise("Generator node requires 'fit,fdt-list' property")
+                    _generate_node(subnode, depth, in_images)
                 else:
                     with fsw.add_node(subnode.name):
                         _AddNode(base_node, depth + 1, subnode)
-- 
2.35.0.263.gb82422642f-goog

Re: [PATCH 17/24] binman: fit: Refactor to reduce function size

[Alper Nebi Yasak]

On 08/02/2022 21:50, Simon Glass wrote:
> Split subnode and property processing into separate functions to make
> the _AddNode() function a little smaller. Tweak a few comments.
> 
> This does not change any functionality.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---

I know this just moves code around a bit, but I think the code here
could be cleaned up much further with a bit of redesign. I'm not sure of
the details, but was thinking of at least:

- self._add_fit_image() to handle image/* subnodes
- self._add_fit_config() to handle configuration/* subnodes
- self._gen_fdt_nodes() to handle template nodes by calling the above
- Switching away from recursion to iterating subnodes of fixed nodes

> 
>  tools/binman/etype/fit.py | 116 ++++++++++++++++++++++++--------------
>  1 file changed, 73 insertions(+), 43 deletions(-)
> 
> diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
> index 6ad4a686df..b159844960 100644
> --- a/tools/binman/etype/fit.py
> +++ b/tools/binman/etype/fit.py
> @@ -141,12 +141,82 @@ class Entry_fit(Entry):
>          super().ReadNode()
>  
>      def ReadEntries(self):

I think the following functions could be moved out of this one into the
class scope, even including _AddNode.

> +        def _process_prop(pname, prop):
> +            """Process special properties
> +
> +            Handles properties with generated values. At present the only
> +            supported property is 'default', i.e. the default device tree in
> +            the configurations node.
> +
> +            Args:
> +                pname (str): Name of property
> +                prop (Prop): Property to process
> +            """
> +            if pname == 'default':
> +                val = prop.value
> +                # Handle the 'default' property
> +                if val.startswith('@'):
> +                    if not self._fdts:
> +                        return
> +                    if not self._fit_default_dt:
> +                        self.Raise("Generated 'default' node requires default-dt entry argument")
> +                    if self._fit_default_dt not in self._fdts:
> +                        self.Raise("default-dt entry argument '%s' not found in fdt list: %s" %
> +                                   (self._fit_default_dt,
> +                                    ', '.join(self._fdts)))
> +                    seq = self._fdts.index(self._fit_default_dt)
> +                    val = val[1:].replace('DEFAULT-SEQ', str(seq + 1))
> +                    fsw.property_string(pname, val)
> +                    return
> +            fsw.property(pname, prop.bytes)
> +
> +        def _generate_node(subnode, depth, in_images):
> +            """Generate nodes from a template
> +
> +            This creates one node for each member of self._fdts using the
> +            provided template. If a property value contains 'NAME' it is
> +            replaced with the filename of the FDT. If a property value contains
> +            SEQ it is replaced with the node sequence number, where 1 is the
> +            first.
> +
> +            Args:
> +                subnode (None): 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:
> +                # Generate nodes for each FDT
> +                for seq, fdt_fname in enumerate(self._fdts):
> +                    node_name = subnode.name[1:].replace('SEQ',
> +                                                         str(seq + 1))
> +                    fname = tools.GetInputFilename(fdt_fname + '.dtb')
> +                    with fsw.add_node(node_name):
> +                        for pname, prop in subnode.props.items():
> +                            val = prop.bytes.replace(
> +                                b'NAME', tools.ToBytes(fdt_fname))
> +                            val = val.replace(
> +                                b'SEQ', tools.ToBytes(str(seq + 1)))
> +                            fsw.property(pname, val)
> +
> +                        # Add data for 'images' nodes (but not 'config')
> +                        if depth == 1 and in_images:
> +                            fsw.property('data',
> +                                         tools.ReadFile(fname))
> +            else:
> +                if self._fdts is None:
> +                    if self._fit_list_prop:
> +                        self.Raise("Generator node requires '%s' entry argument" %
> +                                   self._fit_list_prop.value)
> +                    else:
> +                        self.Raise("Generator node requires 'fit,fdt-list' property")
> +
>          def _AddNode(base_node, depth, node):
>              """Add a node to the FIT
>  
>              Args:
>                  base_node: Base Node of the FIT (with 'description' property)
> -                depth: Current node depth (0 is the base node)
> +                depth: Current node depth (0 is the base 'fit' node)
>                  node: Current node to process
>  
>              There are two cases to deal with:
> @@ -156,23 +226,7 @@ class Entry_fit(Entry):
>              """
>              for pname, prop in node.props.items():
>                  if not pname.startswith('fit,'):
> -                    if pname == 'default':
> -                        val = prop.value
> -                        # Handle the 'default' property
> -                        if val.startswith('@'):
> -                            if not self._fdts:
> -                                continue
> -                            if not self._fit_default_dt:
> -                                self.Raise("Generated 'default' node requires default-dt entry argument")
> -                            if self._fit_default_dt not in self._fdts:
> -                                self.Raise("default-dt entry argument '%s' not found in fdt list: %s" %
> -                                           (self._fit_default_dt,
> -                                            ', '.join(self._fdts)))
> -                            seq = self._fdts.index(self._fit_default_dt)
> -                            val = val[1:].replace('DEFAULT-SEQ', str(seq + 1))
> -                            fsw.property_string(pname, val)
> -                            continue
> -                    fsw.property(pname, prop.bytes)
> +                    _process_prop(pname, prop)
>  
>              rel_path = node.path[len(base_node.path):]
>              in_images = rel_path.startswith('/images')
> @@ -195,31 +249,7 @@ class Entry_fit(Entry):
>                      # fsw.add_node() or _AddNode() for it.
>                      pass
>                  elif self.GetImage().generate and subnode.name.startswith('@'):
> -                    if self._fdts:
> -                        # Generate notes for each FDT
> -                        for seq, fdt_fname in enumerate(self._fdts):
> -                            node_name = subnode.name[1:].replace('SEQ',
> -                                                                 str(seq + 1))
> -                            fname = tools.GetInputFilename(fdt_fname + '.dtb')
> -                            with fsw.add_node(node_name):
> -                                for pname, prop in subnode.props.items():
> -                                    val = prop.bytes.replace(
> -                                        b'NAME', tools.ToBytes(fdt_fname))
> -                                    val = val.replace(
> -                                        b'SEQ', tools.ToBytes(str(seq + 1)))
> -                                    fsw.property(pname, val)
> -
> -                                # Add data for 'fdt' nodes (but not 'config')
> -                                if depth == 1 and in_images:
> -                                    fsw.property('data',
> -                                                 tools.ReadFile(fname))
> -                    else:
> -                        if self._fdts is None:
> -                            if self._fit_list_prop:
> -                                self.Raise("Generator node requires '%s' entry argument" %
> -                                           self._fit_list_prop.value)
> -                            else:
> -                                self.Raise("Generator node requires 'fit,fdt-list' property")
> +                    _generate_node(subnode, depth, in_images)
>                  else:
>                      with fsw.add_node(subnode.name):
>                          _AddNode(base_node, depth + 1, subnode)

Re: [PATCH 17/24] binman: fit: Refactor to reduce function size

[Simon Glass]

Hi Alper,

On Tue, 15 Feb 2022 at 04:53, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> On 08/02/2022 21:50, Simon Glass wrote:
> > Split subnode and property processing into separate functions to make
> > the _AddNode() function a little smaller. Tweak a few comments.
> >
> > This does not change any functionality.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
>
> I know this just moves code around a bit, but I think the code here
> could be cleaned up much further with a bit of redesign. I'm not sure of
> the details, but was thinking of at least:
>
> - self._add_fit_image() to handle image/* subnodes
> - self._add_fit_config() to handle configuration/* subnodes
> - self._gen_fdt_nodes() to handle template nodes by calling the above
> - Switching away from recursion to iterating subnodes of fixed nodes

This code is significantly different now, so see what you think.

The last point is interesting...at present you can use property
substitution in any node. I tend to agree the recursion doesn't really
help, but let me know what you think of the latest version.

>
> >
> >  tools/binman/etype/fit.py | 116 ++++++++++++++++++++++++--------------
> >  1 file changed, 73 insertions(+), 43 deletions(-)
> >
> > diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
> > index 6ad4a686df..b159844960 100644
> > --- a/tools/binman/etype/fit.py
> > +++ b/tools/binman/etype/fit.py
> > @@ -141,12 +141,82 @@ class Entry_fit(Entry):
> >          super().ReadNode()
> >
> >      def ReadEntries(self):
>
> I think the following functions could be moved out of this one into the
> class scope, even including _AddNode.

OK, will hold off on this one until after the series.

[..]

Regards,
Simon

Re: [PATCH 17/24] binman: fit: Refactor to reduce function size

[Simon Glass]

On 08/02/2022 21:50, Simon Glass wrote:
> Split subnode and property processing into separate functions to make
> the _AddNode() function a little smaller. Tweak a few comments.
>
> This does not change any functionality.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---

I know this just moves code around a bit, but I think the code here
could be cleaned up much further with a bit of redesign. I'm not sure of
the details, but was thinking of at least:

- self._add_fit_image() to handle image/* subnodes
- self._add_fit_config() to handle configuration/* subnodes
- self._gen_fdt_nodes() to handle template nodes by calling the above
- Switching away from recursion to iterating subnodes of fixed nodes

>
>  tools/binman/etype/fit.py | 116 ++++++++++++++++++++++++--------------
>  1 file changed, 73 insertions(+), 43 deletions(-)
>
Applied to u-boot-dm, thanks!

[PATCH 18/24] binman: Tidy up the docs a little with fit

[Simon Glass]

Add a few quotes and clarify the data property.

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

 tools/binman/entries.rst  | 15 ++++++++-------
 tools/binman/etype/fit.py | 15 ++++++++-------
 2 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
index b32dc58933..d4a1f333dc 100644
--- a/tools/binman/entries.rst
+++ b/tools/binman/entries.rst
@@ -554,10 +554,10 @@ For example, this creates an image containing a FIT with U-Boot SPL::
     };
 
 U-Boot supports creating fdt and config nodes automatically. To do this,
-pass an of-list property (e.g. -a of-list=file1 file2). This tells binman
-that you want to generates nodes for two files: file1.dtb and file2.dtb
-The fit,fdt-list property (see above) indicates that of-list should be used.
-If the property is missing you will get an error.
+pass an `of-list` property (e.g. `-a of-list=file1 file2`). This tells
+binman that you want to generates nodes for two files: `file1.dtb` and
+`file2.dtb`. The `fit,fdt-list` property (see above) indicates that
+`of-list` should be used. If the property is missing you will get an error.
 
 Then add a 'generator node', a node with a name starting with '@'::
 
@@ -569,10 +569,11 @@ Then add a 'generator node', a node with a name starting with '@'::
         };
     };
 
-This tells binman to create nodes fdt-1 and fdt-2 for each of your two
+This tells binman to create nodes `fdt-1` and `fdt-2` for each of your two
 files. All the properties you specify will be included in the node. This
 node acts like a template to generate the nodes. The generator node itself
 does not appear in the output - it is replaced with what binman generates.
+A 'data' property is created with the contents of the FDT file.
 
 You can create config nodes in a similar way::
 
@@ -586,8 +587,8 @@ You can create config nodes in a similar way::
         };
     };
 
-This tells binman to create nodes config-1 and config-2, i.e. a config for
-each of your two files.
+This tells binman to create nodes `config-1` and `config-2`, i.e. a config
+for each of your two files.
 
 Available substitutions for '@' nodes are:
 
diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
index b159844960..bf84246904 100644
--- a/tools/binman/etype/fit.py
+++ b/tools/binman/etype/fit.py
@@ -47,10 +47,10 @@ class Entry_fit(Entry):
         };
 
     U-Boot supports creating fdt and config nodes automatically. To do this,
-    pass an of-list property (e.g. -a of-list=file1 file2). This tells binman
-    that you want to generates nodes for two files: file1.dtb and file2.dtb
-    The fit,fdt-list property (see above) indicates that of-list should be used.
-    If the property is missing you will get an error.
+    pass an `of-list` property (e.g. `-a of-list=file1 file2`). This tells
+    binman that you want to generates nodes for two files: `file1.dtb` and
+    `file2.dtb`. The `fit,fdt-list` property (see above) indicates that
+    `of-list` should be used. If the property is missing you will get an error.
 
     Then add a 'generator node', a node with a name starting with '@'::
 
@@ -62,10 +62,11 @@ class Entry_fit(Entry):
             };
         };
 
-    This tells binman to create nodes fdt-1 and fdt-2 for each of your two
+    This tells binman to create nodes `fdt-1` and `fdt-2` for each of your two
     files. All the properties you specify will be included in the node. This
     node acts like a template to generate the nodes. The generator node itself
     does not appear in the output - it is replaced with what binman generates.
+    A 'data' property is created with the contents of the FDT file.
 
     You can create config nodes in a similar way::
 
@@ -79,8 +80,8 @@ class Entry_fit(Entry):
             };
         };
 
-    This tells binman to create nodes config-1 and config-2, i.e. a config for
-    each of your two files.
+    This tells binman to create nodes `config-1` and `config-2`, i.e. a config
+    for each of your two files.
 
     Available substitutions for '@' nodes are:
 
-- 
2.35.0.263.gb82422642f-goog

[PATCH 19/24] binman: Allow different operations in FIT generator nodes

[Simon Glass]

At present we only support expanding out FDT nodes. Make the operation
into an @operation property, so that others can be supported.

Re-arrange and tidy up the documentation so that it has separate
headings for each topic.

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

 tools/binman/entries.rst               |  85 +++++++++++-----
 tools/binman/etype/fit.py              | 136 ++++++++++++++++++++-----
 tools/binman/ftest.py                  |   7 ++
 tools/binman/test/220_fit_bad_oper.dts |  27 +++++
 4 files changed, 206 insertions(+), 49 deletions(-)
 create mode 100644 tools/binman/test/220_fit_bad_oper.dts

diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
index d4a1f333dc..d483169712 100644
--- a/tools/binman/entries.rst
+++ b/tools/binman/entries.rst
@@ -553,6 +553,68 @@ For example, this creates an image containing a FIT with U-Boot SPL::
         };
     };
 
+More complex setups can be created, with generated nodes, as described
+below.
+
+Properties (in the 'fit' node itself)
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Special properties have a `fit,` prefix, indicating that they should be
+processed but not included in the final FIT.
+
+The top-level 'fit' node supports the following special properties:
+
+    fit,external-offset
+        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,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
+        `of-list` meaning that `-a of-list="dtb1 dtb2..."` should be passed
+        to binman.
+
+Substitutions
+~~~~~~~~~~~~~
+
+Node names and property values support a basic string-substitution feature.
+Available substitutions for '@' nodes (and property values) are:
+
+SEQ:
+    Sequence number of the generated fdt (1, 2, ...)
+NAME
+    Name of the dtb as provided (i.e. without adding '.dtb')
+
+The `default` property, if present, will be automatically set to the name
+if of configuration whose devicetree matches the `default-dt` entry
+argument, e.g. with `-a default-dt=sun50i-a64-pine64-lts`.
+
+Available substitutions for property values in these nodes are:
+
+DEFAULT-SEQ:
+    Sequence number of the default fdt, as provided by the 'default-dt'
+    entry argument
+
+Available operations
+~~~~~~~~~~~~~~~~~~~~
+
+You can add an operation to an '@' node to indicate which operation is
+required::
+
+    @fdt-SEQ {
+        fit,operation = "gen-fdt-nodes";
+        ...
+    };
+
+Available operations are:
+
+gen-fdt-nodes
+    Generate FDT nodes as above. This is the default if there is no
+    `fit,operation` property.
+
+Generating nodes from an FDT list (gen-fdt-nodes)
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
 U-Boot supports creating fdt and config nodes automatically. To do this,
 pass an `of-list` property (e.g. `-a of-list=file1 file2`). This tells
 binman that you want to generates nodes for two files: `file1.dtb` and
@@ -590,32 +652,9 @@ You can create config nodes in a similar way::
 This tells binman to create nodes `config-1` and `config-2`, i.e. a config
 for each of your two files.
 
-Available substitutions for '@' nodes are:
-
-SEQ:
-    Sequence number of the generated fdt (1, 2, ...)
-NAME
-    Name of the dtb as provided (i.e. without adding '.dtb')
-
 Note that if no devicetree files are provided (with '-a of-list' as above)
 then no nodes will be generated.
 
-The 'default' property, if present, will be automatically set to the name
-if of configuration whose devicetree matches the 'default-dt' entry
-argument, e.g. with '-a default-dt=sun50i-a64-pine64-lts'.
-
-Available substitutions for '@' property values are
-
-DEFAULT-SEQ:
-    Sequence number of the default fdt,as provided by the 'default-dt' entry
-    argument
-
-Properties (in the 'fit' node itself):
-    fit,external-offset: Indicates that the contents of the FIT are external
-        and provides the external offset. This is passsed to mkimage via
-        the -E and -p flags.
-
-
 
 
 Entry: fmap: An entry which contains an Fmap section
diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
index bf84246904..6210deeef7 100644
--- a/tools/binman/etype/fit.py
+++ b/tools/binman/etype/fit.py
@@ -13,6 +13,12 @@ from dtoc import fdt_util
 from dtoc.fdt import Fdt
 from patman import tools
 
+# Supported operations, with the fit,operation property
+OP_GEN_FDT_NODES = range(1)
+OPERATIONS = {
+    'gen-fdt-nodes': OP_GEN_FDT_NODES,
+    }
+
 class Entry_fit(Entry):
     """Flat Image Tree (FIT)
 
@@ -46,6 +52,68 @@ class Entry_fit(Entry):
             };
         };
 
+    More complex setups can be created, with generated nodes, as described
+    below.
+
+    Properties (in the 'fit' node itself)
+    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+    Special properties have a `fit,` prefix, indicating that they should be
+    processed but not included in the final FIT.
+
+    The top-level 'fit' node supports the following special properties:
+
+        fit,external-offset
+            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,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
+            `of-list` meaning that `-a of-list="dtb1 dtb2..."` should be passed
+            to binman.
+
+    Substitutions
+    ~~~~~~~~~~~~~
+
+    Node names and property values support a basic string-substitution feature.
+    Available substitutions for '@' nodes (and property values) are:
+
+    SEQ:
+        Sequence number of the generated fdt (1, 2, ...)
+    NAME
+        Name of the dtb as provided (i.e. without adding '.dtb')
+
+    The `default` property, if present, will be automatically set to the name
+    if of configuration whose devicetree matches the `default-dt` entry
+    argument, e.g. with `-a default-dt=sun50i-a64-pine64-lts`.
+
+    Available substitutions for property values in these nodes are:
+
+    DEFAULT-SEQ:
+        Sequence number of the default fdt, as provided by the 'default-dt'
+        entry argument
+
+    Available operations
+    ~~~~~~~~~~~~~~~~~~~~
+
+    You can add an operation to an '@' node to indicate which operation is
+    required::
+
+        @fdt-SEQ {
+            fit,operation = "gen-fdt-nodes";
+            ...
+        };
+
+    Available operations are:
+
+    gen-fdt-nodes
+        Generate FDT nodes as above. This is the default if there is no
+        `fit,operation` property.
+
+    Generating nodes from an FDT list (gen-fdt-nodes)
+    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
     U-Boot supports creating fdt and config nodes automatically. To do this,
     pass an `of-list` property (e.g. `-a of-list=file1 file2`). This tells
     binman that you want to generates nodes for two files: `file1.dtb` and
@@ -83,31 +151,8 @@ class Entry_fit(Entry):
     This tells binman to create nodes `config-1` and `config-2`, i.e. a config
     for each of your two files.
 
-    Available substitutions for '@' nodes are:
-
-    SEQ:
-        Sequence number of the generated fdt (1, 2, ...)
-    NAME
-        Name of the dtb as provided (i.e. without adding '.dtb')
-
     Note that if no devicetree files are provided (with '-a of-list' as above)
     then no nodes will be generated.
-
-    The 'default' property, if present, will be automatically set to the name
-    if of configuration whose devicetree matches the 'default-dt' entry
-    argument, e.g. with '-a default-dt=sun50i-a64-pine64-lts'.
-
-    Available substitutions for '@' property values are
-
-    DEFAULT-SEQ:
-        Sequence number of the default fdt,as provided by the 'default-dt' entry
-        argument
-
-    Properties (in the 'fit' node itself):
-        fit,external-offset: Indicates that the contents of the FIT are external
-            and provides the external offset. This is passsed to mkimage via
-            the -E and -p flags.
-
     """
     def __init__(self, section, etype, node):
         """
@@ -141,6 +186,26 @@ class Entry_fit(Entry):
         self.ReadEntries()
         super().ReadNode()
 
+    def _get_operation(self, subnode):
+        """Get the operation referenced by a subnode
+
+        Args:
+            subnode (Node): Subnode (of the FIT) to check
+
+        Returns:
+            int: Operation to perform
+
+        Raises:
+            ValueError: Invalid operation name
+        """
+        oper_name = subnode.props.get('fit,operation')
+        if not oper_name:
+            return OP_GEN_FDT_NODES
+        oper = OPERATIONS.get(oper_name.value)
+        if not oper:
+            self.Raise(f"Unknown operation '{oper_name.value}'")
+        return oper
+
     def ReadEntries(self):
         def _process_prop(pname, prop):
             """Process special properties
@@ -171,8 +236,8 @@ class Entry_fit(Entry):
                     return
             fsw.property(pname, prop.bytes)
 
-        def _generate_node(subnode, depth, in_images):
-            """Generate nodes from a template
+        def _scan_gen_fdt_nodes(subnode, depth, in_images):
+            """Generate FDT nodes
 
             This creates one node for each member of self._fdts using the
             provided template. If a property value contains 'NAME' it is
@@ -212,6 +277,25 @@ class Entry_fit(Entry):
                     else:
                         self.Raise("Generator node requires 'fit,fdt-list' property")
 
+        def _scan_node(subnode, depth, in_images):
+            """Generate nodes from a template
+
+            This creates one node for each member of self._fdts using the
+            provided template. If a property value contains 'NAME' it is
+            replaced with the filename of the FDT. If a property value contains
+            SEQ it is replaced with the node sequence number, where 1 is the
+            first.
+
+            Args:
+                subnode (None): 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
+            """
+            oper = self._get_operation(subnode)
+            if oper == OP_GEN_FDT_NODES:
+                _scan_gen_fdt_nodes(subnode, depth, in_images)
+
         def _AddNode(base_node, depth, node):
             """Add a node to the FIT
 
@@ -250,7 +334,7 @@ class Entry_fit(Entry):
                     # fsw.add_node() or _AddNode() for it.
                     pass
                 elif self.GetImage().generate and subnode.name.startswith('@'):
-                    _generate_node(subnode, depth, in_images)
+                    _scan_node(subnode, depth, in_images)
                 else:
                     with fsw.add_node(subnode.name):
                         _AddNode(base_node, depth + 1, subnode)
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index 5400f76c67..5a0dc70ed9 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -5118,6 +5118,13 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
         finally:
             shutil.rmtree(tmpdir)
 
+    def testFitFdtBadOper(self):
+        """Check handling of an FDT map when the section cannot be found"""
+        with self.assertRaises(ValueError) as exc:
+            self._DoReadFileDtb('220_fit_bad_oper.dts')
+        self.assertIn("Node '/binman/fit': Unknown operation 'unknown'",
+                      str(exc.exception))
+
 
 if __name__ == "__main__":
     unittest.main()
diff --git a/tools/binman/test/220_fit_bad_oper.dts b/tools/binman/test/220_fit_bad_oper.dts
new file mode 100644
index 0000000000..cee801e2ea
--- /dev/null
+++ b/tools/binman/test/220_fit_bad_oper.dts
@@ -0,0 +1,27 @@
+// 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 {
+				@fdt-SEQ {
+					fit,operation = "unknown";
+					description = "fdt-NAME.dtb";
+					type = "flat_dt";
+					compression = "none";
+				};
+			};
+		};
+		fdtmap {
+		};
+	};
+};
-- 
2.35.0.263.gb82422642f-goog

[PATCH 20/24] binman: Support splitting an ELF file into multiple nodes

[Simon Glass]

Some boards need to load an ELF file using the 'loadables' property, but
the file has segments at different memory addresses. This means that it
cannot be supplied as a flat binary.

Allow generating a separate node in the FIT for each segment in the ELF,
with a different load address for each.

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

 tools/binman/entries.rst                     | 146 +++++++++++
 tools/binman/etype/fit.py                    | 259 ++++++++++++++++++-
 tools/binman/ftest.py                        | 116 +++++++++
 tools/binman/test/221_fit_split_elf.dts      |  67 +++++
 tools/binman/test/222_fit_bad_dir.dts        |   9 +
 tools/binman/test/223_fit_bad_dir_config.dts |   9 +
 6 files changed, 594 insertions(+), 12 deletions(-)
 create mode 100644 tools/binman/test/221_fit_split_elf.dts
 create mode 100644 tools/binman/test/222_fit_bad_dir.dts
 create mode 100644 tools/binman/test/223_fit_bad_dir_config.dts

diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
index d483169712..079fed1a9c 100644
--- a/tools/binman/entries.rst
+++ b/tools/binman/entries.rst
@@ -612,6 +612,9 @@ gen-fdt-nodes
     Generate FDT nodes as above. This is the default if there is no
     `fit,operation` property.
 
+split-elf
+    Split an ELF file into a separate node for each segment.
+
 Generating nodes from an FDT list (gen-fdt-nodes)
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
@@ -655,6 +658,149 @@ for each of your two files.
 Note that if no devicetree files are provided (with '-a of-list' as above)
 then no nodes will be generated.
 
+Generating nodes from an ELF file (split-elf)
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+This uses the node as a template to generate multiple nodes. The following
+special properties are available:
+
+split-elf
+    Split an ELF file into a separate node for each segment. This uses the
+    node as a template to generate multiple nodes. The following special
+    properties are available:
+
+    fit,load
+        Generates a `load = <...>` property with the load address of the
+        segmnet
+
+    fit,entry
+        Generates a `entry = <...>` property with the entry address of the
+        ELF. This is only produced for the first entry
+
+    fit,data
+        Generates a `data = <...>` property with the contents of the segment
+
+    fit,loadables
+        Generates a `loadable = <...>` property with a list of the generated
+        nodes (including all nodes if this operation is used multiple times)
+
+
+Here is an example showing ATF, TEE and a device tree all combined::
+
+    fit {
+        description = "test-desc";
+        #address-cells = <1>;
+        fit,fdt-list = "of-list";
+
+        images {
+            u-boot {
+                description = "U-Boot (64-bit)";
+                type = "standalone";
+                os = "U-Boot";
+                arch = "arm64";
+                compression = "none";
+                load = <CONFIG_SYS_TEXT_BASE>;
+                u-boot-nodtb {
+                };
+            };
+            @fdt-SEQ {
+                description = "fdt-NAME.dtb";
+                type = "flat_dt";
+                compression = "none";
+            };
+            @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 {
+                };
+            };
+
+            @tee-SEQ {
+                fit,operation = "split-elf";
+                description = "TEE";
+                type = "tee";
+                arch = "arm64";
+                os = "tee";
+                compression = "none";
+                fit,load;
+                fit,entry;
+                fit,data;
+
+                op-tee {
+                };
+            };
+        };
+
+        configurations {
+            default = "@config-DEFAULT-SEQ";
+            @config-SEQ {
+                description = "conf-NAME.dtb";
+                fdt = "fdt-SEQ";
+                firmware = "u-boot";
+                fit,loadables;
+            };
+        };
+    };
+
+If ATF-BL31 is available, this generates a node for each segment in the
+ELF file, for example::
+
+    images {
+        atf-1 {
+            data = <...contents of first segment...>;
+            data-offset = <0x00000000>;
+            entry = <0x00040000>;
+            load = <0x00040000>;
+            compression = "none";
+            os = "arm-trusted-firmware";
+            arch = "arm64";
+            type = "firmware";
+            description = "ARM Trusted Firmware";
+        };
+        atf-2 {
+            data = <...contents of second segment...>;
+            load = <0xff3b0000>;
+            compression = "none";
+            os = "arm-trusted-firmware";
+            arch = "arm64";
+            type = "firmware";
+            description = "ARM Trusted Firmware";
+        };
+    };
+
+The same applies for OP-TEE if that is available.
+
+If each binary is not available, the relevant template node (@atf-SEQ or
+@tee-SEQ) is removed from the output.
+
+This also generates a `config-xxx` node for each device tree in `of-list`.
+Note that the U-Boot build system uses `-a of-list=$(CONFIG_OF_LIST)`
+so you can use `CONFIG_OF_LIST` to define that list. In this example it is
+set up for `firefly-rk3399` with a single device tree and the default set
+with `-a default-dt=$(CONFIG_DEFAULT_DEVICE_TREE)`, so the resulting output
+is::
+
+    configurations {
+        default = "config-1";
+        config-1 {
+            loadables = "atf-1", "atf-2", "atf-3", "tee-1", "tee-2";
+            description = "rk3399-firefly.dtb";
+            fdt = "fdt-1";
+            firmware = "u-boot";
+        };
+    };
+
+U-Boot SPL can then load the firmware (U-Boot proper) and all the loadables
+(ATF and TEE), then proceed with the boot.
+
 
 
 Entry: fmap: An entry which contains an Fmap section
diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
index 6210deeef7..b2a037c742 100644
--- a/tools/binman/etype/fit.py
+++ b/tools/binman/etype/fit.py
@@ -6,17 +6,20 @@
 #
 
 from collections import defaultdict, OrderedDict
+import io
 import libfdt
 
 from binman.entry import Entry, EntryArg
+from binman import elf
 from dtoc import fdt_util
 from dtoc.fdt import Fdt
 from patman import tools
 
 # Supported operations, with the fit,operation property
-OP_GEN_FDT_NODES = range(1)
+OP_GEN_FDT_NODES, OP_SPLIT_ELF = range(2)
 OPERATIONS = {
     'gen-fdt-nodes': OP_GEN_FDT_NODES,
+    'split-elf': OP_SPLIT_ELF,
     }
 
 class Entry_fit(Entry):
@@ -111,6 +114,9 @@ class Entry_fit(Entry):
         Generate FDT nodes as above. This is the default if there is no
         `fit,operation` property.
 
+    split-elf
+        Split an ELF file into a separate node for each segment.
+
     Generating nodes from an FDT list (gen-fdt-nodes)
     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
@@ -153,6 +159,149 @@ class Entry_fit(Entry):
 
     Note that if no devicetree files are provided (with '-a of-list' as above)
     then no nodes will be generated.
+
+    Generating nodes from an ELF file (split-elf)
+    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+    This uses the node as a template to generate multiple nodes. The following
+    special properties are available:
+
+    split-elf
+        Split an ELF file into a separate node for each segment. This uses the
+        node as a template to generate multiple nodes. The following special
+        properties are available:
+
+        fit,load
+            Generates a `load = <...>` property with the load address of the
+            segmnet
+
+        fit,entry
+            Generates a `entry = <...>` property with the entry address of the
+            ELF. This is only produced for the first entry
+
+        fit,data
+            Generates a `data = <...>` property with the contents of the segment
+
+        fit,loadables
+            Generates a `loadable = <...>` property with a list of the generated
+            nodes (including all nodes if this operation is used multiple times)
+
+
+    Here is an example showing ATF, TEE and a device tree all combined::
+
+        fit {
+            description = "test-desc";
+            #address-cells = <1>;
+            fit,fdt-list = "of-list";
+
+            images {
+                u-boot {
+                    description = "U-Boot (64-bit)";
+                    type = "standalone";
+                    os = "U-Boot";
+                    arch = "arm64";
+                    compression = "none";
+                    load = <CONFIG_SYS_TEXT_BASE>;
+                    u-boot-nodtb {
+                    };
+                };
+                @fdt-SEQ {
+                    description = "fdt-NAME.dtb";
+                    type = "flat_dt";
+                    compression = "none";
+                };
+                @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 {
+                    };
+                };
+
+                @tee-SEQ {
+                    fit,operation = "split-elf";
+                    description = "TEE";
+                    type = "tee";
+                    arch = "arm64";
+                    os = "tee";
+                    compression = "none";
+                    fit,load;
+                    fit,entry;
+                    fit,data;
+
+                    op-tee {
+                    };
+                };
+            };
+
+            configurations {
+                default = "@config-DEFAULT-SEQ";
+                @config-SEQ {
+                    description = "conf-NAME.dtb";
+                    fdt = "fdt-SEQ";
+                    firmware = "u-boot";
+                    fit,loadables;
+                };
+            };
+        };
+
+    If ATF-BL31 is available, this generates a node for each segment in the
+    ELF file, for example::
+
+        images {
+            atf-1 {
+                data = <...contents of first segment...>;
+                data-offset = <0x00000000>;
+                entry = <0x00040000>;
+                load = <0x00040000>;
+                compression = "none";
+                os = "arm-trusted-firmware";
+                arch = "arm64";
+                type = "firmware";
+                description = "ARM Trusted Firmware";
+            };
+            atf-2 {
+                data = <...contents of second segment...>;
+                load = <0xff3b0000>;
+                compression = "none";
+                os = "arm-trusted-firmware";
+                arch = "arm64";
+                type = "firmware";
+                description = "ARM Trusted Firmware";
+            };
+        };
+
+    The same applies for OP-TEE if that is available.
+
+    If each binary is not available, the relevant template node (@atf-SEQ or
+    @tee-SEQ) is removed from the output.
+
+    This also generates a `config-xxx` node for each device tree in `of-list`.
+    Note that the U-Boot build system uses `-a of-list=$(CONFIG_OF_LIST)`
+    so you can use `CONFIG_OF_LIST` to define that list. In this example it is
+    set up for `firefly-rk3399` with a single device tree and the default set
+    with `-a default-dt=$(CONFIG_DEFAULT_DEVICE_TREE)`, so the resulting output
+    is::
+
+        configurations {
+            default = "config-1";
+            config-1 {
+                loadables = "atf-1", "atf-2", "atf-3", "tee-1", "tee-2";
+                description = "rk3399-firefly.dtb";
+                fdt = "fdt-1";
+                firmware = "u-boot";
+            };
+        };
+
+    U-Boot SPL can then load the firmware (U-Boot proper) and all the loadables
+    (ATF and TEE), then proceed with the boot.
     """
     def __init__(self, section, etype, node):
         """
@@ -182,6 +331,12 @@ class Entry_fit(Entry):
                                                                   str)])[0]
         self.mkimage = None
 
+        # List of generated split-elf nodes, each a None
+        self._loadables = []
+
+        # /configurations/xxx node used as a template
+        self._config_node = None
+
     def ReadNode(self):
         self.ReadEntries()
         super().ReadNode()
@@ -251,19 +406,21 @@ class Entry_fit(Entry):
                 in_images: True if this is inside the 'images' node, so that
                     'data' properties should be generated
             """
+            if depth == 1 and not in_images:
+                self._config_node = subnode
             if self._fdts:
                 # Generate nodes for each FDT
                 for seq, fdt_fname in enumerate(self._fdts):
-                    node_name = subnode.name[1:].replace('SEQ',
-                                                         str(seq + 1))
+                    node_name = subnode.name[1:].replace('SEQ', str(seq + 1))
                     fname = tools.GetInputFilename(fdt_fname + '.dtb')
                     with fsw.add_node(node_name):
                         for pname, prop in subnode.props.items():
-                            val = prop.bytes.replace(
-                                b'NAME', tools.ToBytes(fdt_fname))
-                            val = val.replace(
-                                b'SEQ', tools.ToBytes(str(seq + 1)))
-                            fsw.property(pname, val)
+                            if not pname.startswith('fit,'):
+                                val = prop.bytes.replace(
+                                    b'NAME', tools.ToBytes(fdt_fname))
+                                val = val.replace(
+                                    b'SEQ', tools.ToBytes(str(seq + 1)))
+                                fsw.property(pname, val)
 
                         # Add data for 'images' nodes (but not 'config')
                         if depth == 1 and in_images:
@@ -277,7 +434,18 @@ class Entry_fit(Entry):
                     else:
                         self.Raise("Generator node requires 'fit,fdt-list' property")
 
-        def _scan_node(subnode, depth, in_images):
+        def _scan_split_elf(subnode, rel_path):
+            #data, input_fname, uniq = self.collect_contents_to_file(
+            entry = Entry.Create(self.section, subnode, etype='section')
+            entry.ReadNode()
+            self._fit_sections[rel_path] = entry
+
+            # Add this as a dummy node so we know the required position in the
+            # output FIT. It is replaced later in _BuildInput().
+            with fsw.add_node(subnode.name):
+                pass
+
+        def _scan_node(subnode, depth, in_images, rel_path):
             """Generate nodes from a template
 
             This creates one node for each member of self._fdts using the
@@ -291,10 +459,14 @@ class Entry_fit(Entry):
                 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
+                rel_path (str): Path of subnode relative to the toplevel 'fit'
+                    node
             """
             oper = self._get_operation(subnode)
             if oper == OP_GEN_FDT_NODES:
                 _scan_gen_fdt_nodes(subnode, depth, in_images)
+            elif oper == OP_SPLIT_ELF:
+                _scan_split_elf(subnode, rel_path)
 
         def _AddNode(base_node, depth, node):
             """Add a node to the FIT
@@ -334,7 +506,8 @@ class Entry_fit(Entry):
                     # fsw.add_node() or _AddNode() for it.
                     pass
                 elif self.GetImage().generate and subnode.name.startswith('@'):
-                    _scan_node(subnode, depth, in_images)
+                    _scan_node(subnode, depth, in_images,
+                               f'{rel_path}/{subnode.name}')
                 else:
                     with fsw.add_node(subnode.name):
                         _AddNode(base_node, depth + 1, subnode)
@@ -383,6 +556,45 @@ class Entry_fit(Entry):
 
         return True
 
+    def _add_split_elf(self, orig_node, node, data, missing):
+        """Add nodes for the ELF file, one per group of contiguous segments
+
+        The existing placeholder node is replaced
+
+        Args:
+            orig_node (Node): Template node from the binman definition
+            node (Node): Node to replace (in the FIT being built)
+            data (bytes): ELF-format data to process (may be empty)
+        """
+        # If any pieces are missing, skip this. The missing entries will show
+        # an error
+        if not missing:
+            try:
+                segments, entry = elf.read_segments(data)
+            except ValueError as exc:
+                self.Raise(f'Failed to read ELF file for {orig_node.path}: {str(exc)}')
+            parent = node.parent
+            for (seq, start, data) in segments:
+                node_name = orig_node.name[1:].replace('SEQ', str(seq + 1))
+                subnode = parent.AddSubnode(node_name) #, before=node)
+                self._loadables.append(subnode)
+                for pname, prop in orig_node.props.items():
+                    if not pname.startswith('fit,'):
+                        subnode.AddData(pname, prop.bytes)
+                    elif pname == 'fit,load':
+                        subnode.AddInt('load', start)
+                    elif pname == 'fit,entry':
+                        if not seq:
+                            subnode.AddInt('entry', entry)
+                    elif pname == 'fit,data':
+                        subnode.AddData('data', data)
+                    elif pname != 'fit,operation':
+                        self.Raise(
+                            f"Unknown directive in '{subnode.path}': '{pname}'")
+
+        # Delete the template node as it has served its purpose
+        node.Delete()
+
     def _BuildInput(self, fdt):
         """Finish the FIT by adding the 'data' properties to it
 
@@ -393,13 +605,36 @@ class Entry_fit(Entry):
             New fdt contents (bytes)
         """
         for path, section in self._fit_sections.items():
-            node = fdt.GetNode(path)
             # Entry_section.ObtainContents() either returns True or
             # raises an exception.
             section.ObtainContents()
             section.Pack(0)
             data = section.GetData()
-            node.AddData('data', data)
+            missing_list = []
+            section.CheckMissing(missing_list)
+
+            node = fdt.GetNode(path)
+            oper = self._get_operation(section._node)
+            if oper == OP_GEN_FDT_NODES:
+                node.AddData('data', data)
+            elif oper == OP_SPLIT_ELF:
+                self._add_split_elf(section._node, node, data,
+                                    bool(missing_list))
+
+        # Set up the 'firmware' and 'loadables' properties in all
+        # 'configurations' nodes, but only if we are generating FDTs. Note that
+        # self._config_node is set in _scan_gen_fdt_nodes()
+        node = fdt.GetNode('/configurations')
+        if self._config_node:
+            for subnode in node.subnodes:
+                for pname, prop in self._config_node.props.items():
+                    if pname == 'fit,loadables':
+                        subnode.AddStringList(
+                            'loadables',
+                            [node.name for node in self._loadables])
+                    elif pname.startswith('fit,'):
+                        self.Raise(
+                            f"Unknown directive in '{subnode.path}': '{pname}'")
 
         fdt.Sync(auto_resize=True)
         data = fdt.GetContents()
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index 5a0dc70ed9..dbaf412e9d 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -197,6 +197,13 @@ class TestFunctional(unittest.TestCase):
 
         TestFunctional._MakeInputFile('env.txt', ENV_DATA)
 
+        # ELF file with two sections in different parts of memory, used for both
+        # ATF and OP_TEE
+        TestFunctional._MakeInputFile('bl31.elf',
+            tools.ReadFile(cls.ElfTestFile('elf_sections')))
+        TestFunctional._MakeInputFile('tee.elf',
+            tools.ReadFile(cls.ElfTestFile('elf_sections')))
+
         cls.have_lz4 = comp_util.HAVE_LZ4
 
     @classmethod
@@ -5125,6 +5132,115 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
         self.assertIn("Node '/binman/fit': Unknown operation 'unknown'",
                       str(exc.exception))
 
+    def testFitSplitElf(self):
+        """Test an image with an FIT with an split-elf operation"""
+        entry_args = {
+            'of-list': 'test-fdt1 test-fdt2',
+            'default-dt': 'test-fdt2',
+            'atf-bl31-path': 'bl31.elf',
+            'op-tee-path': 'tee.elf',
+        }
+        test_subdir = os.path.join(self._indir, TEST_FDT_SUBDIR)
+        data = self._DoReadFileDtb(
+            '221_fit_split_elf.dts',
+            entry_args=entry_args,
+            extra_indirs=[test_subdir])[0]
+
+        self.assertEqual(U_BOOT_NODTB_DATA, data[-len(U_BOOT_NODTB_DATA):])
+        fit_data = data[len(U_BOOT_DATA):-len(U_BOOT_NODTB_DATA)]
+
+        base_keys = {'description', 'type', 'arch', 'os', 'compression',
+                     'data', 'load'}
+        dtb = fdt.Fdt.FromData(fit_data)
+        dtb.Scan()
+
+        elf_data = tools.ReadFile(os.path.join(self._indir, 'bl31.elf'))
+        segments, entry = elf.read_segments(elf_data)
+
+        # We assume there are two segments
+        self.assertEquals(2, len(segments))
+
+        atf1 = dtb.GetNode('/images/atf-1')
+        _, start, data = segments[0]
+        self.assertEqual(base_keys | {'entry'}, atf1.props.keys())
+        self.assertEqual(entry,
+                         fdt_util.fdt32_to_cpu(atf1.props['entry'].value))
+        self.assertEqual(start,
+                         fdt_util.fdt32_to_cpu(atf1.props['load'].value))
+        self.assertEqual(data, atf1.props['data'].bytes)
+
+        atf2 = dtb.GetNode('/images/atf-2')
+        self.assertEqual(base_keys, atf2.props.keys())
+        _, start, data = segments[1]
+        self.assertEqual(start,
+                         fdt_util.fdt32_to_cpu(atf2.props['load'].value))
+        self.assertEqual(data, atf2.props['data'].bytes)
+
+        conf = dtb.GetNode('/configurations')
+        self.assertEqual({'default'}, conf.props.keys())
+
+        for subnode in conf.subnodes:
+            self.assertEqual({'description', 'fdt', 'loadables'},
+                             subnode.props.keys())
+            self.assertEqual(
+                ['atf-1', 'atf-2', 'tee-1', 'tee-2'],
+                fdt_util.GetStringList(subnode, 'loadables'))
+
+    def _check_bad_fit(self, dts):
+        """Check a bad FIT
+
+        This runs with the given dts and returns the assertion raised
+
+        Args:
+            dts (str): dts filename to use
+
+        Returns:
+            str: Assertion string raised
+        """
+        entry_args = {
+            'of-list': 'test-fdt1 test-fdt2',
+            'default-dt': 'test-fdt2',
+            'atf-bl31-path': 'bl31.elf',
+            'op-tee-path': 'tee.elf',
+        }
+        test_subdir = os.path.join(self._indir, TEST_FDT_SUBDIR)
+        with self.assertRaises(ValueError) as exc:
+            self._DoReadFileDtb(dts, entry_args=entry_args,
+                                extra_indirs=[test_subdir])[0]
+        return str(exc.exception)
+
+    def testFitSplitElfBadElf(self):
+        """Test a FIT split-elf operation with an invalid ELF file"""
+        TestFunctional._MakeInputFile('bad.elf', tools.GetBytes(100, 100))
+        entry_args = {
+            'of-list': 'test-fdt1 test-fdt2',
+            'default-dt': 'test-fdt2',
+            'atf-bl31-path': 'bad.elf',
+        }
+        test_subdir = os.path.join(self._indir, TEST_FDT_SUBDIR)
+        with self.assertRaises(ValueError) as exc:
+            self._DoReadFileDtb(
+                '221_fit_split_elf.dts',
+                entry_args=entry_args,
+                extra_indirs=[test_subdir])[0]
+        self.assertIn(
+            "Node '/binman/fit': Failed to read ELF file for /binman/fit/images/@atf-SEQ: Magic number does not match",
+            str(exc.exception))
+
+    def testFitSplitElfBadDirective(self):
+        """Test a FIT split-elf invalid fit,xxx directive in an image node"""
+        err = self._check_bad_fit('222_fit_bad_dir.dts')
+        self.assertIn(
+            "Node '/binman/fit': Unknown directive in '/images/atf-1': 'fit,something'",
+            err)
+
+    def testFitSplitElfBadDirectiveConfig(self):
+        """Test a FIT split-elf with invalid fit,xxx directive in config"""
+        err = self._check_bad_fit('223_fit_bad_dir_config.dts')
+        self.assertEqual(
+            "Node '/binman/fit': Unknown directive in '/configurations/config-1': 'fit,config'",
+            err)
+
 
 if __name__ == "__main__":
     unittest.main()
diff --git a/tools/binman/test/221_fit_split_elf.dts b/tools/binman/test/221_fit_split_elf.dts
new file mode 100644
index 0000000000..ec771bd116
--- /dev/null
+++ b/tools/binman/test/221_fit_split_elf.dts
@@ -0,0 +1,67 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	binman {
+		u-boot {
+		};
+		fit {
+			description = "test-desc";
+			#address-cells = <1>;
+			fit,fdt-list = "of-list";
+
+			images {
+				@fdt-SEQ {
+					description = "fdt-NAME.dtb";
+					type = "flat_dt";
+					compression = "none";
+				};
+				atf: @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 {
+					};
+				};
+
+				@tee-SEQ {
+					fit,operation = "split-elf";
+					description = "TEE";
+					type = "tee";
+					arch = "arm64";
+					os = "tee";
+					compression = "none";
+					fit,load;
+					fit,entry;
+					fit,data;
+
+					op-tee {
+					};
+				};
+			};
+
+			configurations {
+				default = "@config-DEFAULT-SEQ";
+				config: @config-SEQ {
+					description = "conf-NAME.dtb";
+					fdt = "fdt-SEQ";
+					fit,loadables;
+				};
+			};
+		};
+
+		u-boot-nodtb {
+		};
+	};
+};
diff --git a/tools/binman/test/222_fit_bad_dir.dts b/tools/binman/test/222_fit_bad_dir.dts
new file mode 100644
index 0000000000..91733c74c4
--- /dev/null
+++ b/tools/binman/test/222_fit_bad_dir.dts
@@ -0,0 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+#include "221_fit_split_elf.dts"
+
+&atf {
+	fit,something = "bad";
+};
diff --git a/tools/binman/test/223_fit_bad_dir_config.dts b/tools/binman/test/223_fit_bad_dir_config.dts
new file mode 100644
index 0000000000..17dae0c5b6
--- /dev/null
+++ b/tools/binman/test/223_fit_bad_dir_config.dts
@@ -0,0 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+#include "221_fit_split_elf.dts"
+
+&config {
+	fit,config = "bad";
+};
-- 
2.35.0.263.gb82422642f-goog

Re: [PATCH 20/24] binman: Support splitting an ELF file into multiple nodes

[Alper Nebi Yasak]

On 08/02/2022 21:50, Simon Glass wrote:
> Some boards need to load an ELF file using the 'loadables' property, but
> the file has segments at different memory addresses. This means that it
> cannot be supplied as a flat binary.
> 
> Allow generating a separate node in the FIT for each segment in the ELF,
> with a different load address for each.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  tools/binman/entries.rst                     | 146 +++++++++++
>  tools/binman/etype/fit.py                    | 259 ++++++++++++++++++-
>  tools/binman/ftest.py                        | 116 +++++++++
>  tools/binman/test/221_fit_split_elf.dts      |  67 +++++
>  tools/binman/test/222_fit_bad_dir.dts        |   9 +
>  tools/binman/test/223_fit_bad_dir_config.dts |   9 +
>  6 files changed, 594 insertions(+), 12 deletions(-)
>  create mode 100644 tools/binman/test/221_fit_split_elf.dts
>  create mode 100644 tools/binman/test/222_fit_bad_dir.dts
>  create mode 100644 tools/binman/test/223_fit_bad_dir_config.dts
> 
> diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
> index d483169712..079fed1a9c 100644
> --- a/tools/binman/entries.rst
> +++ b/tools/binman/entries.rst
> @@ -612,6 +612,9 @@ gen-fdt-nodes
>      Generate FDT nodes as above. This is the default if there is no
>      `fit,operation` property.
>  
> +split-elf
> +    Split an ELF file into a separate node for each segment.
> +
>  Generating nodes from an FDT list (gen-fdt-nodes)
>  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  
> @@ -655,6 +658,149 @@ for each of your two files.
>  Note that if no devicetree files are provided (with '-a of-list' as above)
>  then no nodes will be generated.
>  
> +Generating nodes from an ELF file (split-elf)
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +This uses the node as a template to generate multiple nodes. The following
> +special properties are available:
> +
> +split-elf
> +    Split an ELF file into a separate node for each segment. This uses the
> +    node as a template to generate multiple nodes. The following special
> +    properties are available:
> +
> +    fit,load
> +        Generates a `load = <...>` property with the load address of the
> +        segmnet
> +
> +    fit,entry
> +        Generates a `entry = <...>` property with the entry address of the
> +        ELF. This is only produced for the first entry
> +
> +    fit,data
> +        Generates a `data = <...>` property with the contents of the segment

I think all these should be done by default. I don't see the point of
not setting the properties, or setting them manually to values that will
be the same for multiple nodes.

> +
> +    fit,loadables
> +        Generates a `loadable = <...>` property with a list of the generated
> +        nodes (including all nodes if this operation is used multiple times)

I think this could also be the default behaviour like the above, and
even for configs which have a "loadables" property we could append to it
by default.

Either way, if the "loadables" property exists this needs to append to
it instead of entirely replacing it. (Needs to append to "u-boot", see
comment at example generated config below.)

> +
> +
> +Here is an example showing ATF, TEE and a device tree all combined::

This looks like a better place to write about the template node design
so I'll try here instead of the previous patch. I've always thought they
were ugly but I couldn't think of a good enough design when they were
introduced. Maybe I have now.

> +
> +    fit {
> +        description = "test-desc";
> +        #address-cells = <1>;
> +        fit,fdt-list = "of-list";
> +
> +        images {
> +            u-boot {
> +                description = "U-Boot (64-bit)";
> +                type = "standalone";
> +                os = "U-Boot";
> +                arch = "arm64";
> +                compression = "none";
> +                load = <CONFIG_SYS_TEXT_BASE>;
> +                u-boot-nodtb {
> +                };
> +            };
> +            @fdt-SEQ {
> +                description = "fdt-NAME.dtb";
> +                type = "flat_dt";
> +                compression = "none";
> +            };

Instead of putting these into the images subnode, we could have
images-level subnodes for the operations. For example, instead of the above:

           images@gen-fdt-nodes {
               fdt-list = "of-list";

               fdt {
                   type = "flat_dt";
                   compression = "none";
               };
           };

We can remove the -SEQ if we always append a sequence number, and we can
set "description" to "NAME.dtb" when it's missing, or do the replacement
when it's given. We can go further and use "%s", "%(name)s" or "{name}"
instead of "NAME" for python-ish formatting and likewise for the
sequence number.

> +            @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 {
> +                };
> +            };
> +
> +            @tee-SEQ {
> +                fit,operation = "split-elf";
> +                description = "TEE";
> +                type = "tee";
> +                arch = "arm64";
> +                os = "tee";
> +                compression = "none";
> +                fit,load;
> +                fit,entry;
> +                fit,data;
> +
> +                op-tee {
> +                };
> +            };
> +        };

Similarly, instead of the above two:

           images@split-elf {
               atf {
                   description = "ARM Trusted Firmware";
                   type = "firmware";
                   arch = "arm64";
                   os = "arm-trusted-firmware";
                   compression = "none";

                   atf-bl31 {
                   };
               };

               tee {
                   description = "TEE";
                   type = "tee";
                   arch = "arm64";
                   os = "tee";
                   compression = "none";

                   op-tee {
                   };
               };
           };

The load, entry, data properties could be set automatically without
explicit mention.

> +
> +        configurations {
> +            default = "@config-DEFAULT-SEQ";
> +            @config-SEQ {
> +                description = "conf-NAME.dtb";
> +                fdt = "fdt-SEQ";
> +                firmware = "u-boot";
> +                fit,loadables;
> +            };
> +        };

And instead of the above:

           configurations@gen-fdt-nodes {
               fdt-list = "of-list";

               config {
                   fdt = "fdt";
                   firmware = "u-boot";
               };
           };

Again, we can automatically add a sequence number to "config" node names
and "fdt" property value, and set/replace a description property.

We can collect all loadables from the images@split-elf nodes and create
or append to the "loadables" properties. Or we could specify "atf" and
"fdt" in the loadables and they could be expanded with sequence numbers.

And if the "default" property of "configurations" node (or even the
whole node) is missing, we can make configurations@gen-fdt-nodes set it
to the default-dt.


(Remains to be seen if I'll be annoyed enough to implement this myself.)

> +    };
> +
> +If ATF-BL31 is available, this generates a node for each segment in the
> +ELF file, for example::
> +
> +    images {
> +        atf-1 {
> +            data = <...contents of first segment...>;
> +            data-offset = <0x00000000>;
> +            entry = <0x00040000>;
> +            load = <0x00040000>;
> +            compression = "none";
> +            os = "arm-trusted-firmware";
> +            arch = "arm64";
> +            type = "firmware";
> +            description = "ARM Trusted Firmware";
> +        };
> +        atf-2 {
> +            data = <...contents of second segment...>;
> +            load = <0xff3b0000>;
> +            compression = "none";
> +            os = "arm-trusted-firmware";
> +            arch = "arm64";
> +            type = "firmware";
> +            description = "ARM Trusted Firmware";
> +        };
> +    };
> +
> +The same applies for OP-TEE if that is available.
> +
> +If each binary is not available, the relevant template node (@atf-SEQ or
> +@tee-SEQ) is removed from the output.

I'm running into the fake-file thing with op-tee and getting a "Magic
number does not match" error on consecutive builds as the leftover fake
op-tee isn't an ELF...

> +
> +This also generates a `config-xxx` node for each device tree in `of-list`.
> +Note that the U-Boot build system uses `-a of-list=$(CONFIG_OF_LIST)`
> +so you can use `CONFIG_OF_LIST` to define that list. In this example it is
> +set up for `firefly-rk3399` with a single device tree and the default set
> +with `-a default-dt=$(CONFIG_DEFAULT_DEVICE_TREE)`, so the resulting output
> +is::
> +
> +    configurations {
> +        default = "config-1";
> +        config-1 {
> +            loadables = "atf-1", "atf-2", "atf-3", "tee-1", "tee-2";
> +            description = "rk3399-firefly.dtb";
> +            fdt = "fdt-1";
> +            firmware = "u-boot";
> +        };
> +    };

These loadables/firmware values didn't work on my chromebook_kevin. The
Rockchip generator gets me:

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

instead of the ones above (I didn't pass an op-tee file), and these work
when I hardcode them in the binman definition.

> +
> +U-Boot SPL can then load the firmware (U-Boot proper) and all the loadables
> +(ATF and TEE), then proceed with the boot.
> +
>  
>  
>  Entry: fmap: An entry which contains an Fmap section
> diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
> index 6210deeef7..b2a037c742 100644
> --- a/tools/binman/etype/fit.py
> +++ b/tools/binman/etype/fit.py
> @@ -6,17 +6,20 @@
>  #
>  
>  from collections import defaultdict, OrderedDict
> +import io
>  import libfdt
>  
>  from binman.entry import Entry, EntryArg
> +from binman import elf
>  from dtoc import fdt_util
>  from dtoc.fdt import Fdt
>  from patman import tools
>  
>  # Supported operations, with the fit,operation property
> -OP_GEN_FDT_NODES = range(1)
> +OP_GEN_FDT_NODES, OP_SPLIT_ELF = range(2)
>  OPERATIONS = {
>      'gen-fdt-nodes': OP_GEN_FDT_NODES,
> +    'split-elf': OP_SPLIT_ELF,
>      }
>  
>  class Entry_fit(Entry):
> @@ -111,6 +114,9 @@ class Entry_fit(Entry):
>          Generate FDT nodes as above. This is the default if there is no
>          `fit,operation` property.
>  
> +    split-elf
> +        Split an ELF file into a separate node for each segment.
> +
>      Generating nodes from an FDT list (gen-fdt-nodes)
>      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  
> @@ -153,6 +159,149 @@ class Entry_fit(Entry):
>  
>      Note that if no devicetree files are provided (with '-a of-list' as above)
>      then no nodes will be generated.
> +
> +    Generating nodes from an ELF file (split-elf)
> +    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +    This uses the node as a template to generate multiple nodes. The following
> +    special properties are available:
> +
> +    split-elf
> +        Split an ELF file into a separate node for each segment. This uses the
> +        node as a template to generate multiple nodes. The following special
> +        properties are available:
> +
> +        fit,load
> +            Generates a `load = <...>` property with the load address of the
> +            segmnet
> +
> +        fit,entry
> +            Generates a `entry = <...>` property with the entry address of the
> +            ELF. This is only produced for the first entry
> +
> +        fit,data
> +            Generates a `data = <...>` property with the contents of the segment
> +
> +        fit,loadables
> +            Generates a `loadable = <...>` property with a list of the generated
> +            nodes (including all nodes if this operation is used multiple times)
> +
> +
> +    Here is an example showing ATF, TEE and a device tree all combined::
> +
> +        fit {
> +            description = "test-desc";
> +            #address-cells = <1>;
> +            fit,fdt-list = "of-list";
> +
> +            images {
> +                u-boot {
> +                    description = "U-Boot (64-bit)";
> +                    type = "standalone";
> +                    os = "U-Boot";
> +                    arch = "arm64";
> +                    compression = "none";
> +                    load = <CONFIG_SYS_TEXT_BASE>;
> +                    u-boot-nodtb {
> +                    };
> +                };
> +                @fdt-SEQ {
> +                    description = "fdt-NAME.dtb";
> +                    type = "flat_dt";
> +                    compression = "none";
> +                };
> +                @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 {
> +                    };
> +                };
> +
> +                @tee-SEQ {
> +                    fit,operation = "split-elf";
> +                    description = "TEE";
> +                    type = "tee";
> +                    arch = "arm64";
> +                    os = "tee";
> +                    compression = "none";
> +                    fit,load;
> +                    fit,entry;
> +                    fit,data;
> +
> +                    op-tee {
> +                    };
> +                };
> +            };
> +
> +            configurations {
> +                default = "@config-DEFAULT-SEQ";
> +                @config-SEQ {
> +                    description = "conf-NAME.dtb";
> +                    fdt = "fdt-SEQ";
> +                    firmware = "u-boot";
> +                    fit,loadables;
> +                };
> +            };
> +        };
> +
> +    If ATF-BL31 is available, this generates a node for each segment in the
> +    ELF file, for example::
> +
> +        images {
> +            atf-1 {
> +                data = <...contents of first segment...>;
> +                data-offset = <0x00000000>;
> +                entry = <0x00040000>;
> +                load = <0x00040000>;
> +                compression = "none";
> +                os = "arm-trusted-firmware";
> +                arch = "arm64";
> +                type = "firmware";
> +                description = "ARM Trusted Firmware";
> +            };
> +            atf-2 {
> +                data = <...contents of second segment...>;
> +                load = <0xff3b0000>;
> +                compression = "none";
> +                os = "arm-trusted-firmware";
> +                arch = "arm64";
> +                type = "firmware";
> +                description = "ARM Trusted Firmware";
> +            };
> +        };
> +
> +    The same applies for OP-TEE if that is available.
> +
> +    If each binary is not available, the relevant template node (@atf-SEQ or
> +    @tee-SEQ) is removed from the output.
> +
> +    This also generates a `config-xxx` node for each device tree in `of-list`.
> +    Note that the U-Boot build system uses `-a of-list=$(CONFIG_OF_LIST)`
> +    so you can use `CONFIG_OF_LIST` to define that list. In this example it is
> +    set up for `firefly-rk3399` with a single device tree and the default set
> +    with `-a default-dt=$(CONFIG_DEFAULT_DEVICE_TREE)`, so the resulting output
> +    is::
> +
> +        configurations {
> +            default = "config-1";
> +            config-1 {
> +                loadables = "atf-1", "atf-2", "atf-3", "tee-1", "tee-2";
> +                description = "rk3399-firefly.dtb";
> +                fdt = "fdt-1";
> +                firmware = "u-boot";
> +            };
> +        };
> +
> +    U-Boot SPL can then load the firmware (U-Boot proper) and all the loadables
> +    (ATF and TEE), then proceed with the boot.
>      """
>      def __init__(self, section, etype, node):
>          """
> @@ -182,6 +331,12 @@ class Entry_fit(Entry):
>                                                                    str)])[0]
>          self.mkimage = None
>  
> +        # List of generated split-elf nodes, each a None
> +        self._loadables = []
> +
> +        # /configurations/xxx node used as a template
> +        self._config_node = None
> +
>      def ReadNode(self):
>          self.ReadEntries()
>          super().ReadNode()
> @@ -251,19 +406,21 @@ class Entry_fit(Entry):
>                  in_images: True if this is inside the 'images' node, so that
>                      'data' properties should be generated
>              """
> +            if depth == 1 and not in_images:
> +                self._config_node = subnode
>              if self._fdts:
>                  # Generate nodes for each FDT
>                  for seq, fdt_fname in enumerate(self._fdts):
> -                    node_name = subnode.name[1:].replace('SEQ',
> -                                                         str(seq + 1))
> +                    node_name = subnode.name[1:].replace('SEQ', str(seq + 1))
>                      fname = tools.GetInputFilename(fdt_fname + '.dtb')
>                      with fsw.add_node(node_name):
>                          for pname, prop in subnode.props.items():
> -                            val = prop.bytes.replace(
> -                                b'NAME', tools.ToBytes(fdt_fname))
> -                            val = val.replace(
> -                                b'SEQ', tools.ToBytes(str(seq + 1)))
> -                            fsw.property(pname, val)
> +                            if not pname.startswith('fit,'):
> +                                val = prop.bytes.replace(
> +                                    b'NAME', tools.ToBytes(fdt_fname))
> +                                val = val.replace(
> +                                    b'SEQ', tools.ToBytes(str(seq + 1)))
> +                                fsw.property(pname, val)
>  
>                          # Add data for 'images' nodes (but not 'config')
>                          if depth == 1 and in_images:
> @@ -277,7 +434,18 @@ class Entry_fit(Entry):
>                      else:
>                          self.Raise("Generator node requires 'fit,fdt-list' property")
>  
> -        def _scan_node(subnode, depth, in_images):
> +        def _scan_split_elf(subnode, rel_path):
> +            #data, input_fname, uniq = self.collect_contents_to_file(
> +            entry = Entry.Create(self.section, subnode, etype='section')
> +            entry.ReadNode()
> +            self._fit_sections[rel_path] = entry

I did rebase this on top of dm-pull-8feb22 and my other two patches, and
this series started failing on CheckEntries() complaining about the
template node's entry's content not fitting inside the FIT section. I
didn't really track it down but I guess it's due to adding it to
self._entries here or somewhere else.

> +
> +            # Add this as a dummy node so we know the required position in the
> +            # output FIT. It is replaced later in _BuildInput().
> +            with fsw.add_node(subnode.name):
> +                pass
> +
> +        def _scan_node(subnode, depth, in_images, rel_path):
>              """Generate nodes from a template
>  
>              This creates one node for each member of self._fdts using the
> @@ -291,10 +459,14 @@ class Entry_fit(Entry):
>                  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
> +                rel_path (str): Path of subnode relative to the toplevel 'fit'
> +                    node
>              """
>              oper = self._get_operation(subnode)
>              if oper == OP_GEN_FDT_NODES:
>                  _scan_gen_fdt_nodes(subnode, depth, in_images)
> +            elif oper == OP_SPLIT_ELF:
> +                _scan_split_elf(subnode, rel_path)
>  
>          def _AddNode(base_node, depth, node):
>              """Add a node to the FIT
> @@ -334,7 +506,8 @@ class Entry_fit(Entry):
>                      # fsw.add_node() or _AddNode() for it.
>                      pass
>                  elif self.GetImage().generate and subnode.name.startswith('@'):
> -                    _scan_node(subnode, depth, in_images)
> +                    _scan_node(subnode, depth, in_images,
> +                               f'{rel_path}/{subnode.name}')
>                  else:
>                      with fsw.add_node(subnode.name):
>                          _AddNode(base_node, depth + 1, subnode)
> @@ -383,6 +556,45 @@ class Entry_fit(Entry):
>  
>          return True
>  
> +    def _add_split_elf(self, orig_node, node, data, missing):
> +        """Add nodes for the ELF file, one per group of contiguous segments
> +
> +        The existing placeholder node is replaced
> +
> +        Args:
> +            orig_node (Node): Template node from the binman definition
> +            node (Node): Node to replace (in the FIT being built)
> +            data (bytes): ELF-format data to process (may be empty)
> +        """
> +        # If any pieces are missing, skip this. The missing entries will show
> +        # an error
> +        if not missing:
> +            try:
> +                segments, entry = elf.read_segments(data)
> +            except ValueError as exc:
> +                self.Raise(f'Failed to read ELF file for {orig_node.path}: {str(exc)}')
> +            parent = node.parent
> +            for (seq, start, data) in segments:
> +                node_name = orig_node.name[1:].replace('SEQ', str(seq + 1))
> +                subnode = parent.AddSubnode(node_name) #, before=node)
> +                self._loadables.append(subnode)
> +                for pname, prop in orig_node.props.items():
> +                    if not pname.startswith('fit,'):
> +                        subnode.AddData(pname, prop.bytes)
> +                    elif pname == 'fit,load':
> +                        subnode.AddInt('load', start)
> +                    elif pname == 'fit,entry':
> +                        if not seq:
> +                            subnode.AddInt('entry', entry)
> +                    elif pname == 'fit,data':
> +                        subnode.AddData('data', data)
> +                    elif pname != 'fit,operation':
> +                        self.Raise(
> +                            f"Unknown directive in '{subnode.path}': '{pname}'")
> +
> +        # Delete the template node as it has served its purpose
> +        node.Delete()

Also, with the fit-as-section patches I get an error in SetImagePos()
due to a template subentry in self._entries getting None as its node,
thought it's related to the deletion here but didn't confirm.

> +
>      def _BuildInput(self, fdt):
>          """Finish the FIT by adding the 'data' properties to it
>  
> @@ -393,13 +605,36 @@ class Entry_fit(Entry):
>              New fdt contents (bytes)
>          """
>          for path, section in self._fit_sections.items():
> -            node = fdt.GetNode(path)
>              # Entry_section.ObtainContents() either returns True or
>              # raises an exception.
>              section.ObtainContents()
>              section.Pack(0)
>              data = section.GetData()
> -            node.AddData('data', data)
> +            missing_list = []
> +            section.CheckMissing(missing_list)
> +
> +            node = fdt.GetNode(path)
> +            oper = self._get_operation(section._node)
> +            if oper == OP_GEN_FDT_NODES:
> +                node.AddData('data', data)
> +            elif oper == OP_SPLIT_ELF:
> +                self._add_split_elf(section._node, node, data,
> +                                    bool(missing_list))
> +
> +        # Set up the 'firmware' and 'loadables' properties in all
> +        # 'configurations' nodes, but only if we are generating FDTs. Note that
> +        # self._config_node is set in _scan_gen_fdt_nodes()
> +        node = fdt.GetNode('/configurations')
> +        if self._config_node:
> +            for subnode in node.subnodes:
> +                for pname, prop in self._config_node.props.items():
> +                    if pname == 'fit,loadables':
> +                        subnode.AddStringList(
> +                            'loadables',
> +                            [node.name for node in self._loadables])

As mentioned above, if "loadables" exists this should append to it.

> +                    elif pname.startswith('fit,'):
> +                        self.Raise(
> +                            f"Unknown directive in '{subnode.path}': '{pname}'")
>  
>          fdt.Sync(auto_resize=True)
>          data = fdt.GetContents()
> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> index 5a0dc70ed9..dbaf412e9d 100644
> --- a/tools/binman/ftest.py
> +++ b/tools/binman/ftest.py
> @@ -197,6 +197,13 @@ class TestFunctional(unittest.TestCase):
>  
>          TestFunctional._MakeInputFile('env.txt', ENV_DATA)
>  
> +        # ELF file with two sections in different parts of memory, used for both
> +        # ATF and OP_TEE
> +        TestFunctional._MakeInputFile('bl31.elf',
> +            tools.ReadFile(cls.ElfTestFile('elf_sections')))
> +        TestFunctional._MakeInputFile('tee.elf',
> +            tools.ReadFile(cls.ElfTestFile('elf_sections')))
> +
>          cls.have_lz4 = comp_util.HAVE_LZ4
>  
>      @classmethod
> @@ -5125,6 +5132,115 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
>          self.assertIn("Node '/binman/fit': Unknown operation 'unknown'",
>                        str(exc.exception))
>  
> +    def testFitSplitElf(self):
> +        """Test an image with an FIT with an split-elf operation"""
> +        entry_args = {
> +            'of-list': 'test-fdt1 test-fdt2',
> +            'default-dt': 'test-fdt2',
> +            'atf-bl31-path': 'bl31.elf',
> +            'op-tee-path': 'tee.elf',
> +        }
> +        test_subdir = os.path.join(self._indir, TEST_FDT_SUBDIR)
> +        data = self._DoReadFileDtb(
> +            '221_fit_split_elf.dts',
> +            entry_args=entry_args,
> +            extra_indirs=[test_subdir])[0]
> +
> +        self.assertEqual(U_BOOT_NODTB_DATA, data[-len(U_BOOT_NODTB_DATA):])
> +        fit_data = data[len(U_BOOT_DATA):-len(U_BOOT_NODTB_DATA)]
> +
> +        base_keys = {'description', 'type', 'arch', 'os', 'compression',
> +                     'data', 'load'}
> +        dtb = fdt.Fdt.FromData(fit_data)
> +        dtb.Scan()
> +
> +        elf_data = tools.ReadFile(os.path.join(self._indir, 'bl31.elf'))
> +        segments, entry = elf.read_segments(elf_data)
> +
> +        # We assume there are two segments
> +        self.assertEquals(2, len(segments))
> +
> +        atf1 = dtb.GetNode('/images/atf-1')
> +        _, start, data = segments[0]
> +        self.assertEqual(base_keys | {'entry'}, atf1.props.keys())
> +        self.assertEqual(entry,
> +                         fdt_util.fdt32_to_cpu(atf1.props['entry'].value))
> +        self.assertEqual(start,
> +                         fdt_util.fdt32_to_cpu(atf1.props['load'].value))
> +        self.assertEqual(data, atf1.props['data'].bytes)
> +
> +        atf2 = dtb.GetNode('/images/atf-2')
> +        self.assertEqual(base_keys, atf2.props.keys())
> +        _, start, data = segments[1]
> +        self.assertEqual(start,
> +                         fdt_util.fdt32_to_cpu(atf2.props['load'].value))
> +        self.assertEqual(data, atf2.props['data'].bytes)
> +
> +        conf = dtb.GetNode('/configurations')
> +        self.assertEqual({'default'}, conf.props.keys())
> +
> +        for subnode in conf.subnodes:
> +            self.assertEqual({'description', 'fdt', 'loadables'},
> +                             subnode.props.keys())
> +            self.assertEqual(
> +                ['atf-1', 'atf-2', 'tee-1', 'tee-2'],
> +                fdt_util.GetStringList(subnode, 'loadables'))
> +
> +    def _check_bad_fit(self, dts):
> +        """Check a bad FIT
> +
> +        This runs with the given dts and returns the assertion raised
> +
> +        Args:
> +            dts (str): dts filename to use
> +
> +        Returns:
> +            str: Assertion string raised
> +        """
> +        entry_args = {
> +            'of-list': 'test-fdt1 test-fdt2',
> +            'default-dt': 'test-fdt2',
> +            'atf-bl31-path': 'bl31.elf',
> +            'op-tee-path': 'tee.elf',
> +        }
> +        test_subdir = os.path.join(self._indir, TEST_FDT_SUBDIR)
> +        with self.assertRaises(ValueError) as exc:
> +            self._DoReadFileDtb(dts, entry_args=entry_args,
> +                                extra_indirs=[test_subdir])[0]
> +        return str(exc.exception)
> +
> +    def testFitSplitElfBadElf(self):
> +        """Test a FIT split-elf operation with an invalid ELF file"""
> +        TestFunctional._MakeInputFile('bad.elf', tools.GetBytes(100, 100))
> +        entry_args = {
> +            'of-list': 'test-fdt1 test-fdt2',
> +            'default-dt': 'test-fdt2',
> +            'atf-bl31-path': 'bad.elf',

This test fails for me with the fit-as-section patches but adding
'op-tee-path' here like the ones above makes it succeed. Though, you
might have meant to test a missing op-tee here in addition to the bad.elf.

Could use an independent test that checks a missing op-tee doesn't
result in an error, as it should just omit the nodes in the FIT.

> +        }
> +        test_subdir = os.path.join(self._indir, TEST_FDT_SUBDIR)
> +        with self.assertRaises(ValueError) as exc:
> +            self._DoReadFileDtb(
> +                '221_fit_split_elf.dts',
> +                entry_args=entry_args,
> +                extra_indirs=[test_subdir])[0]
> +        self.assertIn(
> +            "Node '/binman/fit': Failed to read ELF file for /binman/fit/images/@atf-SEQ: Magic number does not match",
> +            str(exc.exception))
> +
> +    def testFitSplitElfBadDirective(self):
> +        """Test a FIT split-elf invalid fit,xxx directive in an image node"""
> +        err = self._check_bad_fit('222_fit_bad_dir.dts')
> +        self.assertIn(
> +            "Node '/binman/fit': Unknown directive in '/images/atf-1': 'fit,something'",
> +            err)
> +
> +    def testFitSplitElfBadDirectiveConfig(self):
> +        """Test a FIT split-elf with invalid fit,xxx directive in config"""
> +        err = self._check_bad_fit('223_fit_bad_dir_config.dts')
> +        self.assertEqual(
> +            "Node '/binman/fit': Unknown directive in '/configurations/config-1': 'fit,config'",
> +            err)
> +
>  
>  if __name__ == "__main__":
>      unittest.main()
> diff --git a/tools/binman/test/221_fit_split_elf.dts b/tools/binman/test/221_fit_split_elf.dts
> new file mode 100644
> index 0000000000..ec771bd116
> --- /dev/null
> +++ b/tools/binman/test/221_fit_split_elf.dts
> @@ -0,0 +1,67 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +/dts-v1/;
> +
> +/ {
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +
> +	binman {
> +		u-boot {
> +		};
> +		fit {
> +			description = "test-desc";
> +			#address-cells = <1>;
> +			fit,fdt-list = "of-list";
> +
> +			images {
> +				@fdt-SEQ {
> +					description = "fdt-NAME.dtb";
> +					type = "flat_dt";
> +					compression = "none";
> +				};
> +				atf: @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 {
> +					};
> +				};
> +
> +				@tee-SEQ {
> +					fit,operation = "split-elf";
> +					description = "TEE";
> +					type = "tee";
> +					arch = "arm64";
> +					os = "tee";
> +					compression = "none";
> +					fit,load;
> +					fit,entry;
> +					fit,data;
> +
> +					op-tee {
> +					};
> +				};
> +			};
> +
> +			configurations {
> +				default = "@config-DEFAULT-SEQ";
> +				config: @config-SEQ {
> +					description = "conf-NAME.dtb";
> +					fdt = "fdt-SEQ";
> +					fit,loadables;
> +				};
> +			};
> +		};
> +
> +		u-boot-nodtb {
> +		};
> +	};
> +};
> diff --git a/tools/binman/test/222_fit_bad_dir.dts b/tools/binman/test/222_fit_bad_dir.dts
> new file mode 100644
> index 0000000000..91733c74c4
> --- /dev/null
> +++ b/tools/binman/test/222_fit_bad_dir.dts
> @@ -0,0 +1,9 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +/dts-v1/;
> +
> +#include "221_fit_split_elf.dts"
> +
> +&atf {
> +	fit,something = "bad";
> +};
> diff --git a/tools/binman/test/223_fit_bad_dir_config.dts b/tools/binman/test/223_fit_bad_dir_config.dts
> new file mode 100644
> index 0000000000..17dae0c5b6
> --- /dev/null
> +++ b/tools/binman/test/223_fit_bad_dir_config.dts
> @@ -0,0 +1,9 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +/dts-v1/;
> +
> +#include "221_fit_split_elf.dts"
> +
> +&config {
> +	fit,config = "bad";
> +};

Re: [PATCH 20/24] binman: Support splitting an ELF file into multiple nodes

[Simon Glass]

Hi Alper,

On Tue, 15 Feb 2022 at 04:53, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> On 08/02/2022 21:50, Simon Glass wrote:
> > Some boards need to load an ELF file using the 'loadables' property, but
> > the file has segments at different memory addresses. This means that it
> > cannot be supplied as a flat binary.
> >
> > Allow generating a separate node in the FIT for each segment in the ELF,
> > with a different load address for each.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  tools/binman/entries.rst                     | 146 +++++++++++
> >  tools/binman/etype/fit.py                    | 259 ++++++++++++++++++-
> >  tools/binman/ftest.py                        | 116 +++++++++
> >  tools/binman/test/221_fit_split_elf.dts      |  67 +++++
> >  tools/binman/test/222_fit_bad_dir.dts        |   9 +
> >  tools/binman/test/223_fit_bad_dir_config.dts |   9 +
> >  6 files changed, 594 insertions(+), 12 deletions(-)
> >  create mode 100644 tools/binman/test/221_fit_split_elf.dts
> >  create mode 100644 tools/binman/test/222_fit_bad_dir.dts
> >  create mode 100644 tools/binman/test/223_fit_bad_dir_config.dts
> >
> > diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
> > index d483169712..079fed1a9c 100644
> > --- a/tools/binman/entries.rst
> > +++ b/tools/binman/entries.rst
> > @@ -612,6 +612,9 @@ gen-fdt-nodes
> >      Generate FDT nodes as above. This is the default if there is no
> >      `fit,operation` property.
> >
> > +split-elf
> > +    Split an ELF file into a separate node for each segment.
> > +
> >  Generating nodes from an FDT list (gen-fdt-nodes)
> >  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > @@ -655,6 +658,149 @@ for each of your two files.
> >  Note that if no devicetree files are provided (with '-a of-list' as above)
> >  then no nodes will be generated.
> >
> > +Generating nodes from an ELF file (split-elf)
> > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > +
> > +This uses the node as a template to generate multiple nodes. The following
> > +special properties are available:
> > +
> > +split-elf
> > +    Split an ELF file into a separate node for each segment. This uses the
> > +    node as a template to generate multiple nodes. The following special
> > +    properties are available:
> > +
> > +    fit,load
> > +        Generates a `load = <...>` property with the load address of the
> > +        segmnet
> > +
> > +    fit,entry
> > +        Generates a `entry = <...>` property with the entry address of the
> > +        ELF. This is only produced for the first entry
> > +
> > +    fit,data
> > +        Generates a `data = <...>` property with the contents of the segment
>
> I think all these should be done by default. I don't see the point of
> not setting the properties, or setting them manually to values that will
> be the same for multiple nodes.

My intent is to make things discoverable and obvious, so that magic
processing is explicit.
>
> > +
> > +    fit,loadables
> > +        Generates a `loadable = <...>` property with a list of the generated
> > +        nodes (including all nodes if this operation is used multiple times)
>
> I think this could also be the default behaviour like the above, and
> even for configs which have a "loadables" property we could append to it
> by default.
>
> Either way, if the "loadables" property exists this needs to append to
> it instead of entirely replacing it. (Needs to append to "u-boot", see
> comment at example generated config below.)

Yes, makes sense.

>
> > +
> > +
> > +Here is an example showing ATF, TEE and a device tree all combined::
>
> This looks like a better place to write about the template node design
> so I'll try here instead of the previous patch. I've always thought they
> were ugly but I couldn't think of a good enough design when they were
> introduced. Maybe I have now.
>
> > +
> > +    fit {
> > +        description = "test-desc";
> > +        #address-cells = <1>;
> > +        fit,fdt-list = "of-list";
> > +
> > +        images {
> > +            u-boot {
> > +                description = "U-Boot (64-bit)";
> > +                type = "standalone";
> > +                os = "U-Boot";
> > +                arch = "arm64";
> > +                compression = "none";
> > +                load = <CONFIG_SYS_TEXT_BASE>;
> > +                u-boot-nodtb {
> > +                };
> > +            };
> > +            @fdt-SEQ {
> > +                description = "fdt-NAME.dtb";
> > +                type = "flat_dt";
> > +                compression = "none";
> > +            };
>
> Instead of putting these into the images subnode, we could have
> images-level subnodes for the operations. For example, instead of the above:
>
>            images@gen-fdt-nodes {
>                fdt-list = "of-list";
>
>                fdt {
>                    type = "flat_dt";
>                    compression = "none";
>                };
>            };

What does that mean, though? I presume it creates a second images {}
node, which is fine as dtc will merge them. But what about ordering?

I certainly prefer this in terms of elegance. I'm just not convinced
that people will understand it as well.

>
> We can remove the -SEQ if we always append a sequence number, and we can
> set "description" to "NAME.dtb" when it's missing, or do the replacement
> when it's given. We can go further and use "%s", "%(name)s" or "{name}"
> instead of "NAME" for python-ish formatting and likewise for the
> sequence number.

Yes, but again this adds more magic. For the Python formatting, we
still need to restrict what is put in there - e.g. we cannot just eval
an arbitrary varaible.

>
> > +            @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 {
> > +                };
> > +            };
> > +
> > +            @tee-SEQ {
> > +                fit,operation = "split-elf";
> > +                description = "TEE";
> > +                type = "tee";
> > +                arch = "arm64";
> > +                os = "tee";
> > +                compression = "none";
> > +                fit,load;
> > +                fit,entry;
> > +                fit,data;
> > +
> > +                op-tee {
> > +                };
> > +            };
> > +        };
>
> Similarly, instead of the above two:
>
>            images@split-elf {
>                atf {
>                    description = "ARM Trusted Firmware";
>                    type = "firmware";
>                    arch = "arm64";
>                    os = "arm-trusted-firmware";
>                    compression = "none";
>
>                    atf-bl31 {
>                    };
>                };
>
>                tee {
>                    description = "TEE";
>                    type = "tee";
>                    arch = "arm64";
>                    os = "tee";
>                    compression = "none";
>
>                    op-tee {
>                    };
>                };
>            };
>
> The load, entry, data properties could be set automatically without
> explicit mention.
>
> > +
> > +        configurations {
> > +            default = "@config-DEFAULT-SEQ";
> > +            @config-SEQ {
> > +                description = "conf-NAME.dtb";
> > +                fdt = "fdt-SEQ";
> > +                firmware = "u-boot";
> > +                fit,loadables;
> > +            };
> > +        };
>
> And instead of the above:
>
>            configurations@gen-fdt-nodes {
>                fdt-list = "of-list";
>
>                config {
>                    fdt = "fdt";
>                    firmware = "u-boot";
>                };
>            };
>
> Again, we can automatically add a sequence number to "config" node names
> and "fdt" property value, and set/replace a description property.
>
> We can collect all loadables from the images@split-elf nodes and create
> or append to the "loadables" properties. Or we could specify "atf" and
> "fdt" in the loadables and they could be expanded with sequence numbers.
>
> And if the "default" property of "configurations" node (or even the
> whole node) is missing, we can make configurations@gen-fdt-nodes set it
> to the default-dt.
>
>
> (Remains to be seen if I'll be annoyed enough to implement this myself.)

We should talk about this some more though. I'm a bit worried it will
get complaints about too much magic. I am trying to make it obvious
that nodes get generated in certain places.

>
> > +    };
> > +
> > +If ATF-BL31 is available, this generates a node for each segment in the
> > +ELF file, for example::
> > +
> > +    images {
> > +        atf-1 {
> > +            data = <...contents of first segment...>;
> > +            data-offset = <0x00000000>;
> > +            entry = <0x00040000>;
> > +            load = <0x00040000>;
> > +            compression = "none";
> > +            os = "arm-trusted-firmware";
> > +            arch = "arm64";
> > +            type = "firmware";
> > +            description = "ARM Trusted Firmware";
> > +        };
> > +        atf-2 {
> > +            data = <...contents of second segment...>;
> > +            load = <0xff3b0000>;
> > +            compression = "none";
> > +            os = "arm-trusted-firmware";
> > +            arch = "arm64";
> > +            type = "firmware";
> > +            description = "ARM Trusted Firmware";
> > +        };
> > +    };
> > +
> > +The same applies for OP-TEE if that is available.
> > +
> > +If each binary is not available, the relevant template node (@atf-SEQ or
> > +@tee-SEQ) is removed from the output.
>
> I'm running into the fake-file thing with op-tee and getting a "Magic
> number does not match" error on consecutive builds as the leftover fake
> op-tee isn't an ELF...

Yes, I added a TODO for that. I think the take files should go in a
binman-fake subdir, removed on startup.

>
> > +
> > +This also generates a `config-xxx` node for each device tree in `of-list`.
> > +Note that the U-Boot build system uses `-a of-list=$(CONFIG_OF_LIST)`
> > +so you can use `CONFIG_OF_LIST` to define that list. In this example it is
> > +set up for `firefly-rk3399` with a single device tree and the default set
> > +with `-a default-dt=$(CONFIG_DEFAULT_DEVICE_TREE)`, so the resulting output
> > +is::
> > +
> > +    configurations {
> > +        default = "config-1";
> > +        config-1 {
> > +            loadables = "atf-1", "atf-2", "atf-3", "tee-1", "tee-2";
> > +            description = "rk3399-firefly.dtb";
> > +            fdt = "fdt-1";
> > +            firmware = "u-boot";
> > +        };
> > +    };
>
> These loadables/firmware values didn't work on my chromebook_kevin. The
> Rockchip generator gets me:
>
>                firmware = "atf-1";
>                loadables = "u-boot", "atf-2", "atf-3";
>
> instead of the ones above (I didn't pass an op-tee file), and these work
> when I hardcode them in the binman definition.

I believe this is fixed now.
[..]

> > -        def _scan_node(subnode, depth, in_images):
> > +        def _scan_split_elf(subnode, rel_path):
> > +            #data, input_fname, uniq = self.collect_contents_to_file(
> > +            entry = Entry.Create(self.section, subnode, etype='section')
> > +            entry.ReadNode()
> > +            self._fit_sections[rel_path] = entry
>
> I did rebase this on top of dm-pull-8feb22 and my other two patches, and
> this series started failing on CheckEntries() complaining about the
> template node's entry's content not fitting inside the FIT section. I
> didn't really track it down but I guess it's due to adding it to
> self._entries here or somewhere else.

Yes I ended up creating a separate list of entries. See what you think.

[..]

> > +    def _add_split_elf(self, orig_node, node, data, missing):
> > +        """Add nodes for the ELF file, one per group of contiguous segments
> > +
> > +        The existing placeholder node is replaced
> > +
> > +        Args:
> > +            orig_node (Node): Template node from the binman definition
> > +            node (Node): Node to replace (in the FIT being built)
> > +            data (bytes): ELF-format data to process (may be empty)
> > +        """
> > +        # If any pieces are missing, skip this. The missing entries will show
> > +        # an error
> > +        if not missing:
> > +            try:
> > +                segments, entry = elf.read_segments(data)
> > +            except ValueError as exc:
> > +                self.Raise(f'Failed to read ELF file for {orig_node.path}: {str(exc)}')
> > +            parent = node.parent
> > +            for (seq, start, data) in segments:
> > +                node_name = orig_node.name[1:].replace('SEQ', str(seq + 1))
> > +                subnode = parent.AddSubnode(node_name) #, before=node)
> > +                self._loadables.append(subnode)
> > +                for pname, prop in orig_node.props.items():
> > +                    if not pname.startswith('fit,'):
> > +                        subnode.AddData(pname, prop.bytes)
> > +                    elif pname == 'fit,load':
> > +                        subnode.AddInt('load', start)
> > +                    elif pname == 'fit,entry':
> > +                        if not seq:
> > +                            subnode.AddInt('entry', entry)
> > +                    elif pname == 'fit,data':
> > +                        subnode.AddData('data', data)
> > +                    elif pname != 'fit,operation':
> > +                        self.Raise(
> > +                            f"Unknown directive in '{subnode.path}': '{pname}'")
> > +
> > +        # Delete the template node as it has served its purpose
> > +        node.Delete()
>
> Also, with the fit-as-section patches I get an error in SetImagePos()
> due to a template subentry in self._entries getting None as its node,
> thought it's related to the deletion here but didn't confirm.

Yes it cannot be deleted twice. Fixed in latest series.

>
> > +
> >      def _BuildInput(self, fdt):
> >          """Finish the FIT by adding the 'data' properties to it
> >
> > @@ -393,13 +605,36 @@ class Entry_fit(Entry):
> >              New fdt contents (bytes)
> >          """
> >          for path, section in self._fit_sections.items():
> > -            node = fdt.GetNode(path)
> >              # Entry_section.ObtainContents() either returns True or
> >              # raises an exception.
> >              section.ObtainContents()
> >              section.Pack(0)
> >              data = section.GetData()
> > -            node.AddData('data', data)
> > +            missing_list = []
> > +            section.CheckMissing(missing_list)
> > +
> > +            node = fdt.GetNode(path)
> > +            oper = self._get_operation(section._node)
> > +            if oper == OP_GEN_FDT_NODES:
> > +                node.AddData('data', data)
> > +            elif oper == OP_SPLIT_ELF:
> > +                self._add_split_elf(section._node, node, data,
> > +                                    bool(missing_list))
> > +
> > +        # Set up the 'firmware' and 'loadables' properties in all
> > +        # 'configurations' nodes, but only if we are generating FDTs. Note that
> > +        # self._config_node is set in _scan_gen_fdt_nodes()
> > +        node = fdt.GetNode('/configurations')
> > +        if self._config_node:
> > +            for subnode in node.subnodes:
> > +                for pname, prop in self._config_node.props.items():
> > +                    if pname == 'fit,loadables':
> > +                        subnode.AddStringList(
> > +                            'loadables',
> > +                            [node.name for node in self._loadables])
>
> As mentioned above, if "loadables" exists this should append to it.

I agree with that, but have not done it yet.

[..]

> > +    def testFitSplitElfBadElf(self):
> > +        """Test a FIT split-elf operation with an invalid ELF file"""
> > +        TestFunctional._MakeInputFile('bad.elf', tools.GetBytes(100, 100))
> > +        entry_args = {
> > +            'of-list': 'test-fdt1 test-fdt2',
> > +            'default-dt': 'test-fdt2',
> > +            'atf-bl31-path': 'bad.elf',
>
> This test fails for me with the fit-as-section patches but adding
> 'op-tee-path' here like the ones above makes it succeed. Though, you
> might have meant to test a missing op-tee here in addition to the bad.elf.
>
> Could use an independent test that checks a missing op-tee doesn't
> result in an error, as it should just omit the nodes in the FIT.

Should be fixed with new series.

[..]

Thanks for all the comments and ideas. I think this all need a little
more thought...

Regards,
Simon

Re: [PATCH 20/24] binman: Support splitting an ELF file into multiple nodes

[Alper Nebi Yasak]

On 24/02/2022 01:59, Simon Glass wrote:
> On Tue, 15 Feb 2022 at 04:53, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>> On 08/02/2022 21:50, Simon Glass wrote:
>>> +    fit,load
>>> +        Generates a `load = <...>` property with the load address of the
>>> +        segmnet
>>> +
>>> +    fit,entry
>>> +        Generates a `entry = <...>` property with the entry address of the
>>> +        ELF. This is only produced for the first entry
>>> +
>>> +    fit,data
>>> +        Generates a `data = <...>` property with the contents of the segment
>>
>> I think all these should be done by default. I don't see the point of
>> not setting the properties, or setting them manually to values that will
>> be the same for multiple nodes.
> 
> My intent is to make things discoverable and obvious, so that magic
> processing is explicit.

OK then.

>> Instead of putting these into the images subnode, we could have
>> images-level subnodes for the operations. For example, instead of the above:
>>
>>            images@gen-fdt-nodes {
>>                fdt-list = "of-list";
>>
>>                fdt {
>>                    type = "flat_dt";
>>                    compression = "none";
>>                };
>>            };
> 
> What does that mean, though? I presume it creates a second images {}
> node, which is fine as dtc will merge them. But what about ordering?

I imagined images@oper, configurations@oper would be binman-only control
nodes that generate and append arbitrary nodes/entries inside whatever
node name precedes the @oper. The meaning of what's inside the @oper
nodes and what's generated would be solely defined by the operation.

Honestly, I didn't think of ordering because I assumed it didn't matter
inside FIT. It's a shortcoming of this design if it's important.

> I certainly prefer this in terms of elegance. I'm just not convinced
> that people will understand it as well.
> 
>>
>> We can remove the -SEQ if we always append a sequence number, and we can
>> set "description" to "NAME.dtb" when it's missing, or do the replacement
>> when it's given. We can go further and use "%s", "%(name)s" or "{name}"
>> instead of "NAME" for python-ish formatting and likewise for the
>> sequence number.
> 
> Yes, but again this adds more magic. For the Python formatting, we
> still need to restrict what is put in there - e.g. we cannot just eval
> an arbitrary varaible.

Python only formats with what you give it (except for f-strings):

    >>> val = "test"
    >>> vals = {"name": "rk3399-gru-kevin", "seq": 1}

    >>> "conf-%(seq)d: %(name)s.dtb" % vals
    'conf-1: rk3399-gru-kevin.dtb'

    >>> "%(val)s" % vals
    KeyError: 'val'

    >>> "conf-{seq}: {name}.dtb".format(**vals)
    'conf-1: rk3399-gru-kevin.dtb'

    >>> "{val}".format(**vals)
    KeyError: 'val'

But neither format style can go in a node name, so if you want the
sequence number explicit in those I guess we're stuck with -SEQ and
everything consistent with it.

>>> [...]
> 
> We should talk about this some more though. I'm a bit worried it will
> get complaints about too much magic. I am trying to make it obvious
> that nodes get generated in certain places.

OK. I know I'm too biased towards magic, so won't insist much on those
parts.

> [...]
> 
> Thanks for all the comments and ideas. I think this all need a little
> more thought...

I'll try replying to the v2 for the things changed in v2.

Re: [PATCH 20/24] binman: Support splitting an ELF file into multiple nodes

[Simon Glass]

Hi Alper,

On Thu, 3 Mar 2022 at 14:14, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> On 24/02/2022 01:59, Simon Glass wrote:
> > On Tue, 15 Feb 2022 at 04:53, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
> >> On 08/02/2022 21:50, Simon Glass wrote:
> >>> +    fit,load
> >>> +        Generates a `load = <...>` property with the load address of the
> >>> +        segmnet
> >>> +
> >>> +    fit,entry
> >>> +        Generates a `entry = <...>` property with the entry address of the
> >>> +        ELF. This is only produced for the first entry
> >>> +
> >>> +    fit,data
> >>> +        Generates a `data = <...>` property with the contents of the segment
> >>
> >> I think all these should be done by default. I don't see the point of
> >> not setting the properties, or setting them manually to values that will
> >> be the same for multiple nodes.
> >
> > My intent is to make things discoverable and obvious, so that magic
> > processing is explicit.
>
> OK then.
>
> >> Instead of putting these into the images subnode, we could have
> >> images-level subnodes for the operations. For example, instead of the above:
> >>
> >>            images@gen-fdt-nodes {
> >>                fdt-list = "of-list";
> >>
> >>                fdt {
> >>                    type = "flat_dt";
> >>                    compression = "none";
> >>                };
> >>            };
> >
> > What does that mean, though? I presume it creates a second images {}
> > node, which is fine as dtc will merge them. But what about ordering?
>
> I imagined images@oper, configurations@oper would be binman-only control
> nodes that generate and append arbitrary nodes/entries inside whatever
> node name precedes the @oper. The meaning of what's inside the @oper
> nodes and what's generated would be solely defined by the operation.
>
> Honestly, I didn't think of ordering because I assumed it didn't matter
> inside FIT. It's a shortcoming of this design if it's important.

It doesn't matter for functionality but it is much nicer for the user
to have nodes and properties in a sensible order. That was one
motivation for the v2 series, since in v1 the node order was a mess.

>
> > I certainly prefer this in terms of elegance. I'm just not convinced
> > that people will understand it as well.
> >
> >>
> >> We can remove the -SEQ if we always append a sequence number, and we can
> >> set "description" to "NAME.dtb" when it's missing, or do the replacement
> >> when it's given. We can go further and use "%s", "%(name)s" or "{name}"
> >> instead of "NAME" for python-ish formatting and likewise for the
> >> sequence number.
> >
> > Yes, but again this adds more magic. For the Python formatting, we
> > still need to restrict what is put in there - e.g. we cannot just eval
> > an arbitrary varaible.
>
> Python only formats with what you give it (except for f-strings):
>
>     >>> val = "test"
>     >>> vals = {"name": "rk3399-gru-kevin", "seq": 1}
>
>     >>> "conf-%(seq)d: %(name)s.dtb" % vals
>     'conf-1: rk3399-gru-kevin.dtb'
>
>     >>> "%(val)s" % vals
>     KeyError: 'val'
>
>     >>> "conf-{seq}: {name}.dtb".format(**vals)
>     'conf-1: rk3399-gru-kevin.dtb'
>
>     >>> "{val}".format(**vals)
>     KeyError: 'val'
>
> But neither format style can go in a node name, so if you want the
> sequence number explicit in those I guess we're stuck with -SEQ and
> everything consistent with it.

Hmm yes that's a point.

>
> >>> [...]
> >
> > We should talk about this some more though. I'm a bit worried it will
> > get complaints about too much magic. I am trying to make it obvious
> > that nodes get generated in certain places.
>
> OK. I know I'm too biased towards magic, so won't insist much on those
> parts.
>
> > [...]
> >
> > Thanks for all the comments and ideas. I think this all need a little
> > more thought...
>
> I'll try replying to the v2 for the things changed in v2.

OK ta.

Regards,
Simon

[PATCH 21/24] rockchip: Include binman script in 64-bit boards

[Simon Glass]

Include the rockchip-u-boot.dtsi file with 64-bit boards and enable binman
so that these boards can also use it, rather than using special Makefile
rules and scripts.

This does not change the Makefile nor remove any scripts, but sets it up
so that this is possible.

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

 arch/arm/dts/px30-u-boot.dtsi              | 2 ++
 arch/arm/dts/rk3308-u-boot.dtsi            | 2 ++
 arch/arm/dts/rk3326-odroid-go2-u-boot.dtsi | 2 ++
 arch/arm/dts/rk3328-u-boot.dtsi            | 2 ++
 arch/arm/dts/rk3368-u-boot.dtsi            | 1 +
 arch/arm/dts/rk3399-u-boot.dtsi            | 5 +++--
 arch/arm/dts/rk3568-u-boot.dtsi            | 2 ++
 arch/arm/mach-rockchip/Kconfig             | 6 ++++++
 8 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/arch/arm/dts/px30-u-boot.dtsi b/arch/arm/dts/px30-u-boot.dtsi
index f102b2aef4..462eaf68f8 100644
--- a/arch/arm/dts/px30-u-boot.dtsi
+++ b/arch/arm/dts/px30-u-boot.dtsi
@@ -3,6 +3,8 @@
  * (C) Copyright 2019 Rockchip Electronics Co., Ltd
  */
 
+#include "rockchip-u-boot.dtsi"
+
 / {
 	aliases {
 		mmc0 = &emmc;
diff --git a/arch/arm/dts/rk3308-u-boot.dtsi b/arch/arm/dts/rk3308-u-boot.dtsi
index 4bfad31fba..ab5bfc2ce9 100644
--- a/arch/arm/dts/rk3308-u-boot.dtsi
+++ b/arch/arm/dts/rk3308-u-boot.dtsi
@@ -3,6 +3,8 @@
  *(C) Copyright 2019 Rockchip Electronics Co., Ltd
  */
 
+#include "rockchip-u-boot.dtsi"
+
 / {
 	aliases {
 		mmc0 = &emmc;
diff --git a/arch/arm/dts/rk3326-odroid-go2-u-boot.dtsi b/arch/arm/dts/rk3326-odroid-go2-u-boot.dtsi
index 95f2652494..16c33735eb 100644
--- a/arch/arm/dts/rk3326-odroid-go2-u-boot.dtsi
+++ b/arch/arm/dts/rk3326-odroid-go2-u-boot.dtsi
@@ -3,6 +3,8 @@
  * Copyright (c) 2020 Theobroma Systems Design und Consulting GmbH
  */
 
+#include "rockchip-u-boot.dtsi"
+
 / {
 	chosen {
 		u-boot,spl-boot-order = &sdmmc;
diff --git a/arch/arm/dts/rk3328-u-boot.dtsi b/arch/arm/dts/rk3328-u-boot.dtsi
index 1633558264..d4a7540a92 100644
--- a/arch/arm/dts/rk3328-u-boot.dtsi
+++ b/arch/arm/dts/rk3328-u-boot.dtsi
@@ -3,6 +3,8 @@
  * (C) Copyright 2019 Rockchip Electronics Co., Ltd
  */
 
+#include "rockchip-u-boot.dtsi"
+
 / {
 	aliases {
 		mmc0 = &emmc;
diff --git a/arch/arm/dts/rk3368-u-boot.dtsi b/arch/arm/dts/rk3368-u-boot.dtsi
index 2767c2678d..b37da4e851 100644
--- a/arch/arm/dts/rk3368-u-boot.dtsi
+++ b/arch/arm/dts/rk3368-u-boot.dtsi
@@ -3,6 +3,7 @@
  * Copyright (c) 2020 Theobroma Systems Design und Consulting GmbH
  */
 
+#include "rockchip-u-boot.dtsi"
 #include <dt-bindings/memory/rk3368-dmc.h>
 
 / {
diff --git a/arch/arm/dts/rk3399-u-boot.dtsi b/arch/arm/dts/rk3399-u-boot.dtsi
index 716b9a433a..fd4150e3f0 100644
--- a/arch/arm/dts/rk3399-u-boot.dtsi
+++ b/arch/arm/dts/rk3399-u-boot.dtsi
@@ -60,8 +60,9 @@
 
 };
 
-#ifdef CONFIG_ROCKCHIP_SPI_IMAGE
 &binman {
+	multiple-images;
+#ifdef CONFIG_ROCKCHIP_SPI_IMAGE
 	rom {
 		filename = "u-boot.rom";
 		size = <0x400000>;
@@ -81,8 +82,8 @@
 		fdtmap {
 		};
 	};
-};
 #endif
+};
 
 &cru {
 	u-boot,dm-pre-reloc;
diff --git a/arch/arm/dts/rk3568-u-boot.dtsi b/arch/arm/dts/rk3568-u-boot.dtsi
index 5a80dda275..fa9b6ae23b 100644
--- a/arch/arm/dts/rk3568-u-boot.dtsi
+++ b/arch/arm/dts/rk3568-u-boot.dtsi
@@ -3,6 +3,8 @@
  * (C) Copyright 2021 Rockchip Electronics Co., Ltd
  */
 
+#include "rockchip-u-boot.dtsi"
+
 / {
 	aliases {
 		mmc0 = &sdhci;
diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
index 92f35309e4..c8df65980f 100644
--- a/arch/arm/mach-rockchip/Kconfig
+++ b/arch/arm/mach-rockchip/Kconfig
@@ -15,6 +15,7 @@ config ROCKCHIP_PX30
 	select DEBUG_UART_BOARD_INIT
 	imply ROCKCHIP_COMMON_BOARD
 	imply SPL_ROCKCHIP_COMMON_BOARD
+	imply BINMAN
 	help
 	  The Rockchip PX30 is a ARM-based SoC with a quad-core Cortex-A35
 	  including NEON and GPU, Mali-400 graphics, several DDR3 options
@@ -146,6 +147,7 @@ config ROCKCHIP_RK3308
 	imply SPL_SERIAL
 	imply TPL_SERIAL
 	imply SPL_SEPARATE_BSS
+	imply BINMAN
 	help
 	  The Rockchip RK3308 is a ARM-based Soc which embedded with quad
 	  Cortex-A35 and highly integrated audio interfaces.
@@ -167,6 +169,7 @@ config ROCKCHIP_RK3328
 	select ENABLE_ARM_SOC_BOOT0_HOOK
 	select DEBUG_UART_BOARD_INIT
 	select SYS_NS16550
+	imply BINMAN
 	help
 	  The Rockchip RK3328 is a ARM-based SoC with a quad-core Cortex-A53.
 	  including NEON and GPU, 1MB L2 cache, Mali-T7 graphics, two
@@ -186,6 +189,7 @@ config ROCKCHIP_RK3368
 	imply SPL_SERIAL
 	imply TPL_SERIAL
 	imply TPL_ROCKCHIP_COMMON_BOARD
+	imply BINMAN
 	help
 	  The Rockchip RK3368 is a ARM-based SoC with a octa-core (organised
 	  into a big and little cluster with 4 cores each) Cortex-A53 including
@@ -244,6 +248,7 @@ config ROCKCHIP_RK3399
 	imply TPL_ROCKCHIP_COMMON_BOARD
 	imply SYS_BOOTCOUNT_SINGLEWORD if BOOTCOUNT_LIMIT
 	imply CMD_BOOTCOUNT if BOOTCOUNT_LIMIT
+	imply BINMAN
 	help
 	  The Rockchip RK3399 is a ARM-based SoC with a dual-core Cortex-A72
 	  and quad-core Cortex-A53.
@@ -264,6 +269,7 @@ config ROCKCHIP_RK3568
 	select SYSCON
 	select BOARD_LATE_INIT
 	imply ROCKCHIP_COMMON_BOARD
+	imply BINMAN
 	help
 	  The Rockchip RK3568 is a ARM-based SoC with quad-core Cortex-A55,
 	  including NEON and GPU, 512K L3 cache, Mali-G52 based graphics,
-- 
2.35.0.263.gb82422642f-goog

[PATCH 22/24] rockchip: Support building the all output files in binman

[Simon Glass]

Add the required binman images to replace the Makefile rules which are
currently used. This includes subsuming:

   - tpl/u-boot-tpl-rockchip.bin if TPL is enabled
   - idbloader.img if either or both of SPL and TPL are enabled
   - u-boot.itb2 if SPL_FIT is enabled
   - u-boot-rockchip.bin if SPL is used, either using u-boot.itb2 when
     SPL_FIT is enabled or u-boot.img when it isn't

For now u-boot.itb2 is used as the FIT filename to avoid conflicting with
the current u-boot.itb file. This will be updated in a future patch.

Note that the intermediate files are dropped with binman, since it
producing everything in one pass. This means that
tpl/u-boot-tpl-rockchip.bin is not created, for example.

Note that for some 32-bit rk3288 boards, rockchip-optee.dtsi is included.

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

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

diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
index eae3ee715d..6246ca12b7 100644
--- a/arch/arm/dts/rockchip-u-boot.dtsi
+++ b/arch/arm/dts/rockchip-u-boot.dtsi
@@ -17,13 +17,93 @@
 		filename = "u-boot-rockchip.bin";
 		pad-byte = <0xff>;
 
-		blob {
-			filename = "idbloader.img";
+#ifdef CONFIG_TPL
+		mkimage {
+			args = "-n", CONFIG_SYS_SOC, "-T rksd";
+
+			u-boot-tpl {
+			};
+		};
+
+		u-boot-spl {
 		};
+#elif defined(CONFIG_SPL) /* SPL only */
+		mkimage {
+			args = "-n", CONFIG_SYS_SOC, "-T rksd";
+
+			u-boot-spl {
+			};
+		};
+#endif
+#if defined(CONFIG_SPL_FIT) && defined(CONFIG_ARM64)
+		fit: fit {
+			description = "FIT image for U-Boot with bl31 (TF-A)";
+			#address-cells = <1>;
+			fit,fdt-list = "of-list";
+			fit,external-offset = <CONFIG_FIT_EXTERNAL_OFFSET>;
+			images {
+				u-boot {
+					description = "U-Boot (64-bit)";
+					type = "standalone";
+					os = "U-Boot";
+					arch = "arm64";
+					compression = "none";
+					load = <CONFIG_SYS_TEXT_BASE>;
+					u-boot-nodtb {
+					};
+				};
 
+				@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 {
+					};
+				};
+				@tee-SEQ {
+					fit,operation = "split-elf";
+					description = "TEE";
+					type = "tee";
+					arch = "arm64";
+					os = "tee";
+					compression = "none";
+					fit,load;
+					fit,entry;
+					fit,data;
+
+					op-tee {
+					};
+				};
+
+				@fdt-SEQ {
+					description = "fdt-NAME";
+					compression = "none";
+					type = "flat_dt";
+				};
+			};
+
+			configurations {
+				default = "@config-DEFAULT-SEQ";
+				@config-SEQ {
+					description = "NAME.dtb";
+					fdt = "fdt-SEQ";
+					firmware = "u-boot";
+					fit,loadables;
+				};
+			};
+		};
+#else
 		u-boot-img {
 			offset = <CONFIG_SPL_PAD_TO>;
 		};
+#endif /* CONFIG_ARM64 */
 	};
 };
 #endif
-- 
2.35.0.263.gb82422642f-goog

Re: [PATCH 22/24] rockchip: Support building the all output files in binman

[Peter Geis]

On Tue, Feb 8, 2022 at 1:54 PM Simon Glass <sjg@chromium.org> wrote:
>

Good Morning,

> Add the required binman images to replace the Makefile rules which are
> currently used. This includes subsuming:
>
>    - tpl/u-boot-tpl-rockchip.bin if TPL is enabled
>    - idbloader.img if either or both of SPL and TPL are enabled
>    - u-boot.itb2 if SPL_FIT is enabled
>    - u-boot-rockchip.bin if SPL is used, either using u-boot.itb2 when
>      SPL_FIT is enabled or u-boot.img when it isn't
>
> For now u-boot.itb2 is used as the FIT filename to avoid conflicting with
> the current u-boot.itb file. This will be updated in a future patch.
>
> Note that the intermediate files are dropped with binman, since it
> producing everything in one pass. This means that
> tpl/u-boot-tpl-rockchip.bin is not created, for example.

A question if binman can handle the following:
Currently, it is impossible to build a rk3568 image automatically.
This is due to the fact that unlike previous boards, you must pass
both TPL and SPL to mkimage at the same time (similar to rk3399 spi).
Note: TPL currently isn't built in mainline, it must be pulled from a
prebuilt binary.
./tools/mkimage -n rk3568 -T rksd -d
rk3568_ddr_1560MHz_v1.12.bin:spl/u-boot-spl.bin idbloader.img

The Makefile method didn't seem to be able to handle this, so I had to
hack in my own function to do it.
I'm hoping this series provides a more elegant solution.

Thanks,
Peter Geis

>
> Note that for some 32-bit rk3288 boards, rockchip-optee.dtsi is included.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  arch/arm/dts/rockchip-u-boot.dtsi | 84 ++++++++++++++++++++++++++++++-
>  1 file changed, 82 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
> index eae3ee715d..6246ca12b7 100644
> --- a/arch/arm/dts/rockchip-u-boot.dtsi
> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
> @@ -17,13 +17,93 @@
>                 filename = "u-boot-rockchip.bin";
>                 pad-byte = <0xff>;
>
> -               blob {
> -                       filename = "idbloader.img";
> +#ifdef CONFIG_TPL
> +               mkimage {
> +                       args = "-n", CONFIG_SYS_SOC, "-T rksd";
> +
> +                       u-boot-tpl {
> +                       };
> +               };
> +
> +               u-boot-spl {
>                 };
> +#elif defined(CONFIG_SPL) /* SPL only */
> +               mkimage {
> +                       args = "-n", CONFIG_SYS_SOC, "-T rksd";
> +
> +                       u-boot-spl {
> +                       };
> +               };
> +#endif
> +#if defined(CONFIG_SPL_FIT) && defined(CONFIG_ARM64)
> +               fit: fit {
> +                       description = "FIT image for U-Boot with bl31 (TF-A)";
> +                       #address-cells = <1>;
> +                       fit,fdt-list = "of-list";
> +                       fit,external-offset = <CONFIG_FIT_EXTERNAL_OFFSET>;
> +                       images {
> +                               u-boot {
> +                                       description = "U-Boot (64-bit)";
> +                                       type = "standalone";
> +                                       os = "U-Boot";
> +                                       arch = "arm64";
> +                                       compression = "none";
> +                                       load = <CONFIG_SYS_TEXT_BASE>;
> +                                       u-boot-nodtb {
> +                                       };
> +                               };
>
> +                               @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 {
> +                                       };
> +                               };
> +                               @tee-SEQ {
> +                                       fit,operation = "split-elf";
> +                                       description = "TEE";
> +                                       type = "tee";
> +                                       arch = "arm64";
> +                                       os = "tee";
> +                                       compression = "none";
> +                                       fit,load;
> +                                       fit,entry;
> +                                       fit,data;
> +
> +                                       op-tee {
> +                                       };
> +                               };
> +
> +                               @fdt-SEQ {
> +                                       description = "fdt-NAME";
> +                                       compression = "none";
> +                                       type = "flat_dt";
> +                               };
> +                       };
> +
> +                       configurations {
> +                               default = "@config-DEFAULT-SEQ";
> +                               @config-SEQ {
> +                                       description = "NAME.dtb";
> +                                       fdt = "fdt-SEQ";
> +                                       firmware = "u-boot";
> +                                       fit,loadables;
> +                               };
> +                       };
> +               };
> +#else
>                 u-boot-img {
>                         offset = <CONFIG_SPL_PAD_TO>;
>                 };
> +#endif /* CONFIG_ARM64 */
>         };
>  };
>  #endif
> --
> 2.35.0.263.gb82422642f-goog
>

Re: [PATCH 22/24] rockchip: Support building the all output files in binman

[Simon Glass]

Hi Peter,

On Thu, 10 Feb 2022 at 08:04, Peter Geis <pgwipeout@gmail.com> wrote:
>
> On Tue, Feb 8, 2022 at 1:54 PM Simon Glass <sjg@chromium.org> wrote:
> >
>
> Good Morning,
>
> > Add the required binman images to replace the Makefile rules which are
> > currently used. This includes subsuming:
> >
> >    - tpl/u-boot-tpl-rockchip.bin if TPL is enabled
> >    - idbloader.img if either or both of SPL and TPL are enabled
> >    - u-boot.itb2 if SPL_FIT is enabled
> >    - u-boot-rockchip.bin if SPL is used, either using u-boot.itb2 when
> >      SPL_FIT is enabled or u-boot.img when it isn't
> >
> > For now u-boot.itb2 is used as the FIT filename to avoid conflicting with
> > the current u-boot.itb file. This will be updated in a future patch.
> >
> > Note that the intermediate files are dropped with binman, since it
> > producing everything in one pass. This means that
> > tpl/u-boot-tpl-rockchip.bin is not created, for example.
>
> A question if binman can handle the following:
> Currently, it is impossible to build a rk3568 image automatically.
> This is due to the fact that unlike previous boards, you must pass
> both TPL and SPL to mkimage at the same time (similar to rk3399 spi).
> Note: TPL currently isn't built in mainline, it must be pulled from a
> prebuilt binary.
> ./tools/mkimage -n rk3568 -T rksd -d
> rk3568_ddr_1560MHz_v1.12.bin:spl/u-boot-spl.bin idbloader.img
>
> The Makefile method didn't seem to be able to handle this, so I had to
> hack in my own function to do it.
> I'm hoping this series provides a more elegant solution.

Binman certainly lets you add multiple things in, but the bin: stuff
is not supported. If it is simply a case of joining the ddr and SPL
then you do that with:

mkimage {
   args = ...

   ddrl {
      type = "blob-ext";
      filename = "rk3568_ddr_1560MHz_v1.12.bin";
   };
   u-boot-spl {
   };
};

At present mkimage is not a subclass of Entry_section, so alignment
and padding of the mkimage input are not supported. But that should be
easy enough to change, if needed.

[..]

Regards,
Simon

Re: [PATCH 22/24] rockchip: Support building the all output files in binman

[Peter Geis]

On Thu, Feb 10, 2022 at 1:58 PM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Peter,
>
> On Thu, 10 Feb 2022 at 08:04, Peter Geis <pgwipeout@gmail.com> wrote:
> >
> > On Tue, Feb 8, 2022 at 1:54 PM Simon Glass <sjg@chromium.org> wrote:
> > >
> >
> > Good Morning,
> >
> > > Add the required binman images to replace the Makefile rules which are
> > > currently used. This includes subsuming:
> > >
> > >    - tpl/u-boot-tpl-rockchip.bin if TPL is enabled
> > >    - idbloader.img if either or both of SPL and TPL are enabled
> > >    - u-boot.itb2 if SPL_FIT is enabled
> > >    - u-boot-rockchip.bin if SPL is used, either using u-boot.itb2 when
> > >      SPL_FIT is enabled or u-boot.img when it isn't
> > >
> > > For now u-boot.itb2 is used as the FIT filename to avoid conflicting with
> > > the current u-boot.itb file. This will be updated in a future patch.
> > >
> > > Note that the intermediate files are dropped with binman, since it
> > > producing everything in one pass. This means that
> > > tpl/u-boot-tpl-rockchip.bin is not created, for example.
> >
> > A question if binman can handle the following:
> > Currently, it is impossible to build a rk3568 image automatically.
> > This is due to the fact that unlike previous boards, you must pass
> > both TPL and SPL to mkimage at the same time (similar to rk3399 spi).
> > Note: TPL currently isn't built in mainline, it must be pulled from a
> > prebuilt binary.
> > ./tools/mkimage -n rk3568 -T rksd -d
> > rk3568_ddr_1560MHz_v1.12.bin:spl/u-boot-spl.bin idbloader.img
> >
> > The Makefile method didn't seem to be able to handle this, so I had to
> > hack in my own function to do it.
> > I'm hoping this series provides a more elegant solution.
>
> Binman certainly lets you add multiple things in, but the bin: stuff
> is not supported. If it is simply a case of joining the ddr and SPL
> then you do that with:

Okay, I'll test this, thanks!
They both needed to be passed to mkimage because they both get
checksummed individually, but stored in the same header.

>
> mkimage {
>    args = ...
>
>    ddrl {
>       type = "blob-ext";
>       filename = "rk3568_ddr_1560MHz_v1.12.bin";
>    };
>    u-boot-spl {
>    };
> };
>
> At present mkimage is not a subclass of Entry_section, so alignment
> and padding of the mkimage input are not supported. But that should be
> easy enough to change, if needed.

Alignment and padding shouldn't be an issue here.
mkimage handles everything and spits out a complete file.

I haven't yet confirmed if the rk33 spi padding issue applies to the
rk35 series, mostly because I haven't yet achieved stable support for
the SFC in mainline yet.

Thanks again!
Peter

>
> [..]
>
> Regards,
> Simon

Re: [PATCH 22/24] rockchip: Support building the all output files in binman

[Peter Geis]

On Thu, Feb 10, 2022 at 2:32 PM Peter Geis <pgwipeout@gmail.com> wrote:
>
> On Thu, Feb 10, 2022 at 1:58 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Peter,
> >
> > On Thu, 10 Feb 2022 at 08:04, Peter Geis <pgwipeout@gmail.com> wrote:
> > >
> > > On Tue, Feb 8, 2022 at 1:54 PM Simon Glass <sjg@chromium.org> wrote:
> > > >
> > >
> > > Good Morning,
> > >
> > > > Add the required binman images to replace the Makefile rules which are
> > > > currently used. This includes subsuming:
> > > >
> > > >    - tpl/u-boot-tpl-rockchip.bin if TPL is enabled
> > > >    - idbloader.img if either or both of SPL and TPL are enabled
> > > >    - u-boot.itb2 if SPL_FIT is enabled
> > > >    - u-boot-rockchip.bin if SPL is used, either using u-boot.itb2 when
> > > >      SPL_FIT is enabled or u-boot.img when it isn't
> > > >
> > > > For now u-boot.itb2 is used as the FIT filename to avoid conflicting with
> > > > the current u-boot.itb file. This will be updated in a future patch.
> > > >
> > > > Note that the intermediate files are dropped with binman, since it
> > > > producing everything in one pass. This means that
> > > > tpl/u-boot-tpl-rockchip.bin is not created, for example.
> > >
> > > A question if binman can handle the following:
> > > Currently, it is impossible to build a rk3568 image automatically.
> > > This is due to the fact that unlike previous boards, you must pass
> > > both TPL and SPL to mkimage at the same time (similar to rk3399 spi).
> > > Note: TPL currently isn't built in mainline, it must be pulled from a
> > > prebuilt binary.
> > > ./tools/mkimage -n rk3568 -T rksd -d
> > > rk3568_ddr_1560MHz_v1.12.bin:spl/u-boot-spl.bin idbloader.img
> > >
> > > The Makefile method didn't seem to be able to handle this, so I had to
> > > hack in my own function to do it.
> > > I'm hoping this series provides a more elegant solution.
> >
> > Binman certainly lets you add multiple things in, but the bin: stuff
> > is not supported. If it is simply a case of joining the ddr and SPL
> > then you do that with:
>
> Okay, I'll test this, thanks!
> They both needed to be passed to mkimage because they both get
> checksummed individually, but stored in the same header.
>
> >
> > mkimage {
> >    args = ...
> >
> >    ddrl {
> >       type = "blob-ext";
> >       filename = "rk3568_ddr_1560MHz_v1.12.bin";
> >    };
> >    u-boot-spl {
> >    };

Good Evening,
I tested this, unfortunately it seems to cat the two files into a
single file and trying to run that through mkimage, which breaks due
to size constraints:
binman: Error 1 running 'mkimage -d ./mkimage.simple-bin.mkimage -n
rk3568 -T rksd ./mkimage-out.simple-bin.mkimage': Error: SPL image is
too large (size 0x22000 than 0x13000)
This matches the behavior listed in the changes to `tools/binman/binman.rst`

> > };
> >
> > At present mkimage is not a subclass of Entry_section, so alignment
> > and padding of the mkimage input are not supported. But that should be
> > easy enough to change, if needed.
>
> Alignment and padding shouldn't be an issue here.
> mkimage handles everything and spits out a complete file.
>
> I haven't yet confirmed if the rk33 spi padding issue applies to the
> rk35 series, mostly because I haven't yet achieved stable support for
> the SFC in mainline yet.
>
> Thanks again!
> Peter
>
> >
> > [..]
> >
> > Regards,
> > Simon

Re: [PATCH 22/24] rockchip: Support building the all output files in binman

[Alper Nebi Yasak]

On 08/02/2022 21:50, Simon Glass wrote:
> Add the required binman images to replace the Makefile rules which are
> currently used. This includes subsuming:
> 
>    - tpl/u-boot-tpl-rockchip.bin if TPL is enabled
>    - idbloader.img if either or both of SPL and TPL are enabled
>    - u-boot.itb2 if SPL_FIT is enabled
>    - u-boot-rockchip.bin if SPL is used, either using u-boot.itb2 when
>      SPL_FIT is enabled or u-boot.img when it isn't
> 
> For now u-boot.itb2 is used as the FIT filename to avoid conflicting with
> the current u-boot.itb file. This will be updated in a future patch.

I don't see a u-boot.itb2 anywhere. Something from an earlier revision
or does binman append the number to avoid overwriting existing files?

> 
> Note that the intermediate files are dropped with binman, since it
> producing everything in one pass. This means that
> tpl/u-boot-tpl-rockchip.bin is not created, for example.
> 
> Note that for some 32-bit rk3288 boards, rockchip-optee.dtsi is included.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  arch/arm/dts/rockchip-u-boot.dtsi | 84 ++++++++++++++++++++++++++++++-
>  1 file changed, 82 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
> index eae3ee715d..6246ca12b7 100644
> --- a/arch/arm/dts/rockchip-u-boot.dtsi
> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
> @@ -17,13 +17,93 @@
>  		filename = "u-boot-rockchip.bin";
>  		pad-byte = <0xff>;
>  
> -		blob {
> -			filename = "idbloader.img";
> +#ifdef CONFIG_TPL
> +		mkimage {
> +			args = "-n", CONFIG_SYS_SOC, "-T rksd";
> +
> +			u-boot-tpl {
> +			};
> +		};
> +
> +		u-boot-spl {
>  		};
> +#elif defined(CONFIG_SPL) /* SPL only */
> +		mkimage {
> +			args = "-n", CONFIG_SYS_SOC, "-T rksd";
> +
> +			u-boot-spl {
> +			};
> +		};
> +#endif
> +#if defined(CONFIG_SPL_FIT) && defined(CONFIG_ARM64)
> +		fit: fit {
> +			description = "FIT image for U-Boot with bl31 (TF-A)";
> +			#address-cells = <1>;
> +			fit,fdt-list = "of-list";
> +			fit,external-offset = <CONFIG_FIT_EXTERNAL_OFFSET>;
> +			images {
> +				u-boot {
> +					description = "U-Boot (64-bit)";
> +					type = "standalone";
> +					os = "U-Boot";
> +					arch = "arm64";
> +					compression = "none";
> +					load = <CONFIG_SYS_TEXT_BASE>;
> +					u-boot-nodtb {
> +					};
> +				};
>  
> +				@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 {
> +					};
> +				};
> +				@tee-SEQ {
> +					fit,operation = "split-elf";
> +					description = "TEE";
> +					type = "tee";
> +					arch = "arm64";
> +					os = "tee";
> +					compression = "none";
> +					fit,load;
> +					fit,entry;
> +					fit,data;
> +
> +					op-tee {
> +					};
> +				};
> +
> +				@fdt-SEQ {
> +					description = "fdt-NAME";
> +					compression = "none";
> +					type = "flat_dt";
> +				};
> +			};
> +
> +			configurations {
> +				default = "@config-DEFAULT-SEQ";
> +				@config-SEQ {
> +					description = "NAME.dtb";
> +					fdt = "fdt-SEQ";
> +					firmware = "u-boot";
> +					fit,loadables;
> +				};
> +			};
> +		};
> +#else

This should be done for the SPI ROM image in rk3399-u-boot.dtsi as well,
and with that my chromebook_kevin can boot into Linux or EFI GRUB quite
nicely.

I changed the "rksd" parts to "rkspi", and as I wrote on a different
patch I had to change the firmware and loadables like so (where I wasn't
passing in an op-tee at all):

    configurations {
            default = "@config-DEFAULT-SEQ";
            @config-SEQ {
                    description = "NAME.dtb";
                    fdt = "fdt-SEQ";
                    firmware = "atf-1";
                    loadables = "u-boot", "atf-2", "atf-3";
            };
    };

I also had to use the same offset as the u-boot-img there, so maybe this
needs the offset below as well.

>  		u-boot-img {
>  			offset = <CONFIG_SPL_PAD_TO>;
>  		};
> +#endif /* CONFIG_ARM64 */
>  	};
>  };
>  #endif

[PATCH 23/24] rockchip: Convert all boards to use binman

[Simon Glass]

Instead of the bash script, use binman to generate the FIT for arm64.

For 32-bit boards, use binman for all images, dropping the intermediate
files.

With this change, only Zynq is now using SPL_FIT_GENERATOR so update the
Kconfig rule accordingly.

Clean up the Makefile to the extent possible. Unfortunately, two boards
do not use SPL_FRAMEWORK so don't enable the u-boot.img rule:

   evb-rk3036
   kylin-rk3036

So a small remnant remains.

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

 Makefile     | 39 ++-------------------------------------
 boot/Kconfig |  2 +-
 2 files changed, 3 insertions(+), 38 deletions(-)

diff --git a/Makefile b/Makefile
index ee1dc11e2f..c7f1cb3d2e 100644
--- a/Makefile
+++ b/Makefile
@@ -976,19 +976,8 @@ ifeq ($(CONFIG_MPC85xx)$(CONFIG_OF_SEPARATE),yy)
 INPUTS-y += u-boot-with-dtb.bin
 endif
 
-ifeq ($(CONFIG_ARCH_ROCKCHIP),y)
-# On ARM64 this target is produced by binman so we don't need this dep
-ifeq ($(CONFIG_ARM64),y)
-ifeq ($(CONFIG_SPL),y)
-# TODO: Get binman to generate this too
-INPUTS-y += u-boot-rockchip.bin
-endif
-else
-ifeq ($(CONFIG_SPL),y)
-# Generate these inputs for binman which will create the output files
-INPUTS-y += idbloader.img u-boot.img
-endif
-endif
+ifeq ($(CONFIG_ARCH_ROCKCHIP)_$(CONFIG_SPL_FRAMEWORK),y_)
+INPUTS-y += u-boot.img
 endif
 
 INPUTS-$(CONFIG_X86) += u-boot-x86-start16.bin u-boot-x86-reset16.bin \
@@ -1472,30 +1461,6 @@ OBJCOPYFLAGS_u-boot-with-spl.bin = -I binary -O binary \
 u-boot-with-spl.bin: $(SPL_IMAGE) $(SPL_PAYLOAD) FORCE
 	$(call if_changed,pad_cat)
 
-ifeq ($(CONFIG_ARCH_ROCKCHIP),y)
-
-# TPL + SPL
-ifeq ($(CONFIG_SPL)$(CONFIG_TPL),yy)
-MKIMAGEFLAGS_u-boot-tpl-rockchip.bin = -n $(CONFIG_SYS_SOC) -T rksd
-tpl/u-boot-tpl-rockchip.bin: tpl/u-boot-tpl.bin FORCE
-	$(call if_changed,mkimage)
-idbloader.img: tpl/u-boot-tpl-rockchip.bin spl/u-boot-spl.bin FORCE
-	$(call if_changed,cat)
-else
-MKIMAGEFLAGS_idbloader.img = -n $(CONFIG_SYS_SOC) -T rksd
-idbloader.img: spl/u-boot-spl.bin FORCE
-	$(call if_changed,mkimage)
-endif
-
-ifeq ($(CONFIG_ARM64),y)
-OBJCOPYFLAGS_u-boot-rockchip.bin = -I binary -O binary \
-	--pad-to=$(CONFIG_SPL_PAD_TO) --gap-fill=0xff
-u-boot-rockchip.bin: idbloader.img u-boot.itb FORCE
-	$(call if_changed,pad_cat)
-endif # CONFIG_ARM64
-
-endif # CONFIG_ARCH_ROCKCHIP
-
 ifeq ($(CONFIG_ARCH_LPC32XX)$(CONFIG_SPL),yy)
 MKIMAGEFLAGS_lpc32xx-spl.img = -T lpc32xximage -a $(CONFIG_SPL_TEXT_BASE)
 
diff --git a/boot/Kconfig b/boot/Kconfig
index b83a4e8400..ae1a37ae92 100644
--- a/boot/Kconfig
+++ b/boot/Kconfig
@@ -275,7 +275,7 @@ config SPL_FIT_SOURCE
 
 config USE_SPL_FIT_GENERATOR
 	bool "Use a script to generate the .its script"
-	default y if SPL_FIT && (!ARCH_SUNXI && !RISCV)
+	default y if SPL_FIT && ARCH_ZYNQMP
 
 config SPL_FIT_GENERATOR
 	string ".its file generator script for U-Boot FIT image"
-- 
2.35.0.263.gb82422642f-goog

[PATCH 24/24] rockchip: Drop the FIT generator script

[Simon Glass]

This is not used anymore. Drop it.

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

 Makefile                               |   3 -
 arch/arm/mach-rockchip/make_fit_atf.py | 240 -------------------------
 boot/Kconfig                           |   1 -
 3 files changed, 244 deletions(-)
 delete mode 100755 arch/arm/mach-rockchip/make_fit_atf.py

diff --git a/Makefile b/Makefile
index c7f1cb3d2e..cc7d358148 100644
--- a/Makefile
+++ b/Makefile
@@ -1351,9 +1351,6 @@ $(U_BOOT_ITS): $(subst ",,$(CONFIG_SPL_FIT_SOURCE))
 else
 ifneq ($(CONFIG_USE_SPL_FIT_GENERATOR),)
 U_BOOT_ITS := u-boot.its
-ifeq ($(CONFIG_SPL_FIT_GENERATOR),"arch/arm/mach-rockchip/make_fit_atf.py")
-U_BOOT_ITS_DEPS += u-boot
-endif
 $(U_BOOT_ITS): $(U_BOOT_ITS_DEPS) FORCE
 	$(srctree)/$(CONFIG_SPL_FIT_GENERATOR) \
 	$(patsubst %,arch/$(ARCH)/dts/%.dtb,$(subst ",,$(CONFIG_OF_LIST))) > $@
diff --git a/arch/arm/mach-rockchip/make_fit_atf.py b/arch/arm/mach-rockchip/make_fit_atf.py
deleted file mode 100755
index f3224d2555..0000000000
--- a/arch/arm/mach-rockchip/make_fit_atf.py
+++ /dev/null
@@ -1,240 +0,0 @@
-#!/usr/bin/env python3
-"""
-# SPDX-License-Identifier: GPL-2.0+
-#
-# A script to generate FIT image source for rockchip boards
-# with ARM Trusted Firmware
-# and multiple device trees (given on the command line)
-#
-# usage: $0 <dt_name> [<dt_name> [<dt_name] ...]
-"""
-
-import os
-import sys
-import getopt
-import logging
-import struct
-
-DT_HEADER = """
-/*
- * This is a generated file.
- */
-/dts-v1/;
-
-/ {
-	description = "FIT image for U-Boot with bl31 (TF-A)";
-	#address-cells = <1>;
-
-	images {
-"""
-
-DT_UBOOT = """
-		uboot {
-			description = "U-Boot (64-bit)";
-			data = /incbin/("u-boot-nodtb.bin");
-			type = "standalone";
-			os = "U-Boot";
-			arch = "arm64";
-			compression = "none";
-			load = <0x%08x>;
-		};
-
-"""
-
-DT_IMAGES_NODE_END = """	};
-
-"""
-
-DT_END = "};"
-
-def append_bl31_node(file, atf_index, phy_addr, elf_entry):
-    # Append BL31 DT node to input FIT dts file.
-    data = 'bl31_0x%08x.bin' % phy_addr
-    file.write('\t\tatf_%d {\n' % atf_index)
-    file.write('\t\t\tdescription = \"ARM Trusted Firmware\";\n')
-    file.write('\t\t\tdata = /incbin/("%s");\n' % data)
-    file.write('\t\t\ttype = "firmware";\n')
-    file.write('\t\t\tarch = "arm64";\n')
-    file.write('\t\t\tos = "arm-trusted-firmware";\n')
-    file.write('\t\t\tcompression = "none";\n')
-    file.write('\t\t\tload = <0x%08x>;\n' % phy_addr)
-    if atf_index == 1:
-        file.write('\t\t\tentry = <0x%08x>;\n' % elf_entry)
-    file.write('\t\t};\n')
-    file.write('\n')
-
-def append_tee_node(file, atf_index, phy_addr, elf_entry):
-    # Append TEE DT node to input FIT dts file.
-    data = 'tee_0x%08x.bin' % phy_addr
-    file.write('\t\tatf_%d {\n' % atf_index)
-    file.write('\t\t\tdescription = \"TEE\";\n')
-    file.write('\t\t\tdata = /incbin/("%s");\n' % data)
-    file.write('\t\t\ttype = "tee";\n')
-    file.write('\t\t\tarch = "arm64";\n')
-    file.write('\t\t\tos = "tee";\n')
-    file.write('\t\t\tcompression = "none";\n')
-    file.write('\t\t\tload = <0x%08x>;\n' % phy_addr)
-    file.write('\t\t\tentry = <0x%08x>;\n' % elf_entry)
-    file.write('\t\t};\n')
-    file.write('\n')
-
-def append_fdt_node(file, dtbs):
-    # Append FDT nodes.
-    cnt = 1
-    for dtb in dtbs:
-        dtname = os.path.basename(dtb)
-        file.write('\t\tfdt_%d {\n' % cnt)
-        file.write('\t\t\tdescription = "%s";\n' % dtname)
-        file.write('\t\t\tdata = /incbin/("%s");\n' % dtb)
-        file.write('\t\t\ttype = "flat_dt";\n')
-        file.write('\t\t\tcompression = "none";\n')
-        file.write('\t\t};\n')
-        file.write('\n')
-        cnt = cnt + 1
-
-def append_conf_section(file, cnt, dtname, segments):
-    file.write('\t\tconfig_%d {\n' % cnt)
-    file.write('\t\t\tdescription = "%s";\n' % dtname)
-    file.write('\t\t\tfirmware = "atf_1";\n')
-    file.write('\t\t\tloadables = "uboot"')
-    if segments > 1:
-        file.write(',')
-    for i in range(1, segments):
-        file.write('"atf_%d"' % (i + 1))
-        if i != (segments - 1):
-            file.write(',')
-        else:
-            file.write(';\n')
-    if segments <= 1:
-        file.write(';\n')
-    file.write('\t\t\tfdt = "fdt_%d";\n' % cnt)
-    file.write('\t\t};\n')
-    file.write('\n')
-
-def append_conf_node(file, dtbs, segments):
-    # Append configeration nodes.
-    cnt = 1
-    file.write('\tconfigurations {\n')
-    file.write('\t\tdefault = "config_1";\n')
-    for dtb in dtbs:
-        dtname = os.path.basename(dtb)
-        append_conf_section(file, cnt, dtname, segments)
-        cnt = cnt + 1
-    file.write('\t};\n')
-    file.write('\n')
-
-def generate_atf_fit_dts_uboot(fit_file, uboot_file_name):
-    segments = unpack_elf(uboot_file_name)
-    if len(segments) != 1:
-        raise ValueError("Invalid u-boot ELF image '%s'" % uboot_file_name)
-    index, entry, p_paddr, data = segments[0]
-    fit_file.write(DT_UBOOT % p_paddr)
-
-def generate_atf_fit_dts_bl31(fit_file, bl31_file_name, tee_file_name, dtbs_file_name):
-    segments = unpack_elf(bl31_file_name)
-    for index, entry, paddr, data in segments:
-        append_bl31_node(fit_file, index + 1, paddr, entry)
-    num_segments = len(segments)
-
-    if tee_file_name:
-        tee_segments = unpack_elf(tee_file_name)
-        for index, entry, paddr, data in tee_segments:
-            append_tee_node(fit_file, num_segments + index + 1, paddr, entry)
-        num_segments = num_segments + len(tee_segments)
-
-    append_fdt_node(fit_file, dtbs_file_name)
-    fit_file.write(DT_IMAGES_NODE_END)
-    append_conf_node(fit_file, dtbs_file_name, num_segments)
-
-def generate_atf_fit_dts(fit_file_name, bl31_file_name, tee_file_name, uboot_file_name, dtbs_file_name):
-    # Generate FIT script for ATF image.
-    if fit_file_name != sys.stdout:
-        fit_file = open(fit_file_name, "wb")
-    else:
-        fit_file = sys.stdout
-
-    fit_file.write(DT_HEADER)
-    generate_atf_fit_dts_uboot(fit_file, uboot_file_name)
-    generate_atf_fit_dts_bl31(fit_file, bl31_file_name, tee_file_name, dtbs_file_name)
-    fit_file.write(DT_END)
-
-    if fit_file_name != sys.stdout:
-        fit_file.close()
-
-def generate_atf_binary(bl31_file_name):
-    for index, entry, paddr, data in unpack_elf(bl31_file_name):
-        file_name = 'bl31_0x%08x.bin' % paddr
-        with open(file_name, "wb") as atf:
-            atf.write(data)
-
-def generate_tee_binary(tee_file_name):
-    if tee_file_name:
-        for index, entry, paddr, data in unpack_elf(tee_file_name):
-            file_name = 'tee_0x%08x.bin' % paddr
-            with open(file_name, "wb") as atf:
-                atf.write(data)
-
-def unpack_elf(filename):
-    with open(filename, 'rb') as file:
-        elf = file.read()
-    if elf[0:7] != b'\x7fELF\x02\x01\x01' or elf[18:20] != b'\xb7\x00':
-        raise ValueError("Invalid arm64 ELF file '%s'" % filename)
-
-    e_entry, e_phoff = struct.unpack_from('<2Q', elf, 0x18)
-    e_phentsize, e_phnum = struct.unpack_from('<2H', elf, 0x36)
-    segments = []
-
-    for index in range(e_phnum):
-        offset = e_phoff + e_phentsize * index
-        p_type, p_flags, p_offset = struct.unpack_from('<LLQ', elf, offset)
-        if p_type == 1: # PT_LOAD
-            p_paddr, p_filesz = struct.unpack_from('<2Q', elf, offset + 0x18)
-            if p_filesz > 0:
-                p_data = elf[p_offset:p_offset + p_filesz]
-                segments.append((index, e_entry, p_paddr, p_data))
-    return segments
-
-def main():
-    uboot_elf = "./u-boot"
-    fit_its = sys.stdout
-    if "BL31" in os.environ:
-        bl31_elf=os.getenv("BL31");
-    elif os.path.isfile("./bl31.elf"):
-        bl31_elf = "./bl31.elf"
-    else:
-        os.system("echo 'int main(){}' > bl31.c")
-        os.system("${CROSS_COMPILE}gcc -c bl31.c -o bl31.elf")
-        bl31_elf = "./bl31.elf"
-        logging.basicConfig(format='%(levelname)s:%(message)s', level=logging.DEBUG)
-        logging.warning(' BL31 file bl31.elf NOT found, resulting binary is non-functional')
-        logging.warning(' Please read Building section in doc/README.rockchip')
-
-    if "TEE" in os.environ:
-        tee_elf = os.getenv("TEE")
-    elif os.path.isfile("./tee.elf"):
-        tee_elf = "./tee.elf"
-    else:
-        tee_elf = ""
-
-    opts, args = getopt.getopt(sys.argv[1:], "o:u:b:t:h")
-    for opt, val in opts:
-        if opt == "-o":
-            fit_its = val
-        elif opt == "-u":
-            uboot_elf = val
-        elif opt == "-b":
-            bl31_elf = val
-        elif opt == "-t":
-            tee_elf = val
-        elif opt == "-h":
-            print(__doc__)
-            sys.exit(2)
-
-    dtbs = args
-
-    generate_atf_fit_dts(fit_its, bl31_elf, tee_elf, uboot_elf, dtbs)
-    generate_atf_binary(bl31_elf)
-    generate_tee_binary(tee_elf)
-
-if __name__ == "__main__":
-    main()
diff --git a/boot/Kconfig b/boot/Kconfig
index ae1a37ae92..1ae5b3a793 100644
--- a/boot/Kconfig
+++ b/boot/Kconfig
@@ -280,7 +280,6 @@ config USE_SPL_FIT_GENERATOR
 config SPL_FIT_GENERATOR
 	string ".its file generator script for U-Boot FIT image"
 	depends on USE_SPL_FIT_GENERATOR
-	default "arch/arm/mach-rockchip/make_fit_atf.py" if SPL_LOAD_FIT && ARCH_ROCKCHIP
 	default "arch/arm/mach-zynqmp/mkimage_fit_atf.sh" if SPL_LOAD_FIT && ARCH_ZYNQMP
 	help
 	  Specifies a (platform specific) script file to generate the FIT
-- 
2.35.0.263.gb82422642f-goog

Re: [PATCH 00/24] binman: rockchip: Migrate from rockchip SPL_FIT_GENERATOR script

[Alper Nebi Yasak]

Hi,

On 08/02/2022 21:49, Simon Glass wrote:
> At present rockchip 64-bit boards make use of a FIT-generator script
> written in Python. The script supports splitting an ELF file into several
> 'loadable' nodes in the FIT. Binman does not current support this feature.
> 
> This series adds binman support for ELF splitting. This works by adding a
> new 'fit,operation' property to the FIT subnodes, allowing this new way of
> generating nodes.
> 
> Some other fixes and improvements are needed along the way.
> 
> A new, common binman description is added for 64-bit boards which includes
> the required u-boot.itb file.
> 
> [...]

I've been testing this on my chromebook_kevin, I will write and send
some sort of a review but it might take me a while, few days at worst;
wanted to let you know in advance.

Re: [PATCH 19/24] binman: Allow different operations in FIT generator nodes

[Simon Glass]

At present we only support expanding out FDT nodes. Make the operation
into an @operation property, so that others can be supported.

Re-arrange and tidy up the documentation so that it has separate
headings for each topic.

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

 tools/binman/entries.rst               |  85 +++++++++++-----
 tools/binman/etype/fit.py              | 136 ++++++++++++++++++++-----
 tools/binman/ftest.py                  |   7 ++
 tools/binman/test/220_fit_bad_oper.dts |  27 +++++
 4 files changed, 206 insertions(+), 49 deletions(-)
 create mode 100644 tools/binman/test/220_fit_bad_oper.dts

Applied to u-boot-dm, thanks!

Re: [PATCH 10/24] binman: Update docs to indicate mkimage is supported

[Simon Glass]

Now that there is a mkimage entry-type, update the docs to remove the
future reference.

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

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

Applied to u-boot-dm, thanks!