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

[Paul Kocialkowski]

Le mercredi 24 août 2016 à 16:52 -0700, Steve Rae a écrit :
> 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?)

I'm not sure it really simplifies much. fastboot is a boot command, so I think
it's a good fit for CONFIG_BOOTCOMMAND. This is where I expect it to be called.

I don't think that the possibility of accidentally wiping it out is a very
legitimate concern (most boards expect a specific CONFIG_BOOTCOMMAND, I don't
see any problem with that). It's up to users to deal with env breakage.

Also, I'm a bit worried about where the logic should be, because there are cases
where we want to trigger fastboot from e.g. a button press. Using an env
variable makes it easy to have button handling (which may also trigger other
modes, not only fastboot) in one place to just set env variables accordingly.

I don't think such button handling should be in the function you're introducing.
Thus, it means that boards will need a second place from where to call fastboot,
which makes it less intuitive and much messier.

With a clear separation between detection (the first half of what the function
you're introducing is doing) and fastboot execution, we can easily manage
different sources that trigger fastboot mode.

Finally, some boards only rely on persistent env storage to set fastboot mode
(and otherwise don't have a specific bit preserved at reset that can be set for
it), so the way you're suggesting won't be a good fit for these boards at all,
which creates disparity between boards and makes the whole thing less intuitive
and more confusing.

> (2) also allow for the "fastboot continue" command (although I think
> that the CONFIG_BOOTCOMMAND also handles this properly!)

Yes, this is already handled 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?

That's not really my concern, and I like to keep functions names consistent. The
original name you suggested is a good match with fb_set_reboot_flag.

Thanks

> 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/
-- 
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/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160825/a00d1cf1/attachment.sig>