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

[Paul Kocialkowski <contact@paulk.fr>]

Le mardi 16 juin 2015 ? 14:36 -0700, Steve Rae a ?crit :
> 
> 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@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")

Well, if I missed someplace in U-Boot where it is stated that only
"fastboot" is sufficient to run the command, I should change that too.

Otherwise, for third party guides, the rationale is the same as
previously stated.

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

-------------- 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/20150616/09d3c841/attachment.sig>