[U-Boot] [RFC] env: ti: Handle reboot reason from BCB

Sun Jun 30 18:49:34 UTC 2019

Hi Sam,

All below is my 2 cents and FWIW, so feel free to just skip it.

On Fri, Jun 21, 2019 at 12:25:44AM +0300, Sam Protsenko wrote:
> *** PLEASE DO NOT MERGE.
> *** This is only RFC, a discussion thread. Patch is only for the
> *** reference here right now, to create a discussion context.
> 
> There are 2 possible reboot use-cases:
>   1. User did "fastboot reboot bootloader" (we're setting "dofastboot"
>      variable as an indicator in this case)
>   2. User did "adb reboot bootloader" or "reboot bootloader" from Linux
>      shell (Android sets "bootonce-bootloader" string into command field
>      of BCB area inside of 'misc' partition as an indicator in this
>      case)

The two requirements above seem to serve the same purpose. It is a bit
unfortunate that each comes with its own implementation and its own
assumption of which non-volatile memory is available to pass the
"reboot into bootloader" message to the bootloader. Still, I can
understand the choice to treat the two independently. It gives
the most flexibility to the user in terms of which features (FB, BCB,
etc) to include in U-Boot, with still getting the expected behavior.

> 
> We already had (1) handling implemented. This patch adds implementation
> of (2). Possible drawbacks are:
>   - user is able to re-define 'preboot' variable and break this
>     mechanism this way

It is also my understanding that ${preboot} was designed to be
overridden at runtime [2]. Looking at the help message of
"config USE_PREBOOT":

 ----8<----
 This feature is especially useful when "preboot" is automatically
 generated or modified. For example, the boot code can modify the
 "preboot" when a user holds down a certain combination of keys.
 ----8<----

In conformance with the help message, we use PREBOOT on R-Car platform
to enter fastboot/bootloader mode on pressing a dedicated key when
{cold,re}-booting the Renesas reference targets.

The above observations may lead to the following conclusions (IMHO):
 - storing non-trivial logic into CONFIG_PREBOOT might play not so well
   with the purpose of the preboot feature. With a complicated flow
   embedded into CONFIG_PREBOOT, users [2] would potentially need to
   extract and add subsets of CONFIG_PREBOOT to ${preboot} at runtime
   and I think this would be very inconvenient.
 - in a more general sense, making use of CONFIG_PREBOOT is probably not
   a good idea at all if there is no need to alter ${preboot} at
   runtime. Yes, there might be some temptation to do things prior to
   the start of the BOOTDELAY countdown [3], but I think that
   the primary use-case of ${preboot} is to handle potential dynamic
   HW changes (e.g. key press) in the pre-main() boot phase.

>   - performance penalty due to scripting usage in 'preboot'

W/o any measurements at hand, I think there is not enough evidence
from which a boot time penalty could be inferred. Are there any numbers
backing up your concern?

> 
> DISCUSSION ITEM 1: possible solution to those can be checking BCB area
> (and 'dofastboot' variable) in C code, somewhere in board file, like
> in late_init() function, and on;y set 'preboot' variable from there,
> if conditions are met, to something like this:
> 
>     preboot=fastboot 1; setenv preboot;
> 
> so that next time 'preboot' variable will be empty. But this way also
> has its shortcomings, like the necessity of board file modification,
> need of BCB C API exposure, etc.

I personally think that calling BCB command (and any other U-Boot shell
command) in C (using run_command() and friends) to some degree defeats
the purpose of having those commands implemented and exposed to the
user. At least in our development environment, we see U-Boot as being
nicely composed of small building blocks (shell commands), which we try
to glue together via externally stored/developed *.scr files. It is my
impression that Alex Kiernan is following a similar process [4]. This
allows minimizing the need to rebuild U-Boot binary and perform all
boot flow experimenting and fine-tuning in versioned text files.

In a nutshell, I would decompose this discussion item into:
 - Is there really a problem in seeing the bootdelay countdown started
   before entering the fastboot mode [3]? Why users/developers should
   be forbidden entering the U-Boot shell before entering fastboot mode?
   Why I am raising this question is because half of the troubles of
   this discussion item assume [3] is already in place.
 - If there is still preference for [3], then I believe the discussion
   must go on and I don't have good ideas how to overcome the arising
   complexity. Particularly, this would place a "lock" on the PREBOOT
   feature (it would be problematic to use preboot for multiple purposes
   as expressed earlier) which will potentially make difficult
   implementing key press detection for entering fastboot on TI
   platforms.
> 
> DISCUSSION ITEM 2: this patch only implements reboot reason handling for
> TI platforms. I presume there could be another parties interested in
> this feature. Should we somehow provide some generic way for handling
> that (at least mechanism for checking), or is it a bad idea, and
> everyone should handle this in their own way?

I think that depends on how the feature is implemented on TI and whether
the same resources are available on other platforms. Particularly,
PREBOOT seems to currently be unused on TI and is free to be employed in
your solution. However, it could have already been used on other
platforms for a different purpose.

> 
> DISCUSSION ITEM 3: I didn't handle "reboot recovery" reason here. Does
> anyone have some ideas on that? This doc [1] a bit of confuses me, but
> as I understand, the 'recovery' partition will remain, so in case of
> "reboot recovery" we just need to load recovery partition into the RAM
> and run bootm for that. Is that correct or I'm missing something?

Based on my experiments, we perform switching between recovery and
normal boot by including/dropping "skip_initramfs" in bootargs. There
could have been other parameters which I missed. This brings me to
another question, i.e. how to efficiently manipulate bootargs,
especially since there is a growing number of occurrences appending
Android-specific information to bootargs. I believe this will have to
be tackled in not so distant future.

Good luck with the bring-up!

> 
> [1] https://source.android.com/devices/tech/ota/ab/ab_implement

[2] u-boot (0352e878d2b8) git grep "env_set.*preboot"
arch/arm/mach-imx/mx6/opos6ul.c:		env_set("preboot", "");
arch/arm/mach-rockchip/boot_mode.c:		env_set("preboot", "setenv preboot; fastboot usb0");
arch/arm/mach-rockchip/boot_mode.c:		env_set("preboot", "setenv preboot; ums mmc 0");
arch/arm/mach-stm32mp/cpu.c:		env_set("preboot", "env set preboot; fastboot 0");
arch/arm/mach-stm32mp/cpu.c:		env_set("preboot", cmd);
arch/arm/mach-stm32mp/cpu.c:		env_set("preboot", "env set preboot; run altbootcmd");
board/boundary/nitrogen6x/nitrogen6x.c:				env_set("preboot", cmd);
board/rockchip/kylin_rk3036/kylin_rk3036.c:		env_set("preboot", "setenv preboot; fastboot usb0");
board/syteco/zmx25/zmx25.c:				env_set("preboot", "run gs_slow_boot");
board/syteco/zmx25/zmx25.c:				env_set("preboot", "run gs_fast_boot");

[3] https://patchwork.ozlabs.org/patch/1119704/
    ("env: ti: Improve "fastboot reboot bootloader" handling")

[4] https://patchwork.ozlabs.org/patch/1101107/#2175414

-- 
Best regards,
Eugeniu.