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

[Sam Protsenko]

Hi Aleksandr,

On Thu, Dec 5, 2019 at 4:17 AM Aleksandr Bulyshchenko
<a.bulyshchenko at globallogic.com> wrote:
>
> 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.
>

Agreed on id+rev usage. Can we still try and keep the interface as
simple as possible, e.g. like this:

    dtimg start index <num> <addr> [varname]
    dtimg start id <id_num> <rev_num> <custom_num> <addr> [varname]

In case when user wants to use only "id" or only  "rev", other bit
should be specified as "-":

    dtimg start id 10 - - <addr> [varname]
    dtimg start id - 10 - <addr> [varname]

It's similar to what is done in 'bootm' command:

    To boot that kernel without an initrd image,use a '-' for the
second argument.

This way we can keep away the 'index' usage, making two things possible:
  1. Code will be easier (we can provide one function for 'index' case
and one function for 'id/rev/custom' case)
  2. Easier for us to split the work and avoid dependencies between
our patch series

If everyone agrees on usage I suggested above, we can go ahead and
change it correspondingly for 'dtimg' and 'abootimg' patch series.

> 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