* [U-Boot] [PATCH] env: ti: boot: Handle reboot reason from BCB
@ 2019-07-09 14:45 Sam Protsenko
2019-07-11 21:39 ` Eugeniu Rosca
2019-07-12 17:35 ` Sam Protsenko
0 siblings, 2 replies; 5+ messages in thread
From: Sam Protsenko @ 2019-07-09 14:45 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] env: ti: boot: Handle reboot reason from BCB
2019-07-09 14:45 [U-Boot] [PATCH] env: ti: boot: Handle reboot reason from BCB Sam Protsenko
@ 2019-07-11 21:39 ` Eugeniu Rosca
2019-07-12 13:18 ` Sam Protsenko
2019-07-12 17:35 ` Sam Protsenko
1 sibling, 1 reply; 5+ messages in thread
From: Eugeniu Rosca @ 2019-07-11 21:39 UTC (permalink / raw)
To: u-boot
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] env: ti: boot: Handle reboot reason from BCB
2019-07-11 21:39 ` Eugeniu Rosca
@ 2019-07-12 13:18 ` Sam Protsenko
2019-07-12 13:25 ` Eugeniu Rosca
0 siblings, 1 reply; 5+ messages in thread
From: Sam Protsenko @ 2019-07-12 13:18 UTC (permalink / raw)
To: u-boot
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] env: ti: boot: Handle reboot reason from BCB
2019-07-12 13:18 ` Sam Protsenko
@ 2019-07-12 13:25 ` Eugeniu Rosca
0 siblings, 0 replies; 5+ messages in thread
From: Eugeniu Rosca @ 2019-07-12 13:25 UTC (permalink / raw)
To: u-boot
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] env: ti: boot: Handle reboot reason from BCB
2019-07-09 14:45 [U-Boot] [PATCH] env: ti: boot: Handle reboot reason from BCB Sam Protsenko
2019-07-11 21:39 ` Eugeniu Rosca
@ 2019-07-12 17:35 ` Sam Protsenko
1 sibling, 0 replies; 5+ messages in thread
From: Sam Protsenko @ 2019-07-12 17:35 UTC (permalink / raw)
To: u-boot
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-07-12 17:35 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-09 14:45 [U-Boot] [PATCH] env: ti: boot: Handle reboot reason from BCB Sam Protsenko
2019-07-11 21:39 ` Eugeniu Rosca
2019-07-12 13:18 ` Sam Protsenko
2019-07-12 13:25 ` Eugeniu Rosca
2019-07-12 17:35 ` Sam Protsenko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.