[U-Boot] [PATCH v2 3/8] cmd: bootimg: Add bootimg command

[Sam Protsenko]

Hi,

On Tue, Dec 3, 2019 at 9:29 PM Eugeniu Rosca <erosca at de.adit-jv.com> wrote:
>
> Hi Sam,
> Cc: Aleksandr, Roman
>
> As expressed in the attached e-mail, to minimize the headaches extending
> the argument list of "bootimg" in future, can we please agree on below?
>
> On Wed, Oct 23, 2019 at 05:34:22PM +0300, Sam Protsenko wrote:
> > +U_BOOT_CMD(
> > +     bootimg, CONFIG_SYS_MAXARGS, 0, do_bootimg,
> > +     "manipulate Android Boot Image",
> > +     "set_addr <addr>\n"
> > +     "    - set the address in RAM where boot image is located\n"
> > +     "      ($loadaddr is used by default)\n"
> > +     "bootimg ver <varname>\n"
>
> Can we make <varname> optional, with the background provided in [1]?
>

Already done in v3 (will send soon).

> > +     "    - get header version\n"
> > +     "bootimg get_dtbo <addr_var> [size_var]\n"
>
> How about converting <addr_var> to an optional argument too?
>

Already done in v3.

> > +     "    - get address and size (hex) of recovery DTBO area in the image\n"
> > +     "      <addr_var>: variable name to contain DTBO area address\n"
> > +     "      [size_var]: variable name to contain DTBO area size\n"
> > +     "bootimg dtb_dump\n"
> > +     "    - print info for all files in DTB area\n"
> > +     "bootimg dtb_load_addr <varname>\n"
>
> Same as above w.r.t. <varname>.
>

Already done in v3.

> > +     "    - get load address (hex) of DTB\n"
> > +     "bootimg get_dtb_file <index> <addr_var> [size_var]\n"
>
> How about converting <addr_var> to an optional argument too?

Already done in v3.

> How do you foresee getting a blob entry based on id=<i> and/or
> rev=<r>? Which of below is your favorite option?
>
>  - get_dtb_file <index> [--id=<i>] [--rev=<r>] [addr_var] [size_var]
>    where: - in case <i> and/or <r> are provided, <index> tells the
>             command to pick the N's entry in the selection filtered by
>             <i> and <r>
>           - in case neither <i> nor <r> is provided, <index>
>             behaves like in the current patch (selects a blob entry
>             found at absolute index value ${index} in the image)
>
>  - get_dtb_file [<index>|[--id=<i>|--rev=<r>]] [addr_var] [size_var]
>    To make it clear, some example commands would be:
>    - get_dtb_file <index>
>      => current behavior
>    - get_dtb_file --id=<i>
>      => get _first_ blob entry matching id value <i>
>    - get_dtb_file --rev=<r>
>      => get _first_ blob entry matching rev value <r>
>    - get_dtb_file --id=<i> --rev=<r>
>      => get _first_ blob entry matching id <i> _and_ rev=<r>
>    - get_dtb_file <index> --id=<i>
>    - get_dtb_file <index> --rev=<r>
>      => Wrong usage
>
>  - get_dtb_file  anything else?
>

I already came up with next usage:

    abootimg get_dtb_file index <num> [addr_var] [size_var]

It's already done in v3. Once your patch series is merged, I will add
next two sub-commands:

    abootimg get_dtb_file id    <num> [addr_var] [size_var]
    abootimg get_dtb_file rev   <num> [addr_var] [size_var]

This way it's extensible. Also please see my review for your patches,
esp. for patch 4/4, where I discuss similar matter in context of
'dtimg' command.

> I think it is crucial to agree on the above, since the very first
> revision of "bootimg"/"abootimg" may impose strict limitations on how
> the command can be extended in future.
>

All those are addressed in v3 now. Along with renaming: 'bootimg' -> 'abootimg'.

The only thing remains to be addressed is Kconfig/Makefile decisions
you mentioned. Will look into that tomorrow, and either make it as you
pointed out or explain why it's good as it is.

> [just noticed your reply shedding more light on a subset of these
> questions, but still sending this out; please skip if already answered]
>
> > +     "    - get address and size (hex) of DTB file in the image\n"
> > +     "      <index>: index of desired DTB file in DTB area\n"
> > +     "      <addr_var>: variable name to contain DTB file address\n"
> > +     "      [size_var]: variable name to contain DTB file size\n"
> > +);
>
> [1] https://patchwork.ozlabs.org/patch/1202579/
>     ("cmd: dtimg: Make <varname> an optional argument")
>
> --
> Best Regards,
> Eugeniu