[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 at 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 at de.adit-jv.com>
>
> [1] https://android.googlesource.com/platform/external/u-boot/+/7d8d87584d7c/cmd/boot_android.c#67
>
> --
> Best Regards,
> Eugeniu.