[SLOF] [PATCH v1] usb-xhci: add keyboard support
Nikunj A Dadhania
nikunj at linux.vnet.ibm.com
Tue Sep 15 18:07:27 AEST 2015
Alexey Kardashevskiy <aik at ozlabs.ru> writes:
> On 09/15/2015 03:55 PM, Nikunj A Dadhania wrote:
>> 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 :-)
>
>
> Mmm. It is still not clear if this is required for the keyboard support or
> not :)
It is, as the link pointer will not be handled properly.
>
>
>>
>>>
>>>
>>>> + 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.
>
>
> Ok. But it does not seem related to the keyboard support as the subject
> suggests => move it to a separate patch.
Yes, that is fine.
>
>
>
>>>
>>>> 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.
>
> Sure, I am not questioning the code, I am just saying that reading this
> pointers math is not very easy.
:-)
>
>
>>> Is there any good reason why XHCI_TRB_SIZE is: #define XHCI_TRB_SIZE
>>> 16
>>
>> Specification.
>
>
> Then imho it should be:
> link = (struct xhci_link_trb *) ((u8 *)seg->trbs + XHCI_TRB_SIZE*(seg->size
> - 1))
link = (struct xhci_link_trb *) (seg->trbs + seg->size - 1);
Is correct.
>
>>
>>>
>>> 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.
>
> If it was "all", then fine but you say "most".
After reviewing the code, all 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.
>
> I wish I knew more about XHCI to comment more :)
>
>
>
> --
> Alexey
More information about the SLOF
mailing list