Re: [RFC] [PATCH] binman: support mkimage split files
On Thu, Mar 3, 2022 at 4:17 PM Alper Nebi Yasak
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
> > ---
> > 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}')
>
Re: [RFC] [PATCH] binman: support mkimage split files
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
> ---
> 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 -
