diff mbox series

[v2] binman: support mkimage separate files

Message ID 20220304195641.1993688-1-pgwipeout@gmail.com
State Changes Requested
Delegated to: Simon Glass
Headers show
Series [v2] binman: support mkimage separate files | expand

Commit Message

Peter Geis March 4, 2022, 7:56 p.m. UTC
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.

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@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(-)

Comments

Simon Glass March 6, 2022, 3:08 a.m. UTC | #1
Hi Peter,

On Fri, 4 Mar 2022 at 12:56, Peter Geis <pgwipeout@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?

>
> 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@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()
>
>      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

> +            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).

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

I suppose we can just use

   data = entry.GetData()

here?

> +            if data is None:
> +              return False
> +            self.fname_tmp = [''.join(self.fname_tmp),input_fname]
> +          input_fname = ":".join(self.fname_tmp)
>          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
Peter Geis March 6, 2022, 2:44 p.m. UTC | #2
On Sat, Mar 5, 2022 at 10:08 PM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Peter,

Good Morning,

>
> On Fri, 4 Mar 2022 at 12:56, Peter Geis <pgwipeout@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?

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.

>
> >
> > 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@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()
> >
> >      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.

>
> > +              return False
> > +            input_fname = ['mkimage', str(self.index)]
> > +            data, input_fname, uniq = self.collect_contents_to_file(
> > +                    [entry], ".".join(input_fname), 1024)
>
> 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).

>
> here?
>
> > +            if data is None:
> > +              return False
> > +            self.fname_tmp = [''.join(self.fname_tmp),input_fname]
> > +          input_fname = ":".join(self.fname_tmp)
> >          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.

Very Respectfully,
Peter
Alper Nebi Yasak March 10, 2022, 7:29 p.m. UTC | #3
On 06/03/2022 17:44, Peter Geis wrote:
> On Sat, Mar 5, 2022 at 10:08 PM Simon Glass <sjg@chromium.org> wrote:
>> On Fri, 4 Mar 2022 at 12:56, Peter Geis <pgwipeout@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@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.
Peter Geis March 10, 2022, 11:19 p.m. UTC | #4
On Thu, Mar 10, 2022 at 2:36 PM Alper Nebi Yasak
<alpernebiyasak@gmail.com> wrote:
>
> On 06/03/2022 17:44, Peter Geis wrote:
> > On Sat, Mar 5, 2022 at 10:08 PM Simon Glass <sjg@chromium.org> wrote:
> >> On Fri, 4 Mar 2022 at 12:56, Peter Geis <pgwipeout@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?

From my limited testing, no rk35xx seems to have fixed this bug.
Thus this series gives me hope for "unified" images for SPI and MMC
where the offsets are the same, and the only thing that changes is the
TPL/SPL actually loaded in place.

>
> >>> 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@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.

Thanks!

>
> >>>
> >>>      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 have no idea what that means.

>
> >>
> >> 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.

Yeah V1 actually implemented that all in a modified
collect_contents_to_file(), so this is much simpler already.

>
> >>
> >> 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.

Ah, there's the little python details I don't know about yet.

>
> >>>          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.

If you would like to, please do!
Otherwise, I will continue stumbling around with it once this series
settles down.

Thanks,
Peter
diff mbox series

Patch

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()
 
     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():
+            self.index = (self.index + 1)
+            if (self.index > 2):
+              print('BINMAN Warn: mkimage only supports a maximum of two separate files')
+              return False
+            input_fname = ['mkimage', str(self.index)]
+            data, input_fname, uniq = self.collect_contents_to_file(
+                    [entry], ".".join(input_fname), 1024)
+            if data is None:
+              return False
+            self.fname_tmp = [''.join(self.fname_tmp),input_fname]
+          input_fname = ":".join(self.fname_tmp)
         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: