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

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

[Steve Rae]

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(+)

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();
-- 
1.8.5

[U-Boot] [PATCH 2/2] bcm: fastboot: implement 'reboot-bootloader'

[Steve Rae]

on bcm235xx and bcm281xx boards

Signed-off-by: Steve Rae <steve.rae@raedomain.com>
---

 board/broadcom/bcm23550_w1d/bcm23550_w1d.c | 30 ++++++++++++++++++++++++++++++
 board/broadcom/bcm28155_ap/bcm28155_ap.c   | 30 ++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+)

diff --git a/board/broadcom/bcm23550_w1d/bcm23550_w1d.c b/board/broadcom/bcm23550_w1d/bcm23550_w1d.c
index 0cb059f..ec0956c 100644
--- a/board/broadcom/bcm23550_w1d/bcm23550_w1d.c
+++ b/board/broadcom/bcm23550_w1d/bcm23550_w1d.c
@@ -26,6 +26,9 @@
 #define CONFIG_USB_SERIALNO "1234567890"
 #endif
 
+#define FB_REBOOT_FLAG_BITS		0x05
+#define FB_REBOOT_FLAG_LOCATION		0x34051f80
+
 DECLARE_GLOBAL_DATA_PTR;
 
 /*
@@ -118,3 +121,30 @@ int board_usb_cleanup(int index, enum usb_init_type init)
 	return 0;
 }
 #endif
+
+int fb_set_reboot_flag(void)
+{
+	/* set 'reboot-bootloader' bits */
+	writel(readl(FB_REBOOT_FLAG_LOCATION) | FB_REBOOT_FLAG_BITS,
+	       FB_REBOOT_FLAG_LOCATION);
+	printf("%s: 0x%08x @ 0x%08x\n", __func__,
+	       readl(FB_REBOOT_FLAG_LOCATION), FB_REBOOT_FLAG_LOCATION);
+	return 0;
+}
+
+void fb_handle_reboot_flag(void)
+{
+	int run_fastboot = (readl(FB_REBOOT_FLAG_LOCATION) &
+			    FB_REBOOT_FLAG_BITS ? 1 : 0);
+
+	if (run_fastboot) {
+		printf("\n%s: performing: 'fastboot 0'\n", __func__);
+
+		/* clear 'reboot-bootloader' bits */
+		writel(readl(FB_REBOOT_FLAG_LOCATION) & ~(FB_REBOOT_FLAG_BITS),
+		       FB_REBOOT_FLAG_LOCATION);
+
+		/* process 'reboot-bootloader' request */
+		run_command("fastboot 0", 0);
+	}
+}
diff --git a/board/broadcom/bcm28155_ap/bcm28155_ap.c b/board/broadcom/bcm28155_ap/bcm28155_ap.c
index b3a4a41..5ac9569 100644
--- a/board/broadcom/bcm28155_ap/bcm28155_ap.c
+++ b/board/broadcom/bcm28155_ap/bcm28155_ap.c
@@ -26,6 +26,9 @@
 #define CONFIG_USB_SERIALNO "1234567890"
 #endif
 
+#define FB_REBOOT_FLAG_BITS		0x05
+#define FB_REBOOT_FLAG_LOCATION		0x34053f98
+
 DECLARE_GLOBAL_DATA_PTR;
 
 /*
@@ -125,3 +128,30 @@ int board_usb_cleanup(int index, enum usb_init_type init)
 	return 0;
 }
 #endif
+
+int fb_set_reboot_flag(void)
+{
+	/* set 'reboot-bootloader' bits */
+	writel(readl(FB_REBOOT_FLAG_LOCATION) | FB_REBOOT_FLAG_BITS,
+	       FB_REBOOT_FLAG_LOCATION);
+	printf("%s: 0x%08x @ 0x%08x\n", __func__,
+	       readl(FB_REBOOT_FLAG_LOCATION), FB_REBOOT_FLAG_LOCATION);
+	return 0;
+}
+
+void fb_handle_reboot_flag(void)
+{
+	int run_fastboot = (readl(FB_REBOOT_FLAG_LOCATION) &
+			    FB_REBOOT_FLAG_BITS ? 1 : 0);
+
+	if (run_fastboot) {
+		printf("\n%s: performing: 'fastboot 0'\n", __func__);
+
+		/* clear 'reboot-bootloader' bits */
+		writel(readl(FB_REBOOT_FLAG_LOCATION) & ~(FB_REBOOT_FLAG_BITS),
+		       FB_REBOOT_FLAG_LOCATION);
+
+		/* process 'reboot-bootloader' request */
+		run_command("fastboot 0", 0);
+	}
+}
-- 
1.8.5

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

[Paul Kocialkowski]

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/
-------------- 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/20160824/127e256d/attachment.sig>

[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@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/

[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@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/
-------------- 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>

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

[Steve Rae]

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...

[U-Boot] [PATCH 2/2] bcm: fastboot: implement 'reboot-bootloader'

[Steve Rae]

On Aug 23, 2016 16:39, "Steve Rae" <steve.rae@raedomain.com> wrote:
>
> on bcm235xx and bcm281xx boards
>
> Signed-off-by: Steve Rae <steve.rae@raedomain.com>
> ---
>
>  board/broadcom/bcm23550_w1d/bcm23550_w1d.c | 30
++++++++++++++++++++++++++++++
>  board/broadcom/bcm28155_ap/bcm28155_ap.c   | 30
++++++++++++++++++++++++++++++
>  2 files changed, 60 insertions(+)
>
> diff --git a/board/broadcom/bcm23550_w1d/bcm23550_w1d.c
b/board/broadcom/bcm23550_w1d/bcm23550_w1d.c
> index 0cb059f..ec0956c 100644
> --- a/board/broadcom/bcm23550_w1d/bcm23550_w1d.c
> +++ b/board/broadcom/bcm23550_w1d/bcm23550_w1d.c
> @@ -26,6 +26,9 @@
>  #define CONFIG_USB_SERIALNO "1234567890"
>  #endif
>
> +#define FB_REBOOT_FLAG_BITS            0x05
> +#define FB_REBOOT_FLAG_LOCATION                0x34051f80
> +
>  DECLARE_GLOBAL_DATA_PTR;
>
>  /*
> @@ -118,3 +121,30 @@ int board_usb_cleanup(int index, enum usb_init_type
init)
>         return 0;
>  }
>  #endif
> +
> +int fb_set_reboot_flag(void)
> +{
> +       /* set 'reboot-bootloader' bits */
> +       writel(readl(FB_REBOOT_FLAG_LOCATION) | FB_REBOOT_FLAG_BITS,
> +              FB_REBOOT_FLAG_LOCATION);
> +       printf("%s: 0x%08x @ 0x%08x\n", __func__,
> +              readl(FB_REBOOT_FLAG_LOCATION), FB_REBOOT_FLAG_LOCATION);
> +       return 0;
> +}
> +
> +void fb_handle_reboot_flag(void)
> +{
> +       int run_fastboot = (readl(FB_REBOOT_FLAG_LOCATION) &
> +                           FB_REBOOT_FLAG_BITS ? 1 : 0);
> +
> +       if (run_fastboot) {
> +               printf("\n%s: performing: 'fastboot 0'\n", __func__);
> +
> +               /* clear 'reboot-bootloader' bits */
> +               writel(readl(FB_REBOOT_FLAG_LOCATION) &
~(FB_REBOOT_FLAG_BITS),
> +                      FB_REBOOT_FLAG_LOCATION);
> +
> +               /* process 'reboot-bootloader' request */
> +               run_command("fastboot 0", 0);
> +       }
> +}
> diff --git a/board/broadcom/bcm28155_ap/bcm28155_ap.c
b/board/broadcom/bcm28155_ap/bcm28155_ap.c
> index b3a4a41..5ac9569 100644
> --- a/board/broadcom/bcm28155_ap/bcm28155_ap.c
> +++ b/board/broadcom/bcm28155_ap/bcm28155_ap.c
> @@ -26,6 +26,9 @@
>  #define CONFIG_USB_SERIALNO "1234567890"
>  #endif
>
> +#define FB_REBOOT_FLAG_BITS            0x05
> +#define FB_REBOOT_FLAG_LOCATION                0x34053f98
> +
>  DECLARE_GLOBAL_DATA_PTR;
>
>  /*
> @@ -125,3 +128,30 @@ int board_usb_cleanup(int index, enum usb_init_type
init)
>         return 0;
>  }
>  #endif
> +
> +int fb_set_reboot_flag(void)
> +{
> +       /* set 'reboot-bootloader' bits */
> +       writel(readl(FB_REBOOT_FLAG_LOCATION) | FB_REBOOT_FLAG_BITS,
> +              FB_REBOOT_FLAG_LOCATION);
> +       printf("%s: 0x%08x @ 0x%08x\n", __func__,
> +              readl(FB_REBOOT_FLAG_LOCATION), FB_REBOOT_FLAG_LOCATION);
> +       return 0;
> +}
> +
> +void fb_handle_reboot_flag(void)
> +{
> +       int run_fastboot = (readl(FB_REBOOT_FLAG_LOCATION) &
> +                           FB_REBOOT_FLAG_BITS ? 1 : 0);
> +
> +       if (run_fastboot) {
> +               printf("\n%s: performing: 'fastboot 0'\n", __func__);
> +
> +               /* clear 'reboot-bootloader' bits */
> +               writel(readl(FB_REBOOT_FLAG_LOCATION) &
~(FB_REBOOT_FLAG_BITS),
> +                      FB_REBOOT_FLAG_LOCATION);
> +
> +               /* process 'reboot-bootloader' request */
> +               run_command("fastboot 0", 0);
> +       }
> +}
> --
> 1.8.5
>

ping...

[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@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,

-- 
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>