From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steve Rae Date: Tue, 16 Jun 2015 14:36:59 -0700 Subject: [U-Boot] [PATCH v2 3/4] usb: board_usb_init and board_usb_cleanup calls in the fastboot command In-Reply-To: <1434489926.2276.20.camel@collins> References: <1434131821-7641-1-git-send-email-contact@paulk.fr> <1434131821-7641-3-git-send-email-contact@paulk.fr> <55808DF2.3050209@broadcom.com> <1434489926.2276.20.camel@collins> Message-ID: <558096FB.9010809@broadcom.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >>> --- >>> 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 >>> #include >>> #include >>> +#include >>> >>> 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" >>> + "\n" >> >> make it optional: >> "[]\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 >