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