[U-Boot] [PATCH v4 2/2] cmd: Add dtimg command

[Eugeniu Rosca <roscaeugeniu@gmail.com>]

Hi Roman,
(CC-ing Igor for Android topics)

On Wed, Nov 13, 2019 at 12:19:59PM +0200, Roman Stratiienko wrote:
> On Tue, Nov 12, 2019 at 8:18 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> >
> > Hello Sam,
> >
> > On Thu, Aug 16, 2018 at 11:34:13PM +0300, Sam Protsenko wrote:
> > > dtimg command allows user to work with Android DTB/DTBO image format.
> > > Such as, getting the address of desired DTB/DTBO file, printing the dump
> > > of the image in U-Boot shell, etc.

[..]

> > Since you are the author and the main stakeholder of "dtimg", could you
> > kindly feedback the command usage you envision for getting the start and
> > size of dtb/dtbo blob given a certain "id" and "rev" fields used by
> > mkdtboimg.py [1] and visible in the output of U-Boot's "dtimg dump" [2]?
> >
> > One option would be to extend the existing "dtimg {start|size}" to
> > accept an argument like "id:<val>" and "rev:<val>".
> >
> > Another possibility is to create brand new dtimg sub-command.
> > What would be your preference? TIA.
> >
> > [1] https://android.googlesource.com/platform/system/libufdt/+/master/utils/src/mkdtboimg.py
> > [2] https://gitlab.denx.de/u-boot/u-boot/commit/e63bf1b13b3a7a
> 
> Let me add some background information to clarify why new command was suggested.
> We came up with this during brainstorming on what is the best way to
> implement lookup fdt by id and rev fields.
> 
> First suggestion was to implement separate lookup command, e.g.:
> 
> --> dtimg lookup id:<board_id> <dt_image_index_variable>
> 
> Second one was to integrate it into existing start/size command to
> make command look more natural:
> 
> --> dtimg start|size <addr> [<index>|id:<id>] <varname>
> 
> Then after some time I suggested to combine 'start/size' subcommands
> into single 'range' subcommand:
> 
> --> dtimg range <addr> id:<id> [rev:<rev>] [<start_varname> [<size_varname>]]
> --> dtimg range <addr> index:<index> [<start_varname> [<size_varname>]]
> 
> Benefits of such combining:
> - Reduce chance of human mistake in between of this commands (for
> example different values for start and size).
> - Increase readability and slightly reduce parsing time.
> 
> Downsides:
> This solution is a little revolutionary since it requires to start
> long-term deprecation process of start/size subcommands.

So, what you are proposing is to migrate away from the "start" and
"size" sub-commands in the long run. While I understand the good
intentions, let me share my opinion what this means for platform
maintainers like us.

As a platform maintainer, it is not uncommon to encounter a scenario
like below:
 - platform X reached "feature freeze" one year ago with U-Boot v2018.11
 - platform Y reaches "feature freeze" soon with U-Boot v2020.01

If "dtimg" is used in both platforms and the command usage is not
backward compatible, this generates all kind of overhead like the need
to maintain two different versions of boot scripts (in case you keep
track of them externally as text files, which is what we do).

This is hugely simplified involving one single command. Imagine
several U-Boot commands fundamentally changing their usage pattern
over time. Things would become pretty messy.

> 
> So the main question to the community is next:
> Are we ready to make long term deprecation in the name of benefits
> mentioned above?

I hope more people will feedback on that, but in my opinion, we have to
honor the existing "dtimg" usage as a blueprint and carefully add
backward-compatible changes to it. This roughly translates to, IMHO:
 - in case of adding a brand new sub-command, it must not overlap in
   its function with any of pre-existing sub-commands
 - in case of enriching/extending a pre-existing sub-command, it only
   sounds appropriate to me adding one or more _optional_ arguments to
   maintain backward compatibility

Both above points can be met if "dtimg {start|size}" is modified as
follows, to account for the "id" and "rev" fields:

 -> dtimg start|size <addr> <index> [id:<id>] [rev:<rev>] <varname>

Some usage examples derived from the above proposal:

 -> dtimg start|size <addr> <index> <varname> 
    current behavior
 -> dtimg start|size <addr> <index> id:<id> <varname> 
    get the "start|size" of the blob carrying "id" value <id>. Assuming
    there are several such blobs in the image, get the <index>th one
 -> dtimg start|size <addr> <index> rev:<rev> <varname> 
    get the "start|size" of the blob carrying "rev" value <rev>.
    Assuming there are several such blobs, get the <index>th one
 -> dtimg start|size <addr> <index> id:<id> rev:<rev> <varname> 
    get the "start|size" of the blob carrying "id" value <id> AND "rev"
    value <rev>. Assuming several such blobs, get the <index>th one

> 
> In case not - we have no other choice but to extend existing
> start/size subcommands.

-- 
Best Regards,
Eugeniu