On 16/05/2022 14:07, Andrew Abbott wrote:
> mkimage supports combining multiple input binaries separating them
> with colons (':'). This is used at least for Rockchip platforms to
> encode payload offsets and sizes in the image header. It is required for
> Rockchip SPI boot since for the rkspi format, after the input binary
> combining, the entire image is spread across only the first 2K bytes of
> each 4K block.
>
> Previous to this change, multiple inputs to a binman mkimage node would
> just be concatenated and the combined input would be passed as the -d
> argument to mkimage. Now, the inputs are instead passed colon-separated.
>
> Signed-off-by: Andrew Abbott <and...@mirx.dev>
Also see another attempt for this [1] and the comments to that for a
more complete picture, though I'll try writing all the points here anyway.
[1] binman: support mkimage separate files
https://lore.kernel.org/u-boot/20220304195641.1993688-1-pgwipe...@gmail.com/
> ---
> This is a bit of a messy implementation for now and would probably break
> existing uses of mkimage that rely on the concatenation behaviour.
I did a `git grep -C10 mkimage -- **/dts/*` and it doesn't look like
anything uses it yet. Except for binman/test/156_mkimage.dts, which
doesn't exactly test the concatenation.
> Questions:
> - Should this be a separate entry type, or an option to the mkimage
> entry type that enables this behaviour?
You can add a 'separate-files' device-tree property like in [1]. I'm
actually OK with this separate-files being the default and only behavior
(concatenation can be done via an inner section), but I'm not sure Simon
would agree.
> - What kind of test(s) should I add?
At the minimum, a test using separate-files with multiple input entries.
You can do something like the _HandleGbbCommand in ftest.py to capture
and check the arguments that'll be passed to mkimage.
I think that'll be enough, but try to run `binman test -T` and check for
100% coverage with all tests succeeding.
> (no changes since v1)
>
> tools/binman/etype/mkimage.py | 33 +++++++++++++++++++++------------
> 1 file changed, 21 insertions(+), 12 deletions(-)
>
> diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py
> index 5f6def2287..8cea618fbd 100644
> --- a/tools/binman/etype/mkimage.py
> +++ b/tools/binman/etype/mkimage.py
> @@ -51,21 +51,30 @@ class Entry_mkimage(Entry):
Expand the docstring with an explanation of the new behavior, and run
`binman entry-docs >tools/binman/entries.rst` to update it there as well.
> self.ReadEntries()
>
> def ObtainContents(self):
> - # Use a non-zero size for any fake files to keep mkimage happy
> - data, input_fname, uniq = self.collect_contents_to_file(
> - self._mkimage_entries.values(), 'mkimage', 1024)
> - if data is None:
> - return False
> - output_fname = tools.get_output_filename('mkimage-out.%s' % uniq)
> - if self.mkimage.run_cmd('-d', input_fname, *self._args,
> - output_fname) is not None:
> + # For multiple inputs to mkimage, we want to separate them by colons.
> + # This is needed for eg. the rkspi format, which treats the first
> data
> + # file as the "init" and the second as "boot" and sets the image
> header
> + # accordingly, then makes the image so that only the first 2 KiB of
> each
> + # 4KiB block is used.
> +
> + data_filenames = []
> + for entry in self._mkimage_entries.values():
> + # First get the input data and put it in a file. If any entry is
> not
> + # available, try later.
> + if not entry.ObtainContents():
> + return False
> +
> + input_fname = tools.get_output_filename('mkimage-in.%s' %
> entry.GetUniqueName())
> + data_filenames.append(input_fname)
> + tools.write_file(input_fname, entry.GetData())
Something like collect_contents_to_file([entry], f'mkimage-in-{idx}',
1024) would be better here. At least, the files must not be empty (or
mkimage exits with an error), where entry.GetData() can be b''.
> +
> + output_fname = tools.get_output_filename('mkimage-out.%s' %
> self.GetUniqueName())
Should be an f-string.
> + if self.mkimage.run_cmd('-d', ":".join(data_filenames), *self._args,
> output_fname):
> self.SetContents(tools.read_file(output_fname))
> + return True
> else:
> - # Bintool is missing; just use the input data as the output
> self.record_missing_bintool(self.mkimage)
> - self.SetContents(data)
> -
> - return True
> + return False
This case must set some dummy contents (I'd guess b'' is fine) and
return True. (False here roughly means "try again later".)
>
> def ReadEntries(self):
> """Read the subnodes to find out what should go in this image"""
