* [U-Boot] [PATCH] Fix to make U-Boot work with more USB sticks
@ 2010-12-10 0:54 Simon Glass
2010-12-11 17:51 ` Remy Bohmer
0 siblings, 1 reply; 12+ messages in thread
From: Simon Glass @ 2010-12-10 0:54 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] Fix to make U-Boot work with more USB sticks
2010-12-10 0:54 [U-Boot] [PATCH] Fix to make U-Boot work with more USB sticks Simon Glass
@ 2010-12-11 17:51 ` Remy Bohmer
2010-12-16 22:14 ` Simon Glass
0 siblings, 1 reply; 12+ messages in thread
From: Remy Bohmer @ 2010-12-11 17:51 UTC (permalink / raw)
To: u-boot
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:
>
> 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
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] Fix to make U-Boot work with more USB sticks
2010-12-11 17:51 ` Remy Bohmer
@ 2010-12-16 22:14 ` Simon Glass
2011-01-31 19:24 ` Remy Bohmer
0 siblings, 1 reply; 12+ messages in thread
From: Simon Glass @ 2010-12-16 22:14 UTC (permalink / raw)
To: u-boot
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:
> >
> > 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
>
> 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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] Fix to make U-Boot work with more USB sticks
2010-12-16 22:14 ` Simon Glass
@ 2011-01-31 19:24 ` Remy Bohmer
2011-01-31 19:54 ` Remy Bohmer
0 siblings, 1 reply; 12+ messages in thread
From: Remy Bohmer @ 2011-01-31 19:24 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] Fix to make U-Boot work with more USB sticks
2011-01-31 19:24 ` Remy Bohmer
@ 2011-01-31 19:54 ` Remy Bohmer
2011-02-01 2:16 ` Simon Glass
0 siblings, 1 reply; 12+ messages in thread
From: Remy Bohmer @ 2011-01-31 19:54 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] Fix to make U-Boot work with more USB sticks
2011-01-31 19:54 ` Remy Bohmer
@ 2011-02-01 2:16 ` Simon Glass
2011-02-01 19:34 ` Remy Bohmer
0 siblings, 1 reply; 12+ messages in thread
From: Simon Glass @ 2011-02-01 2:16 UTC (permalink / raw)
To: u-boot
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] Fix to make U-Boot work with more USB sticks
2011-02-01 2:16 ` Simon Glass
@ 2011-02-01 19:34 ` Remy Bohmer
2011-02-02 0:03 ` Aaron Williams
2011-02-07 22:30 ` Simon Glass
0 siblings, 2 replies; 12+ messages in thread
From: Remy Bohmer @ 2011-02-01 19:34 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] Fix to make U-Boot work with more USB sticks
2011-02-01 19:34 ` Remy Bohmer
@ 2011-02-02 0:03 ` Aaron Williams
2011-02-02 1:46 ` Simon Glass
2011-02-07 22:30 ` Simon Glass
1 sibling, 1 reply; 12+ messages in thread
From: Aaron Williams @ 2011-02-02 0:03 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] Fix to make U-Boot work with more USB sticks
2011-02-02 0:03 ` Aaron Williams
@ 2011-02-02 1:46 ` Simon Glass
2011-02-03 0:25 ` Aaron Williams
0 siblings, 1 reply; 12+ messages in thread
From: Simon Glass @ 2011-02-02 1:46 UTC (permalink / raw)
To: u-boot
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] Fix to make U-Boot work with more USB sticks
2011-02-02 1:46 ` Simon Glass
@ 2011-02-03 0:25 ` Aaron Williams
0 siblings, 0 replies; 12+ messages in thread
From: Aaron Williams @ 2011-02-03 0:25 UTC (permalink / raw)
To: u-boot
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 at 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] Fix to make U-Boot work with more USB sticks
2011-02-01 19:34 ` Remy Bohmer
2011-02-02 0:03 ` Aaron Williams
@ 2011-02-07 22:30 ` Simon Glass
2011-02-11 20:08 ` Remy Bohmer
1 sibling, 1 reply; 12+ messages in thread
From: Simon Glass @ 2011-02-07 22:30 UTC (permalink / raw)
To: u-boot
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH] Fix to make U-Boot work with more USB sticks
2011-02-07 22:30 ` Simon Glass
@ 2011-02-11 20:08 ` Remy Bohmer
0 siblings, 0 replies; 12+ messages in thread
From: Remy Bohmer @ 2011-02-11 20:08 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2011-02-11 20:08 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-12-10 0:54 [U-Boot] [PATCH] Fix to make U-Boot work with more USB sticks Simon Glass
2010-12-11 17:51 ` Remy Bohmer
2010-12-16 22:14 ` Simon Glass
2011-01-31 19:24 ` Remy Bohmer
2011-01-31 19:54 ` Remy Bohmer
2011-02-01 2:16 ` Simon Glass
2011-02-01 19:34 ` Remy Bohmer
2011-02-02 0:03 ` Aaron Williams
2011-02-02 1:46 ` Simon Glass
2011-02-03 0:25 ` Aaron Williams
2011-02-07 22:30 ` Simon Glass
2011-02-11 20:08 ` Remy Bohmer
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.