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

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

[Sam Protsenko]

In case of Android boot, reboot reason can be written into BCB (usually
it's an area in 'misc' partition). U-Boot then can obtain that reboot
reason from BCB and handle it accordingly to achieve correct Android
boot flow, like it was suggested in [1]:
  - if it's empty: perform normal Android boot from eMMC
  - if it contains "bootonce-bootloader": get into fastboot mode
  - if it contains "boot-recovery": perform recovery boot

The latter is not implemented yet, as it depends on some features that
are not implemented on TI platforms yet (in AOSP and in U-Boot).

[1] https://marc.info/?l=u-boot&m=152508418909737&w=2

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

diff --git a/include/environment/ti/boot.h b/include/environment/ti/boot.h
index 54e9b2de4d..e37004af46 100644
--- a/include/environment/ti/boot.h
+++ b/include/environment/ti/boot.h
@@ -98,6 +98,10 @@
 #define AB_SELECT ""
 #endif
 
+#define FASTBOOT_CMD \
+	"echo Booting into fastboot ...; " \
+	"fastboot " __stringify(CONFIG_FASTBOOT_USB_DEV) "; "
+
 #define DEFAULT_COMMON_BOOT_TI_ARGS \
 	"console=" CONSOLEDEV ",115200n8\0" \
 	"fdtfile=undefined\0" \
@@ -117,6 +121,27 @@
 		"setenv mmcroot /dev/mmcblk0p2 rw; " \
 		"run mmcboot;\0" \
 	"emmc_android_boot=" \
+	"if bcb load " __stringify(CONFIG_FASTBOOT_FLASH_MMC_DEV) " misc; " \
+	"then " \
+		"if bcb test command = bootonce-bootloader; then " \
+			"echo BCB: Bootloader boot...; " \
+			"bcb clear command; bcb store; " \
+			FASTBOOT_CMD \
+		"elif bcb test command = boot-recovery; then " \
+			"echo BCB: Recovery boot...; " \
+			"echo Warning: recovery boot is not implemented; " \
+			"echo Performing normal boot for now...; " \
+			"run emmc_android_normal_boot; " \
+		"else " \
+			"echo BCB: Normal boot requested...; " \
+			"run emmc_android_normal_boot; " \
+		"fi; " \
+	"else " \
+		"echo Warning: BCB is corrupted or does not exist; " \
+		"echo Performing normal boot...; " \
+		"run emmc_android_normal_boot; " \
+	"fi;\0" \
+	"emmc_android_normal_boot=" \
 		"echo Trying to boot Android from eMMC ...; " \
 		"run update_to_fit; " \
 		"setenv eval_bootargs setenv bootargs $bootargs; " \
@@ -176,8 +201,7 @@
 	"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) "; " \
+		FASTBOOT_CMD \
 	"fi;" \
 	"if test ${boot_fit} -eq 1; then "	\
 		"run update_to_fit;"	\
-- 
2.20.1

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

[Eugeniu Rosca]

Hi Sam,

On Tue, Jul 09, 2019 at 05:45:43PM +0300, Sam Protsenko wrote:
>  	"emmc_android_boot=" \
> +	"if bcb load " __stringify(CONFIG_FASTBOOT_FLASH_MMC_DEV) " misc; " \
> +	"then " \
> +		"if bcb test command = bootonce-bootloader; then " \
> +			"echo BCB: Bootloader boot...; " \
> +			"bcb clear command; bcb store; " \

Assuming there are multiple reboot reasons of type "bootonce/oneshot"
(i.e. assumed to be cleared once detected/handled), 'bcb test' could
implement the '-c' (clear) flag. This would allow removing the
"bcb clear command; bcb store;" line, w/o altering the behavior.

It could be done in phase 2 (optimization/refinement).

> +			FASTBOOT_CMD \
> +		"elif bcb test command = boot-recovery; then " \
> +			"echo BCB: Recovery boot...; " \
> +			"echo Warning: recovery boot is not implemented; " \
> +			"echo Performing normal boot for now...; " \
> +			"run emmc_android_normal_boot; " \
> +		"else " \
> +			"echo BCB: Normal boot requested...; " \
> +			"run emmc_android_normal_boot; " \
> +		"fi; " \
> +	"else " \
> +		"echo Warning: BCB is corrupted or does not exist; " \
> +		"echo Performing normal boot...; " \
> +		"run emmc_android_normal_boot; " \
> +	"fi;\0" \

As a general comment, yes, arguments can be brought that this scripted
handling is getting hairy and could be replaced by a command like
boot{a,_android} (you name it).

In my opinion, the main downside of "boot{a,_android}" wrapper is that
it implies sprinkling U-Boot with special-purpose variables like
${fastbootcmd} [1], ${recoverycmd}, ${my_usecase_cmd}, etc (the number
of those would likely match the number of if/else branches in this
patch). Decentralized usage of those variables (i.e. set at point A and
read/used at point B) would IMHO:
 - complicate the boot flow and its understanding, hence would
 - require to write and maintain additional documentation
 - open doors for creative issues

I contrast to the above, the approach taken in this patch:
 - avoids any special-purpose global variables
 - avoids spawning yet another boot{*} command
 - centralizes/limits the boot flow handling to one file
 - doesn't require much documentation (the code is self-explanatory)
 - in case of bugs, would require coming back to the same place
 - makes debugging easier

> +	"emmc_android_normal_boot=" \
>  		"echo Trying to boot Android from eMMC ...; " \
>  		"run update_to_fit; " \
>  		"setenv eval_bootargs setenv bootargs $bootargs; " \
> @@ -176,8 +201,7 @@
>  	"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) "; " \
> +		FASTBOOT_CMD \
>  	"fi;" \
>  	"if test ${boot_fit} -eq 1; then "	\
>  		"run update_to_fit;"	\

That said, I still admit that my statements could be highly subjective
and that the best of our collective experience (as users, developers and
maintainers) would be achieved in a different way than described.

Below is based on code review only (can't test due to lack of HW):

Reviewed-by: Eugeniu Rosca <erosca@de.adit-jv.com>

[1] https://android.googlesource.com/platform/external/u-boot/+/7d8d87584d7c/cmd/boot_android.c#67

-- 
Best Regards,
Eugeniu.

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

[Sam Protsenko]

Hi Eugeniu,

On Fri, Jul 12, 2019 at 12:39 AM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
>
> Hi Sam,
>
> On Tue, Jul 09, 2019 at 05:45:43PM +0300, Sam Protsenko wrote:
> >       "emmc_android_boot=" \
> > +     "if bcb load " __stringify(CONFIG_FASTBOOT_FLASH_MMC_DEV) " misc; " \
> > +     "then " \
> > +             "if bcb test command = bootonce-bootloader; then " \
> > +                     "echo BCB: Bootloader boot...; " \
> > +                     "bcb clear command; bcb store; " \
>
> Assuming there are multiple reboot reasons of type "bootonce/oneshot"
> (i.e. assumed to be cleared once detected/handled), 'bcb test' could
> implement the '-c' (clear) flag. This would allow removing the
> "bcb clear command; bcb store;" line, w/o altering the behavior.
>
> It could be done in phase 2 (optimization/refinement).
>
> > +                     FASTBOOT_CMD \
> > +             "elif bcb test command = boot-recovery; then " \
> > +                     "echo BCB: Recovery boot...; " \
> > +                     "echo Warning: recovery boot is not implemented; " \
> > +                     "echo Performing normal boot for now...; " \
> > +                     "run emmc_android_normal_boot; " \
> > +             "else " \
> > +                     "echo BCB: Normal boot requested...; " \
> > +                     "run emmc_android_normal_boot; " \
> > +             "fi; " \
> > +     "else " \
> > +             "echo Warning: BCB is corrupted or does not exist; " \
> > +             "echo Performing normal boot...; " \
> > +             "run emmc_android_normal_boot; " \
> > +     "fi;\0" \
>
> As a general comment, yes, arguments can be brought that this scripted
> handling is getting hairy and could be replaced by a command like
> boot{a,_android} (you name it).
>
> In my opinion, the main downside of "boot{a,_android}" wrapper is that
> it implies sprinkling U-Boot with special-purpose variables like
> ${fastbootcmd} [1], ${recoverycmd}, ${my_usecase_cmd}, etc (the number
> of those would likely match the number of if/else branches in this
> patch). Decentralized usage of those variables (i.e. set at point A and
> read/used at point B) would IMHO:
>  - complicate the boot flow and its understanding, hence would
>  - require to write and maintain additional documentation
>  - open doors for creative issues
>
> I contrast to the above, the approach taken in this patch:
>  - avoids any special-purpose global variables
>  - avoids spawning yet another boot{*} command
>  - centralizes/limits the boot flow handling to one file
>  - doesn't require much documentation (the code is self-explanatory)
>  - in case of bugs, would require coming back to the same place
>  - makes debugging easier
>

Let's finalize modern Android boot flow via scripting (as we need all
those commands anyway), then we can think if we need to wrap the whole
thing into boot_android command. There is a lot of stuff happening
during the Android boot, not only reboot reason check, e.g.
  - AVB
  - A/B slotting
  - partitions reading
  - preparing the cmdline

So once we implement Android-Q requirements for Android boot flow, we
can experiment with separate command. But let's postpone that at least
for next merge window. Then we can decide together which way is
better.

> > +     "emmc_android_normal_boot=" \
> >               "echo Trying to boot Android from eMMC ...; " \
> >               "run update_to_fit; " \
> >               "setenv eval_bootargs setenv bootargs $bootargs; " \
> > @@ -176,8 +201,7 @@
> >       "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) "; " \
> > +             FASTBOOT_CMD \
> >       "fi;" \
> >       "if test ${boot_fit} -eq 1; then "      \
> >               "run update_to_fit;"    \
>
> That said, I still admit that my statements could be highly subjective
> and that the best of our collective experience (as users, developers and
> maintainers) would be achieved in a different way than described.
>
> Below is based on code review only (can't test due to lack of HW):
>
> Reviewed-by: Eugeniu Rosca <erosca@de.adit-jv.com>
>
> [1] https://android.googlesource.com/platform/external/u-boot/+/7d8d87584d7c/cmd/boot_android.c#67
>
> --
> Best Regards,
> Eugeniu.

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

[Eugeniu Rosca]

Hi Sam,

On Fri, Jul 12, 2019 at 04:18:20PM +0300, Sam Protsenko wrote:
> Let's finalize modern Android boot flow via scripting (as we need all
> those commands anyway), then we can think if we need to wrap the whole
> thing into boot_android command. There is a lot of stuff happening
> during the Android boot, not only reboot reason check, e.g.
>   - AVB
>   - A/B slotting
>   - partitions reading
>   - preparing the cmdline
> 
> So once we implement Android-Q requirements for Android boot flow, we
> can experiment with separate command. But let's postpone that at least
> for next merge window. Then we can decide together which way is
> better.

Sounds good. Agreed.

-- 
Best Regards,
Eugeniu.

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

[Sam Protsenko]

Superseded by v2.

On Tue, Jul 9, 2019 at 5:45 PM Sam Protsenko <semen.protsenko@linaro.org> wrote:
>
> In case of Android boot, reboot reason can be written into BCB (usually
> it's an area in 'misc' partition). U-Boot then can obtain that reboot
> reason from BCB and handle it accordingly to achieve correct Android
> boot flow, like it was suggested in [1]:
>   - if it's empty: perform normal Android boot from eMMC
>   - if it contains "bootonce-bootloader": get into fastboot mode
>   - if it contains "boot-recovery": perform recovery boot
>
> The latter is not implemented yet, as it depends on some features that
> are not implemented on TI platforms yet (in AOSP and in U-Boot).
>
> [1] https://marc.info/?l=u-boot&m=152508418909737&w=2
>
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  include/environment/ti/boot.h | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/include/environment/ti/boot.h b/include/environment/ti/boot.h
> index 54e9b2de4d..e37004af46 100644
> --- a/include/environment/ti/boot.h
> +++ b/include/environment/ti/boot.h
> @@ -98,6 +98,10 @@
>  #define AB_SELECT ""
>  #endif
>
> +#define FASTBOOT_CMD \
> +       "echo Booting into fastboot ...; " \
> +       "fastboot " __stringify(CONFIG_FASTBOOT_USB_DEV) "; "
> +
>  #define DEFAULT_COMMON_BOOT_TI_ARGS \
>         "console=" CONSOLEDEV ",115200n8\0" \
>         "fdtfile=undefined\0" \
> @@ -117,6 +121,27 @@
>                 "setenv mmcroot /dev/mmcblk0p2 rw; " \
>                 "run mmcboot;\0" \
>         "emmc_android_boot=" \
> +       "if bcb load " __stringify(CONFIG_FASTBOOT_FLASH_MMC_DEV) " misc; " \
> +       "then " \
> +               "if bcb test command = bootonce-bootloader; then " \
> +                       "echo BCB: Bootloader boot...; " \
> +                       "bcb clear command; bcb store; " \
> +                       FASTBOOT_CMD \
> +               "elif bcb test command = boot-recovery; then " \
> +                       "echo BCB: Recovery boot...; " \
> +                       "echo Warning: recovery boot is not implemented; " \
> +                       "echo Performing normal boot for now...; " \
> +                       "run emmc_android_normal_boot; " \
> +               "else " \
> +                       "echo BCB: Normal boot requested...; " \
> +                       "run emmc_android_normal_boot; " \
> +               "fi; " \
> +       "else " \
> +               "echo Warning: BCB is corrupted or does not exist; " \
> +               "echo Performing normal boot...; " \
> +               "run emmc_android_normal_boot; " \
> +       "fi;\0" \
> +       "emmc_android_normal_boot=" \
>                 "echo Trying to boot Android from eMMC ...; " \
>                 "run update_to_fit; " \
>                 "setenv eval_bootargs setenv bootargs $bootargs; " \
> @@ -176,8 +201,7 @@
>         "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) "; " \
> +               FASTBOOT_CMD \
>         "fi;" \
>         "if test ${boot_fit} -eq 1; then "      \
>                 "run update_to_fit;"    \
> --
> 2.20.1
>