[U-Boot] [PATCH v2 3/4] usb: board_usb_init and board_usb_cleanup calls in the fastboot command
Paul Kocialkowski
contact at paulk.fr
Tue Jun 16 23:56:30 CEST 2015
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 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")
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>
More information about the U-Boot
mailing list