[U-Boot] [PATCH v2 3/4] usb: board_usb_init and board_usb_cleanup calls in the fastboot command

[Steve Rae]

On 15-06-16 02:25 PM, Paul Kocialkowski wrote:
> Le mardi 16 juin 2015 à 13:58 -0700, Steve Rae a écrit :
>> Hi Paul,
>>
>> On 15-06-12 10:57 AM, Paul Kocialkowski wrote:
>>> Each USB download function command calls board_usb_init before registering the
>>> USB gadget and board_usb_cleanup after de-registering it. On devices currently
>>> using fasboot, musb-new is usually initialized earlier, but some other boards
>>> might need the board_usb_init call to properly initialize musb-new.
>>>
>>> This requires adding an argument (the USB controller index) to the fastboot
>>> command, as it is currently done with other USB download gadget functions.
>>>
>>> Signed-off-by: Paul Kocialkowski <contact at paulk.fr>
>>> ---
>>>    common/cmd_fastboot.c             | 31 +++++++++++++++++++++++++------
>>>    include/configs/ti_omap5_common.h |  2 +-
>>>    2 files changed, 26 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/common/cmd_fastboot.c b/common/cmd_fastboot.c
>>> index d52ccfb..86fbddf 100644
>>> --- a/common/cmd_fastboot.c
>>> +++ b/common/cmd_fastboot.c
>>> @@ -10,11 +10,26 @@
>>>    #include <common.h>
>>>    #include <command.h>
>>>    #include <g_dnl.h>
>>> +#include <usb.h>
>>>
>>>    static int do_fastboot(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
>>>    {
>>> +	int controller_index;
>>> +	char *usb_controller;
>>>    	int ret;
>>>
>>> +	if (argc < 2)
>>> +		return CMD_RET_USAGE;
>>> +
>>> +	usb_controller = argv[1];
>>
>> Not backwards compatible.... I would prefer to make it optional:
>>           if (argc < 2)
>>                   controller_index = 0;
>>           else {
>>                   usb_controller = argv[1];
>>                   controller_index = simple_strtoul(usb_controller, NULL, 0);
>>           }
>
> This is definitely a "bug fix". There is no reason to assume that the
> USB controller index is 0 and it was incorrect to assume that from the
> very beginning. Other download USB gadget commands had the controller
> index as a non-optional parameter already.
>
> Since I fixed the configs that use it in U-Boot, I think it's fair
> enough. This may indeed break some hand-written scripts but those will
> be straightforward to fix.
>
> I am strongly against keeping deprecated legacy fallbacks that make the
> code inconsistent and harder to maintain just for the sake of keeping
> backwards compatibility with users' hand-written scripts.
>

I understand, but I'm more worried about _all_ the existing 
documentation that states the the command line is "fastboot" (which now 
would need to be changed to "fastboot 0")

>>> +	controller_index = simple_strtoul(usb_controller, NULL, 0);
>>> +
>>> +	ret = board_usb_init(controller_index, USB_INIT_DEVICE);
>>> +	if (ret) {
>>> +		error("USB init failed: %d", ret);
>>> +		return CMD_RET_FAILURE;
>>> +	}
>>> +
>>>    	g_dnl_clear_detach();
>>>    	ret = g_dnl_register("usb_dnl_fastboot");
>>>    	if (ret)
>>> @@ -23,9 +38,8 @@ static int do_fastboot(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
>>>    	if (!g_dnl_board_usb_cable_connected()) {
>>>    		puts("\rUSB cable not detected.\n" \
>>>    		     "Command exit.\n");
>>> -		g_dnl_unregister();
>>> -		g_dnl_clear_detach();
>>> -		return CMD_RET_FAILURE;
>>> +		ret = CMD_RET_FAILURE;
>>> +		goto exit;
>>>    	}
>>>
>>>    	while (1) {
>>> @@ -36,14 +50,19 @@ static int do_fastboot(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
>>>    		usb_gadget_handle_interrupts(0);
>>>    	}
>>>
>>> +	ret = CMD_RET_SUCCESS;
>>> +
>>> +exit:
>>>    	g_dnl_unregister();
>>>    	g_dnl_clear_detach();
>>> -	return CMD_RET_SUCCESS;
>>> +	board_usb_cleanup(controller_index, USB_INIT_DEVICE);
>>> +
>>> +	return ret;
>>>    }
>>>
>>>    U_BOOT_CMD(
>>> -	fastboot,	1,	0,	do_fastboot,
>>> +	fastboot, 2, 1, do_fastboot,
>>>    	"use USB Fastboot protocol",
>>> -	"\n"
>>> +	"<USB_controller>\n"
>>
>> make it optional:
>> 	"[<USB_controller>]\n"
>>
>>>    	"    - run as a fastboot usb device"
>>>    );
>>> diff --git a/include/configs/ti_omap5_common.h b/include/configs/ti_omap5_common.h
>>> index 4faffef..4fd5669 100644
>>> --- a/include/configs/ti_omap5_common.h
>>> +++ b/include/configs/ti_omap5_common.h
>>> @@ -137,7 +137,7 @@
>>>    	"if test ${dofastboot} -eq 1; then " \
>>>    		"echo Boot fastboot requested, resetting dofastboot ...;" \
>>>    		"setenv dofastboot 0; saveenv;" \
>>> -		"echo Booting into fastboot ...; fastboot;" \
>>> +		"echo Booting into fastboot ...; fastboot 0;" \
>>
>> then this isn't needed either....
>>
>>>    	"fi;" \
>>>    	"run findfdt; " \
>>>    	"run mmcboot;" \
>>>
>>
>> Thanks, Steve
>