On Thu May 19, 2022 at 9:36 PM AEST, Alper Nebi Yasak wrote: > Also see another attempt for this [1] and the comments to that for a > more complete picture, though I'll try writing all the points here anyway. > > [1] binman: support mkimage separate files > https://lore.kernel.org/u-boot/20220304195641.1993688-1-pgwipe...@gmail.com/ > > > --- > > This is a bit of a messy implementation for now and would probably break > > existing uses of mkimage that rely on the concatenation behaviour. > > I did a `git grep -C10 mkimage -- **/dts/*` and it doesn't look like > anything uses it yet. Except for binman/test/156_mkimage.dts, which > doesn't exactly test the concatenation.
I did similar and came to the same conclusion, but I figured I'd ask just in case it's used somewhere I hadn't found, or it's being relied on outside of the U-Boot repository. > You can add a 'separate-files' device-tree property like in [1]. I'm > actually OK with this separate-files being the default and only behavior > (concatenation can be done via an inner section), but I'm not sure Simon > would agree. Similar thoughts here, especially since we couldn't find anything relying on the concatenation behaviour. > > - What kind of test(s) should I add? > > At the minimum, a test using separate-files with multiple input entries. > You can do something like the _HandleGbbCommand in ftest.py to capture > and check the arguments that'll be passed to mkimage. > > I think that'll be enough, but try to run `binman test -T` and check for > 100% coverage with all tests succeeding. > > > (no changes since v1) > > > > tools/binman/etype/mkimage.py | 33 +++++++++++++++++++++------------ > > 1 file changed, 21 insertions(+), 12 deletions(-) > > > > diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py > > index 5f6def2287..8cea618fbd 100644 > > --- a/tools/binman/etype/mkimage.py > > +++ b/tools/binman/etype/mkimage.py > > @@ -51,21 +51,30 @@ class Entry_mkimage(Entry): > > Expand the docstring with an explanation of the new behavior, and run > `binman entry-docs >tools/binman/entries.rst` to update it there as well. > > > self.ReadEntries() > > > > 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) > > - if data is None: > > - return False > > - output_fname = tools.get_output_filename('mkimage-out.%s' % uniq) > > - if self.mkimage.run_cmd('-d', input_fname, *self._args, > > - output_fname) is not None: > > + # For multiple inputs to mkimage, we want to separate them by > > colons. > > + # This is needed for eg. the rkspi format, which treats the first > > data > > + # file as the "init" and the second as "boot" and sets the image > > header > > + # accordingly, then makes the image so that only the first 2 KiB > > of each > > + # 4KiB block is used. > > + > > + data_filenames = [] > > + for entry in self._mkimage_entries.values(): > > + # First get the input data and put it in a file. If any entry > > is not > > + # available, try later. > > + if not entry.ObtainContents(): > > + return False > > + > > + input_fname = tools.get_output_filename('mkimage-in.%s' % > > entry.GetUniqueName()) > > + data_filenames.append(input_fname) > > + tools.write_file(input_fname, entry.GetData()) > > Something like collect_contents_to_file([entry], f'mkimage-in-{idx}', > 1024) would be better here. At least, the files must not be empty (or > mkimage exits with an error), where entry.GetData() can be b''. > > > + > > + output_fname = tools.get_output_filename('mkimage-out.%s' % > > self.GetUniqueName()) > > Should be an f-string. > > > + if self.mkimage.run_cmd('-d', ":".join(data_filenames), > > *self._args, output_fname): > > self.SetContents(tools.read_file(output_fname)) > > + return True > > else: > > - # Bintool is missing; just use the input data as the output > > self.record_missing_bintool(self.mkimage) > > - self.SetContents(data) > > - > > - return True > > + return False > > This case must set some dummy contents (I'd guess b'' is fine) and > return True. (False here roughly means "try again later".) > > > > > def ReadEntries(self): > > """Read the subnodes to find out what should go in this image""" Thanks for your review! I'll make these updates in the next version.