Hi Eugeniu,
On Fri, Jul 12, 2019 at 12:39 AM Eugeniu Rosca <roscaeuge...@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 <ero...@de.adit-jv.com>
>
> [1]
> https://android.googlesource.com/platform/external/u-boot/+/7d8d87584d7c/cmd/boot_android.c#67
>
> --
> Best Regards,
> Eugeniu.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
