[U-Boot] [RFC] env: ti: Handle reboot reason from BCB
*** 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: 1. User did "fastboot reboot bootloader" (we're setting "dofastboot" variable as an indicator in this case) 2. 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)
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 - performance penalty due to scripting usage in 'preboot'
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.
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?
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?
[1] https://source.android.com/devices/tech/ota/ab/ab_implement
Signed-off-by: Sam Protsenko semen.protsenko@linaro.org --- include/environment/ti/boot.h | 43 ++++++++++++++++++++++++++++++----- 1 file changed, 37 insertions(+), 6 deletions(-)
diff --git a/include/environment/ti/boot.h b/include/environment/ti/boot.h index 8ddb0416c4..943adcad40 100644 --- a/include/environment/ti/boot.h +++ b/include/environment/ti/boot.h @@ -126,13 +126,44 @@ "if test $fdtfile = undefined; then " \ "echo WARNING: Could not determine device tree to use; fi; \0"
-#define CONFIG_PREBOOT \ +#define FASTBOOT_CMD \ + "echo Booting into fastboot ...; " \ + "fastboot " __stringify(CONFIG_FASTBOOT_USB_DEV) "; " + +/* Check we are booting from "fastboot reboot bootloader" */ +#define PREBOOT_CHECK_DOFASTBOOT \ "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) "; " \ - "fi;" \ + "echo Boot fastboot requested, resetting dofastboot ...; " \ + "setenv dofastboot 0; saveenv; " \ + FASTBOOT_CMD \ + "fi; " + +/* Check reboot reason in BCB area of 'misc' partition */ +#define PREBOOT_CHECK_BCB \ + "if bcb load " __stringify(CONFIG_FASTBOOT_FLASH_MMC_DEV) " misc; " \ + "then " \ + "if bcb test command = bootonce-bootloader; then " \ + "echo Boot fastboot requested, resetting BCB ...; " \ + "bcb clear command; bcb store; " \ + FASTBOOT_CMD \ + "elif bcb test command = boot-recovery; then " \ + "echo Boot recovery requested; resetting BCB ...; " \ + "bcb clear command; bcb store; " \ + "echo TODO: RECOVERY_CMD is not implemented; " \ + "fi; " \ + "else " \ + "echo Error: BCB/misc is corrupted or does not exist; " \ + "fi; " + +/* Enter fastboot mode if user requested it */ +#if CONFIG_IS_ENABLED(CMD_BCB) +# define CONFIG_PREBOOT \ + PREBOOT_CHECK_DOFASTBOOT \ + PREBOOT_CHECK_BCB +#else +# define CONFIG_PREBOOT \ + PREBOOT_CHECK_DOFASTBOOT +#endif
#define CONFIG_BOOTCOMMAND \ "if test ${boot_fit} -eq 1; then " \
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
Hi Eugeniu,
On Sun, Jun 30, 2019 at 9:49 PM Eugeniu Rosca roscaeugeniu@gmail.com wrote:
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.
You are right. After giving it some thought I came to the same conclusion. It's better to stick to regular CONFIG_BOOTCOMMAND and do things there.
- 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?
Nope. That thought just came across, because scripting in general case takes more CPU time than C code, as it should be parsed and interpreted first.
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.
I meant exposing BCB functions first :) But yeah, that thought of mine was preposterous I guess. We shouldn't use "preboot" for such stuff anyway, I decided to stay away from it.
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.
Exactly. Come to think about it, it was bad idea from the beginning. Now I think user should be able to override everything if countdown is enabled. So I abandoned [3] and will do things differently, without using preboot.
- 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.
Agreed.
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.
I mean exposing BCB routines first in C API. Not sure there is much sense in it though, as BCB commands implementation seems really slim to me, maybe it's ok to duplicate code in this case.
Anyway, I'm starting to think more and more about moving all that Android boot logic into dedicated command, e.g. as it's done in [1,2].
[1] https://android.googlesource.com/platform/external/u-boot/+/c7f85c5f75f95dbb... [2] https://android.googlesource.com/platform/external/u-boot/+/7d8d87584d7cbe83...
With new Google requirements (hopefully fixed now) the boot flow should be pretty much the same for all platforms, and I have a feeling that those requirements could be marked as "mandatory" in future.
For now I want to keep Android boot for TI platforms done via scripting. But it makes the environment cluttered, and if someone else wants to boot Android, all those commands will be repeated for that platform.
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.
I see. This is similar to what's done in [1,2] (links are above). As I mentioned, one way to implement it comprehensively would be "android_boot" implementation, maybe it will be easier to do that in C. Another option is to use something like:
=> env default bootargs => env set bootargs $android_bootargs1 => env save
Not sure if it's what you mean by "efficiently manipulate bootargs" though.
Good luck with the bring-up!
Thank you for putting your time in reviewing this, Eugeniu! I guess now I see the better of implementing Android boot flow.
[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
-- Best regards, Eugeniu.
participants (2)
-
Eugeniu Rosca -
Sam Protsenko