Hi,
On Tue, Dec 3, 2019 at 9:29 PM Eugeniu Rosca <ero...@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
