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

[Paul Kocialkowski]

Hi,

Le samedi 24 septembre 2016 à 18:01 -0700, Steve Rae a écrit :
> 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...

What do you think about the feedback from my previous message?

Cheers,

-- 
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: 801 bytes
Desc: This is a digitally signed message part
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160926/0a0499bb/attachment.sig>