[U-Boot] [PATCH 0/2] Fix repeated USB scans problems
Hi Marek & Stephen,
As discussed before we've a problem where our standard bootcmds sometimes scan usb more then once, causing a large boot delay.
Marek, as discussed with you before, this patch-set tackles this differently then previous sets, by simply making "usb start" a oneshot command (atleast until "usb stop" is called).
Stephen the second patch then modifies config_distro_bootcmd.h to simply call "usb start" unconditionally, since calling it repeatedly now can be done without side-effects.
Assuming you both like these patches, that leaves the question of how to merge them, they can be merged individually each through your own trees, or one of you can pick up both of them, I'll leave that to you.
Regards,
Hans
Currently we've this magic in include/config_distro_bootcmd.h to avoid scanning the usb bus multiple times.
And it does not work when also using an usb keyboard because then the preboot command has already scanned the bus, so we're still scanning it twice.
This commit makes "usb start" only start usb if it is no already started, allowing us to remove all the magic for it from include/config_distro_bootcmd.h and just call it unconditionally.
This also causes "usb start" and "usb reset" to actually do what their different names suggest, rather then both of them doing exactly the same.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- common/cmd_usb.c | 48 +++++++++++++++++++++++++++++++----------------- 1 file changed, 31 insertions(+), 17 deletions(-)
diff --git a/common/cmd_usb.c b/common/cmd_usb.c index c192498..27813f0 100644 --- a/common/cmd_usb.c +++ b/common/cmd_usb.c @@ -441,6 +441,26 @@ static int do_usb_stop_keyboard(int force) return 0; }
+static void do_usb_start(void) +{ + bootstage_mark_name(BOOTSTAGE_ID_USB_START, "usb_start"); + + if (usb_init() < 0) + return; + +#ifdef CONFIG_USB_STORAGE + /* try to recognize storage devices immediately */ + usb_stor_curr_dev = usb_stor_scan(1); +#endif +#ifdef CONFIG_USB_HOST_ETHER + /* try to recognize ethernet devices immediately */ + usb_ether_curr_dev = usb_host_eth_scan(1); +#endif +#ifdef CONFIG_USB_KEYBOARD + drv_usb_kbd_init(); +#endif +} + /****************************************************************************** * usb command intepreter */ @@ -457,26 +477,20 @@ static int do_usb(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) if (argc < 2) return CMD_RET_USAGE;
- if ((strncmp(argv[1], "reset", 5) == 0) || - (strncmp(argv[1], "start", 5) == 0)) { - bootstage_mark_name(BOOTSTAGE_ID_USB_START, "usb_start"); + if (strncmp(argv[1], "start", 5) == 0) { + if (usb_started) + return 0; /* Already started */ + printf("starting USB...\n"); + do_usb_start(); + return 0; + } + + if (strncmp(argv[1], "reset", 5) == 0) { + printf("resetting USB...\n"); if (do_usb_stop_keyboard(1) != 0) return 1; usb_stop(); - printf("(Re)start USB...\n"); - if (usb_init() >= 0) { -#ifdef CONFIG_USB_STORAGE - /* try to recognize storage devices immediately */ - usb_stor_curr_dev = usb_stor_scan(1); -#endif -#ifdef CONFIG_USB_HOST_ETHER - /* try to recognize ethernet devices immediately */ - usb_ether_curr_dev = usb_host_eth_scan(1); -#endif -#ifdef CONFIG_USB_KEYBOARD - drv_usb_kbd_init(); -#endif - } + do_usb_start(); return 0; } if (strncmp(argv[1], "stop", 4) == 0) {
Now that "usb start" will only start usb if not already started, we can simply call "usb start" whenever we (may) need access to usb devices, and it will only actually scan the bus at the first call.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- include/config_distro_bootcmd.h | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-)
diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h index be616e8..becbe3f 100644 --- a/include/config_distro_bootcmd.h +++ b/include/config_distro_bootcmd.h @@ -90,15 +90,8 @@ #endif
#ifdef CONFIG_CMD_USB -#define BOOTENV_RUN_USB_INIT "run usb_init; " -#define BOOTENV_SET_USB_NEED_INIT "setenv usb_need_init; " +#define BOOTENV_RUN_USB_INIT "usb start; " #define BOOTENV_SHARED_USB \ - "usb_init=" \ - "if ${usb_need_init}; then " \ - "setenv usb_need_init false; " \ - "usb start 0; " \ - "fi\0" \ - \ "usb_boot=" \ BOOTENV_RUN_USB_INIT \ BOOTENV_SHARED_BLKDEV_BODY(usb) @@ -106,7 +99,6 @@ #define BOOTENV_DEV_NAME_USB BOOTENV_DEV_NAME_BLKDEV #else #define BOOTENV_RUN_USB_INIT -#define BOOTENV_SET_USB_NEED_INIT #define BOOTENV_SHARED_USB #define BOOTENV_DEV_USB \ BOOT_TARGET_DEVICES_references_USB_without_CONFIG_CMD_USB @@ -202,7 +194,7 @@ \ BOOT_TARGET_DEVICES(BOOTENV_DEV) \ \ - "bootcmd=" BOOTENV_SET_USB_NEED_INIT BOOTENV_SET_SCSI_NEED_INIT \ + "bootcmd=" BOOTENV_SET_SCSI_NEED_INIT \ "for target in ${boot_targets}; do " \ "run bootcmd_${target}; " \ "done\0"
On 01/06/2015 06:27 AM, Hans de Goede wrote:
Hi Marek & Stephen,
As discussed before we've a problem where our standard bootcmds sometimes scan usb more then once, causing a large boot delay.
Marek, as discussed with you before, this patch-set tackles this differently then previous sets, by simply making "usb start" a oneshot command (atleast until "usb stop" is called).
Stephen the second patch then modifies config_distro_bootcmd.h to simply call "usb start" unconditionally, since calling it repeatedly now can be done without side-effects.
Assuming you both like these patches, that leaves the question of how to merge them, they can be merged individually each through your own trees, or one of you can pick up both of them, I'll leave that to you.
Users will have to get used to running "usb reset" rather than "usb start", but I guess that's fine.
Should we rename "usb reset" to "usb restart"?
Either way, the series, Acked-by: Stephen Warren swarren@nvidia.com
Will you do something similar for SCSI?
On Tuesday, January 06, 2015 at 06:02:57 PM, Stephen Warren wrote:
On 01/06/2015 06:27 AM, Hans de Goede wrote:
Hi Marek & Stephen,
As discussed before we've a problem where our standard bootcmds sometimes scan usb more then once, causing a large boot delay.
Marek, as discussed with you before, this patch-set tackles this differently then previous sets, by simply making "usb start" a oneshot command (atleast until "usb stop" is called).
Stephen the second patch then modifies config_distro_bootcmd.h to simply call "usb start" unconditionally, since calling it repeatedly now can be done without side-effects.
Assuming you both like these patches, that leaves the question of how to merge them, they can be merged individually each through your own trees, or one of you can pick up both of them, I'll leave that to you.
Users will have to get used to running "usb reset" rather than "usb start", but I guess that's fine.
Hi!
So why do we not have "usb reset" only, why can we not discard the "usb start" altogether?
Should we rename "usb reset" to "usb restart"?
No, let's not mess with the UI any more than we already did.
Either way, the series, Acked-by: Stephen Warren swarren@nvidia.com
Will you do something similar for SCSI?
Best regards, Marek Vasut
On 01/07/2015 04:35 PM, Marek Vasut wrote:
On Tuesday, January 06, 2015 at 06:02:57 PM, Stephen Warren wrote:
On 01/06/2015 06:27 AM, Hans de Goede wrote:
Hi Marek & Stephen,
As discussed before we've a problem where our standard bootcmds sometimes scan usb more then once, causing a large boot delay.
Marek, as discussed with you before, this patch-set tackles this differently then previous sets, by simply making "usb start" a oneshot command (atleast until "usb stop" is called).
Stephen the second patch then modifies config_distro_bootcmd.h to simply call "usb start" unconditionally, since calling it repeatedly now can be done without side-effects.
Assuming you both like these patches, that leaves the question of how to merge them, they can be merged individually each through your own trees, or one of you can pick up both of them, I'll leave that to you.
Users will have to get used to running "usb reset" rather than "usb start", but I guess that's fine.
Hi!
So why do we not have "usb reset" only, why can we not discard the "usb start" altogether?
The whole point is that we need separate commands for:
* usb start: If USB isn't started, start it and scan the bus, else do nothing.
This is used by automatic scripts that want to ensure that USB is available, but not force bus rescans over and over.
* usb reset: If USB isn't started, start it. Always rescan the bus.
This is used by the user to force a rescan of the USB bus, if they know they've plugged in a new device.
Should we rename "usb reset" to "usb restart"?
No, let's not mess with the UI any more than we already did.
Well, it might not be such a bad idea if the semantics are changing, to give people a heads-up?
Hi,
On 08-01-15 01:19, Stephen Warren wrote:
On 01/07/2015 04:35 PM, Marek Vasut wrote:
On Tuesday, January 06, 2015 at 06:02:57 PM, Stephen Warren wrote:
<snip>
Should we rename "usb reset" to "usb restart"?
No, let's not mess with the UI any more than we already did.
Well, it might not be such a bad idea if the semantics are changing, to give people a heads-up?
Only the semantics of "usb start" are changing, and you are suggesting renaming "usb reset" which remains unchanged ...
Regards,
Hans
On 01/08/2015 01:34 AM, Hans de Goede wrote:
Hi,
On 08-01-15 01:19, Stephen Warren wrote:
On 01/07/2015 04:35 PM, Marek Vasut wrote:
On Tuesday, January 06, 2015 at 06:02:57 PM, Stephen Warren wrote:
<snip>
Should we rename "usb reset" to "usb restart"?
No, let's not mess with the UI any more than we already did.
Well, it might not be such a bad idea if the semantics are changing, to give people a heads-up?
Only the semantics of "usb start" are changing, and you are suggesting renaming "usb reset" which remains unchanged ...
Yes, that's true. So I suppose we should indeed not rename anything.
On Thursday, January 08, 2015 at 05:16:53 PM, Stephen Warren wrote:
On 01/08/2015 01:34 AM, Hans de Goede wrote:
Hi,
Hi!
On 08-01-15 01:19, Stephen Warren wrote:
On 01/07/2015 04:35 PM, Marek Vasut wrote:
On Tuesday, January 06, 2015 at 06:02:57 PM, Stephen Warren wrote:
<snip>
Should we rename "usb reset" to "usb restart"?
No, let's not mess with the UI any more than we already did.
Well, it might not be such a bad idea if the semantics are changing, to give people a heads-up?
Only the semantics of "usb start" are changing, and you are suggesting renaming "usb reset" which remains unchanged ...
Yes, that's true. So I suppose we should indeed not rename anything.
OK, let's go with this approach now.
Best regards, Marek Vasut
Hi,
On 08-01-15 18:06, Marek Vasut wrote:
On Thursday, January 08, 2015 at 05:16:53 PM, Stephen Warren wrote:
On 01/08/2015 01:34 AM, Hans de Goede wrote:
Hi,
Hi!
On 08-01-15 01:19, Stephen Warren wrote:
On 01/07/2015 04:35 PM, Marek Vasut wrote:
On Tuesday, January 06, 2015 at 06:02:57 PM, Stephen Warren wrote:
<snip>
Should we rename "usb reset" to "usb restart"?
No, let's not mess with the UI any more than we already did.
Well, it might not be such a bad idea if the semantics are changing, to give people a heads-up?
Only the semantics of "usb start" are changing, and you are suggesting renaming "usb reset" which remains unchanged ...
Yes, that's true. So I suppose we should indeed not rename anything.
OK, let's go with this approach now.
So we are all in agreement, good, so through who's tree are these 2 patches going to go upstream ? Note I can take them upstream through the sunxi tree, but the usb tree seems better to me ...
Regards,
Hans
On Thursday, January 08, 2015 at 06:23:55 PM, Hans de Goede wrote:
Hi,
On 08-01-15 18:06, Marek Vasut wrote:
On Thursday, January 08, 2015 at 05:16:53 PM, Stephen Warren wrote:
On 01/08/2015 01:34 AM, Hans de Goede wrote:
Hi,
Hi!
On 08-01-15 01:19, Stephen Warren wrote:
On 01/07/2015 04:35 PM, Marek Vasut wrote:
On Tuesday, January 06, 2015 at 06:02:57 PM, Stephen Warren wrote:
<snip>
> Should we rename "usb reset" to "usb restart"?
No, let's not mess with the UI any more than we already did.
Well, it might not be such a bad idea if the semantics are changing, to give people a heads-up?
Only the semantics of "usb start" are changing, and you are suggesting renaming "usb reset" which remains unchanged ...
Yes, that's true. So I suppose we should indeed not rename anything.
OK, let's go with this approach now.
So we are all in agreement, good, so through who's tree are these 2 patches going to go upstream ? Note I can take them upstream through the sunxi tree, but the usb tree seems better to me ...
I can pick them, shall I pick them as they are ? Are they for -next or current ?
Best regards, Marek Vasut
Hi,
On 08-01-15 18:36, Marek Vasut wrote:
On Thursday, January 08, 2015 at 06:23:55 PM, Hans de Goede wrote:
Hi,
On 08-01-15 18:06, Marek Vasut wrote:
On Thursday, January 08, 2015 at 05:16:53 PM, Stephen Warren wrote:
On 01/08/2015 01:34 AM, Hans de Goede wrote:
Hi,
Hi!
On 08-01-15 01:19, Stephen Warren wrote:
On 01/07/2015 04:35 PM, Marek Vasut wrote: > On Tuesday, January 06, 2015 at 06:02:57 PM, Stephen Warren wrote:
<snip>
>> Should we rename "usb reset" to "usb restart"? > > No, let's not mess with the UI any more than we already did.
Well, it might not be such a bad idea if the semantics are changing, to give people a heads-up?
Only the semantics of "usb start" are changing, and you are suggesting renaming "usb reset" which remains unchanged ...
Yes, that's true. So I suppose we should indeed not rename anything.
OK, let's go with this approach now.
So we are all in agreement, good, so through who's tree are these 2 patches going to go upstream ? Note I can take them upstream through the sunxi tree, but the usb tree seems better to me ...
I can pick them, shall I pick them as they are ?
Yes, AFAIK there were no requests for changes.
Are they for -next or current ?
They are intended for -next, the double usb scan when using a usb keyboard *and* booting from usb is a nuisance, but not fatal, and very few people actually use the combo, so given that current is about to be released in a couple of days lets play it safe :)
Regards,
Hans
On Thursday, January 08, 2015 at 06:40:19 PM, Hans de Goede wrote:
Hi,
Hi!
[...]
Only the semantics of "usb start" are changing, and you are suggesting renaming "usb reset" which remains unchanged ...
Yes, that's true. So I suppose we should indeed not rename anything.
OK, let's go with this approach now.
So we are all in agreement, good, so through who's tree are these 2 patches going to go upstream ? Note I can take them upstream through the sunxi tree, but the usb tree seems better to me ...
I can pick them, shall I pick them as they are ?
Yes, AFAIK there were no requests for changes.
Are they for -next or current ?
They are intended for -next, the double usb scan when using a usb keyboard *and* booting from usb is a nuisance, but not fatal, and very few people actually use the combo, so given that current is about to be released in a couple of days lets play it safe :)
OK, applied both to next. Thanks!
Best regards, Marek Vasut
Hi,
On 06-01-15 18:02, Stephen Warren wrote:
On 01/06/2015 06:27 AM, Hans de Goede wrote:
Hi Marek & Stephen,
As discussed before we've a problem where our standard bootcmds sometimes scan usb more then once, causing a large boot delay.
Marek, as discussed with you before, this patch-set tackles this differently then previous sets, by simply making "usb start" a oneshot command (atleast until "usb stop" is called).
Stephen the second patch then modifies config_distro_bootcmd.h to simply call "usb start" unconditionally, since calling it repeatedly now can be done without side-effects.
Assuming you both like these patches, that leaves the question of how to merge them, they can be merged individually each through your own trees, or one of you can pick up both of them, I'll leave that to you.
Users will have to get used to running "usb reset" rather than "usb start", but I guess that's fine.
Should we rename "usb reset" to "usb restart"?
Either way, the series, Acked-by: Stephen Warren swarren@nvidia.com
Will you do something similar for SCSI?
I think it would probably make sense to do something similar for SCSI, but I'm not all that familiar with the SCSI code, and I really don't have time to work on this for SCSI, so atm I do not plan to work that.
Regards,
Hans
participants (3)
-
Hans de Goede -
Marek Vasut -
Stephen Warren