[U-Boot] [PATCH] Fix to make U-Boot work with more USB sticks
I am sending this to the list looking for comments. Testing has been confined to just a few USB sticks - no USB hard drives, USB card readers, etc. But there are reports on the mailing list of problems with U-Boot's detection of USB mass storage devices.
We have had a lot of problems with different USB sticks, specifically the faster bulk storage devices. Two of these sticks have a problem where they do not respond to a USB descriptor submission within the allowed U-Boot timeout. This seems to only happen when a REQUEST SENSE is submitted immediately after a TEST UNIT READY fails.
There is also some oddness with reset, where some devices need more than one reset. This appears to be due to faulty reset code in U-Boot.
USB sticks where this this problem has been noticed are:
ID 13fe:3800 Kingston Technology Company Inc. ID 0930:6545 Toshiba Corp. Kingston DataTraveler 2.0 Stick (4GB) / PNY Attache 4GB Stick
This problem has been reproduced on the Tegra 20 Seaboard, but it is TBD whether it happens on other ARM platforms with HS USB also.
This patch:
1. Increases the USB descriptor submission timeout to 3 seconds since the original 1 second timeout seems arbitrary
2. Replaces the delay-based reset logic with logic which waits until it sees the reset is successful, rather than blindly continuing
3. Resets and retries a USB mass storage device after it fails once, in case even 3 seconds is not enough
BUG=chromiumos-6780 TEST=tried with four different USB sticks, including two which previously failed Change-Id: I5bd8891bff67a74b758e349f2a3b3575806eed59
Signed-off-by: Simon Glass sjg@chromium.org --- common/usb.c | 176 ++++++++++++++++++++++++++++++++++++------- common/usb_storage.c | 55 +++++++++++--- drivers/usb/host/ehci-hcd.c | 20 ++++- include/usb.h | 11 +++ 4 files changed, 217 insertions(+), 45 deletions(-)
diff --git a/common/usb.c b/common/usb.c index 9896f46..3265d5d 100644 --- a/common/usb.c +++ b/common/usb.c @@ -222,10 +222,14 @@ int usb_control_msg(struct usb_device *dev, unsigned int pipe, int usb_bulk_msg(struct usb_device *dev, unsigned int pipe, void *data, int len, int *actual_length, int timeout) { + int err; + if (len < 0) return -1; dev->status = USB_ST_NOT_PROC; /*not yet processed */ - submit_bulk_msg(dev, pipe, data, len); + err = submit_bulk_msg(dev, pipe, data, len); + if (err < 0) + return err; while (timeout--) { if (!((volatile unsigned long)dev->status & USB_ST_NOT_PROC)) break; @@ -758,6 +762,39 @@ struct usb_device *usb_alloc_new_device(void) return &usb_dev[dev_index - 1]; }
+/* find a device's parent hub, and reset this device's port on that hub */ + +int usb_reset_device_on_hub(struct usb_device *dev) +{ + struct usb_device *parent = dev->parent; + unsigned short portstatus; + int port = -1; + int err; + + /* find the port number we're at */ + if (parent) { + int j; + + for (j = 0; j < parent->maxchild; j++) { + if (parent->children[j] == dev) { + port = j; + break; + } + } + if (port < 0) { + printf("usb_new_device:cannot locate device's port.\n"); + return 1; + } + + err = hub_port_reset(dev->parent, port, &portstatus); + if (err < 0) { + printf("\n Couldn't reset port %i\n", port); + return 1; + } + } + return 0; +} + /* * Free the newly created device node. * Called in error cases where configuring a newly attached @@ -941,6 +978,14 @@ int usb_new_device(struct usb_device *dev) return 0; }
+/* reset and restart a device that is misbehaving */ + +int usb_restart_device(struct usb_device *dev) +{ + usb_reset_device_on_hub(dev); /* ignore return value */ + return usb_new_device(dev); +} + /* build device Tree */ void usb_scan_devices(void) { @@ -1069,45 +1114,123 @@ static inline char *portspeed(int portstatus) return "12 Mb/s"; }
-static int hub_port_reset(struct usb_device *dev, int port, - unsigned short *portstat) +/* brought this in from kernel 2.6.36 as it is a problem area. Some USB +sticks do not operate properly with the previous reset code */ +#define PORT_RESET_TRIES 5 +#define SET_ADDRESS_TRIES 2 +#define GET_DESCRIPTOR_TRIES 2 +#define SET_CONFIG_TRIES (2 * (use_both_schemes + 1)) +#define USE_NEW_SCHEME(i) ((i) / 2 == old_scheme_first) + +#define HUB_ROOT_RESET_TIME 50 /* times are in msec */ +#define HUB_SHORT_RESET_TIME 10 +#define HUB_LONG_RESET_TIME 200 +#define HUB_RESET_TIMEOUT 500 + +#define ENOTCONN 107 + +static int hub_port_wait_reset(struct usb_device *dev, int port, + unsigned int delay, unsigned short *portstatus_ret) { - int tries; + int delay_time, ret; struct usb_port_status portsts; unsigned short portstatus, portchange;
- USB_HUB_PRINTF("hub_port_reset: resetting port %d...\n", port); - for (tries = 0; tries < MAX_TRIES; tries++) { - - usb_set_port_feature(dev, port + 1, USB_PORT_FEAT_RESET); - wait_ms(200); + for (delay_time = 0; + delay_time < HUB_RESET_TIMEOUT; + delay_time += delay) { + /* wait to give the device a chance to reset */ + wait_ms(delay);
- if (usb_get_port_status(dev, port + 1, &portsts) < 0) { + /* read and decode port status */ + ret = usb_get_port_status(dev, port + 1, &portsts); + if (ret < 0) { USB_HUB_PRINTF("get_port_status failed status %lX\n", dev->status); - return -1; + return ret; } portstatus = le16_to_cpu(portsts.wPortStatus); portchange = le16_to_cpu(portsts.wPortChange);
- USB_HUB_PRINTF("portstatus %x, change %x, %s\n", - portstatus, portchange, - portspeed(portstatus)); + /* Device went away? */ + if (!(portstatus & USB_PORT_STAT_CONNECTION)) + return -ENOTCONN;
- USB_HUB_PRINTF("STAT_C_CONNECTION = %d STAT_CONNECTION = %d" \ - " USB_PORT_STAT_ENABLE %d\n", - (portchange & USB_PORT_STAT_C_CONNECTION) ? 1 : 0, - (portstatus & USB_PORT_STAT_CONNECTION) ? 1 : 0, - (portstatus & USB_PORT_STAT_ENABLE) ? 1 : 0); + /* bomb out completely if the connection bounced */ + if ((portchange & USB_PORT_STAT_C_CONNECTION)) + return -ENOTCONN;
- if ((portchange & USB_PORT_STAT_C_CONNECTION) || - !(portstatus & USB_PORT_STAT_CONNECTION)) - return -1; + /* if we`ve finished resetting, then break out of the loop */ + if (!(portstatus & USB_PORT_STAT_RESET) && + (portstatus & USB_PORT_STAT_ENABLE)) { + *portstatus_ret = portstatus; + return 0; + }
- if (portstatus & USB_PORT_STAT_ENABLE) - break; + /* switch to the long delay after two short delay failures */ + if (delay_time >= 2 * HUB_SHORT_RESET_TIME) + delay = HUB_LONG_RESET_TIME; + + USB_HUB_PRINTF( + "port %d not reset yet, waiting %dms\n", + port, delay); + } + + return -1; +} + + +static int hub_port_reset(struct usb_device *dev, int port, + unsigned short *portstat) +{ + int tries, status; + unsigned delay = HUB_SHORT_RESET_TIME; + int oldspeed = dev->speed; + + /* root hub ports have a slightly longer reset period + * (from USB 2.0 spec, section 7.1.7.5) + */ + if (!dev->parent) { + delay = HUB_ROOT_RESET_TIME; + }
- wait_ms(200); + /* Some low speed devices have problems with the quick delay, so */ + /* be a bit pessimistic with those devices. RHbug #23670 */ + if (oldspeed == USB_SPEED_LOW) + delay = HUB_LONG_RESET_TIME; + + USB_HUB_PRINTF("hub_port_reset: resetting port %d...\n", port); + for (tries = 0; tries < MAX_TRIES; tries++) { + + status = usb_set_port_feature(dev, port + 1, USB_PORT_FEAT_RESET); + if (status) + USB_HUB_PRINTF("cannot reset port %d (err = %d)\n", + port, status); + else { + status = hub_port_wait_reset(dev, port, delay, + portstat); + if (status && status != -ENOTCONN) + USB_HUB_PRINTF("port_wait_reset: err = %d\n", + status); + } + + /* return on disconnect or reset */ + switch (status) { + case 0: + /* TRSTRCY = 10 ms; plus some extra */ + wait_ms(10 + 40); + /* FALL THROUGH */ + case -1: + /* we have finished trying to reset, so return */ + usb_clear_port_feature(dev, + port + 1, USB_PORT_FEAT_C_RESET); + return 0; + } + + USB_HUB_PRINTF ( + "port %d not enabled, trying reset again...\n", + port); + delay = HUB_LONG_RESET_TIME; }
if (tries == MAX_TRIES) { @@ -1116,9 +1239,6 @@ static int hub_port_reset(struct usb_device *dev, int port, USB_HUB_PRINTF("Maybe the USB cable is bad?\n"); return -1; } - - usb_clear_port_feature(dev, port + 1, USB_PORT_FEAT_C_RESET); - *portstat = portstatus; return 0; }
diff --git a/common/usb_storage.c b/common/usb_storage.c index 76949b8..0fb9c1b 100644 --- a/common/usb_storage.c +++ b/common/usb_storage.c @@ -158,9 +158,10 @@ struct us_data { static struct us_data usb_stor[USB_MAX_STOR_DEV];
+/* start our error numbers after the USB ones */ #define USB_STOR_TRANSPORT_GOOD 0 -#define USB_STOR_TRANSPORT_FAILED -1 -#define USB_STOR_TRANSPORT_ERROR -2 +#define USB_STOR_TRANSPORT_FAILED (USB_ENEXTFREE) +#define USB_STOR_TRANSPORT_ERROR (USB_ENEXTFREE-1)
int usb_stor_get_info(struct usb_device *dev, struct us_data *us, block_dev_desc_t *dev_desc); @@ -213,6 +214,7 @@ int usb_stor_scan(int mode) { unsigned char i; struct usb_device *dev; + int result;
/* GJ */ memset(usb_stor_buf, 0, sizeof(usb_stor_buf)); @@ -244,8 +246,21 @@ int usb_stor_scan(int mode) /* ok, it is a storage devices * get info and fill it in */ - if (usb_stor_get_info(dev, &usb_stor[usb_max_devs], - &usb_dev_desc[usb_max_devs]) == 1) + result = usb_stor_get_info(dev, &usb_stor[usb_max_devs], + &usb_dev_desc[usb_max_devs]); + if (result == USB_EDEVCRITICAL) { + /* + * Something there, but failed badly. + * Retry one more time. This happens + * sometimes with some USB sticks, + * e.g. Patriot Rage ID 13fe:3800 + */ + printf ("."); + usb_restart_device(dev); /* ignore return value */ + result = usb_stor_get_info(dev, &usb_stor[usb_max_devs], + &usb_dev_desc[usb_max_devs]); + } + if (result == 1) usb_max_devs++; } /* if storage device */ @@ -690,10 +705,13 @@ int usb_stor_BBB_transport(ccb *srb, struct us_data *us) goto st; } if (result < 0) { - USB_STOR_PRINTF("usb_bulk_msg error status %ld\n", + USB_STOR_PRINTF("usb_bulk_msg error status %#lx\n", us->pusb_dev->status); usb_stor_BBB_reset(us); - return USB_STOR_TRANSPORT_FAILED; + + /* if we got a critical device error, report it specially */ + return result == USB_EDEVCRITICAL ? result + : USB_STOR_TRANSPORT_FAILED; } #ifdef BBB_XPORT_TRACE for (index = 0; index < data_actlen; index++) @@ -900,6 +918,7 @@ static int usb_inquiry(ccb *srb, struct us_data *ss)
static int usb_request_sense(ccb *srb, struct us_data *ss) { + int result; char *ptr;
ptr = (char *)srb->pdata; @@ -909,7 +928,12 @@ static int usb_request_sense(ccb *srb, struct us_data *ss) srb->datalen = 18; srb->pdata = &srb->sense_buf[0]; srb->cmdlen = 12; - ss->transport(srb, ss); + result = ss->transport(srb, ss); + if (result < 0) { + if (result != USB_EDEVCRITICAL) + USB_STOR_PRINTF("Request Sense failed\n"); + return result; + } USB_STOR_PRINTF("Request Sense returned %02X %02X %02X\n", srb->sense_buf[2], srb->sense_buf[12], srb->sense_buf[13]); @@ -920,6 +944,7 @@ static int usb_request_sense(ccb *srb, struct us_data *ss) static int usb_test_unit_ready(ccb *srb, struct us_data *ss) { int retries = 10; + int result;
do { memset(&srb->cmd[0], 0, 12); @@ -928,7 +953,9 @@ static int usb_test_unit_ready(ccb *srb, struct us_data *ss) srb->cmdlen = 12; if (ss->transport(srb, ss) == USB_STOR_TRANSPORT_GOOD) return 0; - usb_request_sense(srb, ss); + result = usb_request_sense(srb, ss); + if (result == USB_EDEVCRITICAL) + return result; wait_ms(100); } while (retries--);
@@ -1314,6 +1341,7 @@ int usb_stor_get_info(struct usb_device *dev, struct us_data *ss, unsigned long cap[2]; unsigned long *capacity, *blksz; ccb *pccb = &usb_ccb; + int result;
/* for some reasons a couple of devices would not survive this reset */ if ( @@ -1372,11 +1400,14 @@ int usb_stor_get_info(struct usb_device *dev, struct us_data *ss, #endif /* CONFIG_USB_BIN_FIXUP */ USB_STOR_PRINTF("ISO Vers %X, Response Data %X\n", usb_stor_buf[2], usb_stor_buf[3]); - if (usb_test_unit_ready(pccb, ss)) { + result = usb_test_unit_ready(pccb, ss); + if (result) { + if (result == USB_EDEVCRITICAL) + return result; printf("Device NOT ready\n" - " Request Sense returned %02X %02X %02X\n", - pccb->sense_buf[2], pccb->sense_buf[12], - pccb->sense_buf[13]); + " Request Sense returned %02X %02X %02X\n", + pccb->sense_buf[2], pccb->sense_buf[12], + pccb->sense_buf[13]); if (dev_desc->removable == 1) { dev_desc->type = perq; return 1; diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c index 13cd84a..1143cd6 100644 --- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -330,6 +330,7 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, uint32_t c, toggle; uint32_t cmd; int ret = 0; + int result = USB_EFAIL;
#ifdef CONFIG_USB_EHCI_DATA_ALIGN /* In case ehci host requires alignment for buffers */ @@ -342,8 +343,8 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, if (!align_buf) return -1; if ((int)align_buf & (CONFIG_USB_EHCI_DATA_ALIGN - 1)) - buffer = (void *)((int)align_buf + - CONFIG_USB_EHCI_DATA_ALIGN - + buffer = (void *)((int)align_buf + + CONFIG_USB_EHCI_DATA_ALIGN - ((int)align_buf & (CONFIG_USB_EHCI_DATA_ALIGN - 1))); else buffer = align_buf; @@ -475,7 +476,10 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, goto fail; }
- /* Wait for TDs to be processed. */ + /* + * Wait for TDs to be processed. We wait 3s since some USB + * sticks can take a long time immediately after system reset + */ ts = get_timer(0); vtd = td; do { @@ -484,7 +488,7 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, token = hc32_to_cpu(vtd->qt_token); if (!(token & 0x80)) break; - } while (get_timer(ts) < CONFIG_SYS_HZ); + } while (get_timer(ts) < CONFIG_SYS_HZ * 3);
/* Disable async schedule. */ cmd = ehci_readl(&hcor->or_usbcmd); @@ -497,6 +501,12 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, printf("EHCI fail timeout STD_ASS reset\n"); goto fail; } + /* check that the TD processing happened */ + if (token & 0x80) { + printf("EHCI timed out on TD - token=%#x\n", token); + result = USB_EDEVCRITICAL; + goto fail; + }
qh_list.qh_link = cpu_to_hc32((uint32_t)&qh_list | QH_LINK_TYPE_QH);
@@ -559,7 +569,7 @@ fail: td = (void *)hc32_to_cpu(qh->qh_overlay.qt_next); } ehci_free(qh, sizeof(*qh)); - return -1; + return result; }
static inline int min3(int a, int b, int c) diff --git a/include/usb.h b/include/usb.h index afd65e3..91aa441 100644 --- a/include/usb.h +++ b/include/usb.h @@ -42,6 +42,16 @@
#define USB_CNTL_TIMEOUT 100 /* 100ms timeout */
+/* + * Errors we can report, e.g. return USB_EDEVCRITICAL + * Use -ve numbers to fit in with usb_storage + * U-Boot needs some unified numbers + */ +#define USB_EOK 0 /* ok, no error */ +#define USB_EFAIL -1 /* general failure(!) */ +#define USB_EDEVCRITICAL -2 /* must reset device on hub */ +#define USB_ENEXTFREE -3 /* next free error number */ + /* device request (setup) */ struct devrequest { unsigned char requesttype; @@ -198,6 +208,7 @@ int usb_get_class_descriptor(struct usb_device *dev, int ifnum, int usb_clear_halt(struct usb_device *dev, int pipe); int usb_string(struct usb_device *dev, int index, char *buf, size_t size); int usb_set_interface(struct usb_device *dev, int interface, int alternate); +int usb_restart_device(struct usb_device *dev);
/* big endian -> little endian conversion */ /* some CPUs are already little endian e.g. the ARM920T */ -- 1.7.3.1
Hi Simon,
2010/12/10 Simon Glass sjg@chromium.org:
I am sending this to the list looking for comments. Testing has been confined to just a few USB sticks - no USB hard drives, USB card readers, etc. But there are reports on the mailing list of problems with U-Boot's detection of USB mass storage devices.
First impression is that it looks interesting, however I have a few remarks, see below.
This patch:
- Increases the USB descriptor submission timeout to 3 seconds since the
original 1 second timeout seems arbitrary
- Replaces the delay-based reset logic with logic which waits until it
sees the reset is successful, rather than blindly continuing
- Resets and retries a USB mass storage device after it fails once, in
case even 3 seconds is not enough
Can you please split this patch up to multiple patches, each solving a different issue?
BUG=chromiumos-6780 TEST=tried with four different USB sticks, including two which previously failed Change-Id: I5bd8891bff67a74b758e349f2a3b3575806eed59
Signed-off-by: Simon Glass sjg@chromium.org
common/usb.c | 176 ++++++++++++++++++++++++++++++++++++------- common/usb_storage.c | 55 +++++++++++--- drivers/usb/host/ehci-hcd.c | 20 ++++- include/usb.h | 11 +++ 4 files changed, 217 insertions(+), 45 deletions(-)
diff --git a/common/usb.c b/common/usb.c index 9896f46..3265d5d 100644 --- a/common/usb.c +++ b/common/usb.c @@ -222,10 +222,14 @@ int usb_control_msg(struct usb_device *dev, unsigned int pipe, int usb_bulk_msg(struct usb_device *dev, unsigned int pipe, void *data, int len, int *actual_length, int timeout) {
- int err;
if (len < 0) return -1; dev->status = USB_ST_NOT_PROC; /*not yet processed */
- submit_bulk_msg(dev, pipe, data, len);
- err = submit_bulk_msg(dev, pipe, data, len);
- if (err < 0)
- return err;
Seems you only focused on the ehci-hcd driver. How does this change in behaviour impact other host controller drivers like, for example, ohci? Is any regression in those drivers possible or to be expected?
-static int hub_port_reset(struct usb_device *dev, int port,
- unsigned short *portstat)
+/* brought this in from kernel 2.6.36 as it is a problem area. Some USB +sticks do not operate properly with the previous reset code */
Multi line comments should be written like /* * comment line 1 * comment line 2 */ Please fix globally.
+#define PORT_RESET_TRIES 5 +#define SET_ADDRESS_TRIES 2 +#define GET_DESCRIPTOR_TRIES 2 +#define SET_CONFIG_TRIES (2 * (use_both_schemes + 1)) +#define USE_NEW_SCHEME(i) ((i) / 2 == old_scheme_first)
+#define HUB_ROOT_RESET_TIME 50 /* times are in msec */ +#define HUB_SHORT_RESET_TIME 10 +#define HUB_LONG_RESET_TIME 200 +#define HUB_RESET_TIMEOUT 500
How are these times estimated? Are these magic, or is there a deeper thought behind them?
+#define ENOTCONN 107
+static int hub_port_wait_reset(struct usb_device *dev, int port,
- unsigned int delay, unsigned short *portstatus_ret)
{
- int tries;
- int delay_time, ret;
struct usb_port_status portsts; unsigned short portstatus, portchange;
- USB_HUB_PRINTF("hub_port_reset: resetting port %d...\n", port);
- for (tries = 0; tries < MAX_TRIES; tries++) {
- usb_set_port_feature(dev, port + 1, USB_PORT_FEAT_RESET);
- wait_ms(200);
- for (delay_time = 0;
- delay_time < HUB_RESET_TIMEOUT;
You go from a 200msec timeout to a 500msec timeout. Is there a special reason for this?
- delay_time += delay) {
- /* wait to give the device a chance to reset */
- wait_ms(delay);
The ' delay' argument of this routine is misleading since it does not specify a delay time, but the time between 2 polls on usb_get_port_status(). The maximum delay is specified by HUB_RESET_TIMEOUT here.
- if (usb_get_port_status(dev, port + 1, &portsts) < 0) {
- /* read and decode port status */
- ret = usb_get_port_status(dev, port + 1, &portsts);
- if (ret < 0) {
USB_HUB_PRINTF("get_port_status failed status %lX\n", dev->status);
- return -1;
- return ret;
} portstatus = le16_to_cpu(portsts.wPortStatus); portchange = le16_to_cpu(portsts.wPortChange);
- USB_HUB_PRINTF("portstatus %x, change %x, %s\n",
- portstatus, portchange,
- portspeed(portstatus));
- /* Device went away? */
- if (!(portstatus & USB_PORT_STAT_CONNECTION))
- return -ENOTCONN;
- USB_HUB_PRINTF("STAT_C_CONNECTION = %d STAT_CONNECTION = %d" \
- " USB_PORT_STAT_ENABLE %d\n",
- (portchange & USB_PORT_STAT_C_CONNECTION) ? 1 : 0,
- (portstatus & USB_PORT_STAT_CONNECTION) ? 1 : 0,
- (portstatus & USB_PORT_STAT_ENABLE) ? 1 : 0);
- /* bomb out completely if the connection bounced */
- if ((portchange & USB_PORT_STAT_C_CONNECTION))
- return -ENOTCONN;
- if ((portchange & USB_PORT_STAT_C_CONNECTION) ||
- !(portstatus & USB_PORT_STAT_CONNECTION))
- return -1;
- /* if we`ve finished resetting, then break out of the loop */
- if (!(portstatus & USB_PORT_STAT_RESET) &&
- (portstatus & USB_PORT_STAT_ENABLE)) {
- *portstatus_ret = portstatus;
- return 0;
- }
- if (portstatus & USB_PORT_STAT_ENABLE)
- break;
- /* switch to the long delay after two short delay failures */
- if (delay_time >= 2 * HUB_SHORT_RESET_TIME)
- delay = HUB_LONG_RESET_TIME;
You are changing the 'delay' routine argument here. Why even specify it on the argument list if: * it does not do what to expect. * it gets overridden anyway.
- /* root hub ports have a slightly longer reset period
- * (from USB 2.0 spec, section 7.1.7.5)
- */
wrong style multiline comment.
- if (!dev->parent) {
- delay = HUB_ROOT_RESET_TIME;
- }
No parenthesis required.
- wait_ms(200);
- /* Some low speed devices have problems with the quick delay, so */
- /* be a bit pessimistic with those devices. RHbug #23670 */
Multiline comment
- /* TRSTRCY = 10 ms; plus some extra */
magic plus some extra magic...
diff --git a/common/usb_storage.c b/common/usb_storage.c index 76949b8..0fb9c1b 100644 --- a/common/usb_storage.c +++ b/common/usb_storage.c @@ -158,9 +158,10 @@ struct us_data { static struct us_data usb_stor[USB_MAX_STOR_DEV];
+/* start our error numbers after the USB ones */ #define USB_STOR_TRANSPORT_GOOD 0 -#define USB_STOR_TRANSPORT_FAILED -1 -#define USB_STOR_TRANSPORT_ERROR -2 +#define USB_STOR_TRANSPORT_FAILED (USB_ENEXTFREE) +#define USB_STOR_TRANSPORT_ERROR (USB_ENEXTFREE-1)
Weird way to define error codes... Please make it clearer without magic ids that needs to match magically to some ids in a header file (USB_ENEXTFREE = -3) (enum?)
int usb_stor_get_info(struct usb_device *dev, struct us_data *us, block_dev_desc_t *dev_desc); @@ -213,6 +214,7 @@ int usb_stor_scan(int mode) { unsigned char i; struct usb_device *dev;
- int result;
/* GJ */ memset(usb_stor_buf, 0, sizeof(usb_stor_buf)); @@ -244,8 +246,21 @@ int usb_stor_scan(int mode) /* ok, it is a storage devices * get info and fill it in */
Bad style multiline comment
- if (usb_stor_get_info(dev, &usb_stor[usb_max_devs],
- &usb_dev_desc[usb_max_devs]) == 1)
- result = usb_stor_get_info(dev, &usb_stor[usb_max_devs],
- &usb_dev_desc[usb_max_devs]);
- if (result == USB_EDEVCRITICAL) {
You are changing the behaviour for the ehci host controller only (since that one only knows USB_EDEVCRITICAL) Please make it suitable for all other host controller drivers as well, or patch those drivers also. The problems you are trying to fix are not likely to be for ehci only, since you modify generic code...
- /*
- * Something there, but failed badly.
- * Retry one more time. This happens
- * sometimes with some USB sticks,
- * e.g. Patriot Rage ID 13fe:3800
- */
- printf (".");
- usb_restart_device(dev); /* ignore return value */
- result = usb_stor_get_info(dev, &usb_stor[usb_max_devs],
- &usb_dev_desc[usb_max_devs]);
- }
- if (result == 1)
Would a switch case or ' else if' not be nicer?
static int usb_request_sense(ccb *srb, struct us_data *ss) {
- int result;
char *ptr;
ptr = (char *)srb->pdata; @@ -909,7 +928,12 @@ static int usb_request_sense(ccb *srb, struct us_data *ss) srb->datalen = 18; srb->pdata = &srb->sense_buf[0]; srb->cmdlen = 12;
- ss->transport(srb, ss);
- result = ss->transport(srb, ss);
- if (result < 0) {
- if (result != USB_EDEVCRITICAL)
- USB_STOR_PRINTF("Request Sense failed\n");
- return result;
You are changing behaviour for ALL host controller driver types here. Are you sure you want this? (Better: again... please fix all drivers...)
- }
USB_STOR_PRINTF("Request Sense returned %02X %02X %02X\n", srb->sense_buf[2], srb->sense_buf[12], srb->sense_buf[13]);
@@ -1372,11 +1400,14 @@ int usb_stor_get_info(struct usb_device *dev, struct us_data *ss, #endif /* CONFIG_USB_BIN_FIXUP */ USB_STOR_PRINTF("ISO Vers %X, Response Data %X\n", usb_stor_buf[2], usb_stor_buf[3]);
- if (usb_test_unit_ready(pccb, ss)) {
- result = usb_test_unit_ready(pccb, ss);
- if (result) {
- if (result == USB_EDEVCRITICAL)
- return result;
Please fix all drivers...
printf("Device NOT ready\n"
- " Request Sense returned %02X %02X %02X\n",
- pccb->sense_buf[2], pccb->sense_buf[12],
- pccb->sense_buf[13]);
- " Request Sense returned %02X %02X %02X\n",
- pccb->sense_buf[2], pccb->sense_buf[12],
- pccb->sense_buf[13]);
if (dev_desc->removable == 1) { dev_desc->type = perq; return 1; diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c index 13cd84a..1143cd6 100644 --- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -330,6 +330,7 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, uint32_t c, toggle; uint32_t cmd; int ret = 0;
- int result = USB_EFAIL;
#ifdef CONFIG_USB_EHCI_DATA_ALIGN /* In case ehci host requires alignment for buffers */ @@ -342,8 +343,8 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, if (!align_buf) return -1; if ((int)align_buf & (CONFIG_USB_EHCI_DATA_ALIGN - 1))
- buffer = (void *)((int)align_buf +
- CONFIG_USB_EHCI_DATA_ALIGN -
- buffer = (void *)((int)align_buf +
- CONFIG_USB_EHCI_DATA_ALIGN -
((int)align_buf & (CONFIG_USB_EHCI_DATA_ALIGN - 1))); else buffer = align_buf; @@ -475,7 +476,10 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, goto fail; }
- /* Wait for TDs to be processed. */
- /*
- * Wait for TDs to be processed. We wait 3s since some USB
- * sticks can take a long time immediately after system reset
- */
Cool... This multiline comment is correct ;-)
ts = get_timer(0); vtd = td; do { @@ -484,7 +488,7 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, token = hc32_to_cpu(vtd->qt_token); if (!(token & 0x80)) break;
- } while (get_timer(ts) < CONFIG_SYS_HZ);
- } while (get_timer(ts) < CONFIG_SYS_HZ * 3);
Same valid for other host controller drivers?
/* Disable async schedule. */ cmd = ehci_readl(&hcor->or_usbcmd); @@ -497,6 +501,12 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer, printf("EHCI fail timeout STD_ASS reset\n"); goto fail; }
- /* check that the TD processing happened */
- if (token & 0x80) {
- printf("EHCI timed out on TD - token=%#x\n", token);
- result = USB_EDEVCRITICAL;
- goto fail;
- }
this one too...
qh_list.qh_link = cpu_to_hc32((uint32_t)&qh_list | QH_LINK_TYPE_QH);
@@ -559,7 +569,7 @@ fail: td = (void *)hc32_to_cpu(qh->qh_overlay.qt_next); } ehci_free(qh, sizeof(*qh));
- return -1;
- return result;
}
static inline int min3(int a, int b, int c) diff --git a/include/usb.h b/include/usb.h index afd65e3..91aa441 100644 --- a/include/usb.h +++ b/include/usb.h @@ -42,6 +42,16 @@
#define USB_CNTL_TIMEOUT 100 /* 100ms timeout */
+/*
- Errors we can report, e.g. return USB_EDEVCRITICAL
- Use -ve numbers to fit in with usb_storage
Yak...
- U-Boot needs some unified numbers
- */
+#define USB_EOK 0 /* ok, no error */ +#define USB_EFAIL -1 /* general failure(!) */ +#define USB_EDEVCRITICAL -2 /* must reset device on hub */ +#define USB_ENEXTFREE -3 /* next free error number */
/* device request (setup) */ struct devrequest { unsigned char requesttype; @@ -198,6 +208,7 @@ int usb_get_class_descriptor(struct usb_device *dev, int ifnum, int usb_clear_halt(struct usb_device *dev, int pipe); int usb_string(struct usb_device *dev, int index, char *buf, size_t size); int usb_set_interface(struct usb_device *dev, int interface, int alternate); +int usb_restart_device(struct usb_device *dev);
/* big endian -> little endian conversion */ /* some CPUs are already little endian e.g. the ARM920T */
Kind regards,
Remy
Hi Remy,
Thanks for the feedback. I will update the patch with your changes.
Unfortunately I don't have a lot of hardware to try, hence the list post. I will be doing some more testing in January so will come back to it then. In the meantime I am interested in views as to whether this makes a difference on OHCI, different ARM chips, etc.
Regards, Simon
On Sat, Dec 11, 2010 at 9:51 AM, Remy Bohmer linux@bohmer.net wrote:
Hi Simon,
2010/12/10 Simon Glass sjg@chromium.org:
I am sending this to the list looking for comments. Testing has been confined to just a few USB sticks - no USB hard drives, USB card readers, etc. But there are reports on the mailing list of problems with U-Boot's detection of USB mass storage devices.
First impression is that it looks interesting, however I have a few remarks, see below.
This patch:
- Increases the USB descriptor submission timeout to 3 seconds since the
original 1 second timeout seems arbitrary
- Replaces the delay-based reset logic with logic which waits until it
sees the reset is successful, rather than blindly continuing
- Resets and retries a USB mass storage device after it fails once, in
case even 3 seconds is not enough
Can you please split this patch up to multiple patches, each solving a different issue?
BUG=chromiumos-6780 TEST=tried with four different USB sticks, including two which previously failed Change-Id: I5bd8891bff67a74b758e349f2a3b3575806eed59
Signed-off-by: Simon Glass sjg@chromium.org
common/usb.c | 176
++++++++++++++++++++++++++++++++++++-------
common/usb_storage.c | 55 +++++++++++--- drivers/usb/host/ehci-hcd.c | 20 ++++- include/usb.h | 11 +++ 4 files changed, 217 insertions(+), 45 deletions(-)
diff --git a/common/usb.c b/common/usb.c index 9896f46..3265d5d 100644 --- a/common/usb.c +++ b/common/usb.c @@ -222,10 +222,14 @@ int usb_control_msg(struct usb_device *dev,
unsigned int pipe,
int usb_bulk_msg(struct usb_device *dev, unsigned int pipe, void *data, int len, int *actual_length, int
timeout)
{
int err;if (len < 0) return -1; dev->status = USB_ST_NOT_PROC; /*not yet processed */
submit_bulk_msg(dev, pipe, data, len);
err = submit_bulk_msg(dev, pipe, data, len);if (err < 0)return err;Seems you only focused on the ehci-hcd driver. How does this change in behaviour impact other host controller drivers like, for example, ohci? Is any regression in those drivers possible or to be expected?
-static int hub_port_reset(struct usb_device *dev, int port,
unsigned short *portstat)+/* brought this in from kernel 2.6.36 as it is a problem area. Some USB +sticks do not operate properly with the previous reset code */
Multi line comments should be written like /*
- comment line 1
- comment line 2
*/ Please fix globally.
+#define PORT_RESET_TRIES 5 +#define SET_ADDRESS_TRIES 2 +#define GET_DESCRIPTOR_TRIES 2 +#define SET_CONFIG_TRIES (2 * (use_both_schemes + 1)) +#define USE_NEW_SCHEME(i) ((i) / 2 == old_scheme_first)
+#define HUB_ROOT_RESET_TIME 50 /* times are in msec */ +#define HUB_SHORT_RESET_TIME 10 +#define HUB_LONG_RESET_TIME 200 +#define HUB_RESET_TIMEOUT 500
How are these times estimated? Are these magic, or is there a deeper thought behind them?
+#define ENOTCONN 107
+static int hub_port_wait_reset(struct usb_device *dev, int port,
unsigned int delay, unsigned short*portstatus_ret)
{
int tries;
int delay_time, ret; struct usb_port_status portsts; unsigned short portstatus, portchange;
USB_HUB_PRINTF("hub_port_reset: resetting port %d...\n", port);for (tries = 0; tries < MAX_TRIES; tries++) {usb_set_port_feature(dev, port + 1, USB_PORT_FEAT_RESET);wait_ms(200);
for (delay_time = 0;delay_time < HUB_RESET_TIMEOUT;You go from a 200msec timeout to a 500msec timeout. Is there a special reason for this?
delay_time += delay) {/* wait to give the device a chance to reset */wait_ms(delay);The ' delay' argument of this routine is misleading since it does not specify a delay time, but the time between 2 polls on usb_get_port_status(). The maximum delay is specified by HUB_RESET_TIMEOUT here.
if (usb_get_port_status(dev, port + 1, &portsts) < 0) {
/* read and decode port status */ret = usb_get_port_status(dev, port + 1, &portsts);if (ret < 0) { USB_HUB_PRINTF("get_port_status failed status%lX\n",
dev->status);
return -1;
return ret; } portstatus = le16_to_cpu(portsts.wPortStatus); portchange = le16_to_cpu(portsts.wPortChange);
USB_HUB_PRINTF("portstatus %x, change %x, %s\n",portstatus, portchange,portspeed(portstatus));
/* Device went away? */if (!(portstatus & USB_PORT_STAT_CONNECTION))return -ENOTCONN;
USB_HUB_PRINTF("STAT_C_CONNECTION = %d STAT_CONNECTION =%d" \
" USB_PORT_STAT_ENABLE %d\n",(portchange & USB_PORT_STAT_C_CONNECTION) ? 1 :0,
(portstatus & USB_PORT_STAT_CONNECTION) ? 1 : 0,(portstatus & USB_PORT_STAT_ENABLE) ? 1 : 0);
/* bomb out completely if the connection bounced */if ((portchange & USB_PORT_STAT_C_CONNECTION))return -ENOTCONN;
if ((portchange & USB_PORT_STAT_C_CONNECTION) ||!(portstatus & USB_PORT_STAT_CONNECTION))return -1;
/* if we`ve finished resetting, then break out of theloop */
if (!(portstatus & USB_PORT_STAT_RESET) &&(portstatus & USB_PORT_STAT_ENABLE)) {*portstatus_ret = portstatus;return 0;}
if (portstatus & USB_PORT_STAT_ENABLE)break;
/* switch to the long delay after two short delayfailures */
if (delay_time >= 2 * HUB_SHORT_RESET_TIME)delay = HUB_LONG_RESET_TIME;You are changing the 'delay' routine argument here. Why even specify it on the argument list if:
- it does not do what to expect.
- it gets overridden anyway.
/* root hub ports have a slightly longer reset period* (from USB 2.0 spec, section 7.1.7.5)*/wrong style multiline comment.
if (!dev->parent) {delay = HUB_ROOT_RESET_TIME;}No parenthesis required.
wait_ms(200);
/* Some low speed devices have problems with the quick delay, so*/
/* be a bit pessimistic with those devices. RHbug #23670 */Multiline comment
/* TRSTRCY = 10 ms; plus some extra */magic plus some extra magic...
diff --git a/common/usb_storage.c b/common/usb_storage.c index 76949b8..0fb9c1b 100644 --- a/common/usb_storage.c +++ b/common/usb_storage.c @@ -158,9 +158,10 @@ struct us_data { static struct us_data usb_stor[USB_MAX_STOR_DEV];
+/* start our error numbers after the USB ones */ #define USB_STOR_TRANSPORT_GOOD 0 -#define USB_STOR_TRANSPORT_FAILED -1 -#define USB_STOR_TRANSPORT_ERROR -2 +#define USB_STOR_TRANSPORT_FAILED (USB_ENEXTFREE) +#define USB_STOR_TRANSPORT_ERROR (USB_ENEXTFREE-1)
Weird way to define error codes... Please make it clearer without magic ids that needs to match magically to some ids in a header file (USB_ENEXTFREE = -3) (enum?)
int usb_stor_get_info(struct usb_device *dev, struct us_data *us, block_dev_desc_t *dev_desc); @@ -213,6 +214,7 @@ int usb_stor_scan(int mode) { unsigned char i; struct usb_device *dev;
int result; /* GJ */ memset(usb_stor_buf, 0, sizeof(usb_stor_buf));@@ -244,8 +246,21 @@ int usb_stor_scan(int mode) /* ok, it is a storage devices * get info and fill it in */
Bad style multiline comment
if (usb_stor_get_info(dev,&usb_stor[usb_max_devs],
&usb_dev_desc[usb_max_devs]) == 1)
result = usb_stor_get_info(dev,&usb_stor[usb_max_devs],
&usb_dev_desc[usb_max_devs]);
if (result == USB_EDEVCRITICAL) {You are changing the behaviour for the ehci host controller only (since that one only knows USB_EDEVCRITICAL) Please make it suitable for all other host controller drivers as well, or patch those drivers also. The problems you are trying to fix are not likely to be for ehci only, since you modify generic code...
/** Something there, but failed badly.* Retry one more time. This happens* sometimes with some USB sticks,* e.g. Patriot Rage ID 13fe:3800*/printf (".");usb_restart_device(dev); /* ignorereturn value */
result = usb_stor_get_info(dev,&usb_stor[usb_max_devs],
&usb_dev_desc[usb_max_devs]);
}if (result == 1)Would a switch case or ' else if' not be nicer?
static int usb_request_sense(ccb *srb, struct us_data *ss) {
int result; char *ptr; ptr = (char *)srb->pdata;@@ -909,7 +928,12 @@ static int usb_request_sense(ccb *srb, struct
us_data *ss)
srb->datalen = 18; srb->pdata = &srb->sense_buf[0]; srb->cmdlen = 12;
ss->transport(srb, ss);
result = ss->transport(srb, ss);if (result < 0) {if (result != USB_EDEVCRITICAL)USB_STOR_PRINTF("Request Sense failed\n");return result;You are changing behaviour for ALL host controller driver types here. Are you sure you want this? (Better: again... please fix all drivers...)
} USB_STOR_PRINTF("Request Sense returned %02X %02X %02X\n", srb->sense_buf[2], srb->sense_buf[12], srb->sense_buf[13]);@@ -1372,11 +1400,14 @@ int usb_stor_get_info(struct usb_device *dev,
struct us_data *ss,
#endif /* CONFIG_USB_BIN_FIXUP */ USB_STOR_PRINTF("ISO Vers %X, Response Data %X\n",
usb_stor_buf[2],
usb_stor_buf[3]);
if (usb_test_unit_ready(pccb, ss)) {
result = usb_test_unit_ready(pccb, ss);if (result) {if (result == USB_EDEVCRITICAL)return result;Please fix all drivers...
printf("Device NOT ready\n"
" Request Sense returned %02X %02X %02X\n",pccb->sense_buf[2], pccb->sense_buf[12],pccb->sense_buf[13]);
" Request Sense returned %02X %02X %02X\n",pccb->sense_buf[2], pccb->sense_buf[12],pccb->sense_buf[13]); if (dev_desc->removable == 1) { dev_desc->type = perq; return 1;diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c index 13cd84a..1143cd6 100644 --- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -330,6 +330,7 @@ ehci_submit_async(struct usb_device *dev, unsigned
long pipe, void *buffer,
uint32_t c, toggle; uint32_t cmd; int ret = 0;
int result = USB_EFAIL;#ifdef CONFIG_USB_EHCI_DATA_ALIGN /* In case ehci host requires alignment for buffers */ @@ -342,8 +343,8 @@ ehci_submit_async(struct usb_device *dev, unsigned
long pipe, void *buffer,
if (!align_buf) return -1; if ((int)align_buf & (CONFIG_USB_EHCI_DATA_ALIGN - 1))
buffer = (void *)((int)align_buf +CONFIG_USB_EHCI_DATA_ALIGN -
buffer = (void *)((int)align_buf +CONFIG_USB_EHCI_DATA_ALIGN - ((int)align_buf &(CONFIG_USB_EHCI_DATA_ALIGN - 1)));
else buffer = align_buf;@@ -475,7 +476,10 @@ ehci_submit_async(struct usb_device *dev, unsigned
long pipe, void *buffer,
goto fail; }
/* Wait for TDs to be processed. */
/** Wait for TDs to be processed. We wait 3s since some USB* sticks can take a long time immediately after system reset*/Cool... This multiline comment is correct ;-)
ts = get_timer(0); vtd = td; do {@@ -484,7 +488,7 @@ ehci_submit_async(struct usb_device *dev, unsigned
long pipe, void *buffer,
token = hc32_to_cpu(vtd->qt_token); if (!(token & 0x80)) break;
} while (get_timer(ts) < CONFIG_SYS_HZ);
} while (get_timer(ts) < CONFIG_SYS_HZ * 3);Same valid for other host controller drivers?
/* Disable async schedule. */ cmd = ehci_readl(&hcor->or_usbcmd);@@ -497,6 +501,12 @@ ehci_submit_async(struct usb_device *dev, unsigned
long pipe, void *buffer,
printf("EHCI fail timeout STD_ASS reset\n"); goto fail; }
/* check that the TD processing happened */if (token & 0x80) {printf("EHCI timed out on TD - token=%#x\n", token);result = USB_EDEVCRITICAL;goto fail;}this one too...
qh_list.qh_link = cpu_to_hc32((uint32_t)&qh_list |QH_LINK_TYPE_QH);
@@ -559,7 +569,7 @@ fail: td = (void *)hc32_to_cpu(qh->qh_overlay.qt_next); } ehci_free(qh, sizeof(*qh));
return -1;
return result;}
static inline int min3(int a, int b, int c) diff --git a/include/usb.h b/include/usb.h index afd65e3..91aa441 100644 --- a/include/usb.h +++ b/include/usb.h @@ -42,6 +42,16 @@
#define USB_CNTL_TIMEOUT 100 /* 100ms timeout */
+/*
- Errors we can report, e.g. return USB_EDEVCRITICAL
- Use -ve numbers to fit in with usb_storage
Yak...
- U-Boot needs some unified numbers
- */
+#define USB_EOK 0 /* ok, no error */ +#define USB_EFAIL -1 /* general failure(!) */ +#define USB_EDEVCRITICAL -2 /* must reset device on hub */ +#define USB_ENEXTFREE -3 /* next free error number */
/* device request (setup) */ struct devrequest { unsigned char requesttype; @@ -198,6 +208,7 @@ int usb_get_class_descriptor(struct usb_device *dev,
int ifnum,
int usb_clear_halt(struct usb_device *dev, int pipe); int usb_string(struct usb_device *dev, int index, char *buf, size_t
size);
int usb_set_interface(struct usb_device *dev, int interface, int
alternate);
+int usb_restart_device(struct usb_device *dev);
/* big endian -> little endian conversion */ /* some CPUs are already little endian e.g. the ARM920T */
Kind regards,
Remy
Hi Simon,
2010/12/16 Simon Glass sjg@chromium.org:
Hi Remy, Thanks for the feedback. I will update the patch with your changes. Unfortunately I don't have a lot of hardware to try, hence the list post. I will be doing some more testing in January so will come back to it then. In
Have you made any progress about this yet?
Kind regards,
Remy
Hi,
2011/1/31 Remy Bohmer linux@bohmer.net:
Hi Simon,
2010/12/16 Simon Glass sjg@chromium.org:
Hi Remy, Thanks for the feedback. I will update the patch with your changes. Unfortunately I don't have a lot of hardware to try, hence the list post. I will be doing some more testing in January so will come back to it then. In
Have you made any progress about this yet?
FYI: I have stored this patch in my u-boot-usb testing branch after rebasing to the current mainline.
Kind regards,
Remy
Hi Remy,
Still waiting on some boards to try, unfortunately. I expect it will happen in the next 2 weeks though (ARM9 and OMAP4 platforms). Once I get to the bottom of it I will split the patches as requested. I have not seen reports from other users of U-Boot. Also working on USB host Ethernet.
Regards, Simon
On Mon, Jan 31, 2011 at 11:54 AM, Remy Bohmer linux@bohmer.net wrote:
Hi,
2011/1/31 Remy Bohmer linux@bohmer.net:
Hi Simon,
2010/12/16 Simon Glass sjg@chromium.org:
Hi Remy, Thanks for the feedback. I will update the patch with your changes. Unfortunately I don't have a lot of hardware to try, hence the list
post. I
will be doing some more testing in January so will come back to it then.
In
Have you made any progress about this yet?
FYI: I have stored this patch in my u-boot-usb testing branch after rebasing to the current mainline.
Kind regards,
Remy
Hi,
2011/2/1 Simon Glass sjg@chromium.org:
Hi Remy, Still waiting on some boards to try, unfortunately. I expect it will happen in the next 2 weeks though (ARM9 and OMAP4 platforms). Once I get to the bottom of it I will split the patches as requested. I have not seen reports from other users of U-Boot. Also working on USB host Ethernet. Regards, Simon
FWIW: I tested it on Atmel at91 and it does not seem to break OHCI, it does not seem to fix some troubles USB sticks I have here either...
Kind regards,
Remy
I too am interested in this. I am running EHCI and have had problems with a number of USB storage devices, one of which (the SanDisk Cruzer) causes a crash due to it reporting the maximum LUN as 1. I'm also seeing some interesting performance issues with ext2 being extremely slow compared to FAT.
Additionally, I have been trying various USB hubs with EHCI and have found that a large percentage fail to work. Hopefully I'll look into it more soon when I get ahold of our USB analyzer again.
-Aaron
On Tuesday, February 01, 2011 11:34:00 am Remy Bohmer wrote:
Hi,
2011/2/1 Simon Glass sjg@chromium.org:
Hi Remy, Still waiting on some boards to try, unfortunately. I expect it will happen in the next 2 weeks though (ARM9 and OMAP4 platforms). Once I get to the bottom of it I will split the patches as requested. I have not seen reports from other users of U-Boot. Also working on USB host Ethernet. Regards, Simon
FWIW: I tested it on Atmel at91 and it does not seem to break OHCI, it does not seem to fix some troubles USB sticks I have here either...
Kind regards,
Remy
Hi Aaron,
OK good. Yes I notice ext2 is slow also, but the dcache is still off so I assumed that was it.
I have had no hub problems, but have only tried two types. I do recall a LUN patch floating around. I was going to try with an uSD card to see if it is needed there.
Regards, Simon
On Tue, Feb 1, 2011 at 4:03 PM, Aaron Williams < Aaron.Williams@caviumnetworks.com> wrote:
I too am interested in this. I am running EHCI and have had problems with a number of USB storage devices, one of which (the SanDisk Cruzer) causes a crash due to it reporting the maximum LUN as 1. I'm also seeing some interesting performance issues with ext2 being extremely slow compared to FAT.
Additionally, I have been trying various USB hubs with EHCI and have found that a large percentage fail to work. Hopefully I'll look into it more soon when I get ahold of our USB analyzer again.
-Aaron
On Tuesday, February 01, 2011 11:34:00 am Remy Bohmer wrote:
Hi,
2011/2/1 Simon Glass sjg@chromium.org:
Hi Remy, Still waiting on some boards to try, unfortunately. I expect it will happen in the next 2 weeks though (ARM9 and OMAP4 platforms). Once I
get
to the bottom of it I will split the patches as requested. I have not seen reports from other users of U-Boot. Also working on USB host Ethernet. Regards, Simon
FWIW: I tested it on Atmel at91 and it does not seem to break OHCI, it does not seem to fix some troubles USB sticks I have here either...
Kind regards,
Remy
We have our cache enabled since we're a cache coherent architecture so the performance is not due to cache (otherwise FAT would also be slow).
The hub that's failing has a transaction translator built in whereas the ones that work do not. For some reason it fails trying to reset this hub.
Granted, this hub is getting a lot of poor reviews, though I suspect the hub draws a lot more power than advertized due to all of its insanely bright LEDs. At least from within Linux it seems to work fine, both the embedded board and my desktop.
http://terminus-tech.com/images/FE2.1%20Product%20Brief%20(Rev.%201.0).pdf
http://www.amazon.com/Hi-Speed-Port-USB-Power- Adapter/dp/B000Y8EXSS/ref=sr_1_16?s=electronics&ie=UTF8&qid=1296691437&sr=1-16
# lsusb -v -s 1:12
Bus 001 Device 012: ID 1a40:0201 TERMINUS TECHNOLOGY INC. Device Descriptor: bLength 18 bDescriptorType 1 bcdUSB 2.00 bDeviceClass 9 Hub bDeviceSubClass 0 Unused bDeviceProtocol 2 TT per port bMaxPacketSize0 64 idVendor 0x1a40 TERMINUS TECHNOLOGY INC. idProduct 0x0201 bcdDevice 1.00 iManufacturer 0 iProduct 1 USB 2.0 Hub [MTT] iSerial 0 bNumConfigurations 1 Configuration Descriptor: bLength 9 bDescriptorType 2 wTotalLength 41 bNumInterfaces 1 bConfigurationValue 1 iConfiguration 0 bmAttributes 0xe0 Self Powered Remote Wakeup MaxPower 100mA Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber 0 bAlternateSetting 0 bNumEndpoints 1 bInterfaceClass 9 Hub bInterfaceSubClass 0 Unused bInterfaceProtocol 1 Single TT iInterface 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x81 EP 1 IN bmAttributes 3 Transfer Type Interrupt Synch Type None Usage Type Data wMaxPacketSize 0x0001 1x 1 bytes bInterval 12 Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber 0 bAlternateSetting 1 bNumEndpoints 1 bInterfaceClass 9 Hub bInterfaceSubClass 0 Unused bInterfaceProtocol 2 TT per port iInterface 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x81 EP 1 IN bmAttributes 3 Transfer Type Interrupt Synch Type None Usage Type Data wMaxPacketSize 0x0001 1x 1 bytes bInterval 12 Hub Descriptor: bLength 9 bDescriptorType 41 nNbrPorts 7 wHubCharacteristic 0x0088 Ganged power switching Per-port overcurrent protection TT think time 8 FS bits Port indicators bPwrOn2PwrGood 50 * 2 milli seconds bHubContrCurrent 100 milli Ampere DeviceRemovable 0x00 PortPwrCtrlMask 0xff Hub Port Status: Port 1: 0000.0100 power Port 2: 0000.0100 power Port 3: 0000.0100 power Port 4: 0000.0100 power Port 5: 0000.0100 power Port 6: 0000.0100 power Port 7: 0000.0100 power Device Qualifier (for other device speed): bLength 10 bDescriptorType 6 bcdUSB 2.00 bDeviceClass 9 Hub bDeviceSubClass 0 Unused bDeviceProtocol 0 Full speed (or root) hub bMaxPacketSize0 64 bNumConfigurations 1 Device Status: 0x0001 Self Powered
On Tuesday, February 01, 2011 05:46:51 pm Simon Glass wrote:
Hi Aaron,
OK good. Yes I notice ext2 is slow also, but the dcache is still off so I assumed that was it.
I have had no hub problems, but have only tried two types. I do recall a LUN patch floating around. I was going to try with an uSD card to see if it is needed there.
Regards, Simon
On Tue, Feb 1, 2011 at 4:03 PM, Aaron Williams <
Aaron.Williams@caviumnetworks.com> wrote:
I too am interested in this. I am running EHCI and have had problems with a number of USB storage devices, one of which (the SanDisk Cruzer) causes a crash due to it reporting the maximum LUN as 1. I'm also seeing some interesting performance issues with ext2 being extremely slow compared to FAT.
Additionally, I have been trying various USB hubs with EHCI and have found that a large percentage fail to work. Hopefully I'll look into it more soon when I get ahold of our USB analyzer again.
-Aaron
On Tuesday, February 01, 2011 11:34:00 am Remy Bohmer wrote:
Hi,
2011/2/1 Simon Glass sjg@chromium.org:
Hi Remy, Still waiting on some boards to try, unfortunately. I expect it will happen in the next 2 weeks though (ARM9 and OMAP4 platforms). Once I
get
to the bottom of it I will split the patches as requested. I have not seen reports from other users of U-Boot. Also working on USB host Ethernet. Regards, Simon
FWIW: I tested it on Atmel at91 and it does not seem to break OHCI, it does not seem to fix some troubles USB sticks I have here either...
Kind regards,
Remy
Hi Remy,
I have now tested on OHCI with AT91SAM9G45 and found that the code already includes a longer timeout and does not have the submission bug. So for now I would like to replace that patch with a new one which I will send to the list today.
For now I will drop the reset and restart parts of the patch since I don't have evidence that they are required. I still believe the reset is too ad-hoc, so am happy to resubmit that part of it if you like.
Regarding your troubled sticks, can you please advise make / model numbers and USB ID? It would be good to at least look at these problems and see what can be done.
Regards, Simon
On Tue, Feb 1, 2011 at 11:34 AM, Remy Bohmer linux@bohmer.net wrote:
Hi,
2011/2/1 Simon Glass sjg@chromium.org:
Hi Remy, Still waiting on some boards to try, unfortunately. I expect it will happen in the next 2 weeks though (ARM9 and OMAP4 platforms). Once I get to the bottom of it I will split the patches as requested. I have not seen reports from other users of U-Boot. Also working on USB host Ethernet. Regards, Simon
FWIW: I tested it on Atmel at91 and it does not seem to break OHCI, it does not seem to fix some troubles USB sticks I have here either...
Kind regards,
Remy
Hi,
2011/2/7 Simon Glass sjg@chromium.org:
Hi Remy,
I have now tested on OHCI with AT91SAM9G45 and found that the code already includes a longer timeout and does not have the submission bug. So for now I would like to replace that patch with a new one which I will send to the list today.
OK
For now I will drop the reset and restart parts of the patch since I don't have evidence that they are required. I still believe the reset is too ad-hoc, so am happy to resubmit that part of it if you like.
Regarding your troubled sticks, can you please advise make / model numbers and USB ID? It would be good to at least look at these problems and see what can be done.
I have for example these sticks that show problems on a OHCI host: 13fe:1a20 Kingston Technology Company Inc. 0204:6025 Chipsbank Microelectronics Co., Ltd CBM2080 Flash drive controller 2008:2018
Note that these sticks give only problems on a at91sam9261 processor if it runs on 200MHz (10MHz X-tal) and not if it runs on 198.656 MHz (18.432 MHz X-tal) One stick also stop showing the problem if it has been powered on for about 10 minutes...(Then it feels a little warmer). I have more sticks of the same brand/type/manufacturing-date that do not show a problem at all... Furthermore, the exact same software is being used in both cases, and same stick with same contents. U-boot since 2010.06 seems to be more sensitive to make these sticks fail, so there seems to be some regression here. Failure is usually being visualised as a stall of an endpoint. Sometimes failure of the stick is that hard, that a power cycle of the stick is required.
I have not had the time to hook up the USB analyser to see what exactly is going wrong, but my guts tell me that there are some tricky timing issues involved.
Kind regards,
Remy
participants (3)
-
Aaron Williams -
Remy Bohmer -
Simon Glass