Hi Eugeniu,

On Fri, Jul 12, 2019 at 12:39 AM Eugeniu Rosca roscaeugeniu@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@de.adit-jv.com

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

-- Best Regards, Eugeniu.