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

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

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) {

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())

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

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

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'))

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

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:
  

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";

+};

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

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