From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paul Kocialkowski Date: Tue, 16 Jun 2015 23:56:30 +0200 Subject: [U-Boot] [PATCH v2 3/4] usb: board_usb_init and board_usb_cleanup calls in the fastboot command In-Reply-To: <558096FB.9010809@broadcom.com> 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> <558096FB.9010809@broadcom.com> Message-ID: <1434491790.2276.22.camel@collins> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 > >>> --- > >>> 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") 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" > >>> + "\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 > > -------------- 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: