From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eugeniu Rosca Date: Thu, 9 May 2019 03:41:44 +0200 Subject: [U-Boot] [PATCH 1/2] include: android_bl_msg.h: Initial import In-Reply-To: References: <20190407220206.7655-1-erosca@de.adit-jv.com> <20190407220206.7655-2-erosca@de.adit-jv.com> <20190508144557.GA23415@vmlxhi-102.adit-jv.com> Message-ID: <20190509014144.GA11880@x230> 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, Thanks for the amazing effort to put the recent BCB/AB-related advancements together and make them work. I really look forward to seeing this part of mainline. Still, I have some concerns/questions and hope to get your feedback. First, the ("Implement Reboot reason support") series included in your "misc-reboot-reason" branch looks to be brand new, i.e. it hasn't been previously submitted for community review. I have comments in that area. Second, some review comments of mine posted in various patches of https://patchwork.ozlabs.org/cover/1044152/ ("[U-Boot,v3,0/7] android: implement A/B boot process") still haven't been addressed. Who is going to handle that work? Third, yes, there is more overlap than I initially expected. It is mostly between ("Add 'bcb' command to read/modify/write Android BCB") and ("Implement Reboot reason support"). The resolution is not as straightforward as one might assume. It is both a conflict of code and a conflict of perspective on how Android bootloader flow should be implemented in U-Boot. More on that later. Fourth, some words on commit order and split. I think the most natural commit order is the one reflecting the development of features in AOSP, i.e. I would expect getting the reboot reason to come before the A/B support, just b/c BCB structure with its "command" field existed in AOSP for ages (since the inception of "recovery" repository in 2008) while A/B support came _much_ (at least 7 years) later. WRT commit split, one comment is that we would potentially like to import getting reboot reason w/o importing A/B support. Surprisingly, this is not possible if the bootloader message header is glued to commit ("common: Implement A/B metadata"). So, the best order to me is: - add android_bl_msg.h in a standalone commit - add bcb command - add getting reboot reason (if needed at all, more on it later) - add A/B support - update platform support More comments below. On Wed, May 08, 2019 at 08:25:15PM +0300, Sam Protsenko wrote: > Hi Eugeniu, > > I created GitHub repo with all related patches applied on top of most > recent mainline U-Boot master: [1]. It contains next patches (on > "misc-reboot-reason" branch): > > 1. Series from Ruslan Trofymenko: [PATCH v3 0/7] android: implement > A/B boot process: > * cmd: part: Add 'number' sub-command > * disk: part: Extend API to get partition info > * common: Implement A/B metadata > * cmd: Add 'ab_select' command > * test/py: Add base test case for A/B updates > * doc: android: Add simple guide for A/B updates > * env: am57xx: Implement A/B boot process > 2. Series from Ruslan Trofymenko: [U-Boot][PATCH 0/4] Implement Reboot > reason support: > * common: Implement Reboot reason flow > * cmd: Add 'rb_reason' command > * env: am57xx: Add Reboot reason support > * configs: am57xx_evm: Enable Reboot reason support > * Rename android_bl_msg.h -> android_bl_msg2.h (my patch to make > this series apply along with next patches from Eugeniu) > 3. Series from Eugeniu Rosca: [PATCH 0/2] Add 'bcb' command to > read/modify/write Android BCB: > * include: android_bl_msg.h: Initial import > * cmd: Add 'bcb' command to read/modify/write BCB fields > 4. My local patches to enable it all on X15 board, make it work > correctly, and use original android_bl_msg.h (as close as possible to > AOSP file): > * configs: am57xx: Enable BCB command > * environment: ti: Fix USB controller number > * android: reboot reason: Use original android_bl_msg.h > * android: ab: Use original android_bl_msg.h > * android: Remove android_bl_msg2.h > * android: bcb: Use original android_bl_msg.h API > > With all those patches applied, I'm able to do next (after "adb reboot > bootloader" command): > 1. Use "bcb" command by Eugeniu: > => bcb load 1 4 "bcb load 1 misc" should be also supported, in case it helps. > => bcb dump command > 2. Use "rb_reason" command by Ruslan: > => rb_reason mmc 1:4 > => rb_reason mmc 1:misc Well, here we have two different perspectives. Below should be a 'bcb' equivalent (mostly pseudo code, not tested): if bcb load 1 misc; then # Valid BCB found if bcb test command = bootonce-bootloader; then bcb clear command; bcb store; # do the equivalent of $fastbootcmd else if bcb test command = boot-recovery; then bcb clear command; bcb store; # do the equivalent of $recoverycmd else # boot Android OS normally fi else # Corrupted/non-existent BCB # Boot non-Android OS? fi Here I see some room for discussion, since we have two approaches to getting the reboot reason and act accordingly. I'll point out some pros (+) and cons (-) in each case (IMHO): rb_reason: + compact when used (one-liner) - does much more than just reading the boot reason: - clears the 'command' field in BCB - runs $fastbootcmd/$recoverycmd, presumably populated beforehand => the above means that: - command name does not reflect its function, i.e. the name is misleading - U-Boot gets sprinkled by and its flow becomes dependent on a number of prerequisite environment variables (fastbootcmd/ recoverycmd). Boot flow becomes more complex and harder to comprehend/follow/debug. It's dispersed across several components communicating via environment variables (not at all centralized) - If we need to read any other BCB fields (status, recovery, stage) as part of Android boot flow management, we will need to either spawn new U-Boot commands or further obfuscate rb_reason. In comparison, bcb: - less compact when used (multi-line) + brings more transparency via sub-commands + frees the need for fastbootcmd/recoverycmd, i.e. centralizes Android boot flow management in the U-Boot environment. This is easier to read and debug. + can be used to take action based on the contents of other BCB fields The above is my subjective view and I am open for different opinions. > 3. U-Boot automatically gets into fastboot mode when "adb reboot > bootloader" command was issued > > Now we need to figure out how to do next (w.r.t. repo [1]): > 1. How to merge your "bcb" command and Ruslan's "rb_reason" command; > I can see they both are working with "misc" BCB. So maybe it's good > idea to merge them into one command. > 2. How to handle android_bl_msg.h: it's a dependency for all 3 patch > series (A/B, "reboot reason" command, BCB command). Maybe we should > rework it and send as a separate patch, mentioning why it's needed, > and after it's applied, we can re-send our patch series without that > file included. > > Please let me know what do you think. I guess I touched most of your comments. I look forward for your feedback! > > Thanks! Likewise! > > [1] https://github.com/joe-skb7/u-boot-misc/commits/misc-reboot-reason > > > -- > > Best Regards, > > Eugeniu.