[U-Boot] [RFC PATCH v2 13/20] fastboot: Merge reboot-bootloader handling

[Alex Kiernan]

On Tue, May 1, 2018 at 7:50 AM Jocelyn Bohr <bohr at google.com> wrote:



> On Mon, Apr 30, 2018 at 1:33 AM Alex Kiernan <alex.kiernan at gmail.com>
wrote:

>> Extract fb_set_reboot_flag() from USB code and ensure all the overides
>> are included, then make the UDP fastboot code go through this same
>> path.

>> Note this changes the behaviour of the fastboot net code such that
>> "reboot-bootloader" is no longer written to CONFIG_FASTBOOT_BUF_ADDR for
>> use as a marker on reboot (the AOSP code in common/android-bootloader.c
>> uses this marker - this code could be reinstated there if that gets
>> merged).

>> Signed-off-by: Alex Kiernan <alex.kiernan at gmail.com>
>> ---

>> Changes in v2: None

>>   arch/arm/mach-omap2/boot-common.c     |  2 +-
>>   arch/arm/mach-rockchip/rk3128-board.c |  2 +-
>>   arch/arm/mach-rockchip/rk322x-board.c |  2 +-
>>   drivers/fastboot/fb_common.c          |  5 +++++
>>   drivers/usb/gadget/f_fastboot.c       |  5 -----
>>   include/fastboot.h                    |  1 +
>>   net/fastboot.c                        | 17 +++++++++--------
>>   7 files changed, 18 insertions(+), 16 deletions(-)

>> diff --git a/arch/arm/mach-omap2/boot-common.c
b/arch/arm/mach-omap2/boot-common.c
>> index f9ab5da..2be5c11 100644
>> --- a/arch/arm/mach-omap2/boot-common.c
>> +++ b/arch/arm/mach-omap2/boot-common.c
>> @@ -238,7 +238,7 @@ void arch_preboot_os(void)
>>   }
>>   #endif

>> -#if defined(CONFIG_USB_FUNCTION_FASTBOOT) &&
!defined(CONFIG_ENV_IS_NOWHERE)
>> +#if CONFIG_IS_ENABLED(FASTBOOT) && !CONFIG_IS_ENABLED(ENV_IS_NOWHERE)
>>   int fb_set_reboot_flag(void)
>>   {
>>          printf("Setting reboot to fastboot flag ...\n");
>> diff --git a/arch/arm/mach-rockchip/rk3128-board.c
b/arch/arm/mach-rockchip/rk3128-board.c
>> index 2e8393d..00ad563 100644
>> --- a/arch/arm/mach-rockchip/rk3128-board.c
>> +++ b/arch/arm/mach-rockchip/rk3128-board.c
>> @@ -112,7 +112,7 @@ int board_usb_cleanup(int index, enum usb_init_type
init)
>>   }
>>   #endif

>> -#if defined(CONFIG_USB_FUNCTION_FASTBOOT)
>> +#if CONFIG_IS_ENABLED(FASTBOOT)
>>   int fb_set_reboot_flag(void)
>>   {
>>          struct rk3128_grf *grf;
>> diff --git a/arch/arm/mach-rockchip/rk322x-board.c
b/arch/arm/mach-rockchip/rk322x-board.c
>> index 8642a90..0ddfac8 100644
>> --- a/arch/arm/mach-rockchip/rk322x-board.c
>> +++ b/arch/arm/mach-rockchip/rk322x-board.c
>> @@ -140,7 +140,7 @@ int board_usb_cleanup(int index, enum usb_init_type
init)
>>   }
>>   #endif

>> -#if defined(CONFIG_USB_FUNCTION_FASTBOOT)
>> +#if CONFIG_IS_ENABLED(FASTBOOT)
>>   int fb_set_reboot_flag(void)
>>   {
>>          struct rk322x_grf *grf;
>> diff --git a/drivers/fastboot/fb_common.c b/drivers/fastboot/fb_common.c
>> index 8b3627b..36ef669 100644
>> --- a/drivers/fastboot/fb_common.c
>> +++ b/drivers/fastboot/fb_common.c
>> @@ -102,3 +102,8 @@ int fastboot_lookup_command(const char *cmd_string)

>>          return -1;
>>   }
>> +
>> +int __weak fb_set_reboot_flag(void)
>> +{
>> +       return -1;
>> +}

> Instead could this write the string "reboot-bootloader" to
CONFIG_FASTBOOT_BUF_ADDR?


I guess the thing you lose is you don't then get a default "I don't know
how to do that" response. Am I right in thinking you're only using this
behaviour on Raspberry Pi, or is it broader?


>> diff --git a/drivers/usb/gadget/f_fastboot.c
b/drivers/usb/gadget/f_fastboot.c
>> index a493c75..84515da 100644
>> --- a/drivers/usb/gadget/f_fastboot.c
>> +++ b/drivers/usb/gadget/f_fastboot.c
>> @@ -357,11 +357,6 @@ static void compl_do_reset(struct usb_ep *ep,
struct usb_request *req)
>>          do_reset(NULL, 0, 0, NULL);
>>   }

>> -int __weak fb_set_reboot_flag(void)
>> -{
>> -       return -ENOSYS;
>> -}
>> -
>>   static void cb_reboot(struct usb_ep *ep, struct usb_request *req)
>>   {
>>          char *cmd = req->buf;
>> diff --git a/include/fastboot.h b/include/fastboot.h
>> index de07220..9767065 100644
>> --- a/include/fastboot.h
>> +++ b/include/fastboot.h
>> @@ -75,4 +75,5 @@ void timed_send_info(ulong *start, const char *msg);
>>   int strcmp_l1(const char *s1, const char *s2);

>>   int fastboot_lookup_command(const char *cmd_string);
>> +int fb_set_reboot_flag(void);
>>   #endif /* _FASTBOOT_H_ */
>> diff --git a/net/fastboot.c b/net/fastboot.c
>> index 155049a..edf78df 100644
>> --- a/net/fastboot.c
>> +++ b/net/fastboot.c
>> @@ -65,7 +65,7 @@ static void cb_flash(char *, char *, unsigned int,
char *);
>>   static void cb_erase(char *, char *, unsigned int, char *);
>>   #endif
>>   static void cb_continue(char *, char *, unsigned int, char *);
>> -static void cb_reboot(char *, char *, unsigned int, char *);
>> +static void cb_reboot_bootloader(char *, char *, unsigned int, char *);

>>   static void (*fb_net_dispatch[])(char *cmd_parameter,
>>                                   char *fastboot_data,
>> @@ -83,8 +83,8 @@ static void (*fb_net_dispatch[])(char *cmd_parameter,
>>   #endif
>>          [FB_CMD_BOOT] = cb_okay,
>>          [FB_CMD_CONTINUE] = cb_continue,
>> -       [FB_CMD_REBOOT] = cb_reboot,

> Why is the normal reboot removed here?


The reboot handling is really all in the post-packet handling, so for a
reboot all you need is the OKAY which gets added two lines down in that
patch:

-       [FB_CMD_REBOOT] = cb_reboot,
-       [FB_CMD_REBOOT_BOOTLOADER] = cb_reboot,
+       [FB_CMD_REBOOT] = cb_okay,
+       [FB_CMD_REBOOT_BOOTLOADER] = cb_reboot_bootloader,

And then leverage the reboot handling further down the flow:

                 } else if (cmd == FB_CMD_REBOOT ||
                            cmd == FB_CMD_REBOOT_BOOTLOADER) {
                         do_reset(NULL, 0, 0, NULL);
                 }

I guess the biggest change overall here is to introduce named identifiers
for the commands rather than doing prefix matching.

That said I should split this patch out as it's not doing what the commit
says.

-- 
Alex Kiernan