[RFC PATCH v3 0/6] Improve USB Keyboard support for rpi3/rpi4
At this point all the USB keyboards I had laying around now work and I can USB boot the rpi3 and rpi4, so perhaps this series can move beyond the RFC stage. More testing will occur over the next week or so.
v3: - Add in patch 6 - Finally got the GearHead keyboard + mouse composite USB device working
v2: - Minor cleanups to patches 1-3 based on prior review - Patch 4 & 5 are new to fix various xhchi crashes while having additional devices plugged in along with booting off the usb port. The nonblocking mode turned out to be somewhat complex.
v1:
For testing this patch series, I did apply the USB patches for the rpi4 board which had not been merged yet. Once the rpi4 USB changes are eventually merged the keyboard delay issues will show up and I imagine this issue is already a problem for other boards. This series does not depend on rpi4 patches as the changes are in the core USB functions.
I performed tests with a number of usb modules attached including storage, ethernet, serial, wifi, keyboard, and mouse without any issues.
---------------------------------------------------------------- Jason Wessel (6): xhci: Add polling support for USB keyboards usb_kbd: Do not fail the keyboard if it does not have an interrupt pending common/usb.c: Work around keyboard reporting "USB device not accepting new address" xhci-ring.c: Add the poll_pend state to properly abort transactions xhci-ring: Fix crash when issuing "usb reset" usb.c: Add a retry in the usb_prepare_device()
common/usb.c | 18 +++++++++--- common/usb_kbd.c | 4 ++- drivers/usb/host/xhci-ring.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------ drivers/usb/host/xhci.c | 11 ++++---- include/usb/xhci.h | 5 ++-- 5 files changed, 122 insertions(+), 39 deletions(-)
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 86aeaab412..b7b2e16410 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 ebd2954571..7e193786b0 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -464,7 +464,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);
@@ -660,7 +660,7 @@ static int xhci_address_device(struct usb_device *udev, int root_portnr) 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))) { @@ -735,7 +735,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);
@@ -1121,6 +1121,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, @@ -1138,7 +1139,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); }
/** @@ -1158,7 +1159,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 20e4a21066..875ddb2cd5 100644 --- a/include/usb/xhci.h +++ b/include/usb/xhci.h @@ -1255,9 +1255,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);
After the initial configuration some USB keyboard+mouse devices never return any kind of event on the interrupt line. In particular, the device identified by "Cypress Cypress USB Keyboard / PS2 Mouse as /devices/platform/soc/3f980000.usb/usb1/1-1/1-1.3/1-1.3:1.0/0003:04B4:0101.0001/input/input0" never returns a data packet until the first external input event.
I found this was also true with some newer model Dell keyboards.
When the device is plugged into a xhci controller there is also no point in waiting 5 seconds for a device that is never going to present data, so the call to the interrupt service was changed to a nonblocking operation for the controllers that support this.
With the patch applied, the rpi3 and rpi4 work well with the more complex keyboard devices.
Signed-off-by: Jason Wessel jason.wessel@windriver.com --- common/usb_kbd.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/common/usb_kbd.c b/common/usb_kbd.c index b316807844..3c0056e1b9 100644 --- a/common/usb_kbd.c +++ b/common/usb_kbd.c @@ -519,7 +519,9 @@ static int usb_kbd_probe_dev(struct usb_device *dev, unsigned int ifnum) 1, 0, data->new, USB_KBD_BOOT_REPORT_SIZE) < 0) { #else if (usb_int_msg(dev, data->intpipe, data->new, data->intpktsize, - data->intinterval, false) < 0) { + data->intinterval, true) < 0) { + /* Read first packet if the device provides it, else pick it up later */ + return 1; #endif printf("Failed to get keyboard state from device %04x:%04x\n", dev->descriptor.idVendor, dev->descriptor.idProduct);
When resetting the rpi3 board sometimes it will display: USB device not accepting new address (error=0)
After the message appears, the usb keyboard will not work. It seems that the configuration actually did succeed however. Checking the device status for a return code of zero and continuing allows the usb keyboard and other usb devices to work function.
Signed-off-by: Jason Wessel jason.wessel@windriver.com --- common/usb.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/common/usb.c b/common/usb.c index aad13fd9c5..ba83bb960b 100644 --- a/common/usb.c +++ b/common/usb.c @@ -1054,11 +1054,12 @@ static int usb_prepare_device(struct usb_device *dev, int addr, bool do_read, dev->devnum = addr;
err = usb_set_address(dev); /* set address */ - - if (err < 0) { - printf("\n USB device not accepting new address " \ + if (err < 0) + debug("\n usb_set_address return < 0\n"); + if (err < 0 && dev->status != 0) { + printf("\n USB device not accepting new address " \ "(error=%lX)\n", dev->status); - return err; + return err; }
mdelay(10); /* Let the SET_ADDRESS settle */
xhci_trl_tx and xhchi_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.
And pending polling transactions must be aborted before switching modes to avoid corrupting the state of the controller.
Signed-off-by: Jason Wessel jason.wessel@windriver.com --- drivers/usb/host/xhci-ring.c | 86 +++++++++++++++++++++++++++++------- 1 file changed, 70 insertions(+), 16 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index b7b2e16410..1c00f2d496 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -24,6 +24,12 @@
#include <usb/xhci.h>
+static void *last_bulk_tx_buf = NULL; +static struct usb_device *poll_last_udev; +int poll_last_ep_index; +static unsigned long bulk_tx_poll_ts = 0; +static bool poll_pend = false; + /** * Is this TRB a link TRB or was the last TRB the last TRB in this event ring * segment? I.e. would the updated event TRB pointer step off the end of the @@ -549,19 +555,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 +570,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 +713,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 (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(bulk_tx_poll_ts) > XHCI_TIMEOUT || + last_bulk_tx_buf != buffer || poll_last_udev != udev || + ep_index != poll_last_ep_index) { + abort_td(poll_last_udev, poll_last_ep_index); + poll_last_udev->status = USB_ST_NAK_REC; /* closest thing to a timeout */ + poll_last_udev->act_len = 0; + poll_pend = false; + } + } /* No else here because poll_pend might have changed above */ + if (!poll_pend) { + 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 (!poll_pend) { + /* Start the timer */ + bulk_tx_poll_ts = get_timer(0); + poll_last_udev = udev; + poll_last_ep_index = ep_index; + 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; + poll_pend = false; return -ETIMEDOUT; } + 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 +826,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 (poll_pend) { + abort_td(poll_last_udev, poll_last_ep_index); + poll_last_udev->status = USB_ST_NAK_REC; /* closest thing to a timeout */ + poll_last_udev->act_len = 0; + poll_pend = false; + } + ep_index = usb_pipe_ep_index(pipe);
ep_ring = virt_dev->eps[ep_index].ring;
If a "usb reset" is issued when the poll_pend state is set the abort_td() function will hit one of the BUG() statements in abort_td() or the BUG() statement at the end of xhci_wait_for_event().
The controller has been reset, so the rest of the cleanup should be skipped and poll_pend flag should be cleared.
Signed-off-by: Jason Wessel jason.wessel@windriver.com --- drivers/usb/host/xhci-ring.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index 1c00f2d496..ed0dea9fca 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -483,6 +483,8 @@ union xhci_trb *xhci_wait_for_event(struct xhci_ctrl *ctrl, trb_type expected, if (expected == TRB_TRANSFER) return NULL;
+ if (poll_pend) + return NULL; printf("XHCI timeout on event type %d... cannot recover.\n", expected); BUG(); } @@ -505,11 +507,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);
I have found through testing some USB 2 composite mouse/keyboard devices do not response to the usb_set_address call immediately following the port reset. It can take anywhere from 2ms to 20ms.
This patch adds a retry and delay for usb_prepare_device() and allows all the USB keyboards I tried to function properly.
Signed-off-by: Jason Wessel jason.wessel@windriver.com --- common/usb.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/common/usb.c b/common/usb.c index ba83bb960b..fb091440cc 100644 --- a/common/usb.c +++ b/common/usb.c @@ -1054,6 +1054,15 @@ static int usb_prepare_device(struct usb_device *dev, int addr, bool do_read, dev->devnum = addr;
err = usb_set_address(dev); /* set address */ + /* Retry for old composite keyboard/mouse usb2 hardware */ + int i = 0; + while (err < 0 && i <= 40) { + i += 20; + mdelay(20); + err = usb_set_address(dev); /* set address */ + } + if (i > 0) + debug("usb_set_address delay: %i\n", i); if (err < 0) debug("\n usb_set_address return < 0\n"); if (err < 0 && dev->status != 0) {
participants (1)
-
Jason Wessel