[SLOF] [PATCH v1] usb-xhci: add keyboard support
Nikunj A Dadhania
nikunj at linux.vnet.ibm.com
Tue Sep 15 15:55:59 AEST 2015
Alexey Kardashevskiy <aik at ozlabs.ru> writes:
> 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.
Sure, I can do that.
>
>> }
>> 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?
With keyboard addition, the events are generated/queued pretty fast. And
I found that we do not need a LINK pointer in event queue. So you can
call it a bugfix :-)
>
>
>> + 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?
Will document
>
>
>
>> + }
>> 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"?
USB major number, basically saying that these are usb 2.0 devices
>
>> + 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.
Good point, let me check that.
>
>> + 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?
Possible, will have a look in next version
>
>
>> 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?
So on the other thread, Ben mentioned that when we stop the usb xhci
controller, it returns immediately and qemu code is still not done the
complete shutdown. This was to have a grace period. The right fix will
definitely be in QEMU.
>
>> 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.
Again not a bug really, but for keyboard case, the fetching of the trb
is much faster. So by the time i have programmed the link register, qemu
has already picked it up.
So now I am making the link structure ready when i am programming n-1.
For example if I have 16 transfer buffers(trb), 16th trb should be the link to
1st trb. Now when I am at 15th trb, I will make 16th trb a link trb. So
its is ready for next fetch.
> Is there any good reason why XHCI_TRB_SIZE is: #define XHCI_TRB_SIZE
> 16
Specification.
>
> 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.
Can send as a separate fix.
>
>> }
>> } 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?
Why ULL? most of them are being used in 32bit assignments.
Though I can change it to:
#define BIT(x) (1 << (x))
>
>
>> + 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().
So you are basically pointing to the trust part, it already checked so
no need to check within *_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()? :)
Sure
>
>
>> +
>> + 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()?
Sure
>
>
>> + 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 */
>>
Thanks for the review.
Regards
Nikunj
More information about the SLOF
mailing list