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

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

[Sam Protsenko]

*** 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)

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
  - performance penalty due to scripting usage in 'preboot'

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.

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?

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?

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

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 include/environment/ti/boot.h | 43 ++++++++++++++++++++++++++++++-----
 1 file changed, 37 insertions(+), 6 deletions(-)

diff --git a/include/environment/ti/boot.h b/include/environment/ti/boot.h
index 8ddb0416c4..943adcad40 100644
--- a/include/environment/ti/boot.h
+++ b/include/environment/ti/boot.h
@@ -126,13 +126,44 @@
 		"if test $fdtfile = undefined; then " \
 			"echo WARNING: Could not determine device tree to use; fi; \0"
 
-#define CONFIG_PREBOOT \
+#define FASTBOOT_CMD \
+	"echo Booting into fastboot ...; " \
+	"fastboot " __stringify(CONFIG_FASTBOOT_USB_DEV) "; "
+
+/* Check we are booting from "fastboot reboot bootloader" */
+#define PREBOOT_CHECK_DOFASTBOOT \
 	"if test ${dofastboot} -eq 1; then " \
-		"echo Boot fastboot requested, resetting dofastboot ...;" \
-		"setenv dofastboot 0; saveenv;" \
-		"echo Booting into fastboot ...; " \
-		"fastboot " __stringify(CONFIG_FASTBOOT_USB_DEV) "; " \
-	"fi;" \
+		"echo Boot fastboot requested, resetting dofastboot ...; " \
+		"setenv dofastboot 0; saveenv; " \
+		FASTBOOT_CMD \
+	"fi; "
+
+/* Check reboot reason in BCB area of 'misc' partition */
+#define PREBOOT_CHECK_BCB \
+	"if bcb load " __stringify(CONFIG_FASTBOOT_FLASH_MMC_DEV) " misc; " \
+	"then " \
+		"if bcb test command = bootonce-bootloader; then " \
+			"echo Boot fastboot requested, resetting BCB ...; " \
+			"bcb clear command; bcb store; " \
+			FASTBOOT_CMD \
+		"elif bcb test command = boot-recovery; then " \
+			"echo Boot recovery requested; resetting BCB ...; " \
+			"bcb clear command; bcb store; " \
+			"echo TODO: RECOVERY_CMD is not implemented; " \
+		"fi; " \
+	"else " \
+		"echo Error: BCB/misc is corrupted or does not exist; " \
+	"fi; "
+
+/* Enter fastboot mode if user requested it */
+#if CONFIG_IS_ENABLED(CMD_BCB)
+# define CONFIG_PREBOOT \
+	PREBOOT_CHECK_DOFASTBOOT \
+	PREBOOT_CHECK_BCB
+#else
+# define CONFIG_PREBOOT \
+	PREBOOT_CHECK_DOFASTBOOT
+#endif
 
 #define CONFIG_BOOTCOMMAND \
 	"if test ${boot_fit} -eq 1; then "	\
-- 
2.20.1

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

[Eugeniu Rosca]

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@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.

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

[Sam Protsenko]

Hi Eugeniu,

On Sun, Jun 30, 2019 at 9:49 PM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
>
> 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.
>

You are right. After giving it some thought I came to the same
conclusion. It's better to stick to regular CONFIG_BOOTCOMMAND and do
things there.

> >   - 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?
>

Nope. That thought just came across, because scripting in general case
takes more CPU time than C code, as it should be parsed and
interpreted first.

> >
> > 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.
>

I meant exposing BCB functions first :) But yeah, that thought of mine
was preposterous I guess. We shouldn't use "preboot" for such stuff
anyway, I decided to stay away from it.

> 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.

Exactly. Come to think about it, it was bad idea from the beginning.
Now I think user should be able to override everything if countdown is
enabled. So I abandoned [3] and will do things differently, without
using preboot.

>  - 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.

Agreed.

> >
> > 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.
>

I mean exposing BCB routines first in C API. Not sure there is much
sense in it though, as BCB commands implementation seems really slim
to me, maybe it's ok to duplicate code in this case.

Anyway, I'm starting to think more and more about moving all that
Android boot logic into dedicated command, e.g. as it's done in [1,2].

[1] https://android.googlesource.com/platform/external/u-boot/+/c7f85c5f75f95dbbd3cedcc3a399eee6dbb59cdc
[2] https://android.googlesource.com/platform/external/u-boot/+/7d8d87584d7cbe830689e06475c1361081fe1403

With new Google requirements (hopefully fixed now) the boot flow
should be pretty much the same for all platforms, and I have a feeling
that those requirements could be marked as "mandatory" in future.

For now I want to keep Android boot for TI platforms done via
scripting. But it makes the environment cluttered, and if someone else
wants to boot Android, all those commands will be repeated for that
platform.

> >
> > 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.
>

I see. This is similar to what's done in [1,2] (links are above). As I
mentioned, one way to implement it comprehensively would be
"android_boot" implementation, maybe it will be easier to do that in
C. Another option is to use something like:

    => env default bootargs
    => env set bootargs $android_bootargs1
    => env save

Not sure if it's what you mean by "efficiently manipulate bootargs" though.

> Good luck with the bring-up!
>

Thank you for putting your time in reviewing this, Eugeniu! I guess
now I see the better of implementing Android boot flow.

> >
> > [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.