[U-Boot] [RFC] env: ti: Handle reboot reason from BCB
Sam Protsenko
semen.protsenko at linaro.org
Thu Jul 4 16:39:30 UTC 2019
Hi Eugeniu,
On Sun, Jun 30, 2019 at 9:49 PM Eugeniu Rosca <roscaeugeniu at 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:
> > 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)
>
> 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/+/c7f85c5f75f95dbbd3cedcc3a399eee6dbb59cdc
[2] https://android.googlesource.com/platform/external/u-boot/+/7d8d87584d7cbe830689e06475c1361081fe1403
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.
More information about the U-Boot
mailing list