| Message ID | 1434131821-7641-3-git-send-email-contact@paulk.fr |
|---|---|
| State | Accepted |
| Delegated to: | Łukasz Majewski |
| Headers | show |
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); } > + 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
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
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") >>> + 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 >
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 > >
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]; + 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" " - 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;" \ "fi;" \ "run findfdt; " \ "run mmcboot;" \
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(-)