Re: [RFC] [PATCH] binman: support mkimage split files

Alper Nebi Yasak
2022-03-03T21:19:47Z

On 01/03/2022 05:48, Peter Geis wrote:
> Good Evening,
>
> I successfully tested your v2 patch series to create a bootable sdcard
> image out of the box for rockpro64-rk3399.
> Unfortunately, rk356x and rk3399-spi modes are broken, due to the
> inability to pass multiple images to mkimage at the same time.
> rk3399-spi mode is already supported manually, see:
> https://elixir.bootlin.com/u-boot/v2022.04-rc3/source/doc/board/rockchip/rockchip.rst#L182
>
> rk356x is currently only supported manually, the image built by the old
> Makefile method is non functional. (u-boot-rockchip.bin)
>
> Knowing absolutely nothing about python, I've hacked together something
> that works for splitting the image in the way mkimage expects.
> The file name passed to mkimage with the -d flag is:
> ./mkimage.simple-bin.mkimage.1:./mkimage.simple-bin.mkimage.2
>
> I definitely don't expect this to be accepted as is, I just use it as an
> example of what we need to fully support this in binman.
> Adding the following allows me to build images automatically for rk356x:
>
> mkimage {
> args = "-n", CONFIG_SYS_SOC, "-T", "rksd";
> mkimage,separate_files;

Adding a property to toggle this sounds reasonable to me. The prefix
might not be necessary, and I think dashes are preferred to underscores
in property names.

>
> ddrl {
> type = "blob-ext";
> filename = "rk3568_ddr_1560MHz_v1.12.bin";
> };
>
> u-boot-spl {
> };
> };
>
> This is my first attempt to use in-reply-to, so I hope this works.

FYI, I see it as a reply to 00/25 of the series.

>
> Thanks,
> Peter Geis
>
> Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> ---
> tools/binman/entry.py | 43 ++++++++++++++++++++++++++---------
> tools/binman/etype/mkimage.py | 3 ++-
> 2 files changed, 34 insertions(+), 12 deletions(-)
>
> diff --git a/tools/binman/entry.py b/tools/binman/entry.py
> index 249f117ace56..48e552fc6af3 100644
> --- a/tools/binman/entry.py
> +++ b/tools/binman/entry.py
> @@ -114,6 +114,8 @@ class Entry(object):
> self.bintools = {}
> self.missing_bintools = []
> self.update_hash = True
> + self.fname_tmp = str()
> + self.index = 0
>
> @staticmethod
> def FindEntryClass(etype, expanded):
> @@ -1134,7 +1136,7 @@ features to produce new behaviours.
> """
> self.update_hash = update_hash
>
> - def collect_contents_to_file(self, entries, prefix, fake_size=0):
> + def collect_contents_to_file(self, entries, prefix, fake_size=0, separate=False):
> """Put the contents of a list of entries into a file
>
> Args:
> @@ -1152,13 +1154,32 @@ features to produce new behaviours.
> str: Unique portion of filename (or None 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(fake_size=fake_size):
> - return None, None, None
> - data += entry.GetData()
> - uniq = self.GetUniqueName()
> - fname = tools.get_output_filename(f'{prefix}.{uniq}')
> - tools.write_file(fname, data)
> - return data, fname, uniq
> + if separate is False:
> + for entry in entries:
> + # First get the input data and put it in a file. If not available,
> + # try later.
> + if not entry.ObtainContents(fake_size=fake_size):
> + return None, None, None
> + data += entry.GetData()
> + uniq = self.GetUniqueName()
> + fname = tools.get_output_filename(f'{prefix}.{uniq}')
> + tools.write_file(fname, data)
> + return data, fname, uniq
> + else:
> + for entry in entries:
> + self.index = (self.index + 1)
> + if (self.index > 2):
> + print('BINMAN Warn: mkimage only supports a maximum of two separate files')
> + break
> + # First get the input data and put it in a file. If not available,
> + # try later.
> + if not entry.ObtainContents(fake_size=fake_size):
> + return None, None, None
> + data = entry.GetData()
> + uniq = self.GetUniqueName()
> + fname = tools.get_output_filename(f'{prefix}.{uniq}.{self.index}')
> + tools.write_file(fname, data)
> + self.fname_tmp = [''.join(self.fname_tmp),fname]
> + fname = ':'.join(self.fname_tmp)
> + uniq = self.GetUniqueName()
> + return data, fname, uniq

I would keep this function as-is and call it multiple times in the
mkimage etype code below (once per subentry), and also do the
mkimage-specific checks and 'file1:file2' argument joining there as well.

> diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py
> index 5f6def2287f6..ce5f6b6b543a 100644
> --- a/tools/binman/etype/mkimage.py
> +++ b/tools/binman/etype/mkimage.py
> @@ -46,6 +46,7 @@ class Entry_mkimage(Entry):
> def __init__(self, section, etype, node):
> super().__init__(section, etype, node)
> self._args = fdt_util.GetArgs(self._node, 'args')
> + self._mkimage_separate = fdt_util.GetBool(self._node, 'mkimage,separate_files')
> self._mkimage_entries = OrderedDict()
> self.align_default = None
> self.ReadEntries()
> @@ -53,7 +54,7 @@ class Entry_mkimage(Entry):
> 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)
> + self._mkimage_entries.values(), 'mkimage', 1024, self._mkimage_separate)
> if data is None:
> return False
> output_fname = tools.get_output_filename('mkimage-out.%s' % uniq)