Hi Sam,
All below is my 2 cents and FWIW, so feel free to just skip it.
On Fri, Jun 21, 2019 at 12:25:44AM +0300, Sam Protsenko wrote:
*** PLEASE DO NOT MERGE. *** This is only RFC, a discussion thread. Patch is only for the *** reference here right now, to create a discussion context.
There are 2 possible reboot use-cases:
- User did "fastboot reboot bootloader" (we're setting "dofastboot" variable as an indicator in this case)
- User did "adb reboot bootloader" or "reboot bootloader" from Linux shell (Android sets "bootonce-bootloader" string into command field of BCB area inside of 'misc' partition as an indicator in this case)
The two requirements above seem to serve the same purpose. It is a bit unfortunate that each comes with its own implementation and its own assumption of which non-volatile memory is available to pass the "reboot into bootloader" message to the bootloader. Still, I can understand the choice to treat the two independently. It gives the most flexibility to the user in terms of which features (FB, BCB, etc) to include in U-Boot, with still getting the expected behavior.
We already had (1) handling implemented. This patch adds implementation of (2). Possible drawbacks are:
- user is able to re-define 'preboot' variable and break this mechanism this way
It is also my understanding that ${preboot} was designed to be overridden at runtime [2]. Looking at the help message of "config USE_PREBOOT":
----8<---- This feature is especially useful when "preboot" is automatically generated or modified. For example, the boot code can modify the "preboot" when a user holds down a certain combination of keys. ----8<----
In conformance with the help message, we use PREBOOT on R-Car platform to enter fastboot/bootloader mode on pressing a dedicated key when {cold,re}-booting the Renesas reference targets.
The above observations may lead to the following conclusions (IMHO): - storing non-trivial logic into CONFIG_PREBOOT might play not so well with the purpose of the preboot feature. With a complicated flow embedded into CONFIG_PREBOOT, users [2] would potentially need to extract and add subsets of CONFIG_PREBOOT to ${preboot} at runtime and I think this would be very inconvenient. - in a more general sense, making use of CONFIG_PREBOOT is probably not a good idea at all if there is no need to alter ${preboot} at runtime. Yes, there might be some temptation to do things prior to the start of the BOOTDELAY countdown [3], but I think that the primary use-case of ${preboot} is to handle potential dynamic HW changes (e.g. key press) in the pre-main() boot phase.
- performance penalty due to scripting usage in 'preboot'
W/o any measurements at hand, I think there is not enough evidence from which a boot time penalty could be inferred. Are there any numbers backing up your concern?
DISCUSSION ITEM 1: possible solution to those can be checking BCB area (and 'dofastboot' variable) in C code, somewhere in board file, like in late_init() function, and on;y set 'preboot' variable from there, if conditions are met, to something like this:
preboot=fastboot 1; setenv preboot;so that next time 'preboot' variable will be empty. But this way also has its shortcomings, like the necessity of board file modification, need of BCB C API exposure, etc.
I personally think that calling BCB command (and any other U-Boot shell command) in C (using run_command() and friends) to some degree defeats the purpose of having those commands implemented and exposed to the user. At least in our development environment, we see U-Boot as being nicely composed of small building blocks (shell commands), which we try to glue together via externally stored/developed *.scr files. It is my impression that Alex Kiernan is following a similar process [4]. This allows minimizing the need to rebuild U-Boot binary and perform all boot flow experimenting and fine-tuning in versioned text files.
In a nutshell, I would decompose this discussion item into: - Is there really a problem in seeing the bootdelay countdown started before entering the fastboot mode [3]? Why users/developers should be forbidden entering the U-Boot shell before entering fastboot mode? Why I am raising this question is because half of the troubles of this discussion item assume [3] is already in place. - If there is still preference for [3], then I believe the discussion must go on and I don't have good ideas how to overcome the arising complexity. Particularly, this would place a "lock" on the PREBOOT feature (it would be problematic to use preboot for multiple purposes as expressed earlier) which will potentially make difficult implementing key press detection for entering fastboot on TI platforms.
DISCUSSION ITEM 2: this patch only implements reboot reason handling for TI platforms. I presume there could be another parties interested in this feature. Should we somehow provide some generic way for handling that (at least mechanism for checking), or is it a bad idea, and everyone should handle this in their own way?
I think that depends on how the feature is implemented on TI and whether the same resources are available on other platforms. Particularly, PREBOOT seems to currently be unused on TI and is free to be employed in your solution. However, it could have already been used on other platforms for a different purpose.
DISCUSSION ITEM 3: I didn't handle "reboot recovery" reason here. Does anyone have some ideas on that? This doc [1] a bit of confuses me, but as I understand, the 'recovery' partition will remain, so in case of "reboot recovery" we just need to load recovery partition into the RAM and run bootm for that. Is that correct or I'm missing something?
Based on my experiments, we perform switching between recovery and normal boot by including/dropping "skip_initramfs" in bootargs. There could have been other parameters which I missed. This brings me to another question, i.e. how to efficiently manipulate bootargs, especially since there is a growing number of occurrences appending Android-specific information to bootargs. I believe this will have to be tackled in not so distant future.
Good luck with the bring-up!
[1] https://source.android.com/devices/tech/ota/ab/ab_implement
[2] u-boot (0352e878d2b8) git grep "env_set.*preboot" arch/arm/mach-imx/mx6/opos6ul.c: env_set("preboot", ""); arch/arm/mach-rockchip/boot_mode.c: env_set("preboot", "setenv preboot; fastboot usb0"); arch/arm/mach-rockchip/boot_mode.c: env_set("preboot", "setenv preboot; ums mmc 0"); arch/arm/mach-stm32mp/cpu.c: env_set("preboot", "env set preboot; fastboot 0"); arch/arm/mach-stm32mp/cpu.c: env_set("preboot", cmd); arch/arm/mach-stm32mp/cpu.c: env_set("preboot", "env set preboot; run altbootcmd"); board/boundary/nitrogen6x/nitrogen6x.c: env_set("preboot", cmd); board/rockchip/kylin_rk3036/kylin_rk3036.c: env_set("preboot", "setenv preboot; fastboot usb0"); board/syteco/zmx25/zmx25.c: env_set("preboot", "run gs_slow_boot"); board/syteco/zmx25/zmx25.c: env_set("preboot", "run gs_fast_boot");
[3] https://patchwork.ozlabs.org/patch/1119704/ ("env: ti: Improve "fastboot reboot bootloader" handling")
[4] https://patchwork.ozlabs.org/patch/1101107/#2175414