| Message ID | 1471995532-18649-1-git-send-email-steve.rae@raedomain.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Lukasz Majewski |
| Headers | show |
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@raedomain.com> > cc: Alexey Firago <alexey_firago@mentor.com> > cc: Paul Kocialkowski <contact@paulk.fr> > cc: Tom Rini <trini@konsulko.com> > cc: Angela Stegmaier <angelabaker@ti.com> > cc: Dileep Katta <dileep.katta@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();
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@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@raedomain.com>
>> cc: Alexey Firago <alexey_firago@mentor.com>
>> cc: Paul Kocialkowski <contact@paulk.fr>
>> cc: Tom Rini <trini@konsulko.com>
>> cc: Angela Stegmaier <angelabaker@ti.com>
>> cc: Dileep Katta <dileep.katta@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/
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@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@raedomain.com> > > > cc: Alexey Firago <alexey_firago@mentor.com> > > > cc: Paul Kocialkowski <contact@paulk.fr> > > > cc: Tom Rini <trini@konsulko.com> > > > cc: Angela Stegmaier <angelabaker@ti.com> > > > cc: Dileep Katta <dileep.katta@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/
On Aug 25, 2016 01:30, "Paul Kocialkowski" <contact@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@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@raedomain.com> > > > > cc: Alexey Firago <alexey_firago@mentor.com> > > > > cc: Paul Kocialkowski <contact@paulk.fr> > > > > cc: Tom Rini <trini@konsulko.com> > > > > cc: Angela Stegmaier <angelabaker@ti.com> > > > > cc: Dileep Katta <dileep.katta@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...
Hi, Le samedi 24 septembre 2016 à 18:01 -0700, Steve Rae a écrit : > On Aug 25, 2016 01:30, "Paul Kocialkowski" <contact@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@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@raedomain.com> > > > > > cc: Alexey Firago <alexey_firago@mentor.com> > > > > > cc: Paul Kocialkowski <contact@paulk.fr> > > > > > cc: Tom Rini <trini@konsulko.com> > > > > > cc: Angela Stegmaier <angelabaker@ti.com> > > > > > cc: Dileep Katta <dileep.katta@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,
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();
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. Signed-off-by: Steve Rae <steve.rae@raedomain.com> cc: Alexey Firago <alexey_firago@mentor.com> cc: Paul Kocialkowski <contact@paulk.fr> cc: Tom Rini <trini@konsulko.com> cc: Angela Stegmaier <angelabaker@ti.com> cc: Dileep Katta <dileep.katta@linaro.org> --- common/main.c | 8 ++++++++ 1 file changed, 8 insertions(+)