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 <li...@bohmer.net> wrote:
> Hi Simon,
>
> 2010/12/10 Simon Glass <s...@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 <s...@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
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
