Hi again,
[I would be willing to take this discussion offline, if you consider it too noisy for ML]
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.
- "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.
- "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?
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)
+);