On 06/03/2022 17:44, Peter Geis wrote:
> On Sat, Mar 5, 2022 at 10:08 PM Simon Glass <s...@chromium.org> wrote:
>> On Fri, 4 Mar 2022 at 12:56, Peter Geis <pgwipe...@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 <pgwipe...@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.
