[SLOF] [PATCH v1] usb-xhci: add keyboard support
Alexey Kardashevskiy
aik at ozlabs.ru
Mon Sep 14 21:23:28 AEST 2015
On 09/14/2015 08:07 PM, Nikunj A Dadhania wrote:
> Current implementation enumerated only the USB3 ports and supported USB
> MSC device.
>
> QEMU USB keyboard attached to a xhci controller appears as high speed
> device.
> * Add support for scanning usb2 ports.
> * Add support for xhci interrupt pipes needed for keyboard handling
> * Improve usb key fetching
> * Introduce delay during xhci exit path as qemu can still use it for
> a while
>
> Signed-off-by: Nikunj A Dadhania <nikunj at linux.vnet.ibm.com>
> ---
> lib/libusb/usb-hid.c | 11 ++--
> lib/libusb/usb-xhci.c | 174 +++++++++++++++++++++++++++++++++++++++++++++++---
> lib/libusb/usb-xhci.h | 5 ++
> 3 files changed, 173 insertions(+), 17 deletions(-)
>
> diff --git a/lib/libusb/usb-hid.c b/lib/libusb/usb-hid.c
> index f0cab8a..0dffd0c 100644
> --- a/lib/libusb/usb-hid.c
> +++ b/lib/libusb/usb-hid.c
> @@ -443,11 +443,8 @@ unsigned char usb_key_available(void *dev)
>
> unsigned char usb_read_keyb(void *vdev)
> {
> - if (!vdev)
> - return false;
> -
> - while (usb_poll_key(vdev)) {
> - /* loop for all pending keys */
> - }
> - return read_key();
> + if (usb_key_available(vdev))
> + return read_key();
> + else
> + return 0;
Is this the only part for "Improve usb key fetching"? If so, make it a
separate patch please.
> }
> diff --git a/lib/libusb/usb-xhci.c b/lib/libusb/usb-xhci.c
> index 0c3d6e4..040b047 100644
> --- a/lib/libusb/usb-xhci.c
> +++ b/lib/libusb/usb-xhci.c
> @@ -388,10 +388,12 @@ static void xhci_init_seg(struct xhci_seg *seg, uint32_t size, uint32_t type)
> seg->deq = (uint64_t)seg->trbs;
> memset((void *)seg->trbs, 0, size);
>
> - link =(struct xhci_link_trb *) (seg->trbs + seg->size - 1);
> - link->addr = cpu_to_le64(seg->trbs_dma);
> - link->field2 = 0;
> - link->field3 = cpu_to_le32(0x1 | TRB_CMD_TYPE(TRB_LINK));
> + if (type != TYPE_EVENT) {
This "if" looks like a bugfix for something, what is it?
> + link =(struct xhci_link_trb *) (seg->trbs + seg->size - 1);
> + link->addr = cpu_to_le64(seg->trbs_dma);
> + link->field2 = 0;
> + link->field3 = cpu_to_le32(0x1 | TRB_CMD_TYPE(TRB_LINK));
What is "0x1" for here?
> + }
> return;
> }
>
> @@ -616,6 +618,7 @@ static void xhci_free_dev(struct xhci_dev *xdev)
> {
> xhci_free_seg(&xdev->bulk_in, XHCI_DATA_TRBS_SIZE);
> xhci_free_seg(&xdev->bulk_out, XHCI_DATA_TRBS_SIZE);
> + xhci_free_seg(&xdev->intr, XHCI_INTR_TRBS_SIZE);
> xhci_free_seg(&xdev->control, XHCI_CONTROL_TRBS_SIZE);
> xhci_free_ctx(&xdev->in_ctx, XHCI_CTX_BUF_SIZE);
> xhci_free_ctx(&xdev->out_ctx, XHCI_CTX_BUF_SIZE);
> @@ -697,6 +700,51 @@ static int xhci_hub_check_ports(struct xhci_hcd *xhcd)
> }
> }
> }
> +
> + /* Check USB2 ports */
> + /* Read the xHCI extented capability to find usb2 ports and offset*/
> + xecp_off = XHCI_HCCPARAMS_XECP(read_reg32(&cap->hccparams));
> + base = (uint32_t *)cap;
> + while (xecp_off > 0) {
> + xecp_addr = base + xecp_off;
> + dprintf("xecp_off %d %p %p \n", xecp_off, base, xecp_addr);
> +
> + if (XHCI_XECP_CAP_ID(read_reg32(xecp_addr)) == XHCI_XECP_CAP_SP &&
> + XHCI_XECP_CAP_SP_MJ(read_reg32(xecp_addr)) == 2 &&
> + XHCI_XECP_CAP_SP_MN(read_reg32(xecp_addr)) == 0) {
What are these "2" and "0"?
> + port_cnt = XHCI_XECP_CAP_SP_PC(read_reg32(xecp_addr + 2));
> + port_off = XHCI_XECP_CAP_SP_PO(read_reg32(xecp_addr + 2));
> + dprintf("PortCount %d Portoffset %d\n", port_cnt, port_off);
> + }
> + base = xecp_addr;
> + xecp_off = XHCI_XECP_NEXT_PTR(read_reg32(xecp_addr));
> + }
> + if (port_off == 0) /* port_off should always start from 1 */
Do not you want to reset port_off to 1 before checking for USB2 ports? It
might be not "1" after USB3 ports scan.
> + return false;
> + for (i = (port_off - 1); i < (port_off + port_cnt - 1); i++) {
> + prs = &op->prs[i];
> + portsc = read_reg32(&prs->portsc);
> + print_port_status(prs);
> + if ((portsc & PORTSC_CCS) && (portsc & PORTSC_CSC)) {
> + /* Device present and disabled */
> + dprintf("Device present on port %d, disabled\n", i);
> + /* Reset the port */
> + portsc = read_reg32(&prs->portsc);
> + portsc = portsc | PORTSC_PR;
> + write_reg32(&prs->portsc, portsc);
> + /* FIXME poll for port event */
> + SLOF_msleep(20);
> + xhci_poll_event(xhcd, 0);
> + portsc = read_reg32(&prs->portsc);
> + if (portsc & ~PORTSC_PRC) {
> + dprintf("Port reset complete %d\n", i);
> + }
> + print_port_status(prs);
> + if (!usb3_dev_init(xhcd, (i - (port_off - 1)))) {
> + dprintf("USB device initialization failed\n");
> + }
> + }
> + }
This looks very much like cut-n-paste. Can this go to a helper?
> dprintf("exit\n");
> return true;
> }
> @@ -868,6 +916,8 @@ static bool xhci_hcd_exit(struct xhci_hcd *xhcd)
> SLOF_dma_map_out(xhcd->dcbaap_dma, (void *)xhcd->dcbaap, XHCI_DCBAAP_MAX_SIZE);
> SLOF_dma_free((void *)xhcd->dcbaap, XHCI_DCBAAP_MAX_SIZE);
> }
> + /* Wait for a while till qemu stops using this device */
> + SLOF_msleep(50);
What does "using" mean here? At this point you release all buffers, etc,
any "use" would kill the guest, no?
> return true;
> }
>
> @@ -1079,18 +1129,16 @@ static inline struct xhci_seg *xhci_pipe_get_seg(struct usb_pipe *pipe)
> static inline void *xhci_get_trb(struct xhci_seg *seg)
> {
> uint64_t val, enq;
> - uint32_t size;
> struct xhci_link_trb *link;
> + int index;
>
> enq = val = seg->enq;
> val = val + XHCI_TRB_SIZE;
> - size = seg->size * XHCI_TRB_SIZE;
> + index = (enq - (uint64_t)seg->trbs) / XHCI_TRB_SIZE + 1;
> + dprintf("%s: enq %llx, val %llx %x\n", __func__, enq, val, index);
> /* TRBs being a cyclic buffer, here we cycle back to beginning. */
> - if ((val % size) == 0) {
> + if (index == (seg->size - 1)) {
> seg->enq = (uint64_t)seg->trbs;
> - enq = seg->enq;
> - seg->enq = seg->enq + XHCI_TRB_SIZE;
> - val = 0;
> seg->cycle_state ^= seg->cycle_state;
> link = (struct xhci_link_trb *) (seg->trbs + seg->size - 1);
Ufff. Tricky. I thought there is a bug but it seems there is none. Is there
any good reason why XHCI_TRB_SIZE is:
#define XHCI_TRB_SIZE 16
and not:
#define XHCI_TRB_SIZE sizeof(xhci_trb)
?
> link->addr = cpu_to_le64(seg->trbs_dma);
> @@ -1215,6 +1263,7 @@ static void xhci_init_bulk_ep(struct usb_dev *dev, struct usb_pipe *pipe)
> if (!seg->trbs) {
> if (!xhci_alloc_seg(seg, XHCI_DATA_TRBS_SIZE, TYPE_BULK)) {
> dprintf("Failed allocating seg\n");
> + return;
Feels like an unrelated bugfix.
> }
> } else {
> xhci_init_seg(seg, XHCI_DATA_TRBS_SIZE, TYPE_BULK);
> @@ -1235,6 +1284,64 @@ static void xhci_init_bulk_ep(struct usb_dev *dev, struct usb_pipe *pipe)
> xpipe->seg = seg;
> }
>
> +static int xhci_get_pipe_intr(struct usb_pipe *pipe,
> + struct xhci_hcd *xhcd,
> + char *buf, size_t len)
> +{
> + struct xhci_dev *xdev;
> + struct xhci_seg *seg;
> + struct xhci_pipe *xpipe;
> + struct xhci_control_ctx *ctrl;
> + struct xhci_ep_ctx *ep;
> + uint32_t x_epno, val, type;
> + struct usb_dev *dev;
> + struct xhci_transfer_trb *trb;
> +
> + if (!pipe || !pipe->dev || !xhcd)
> + return false;
All three cannot be NULL here.
> +
> + dev = pipe->dev;
> + if (dev->class != DEV_HID_KEYB)
> + return false;
> +
> + xdev = dev->priv;
> + pipe->mps = 8;
> + seg = xhci_pipe_get_seg(pipe);
> + xpipe = xhci_pipe_get_xpipe(pipe);
> + type = EP_INT_IN;
> + seg = &xdev->intr;
> +
> + if (!seg->trbs) {
> + if (!xhci_alloc_seg(seg, XHCI_INTR_TRBS_SIZE, TYPE_BULK)) {
> + dprintf("Failed allocating seg\n");
> + return false;
> + }
> + } else {
> + xhci_init_seg(seg, XHCI_EVENT_TRBS_SIZE, TYPE_BULK);
> + }
> +
> + xpipe->buf = buf;
> + xpipe->buf_phys = SLOF_dma_map_in(buf, len, false);
> + xpipe->buflen = len;
> +
> + ctrl = xhci_get_control_ctx(&xdev->in_ctx);
> + x_epno = xhci_get_epno(pipe);
> + ep = xhci_get_ep_ctx(&xdev->in_ctx, xdev->ctx_size, x_epno);
> + val = EP_TYPE(type) | MAX_BURST(0) | ERROR_COUNT(3) |
> + MAX_PACKET_SIZE(pipe->mps);
> + ep->field2 = cpu_to_le32(val);
> + ep->deq_addr = cpu_to_le64(seg->trbs_dma | seg->cycle_state);
> + ep->field4 = cpu_to_le32(8);
> + ctrl->a_flags = cpu_to_le32(BIT(x_epno) | 0x1);
BIT() is:
#define BIT(x) (1 << x)
while it should be:
#define BIT(x) (1 << (x))
or even
#define BIT(x) (1ULL << (x))
Are you sure we do not want 1ULL in the chunk above?
> + ctrl->d_flags = 0;
> + xhci_configure_ep(xhcd, xdev->slot_id, xdev->in_ctx.dma_addr);
> + xpipe->seg = seg;
> +
> + trb = xhci_get_trb(seg);
> + fill_normal_trb(trb, (void *)xpipe->buf_phys, pipe->mps);
> + return true;
> + }
> +
> static struct usb_pipe* xhci_get_pipe(struct usb_dev *dev, struct usb_ep_descr *ep, char *buf, size_t len)
> {
> struct xhci_hcd *xhcd;
> @@ -1264,6 +1371,10 @@ static struct usb_pipe* xhci_get_pipe(struct usb_dev *dev, struct usb_ep_descr *
> new->dir = (ep->bEndpointAddress & 0x80) >> 7;
> new->epno = ep->bEndpointAddress & 0x0f;
>
> + if (new->type == USB_EP_TYPE_INTR)
> + if (!xhci_get_pipe_intr(new, xhcd, buf, len))
You are checking new!=NULL inside xhci_get_pipe_intr() but it is too late -
if it is NULL, "new->type" will fail before calling xhci_get_pipe_intr().
> + dprintf("usb-xhci: %s alloc_intr failed %p\n",
> + __func__, new);
> if (new->type == USB_EP_TYPE_BULK)
> xhci_init_bulk_ep(dev, new);
>
> @@ -1298,6 +1409,48 @@ static void xhci_put_pipe(struct usb_pipe *pipe)
> dprintf("usb-xhci: %s exit\n", __func__);
> }
>
> +static int xhci_poll_intr(struct usb_pipe *pipe, uint8_t *data)
> +{
> + struct xhci_transfer_trb *trb;
> + struct xhci_seg *seg;
> + struct xhci_pipe *xpipe;
> + struct xhci_dev *xdev;
> + struct xhci_hcd *xhcd;
> + struct xhci_db_regs *dbr;
> + uint32_t x_epno;
> + uint8_t *buf, ret = 1;
> +
> + if (!pipe || !pipe->dev || !pipe->dev->hcidev)
> + return 0;
> + xdev = pipe->dev->priv;
> + xhcd = (struct xhci_hcd *)pipe->dev->hcidev->priv;
> + x_epno = xhci_get_epno(pipe);
> + seg = xhci_pipe_get_seg(pipe);
> + xpipe = xhci_pipe_get_xpipe(pipe);
> +
> + buf = xpipe->buf;
> + for (int i = 0; i < 8; i++)
> + buf[i] = 0;
memset()? :)
> +
> + mb();
> + /* Ring the doorbell - x_epno */
> + dbr = xhcd->db_regs;
> + write_reg32(&dbr->db[xdev->slot_id], x_epno);
> + if (!xhci_poll_event(xhcd, 0)) {
> + printf("poll intr failed\n");
> + return 0;
> + }
> + mb();
> +
> + for (int i = 0; i < 8; i++)
> + data[i] = *(buf + i);
memcpy()?
> + trb = xhci_get_trb(seg);
> + fill_normal_trb(trb, (void *)xpipe->buf_phys, pipe->mps);
> + mb();
> + return ret;
> +}
> +
> +
extra empty line.
> struct usb_hcd_ops xhci_ops = {
> .name = "xhci-hcd",
> .init = xhci_init,
> @@ -1305,6 +1458,7 @@ struct usb_hcd_ops xhci_ops = {
> .usb_type = USB_XHCI,
> .get_pipe = xhci_get_pipe,
> .put_pipe = xhci_put_pipe,
> + .poll_intr = xhci_poll_intr,
> .send_ctrl = xhci_send_ctrl,
> .transfer_bulk = xhci_transfer_bulk,
> .next = NULL,
> diff --git a/lib/libusb/usb-xhci.h b/lib/libusb/usb-xhci.h
> index faeb07e..cc6a197 100644
> --- a/lib/libusb/usb-xhci.h
> +++ b/lib/libusb/usb-xhci.h
> @@ -264,6 +264,7 @@ struct xhci_seg {
>
> #define XHCI_TRB_SIZE 16
> #define XHCI_EVENT_TRBS_SIZE 4096
> +#define XHCI_INTR_TRBS_SIZE 4096
> #define XHCI_CONTROL_TRBS_SIZE 4096
> #define XHCI_DATA_TRBS_SIZE 4096
> #define XHCI_ERST_NUM_SEGS 1
> @@ -349,6 +350,7 @@ struct xhci_dev {
> struct xhci_ctx in_ctx;
> struct xhci_ctx out_ctx;
> struct xhci_seg control;
> + struct xhci_seg intr;
> struct xhci_seg bulk_in;
> struct xhci_seg bulk_out;
> uint32_t ctx_size;
> @@ -381,6 +383,9 @@ struct xhci_hcd {
> struct xhci_pipe {
> struct usb_pipe pipe;
> struct xhci_seg *seg;
> + void *buf;
> + long buf_phys;
> + uint32_t buflen;
> };
>
> #endif /* USB_XHCI_H */
>
--
Alexey
More information about the SLOF
mailing list