[PATCH v2] binman: support mkimage separate files

[Alper Nebi Yasak]

On 06/03/2022 17:44, Peter Geis wrote:
> On Sat, Mar 5, 2022 at 10:08 PM Simon Glass <sjg at chromium.org> wrote:
>> On Fri, 4 Mar 2022 at 12:56, Peter Geis <pgwipeout at gmail.com> wrote:
>>> mkimage has the ability to process two files at the same time.
>>> This is necessary for rk356x support as both TPL and SPL need to be
>>> hashed individually in the resulting header.
>>> It also eases support for rkspi as mkimage handles everything for making
>>> the requisite file for maskrom loading.
>>
>> This makes me wonder if we should move that functoinality out of
>> mkimage and into binman?

I think we should have entry types for the formats mkimage supports,
even if they're just stubs that run 'mkimage -T <type>'. Primarily
because I don't see 'mkimage' as a meaningful 'type' of an entry, but I
don't know if there's a common format to all the types it supports.
Creating stub entries would also let us switch to native implementations
one by one if we want that later.

> Rockchip rk32 and rk33 maskrom loading from SPI has a bug that causes
> it to only load 2048 bytes out of each 4096 byte chunk.
> RKSPI splits the TPL/SPL (the portion directly loaded by maskrom) into
> 2048 chunks then pads each chunk with blank space so the image can
> load correctly.
> While it could be moved to binman, as far as I'm aware this is a
> corner case and I don't know any other chip that would need this
> functionality.

Do you know if rk35xx chips have the same bug? Does rkspi also do the
weird chunking for those when it maybe doesn't need to?

>>> Add a new flag "separate_files" for mkimage handling to gather the files
>>> separately rather than combining them before passing them to mkimage.
>>>
>>> Signed-off-by: Peter Geis <pgwipeout at gmail.com>
>>> ---
>>> Changelog:
>>> v2:
>>> I've managed to move this all into mkimage.py as per Alper's suggestion.
>>> I've added an example to the readme portion of the function.
>>> mkimage,separate_files is now separate-files.
>>>
>>>  tools/binman/etype/mkimage.py | 41 ++++++++++++++++++++++++++++++++---
>>>  1 file changed, 38 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py
>>> index 5f6def2287f6..0a86f904a2b7 100644
>>> --- a/tools/binman/etype/mkimage.py
>>> +++ b/tools/binman/etype/mkimage.py
>>> @@ -17,6 +17,7 @@ class Entry_mkimage(Entry):
>>>      Properties / Entry arguments:
>>>          - datafile: Filename for -d argument
>>>          - args: Other arguments to pass
>>> +        - separate-files: Boolean flag for passing files individually.
>>>
>>>      The data passed to mkimage is collected from subnodes of the mkimage node,
>>>      e.g.::
>>> @@ -42,20 +43,54 @@ class Entry_mkimage(Entry):
>>>              };
>>>          };
>>>
>>> +    This calls mkimage to create a rksd image with a standalone ram init
>>> +    binary file and u-boot-spl.bin as individual input files. The output from
>>> +    mkimage then becomes part of the image produced by binman.
>>> +
>>> +        mkimage {
>>> +            args = "-n", CONFIG_SYS_SOC, "-T", "rksd";
>>> +            separate-files;
>>> +
>>> +            ddrl {
>>> +                type = "blob-ext";
>>> +                filename = "rk3568_ddr_1560MHz_v1.12.bin";
>>> +            };
>>> +
>>> +            u-boot-spl {
>>> +            };
>>> +        };
>>> +
>>>      """
>>>      def __init__(self, section, etype, node):
>>>          super().__init__(section, etype, node)
>>>          self._args = fdt_util.GetArgs(self._node, 'args')
>>> +        self._separate = fdt_util.GetBool(self._node, 'separate-files')
>>>          self._mkimage_entries = OrderedDict()
>>>          self.align_default = None
>>>          self.ReadEntries()
>>> +        self.index = 0
>>> +        self.fname_tmp = str()

These two wouldn't be useful for anything except the function below, so
should be defined there as local variables.

>>>
>>>      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:
>>> +        if self._separate is False:
>>> +          data, input_fname, uniq = self.collect_contents_to_file(
>>> +              self._mkimage_entries.values(), 'mkimage', 1024)
>>> +          if data is None:
>>>              return False
>>> +        else:
>>> +          for entry in self._mkimage_entries.values():
>>
>> We can do:
>>
>> for index, entry in enumerate(self._mkimage_entries.values()):
>>
>> then you don't need self.index
> 
> Thanks!
> 
>>
>>> +            self.index = (self.index + 1)
>>> +            if (self.index > 2):
>>> +              print('BINMAN Warn: mkimage only supports a maximum of two separate files')
>>
>> Can we use self.Raise() here instead? It seems like a fatal error.
>> Also this check should go in ReadNode() since we don't want to die
>> this late in the picture if we know it is wrong upfront. (BTW I am
>> moving the node-reading code to ReadNode() in my v3 series as
>> suggested by Alper).
> 
> Certainly, this really would be a fatal error.

(The "BINMAN Warn:" prefix should also be dropped with self.Raise())

> 
>>
>>> +              return False
>>> +            input_fname = ['mkimage', str(self.index)]
>>> +            data, input_fname, uniq = self.collect_contents_to_file(
>>> +                    [entry], ".".join(input_fname), 1024)

This could have f"mkimage.{index}" as the prefix argument instead.

>>
>> I suppose we can just use
>>
>>    data = entry.GetData()
> 
> We don't actually use data directly here, collect_contents_to_file
> collects the data into separate files and passes the file name back.
> data is just used to tell if that function failed, the file names are
> what we care about.
> Really as far as I can tell collect_contents_to_file doesn't need to
> pass data back at all, because input_fname and uniq would be returned
> False as well and either could be used for this check.
> uniq is also used later on (I checked, each time returns the same
> value so clobbering it in each iteration doesn't cause issues).

Yeah, I think collect_contents_to_file() is better here for the
one-entry case as well. The alternative is doing exactly everything it
does manually.

>>
>> here?
>>
>>> +            if data is None:
>>> +              return False
>>> +            self.fname_tmp = [''.join(self.fname_tmp),input_fname]
>>> +          input_fname = ":".join(self.fname_tmp)

Instead of these, I'd set input_fnames = [] before the for loop, use
input_fnames.append(input_fname) inside the loop to collect the names,
then ":".join(input_fnames) to build the argument.

>>>          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:
>>> --
>>> 2.25.1
>>>
>>
>> Looks OK to me, needs a test or two.
>>
>> Regards,
>> Simon
> 
> Honestly, if you can implement this better than I did in your series, please do.
> As I said previously, all the python I know now I learned to make this
> happen, so I imagine it is not optimal.

Well, you're 90% there already (with the other 90% being the tests...),
but if you don't feel like working on this, tell me and I can do so in a
few days.