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