[U-Boot] [PATCH v2 3/8] cmd: bootimg: Add bootimg command

[Eugeniu Rosca]

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/heads/master/include/bootimg/bootimg.h

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)

> +);

-- 
Best Regards,
Eugeniu