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

> > +	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/d706b932/attachment.sig>