[U-Boot] [PATCH 1/2] fastboot: more support for reboot-bootloader command

[Steve Rae]

Correct, with the current implementation, I think that you have the
only solution...
(1) add code during the startup sequence to test that rebooting into
fastboot is desired (as done in the misc_init_r functions that you
described)
(2) once this has been detected, then write an env variable (as done
with "reboot-mode" variable)
(3) ensure that the CONFIG_BOOTCOMMAND has a test for this env
variable and that it launches the "fastboot" command (as done with: if
test reboot-${reboot-mode} = reboot-b; then echo fastboot; fastboot 0;
fi)

So, I wanted to:
(1) simplify this to not depend on any env variable, and not depend on
the CONFIG_BOOTCOMMAND (can this be accidentally wiped out in the
environment?)
(2) also allow for the "fastboot continue" command (although I think
that the CONFIG_BOOTCOMMAND also handles this properly!)

IMO - this series seems to be a much more straightforward approach....
perhaps if I changed the function name to:
      fb_handle_reboot_bootloader_flag()  or
      handle_fastboot_reboot_bootloader_flag()
because it is not trying to handle all possible reboot modes, only the
"fastboot reboot-bootloader"....
Would that help?
Thanks, Steve

On Wed, Aug 24, 2016 at 3:07 AM, Paul Kocialkowski <contact at paulk.fr> wrote:
> Hi,
>
> Le mardi 23 août 2016 à 16:38 -0700, Steve Rae a écrit :
>> The "fastboot reboot-bootloader" command is defined to
>> re-enter into fastboot mode after rebooting into the
>> bootloader.
>>
>> There is current support for setting the reset flag
>> via the __weak fb_set_reboot_flag() function.
>>
>> This commit adds a generic handler to implement code
>> which could launch fastboot during the boot sequence
>> via this __weak fb_handle_reboot_flag() function.
>> The actual handling this reset flag should be implemented
>> by board/SoC specific code.
>
> So far, we've been calling the fastboot command from CONFIG_BOOTCOMMAND (more or
> less directly) by setting an env variable (reboot-mode, dofastboot, etc), which
> I think is a good fit. Since fastboot is a standalone command, I think it makes
> sense to call it from the bootcommand instead of calling it from the function
> you introduce.
>
> IMO the fb_handle_reboot_flag function you're introducing should only detect
> that fastboot mode is requested and set an env variable (like it's done
> in misc_init_r in sniper and kc1) so that the bootcommand can pick it up and act
> accordingly. This clearly separates the logic and puts each side of it where it
> belongs.
>
>> Signed-off-by: Steve Rae <steve.rae at raedomain.com>
>> cc: Alexey Firago <alexey_firago at mentor.com>
>> cc: Paul Kocialkowski <contact at paulk.fr>
>> cc: Tom Rini <trini at konsulko.com>
>> cc: Angela Stegmaier <angelabaker at ti.com>
>> cc: Dileep Katta <dileep.katta at linaro.org>
>> ---
>>
>>  common/main.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/common/main.c b/common/main.c
>> index 2116a9e..ea3fe42 100644
>> --- a/common/main.c
>> +++ b/common/main.c
>> @@ -20,6 +20,12 @@ DECLARE_GLOBAL_DATA_PTR;
>>   */
>>  __weak void show_boot_progress(int val) {}
>>
>> +/*
>> + * Board-specific Platform code must implement fb_handle_reboot_flag(), if
>> + * this feature is desired
>> + */
>> +__weak void fb_handle_reboot_flag(void) {}
>> +
>>  static void run_preboot_environment_command(void)
>>  {
>>  #ifdef CONFIG_PREBOOT
>> @@ -63,6 +69,8 @@ void main_loop(void)
>>       if (cli_process_fdt(&s))
>>               cli_secure_boot_cmd(s);
>>
>> +     fb_handle_reboot_flag();
>> +
>>       autoboot_command(s);
>>
>>       cli_loop();
> --
> Paul Kocialkowski, developer of low-level free software for embedded devices
>
> Website: https://www.paulk.fr/
> Coding blog: https://code.paulk.fr/
> Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/