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

[Aleksandr Bulyshchenko]

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.

The same remains relevant for abootimg as well.

Thanks,
Aleksandr Bulyshchenko

On Wed, Dec 4, 2019 at 9:12 PM Sam Protsenko <semen.protsenko at linaro.org>
wrote:

> Hi,
>
> On Wed, Dec 4, 2019 at 7:33 PM Eugeniu Rosca <erosca at 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
>