Hi Aleksandr,
On Thu, Dec 5, 2019 at 4:17 AM Aleksandr Bulyshchenko a.bulyshchenko@globallogic.com wrote:
Hello Sam,
I'd like to add my 5 cents regarding separating dtimg start|size into 3 subcommands
dtimg start index <num> <addr> [varname] dtimg start id <num> <addr> [varname] dtimg start rev <num> <addr> [varname]
While I don't see real usecases for combining index with id or rev (if someone applies metainformation to dtb entries for meaningful lookup, identical entries most probably mean copy-paste error), but at the same time I see space for at least two-factor identification (e.g. model and revision). Thus API should allow (but not require) combining id and rev.
Agreed on id+rev usage. Can we still try and keep the interface as simple as possible, e.g. like this:
dtimg start index <num> <addr> [varname] dtimg start id <id_num> <rev_num> <custom_num> <addr> [varname]
In case when user wants to use only "id" or only "rev", other bit should be specified as "-":
dtimg start id 10 - - <addr> [varname] dtimg start id - 10 - <addr> [varname]
It's similar to what is done in 'bootm' command:
To boot that kernel without an initrd image,use a '-' for the second argument.
This way we can keep away the 'index' usage, making two things possible: 1. Code will be easier (we can provide one function for 'index' case and one function for 'id/rev/custom' case) 2. Easier for us to split the work and avoid dependencies between our patch series
If everyone agrees on usage I suggested above, we can go ahead and change it correspondingly for 'dtimg' and 'abootimg' patch series.
The same remains relevant for abootimg as well.
Thanks, Aleksandr Bulyshchenko
On Wed, Dec 4, 2019 at 9:12 PM Sam Protsenko semen.protsenko@linaro.org wrote:
Hi,
On Wed, Dec 4, 2019 at 7:33 PM Eugeniu Rosca erosca@de.adit-jv.com wrote:
Hello Sam, Please, see one more suggestion below.
On Tue, Dec 03, 2019 at 08:29:10PM +0100, Eugeniu Rosca 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]?
- " - get header version\n"
- "bootimg get_dtbo <addr_var> [size_var]\n"
How about converting <addr_var> to an optional argument too?
- " - 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>.
- " - get load address (hex) of DTB\n"
- "bootimg get_dtb_file <index> <addr_var> [size_var]\n"
How about "get_dte" or "get_dtbe" instead of "get_dtb_file" ? It's shorter and should be easier to remember (dt{b}e = DT{B} Entry).
Sorry, I like get_dtb more. It's .dtb file in the end, and it's called exactly "dtb" in boot.img struct. So this is a keeper :)
-- Best Regards, Eugeniu