Keith Short has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33671
Change subject: libpayload/usb: Increase the SET ADDRESS timeout ......................................................................
libpayload/usb: Increase the SET ADDRESS timeout
The SET ADDRESS timeout was set to only 100 ms for the xHCI driver. The USB specification indicates a maximum response time of 50 ms for the SET ADDRESS request, but this does not account for propagation time of the request through USB hubs.
Analysis of other USB host driver timeouts: EHCI: 3 seconds OHCI: 2 seconds UHCI: 30 ms DWC: 3 seconds
BUG=b:124730179 BRANCH=none TEST=Build libpayload and depthcharge on sarien/arcada. TEST=Without change replicate USB set address timeouts in depthcharge when dock and 4K monitor connected (which includes a total of 4 USB hubs). With timeout fix, depthcharge boots OS with no USB errors and the same USB topology.
Change-Id: I53e3e67d893420e7c9e8b52c47dd0edb979e5468 Signed-off-by: Keith Short keithshort@chromium.org --- M payloads/libpayload/drivers/usb/xhci_events.c 1 file changed, 6 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/33671/1
diff --git a/payloads/libpayload/drivers/usb/xhci_events.c b/payloads/libpayload/drivers/usb/xhci_events.c index dacb5d86..e2c124f 100644 --- a/payloads/libpayload/drivers/usb/xhci_events.c +++ b/payloads/libpayload/drivers/usb/xhci_events.c @@ -281,12 +281,13 @@ const int clear_event) { /* - * The Address Device Command should take most time, as it has to - * communicate with the USB device. Set address processing shouldn't - * take longer than 50ms (at the slave). Let's take a timeout of - * 100ms. + * The USB specification indicates that maximum response time to a SET + * ADDRESS request is 50 ms. However, this time does not account for + * the propagation delay to send the request through any hubs connected + * between the device and the root hub. Set the timeout to the maximum + * USB processing time, 5 seconds, to allow for complex USB topologies. */ - unsigned long timeout_us = 100 * 1000; /* 100ms */ + unsigned long timeout_us = 5 * 1000 * 1000; /* 5s */ int cc = TIMEOUT; while (xhci_wait_for_event_type(xhci, TRB_EV_CMD_CMPL, &timeout_us)) { if ((xhci->er.cur->ptr_low == virt_to_phys(address)) &&
Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33671 )
Change subject: libpayload/usb: Increase the SET ADDRESS timeout ......................................................................
Patch Set 1: Code-Review+1
I believe that value match with Kernel
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33671 )
Change subject: libpayload/usb: Increase the SET ADDRESS timeout ......................................................................
Patch Set 1: Code-Review+2
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33671 )
Change subject: libpayload/usb: Increase the SET ADDRESS timeout ......................................................................
Patch Set 1:
Verify Pass on my side.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33671 )
Change subject: libpayload/usb: Increase the SET ADDRESS timeout ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/#/c/33671/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33671/1//COMMIT_MSG@7 PS1, Line 7: libpayload/usb: Increase the SET ADDRESS timeout Maybe use:
libpayload/usb: Increase SET ADDRESS time-out to 5 s
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33671 )
Change subject: libpayload/usb: Increase the SET ADDRESS timeout ......................................................................
Patch Set 1:
@Duncan,do we need this into our FW branch? Or we can keep this for AU, if we don't have time.
Hello EricR Lai, Julius Werner, Duncan Laurie, Paul Menzel, Lance Zhao, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33671
to look at the new patch set (#2).
Change subject: libpayload/usb: Increase SET ADDRESS time-out to 5 s ......................................................................
libpayload/usb: Increase SET ADDRESS time-out to 5 s
The SET ADDRESS timeout was set to only 100 ms for the xHCI driver. The USB specification indicates a maximum response time of 50 ms for the SET ADDRESS request, but this does not account for propagation time of the request through USB hubs.
Analysis of other USB host driver timeouts: EHCI: 3 seconds OHCI: 2 seconds UHCI: 30 ms DWC: 3 seconds
BUG=b:124730179 BRANCH=sarien TEST=Build libpayload and depthcharge on sarien/arcada. TEST=Without change replicate USB set address timeouts in depthcharge when dock and 4K monitor connected (which includes a total of 4 USB hubs). With timeout fix, depthcharge boots OS with no USB errors and the same USB topology.
Change-Id: I53e3e67d893420e7c9e8b52c47dd0edb979e5468 Signed-off-by: Keith Short keithshort@chromium.org --- M payloads/libpayload/drivers/usb/xhci_events.c 1 file changed, 6 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/33671/2
Keith Short has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33671 )
Change subject: libpayload/usb: Increase SET ADDRESS time-out to 5 s ......................................................................
Patch Set 2:
Patch Set 1:
@Duncan,do we need this into our FW branch? Or we can keep this for AU, if we don't have time.
This is needed in the FW branch, but it can land in the AU. This bug only occurs when developer mode or recovery mode is enabled, and the USB topology is quite complex.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33671 )
Change subject: libpayload/usb: Increase SET ADDRESS time-out to 5 s ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 1:
@Duncan,do we need this into our FW branch? Or we can keep this for AU, if we don't have time.
This is needed in the FW branch, but it can land in the AU. This bug only occurs when developer mode or recovery mode is enabled, and the USB topology is quite complex.
okay, got it. this is corner case and no user impact. thx
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33671 )
Change subject: libpayload/usb: Increase SET ADDRESS time-out to 5 s ......................................................................
Patch Set 2:
(2 comments)
This is needed in the FW branch, but it can land in the AU. This bug only occurs when developer mode or recovery mode is enabled, and the USB topology is quite complex.
Be aware that recovery mode always runs RO firmware code that can never be updated. If this fixes a recovery mode problem and you need that to be fixed on your devices, you have to get it into the initial factory firmware.
https://review.coreboot.org/#/c/33671/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33671/2//COMMIT_MSG@14 PS2, Line 14: Analysis of other USB host driver timeouts: Why not unify all drivers to a consistent value then, rather than changing XHCI to yet another number that's not shared with anything else?
https://review.coreboot.org/#/c/33671/2/payloads/libpayload/drivers/usb/xhci... File payloads/libpayload/drivers/usb/xhci_events.c:
https://review.coreboot.org/#/c/33671/2/payloads/libpayload/drivers/usb/xhci... PS2, Line 288: * USB processing time, 5 seconds, to allow for complex USB topologies. I'm okay with the patch in general but this explanation sounds wrong. What is the "USB processing time"? USB packets are scheduled in 125us frames and SET_ADDRESS is a very small packet that should only take up a tiny fraction of that. Electrical propagation delays through hubs should be on the order of nanoseconds and completely irrelevant for this. There should be no way any USB topology should have a significant impact on the time needed to assign an address to a single device, let alone something as extreme as whole seconds.
The truth with these kinds of issues is usually that some device (might be the hub) is garbage and takes way longer than the spec demands to accept the SET_ADDRESS command.
Keith Short has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33671 )
Change subject: libpayload/usb: Increase SET ADDRESS time-out to 5 s ......................................................................
Patch Set 2:
(1 comment)
Patch Set 2:
(2 comments)
This is needed in the FW branch, but it can land in the AU. This bug only occurs when developer mode or recovery mode is enabled, and the USB topology is quite complex.
Be aware that recovery mode always runs RO firmware code that can never be updated. If this fixes a recovery mode problem and you need that to be fixed on your devices, you have to get it into the initial factory firmware.
Thanks for clarifying. I'll check with the platform team to see if we need this feature in the RO firmware.
https://review.coreboot.org/#/c/33671/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33671/2//COMMIT_MSG@14 PS2, Line 14: Analysis of other USB host driver timeouts:
Why not unify all drivers to a consistent value then, rather than changing XHCI to yet another numbe […]
I agree a unified timeout value is the preferred solution. What would suggest in terms of testing out the change? How would I determine which platforms include the older USB driver models?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33671 )
Change subject: libpayload/usb: Increase SET ADDRESS time-out to 5 s ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/33671/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33671/2//COMMIT_MSG@14 PS2, Line 14: Analysis of other USB host driver timeouts:
I agree a unified timeout value is the preferred solution. […]
We haven't had any Chromebooks with anything other than XHCI for a while. The last devices where I can say for sure what they had were Nyan_Big/_Blaze for EHCI, Daisy for OHCI, ZGB for UHCI and Veyron_* for DWC2. I don't think you want to dig any of those out of the e-waste bin just to test this.
I think in coreboot we usually accept best effort testing -- if you can test it easily do so, but if you can't it's not required. Changing a timeout from one number to another should be pretty harmless.
Hello EricR Lai, Julius Werner, Duncan Laurie, Paul Menzel, Lance Zhao, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33671
to look at the new patch set (#3).
Change subject: libpayload/usb: Increase SET ADDRESS time-out to 5 s ......................................................................
libpayload/usb: Increase SET ADDRESS time-out to 5 s
Increase the SET ADDRESS timeout for all USB host controllers to 5 seconds. The USB specification indicates a maximum response time of 50 ms for the SET ADDRESS request, but this does not account for propagation time of the request through USB hubs.
BUG=b:124730179 BRANCH=sarien TEST=Build libpayload and depthcharge on sarien/arcada. TEST=Without change replicate USB set address timeouts in depthcharge when dock and 4K monitor connected (which includes a total of 4 USB hubs). With timeout fix, depthcharge boots OS with no USB errors and the same USB topology. Note that this tests xHCI operation only.
Change-Id: I53e3e67d893420e7c9e8b52c47dd0edb979e5468 Signed-off-by: Keith Short keithshort@chromium.org --- M payloads/libpayload/drivers/usb/dwc2.c M payloads/libpayload/drivers/usb/ehci.c M payloads/libpayload/drivers/usb/ohci.c M payloads/libpayload/drivers/usb/uhci.c M payloads/libpayload/drivers/usb/xhci_events.c M payloads/libpayload/include/usb/usb.h 6 files changed, 30 insertions(+), 29 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/33671/3
Keith Short has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33671 )
Change subject: libpayload/usb: Increase SET ADDRESS time-out to 5 s ......................................................................
Patch Set 3:
Patchset 2 uses a common timeout of 5 seconds for all USB host controllers.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33671 )
Change subject: libpayload/usb: Increase SET ADDRESS time-out to 5 s ......................................................................
Patch Set 3: Code-Review+1
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33671 )
Change subject: libpayload/usb: Increase SET ADDRESS time-out to 5 s ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/#/c/33671/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33671/3//COMMIT_MSG@11 PS3, Line 11: but this does not account for : propagation time of the request through USB hubs. nit: this explanation is still incorrect
https://review.coreboot.org/#/c/33671/3/payloads/libpayload/drivers/usb/dwc2... File payloads/libpayload/drivers/usb/dwc2.c:
https://review.coreboot.org/#/c/33671/3/payloads/libpayload/drivers/usb/dwc2... PS3, Line 384: 3000 I think you need to change this as well (note that even though the udelay is 500us, this still counts full milliseconds, like the comment below explains).
https://review.coreboot.org/#/c/33671/3/payloads/libpayload/drivers/usb/ehci... File payloads/libpayload/drivers/usb/ehci.c:
https://review.coreboot.org/#/c/33671/3/payloads/libpayload/drivers/usb/ehci... PS3, Line 265: Maybe move this info to the definition of USB_MAX_PROCESSING_TIME_US?
https://review.coreboot.org/#/c/33671/3/payloads/libpayload/drivers/usb/xhci... File payloads/libpayload/drivers/usb/xhci_events.c:
https://review.coreboot.org/#/c/33671/3/payloads/libpayload/drivers/usb/xhci... PS3, Line 288: * USB processing time, 5 seconds, to allow for complex USB topologies. This explanation is still wrong.
Hello EricR Lai, Julius Werner, Paul Menzel, Duncan Laurie, Lance Zhao, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33671
to look at the new patch set (#4).
Change subject: libpayload/usb: Increase USB request timeout to 5 s ......................................................................
libpayload/usb: Increase USB request timeout to 5 s
Increase the timeout for USB requests to 5 seconds for all USB host controllers.
Prior to this fix, the xCHI driver was detecting false timeouts during SET ADDRESS requests when nested downstream hubs were connected to the xHCI root hub.
BUG=b:124730179 BRANCH=sarien TEST=Build libpayload and depthcharge on sarien/arcada. TEST=Without change replicate USB set address timeouts in depthcharge when dock and 4K monitor connected (which includes a total of 4 USB hubs). With timeout fix, depthcharge boots OS with no USB errors and the same USB topology. Note that this tests xHCI operation only.
Change-Id: I53e3e67d893420e7c9e8b52c47dd0edb979e5468 Signed-off-by: Keith Short keithshort@chromium.org --- M payloads/libpayload/drivers/usb/dwc2.c M payloads/libpayload/drivers/usb/ehci.c M payloads/libpayload/drivers/usb/ohci.c M payloads/libpayload/drivers/usb/uhci.c M payloads/libpayload/drivers/usb/xhci_events.c M payloads/libpayload/include/usb/usb.h 6 files changed, 45 insertions(+), 35 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/33671/4
Keith Short has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33671 )
Change subject: libpayload/usb: Increase USB request timeout to 5 s ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/#/c/33671/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33671/3//COMMIT_MSG@11 PS3, Line 11: but this does not account for : propagation time of the request through USB hubs.
nit: this explanation is still incorrect
Done
https://review.coreboot.org/#/c/33671/3/payloads/libpayload/drivers/usb/dwc2... File payloads/libpayload/drivers/usb/dwc2.c:
https://review.coreboot.org/#/c/33671/3/payloads/libpayload/drivers/usb/dwc2... PS3, Line 384: 3000
I think you need to change this as well (note that even though the udelay is 500us, this still count […]
Done
https://review.coreboot.org/#/c/33671/3/payloads/libpayload/drivers/usb/ehci... File payloads/libpayload/drivers/usb/ehci.c:
https://review.coreboot.org/#/c/33671/3/payloads/libpayload/drivers/usb/ehci... PS3, Line 265:
Maybe move this info to the definition of USB_MAX_PROCESSING_TIME_US?
Done
https://review.coreboot.org/#/c/33671/3/payloads/libpayload/drivers/usb/xhci... File payloads/libpayload/drivers/usb/xhci_events.c:
https://review.coreboot.org/#/c/33671/3/payloads/libpayload/drivers/usb/xhci... PS3, Line 288: * USB processing time, 5 seconds, to allow for complex USB topologies.
This explanation is still wrong.
Done
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33671 )
Change subject: libpayload/usb: Increase USB request timeout to 5 s ......................................................................
Patch Set 4: Code-Review+2
(3 comments)
https://review.coreboot.org/#/c/33671/4/payloads/libpayload/drivers/usb/dwc2... File payloads/libpayload/drivers/usb/dwc2.c:
https://review.coreboot.org/#/c/33671/4/payloads/libpayload/drivers/usb/dwc2... PS4, Line 149: DWC nit: better to use DWC2, there are other DesignWare USB controllers that this driver has nothing to do with
https://review.coreboot.org/#/c/33671/4/payloads/libpayload/drivers/usb/dwc2... PS4, Line 379: DWC_SPLIT_SLEEP_TIME_US nit: if you want to define this, it should be USB_LOW_SPEED_FRAME_US
https://review.coreboot.org/#/c/33671/4/payloads/libpayload/include/usb/usb.... File payloads/libpayload/include/usb/usb.h:
https://review.coreboot.org/#/c/33671/4/payloads/libpayload/include/usb/usb.... PS4, Line 75: command to be processed nit: it's really more "for any transfer to be completed". commands are an XHCI-specific thing, but in the end they are used to tell the controller to make a transfer.
Hello EricR Lai, Julius Werner, Paul Menzel, Duncan Laurie, Lance Zhao, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33671
to look at the new patch set (#5).
Change subject: libpayload/usb: Increase USB request timeout to 5 s ......................................................................
libpayload/usb: Increase USB request timeout to 5 s
Increase the timeout for USB requests to 5 seconds for all USB host controllers.
Prior to this fix, the xCHI driver was detecting false timeouts during SET ADDRESS requests when nested downstream hubs were connected to the xHCI root hub.
BUG=b:124730179 BRANCH=sarien TEST=Build libpayload and depthcharge on sarien/arcada. TEST=Without change replicate USB set address timeouts in depthcharge when dock and 4K monitor connected (which includes a total of 4 USB hubs). With timeout fix, depthcharge boots OS with no USB errors and the same USB topology. Note that this tests xHCI operation only.
Change-Id: I53e3e67d893420e7c9e8b52c47dd0edb979e5468 Signed-off-by: Keith Short keithshort@chromium.org --- M payloads/libpayload/drivers/usb/dwc2.c M payloads/libpayload/drivers/usb/ehci.c M payloads/libpayload/drivers/usb/ohci.c M payloads/libpayload/drivers/usb/uhci.c M payloads/libpayload/drivers/usb/xhci_events.c M payloads/libpayload/include/usb/usb.h 6 files changed, 47 insertions(+), 35 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/33671/5
Keith Short has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33671 )
Change subject: libpayload/usb: Increase USB request timeout to 5 s ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/#/c/33671/4/payloads/libpayload/drivers/usb/dwc2... File payloads/libpayload/drivers/usb/dwc2.c:
https://review.coreboot.org/#/c/33671/4/payloads/libpayload/drivers/usb/dwc2... PS4, Line 149: DWC
nit: better to use DWC2, there are other DesignWare USB controllers that this driver has nothing to […]
Done
https://review.coreboot.org/#/c/33671/4/payloads/libpayload/drivers/usb/dwc2... PS4, Line 379: DWC_SPLIT_SLEEP_TIME_US
nit: if you want to define this, it should be USB_LOW_SPEED_FRAME_US
Done
https://review.coreboot.org/#/c/33671/4/payloads/libpayload/include/usb/usb.... File payloads/libpayload/include/usb/usb.h:
https://review.coreboot.org/#/c/33671/4/payloads/libpayload/include/usb/usb.... PS4, Line 75: command to be processed
nit: it's really more "for any transfer to be completed". […]
Done
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33671 )
Change subject: libpayload/usb: Increase USB request timeout to 5 s ......................................................................
Patch Set 5: Code-Review+2
Keith Short has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33671 )
Change subject: libpayload/usb: Increase USB request timeout to 5 s ......................................................................
Patch Set 5:
Added Patrick to get this change cherry picked.
Duncan Laurie has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33671 )
Change subject: libpayload/usb: Increase USB request timeout to 5 s ......................................................................
libpayload/usb: Increase USB request timeout to 5 s
Increase the timeout for USB requests to 5 seconds for all USB host controllers.
Prior to this fix, the xCHI driver was detecting false timeouts during SET ADDRESS requests when nested downstream hubs were connected to the xHCI root hub.
BUG=b:124730179 BRANCH=sarien TEST=Build libpayload and depthcharge on sarien/arcada. TEST=Without change replicate USB set address timeouts in depthcharge when dock and 4K monitor connected (which includes a total of 4 USB hubs). With timeout fix, depthcharge boots OS with no USB errors and the same USB topology. Note that this tests xHCI operation only.
Change-Id: I53e3e67d893420e7c9e8b52c47dd0edb979e5468 Signed-off-by: Keith Short keithshort@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/33671 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Julius Werner jwerner@chromium.org --- M payloads/libpayload/drivers/usb/dwc2.c M payloads/libpayload/drivers/usb/ehci.c M payloads/libpayload/drivers/usb/ohci.c M payloads/libpayload/drivers/usb/uhci.c M payloads/libpayload/drivers/usb/xhci_events.c M payloads/libpayload/include/usb/usb.h 6 files changed, 47 insertions(+), 35 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved
diff --git a/payloads/libpayload/drivers/usb/dwc2.c b/payloads/libpayload/drivers/usb/dwc2.c index 34de3bc..06c54e1 100644 --- a/payloads/libpayload/drivers/usb/dwc2.c +++ b/payloads/libpayload/drivers/usb/dwc2.c @@ -146,6 +146,8 @@ return !(hprt.prtena && hprt.prtconnsts); }
+#define DWC2_SLEEP_TIME_US 5 + /* * This function returns the actual transfer length when the transfer succeeded * or an error code if the transfer failed @@ -157,14 +159,14 @@ hcchar_t hcchar; hctsiz_t hctsiz; dwc2_reg_t *reg = DWC2_REG(ep->dev->controller); - int timeout = 600000; /* time out after 600000 * 5us == 3s */ + int timeout = USB_MAX_PROCESSING_TIME_US / DWC_SLEEP_TIME_US;
/* * TODO: We should take care of up to three times of transfer error * retry here, according to the USB 2.0 spec 4.5.2 */ do { - udelay(5); + udelay(DWC_SLEEP_TIME_US); hcint.d32 = readl(®->host.hchn[ch_num].hcintn); hctsiz.d32 = readl(®->host.hchn[ch_num].hctsizn);
@@ -374,12 +376,15 @@ return 1; }
+#define USB_FULL_LOW_SPEED_FRAME_US 1000 + static int dwc2_transfer(endpoint_t *ep, int size, int pid, ep_dir_t dir, uint32_t ch_num, u8 *src, uint8_t skip_nak) { split_info_t split; - int ret, short_pkt, transferred = 0, timeout = 3000; + int ret, short_pkt, transferred = 0; + int timeout = USB_MAX_PROCESSING_TIME_US / USB_FULL_LOW_SPEED_FRAME_US;
ep->toggle = pid;
@@ -393,11 +398,11 @@
/* * dwc2_split_transfer() waits for the next FullSpeed - * frame boundary, so we have one try per millisecond. - * It's 3s timeout for each split transfer. + * frame boundary, so we only need to delay 500 us + * here to have one try per millisecond. */ if (ret == -HCSTAT_NAK && !skip_nak && --timeout) { - udelay(500); + udelay(USB_FULL_LOW_SPEED_FRAME_US / 2); goto nak_retry; } } else { diff --git a/payloads/libpayload/drivers/usb/ehci.c b/payloads/libpayload/drivers/usb/ehci.c index 3d6c077..6876340 100644 --- a/payloads/libpayload/drivers/usb/ehci.c +++ b/payloads/libpayload/drivers/usb/ehci.c @@ -247,6 +247,8 @@ free((void *)qh); }
+#define EHCI_SLEEP_TIME_US 50 + static int wait_for_tds(qtd_t *head) { /* returns the amount of bytes *not* transmitted, or -1 for error */ @@ -256,18 +258,10 @@ if (0) dump_td(virt_to_phys(cur));
/* wait for results */ - /* how long to wait? - * tested with some USB2.0 flash sticks: - * TUR turn around took - * about 2.2s for the slowest (13fe:3800) - * max. 250ms for the others - * slowest non-TUR turn around took about 1.3s - * set to 3s to be safe as a failed TUR can be fatal - */ - int timeout = 60000; /* time out after 60000 * 50us == 3s */ + int timeout = USB_MAX_PROCESSING_TIME_US / EHCI_SLEEP_TIME_US; while ((cur->token & QTD_ACTIVE) && !(cur->token & QTD_HALTED) && timeout--) - udelay(50); + udelay(EHCI_SLEEP_TIME_US); if (timeout < 0) { usb_debug("Error: ehci: queue transfer " "processing timed out.\n"); diff --git a/payloads/libpayload/drivers/usb/ohci.c b/payloads/libpayload/drivers/usb/ohci.c index ecd1084..f1dc081 100644 --- a/payloads/libpayload/drivers/usb/ohci.c +++ b/payloads/libpayload/drivers/usb/ohci.c @@ -292,14 +292,13 @@ OHCI_INST (controller)->opreg->HcControl &= ~PeriodicListEnable; }
+#define OHCI_SLEEP_TIME_US 1000 + static int wait_for_ed(usbdev_t *dev, ed_t *head, int pages) { /* wait for results */ - /* TOTEST: how long to wait? - * give 2s per TD (2 pages) plus another 2s for now - */ - int timeout = pages*1000 + 2000; + int timeout = USB_MAX_PROCESSING_TIME_US / OHCI_SLEEP_TIME_US; while (((head->head_pointer & ~3) != head->tail_pointer) && !(head->head_pointer & 1) && ((((td_t*)phys_to_virt(head->head_pointer & ~3))->config @@ -315,9 +314,9 @@ ((td_t*)phys_to_virt(head->head_pointer & ~3))->next_td, head->tail_pointer, (((td_t*)phys_to_virt(head->head_pointer & ~3))->config & TD_CC_MASK) >> TD_CC_SHIFT); - mdelay(1); + udelay(OHCI_SLEEP_TIME_US); } - if (timeout < 0) + if (timeout <= 0) usb_debug("Error: ohci: endpoint " "descriptor processing timed out.\n"); /* Clear the done queue. */ diff --git a/payloads/libpayload/drivers/usb/uhci.c b/payloads/libpayload/drivers/usb/uhci.c index e62fd25..32d4090 100644 --- a/payloads/libpayload/drivers/usb/uhci.c +++ b/payloads/libpayload/drivers/usb/uhci.c @@ -272,21 +272,23 @@ uhci_reg_read16(controller, USBCMD) & ~1); // stop work on schedule }
+#define UHCI_SLEEP_TIME_US 30 +#define UHCI_TIMEOUT (USB_MAX_PROCESSING_TIME_US / UHCI_SLEEP_TIME_US) #define GET_TD(x) ((void*)(((unsigned int)(x))&~0xf))
static td_t * wait_for_completed_qh (hci_t *controller, qh_t *qh) { - int timeout = 1000; /* max 30 ms. */ + int timeout = UHCI_TIMEOUT; void *current = GET_TD (qh->elementlinkptr); while (((qh->elementlinkptr & FLISTP_TERMINATE) == 0) && (timeout-- > 0)) { if (current != GET_TD (qh->elementlinkptr)) { current = GET_TD (qh->elementlinkptr); - timeout = 1000; + timeout = UHCI_TIMEOUT; } uhci_reg_write16(controller, USBSTS, uhci_reg_read16(controller, USBSTS) | 0); // clear resettable registers - udelay (30); + udelay(UHCI_SLEEP_TIME_US); } return (GET_TD (qh->elementlinkptr) == 0) ? 0 : GET_TD (phys_to_virt (qh->elementlinkptr)); diff --git a/payloads/libpayload/drivers/usb/xhci_events.c b/payloads/libpayload/drivers/usb/xhci_events.c index dacb5d86..c21797d 100644 --- a/payloads/libpayload/drivers/usb/xhci_events.c +++ b/payloads/libpayload/drivers/usb/xhci_events.c @@ -236,7 +236,7 @@ * we don't get a response after 5s. Still, let the caller decide, * what to do then. */ - unsigned long timeout_us = 5 * 1000 * 1000; /* 5s */ + unsigned long timeout_us = USB_MAX_PROCESSING_TIME_US; /* 5s */ int cc = TIMEOUT; /* * Expects two command completion events: @@ -280,13 +280,7 @@ const trb_t *const address, const int clear_event) { - /* - * The Address Device Command should take most time, as it has to - * communicate with the USB device. Set address processing shouldn't - * take longer than 50ms (at the slave). Let's take a timeout of - * 100ms. - */ - unsigned long timeout_us = 100 * 1000; /* 100ms */ + unsigned long timeout_us = USB_MAX_PROCESSING_TIME_US; /* 5s */ int cc = TIMEOUT; while (xhci_wait_for_event_type(xhci, TRB_EV_CMD_CMPL, &timeout_us)) { if ((xhci->er.cur->ptr_low == virt_to_phys(address)) && @@ -311,8 +305,8 @@ xhci_wait_for_transfer(xhci_t *const xhci, const int slot_id, const int ep_id) { xhci_spew("Waiting for transfer on ID %d EP %d\n", slot_id, ep_id); - /* 3s for all types of transfers */ /* TODO: test, wait longer? */ - unsigned long timeout_us = 3 * 1000 * 1000; + /* 5s for all types of transfers */ + unsigned long timeout_us = USB_MAX_PROCESSING_TIME_US; int ret = TIMEOUT; while (xhci_wait_for_event_type(xhci, TRB_EV_TRANSFER, &timeout_us)) { if (TRB_GET(ID, xhci->er.cur) == slot_id && diff --git a/payloads/libpayload/include/usb/usb.h b/payloads/libpayload/include/usb/usb.h index 79c4586..db7ec57 100644 --- a/payloads/libpayload/include/usb/usb.h +++ b/payloads/libpayload/include/usb/usb.h @@ -71,6 +71,24 @@ /* SetAddress() recovery interval (USB 2.0 specification 9.2.6.3 */ #define SET_ADDRESS_MDELAY 2
+/* + * USB sets an upper limit of 5 seconds for any transfer to be completed. + * + * Data originally from EHCI driver: + * Tested with some USB2.0 flash sticks: + * TUR turn around took about 2.2s for the slowest (13fe:3800), maximum + * of 250ms for the others. + * + * SET ADDRESS on xHCI controllers. + * The USB specification indicates that devices must complete processing + * of a SET ADDRESS request within 50 ms. However, some hubs were found + * to take more than 100 ms to complete a SET ADDRESS request on a + * downstream port. + */ +#define USB_MAX_PROCESSING_TIME_US (5 * 1000 * 1000) + +#define USB_FULL_LOW_SPEED_FRAME_US 1000 + typedef struct { unsigned char bDescLength; unsigned char bDescriptorType;
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33671 )
Change subject: libpayload/usb: Increase USB request timeout to 5 s ......................................................................
Patch Set 6:
(3 comments)
this fails to build (and apparently we need more libpayload configs to test it on qa.c.o)
https://review.coreboot.org/c/coreboot/+/33671/6/payloads/libpayload/drivers... File payloads/libpayload/drivers/usb/dwc2.c:
https://review.coreboot.org/c/coreboot/+/33671/6/payloads/libpayload/drivers... PS6, Line 149: DWC2_SLEEP_TIME_US DWC2_*
https://review.coreboot.org/c/coreboot/+/33671/6/payloads/libpayload/drivers... PS6, Line 162: DWC_SLEEP_TIME_US DWC_*
https://review.coreboot.org/c/coreboot/+/33671/6/payloads/libpayload/drivers... PS6, Line 169: DWC_SLEEP_TIME_US DWC_*
-
Duncan Laurie (Code Review) -
EricR Lai (Code Review)
-
Julius Werner (Code Review)
-
Keith Short (Code Review)
-
Lance Zhao (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)