From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eugeniu Rosca Date: Tue, 21 May 2019 13:20:15 +0200 Subject: [U-Boot] [PATCH v2 2/2] cmd: Add 'bcb' command to read/modify/write BCB fields In-Reply-To: References: <20190517144503.3275-1-erosca@de.adit-jv.com> <20190517144503.3275-3-erosca@de.adit-jv.com> <20190520072257.GA4084@vmlxhi-102.adit-jv.com> Message-ID: <20190521112015.GA8578@vmlxhi-102.adit-jv.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 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 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.