[U-Boot] [PATCH v2 2/2] cmd: Add 'bcb' command to read/modify/write BCB fields

[Eugeniu Rosca]

Hi Sam,

On Mon, May 20, 2019 at 06:16:38PM +0300, Sam Protsenko wrote:
> Hi Eugeniu,
> 
> On Mon, May 20, 2019 at 10:23 AM Eugeniu Rosca <erosca at de.adit-jv.com> wrote:
[..]
> > Igor, Sam, what's your view on the above? Would you suggest creating
> > a doc/README.android-bcb or there is a more elegant solution to it?
> >
> 
> Documentation would be nice. Although you already provided a generic
> description of 'bcb' command in 'help bcb', the user often wants to
> read more high-level documentation. I'd say, you can copy the
> description from commit message to doc/README.android-bcb, extending
> it with usual use-cases, and how this command can be used in those
> use-cases. For example, your pseudo-code for reboot reason you
> provided to me earlier, etc.

Agreed. In my queue.

> Also, it can be useful to mention if
> Google have any requirements (mandatory or optional) for the
> bootloader (misc partition, reboot reason, recovery, etc), and how
> this 'bcb' command can help in those requirements implementation.

Well, a subset of the Android bootloader requirements is publicly
available via https://source.android.com/devices/bootloader, but I saw
traces of the "Android Boot/Bootloader Requirements" document name
mentioned in the descriptions of U-Boot commits received from our
suppliers. I _do not_ have access to the latter. If anybody from Google
sees this message and can share the document or a subset of it, that
would be great.

To be clear, the background behind the 'bcb' command is not because I
was assigned the task to implement any of the Android bootloader
requirements, but because I saw improper implementations of some of
those requirements in the patches received from our supplier and
wanted to showcase a better/U-Boot-compliant way to accomplish those.

So, I don't give any commitment to support the Android-related parts in
U-Boot, but will signal and express my opinion whenever any features
backported from AOSP don't follow the patterns established in community.

> 
> All that said, IMHO documentation/test wise: it's not critical in this
> case, you can add that later, just to speed-up the whole development
> process and divide it into iterations. But that's for maintainers to
> decide, of course.
> 
> Also, I've a feeling that we have another problem, more common than
> just a documentation. At the moment we have a bunch of Android related
> features, which don't have namespace separation on several levels:
>  - file/directories: we may want to move all Android related commands
> to sub-directory
>  - commands: we may want to add main command called "android" for all
> Android-related commands, or maybe just a prefix
>  - Kconfig: we may want to have some generic naming scheme for all
> Android-related commands
>  - Documentation, tests: the same here
> 
> So at some point we will probably need to discuss and fix that
> somehow. Not here, of course :)

Agree with the most of the bullets. However, WRT merging all the
available Android commands into a single command, I see room for
more discussion. Here is some valuable feedback from Ruslan Trofymenko:

 --- quote from https://patchwork.ozlabs.org/patch/1004002/#2049412 ---
On Thu, 6 Dec 2018 at 03:31, Simon Glass <sjg at chromium.org> wrote:
> Perhaps we need a new 'android' command with an 'ab_select'
> subcommand? Then the automatica abbreviation will work.
>

Agree with all your previous comments, will send v2 shortly. But this
one I'd like to leave as is (I will drop android_ prefix though). I
think adding "android" command may lead to unneeded architecture
complexity in future, e.g.:
  - we will need to re-work next commands as sub-commands of "android"
command: fastboot, dtimg, mmc_swrite (which can be hard as ab_select
command file has BSD license and other commands have GPL license)
 - we will need to implement sub-sub-commands (e.g. for "android dtimg
dump ..." etc.)
 - having "android" command can be inconvenient for users and may
break existing API (e.g. it will force users to use "android fastboot"
instead of just "fastboot" command)
 - actually I don't see any namespace issues that can be solved by
adding "android" command

Also, in subsequent patch, I will add the dedicated menu option for
Android-related commands and will pull all existing Android commands
(along with ab_select) to that menu entry.

So I hope it's fine with you if I leave this command as "ab_select"?
 --- end quote https://patchwork.ozlabs.org/patch/1004002/#2049412 ---

In addition, based on the feedback from Alex Kiernan, 'bcb' might be
useful in non-Android contexts. If so, then embedding it as sub-command
into a higher level command (e.g. 'android') does not make much sense.

-- 
Best Regards,
Eugeniu.