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

[Peter Geis]

On Thu, Mar 3, 2022 at 4:17 PM Alper Nebi Yasak
<alpernebiyasak at gmail.com> wrote:
>
> 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.

Thanks! I thought including mkimage as a prefix was necessary to make
it clear what was happening, but on second thought it's in the mkimage
node.

>
> >
> >       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.

I appreciate the confirmation.

>
> >
> > Thanks,
> > Peter Geis
> >
> > Signed-off-by: Peter Geis <pgwipeout at 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.

Hmm, thinking about it I think I can pull this off.
My first (non public) attempt was something similar to this, which
just hardcoded the first file name.

I'm not kidding when I said all the python I know I learned to make
this happen, so it's a learning event.
Thanks for the ideas!

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