[U-Boot] [PATCH] env: ti: boot: Handle reboot reason from BCB

[Eugeniu Rosca]

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 at de.adit-jv.com>

[1] https://android.googlesource.com/platform/external/u-boot/+/7d8d87584d7c/cmd/boot_android.c#67

-- 
Best Regards,
Eugeniu.