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