[SLOF] [PATCH v1] usb-xhci: add keyboard support
Alexey Kardashevskiy
aik at ozlabs.ru
Tue Sep 15 16:09:29 AEST 2015
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 :)
>
>>
>>
>>> + 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.
>>
>>> 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))
>
>>
>> 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".
> 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