[U-Boot] [PATCH 1/7] usb: Don't init pointer to zero, but NULL
The pointer should always be inited to NULL, not zero (0). These are two different things and not necessarily equal.
Signed-off-by: Marek Vasut marex@denx.de Cc: Chin Liang See clsee@altera.com Cc: Dinh Nguyen dinguyen@opensource.altera.com Cc: Hans de Goede hdegoede@redhat.com Cc: Stefan Roese sr@denx.de Cc: Stephen Warren swarren@nvidia.com --- common/usb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/usb.c b/common/usb.c index 4d0de4d..63429d4 100644 --- a/common/usb.c +++ b/common/usb.c @@ -1064,7 +1064,7 @@ static int usb_prepare_device(struct usb_device *dev, int addr, bool do_read,
int usb_select_config(struct usb_device *dev) { - unsigned char *tmpbuf = 0; + unsigned char *tmpbuf = NULL; int err;
err = get_descriptor_len(dev, USB_DT_DEVICE_SIZE, USB_DT_DEVICE_SIZE);
The USB function can respond to a Setup packet with ACK or, in case it's busy, it can ignore the Setup packet. The USB function usually gets busy if we hammer it with Control EP transfers too much (ie. sending multiple Get Descriptor requests in a single microframe tends to trigger it on certain USB sticks). The DWC2 controller will interpret not receiving an ACK after Setup packet as XACTERR. Check for this condition and if it happens, retry sending the Setup packet.
Signed-off-by: Marek Vasut marex@denx.de Cc: Chin Liang See clsee@altera.com Cc: Dinh Nguyen dinguyen@opensource.altera.com Cc: Hans de Goede hdegoede@redhat.com Cc: Stefan Roese sr@denx.de Cc: Stephen Warren swarren@nvidia.com --- drivers/usb/host/dwc2.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c index 30b51b3..8d3949e 100644 --- a/drivers/usb/host/dwc2.c +++ b/drivers/usb/host/dwc2.c @@ -737,6 +737,7 @@ int wait_for_chhltd(struct dwc2_hc_regs *hc_regs, uint32_t *sub, u8 *toggle) { int ret; uint32_t hcint, hctsiz; + u8 pid = *toggle;
ret = wait_for_bit(__func__, &hc_regs->hcint, DWC2_HCINT_CHHLTD, true, 1000, false); @@ -758,6 +759,19 @@ int wait_for_chhltd(struct dwc2_hc_regs *hc_regs, uint32_t *sub, u8 *toggle) if (hcint & (DWC2_HCINT_NAK | DWC2_HCINT_FRMOVRUN)) return -EAGAIN;
+ /* + * The USB function can respond to a Setup packet with ACK or, in + * case it's busy, it can ignore the Setup packet. The USB function + * usually gets busy if we hammer it with Control EP transfers too + * much (ie. sending multiple Get Descriptor requests in a single + * microframe tends to trigger it on certain USB sticks). The DWC2 + * controller will interpret not receiving an ACK after Setup packet + * as XACTERR. Check for this condition and if it happens, retry + * sending the Setup packet. + */ + if (hcint & DWC2_HCINT_XACTERR && (pid == DWC2_HC_PID_SETUP)) + return -EAGAIN; + debug("%s: Error (HCINT=%08x)\n", __func__, hcint); return -EINVAL; }
On Tue, 2016-05-03 at 22:51 +0200, Marek Vasut wrote:
The USB function can respond to a Setup packet with ACK or, in case it's busy, it can ignore the Setup packet. The USB function usually gets busy if we hammer it with Control EP transfers too much (ie. sending multiple Get Descriptor requests in a single microframe tends to trigger it on certain USB sticks). The DWC2 controller will interpret not receiving an ACK after Setup packet as XACTERR. Check for this condition and if it happens, retry sending the Setup packet.
Signed-off-by: Marek Vasut marex@denx.de Cc: Chin Liang See clsee@altera.com Cc: Dinh Nguyen dinguyen@opensource.altera.com Cc: Hans de Goede hdegoede@redhat.com Cc: Stefan Roese sr@denx.de Cc: Stephen Warren swarren@nvidia.com
drivers/usb/host/dwc2.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
Reviewed-by: Chin Liang See clsee@altera.com Tested-by: Chin Liang See clsee@altera.com
Thanks Chin Liang
Abort the request in case any of the tokens in the packet failed to complete transfer 10 times. This is a precaution needed so that we don't end in endless loop when scanning the bus with some braindead devices.
Signed-off-by: Marek Vasut marex@denx.de Cc: Chin Liang See clsee@altera.com Cc: Dinh Nguyen dinguyen@opensource.altera.com Cc: Hans de Goede hdegoede@redhat.com Cc: Stefan Roese sr@denx.de Cc: Stephen Warren swarren@nvidia.com --- drivers/usb/host/dwc2.c | 44 ++++++++++++++++++++++++++++++++------------ 1 file changed, 32 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c index 8d3949e..27fcf7c 100644 --- a/drivers/usb/host/dwc2.c +++ b/drivers/usb/host/dwc2.c @@ -993,6 +993,8 @@ static int _submit_control_msg(struct dwc2_priv *priv, struct usb_device *dev, u8 pid; /* For CONTROL endpoint pid should start with DATA1 */ int status_direction; + int againctr = 0; + const int againmax = 10;
if (devnum == priv->root_hub_devnum) { dev->status = 0; @@ -1003,25 +1005,37 @@ static int _submit_control_msg(struct dwc2_priv *priv, struct usb_device *dev,
/* SETUP stage */ pid = DWC2_HC_PID_SETUP; - do { + againctr = 0; + while (true) { ret = chunk_msg(priv, dev, pipe, &pid, 0, setup, 8); - } while (ret == -EAGAIN); - if (ret) - return ret; + if (!ret) + break; + if (ret != -EAGAIN) + return ret; + if (againctr == againmax) + return -EINVAL; + againctr++; + };
/* DATA stage */ act_len = 0; if (buffer) { pid = DWC2_HC_PID_DATA1; - do { + againctr = 0; + while (true) { ret = chunk_msg(priv, dev, pipe, &pid, usb_pipein(pipe), buffer, len); act_len += dev->act_len; buffer += dev->act_len; len -= dev->act_len; - } while (ret == -EAGAIN); - if (ret) - return ret; + if (!ret) + break; + if (ret != -EAGAIN) + return ret; + if (againctr == againmax) + return -EINVAL; + againctr++; + }; status_direction = usb_pipeout(pipe); } else { /* No-data CONTROL always ends with an IN transaction */ @@ -1030,12 +1044,18 @@ static int _submit_control_msg(struct dwc2_priv *priv, struct usb_device *dev,
/* STATUS stage */ pid = DWC2_HC_PID_DATA1; - do { + againctr = 0; + while (true) { ret = chunk_msg(priv, dev, pipe, &pid, status_direction, priv->status_buffer, 0); - } while (ret == -EAGAIN); - if (ret) - return ret; + if (!ret) + break; + if (ret != -EAGAIN) + return ret; + if (againctr == againmax) + return -EINVAL; + againctr++; + };
dev->act_len = act_len;
On Tue, 2016-05-03 at 22:51 +0200, Marek Vasut wrote:
Abort the request in case any of the tokens in the packet failed to complete transfer 10 times. This is a precaution needed so that we don't end in endless loop when scanning the bus with some braindead devices.
Signed-off-by: Marek Vasut marex@denx.de Cc: Chin Liang See clsee@altera.com Cc: Dinh Nguyen dinguyen@opensource.altera.com Cc: Hans de Goede hdegoede@redhat.com Cc: Stefan Roese sr@denx.de Cc: Stephen Warren swarren@nvidia.com
drivers/usb/host/dwc2.c | 44 ++++++++++++++++++++++++++++++++-------
1 file changed, 32 insertions(+), 12 deletions(-)
Reviewed-by: Chin Liang See clsee@altera.com Tested-by: Chin Liang See clsee@altera.com
Thanks Chin Liang
On 05/03/2016 02:51 PM, Marek Vasut wrote:
Abort the request in case any of the tokens in the packet failed to complete transfer 10 times. This is a precaution needed so that we don't end in endless loop when scanning the bus with some braindead devices.
Does this affect USB keyboards when SYS_USB_EVENT_POLL_VIA_CONTROL_EP is enabled? IIRC control transactions to HID devices can be held off for some duration based on polling intervals, and this patch might abort them early?
Or do we typically expect to use interrupt transfers for keyboards, so this isn't too relevant (although there are some platforms that enable SYS_USB_EVENT_POLL_VIA_CONTROL_EP). Maybe not DWC2 platforms though; I didn't check.
On 05/04/2016 07:08 PM, Stephen Warren wrote:
On 05/03/2016 02:51 PM, Marek Vasut wrote:
Abort the request in case any of the tokens in the packet failed to complete transfer 10 times. This is a precaution needed so that we don't end in endless loop when scanning the bus with some braindead devices.
Does this affect USB keyboards when SYS_USB_EVENT_POLL_VIA_CONTROL_EP is enabled? IIRC control transactions to HID devices can be held off for some duration based on polling intervals, and this patch might abort them early?
I didn't try this with keyboard, so I am not quite sure on this one. Do you have RPi zero or somesuch on which you could try ?
btw are usb 1.1 keyboards supposed to work with DWC2 in U-Boot ?
Or do we typically expect to use interrupt transfers for keyboards, so this isn't too relevant (although there are some platforms that enable SYS_USB_EVENT_POLL_VIA_CONTROL_EP). Maybe not DWC2 platforms though; I didn't check.
The platforms which enable POLL_VIA_CONTROL_EP are all chipidea otg, so this should be fine.
On 05/04/2016 07:08 PM, Stephen Warren wrote:
On 05/03/2016 02:51 PM, Marek Vasut wrote:
Abort the request in case any of the tokens in the packet failed to complete transfer 10 times. This is a precaution needed so that we don't end in endless loop when scanning the bus with some braindead devices.
Does this affect USB keyboards when SYS_USB_EVENT_POLL_VIA_CONTROL_EP is enabled? IIRC control transactions to HID devices can be held off for some duration based on polling intervals, and this patch might abort them early?
Or do we typically expect to use interrupt transfers for keyboards, so this isn't too relevant (although there are some platforms that enable SYS_USB_EVENT_POLL_VIA_CONTROL_EP). Maybe not DWC2 platforms though; I didn't check.
Hm, I found one keyboard with which I have trouble with 2/7 and 3/7 , so I will drop them from my USB PR. This means half of my USB sticks will no longer work again, but at least it won't introduce any regressions. We need to revisit this for 2016.07. Do you have any ideas how to deal with this ? I am tempted to just add udelay(125); after each control transfer.
Some devices, like the SanDisk Cruzer Pop need some time to process the Set Configuration request, so wait a little until they are ready.
Signed-off-by: Marek Vasut marex@denx.de Cc: Chin Liang See clsee@altera.com Cc: Dinh Nguyen dinguyen@opensource.altera.com Cc: Hans de Goede hdegoede@redhat.com Cc: Stefan Roese sr@denx.de Cc: Stephen Warren swarren@nvidia.com --- common/usb.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/common/usb.c b/common/usb.c index 63429d4..205041b 100644 --- a/common/usb.c +++ b/common/usb.c @@ -1107,6 +1107,14 @@ int usb_select_config(struct usb_device *dev) "len %d, status %lX\n", dev->act_len, dev->status); return err; } + + /* + * Wait until the Set Configuration request gets processed by the + * device. This is required by at least SanDisk Cruzer Pop USB 2.0 + * and Kingston DT Ultimate 32GB USB 3.0 on DWC2 OTG controller. + */ + mdelay(10); + debug("new device strings: Mfr=%d, Product=%d, SerialNumber=%d\n", dev->descriptor.iManufacturer, dev->descriptor.iProduct, dev->descriptor.iSerialNumber);
On 03.05.2016 22:51, Marek Vasut wrote:
Some devices, like the SanDisk Cruzer Pop need some time to process the Set Configuration request, so wait a little until they are ready.
Signed-off-by: Marek Vasut marex@denx.de Cc: Chin Liang See clsee@altera.com Cc: Dinh Nguyen dinguyen@opensource.altera.com Cc: Hans de Goede hdegoede@redhat.com Cc: Stefan Roese sr@denx.de Cc: Stephen Warren swarren@nvidia.com
common/usb.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/common/usb.c b/common/usb.c index 63429d4..205041b 100644 --- a/common/usb.c +++ b/common/usb.c @@ -1107,6 +1107,14 @@ int usb_select_config(struct usb_device *dev) "len %d, status %lX\n", dev->act_len, dev->status); return err; }
- /*
* Wait until the Set Configuration request gets processed by the* device. This is required by at least SanDisk Cruzer Pop USB 2.0* and Kingston DT Ultimate 32GB USB 3.0 on DWC2 OTG controller.*/- mdelay(10);
- debug("new device strings: Mfr=%d, Product=%d, SerialNumber=%d\n", dev->descriptor.iManufacturer, dev->descriptor.iProduct, dev->descriptor.iSerialNumber);
As you might have expected, I'm not a fan of adding new delays to the common USB code. As this negatively affects all platforms. Did you test these two sticks that require this delay on other platforms than SoCFPGA? I would be very interested to know, if these keys are successfully detected without this patch on other platforms. I don't have access to these USB keys, so I can't test it on my platforms.
Thanks, Stefan
On Wed, 2016-05-04 at 09:41 +0200, Stefan Roese wrote:
On 03.05.2016 22:51, Marek Vasut wrote:
Some devices, like the SanDisk Cruzer Pop need some time to process the Set Configuration request, so wait a little until they are ready.
Signed-off-by: Marek Vasut marex@denx.de Cc: Chin Liang See clsee@altera.com Cc: Dinh Nguyen dinguyen@opensource.altera.com Cc: Hans de Goede hdegoede@redhat.com Cc: Stefan Roese sr@denx.de Cc: Stephen Warren swarren@nvidia.com
common/usb.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/common/usb.c b/common/usb.c index 63429d4..205041b 100644 --- a/common/usb.c +++ b/common/usb.c @@ -1107,6 +1107,14 @@ int usb_select_config(struct usb_device *dev) "len %d, status %lX\n", dev->act_len, dev ->status); return err; }
- /*
* Wait until the Set Configuration request gets processedby the
* device. This is required by at least SanDisk Cruzer PopUSB 2.0
* and Kingston DT Ultimate 32GB USB 3.0 on DWC2 OTGcontroller.
*/- mdelay(10);
- debug("new device strings: Mfr=%d, Product=%d,
SerialNumber=%d\n", dev->descriptor.iManufacturer, dev ->descriptor.iProduct, dev->descriptor.iSerialNumber);
As you might have expected, I'm not a fan of adding new delays to the common USB code. As this negatively affects all platforms. Did you test these two sticks that require this delay on other platforms than SoCFPGA? I would be very interested to know, if these keys are successfully detected without this patch on other platforms. I don't have access to these USB keys, so I can't test it on my platforms.
Actually this series of patches (include the delay) help for all my problematic pen drives too. It sound to me these pen drives need time to process. But at same time, its strange to me that the device didn't NAK (as I added printout for NAK) to indicate that its busy.
Seems that this issue is noticed at Freescale too. http://lists.denx.de/pipermail/u-boot/2015-December/238434.html. Cc'ed them as they might can share more details on this.
Chin Liang
Thanks, Stefan
On 04.05.2016 11:45, Chin Liang See wrote:
On Wed, 2016-05-04 at 09:41 +0200, Stefan Roese wrote:
On 03.05.2016 22:51, Marek Vasut wrote:
Some devices, like the SanDisk Cruzer Pop need some time to process the Set Configuration request, so wait a little until they are ready.
Signed-off-by: Marek Vasut marex@denx.de Cc: Chin Liang See clsee@altera.com Cc: Dinh Nguyen dinguyen@opensource.altera.com Cc: Hans de Goede hdegoede@redhat.com Cc: Stefan Roese sr@denx.de Cc: Stephen Warren swarren@nvidia.com
common/usb.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/common/usb.c b/common/usb.c index 63429d4..205041b 100644 --- a/common/usb.c +++ b/common/usb.c @@ -1107,6 +1107,14 @@ int usb_select_config(struct usb_device *dev) "len %d, status %lX\n", dev->act_len, dev ->status); return err; }
- /*
* Wait until the Set Configuration request gets processedby the
* device. This is required by at least SanDisk Cruzer PopUSB 2.0
* and Kingston DT Ultimate 32GB USB 3.0 on DWC2 OTGcontroller.
*/- mdelay(10);
- debug("new device strings: Mfr=%d, Product=%d,
SerialNumber=%d\n", dev->descriptor.iManufacturer, dev ->descriptor.iProduct, dev->descriptor.iSerialNumber);
As you might have expected, I'm not a fan of adding new delays to the common USB code. As this negatively affects all platforms. Did you test these two sticks that require this delay on other platforms than SoCFPGA? I would be very interested to know, if these keys are successfully detected without this patch on other platforms. I don't have access to these USB keys, so I can't test it on my platforms.
Actually this series of patches (include the delay) help for all my problematic pen drives too. It sound to me these pen drives need time to process.
This delay (I'm talking mainly about the 1000ms delay per USB hub in patch 7/7) does not seem to be necessary on other platforms.
Chin, could you please test again without this patch 7/7 but with "usb_pgood_delay" set to 1000? And let us know if this also fixes all the problems with your problematic pen drives? That would be very helpful.
Thanks, Stefan
On Wed, 2016-05-04 at 12:00 +0200, Stefan Roese wrote:
On 04.05.2016 11:45, Chin Liang See wrote:
On Wed, 2016-05-04 at 09:41 +0200, Stefan Roese wrote:
On 03.05.2016 22:51, Marek Vasut wrote:
Some devices, like the SanDisk Cruzer Pop need some time to process the Set Configuration request, so wait a little until they are ready.
Signed-off-by: Marek Vasut marex@denx.de Cc: Chin Liang See clsee@altera.com Cc: Dinh Nguyen dinguyen@opensource.altera.com Cc: Hans de Goede hdegoede@redhat.com Cc: Stefan Roese sr@denx.de Cc: Stephen Warren swarren@nvidia.com
common/usb.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/common/usb.c b/common/usb.c index 63429d4..205041b 100644 --- a/common/usb.c +++ b/common/usb.c @@ -1107,6 +1107,14 @@ int usb_select_config(struct usb_device *dev) "len %d, status %lX\n", dev ->act_len, dev ->status); return err; }
- /*
* Wait until the Set Configuration request getsprocessed by the
* device. This is required by at least SanDisk CruzerPop USB 2.0
* and Kingston DT Ultimate 32GB USB 3.0 on DWC2 OTGcontroller.
*/- mdelay(10);
- debug("new device strings: Mfr=%d, Product=%d,
SerialNumber=%d\n", dev->descriptor.iManufacturer, dev ->descriptor.iProduct, dev->descriptor.iSerialNumber);
As you might have expected, I'm not a fan of adding new delays to the common USB code. As this negatively affects all platforms. Did you test these two sticks that require this delay on other platforms than SoCFPGA? I would be very interested to know, if these keys are successfully detected without this patch on other platforms. I don't have access to these USB keys, so I can't test it on my platforms.
Actually this series of patches (include the delay) help for all my problematic pen drives too. It sound to me these pen drives need time to process.
This delay (I'm talking mainly about the 1000ms delay per USB hub in patch 7/7) does not seem to be necessary on other platforms.
Chin, could you please test again without this patch 7/7 but with "usb_pgood_delay" set to 1000? And let us know if this also fixes all the problems with your problematic pen drives? That would be very helpful.
Yup, it works for my problematic pen drives. This apply to both with and without setting the usb_pgood_delay to 1000.
Thanks Chin Liang
Thanks, Stefan
-----Original Message----- From: Chin Liang See [mailto:clsee@altera.com] Sent: Wednesday, May 04, 2016 3:15 PM To: Stefan Roese sr@denx.de; Marek Vasut marex@denx.de; u- boot@lists.denx.de Cc: Dinh Nguyen dinguyen@opensource.altera.com; Hans de Goede hdegoede@redhat.com; Stephen Warren swarren@nvidia.com; sriram.dash@freescale.com; rajesh.bhagat@freescale.com Subject: Re: [PATCH 4/7] usb: Wait after sending Set Configuration request
On Wed, 2016-05-04 at 09:41 +0200, Stefan Roese wrote:
On 03.05.2016 22:51, Marek Vasut wrote:
Some devices, like the SanDisk Cruzer Pop need some time to process the Set Configuration request, so wait a little until they are ready.
Signed-off-by: Marek Vasut marex@denx.de Cc: Chin Liang See clsee@altera.com Cc: Dinh Nguyen dinguyen@opensource.altera.com Cc: Hans de Goede hdegoede@redhat.com Cc: Stefan Roese sr@denx.de Cc: Stephen Warren swarren@nvidia.com
common/usb.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/common/usb.c b/common/usb.c index 63429d4..205041b 100644 --- a/common/usb.c +++ b/common/usb.c @@ -1107,6 +1107,14 @@ int usb_select_config(struct usb_device *dev) "len %d, status %lX\n", dev->act_len, dev ->status); return err; }
- /*
* Wait until the Set Configuration request gets processedby the
* device. This is required by at least SanDisk Cruzer PopUSB 2.0
* and Kingston DT Ultimate 32GB USB 3.0 on DWC2 OTGcontroller.
*/- mdelay(10);
- debug("new device strings: Mfr=%d, Product=%d,
SerialNumber=%d\n", dev->descriptor.iManufacturer, dev ->descriptor.iProduct, dev->descriptor.iSerialNumber);
As you might have expected, I'm not a fan of adding new delays to the common USB code. As this negatively affects all platforms. Did you test these two sticks that require this delay on other platforms than SoCFPGA? I would be very interested to know, if these keys are successfully detected without this patch on other platforms. I don't have access to these USB keys, so I can't test it on my platforms.
Actually this series of patches (include the delay) help for all my problematic pen drives too. It sound to me these pen drives need time to process. But at same time, its strange to me that the device didn't NAK (as I added printout for NAK) to indicate that its busy.
Seems that this issue is noticed at Freescale too. http://lists.denx.de/pipermail/u-boot/2015-December/238434.html. Cc'ed them as they might can share more details on this.
Hello,
In our case, for some USB 2.0 stick(notably : Sandisk Cruzer blade 4 GB) on USB 3.0 controller, we were getting timeouts in get descriptor string, and were not able to retrieve the manufacturer, product id , serial number.
Later we discovered, they needed a little more time (~ 2 ms) after setting configuration. However, as per the usb set address delay, we choose 10 ms delay(to support even slower devices).
Sriram
Chin Liang
Thanks, Stefan
On 05/04/2016 09:41 AM, Stefan Roese wrote:
On 03.05.2016 22:51, Marek Vasut wrote:
Some devices, like the SanDisk Cruzer Pop need some time to process the Set Configuration request, so wait a little until they are ready.
Signed-off-by: Marek Vasut marex@denx.de Cc: Chin Liang See clsee@altera.com Cc: Dinh Nguyen dinguyen@opensource.altera.com Cc: Hans de Goede hdegoede@redhat.com Cc: Stefan Roese sr@denx.de Cc: Stephen Warren swarren@nvidia.com
common/usb.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/common/usb.c b/common/usb.c index 63429d4..205041b 100644 --- a/common/usb.c +++ b/common/usb.c @@ -1107,6 +1107,14 @@ int usb_select_config(struct usb_device *dev) "len %d, status %lX\n", dev->act_len, dev->status); return err; }
- /*
* Wait until the Set Configuration request gets processed by the* device. This is required by at least SanDisk Cruzer Pop USB 2.0* and Kingston DT Ultimate 32GB USB 3.0 on DWC2 OTG controller.*/- mdelay(10);
debug("new device strings: Mfr=%d, Product=%d, SerialNumber=%d\n", dev->descriptor.iManufacturer, dev->descriptor.iProduct, dev->descriptor.iSerialNumber);As you might have expected, I'm not a fan of adding new delays to the common USB code. As this negatively affects all platforms. Did you test these two sticks that require this delay on other platforms than SoCFPGA?
Yes, on intel. Many microframes pass between these control EP requests, while on the dwc2 in u-boot, quite a few of these requests end in the same microframe because the code is fast, which is what the stick does not like.
I would be very interested to know, if these keys are successfully detected without this patch on other platforms. I don't have access to these USB keys, so I can't test it on my platforms.
Thanks, Stefan
The Kingston DT Ultimate USB 3.0 stick is sensitive to this first Get Descriptor request and if the request is not in a separate microframe, the stick refuses to operate. Add slight delay, which is enough for one microframe to pass on any USB spec revision.
Signed-off-by: Marek Vasut marex@denx.de Cc: Chin Liang See clsee@altera.com Cc: Dinh Nguyen dinguyen@opensource.altera.com Cc: Hans de Goede hdegoede@redhat.com Cc: Stefan Roese sr@denx.de Cc: Stephen Warren swarren@nvidia.com --- common/usb.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/common/usb.c b/common/usb.c index 205041b..8d9efe5 100644 --- a/common/usb.c +++ b/common/usb.c @@ -1077,6 +1077,14 @@ int usb_select_config(struct usb_device *dev) le16_to_cpus(&dev->descriptor.idProduct); le16_to_cpus(&dev->descriptor.bcdDevice);
+ /* + * Kingston DT Ultimate 32GB USB 3.0 seems to be extremely sensitive + * about this first Get Descriptor request. If there are any other + * requests in the first microframe, the stick crashes. Wait about + * one microframe duration here (1mS for USB 1.x , 125uS for USB 2.0). + */ + mdelay(1); + /* only support for one config for now */ err = usb_get_configuration_len(dev, 0); if (err >= 0) {
On 03.05.2016 22:51, Marek Vasut wrote:
The Kingston DT Ultimate USB 3.0 stick is sensitive to this first Get Descriptor request and if the request is not in a separate microframe, the stick refuses to operate. Add slight delay, which is enough for one microframe to pass on any USB spec revision.
Signed-off-by: Marek Vasut marex@denx.de Cc: Chin Liang See clsee@altera.com Cc: Dinh Nguyen dinguyen@opensource.altera.com Cc: Hans de Goede hdegoede@redhat.com Cc: Stefan Roese sr@denx.de Cc: Stephen Warren swarren@nvidia.com
common/usb.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/common/usb.c b/common/usb.c index 205041b..8d9efe5 100644 --- a/common/usb.c +++ b/common/usb.c @@ -1077,6 +1077,14 @@ int usb_select_config(struct usb_device *dev) le16_to_cpus(&dev->descriptor.idProduct); le16_to_cpus(&dev->descriptor.bcdDevice);
- /*
* Kingston DT Ultimate 32GB USB 3.0 seems to be extremely sensitive* about this first Get Descriptor request. If there are any other* requests in the first microframe, the stick crashes. Wait about* one microframe duration here (1mS for USB 1.x , 125uS for USB 2.0).*/- mdelay(1);
- /* only support for one config for now */ err = usb_get_configuration_len(dev, 0); if (err >= 0) {
Again my question, if this problem also occurs on other platforms with this USB key. A 1ms delay is not really a big deal, but its my general feeling that we should manifest such changes by testing on different platforms.
Thanks, Stefan
On 05/04/2016 10:03 AM, Stefan Roese wrote:
On 03.05.2016 22:51, Marek Vasut wrote:
The Kingston DT Ultimate USB 3.0 stick is sensitive to this first Get Descriptor request and if the request is not in a separate microframe, the stick refuses to operate. Add slight delay, which is enough for one microframe to pass on any USB spec revision.
Signed-off-by: Marek Vasut marex@denx.de Cc: Chin Liang See clsee@altera.com Cc: Dinh Nguyen dinguyen@opensource.altera.com Cc: Hans de Goede hdegoede@redhat.com Cc: Stefan Roese sr@denx.de Cc: Stephen Warren swarren@nvidia.com
common/usb.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/common/usb.c b/common/usb.c index 205041b..8d9efe5 100644 --- a/common/usb.c +++ b/common/usb.c @@ -1077,6 +1077,14 @@ int usb_select_config(struct usb_device *dev) le16_to_cpus(&dev->descriptor.idProduct); le16_to_cpus(&dev->descriptor.bcdDevice);
- /*
* Kingston DT Ultimate 32GB USB 3.0 seems to be extremely sensitive* about this first Get Descriptor request. If there are any other* requests in the first microframe, the stick crashes. Wait about* one microframe duration here (1mS for USB 1.x , 125uS for USB2.0).
*/- mdelay(1);
/* only support for one config for now */ err = usb_get_configuration_len(dev, 0); if (err >= 0) {Again my question, if this problem also occurs on other platforms with this USB key. A 1ms delay is not really a big deal, but its my general feeling that we should manifest such changes by testing on different platforms.
I tested it by connecting the bus analyzer between the stick and socfpga and between the stick and x86 host. I wouldn't be able to come up with this solution. btw. any ehci ends up inserting these control requests into separate microframes, but we need the mdelay to also cater for ohci/uhci, which has longer frames (1ms).
On 05/03/2016 02:51 PM, Marek Vasut wrote:
The Kingston DT Ultimate USB 3.0 stick is sensitive to this first Get Descriptor request and if the request is not in a separate microframe, the stick refuses to operate. Add slight delay, which is enough for one microframe to pass on any USB spec revision.
diff --git a/common/usb.c b/common/usb.c
- /*
* Kingston DT Ultimate 32GB USB 3.0 seems to be extremely sensitive* about this first Get Descriptor request. If there are any other* requests in the first microframe, the stick crashes. Wait about* one microframe duration here (1mS for USB 1.x , 125uS for USB 2.0).*/- mdelay(1);
Do we know the connection speed here? If so, we could sleep 1ms for USB1.x and 125us for USB2.x, thus reducing any performance impact. Still, this is a short delay that I think only happens once per actual device so perhaps it isn't worth it.
The code shouldn't continue probing the port if get_port_status() failed.
Signed-off-by: Marek Vasut marex@denx.de Cc: Chin Liang See clsee@altera.com Cc: Dinh Nguyen dinguyen@opensource.altera.com Cc: Hans de Goede hdegoede@redhat.com Cc: Stefan Roese sr@denx.de Cc: Stephen Warren swarren@nvidia.com --- common/usb_hub.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/common/usb_hub.c b/common/usb_hub.c index 4f59802..0f39c9f 100644 --- a/common/usb_hub.c +++ b/common/usb_hub.c @@ -402,6 +402,7 @@ static int usb_scan_port(struct usb_device_scan *usb_scan) free(usb_scan); return 0; } + return 0; }
portstatus = le16_to_cpu(portsts->wPortStatus);
On 03.05.2016 22:51, Marek Vasut wrote:
The code shouldn't continue probing the port if get_port_status() failed.
Signed-off-by: Marek Vasut marex@denx.de Cc: Chin Liang See clsee@altera.com Cc: Dinh Nguyen dinguyen@opensource.altera.com Cc: Hans de Goede hdegoede@redhat.com Cc: Stefan Roese sr@denx.de Cc: Stephen Warren swarren@nvidia.com
common/usb_hub.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/common/usb_hub.c b/common/usb_hub.c index 4f59802..0f39c9f 100644 --- a/common/usb_hub.c +++ b/common/usb_hub.c @@ -402,6 +402,7 @@ static int usb_scan_port(struct usb_device_scan *usb_scan) free(usb_scan); return 0; }
return 0;}
portstatus = le16_to_cpu(portsts->wPortStatus);
Thanks for spotting this:
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
On Wed, 2016-05-04 at 10:05 +0200, Stefan Roese wrote:
On 03.05.2016 22:51, Marek Vasut wrote:
The code shouldn't continue probing the port if get_port_status() failed.
Signed-off-by: Marek Vasut marex@denx.de Cc: Chin Liang See clsee@altera.com Cc: Dinh Nguyen dinguyen@opensource.altera.com Cc: Hans de Goede hdegoede@redhat.com Cc: Stefan Roese sr@denx.de Cc: Stephen Warren swarren@nvidia.com
common/usb_hub.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/common/usb_hub.c b/common/usb_hub.c index 4f59802..0f39c9f 100644 --- a/common/usb_hub.c +++ b/common/usb_hub.c @@ -402,6 +402,7 @@ static int usb_scan_port(struct usb_device_scan *usb_scan) free(usb_scan); return 0; }
return 0;}
portstatus = le16_to_cpu(portsts->wPortStatus);
Thanks for spotting this:
Reviewed-by: Stefan Roese sr@denx.de
Reviewed-by: Chin Liang See clsee@altera.com Tested-by: Chin Liang See clsee@altera.com
Thanks Chin Liang
Thanks, Stefan
Increase the query delay, otherwise some sticks are not detected. The problem shows up on the USB bus analyzer such that the stick takes longer time to switch from FS mode to HS mode than the code allows.
Signed-off-by: Marek Vasut marex@denx.de Cc: Chin Liang See clsee@altera.com Cc: Dinh Nguyen dinguyen@opensource.altera.com Cc: Hans de Goede hdegoede@redhat.com Cc: Stefan Roese sr@denx.de Cc: Stephen Warren swarren@nvidia.com --- common/usb_hub.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/usb_hub.c b/common/usb_hub.c index 0f39c9f..6cd274a 100644 --- a/common/usb_hub.c +++ b/common/usb_hub.c @@ -145,7 +145,7 @@ static void usb_hub_power_on(struct usb_hub_device *hub) * Do a minimum delay of the larger value of 100ms or pgood_delay * so that the power can stablize before the devices are queried */ - hub->query_delay = get_timer(0) + max(100, (int)pgood_delay); + hub->query_delay = get_timer(0) + max(1000, (int)pgood_delay);
/* * Record the power-on timeout here. The max. delay (timeout)
On 03.05.2016 22:51, Marek Vasut wrote:
Increase the query delay, otherwise some sticks are not detected. The problem shows up on the USB bus analyzer such that the stick takes longer time to switch from FS mode to HS mode than the code allows.
Signed-off-by: Marek Vasut marex@denx.de Cc: Chin Liang See clsee@altera.com Cc: Dinh Nguyen dinguyen@opensource.altera.com Cc: Hans de Goede hdegoede@redhat.com Cc: Stefan Roese sr@denx.de Cc: Stephen Warren swarren@nvidia.com
common/usb_hub.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/usb_hub.c b/common/usb_hub.c index 0f39c9f..6cd274a 100644 --- a/common/usb_hub.c +++ b/common/usb_hub.c @@ -145,7 +145,7 @@ static void usb_hub_power_on(struct usb_hub_device *hub) * Do a minimum delay of the larger value of 100ms or pgood_delay * so that the power can stablize before the devices are queried */
- hub->query_delay = get_timer(0) + max(100, (int)pgood_delay);
hub->query_delay = get_timer(0) + max(1000, (int)pgood_delay);
/*
- Record the power-on timeout here. The max. delay (timeout)
We have touched this part of the delay a number of times in my USB scanning patch series. I've integrated all very valuable suggestions from Stephen and Hans and I'm pretty sure that the current implementation is aligned with the USB spec. And tested successfully one multiple platforms without regression. Stephen did some tests on Tegra and Hans on sunxi. I tested mainly on x86. But I now also tested on Armada XP (see below).
As mentioned before, the current version causes no problems with all my USB sticks on the congatec x86 board. Even the 2 ones that are problematic when connected to the SoCFPGA. They are detected without any issues on this board. Thats why I assume, that the real problem here is the DWC driver and not the generic USB handling code. My feeling is that increasing this initial delay (before the scanning starts) just papers over the real problem most likely hidden somewhere in the DWC driver. This is just a feeling though which I can't prove somehow other than testing the common USB code on different platforms. And I really have no deeper knowledge of the DWC driver to manifest this feeling or even fix this potential problem there.
I've already mentioned this in another mail. My suggestion for SoCFPGA boards that need to use these problematic USB keys is to use the already available solution to set "usb_pgood_delay" to 1000. This effectively does the same as this patch. Without implying this general 1 second delay per hub (!!!) to all other platforms that use USB in U-Boot.
To test my suspicions about this being a DWC (SoCFPGA?) only problem, I've also tested all my current USB sticks including the 2 problematic ones (on SoCrates) on another ARM platform (additionally to all my test on x86). I've used the Marvell Armada XP development board (db-mv784mp-gp) for this. And all USB sticks are detected without any problems on this platform.
As a result of all this, I would like to have this patch not applied. As it negatively touches the common USB code to fix (paper over?) a problem only seen on one platform (AFAIK). And we already have the solution of this "usb_pgood_delay" that can be used on SoCFPGA. To manifest this, here again the numbers for the USB scanning time on x86, without and with this patch:
Without this patch: starting USB... USB0: USB EHCI 1.00 scanning bus 0 for devices... 9 USB Device(s) found
time: 1.940 seconds
With this patch: => time usb start starting USB... USB0: USB EHCI 1.00 scanning bus 0 for devices... 9 USB Device(s) found
time: 5.421 seconds
Thanks, Stefan
On 05/04/2016 10:07 AM, Stefan Roese wrote:
On 03.05.2016 22:51, Marek Vasut wrote:
Increase the query delay, otherwise some sticks are not detected. The problem shows up on the USB bus analyzer such that the stick takes longer time to switch from FS mode to HS mode than the code allows.
Signed-off-by: Marek Vasut marex@denx.de Cc: Chin Liang See clsee@altera.com Cc: Dinh Nguyen dinguyen@opensource.altera.com Cc: Hans de Goede hdegoede@redhat.com Cc: Stefan Roese sr@denx.de Cc: Stephen Warren swarren@nvidia.com
common/usb_hub.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/usb_hub.c b/common/usb_hub.c index 0f39c9f..6cd274a 100644 --- a/common/usb_hub.c +++ b/common/usb_hub.c @@ -145,7 +145,7 @@ static void usb_hub_power_on(struct usb_hub_device *hub) * Do a minimum delay of the larger value of 100ms or pgood_delay * so that the power can stablize before the devices are queried */
- hub->query_delay = get_timer(0) + max(100, (int)pgood_delay);
hub->query_delay = get_timer(0) + max(1000, (int)pgood_delay);
/*
- Record the power-on timeout here. The max. delay (timeout)
We have touched this part of the delay a number of times in my USB scanning patch series. I've integrated all very valuable suggestions from Stephen and Hans and I'm pretty sure that the current implementation is aligned with the USB spec.
Sadly, not everyone goes exactly be the USB spec it seems. Especially the cheap sticks which are the majority in the market.
And tested successfully one multiple platforms without regression. Stephen did some tests on Tegra and Hans on sunxi. I tested mainly on x86. But I now also tested on Armada XP (see below).
All of which use the same EHCI or XHCI controller, correct ?
As mentioned before, the current version causes no problems with all my USB sticks on the congatec x86 board. Even the 2 ones that are problematic when connected to the SoCFPGA. They are detected without any issues on this board. Thats why I assume, that the real problem here is the DWC driver and not the generic USB handling code.
The logic here is flawed. If the code works against EHCI controller and does not work against DWC2 controller, you cannot infer from this that the DWC2 controller is the problem.
This can also be specific behavior of the EHCI controller, and I in fact suspect it is, but you cannot decide this without checking the bus itself.
My feeling is that increasing this initial delay (before the scanning starts) just papers over the real problem most likely hidden somewhere in the DWC driver. This is just a feeling though which I can't prove somehow other than testing the common USB code on different platforms. And I really have no deeper knowledge of the DWC driver to manifest this feeling or even fix this potential problem there.
The bus analyzer tells me that the stick just takes longer to come out of FS mode and switch to HS mode when the USB started from reset state of the controller, I decide to trust what the analyzer shows me instead.
See attached logs.
I've already mentioned this in another mail. My suggestion for SoCFPGA boards that need to use these problematic USB keys is to use the already available solution to set "usb_pgood_delay" to 1000. This effectively does the same as this patch. Without implying this general 1 second delay per hub (!!!) to all other platforms that use USB in U-Boot.
To test my suspicions about this being a DWC (SoCFPGA?) only problem, I've also tested all my current USB sticks including the 2 problematic ones (on SoCrates) on another ARM platform (additionally to all my test on x86). I've used the Marvell Armada XP development board (db-mv784mp-gp) for this. And all USB sticks are detected without any problems on this platform.
I see, it would be interesting to know what happens on the bus on the marvell board compared to the socfpga board. For the socfpga, the bus starts from complete cold state, could it be the marvell does have the EHCI running when you do your tests ? That would explain why the stick might be ready much faster on your platform.
As a result of all this, I would like to have this patch not applied. As it negatively touches the common USB code to fix (paper over?) a problem only seen on one platform (AFAIK). And we already have the solution of this "usb_pgood_delay" that can be used on SoCFPGA. To manifest this, here again the numbers for the USB scanning time on x86, without and with this patch:
Without this patch: starting USB... USB0: USB EHCI 1.00 scanning bus 0 for devices... 9 USB Device(s) found
time: 1.940 seconds
With this patch: => time usb start starting USB... USB0: USB EHCI 1.00 scanning bus 0 for devices... 9 USB Device(s) found
time: 5.421 seconds
Well that's great, removing some delays makes the code run faster and breaks some platform. Stefan, I need a solution for this release and the release is coming very quickly. So far, I see no other proposals how to fix this issue and it's been reported for well over a month.
As I don't want to have regressions in this release, here are the two options: a) I revert "usb: Change power-on / scanning timeout handling" b) I apply these 7 patches. I am willing to isolate this increase of delay with an ifdef to DWC2 controller, but that does NOT seem correct according to the analyzer. I would just wait for an EHCI controller on which this breaks.
Thanks, Stefan
On 04.05.2016 13:39, Marek Vasut wrote:
On 05/04/2016 10:07 AM, Stefan Roese wrote:
On 03.05.2016 22:51, Marek Vasut wrote:
Increase the query delay, otherwise some sticks are not detected. The problem shows up on the USB bus analyzer such that the stick takes longer time to switch from FS mode to HS mode than the code allows.
Signed-off-by: Marek Vasut marex@denx.de Cc: Chin Liang See clsee@altera.com Cc: Dinh Nguyen dinguyen@opensource.altera.com Cc: Hans de Goede hdegoede@redhat.com Cc: Stefan Roese sr@denx.de Cc: Stephen Warren swarren@nvidia.com
common/usb_hub.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/usb_hub.c b/common/usb_hub.c index 0f39c9f..6cd274a 100644 --- a/common/usb_hub.c +++ b/common/usb_hub.c @@ -145,7 +145,7 @@ static void usb_hub_power_on(struct usb_hub_device *hub) * Do a minimum delay of the larger value of 100ms or pgood_delay * so that the power can stablize before the devices are queried */
- hub->query_delay = get_timer(0) + max(100, (int)pgood_delay);
hub->query_delay = get_timer(0) + max(1000, (int)pgood_delay);
/*
- Record the power-on timeout here. The max. delay (timeout)
We have touched this part of the delay a number of times in my USB scanning patch series. I've integrated all very valuable suggestions from Stephen and Hans and I'm pretty sure that the current implementation is aligned with the USB spec.
Sadly, not everyone goes exactly be the USB spec it seems. Especially the cheap sticks which are the majority in the market.
And tested successfully one multiple platforms without regression. Stephen did some tests on Tegra and Hans on sunxi. I tested mainly on x86. But I now also tested on Armada XP (see below).
All of which use the same EHCI or XHCI controller, correct ?
As mentioned before, the current version causes no problems with all my USB sticks on the congatec x86 board. Even the 2 ones that are problematic when connected to the SoCFPGA. They are detected without any issues on this board. Thats why I assume, that the real problem here is the DWC driver and not the generic USB handling code.
The logic here is flawed. If the code works against EHCI controller and does not work against DWC2 controller, you cannot infer from this that the DWC2 controller is the problem.
This can also be specific behavior of the EHCI controller, and I in fact suspect it is, but you cannot decide this without checking the bus itself.
My feeling is that increasing this initial delay (before the scanning starts) just papers over the real problem most likely hidden somewhere in the DWC driver. This is just a feeling though which I can't prove somehow other than testing the common USB code on different platforms. And I really have no deeper knowledge of the DWC driver to manifest this feeling or even fix this potential problem there.
The bus analyzer tells me that the stick just takes longer to come out of FS mode and switch to HS mode when the USB started from reset state of the controller, I decide to trust what the analyzer shows me instead.
See attached logs.
I've already mentioned this in another mail. My suggestion for SoCFPGA boards that need to use these problematic USB keys is to use the already available solution to set "usb_pgood_delay" to 1000. This effectively does the same as this patch. Without implying this general 1 second delay per hub (!!!) to all other platforms that use USB in U-Boot.
To test my suspicions about this being a DWC (SoCFPGA?) only problem, I've also tested all my current USB sticks including the 2 problematic ones (on SoCrates) on another ARM platform (additionally to all my test on x86). I've used the Marvell Armada XP development board (db-mv784mp-gp) for this. And all USB sticks are detected without any problems on this platform.
I see, it would be interesting to know what happens on the bus on the marvell board compared to the socfpga board. For the socfpga, the bus starts from complete cold state, could it be the marvell does have the EHCI running when you do your tests ? That would explain why the stick might be ready much faster on your platform.
As a result of all this, I would like to have this patch not applied. As it negatively touches the common USB code to fix (paper over?) a problem only seen on one platform (AFAIK). And we already have the solution of this "usb_pgood_delay" that can be used on SoCFPGA. To manifest this, here again the numbers for the USB scanning time on x86, without and with this patch:
Without this patch: starting USB... USB0: USB EHCI 1.00 scanning bus 0 for devices... 9 USB Device(s) found
time: 1.940 seconds
With this patch: => time usb start starting USB... USB0: USB EHCI 1.00 scanning bus 0 for devices... 9 USB Device(s) found
time: 5.421 seconds
Well that's great, removing some delays makes the code run faster and breaks some platform. Stefan, I need a solution for this release and the release is coming very quickly. So far, I see no other proposals how to fix this issue and it's been reported for well over a month.
As I don't want to have regressions in this release, here are the two options: a) I revert "usb: Change power-on / scanning timeout handling" b) I apply these 7 patches. I am willing to isolate this increase of delay with an ifdef to DWC2 controller, but that does NOT seem correct according to the analyzer. I would just wait for an EHCI controller on which this breaks.
You are missing c), use "usb_pgood_delay" here. But okay, please use option b) for this release. We can always re-visit this issue later and try to find a "better" way to fix this.
Thanks, Stefan
On 05/03/2016 02:51 PM, Marek Vasut wrote:
Increase the query delay, otherwise some sticks are not detected. The problem shows up on the USB bus analyzer such that the stick takes longer time to switch from FS mode to HS mode than the code allows.
diff --git a/common/usb_hub.c b/common/usb_hub.c
@@ -145,7 +145,7 @@ static void usb_hub_power_on(struct usb_hub_device *hub) * Do a minimum delay of the larger value of 100ms or pgood_delay * so that the power can stablize before the devices are queried */
- hub->query_delay = get_timer(0) + max(100, (int)pgood_delay);
- hub->query_delay = get_timer(0) + max(1000, (int)pgood_delay);
The comment right above that line of code should be updated to say 1000 not 100, and to mention why we're using the non-spec value.
Increase the query delay, otherwise some sticks are not detected. The problem shows up on the USB bus analyzer such that the stick takes longer time to switch from FS mode to HS mode than the code allows when the controller starts from completely off state.
Signed-off-by: Marek Vasut marex@denx.de Cc: Chin Liang See clsee@altera.com Cc: Dinh Nguyen dinguyen@opensource.altera.com Cc: Hans de Goede hdegoede@redhat.com Cc: Stefan Roese sr@denx.de Cc: Stephen Warren swarren@nvidia.com --- V2: Special-case this only for DWC2 --- common/usb_hub.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/common/usb_hub.c b/common/usb_hub.c index 0f39c9f..4795ea1 100644 --- a/common/usb_hub.c +++ b/common/usb_hub.c @@ -144,8 +144,14 @@ static void usb_hub_power_on(struct usb_hub_device *hub) /* * Do a minimum delay of the larger value of 100ms or pgood_delay * so that the power can stablize before the devices are queried + * DWC2 (and possibly others?) needs longer time to stabilize when + * coming out of cold start. 1 second seems to work reliably. */ +#ifdef CONFIG_USB_DWC2 + hub->query_delay = get_timer(0) + max(1000, (int)pgood_delay); +#else hub->query_delay = get_timer(0) + max(100, (int)pgood_delay); +#endif
/* * Record the power-on timeout here. The max. delay (timeout)
On 05/04/2016 03:32 PM, Marek Vasut wrote:
Increase the query delay, otherwise some sticks are not detected. The problem shows up on the USB bus analyzer such that the stick takes longer time to switch from FS mode to HS mode than the code allows when the controller starts from completely off state.
Acked-by: Stephen Warren swarren@nvidia.com
(I suspect this is more to do with the board than the USB controller, since the board controls the VBUS power. However, if this works, it's probably fine for now.)
On 05/04/2016 11:34 PM, Stephen Warren wrote:
On 05/04/2016 03:32 PM, Marek Vasut wrote:
Increase the query delay, otherwise some sticks are not detected. The problem shows up on the USB bus analyzer such that the stick takes longer time to switch from FS mode to HS mode than the code allows when the controller starts from completely off state.
Acked-by: Stephen Warren swarren@nvidia.com
(I suspect this is more to do with the board than the USB controller, since the board controls the VBUS power. However, if this works, it's probably fine for now.)
For now, maybe, but I am extremely unhappy to have this hack in the common code.
On 03.05.2016 22:51, Marek Vasut wrote:
The pointer should always be inited to NULL, not zero (0). These are two different things and not necessarily equal.
Signed-off-by: Marek Vasut marex@denx.de Cc: Chin Liang See clsee@altera.com Cc: Dinh Nguyen dinguyen@opensource.altera.com Cc: Hans de Goede hdegoede@redhat.com Cc: Stefan Roese sr@denx.de Cc: Stephen Warren swarren@nvidia.com
common/usb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/usb.c b/common/usb.c index 4d0de4d..63429d4 100644 --- a/common/usb.c +++ b/common/usb.c @@ -1064,7 +1064,7 @@ static int usb_prepare_device(struct usb_device *dev, int addr, bool do_read,
int usb_select_config(struct usb_device *dev) {
- unsigned char *tmpbuf = 0;
unsigned char *tmpbuf = NULL; int err;
err = get_descriptor_len(dev, USB_DT_DEVICE_SIZE, USB_DT_DEVICE_SIZE);
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
On Wed, 2016-05-04 at 09:36 +0200, Stefan Roese wrote:
On 03.05.2016 22:51, Marek Vasut wrote:
The pointer should always be inited to NULL, not zero (0). These are two different things and not necessarily equal.
Signed-off-by: Marek Vasut marex@denx.de Cc: Chin Liang See clsee@altera.com Cc: Dinh Nguyen dinguyen@opensource.altera.com Cc: Hans de Goede hdegoede@redhat.com Cc: Stefan Roese sr@denx.de Cc: Stephen Warren swarren@nvidia.com
common/usb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/usb.c b/common/usb.c index 4d0de4d..63429d4 100644 --- a/common/usb.c +++ b/common/usb.c @@ -1064,7 +1064,7 @@ static int usb_prepare_device(struct usb_device *dev, int addr, bool do_read,
int usb_select_config(struct usb_device *dev) {
- unsigned char *tmpbuf = 0;
unsigned char *tmpbuf = NULL; int err;
err = get_descriptor_len(dev, USB_DT_DEVICE_SIZE,
USB_DT_DEVICE_SIZE);
Reviewed-by: Stefan Roese sr@denx.de
Reviewed-by: Chin Liang See clsee@altera.com Tested-by: Chin Liang See clsee@altera.com
Thanks Chin Liang
Thanks, Stefan
On 05/03/2016 10:51 PM, Marek Vasut wrote:
The pointer should always be inited to NULL, not zero (0). These are two different things and not necessarily equal.
Signed-off-by: Marek Vasut marex@denx.de Cc: Chin Liang See clsee@altera.com Cc: Dinh Nguyen dinguyen@opensource.altera.com Cc: Hans de Goede hdegoede@redhat.com Cc: Stefan Roese sr@denx.de Cc: Stephen Warren swarren@nvidia.com
1..6 applied, 7 dropped in favor of usb: dwc2: Add delay to fix the USB detection problem on SoCFPGA
Hopefully there is no more breakage in this release.
Best regards, Marek Vasut
participants (5)
-
Chin Liang See -
Marek Vasut -
Sriram Dash -
Stefan Roese -
Stephen Warren