[U-Boot] [PATCH] Fix to make U-Boot work with more USB sticks
Remy Bohmer
linux at bohmer.net
Sat Dec 11 18:51:05 CET 2010
Hi Simon,
2010/12/10 Simon Glass <sjg at 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 at 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
More information about the U-Boot
mailing list