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