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

[Steve Rae]

On Aug 25, 2016 01:30, "Paul Kocialkowski" <contact at paulk.fr> wrote:
>
> 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/

ping...