[PATCH 0/4][v2] xhci fixes for rpi4
This is the second iteration of the xhci patch set for making USB keyboards usable on the rpi4 board.
Patch 1 is new to deal with the USB device not responding immediately after a port reset.
Patch 3 is refactored after the review from Bin Meng.
---------------------------------------------------------------- Jason Wessel (4): xhci.c: Add retry in xhci_address_device() xhci.c: Add polling support for USB keyboards xhci-ring.c: Add poll pending state to properly abort transactions xhci-ring.c: Fix crash when issuing "usb reset"
drivers/usb/host/xhci-ring.c | 115 +++++++++++++++++++++++++++++++++---------- drivers/usb/host/xhci.c | 20 ++++++-- include/usb/xhci.h | 10 +++- 3 files changed, 111 insertions(+), 34 deletions(-)
After a port reset some usb keyboard devices do not respond immediately, and instead the controller reports COMP_TX_ERR. Adding a retry after the first TX error is returned resolves the issue.
Without the patch u-boot prints:
Starting the controller USB XHCI 1.00 scanning bus xhci_pci for devices... Device not responding to set address.
USB device not accepting new address (error=80000000)
Signed-off-by: Jason Wessel jason.wessel@windriver.com --- drivers/usb/host/xhci.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 126dabc11b..9a31eba2bb 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -675,6 +675,7 @@ static int xhci_address_device(struct usb_device *udev, int root_portnr) struct xhci_virt_device *virt_dev; int slot_id = udev->slot_id; union xhci_trb *event; + int retry_cnt = 0;
virt_dev = ctrl->devs[slot_id];
@@ -685,6 +686,7 @@ static int xhci_address_device(struct usb_device *udev, int root_portnr) debug("Setting up addressable devices %p\n", ctrl->dcbaa); xhci_setup_addressable_virt_dev(ctrl, udev, root_portnr);
+retry: ctrl_ctx = xhci_get_input_control_ctx(virt_dev->in_ctx); ctrl_ctx->add_flags = cpu_to_le32(SLOT_FLAG | EP0_FLAG); ctrl_ctx->drop_flags = 0; @@ -701,6 +703,13 @@ static int xhci_address_device(struct usb_device *udev, int root_portnr) ret = -EINVAL; break; case COMP_TX_ERR: + retry_cnt++; + if (retry_cnt < 2) { + /* Retry in case this was just after a port reset */ + debug("COMP_TX_ERR retry\n"); + xhci_acknowledge_event(ctrl); + goto retry; + } puts("Device not responding to set address.\n"); ret = -EPROTO; break;
The xhci driver was causing intermittent 5 second delays from the USB keyboard polling hook. Executing something like a "sleep 1" for example would sleep for 5 seconds, unless an event occurred on the USB bus to shorten the delay.
Modeled after the code in the DWC2 driver, a nonblock state was added to quickly return instead of blocking for up to 5 seconds waiting for an event before timing out.
Signed-off-by: Jason Wessel jason.wessel@windriver.com --- drivers/usb/host/xhci-ring.c | 26 +++++++++++++++++--------- drivers/usb/host/xhci.c | 11 ++++++----- include/usb/xhci.h | 5 +++-- 3 files changed, 26 insertions(+), 16 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index 092ed6eaf1..607d4f715e 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -432,9 +432,11 @@ static int event_ready(struct xhci_ctrl *ctrl) * * @param ctrl Host controller data structure * @param expected TRB type expected from Event TRB + * @param nonblock when true do not block waiting for response * @return pointer to event trb */ -union xhci_trb *xhci_wait_for_event(struct xhci_ctrl *ctrl, trb_type expected) +union xhci_trb *xhci_wait_for_event(struct xhci_ctrl *ctrl, trb_type expected, + bool nonblock) { trb_type type; unsigned long ts = get_timer(0); @@ -442,8 +444,11 @@ union xhci_trb *xhci_wait_for_event(struct xhci_ctrl *ctrl, trb_type expected) do { union xhci_trb *event = ctrl->event_ring->dequeue;
- if (!event_ready(ctrl)) + if (!event_ready(ctrl)) { + if (nonblock) + return NULL; continue; + }
type = TRB_FIELD_TO_TYPE(le32_to_cpu(event->event_cmd.flags)); if (type == expected) @@ -493,7 +498,7 @@ static void abort_td(struct usb_device *udev, int ep_index)
xhci_queue_command(ctrl, NULL, udev->slot_id, ep_index, TRB_STOP_RING);
- event = xhci_wait_for_event(ctrl, TRB_TRANSFER); + event = xhci_wait_for_event(ctrl, TRB_TRANSFER, false); field = le32_to_cpu(event->trans_event.flags); BUG_ON(TRB_TO_SLOT_ID(field) != udev->slot_id); BUG_ON(TRB_TO_EP_INDEX(field) != ep_index); @@ -501,7 +506,7 @@ static void abort_td(struct usb_device *udev, int ep_index) != COMP_STOP))); xhci_acknowledge_event(ctrl);
- event = xhci_wait_for_event(ctrl, TRB_COMPLETION); + event = xhci_wait_for_event(ctrl, TRB_COMPLETION, false); BUG_ON(TRB_TO_SLOT_ID(le32_to_cpu(event->event_cmd.flags)) != udev->slot_id || GET_COMP_CODE(le32_to_cpu( event->event_cmd.status)) != COMP_SUCCESS); @@ -509,7 +514,7 @@ static void abort_td(struct usb_device *udev, int ep_index)
xhci_queue_command(ctrl, (void *)((uintptr_t)ring->enqueue | ring->cycle_state), udev->slot_id, ep_index, TRB_SET_DEQ); - event = xhci_wait_for_event(ctrl, TRB_COMPLETION); + event = xhci_wait_for_event(ctrl, TRB_COMPLETION, false); BUG_ON(TRB_TO_SLOT_ID(le32_to_cpu(event->event_cmd.flags)) != udev->slot_id || GET_COMP_CODE(le32_to_cpu( event->event_cmd.status)) != COMP_SUCCESS); @@ -552,10 +557,11 @@ static void record_transfer_result(struct usb_device *udev, * @param pipe contains the DIR_IN or OUT , devnum * @param length length of the buffer * @param buffer buffer to be read/written based on the request + * @param nonblock when true do not block waiting for response * @return returns 0 if successful else -1 on failure */ int xhci_bulk_tx(struct usb_device *udev, unsigned long pipe, - int length, void *buffer) + int length, void *buffer, bool nonblock) { int num_trbs = 0; struct xhci_generic_trb *start_trb; @@ -714,8 +720,10 @@ int xhci_bulk_tx(struct usb_device *udev, unsigned long pipe,
giveback_first_trb(udev, ep_index, start_cycle, start_trb);
- event = xhci_wait_for_event(ctrl, TRB_TRANSFER); + event = xhci_wait_for_event(ctrl, TRB_TRANSFER, nonblock); if (!event) { + if (nonblock) + return -EINVAL; debug("XHCI bulk transfer timed out, aborting...\n"); abort_td(udev, ep_index); udev->status = USB_ST_NAK_REC; /* closest thing to a timeout */ @@ -911,7 +919,7 @@ int xhci_ctrl_tx(struct usb_device *udev, unsigned long pipe,
giveback_first_trb(udev, ep_index, start_cycle, start_trb);
- event = xhci_wait_for_event(ctrl, TRB_TRANSFER); + event = xhci_wait_for_event(ctrl, TRB_TRANSFER, false); if (!event) goto abort; field = le32_to_cpu(event->trans_event.flags); @@ -929,7 +937,7 @@ int xhci_ctrl_tx(struct usb_device *udev, unsigned long pipe, if (GET_COMP_CODE(le32_to_cpu(event->trans_event.transfer_len)) == COMP_SHORT_TX) { /* Short data stage, clear up additional status stage event */ - event = xhci_wait_for_event(ctrl, TRB_TRANSFER); + event = xhci_wait_for_event(ctrl, TRB_TRANSFER, false); if (!event) goto abort; BUG_ON(TRB_TO_SLOT_ID(field) != slot_id); diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 9a31eba2bb..3a359f0adc 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -495,7 +495,7 @@ static int xhci_configure_endpoints(struct usb_device *udev, bool ctx_change) xhci_flush_cache((uintptr_t)in_ctx->bytes, in_ctx->size); xhci_queue_command(ctrl, in_ctx->bytes, udev->slot_id, 0, ctx_change ? TRB_EVAL_CONTEXT : TRB_CONFIG_EP); - event = xhci_wait_for_event(ctrl, TRB_COMPLETION); + event = xhci_wait_for_event(ctrl, TRB_COMPLETION, false); BUG_ON(TRB_TO_SLOT_ID(le32_to_cpu(event->event_cmd.flags)) != udev->slot_id);
@@ -692,7 +692,7 @@ retry: ctrl_ctx->drop_flags = 0;
xhci_queue_command(ctrl, (void *)ctrl_ctx, slot_id, 0, TRB_ADDR_DEV); - event = xhci_wait_for_event(ctrl, TRB_COMPLETION); + event = xhci_wait_for_event(ctrl, TRB_COMPLETION, false); BUG_ON(TRB_TO_SLOT_ID(le32_to_cpu(event->event_cmd.flags)) != slot_id);
switch (GET_COMP_CODE(le32_to_cpu(event->event_cmd.status))) { @@ -774,7 +774,7 @@ static int _xhci_alloc_device(struct usb_device *udev) }
xhci_queue_command(ctrl, NULL, 0, 0, TRB_ENABLE_SLOT); - event = xhci_wait_for_event(ctrl, TRB_COMPLETION); + event = xhci_wait_for_event(ctrl, TRB_COMPLETION, false); BUG_ON(GET_COMP_CODE(le32_to_cpu(event->event_cmd.status)) != COMP_SUCCESS);
@@ -1160,6 +1160,7 @@ unknown: * @param buffer buffer to be read/written based on the request * @param length length of the buffer * @param interval interval of the interrupt + * @param nonblock when true do not block waiting for response * @return 0 */ static int _xhci_submit_int_msg(struct usb_device *udev, unsigned long pipe, @@ -1177,7 +1178,7 @@ static int _xhci_submit_int_msg(struct usb_device *udev, unsigned long pipe, * (at most) one TD. A TD (comprised of sg list entries) can * take several service intervals to transmit. */ - return xhci_bulk_tx(udev, pipe, length, buffer); + return xhci_bulk_tx(udev, pipe, length, buffer, nonblock); }
/** @@ -1197,7 +1198,7 @@ static int _xhci_submit_bulk_msg(struct usb_device *udev, unsigned long pipe, return -EINVAL; }
- return xhci_bulk_tx(udev, pipe, length, buffer); + return xhci_bulk_tx(udev, pipe, length, buffer, false); }
/** diff --git a/include/usb/xhci.h b/include/usb/xhci.h index 7d34103fd5..73db77fc02 100644 --- a/include/usb/xhci.h +++ b/include/usb/xhci.h @@ -1249,9 +1249,10 @@ void xhci_setup_addressable_virt_dev(struct xhci_ctrl *ctrl, void xhci_queue_command(struct xhci_ctrl *ctrl, u8 *ptr, u32 slot_id, u32 ep_index, trb_type cmd); void xhci_acknowledge_event(struct xhci_ctrl *ctrl); -union xhci_trb *xhci_wait_for_event(struct xhci_ctrl *ctrl, trb_type expected); +union xhci_trb *xhci_wait_for_event(struct xhci_ctrl *ctrl, trb_type expected, + bool nonblock); int xhci_bulk_tx(struct usb_device *udev, unsigned long pipe, - int length, void *buffer); + int length, void *buffer, bool nonblock); int xhci_ctrl_tx(struct usb_device *udev, unsigned long pipe, struct devrequest *req, int length, void *buffer); int xhci_check_maxpacket(struct usb_device *udev);
Both xhci_ctrl_tx() and xhci_bulk_tx() can be called synchronously by other drivers such as the usb storage or network, while the keyboard driver exclusively uses the polling mode.
The reason the abort needs to happen is for the case when a keyboard poll was issue but there was no response packet. If another driver such as the usb mass storage is called, it could receive the response from the keyboard because only a single TRB queue is used.
Any pending polling transactions must be aborted before switching modes to avoid corrupting the state of the controller and the driver which expects a series of commands and responses from a specific device.
Signed-off-by: Jason Wessel jason.wessel@windriver.com --- drivers/usb/host/xhci-ring.c | 80 ++++++++++++++++++++++++++++-------- include/usb/xhci.h | 5 +++ 2 files changed, 69 insertions(+), 16 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index 607d4f715e..00a0491771 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -549,19 +549,8 @@ static void record_transfer_result(struct usb_device *udev, } }
-/**** Bulk and Control transfer methods ****/ -/** - * Queues up the BULK Request - * - * @param udev pointer to the USB device structure - * @param pipe contains the DIR_IN or OUT , devnum - * @param length length of the buffer - * @param buffer buffer to be read/written based on the request - * @param nonblock when true do not block waiting for response - * @return returns 0 if successful else -1 on failure - */ -int xhci_bulk_tx(struct usb_device *udev, unsigned long pipe, - int length, void *buffer, bool nonblock) +static int _xhci_bulk_tx_queue(struct usb_device *udev, unsigned long pipe, + int length, void *buffer) { int num_trbs = 0; struct xhci_generic_trb *start_trb; @@ -575,7 +564,6 @@ int xhci_bulk_tx(struct usb_device *udev, unsigned long pipe, struct xhci_virt_device *virt_dev; struct xhci_ep_ctx *ep_ctx; struct xhci_ring *ring; /* EP transfer ring */ - union xhci_trb *event;
int running_total, trb_buff_len; unsigned int total_packet_count; @@ -719,20 +707,73 @@ int xhci_bulk_tx(struct usb_device *udev, unsigned long pipe, } while (running_total < length);
giveback_first_trb(udev, ep_index, start_cycle, start_trb); + return 0; +} + +/**** Bulk and Control transfer methods ****/ +/** + * Queues up the BULK Request + * + * @param udev pointer to the USB device structure + * @param pipe contains the DIR_IN or OUT , devnum + * @param length length of the buffer + * @param buffer buffer to be read/written based on the request + * @param nonblock when true do not block waiting for response + * @return returns 0 if successful else -1 on failure + */ +int xhci_bulk_tx(struct usb_device *udev, unsigned long pipe, + int length, void *buffer, bool nonblock) +{ + u32 field; + int ret; + union xhci_trb *event; + struct xhci_ctrl *ctrl = xhci_get_ctrl(udev); + int ep_index = usb_pipe_ep_index(pipe);
+ if (ctrl->poll_pend) { + /* + * Abort a pending poll operation if it should have + * timed out, or if this is a different buffer from a + * separate request + */ + if (get_timer(ctrl->bulk_tx_poll_ts) > XHCI_TIMEOUT || + ctrl->last_bulk_tx_buf != buffer || ctrl->poll_last_udev != udev || + ep_index != ctrl->poll_last_ep_index) { + abort_td(ctrl->poll_last_udev, ctrl->poll_last_ep_index); + ctrl->poll_last_udev->status = USB_ST_NAK_REC; /* closest thing to a timeout */ + ctrl->poll_last_udev->act_len = 0; + ctrl->poll_pend = false; + } + } /* No else here because poll_pend might have changed above */ + if (!ctrl->poll_pend) { + ctrl->last_bulk_tx_buf = buffer; + ret = _xhci_bulk_tx_queue(udev, pipe, length, buffer); + if (ret) + return ret; + } event = xhci_wait_for_event(ctrl, TRB_TRANSFER, nonblock); if (!event) { - if (nonblock) + if (nonblock) { + if (!ctrl->poll_pend) { + /* Start the timer */ + ctrl->bulk_tx_poll_ts = get_timer(0); + ctrl->poll_last_udev = udev; + ctrl->poll_last_ep_index = ep_index; + ctrl->poll_pend = true; + } return -EINVAL; + } debug("XHCI bulk transfer timed out, aborting...\n"); abort_td(udev, ep_index); udev->status = USB_ST_NAK_REC; /* closest thing to a timeout */ udev->act_len = 0; + ctrl->poll_pend = false; return -ETIMEDOUT; } + ctrl->poll_pend = false; field = le32_to_cpu(event->trans_event.flags);
- BUG_ON(TRB_TO_SLOT_ID(field) != slot_id); + BUG_ON(TRB_TO_SLOT_ID(field) != udev->slot_id); BUG_ON(TRB_TO_EP_INDEX(field) != ep_index); BUG_ON(*(void **)(uintptr_t)le64_to_cpu(event->trans_event.buffer) - buffer > (size_t)length); @@ -779,6 +820,13 @@ int xhci_ctrl_tx(struct usb_device *udev, unsigned long pipe, le16_to_cpu(req->value), le16_to_cpu(req->value), le16_to_cpu(req->index));
+ if (ctrl->poll_pend) { + abort_td(ctrl->poll_last_udev, ctrl->poll_last_ep_index); + ctrl->poll_last_udev->status = USB_ST_NAK_REC; /* closest thing to a timeout */ + ctrl->poll_last_udev->act_len = 0; + ctrl->poll_pend = false; + } + ep_index = usb_pipe_ep_index(pipe);
ep_ring = virt_dev->eps[ep_index].ring; diff --git a/include/usb/xhci.h b/include/usb/xhci.h index 73db77fc02..72082b3775 100644 --- a/include/usb/xhci.h +++ b/include/usb/xhci.h @@ -1227,6 +1227,11 @@ struct xhci_ctrl { struct xhci_scratchpad *scratchpad; struct xhci_virt_device *devs[MAX_HC_SLOTS]; int rootdev; + bool poll_pend; /* Pending state for polling for an event for a udev */ + unsigned long bulk_tx_poll_ts; + void *last_bulk_tx_buf; + struct usb_device *poll_last_udev; + int poll_last_ep_index; };
unsigned long trb_addr(struct xhci_segment *seg, union xhci_trb *trb);
When TRB_TRANSFER is set for a call to xhci_wait_for_event, it can return null, which causes u-boot to crash. This is an intermittent problem found during "usb reset" testing.
If the null return occurs it means there was a timeout, and the abort_td() should return immediately vs crashing u-boot from a null dereference.
Signed-off-by: Jason Wessel jason.wessel@windriver.com --- drivers/usb/host/xhci-ring.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index 00a0491771..bbb4410908 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -499,11 +499,16 @@ static void abort_td(struct usb_device *udev, int ep_index) xhci_queue_command(ctrl, NULL, udev->slot_id, ep_index, TRB_STOP_RING);
event = xhci_wait_for_event(ctrl, TRB_TRANSFER, false); - field = le32_to_cpu(event->trans_event.flags); - BUG_ON(TRB_TO_SLOT_ID(field) != udev->slot_id); - BUG_ON(TRB_TO_EP_INDEX(field) != ep_index); - BUG_ON(GET_COMP_CODE(le32_to_cpu(event->trans_event.transfer_len - != COMP_STOP))); + if (event) { + field = le32_to_cpu(event->trans_event.flags); + BUG_ON(TRB_TO_SLOT_ID(field) != udev->slot_id); + BUG_ON(TRB_TO_EP_INDEX(field) != ep_index); + BUG_ON(GET_COMP_CODE(le32_to_cpu(event->trans_event.transfer_len + != COMP_STOP))); + } else { + debug("XHCI abort timeout\n"); + return; + } xhci_acknowledge_event(ctrl);
event = xhci_wait_for_event(ctrl, TRB_COMPLETION, false);
participants (1)
-
Jason Wessel