usb: Don't init pointer to zero, but NULL
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.
--- common/usb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
common/usb.c +1 -1
@@ -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 );
Signature · 1 line
2.7.0
PATCH 1/7 usb: Don't init pointer to zero, but NULL
Quoted 9 lines · Marek Vasut
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 <[email protected]>Cc Chin Liang See <[email protected]>Cc Dinh Nguyen <[email protected]>Cc Hans de Goede <[email protected]>Cc Stefan Roese <[email protected]>Cc Stephen Warren <[email protected]>
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
PATCH 2/7 usb: dwc2: Resend setup packet in all fail cases
In reply to Marek VasutThe 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.
--- drivers/usb/host/dwc2.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
drivers/usb/host/dwc2.c +14 -0
@@ -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 ; }
Signature · 1 line
2.7.0
PATCH 2/7 usb: dwc2: Resend setup packet in all fail cases
Quoted 15 lines · Marek Vasut
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 <[email protected]>Cc Chin Liang See <[email protected]>Cc Dinh Nguyen <[email protected]>Cc Hans de Goede <[email protected]>Cc Stefan Roese <[email protected]>Cc Stephen Warren <[email protected]>
Quoted 4 lines
--- drivers/usb/host/dwc2.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
Thanks
Chin Liang
PATCH 3/7 usb: dwc2: Throttle the setup packet resending
In reply to Marek VasutAbort 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.
--- drivers/usb/host/dwc2.c | 44 ++++++++++++++++++++++++++++++++------------ 1 file changed, 32 insertions(+), 12 deletions(-)
drivers/usb/host/dwc2.c +32 -12
@@ -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;
Signature · 1 line
2.7.0
PATCH 3/7 usb: dwc2: Throttle the setup packet resending
Quoted 11 lines · Marek Vasut
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 <[email protected]>Cc Chin Liang See <[email protected]>Cc Dinh Nguyen <[email protected]>Cc Hans de Goede <[email protected]>Cc Stefan Roese <[email protected]>Cc Stephen Warren <[email protected]>
Quoted 5 lines
--- drivers/usb/host/dwc2.c | 44 ++++++++++++++++++++++++++++++++------- ----- 1 file changed, 32 insertions(+), 12 deletions(-)
Thanks
Chin Liang
PATCH 3/7 usb: dwc2: Throttle the setup packet resending
In reply to Marek VasutQuoted 4 lines · Marek Vasut
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.
PATCH 3/7 usb: dwc2: Throttle the setup packet resending
Quoted 10 lines · Stephen Warren
Quoted 4 lines · Marek Vasut
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_EPis 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 ?
Quoted 4 lines
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.
Signature · 2 lines
Best regards, Marek Vasut
PATCH 3/7 usb: dwc2: Throttle the setup packet resending
In reply to Stephen WarrenQuoted 11 lines · Stephen Warren
Quoted 4 lines · Marek Vasut
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_EPis 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?
Quoted 4 lines
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.
Signature · 2 lines
Best regards, Marek Vasut
PATCH 4/7 usb: Wait after sending Set Configuration request
In reply to Marek VasutSome devices, like the SanDisk Cruzer Pop need some time to process the Set Configuration request, so wait a little until they are ready.
--- common/usb.c | 8 ++++++++ 1 file changed, 8 insertions(+)
common/usb.c +8 -0
@@ -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);
Signature · 1 line
2.7.0
PATCH 4/7 usb: Wait after sending Set Configuration request
Quoted 23 lines · Marek Vasut
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 <[email protected]>Cc Chin Liang See <[email protected]>Cc Dinh Nguyen <[email protected]>Cc Hans de Goede <[email protected]>Cc Stefan Roese <[email protected]>Cc Stephen Warren <[email protected]>--- common/usb.c | 8 ++++++++ 1 file changed, 8 insertions(+)common/usb.c +1
-0@@ -1107,6 +1107,14 @@ int usb_select_config(struct usb_device *dev) "len %d, status %lX\n", dev->act_len, dev->status);return err; } +
common/usb.c int usb_select_config(struct usb_device *dev) · line 1120
Quoted 10 lines
common/usb.c +7
-0+/* +* 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
PATCH 4/7 usb: Wait after sending Set Configuration request
Quoted 43 lines · Stefan Roese
Quoted 36 lines · Marek Vasut
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 <[email protected]>Cc Chin Liang See <[email protected]>Cc Dinh Nguyen <[email protected]>Cc Hans de Goede <[email protected]>Cc Stefan Roese <[email protected]>Cc Stephen Warren <[email protected]>--- common/usb.c | 8 ++++++++ 1 file changed, 8 insertions(+)common/usb.c +3
-1@@ -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
common/usb.c +1
-0+* device. This is required by at least SanDisk Cruzer Pop USB 2.0
common/usb.c +1
-0+* and Kingston DT Ultimate 32GB USB 3.0 on DWC2 OTG controller.
common/usb.c +3
-0+*/ +mdelay (10 ); + debug("new device strings: Mfr=%d, Product=%d,Quoted 5 lines
SerialNumber=%d\n",
dev->descriptor.iManufacturer, dev
->descriptor.iProduct,
dev->descriptor.iSerialNumber);
Quoted 8 lines
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
Quoted 2 lines
Thanks,
Stefan
PATCH 4/7 usb: Wait after sending Set Configuration request
Quoted 53 lines · Chin Liang See
Quoted 43 lines · Stefan Roese
Quoted 36 lines · Marek Vasut
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 <[email protected]>Cc Chin Liang See <[email protected]>Cc Dinh Nguyen <[email protected]>Cc Hans de Goede <[email protected]>Cc Stefan Roese <[email protected]>Cc Stephen Warren <[email protected]>--- common/usb.c | 8 ++++++++ 1 file changed, 8 insertions(+)common/usb.c +3
-1@@ -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
common/usb.c +1
-0+* device. This is required by at least SanDisk Cruzer Pop USB 2.0
common/usb.c +1
-0+* and Kingston DT Ultimate 32GB USB 3.0 on DWC2 OTG controller.
common/usb.c +3
-0+*/ +mdelay (10 ); + debug("new device strings: Mfr=%d, Product=%d,Quoted 5 lines
SerialNumber=%d\n",
dev->descriptor.iManufacturer, dev
->descriptor.iProduct,
dev->descriptor.iSerialNumber);Quoted 8 lines
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.
Quoted 3 lines
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
PATCH 4/7 usb: Wait after sending Set Configuration request
Quoted 66 lines · Stefan Roese
Quoted 60 lines · Chin Liang See
Quoted 49 lines · Stefan Roese
Quoted 40 lines · Marek Vasut
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 <[email protected]>Cc Chin Liang See <[email protected]>Cc Dinh Nguyen <[email protected]>Cc Hans de Goede <[email protected]>Cc Stefan Roese <[email protected]>Cc Stephen Warren <[email protected]>--- common/usb.c | 8 ++++++++ 1 file changed, 8 insertions(+)common/usb.c +3
-2@@ -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 thecommon/usb.c +1
-0+* device. This is required by at least SanDisk Cruzer Pop
USB 2.0common/usb.c +1
-0+* and Kingston DT Ultimate 32GB USB 3.0 on DWC2 OTG controller.
common/usb.c +3
-0+*/ +mdelay (10 ); + debug("new device strings: Mfr=%d, Product=%d,Quoted 5 lines
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.
Quoted 9 lines
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.Quoted 4 lines
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.
Quoted 8 lines
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
Quoted 2 lines
Thanks,
Stefan
PATCH 4/7 usb: Wait after sending Set Configuration request
In reply to Chin Liang SeeQuoted 62 lines
-----Original Message-----
From: Chin Liang See [mailto:clsee at altera.com]
Sent: Wednesday, May 04, 2016 3:15 PM
To: Stefan Roese <[email protected]>; Marek Vasut <[email protected]>; u- boot at lists.denx.de
Cc: Dinh Nguyen <[email protected]>; Hans de Goede <[email protected]>; Stephen Warren <[email protected]>; sriram.dash at freescale.com; rajesh.bhagat at freescale.com
Subject: Re: [PATCH 4/7] usb: Wait after sending Set Configuration requestQuoted 41 lines · Stefan Roese
Quoted 40 lines · Marek Vasut
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 <[email protected]>Cc Chin Liang See <[email protected]>Cc Dinh Nguyen <[email protected]>Cc Hans de Goede <[email protected]>Cc Stefan Roese <[email protected]>Cc Stephen Warren <[email protected]>--- common/usb.c | 8 ++++++++ 1 file changed, 8 insertions(+)common/usb.c index 63429d4..205041b renamed from common/usb.c +0
-0100644
common/usb.c +3
-1@@ -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
common/usb.c +1
-0+* device. This is required by at least SanDisk Cruzer Pop USB 2.0
common/usb.c +1
-0+* and Kingston DT Ultimate 32GB USB 3.0 on DWC2 OTG controller.
common/usb.c +3
-0+*/ +mdelay (10 ); + debug("new device strings: Mfr=%d, Product=%d,SerialNumber=%d\n",
dev->descriptor.iManufacturer, dev
->descriptor.iProduct,
dev->descriptor.iSerialNumber);Quoted 9 lines
Quoted 1 line
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.
Quoted 9 lines
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
Quoted 6 lines
Chin Liang
Quoted 2 lines
Thanks,
Stefan
PATCH 4/7 usb: Wait after sending Set Configuration request
In reply to Stefan RoeseQuoted 31 lines · Stefan Roese
Quoted 30 lines · Marek Vasut
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 <[email protected]>Cc Chin Liang See <[email protected]>Cc Dinh Nguyen <[email protected]>Cc Hans de Goede <[email protected]>Cc Stefan Roese <[email protected]>Cc Stephen Warren <[email protected]>--- common/usb.c | 8 ++++++++ 1 file changed, 8 insertions(+)common/usb.c +8
-0@@ -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",
Quoted 8 lines
common/usb.c int usb_select_config(struct usb_device *dev) · line 1120
Quoted 3 lines
common/usb.c +0
-0dev->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.
Quoted 7 lines
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
Signature · 2 lines
Best regards, Marek Vasut
PATCH 5/7 usb: Assure Get Descriptor request is in separate microframe
In reply to Marek VasutThe 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.
--- common/usb.c | 8 ++++++++ 1 file changed, 8 insertions(+)
common/usb.c +8 -0
@@ -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 ) {
Signature · 1 line
2.7.0
PATCH 5/7 usb: Assure Get Descriptor request is in separate microframe
Quoted 25 lines · Marek Vasut
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 <[email protected]>Cc Chin Liang See <[email protected]>Cc Dinh Nguyen <[email protected]>Cc Hans de Goede <[email protected]>Cc Stefan Roese <[email protected]>Cc Stephen Warren <[email protected]>--- common/usb.c | 8 ++++++++ 1 file changed, 8 insertions(+)common/usb.c +1
-0@@ -1077,6 +1077,14 @@ int usb_select_config(struct usb_device *dev)le16_to_cpus (&dev->descriptor.idProduct);le16_to_cpus (&dev->descriptor.bcdDevice); +/*
common/usb.c int usb_select_config(struct usb_device *dev) · line 1090
Quoted 10 lines
common/usb.c +7
-0+* 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
PATCH 5/7 usb: Assure Get Descriptor request is in separate microframe
Quoted 34 lines · Stefan Roese
Quoted 33 lines · Marek Vasut
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 <[email protected]>Cc Chin Liang See <[email protected]>Cc Dinh Nguyen <[email protected]>Cc Hans de Goede <[email protected]>Cc Stefan Roese <[email protected]>Cc Stephen Warren <[email protected]>--- common/usb.c | 8 ++++++++ 1 file changed, 8 insertions(+)common/usb.c +5
-0@@ -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).
common/usb.c +3
-0+*/ +mdelay (1 ); +/* only support for one config for now */
Quoted 8 lines
Quoted 3 lines
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).
Signature · 2 lines
Best regards, Marek Vasut
PATCH 5/7 usb: Assure Get Descriptor request is in separate microframe
In reply to Marek VasutQuoted 6 lines · Marek Vasut
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.
common/usb.c int usb_select_config(struct usb_device *dev) · line 1086
Quoted 8 lines
common/usb.c +7
-0+/* +* 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.
PATCH 6/7 usb: hub: Don't continue on get_port_status failure
In reply to Marek VasutThe code shouldn't continue probing the port if get_port_status() failed.
--- common/usb_hub.c | 1 + 1 file changed, 1 insertion(+)
common/usb_hub.c +1 -0
@@ -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);
Signature · 1 line
2.7.0
PATCH 6/7 usb: hub: Don't continue on get_port_status failure
Quoted 18 lines · Marek Vasut
The code shouldn't continue probing the port if
get_port_status()failed.Signed-off-by Marek Vasut <[email protected]>Cc Chin Liang See <[email protected]>Cc Dinh Nguyen <[email protected]>Cc Hans de Goede <[email protected]>Cc Stefan Roese <[email protected]>Cc Stephen Warren <[email protected]>--- common/usb_hub.c | 1 + 1 file changed, 1 insertion(+)common/usb_hub.c +0
-0
common/usb_hub.c static int usb_scan_port(struct usb_device_scan *usb_scan) · line 408
Quoted 7 lines
common/usb_hub.c +1
-0@@ -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:
Thanks,
Stefan
PATCH 6/7 usb: hub: Don't continue on get_port_status failure
Quoted 26 lines · Stefan Roese
Quoted 25 lines · Marek Vasut
The code shouldn't continue probing the port if
get_port_status()failed.Signed-off-by Marek Vasut <[email protected]>Cc Chin Liang See <[email protected]>Cc Dinh Nguyen <[email protected]>Cc Hans de Goede <[email protected]>Cc Stefan Roese <[email protected]>Cc Stephen Warren <[email protected]>--- common/usb_hub.c | 1 + 1 file changed, 1 insertion(+)common/usb_hub.c +1
-0@@ -402,6 +402,7 @@ static int usb_scan_port(struct usb_device_scan *usb_scan)free (usb_scan);return 0 ; } +return 0 ; }
Quoted 7 lines
Quoted 2 lines
portstatus = le16_to_cpu(portsts->wPortStatus);Thanks for spotting this:
Reviewed-by Stefan Roese <[email protected]>
Thanks
Chin Liang
Quoted 2 lines
Thanks,
Stefan
PATCH 1/7 usb: Don't init pointer to zero, but NULL
In reply to Marek VasutQuoted 19 lines · Marek Vasut
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 <[email protected]>Cc Chin Liang See <[email protected]>Cc Dinh Nguyen <[email protected]>Cc Hans de Goede <[email protected]>Cc Stefan Roese <[email protected]>Cc Stephen Warren <[email protected]>--- common/usb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)common/usb.c +0
-0
common/usb.c static int usb_prepare_device(struct usb_device *dev, int addr, bool do_read, · line 1070
Quoted 8 lines
common/usb.c +1
-1@@ -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 );
Thanks,
Stefan
PATCH 1/7 usb: Don't init pointer to zero, but NULL
Quoted 28 lines · Stefan Roese
Quoted 27 lines · Marek Vasut
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 <[email protected]>Cc Chin Liang See <[email protected]>Cc Dinh Nguyen <[email protected]>Cc Hans de Goede <[email protected]>Cc Stefan Roese <[email protected]>Cc Stephen Warren <[email protected]>--- common/usb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)common/usb.c +1
-1@@ -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;
Quoted 6 lines
Quoted 3 lines
err = get_descriptor_len(dev,
USB_DT_DEVICE_SIZE,USB_DT_DEVICE_SIZE);Reviewed-by Stefan Roese <[email protected]>
Thanks
Chin Liang
Quoted 2 lines
Thanks,
Stefan
PATCH 7/7 usb: hub: Increase the query delay
In reply to Marek VasutIncrease 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.
--- common/usb_hub.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
common/usb_hub.c +1 -1
@@ -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)
Signature · 1 line
2.7.0
PATCH 7/7 usb: hub: Increase the query delay
Quoted 21 lines · Marek Vasut
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 <[email protected]>Cc Chin Liang See <[email protected]>Cc Dinh Nguyen <[email protected]>Cc Hans de Goede <[email protected]>Cc Stefan Roese <[email protected]>Cc Stephen Warren <[email protected]>--- common/usb_hub.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)common/usb_hub.c +0
-0
common/usb_hub.c static void usb_hub_power_on(struct usb_hub_device *hub) · line 151
Quoted 8 lines
common/usb_hub.c +1
-1@@ -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
PATCH 7/7 usb: hub: Increase the query delay
Quoted 28 lines · Stefan Roese
Quoted 27 lines · Marek Vasut
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 <[email protected]>Cc Chin Liang See <[email protected]>Cc Dinh Nguyen <[email protected]>Cc Hans de Goede <[email protected]>Cc Stefan Roese <[email protected]>Cc Stephen Warren <[email protected]>--- common/usb_hub.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)common/usb_hub.c +1
-1@@ -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);
Quoted 8 lines
Quoted 3 lines
common/usb_hub.c +0
-0/* * 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.
Quoted 5 lines
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 ?
Quoted 6 lines
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.
Quoted 7 lines
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.
Quoted 7 lines
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.
Quoted 6 lines
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.
Quoted 15 lines
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) foundtime: 1.940 seconds
Quoted 7 lines
With this patch:
=> time usb start
starting USB...
USB0: USB EHCI 1.00
scanning bus 0 for devices... 9 USB Device(s) foundtime: 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.
Quoted 2 lines
Thanks,
Stefan
Signature · 23 lines
Best regards, Marek Vasut -------------- next part -------------- A non-text attachment was scrubbed... Name: trace-dwc2-with-delay.csv Type: text/csv Size: 4657 bytes Desc: not available URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160504/74350c99/attachment.csv> -------------- next part -------------- A non-text attachment was scrubbed... Name: trace-dwc2-without-delay.csv Type: text/csv Size: 1066 bytes Desc: not available URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160504/74350c99/attachment-0001.csv> -------------- next part -------------- A non-text attachment was scrubbed... Name: trace-ehci.csv Type: text/csv Size: 4362 bytes Desc: not available URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160504/74350c99/attachment-0002.csv>
PATCH 7/7 usb: hub: Increase the query delay
Quoted 126 lines · Marek Vasut
Quoted 28 lines · Stefan Roese
Quoted 27 lines · Marek Vasut
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 <[email protected]>Cc Chin Liang See <[email protected]>Cc Dinh Nguyen <[email protected]>Cc Hans de Goede <[email protected]>Cc Stefan Roese <[email protected]>Cc Stephen Warren <[email protected]>--- common/usb_hub.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)common/usb_hub.c +1
-1@@ -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);Quoted 8 lines
Quoted 3 lines
common/usb_hub.c +0
-0/* * 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.
Quoted 5 lines
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 ?
Quoted 6 lines
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.
Quoted 7 lines
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.
Quoted 7 lines
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.Quoted 6 lines
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.
Quoted 15 lines
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) foundtime: 1.940 seconds
Quoted 7 lines
With this patch:
=> time usb start
starting USB...
USB0: USB EHCI 1.00
scanning bus 0 for devices... 9 USB Device(s) foundtime: 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.
Quoted 7 lines
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
PATCH 7/7 usb: hub: Increase the query delay
In reply to Marek Vasutcommon/usb_hub.c static void usb_hub_power_on(struct usb_hub_device *hub) · line 148
Quoted 13 lines · Marek Vasut
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.
common/usb_hub.c +1
-1@@ -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.
PATCH 1/1 usb: hub: Increase the query delay
In reply to Marek VasutIncrease 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.
---
V2: Special-case this only for DWC2
--- common/usb_hub.c | 6 ++++++ 1 file changed, 6 insertions(+)
common/usb_hub.c +6 -0
@@ -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)
Signature · 1 line
2.7.0
PATCH 1/1 usb: hub: Increase the query delay
Quoted 4 lines · Marek Vasut
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.
(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.)
PATCH 1/1 usb: hub: Increase the query delay
Quoted 11 lines · Stephen Warren
Quoted 4 lines · Marek Vasut
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 <[email protected]>(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.
Signature · 2 lines
Best regards, Marek Vasut