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

[Alper Nebi Yasak <alpernebiyasak@gmail.com>]

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)