Message ID | 20220301024826.1228290-1-pgwipeout@gmail.com |
---|---|
State | Superseded |
Delegated to: | Simon Glass |
Headers | show |
Series | [RFC] binman: support mkimage split files | expand |
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)
On Thu, Mar 3, 2022 at 4:17 PM Alper Nebi Yasak <alpernebiyasak@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@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)
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 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)
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; 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. 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(-)