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

[Sam Protsenko]

Hi,

On Wed, Dec 4, 2019 at 8:25 PM Eugeniu Rosca <erosca at 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/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.
>

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