Hi,
On Wed, Dec 4, 2019 at 8:25 PM Eugeniu Rosca erosca@de.adit-jv.com wrote:
Hi again,
[I would be willing to take this discussion offline, if you consider it too noisy for ML]
Agreed. Please find me on freenode IRC, nick: joeskb7. There is #u-boot channel, or #linaro-android, whichever suits you best.
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"" - get header version\n""bootimg get_dtbo <addr_var> [size_var]\n"" - 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"I now see that "DTBO area" is used intermixed with "DTB area". I think it makes sense to use one of the two consistently and drop the other. Otherwise, users might think there are two distinct areas in the same Android image.
Those are, in fact, two different areas in boot.img. "DTBO area" is a payload in boot.img where recovery dtbo file can be placed. "DTB area" is a payload in boot.img where DTB files can be placed (either concatenated or wrapped into DTBO image format).
"bootimg dtb_load_addr <varname>\n"" - get load address (hex) of DTB\n"This is actually not something retrieved from the DTB/DTBO area, but from the "dtb_addr" field of the Android Image itself, as defined in https://android.googlesource.com/platform/system/tools/mkbootimg/+/refs/head...
I think this little bit of information is essential, but not sure how to make it more transparent to the user, since purely based on the available help message, I tend to infer that there is connection between "DTB" in "get load address (hex) of DTB" and the "DTBO area", while there is no connection whatsoever.
Let's keep command usage length reasonable. We are bloating U-Boot footprint too much as it is already... I think user shouldn't care where the load address is obtained from, really. Prefer to keep it short, as it is.
"bootimg get_dtb_file <index> <addr_var> [size_var]\n"" - 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"Would it make more sense to use "DTB entry" instead of "DTB file" since this is the wording used in the Google spec/header?
I'd argue that "DTB file" is more clear for user, than "entry". If you don't have a strong preference on this one, let's keep it as is.
Another general comment regarding the current sub-commands:
- set_addr
- ver
- get_dtbo
- dtb_dump
- dtb_load_addr
- get_dtb_file
I observe the following (inconsistent) pattern:
- <do>_<what>
<what>
- <do>_<what>
- <what>_<do>
- <what>_<do>_<what>
- <do>_<what>
Looking at the "fdt" command, I find its sub-commands somehow better partitioned and easier to digest/remember:
fdt addr [-c] <addr> [<length>] fdt apply <addr> fdt move <fdt> <newaddr> <length> fdt resize [<extrasize>] fdt print <path> [<prop>] fdt list <path> [<prop>] fdt get value <var> <path> <prop> fdt get name <var> <path> <index> fdt get addr <var> <path> <prop> fdt get size <var> <path> [<prop>] fdt set <path> <prop> [<val>] fdt mknode <path> <node> fdt rm <path> [<prop>] fdt header [get <var> <member>]
Its syntax seems to be: <command> <do> <what> [options]
Would it make sense to borrow this naming style/principle? It could translate to the following for abootimg:
abootimg (current sub-command name enclosed in brackets):
- addr (set_addr)
- ver
- dump dtbo (dtb_dump)
- get dtbo (get_dtbo)
- get dtbe (get_dtb_file)
- get dtla (dtb_load_addr)
Makes sense. I'll think about it.
Thanks for review!
+);
-- Best Regards, Eugeniu