[U-Boot] [PATCH 1/2] include: android_bl_msg.h: Initial import
Eugeniu Rosca
roscaeugeniu at gmail.com
Thu May 9 01:41:44 UTC 2019
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.
More information about the U-Boot
mailing list