Re: [PATCH v3 2/3] usb: xhci: Add DbC support in xHCI driver
From: Lu Baolu
Date: Tue Oct 24 2017 - 22:15:19 EST
Hi Mathias,
Thanks for your time.
On 10/25/2017 01:03 AM, Mathias Nyman wrote:
> Hi
>
> Skipping lists, partial review.
> Many of the questions are real questions that I was wondering about
> while lookinga the code. Not necessarily thing that must be changed.
No problem.
>
> On 05.09.2017 04:58, Lu Baolu wrote:
>> xHCI compatible USB host controllers(i.e. super-speed USB3 controllers)
>> can be implemented with the Debug Capability(DbC). It presents a debug
>> device which is fully compliant with the USB framework and provides the
>> equivalent of a very high performance full-duplex serial link. The debug
>> capability operation model and registers interface are defined in 7.6.8
>> of the xHCI specification, revision 1.1.
>>
>> The DbC debug device shares a root port with the xHCI host. By default,
>> the debug capability is disabled and the root port is assigned to xHCI.
>> When the DbC is enabled, the root port will be assigned to the DbC debug
>> device, and the xHCI sees nothing on this port. This implementation uses
>> a sysfs node named <dbc> under the xHCI device to manage the enabling
>> and disabling of the debug capability.
>>
>> When the debug capability is enabled, it will present a debug device
>> through the debug port. This debug device is fully compliant with the
>> USB3 framework, and it can be enumerated by a debug host on the other
>> end of the USB link. As soon as the debug device is configured, a TTY
>> serial device named /dev/ttyDBC0 will be created.
>>
>> One use of this link is running a login service on the debug target.
>> Hence it can be remote accessed by a debug host. Another use case can
>> probably be found in servers. It provides a peer-to-peer USB link
>> between two host-only machines. This provides a reasonable out-of-band
>> communication method between two servers.
>>
>> Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
>> ---
>> .../ABI/testing/sysfs-bus-pci-drivers-xhci_hcd | 25 +
>> drivers/usb/host/Kconfig | 9 +
>> drivers/usb/host/Makefile | 5 +
>> drivers/usb/host/xhci-dbgcap.c | 1016 ++++++++++++++++++++
>> drivers/usb/host/xhci-dbgcap.h | 247 +++++
>> drivers/usb/host/xhci-dbgtty.c | 586 +++++++++++
>> drivers/usb/host/xhci-trace.h | 60 ++
>> drivers/usb/host/xhci.c | 10 +
>> drivers/usb/host/xhci.h | 1 +
>> 9 files changed, 1959 insertions(+)
>> create mode 100644 Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
>> create mode 100644 drivers/usb/host/xhci-dbgcap.c
>> create mode 100644 drivers/usb/host/xhci-dbgcap.h
>> create mode 100644 drivers/usb/host/xhci-dbgtty.c
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd b/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
>> new file mode 100644
>> index 0000000..0088aba
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
>> @@ -0,0 +1,25 @@
>> +What: /sys/bus/pci/drivers/xhci_hcd/.../dbc
>> +Date: June 2017
>> +Contact: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
>> +Description:
>> + xHCI compatible USB host controllers (i.e. super-speed
>> + USB3 controllers) are often implemented with the Debug
>> + Capability (DbC). It can present a debug device which
>> + is fully compliant with the USB framework and provides
>> + the equivalent of a very high performance full-duplex
>> + serial link for debug purpose.
>> +
>> + The DbC debug device shares a root port with xHCI host.
>> + When the DbC is enabled, the root port will be assigned
>> + to the Debug Capability. Otherwise, it will be assigned
>> + to xHCI.
>> +
>> + Writing "enable" to this attribute will enable the DbC
>> + functionality and the shared root port will be assigned
>> + to the DbC device. Writing "disable" to this attribute
>> + will disable the DbC functionality and the shared root
>> + port will roll back to the xHCI.
>> +
>> + Reading this attribute gives the state of the DbC. It
>> + can be one of the following states: disabled, enabled,
>> + initialized, connected, configured and stalled.
>> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
>> index fa5692d..968a196 100644
>> --- a/drivers/usb/host/Kconfig
>> +++ b/drivers/usb/host/Kconfig
>> @@ -27,6 +27,15 @@ config USB_XHCI_HCD
>> module will be called xhci-hcd.
>>
>> if USB_XHCI_HCD
>> +config USB_XHCI_DBGCAP
>> + bool "xHCI support for debug capability"
>> + depends on TTY
>> + select USB_U_SERIAL
>> + ---help---
>> + Say 'Y' to enable the support for the xHCI debug capability. Make
>> + sure that your xHCI host supports the extended debug capability and
>> + you want a TTY serial device based on the xHCI debug capability
>> + before enabling this option. If unsure, say 'N'.
>>
>> config USB_XHCI_PCI
>> tristate
>> diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
>> index cf2691f..175c80a 100644
>> --- a/drivers/usb/host/Makefile
>> +++ b/drivers/usb/host/Makefile
>> @@ -13,6 +13,11 @@ fhci-$(CONFIG_FHCI_DEBUG) += fhci-dbg.o
>> xhci-hcd-y := xhci.o xhci-mem.o
>> xhci-hcd-y += xhci-ring.o xhci-hub.o xhci-dbg.o
>> xhci-hcd-y += xhci-trace.o
>> +
>> +ifneq ($(CONFIG_USB_XHCI_DBGCAP), )
>> + xhci-hcd-y += xhci-dbgcap.o xhci-dbgtty.o
>> +endif
>> +
>> ifneq ($(CONFIG_USB_XHCI_MTK), )
>> xhci-hcd-y += xhci-mtk-sch.o
>> endif
>> diff --git a/drivers/usb/host/xhci-dbgcap.c b/drivers/usb/host/xhci-dbgcap.c
>> new file mode 100644
>> index 0000000..9816085
>> --- /dev/null
>> +++ b/drivers/usb/host/xhci-dbgcap.c
>> @@ -0,0 +1,1016 @@
>> +/**
>> + * xhci-dbgcap.c - xHCI debug capability support
>> + *
>> + * Copyright (C) 2017 Intel Corporation
>> + *
>> + * Author: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +#include <linux/dma-mapping.h>
>> +#include <linux/slab.h>
>> +
>> +#include "xhci.h"
>> +#include "xhci-trace.h"
>> +#include "xhci-dbgcap.h"
>> +
>> +static inline void *
>> +dbc_dma_alloc_coherent(struct xhci_hcd *xhci, size_t size,
>> + dma_addr_t *dma_handle, gfp_t flags)
>> +{
>> + void *vaddr;
>> +
>> + vaddr = dma_alloc_coherent(xhci_to_hcd(xhci)->self.sysdev,
>> + size, dma_handle, flags);
>> + memset(vaddr, 0, size);
>> + return vaddr;
>> +}
>> +
>> +static inline void
>> +dbc_dma_free_coherent(struct xhci_hcd *xhci, size_t size,
>> + void *cpu_addr, dma_addr_t dma_handle)
>> +{
>> + if (cpu_addr)
>> + dma_free_coherent(xhci_to_hcd(xhci)->self.sysdev,
>> + size, cpu_addr, dma_handle);
>> +}
>> +
>
> Is there any benefit in having these dbc wrapped variants
> of dma alloc/free coherent functions?
> Can't dma_zalloc_coherent() and dma_free_coherent() be used directly?
The benefit of this wrapper is to zero out the dma space during allocation.
>
>> +static inline void xhci_put_utf16(u16 *s, const char *c)
>> +{
>> + int i;
>> + size_t size;
>> +
>> + size = strlen(c);
>> + for (i = 0; i < size; i++)
>> + s[i] = cpu_to_le16(c[i]);
>> +}
>> +
>
> could utf8s_to_utf16s() be used instead?
> At least drivers/usb/misc/usb251xb.c is using it to set some string descriptors.
Good suggestion. I will try this in code.
>
>> +static u32 xhci_dbc_populate_strings(struct dbc_strings *strings)
>> +{
>> + struct usb_string_descriptor *s_desc;
>> + u32 string_length;
>> +
>> + /* Serial string: */
>> + s_desc = (struct usb_string_descriptor *)strings->serial;
>
> Took me a second to understand that dbc_strings is not just strings, but char
> arrays where we store entire string descriptors with length, type and UTF16LE string.
>
> small thing, but maybe it structure could be called struct dbc_str_descs or similar
Yes, dbc_str_descs is easier for understanding.
>
>> + xhci_put_utf16(s_desc->wData, DBC_STRING_SERIAL);
>> +
>> + s_desc->bLength = (strlen(DBC_STRING_SERIAL) + 1) * 2;
>> + s_desc->bDescriptorType = USB_DT_STRING;
>> + string_length = s_desc->bLength;
>> + string_length <<= 8;
>> +
>> + /* Product string: */
>> + s_desc = (struct usb_string_descriptor *)strings->product;
>> + xhci_put_utf16(s_desc->wData, DBC_STRING_PRODUCT);
>> +
>> + s_desc->bLength = (strlen(DBC_STRING_PRODUCT) + 1) * 2;
>> + s_desc->bDescriptorType = USB_DT_STRING;
>> + string_length += s_desc->bLength;
>> + string_length <<= 8;
>> +
>> + /* Manufacture string: */
>> + s_desc = (struct usb_string_descriptor *)strings->manufacturer;
>> + xhci_put_utf16(s_desc->wData, DBC_STRING_MANUFACTURER);
>> +
>> + s_desc->bLength = (strlen(DBC_STRING_MANUFACTURER) + 1) * 2;
>> + s_desc->bDescriptorType = USB_DT_STRING;
>> + string_length += s_desc->bLength;
>> + string_length <<= 8;
>> +
>> + /* String0: */
>> + strings->string0[0] = 4;
>> + strings->string0[1] = USB_DT_STRING;
>> + strings->string0[2] = 0x09;
>> + strings->string0[3] = 0x04;
>> + string_length += 4;
>> +
>> + return string_length;
>> +}
>> +
>> +static void xhci_dbc_populate_contexts(struct xhci_hcd *xhci, u32 string_length)
>> +{
>> + struct xhci_dbc *dbc;
>> + struct dbc_info_context *info;
>> + struct xhci_ep_ctx *ep_ctx;
>> + u32 dev_info;
>> + dma_addr_t deq, dma;
>> + unsigned int max_burst;
>> +
>> + dbc = xhci->dbc;
>> + if (!dbc)
>> + return;
>> +
>> + /* Populate info Context: */
>> + info = (struct dbc_info_context *)dbc->ctx->bytes;
>> + dma = dbc->string_dma;
>> + info->string0 = cpu_to_le64(dma);
>> + info->manufacturer = cpu_to_le64(dma + DBC_MAX_STRING_LENGTH);
>> + info->product = cpu_to_le64(dma + DBC_MAX_STRING_LENGTH * 2);
>> + info->serial = cpu_to_le64(dma + DBC_MAX_STRING_LENGTH * 3);
>> + info->length = cpu_to_le32(string_length);
>> +
>> + /* Populate bulk out endpoint context: */
>> + ep_ctx = dbc_bulkout_ctx(dbc);
>> + max_burst = DBC_CTRL_MAXBURST(readl(&dbc->regs->control));
>> + deq = dbc_bulkout_enq(dbc);
>> + ep_ctx->ep_info = 0;
>> + ep_ctx->ep_info2 = dbc_epctx_info2(BULK_OUT_EP, 1024, max_burst);
>> + ep_ctx->deq = cpu_to_le64(deq | dbc->ring_out->cycle_state);
>> +
>> + /* Populate bulk in endpoint context: */
>> + ep_ctx = dbc_bulkin_ctx(dbc);
>> + deq = dbc_bulkin_enq(dbc);
>> + ep_ctx->ep_info = 0;
>> + ep_ctx->ep_info2 = dbc_epctx_info2(BULK_IN_EP, 1024, max_burst);
>> + ep_ctx->deq = cpu_to_le64(deq | dbc->ring_in->cycle_state);
>
> Are we setting the ep_ctx->deq to ring deq instead of
> segment start because we are going to call this runtime/mid tranfer?
Ah, that's my fault. I should set it to the segment start although they
have the same values.
This function can't be called during DbC runtime. DbC must be stopped
and reinitialized before re-enabling it.
>
>> +
>> + /* Set DbC context and info registers: */
>> + xhci_write_64(xhci, dbc->ctx->dma, &dbc->regs->dccp);
>> +
>> + dev_info = cpu_to_le32((DBC_VENDOR_ID << 16) | DBC_PROTOCOL);
>> + writel(dev_info, &dbc->regs->devinfo1);
>> +
>> + dev_info = cpu_to_le32((DBC_DEVICE_REV << 16) | DBC_PRODUCT_ID);
>> + writel(dev_info, &dbc->regs->devinfo2);
>
> Minor thing again but this function initializes a couple registers in addition
> to just populating the contexts. Maybe split or rename function
Okay, will clean it.
>
>> +}
>> +
>> +static void xhci_dbc_giveback(struct dbc_request *req, int status)
>> + __releases(&dbc->lock)
>> + __acquires(&dbc->lock)
>> +{
>> + struct dbc_ep *dep = req->dep;
>> + struct xhci_dbc *dbc = dep->dbc;
>> + struct xhci_hcd *xhci = dbc->xhci;
>> + struct device *dev = xhci_to_hcd(dbc->xhci)->self.sysdev;
>> +
>> + trace_xhci_dbc_giveback_request(req);
>> +
>> + list_del_init(&req->list_pending);
>
> why do we need to reinitialize the entry, is this entry reused
> or accessed after later afetr this. would list_del() work?
The requests are preallocated in a pool, so they can be reused.
>
>> + req->trb_dma = 0;
>> + req->trb = NULL;
>> +
>> + if (req->status == -EINPROGRESS)
>> + req->status = status;
>> +
>> + dma_unmap_single(dev,
>> + req->dma,
>> + req->length,
>> + dbc_ep_dma_direction(dep));
>> +
>> + /* Give back the transfer request: */
>> + spin_unlock(&dbc->lock);
>> + req->complete(xhci, req);
>> + spin_lock(&dbc->lock);
>> +}
>> +
>> +static void xhci_dbc_flush_single_request(struct dbc_request *req)
>> +{
>> + union xhci_trb *trb = req->trb;
>> +
>> + trb->generic.field[0] = 0;
>> + trb->generic.field[1] = 0;
>> + trb->generic.field[2] = 0;
>> + trb->generic.field[3] &= cpu_to_le32(TRB_CYCLE);
>> + trb->generic.field[3] |= cpu_to_le32(TRB_TYPE(TRB_TR_NOOP));
>> +
>> + xhci_dbc_giveback(req, -ESHUTDOWN);
>> +}
>> +
>> +static void xhci_dbc_flush_endpoint_requests(struct dbc_ep *dep)
>> +{
>> + struct dbc_request *req, *tmp;
>> +
>> + list_for_each_entry_safe(req, tmp, &dep->list_pending, list_pending)
>> + xhci_dbc_flush_single_request(req);
>> +}
>> +
>> +static void xhci_dbc_flush_reqests(struct xhci_dbc *dbc)
>> +{
>> + int i;
>> + struct dbc_ep *dep;
>> +
>> + for (i = BULK_OUT; i <= BULK_IN; i++) {
>> + dep = &dbc->eps[i];
>> + xhci_dbc_flush_endpoint_requests(dep);
>> + }
>
> we only have one in and one out endpiont, how about just
> xhci_dbc_flush_endpoint_requests(&dbc->eps[BULK_OUT]);
> xhci_dbc_flush_endpoint_requests(&dbc->eps[BULK_IN]);
Yes, will clean it.
>
>> +}
>> +
>> +static struct dbc_request *
>> +dbc_ep_alloc_request(struct dbc_ep *dep, gfp_t gfp_flags)
>> +{
>> + struct dbc_request *req;
>> +
>> + req = kzalloc(sizeof(*req), gfp_flags);
>> + if (!req)
>> + return NULL;
>> +
>> + req->dep = dep;
>> + INIT_LIST_HEAD(&req->list_pending);
>> + INIT_LIST_HEAD(&req->list_pool);
>> +
>> + trace_xhci_dbc_alloc_request(req);
>> +
>> + return req;
>> +}
>> +
>> +static void
>> +dbc_ep_free_request(struct dbc_ep *dep, struct dbc_request *req)
>> +{
>> + trace_xhci_dbc_free_request(req);
>> +
>> + kfree(req);
>> +}
>> +
>> +static void
>> +xhci_dbc_queue_trb(struct xhci_ring *ring, u32 field1,
>> + u32 field2, u32 field3, u32 field4)
>> +{
>> + union xhci_trb *trb, *next;
>> +
>> + trb = ring->enqueue;
>> + trb->generic.field[0] = cpu_to_le32(field1);
>> + trb->generic.field[1] = cpu_to_le32(field2);
>> + trb->generic.field[2] = cpu_to_le32(field3);
>> + trb->generic.field[3] = cpu_to_le32(field4);
>> +
>> + trace_xhci_dbc_gadget_ep_queue(ring, &trb->generic);
>> +
>> + ring->num_trbs_free--;
>> + next = ++(ring->enqueue);
>> + if (TRB_TYPE_LINK_LE32(next->link.control)) {
>> + next->link.control ^= cpu_to_le32(TRB_CYCLE);
>> + ring->enqueue = ring->enq_seg->trbs;
>> + ring->cycle_state ^= 1;
>> + }
>> +}
>> +
>> +static int xhci_dbc_queue_bulk_tx(struct dbc_ep *dep,
>> + struct dbc_request *req)
>> +{
>> + u64 addr;
>> + union xhci_trb *trb;
>> + unsigned int num_trbs;
>> + struct xhci_dbc *dbc = dep->dbc;
>> + struct xhci_ring *ring = dep->ring;
>> + u32 length, control, cycle;
>> +
>> + num_trbs = count_trbs(req->dma, req->length);
>> + WARN_ON(num_trbs != 1);
>> + if (ring->num_trbs_free < num_trbs)
>> + return -EBUSY;
>> +
>> + addr = req->dma;
>> + trb = ring->enqueue;
>> + cycle = ring->cycle_state;
>> + length = TRB_LEN(req->length);
>> + control = TRB_TYPE(TRB_NORMAL) | TRB_IOC;
>> +
>> + if (cycle)
>> + control &= cpu_to_le32(~TRB_CYCLE);
>> + else
>> + control |= cpu_to_le32(TRB_CYCLE);
>> +
>> + req->trb = ring->enqueue;
>> + req->trb_dma = xhci_trb_virt_to_dma(ring->enq_seg, ring->enqueue);
>> + xhci_dbc_queue_trb(ring,
>> + lower_32_bits(addr),
>> + upper_32_bits(addr),
>> + length, control);
>> +
>> + /*
>> + * Add a barrier between writes of trb fields and flipping
>> + * the cycle bit:
>> + */
>> + wmb();
>> +
>> + if (cycle)
>> + trb->generic.field[3] |= cpu_to_le32(TRB_CYCLE);
>> + else
>> + trb->generic.field[3] &= cpu_to_le32(~TRB_CYCLE);
>> +
>> + writel(DBC_DOOR_BELL_TARGET(dep->direction), &dbc->regs->doorbell);
>> +
>> + return 0;
>> +}
>> +
>> +static int
>> +dbc_ep_do_queue(struct dbc_ep *dep, struct dbc_request *req)
>> +{
>> + int ret;
>> + struct device *dev;
>> + struct xhci_dbc *dbc = dep->dbc;
>> + struct xhci_hcd *xhci = dbc->xhci;
>> +
>> + dev = xhci_to_hcd(xhci)->self.sysdev;
>> +
>> + if (!req->length || !req->buf)
>> + return -EINVAL;
>> +
>> + req->actual = 0;
>> + req->status = -EINPROGRESS;
>> +
>> + req->dma = dma_map_single(dev,
>> + req->buf,
>> + req->length,
>> + dbc_ep_dma_direction(dep));
>> + if (dma_mapping_error(dev, req->dma)) {
>> + xhci_err(xhci, "failed to map buffer\n");
>> + return -EFAULT;
>> + }
>> +
>> + ret = xhci_dbc_queue_bulk_tx(dep, req);
>> + if (ret) {
>> + xhci_err(xhci, "failed to queue trbs\n");
>> + dma_unmap_single(dev,
>> + req->dma,
>> + req->length,
>> + dbc_ep_dma_direction(dep));
>> + return -EFAULT;
>> + }
>> +
>> + list_add_tail(&req->list_pending, &dep->list_pending);
>> +
>> + return 0;
>> +}
>> +
>> +static int dbc_ep_queue(struct dbc_ep *dep,
>> + struct dbc_request *req,
>> + gfp_t gfp_flags)
>> +{
>> + struct xhci_dbc *dbc = dep->dbc;
>> + int ret = -ESHUTDOWN;
>> +
>> + spin_lock(&dbc->lock);
>> + if (dbc->state == DS_CONFIGURED)
>> + ret = dbc_ep_do_queue(dep, req);
>> + spin_unlock(&dbc->lock);
>> +
>> + trace_xhci_dbc_queue_request(req);
>> +
>> + return ret;
>> +}
>> +
>> +static inline void xhci_dbc_do_eps_init(struct xhci_hcd *xhci, bool direction)
>> +{
>> + struct dbc_ep *dep;
>> + struct xhci_dbc *dbc = xhci->dbc;
>> +
>> + dep = &dbc->eps[direction];
>> + dep->dbc = dbc;
>> + dep->direction = direction;
>> + dep->ring = direction ? dbc->ring_in : dbc->ring_out;
>> + dep->alloc_request = dbc_ep_alloc_request;
>> + dep->free_request = dbc_ep_free_request;
>> + dep->queue = dbc_ep_queue;
>
> why do we need the alloc_request, free_request and queue function pointers in struct dbc_ep?
> They are always pointing to the same functions.
I did this in order to make DbC base driver and upper glue layer independent.
It's unnecessary now. I will re-factor this code to make it simple.
>
>> +
>> + INIT_LIST_HEAD(&dep->list_pending);
>> +}
>> +
>> +static void xhci_dbc_eps_init(struct xhci_hcd *xhci)
>> +{
>> + xhci_dbc_do_eps_init(xhci, BULK_OUT);
>> + xhci_dbc_do_eps_init(xhci, BULK_IN);
>> +}
>> +
>> +static void xhci_dbc_eps_exit(struct xhci_hcd *xhci)
>> +{
>> + struct xhci_dbc *dbc = xhci->dbc;
>> +
>> + memset(dbc->eps, 0, ARRAY_SIZE(dbc->eps));
>> +}
>> +
>> +static int xhci_dbc_mem_init(struct xhci_hcd *xhci, gfp_t flags)
>> +{
>> + int ret;
>> + dma_addr_t deq;
>> + u32 string_length;
>> + struct xhci_dbc *dbc = xhci->dbc;
>> +
>> + /* Allocate various rings for events and transfers: */
>> + dbc->ring_evt = xhci_ring_alloc(xhci, 1, 1, TYPE_EVENT, 0, flags);
>> + if (!dbc->ring_evt)
>> + goto evt_fail;
>> +
>> + dbc->ring_in = xhci_ring_alloc(xhci, 1, 1, TYPE_BULK, 0, flags);
>> + if (!dbc->ring_in)
>> + goto in_fail;
>> +
>> + dbc->ring_out = xhci_ring_alloc(xhci, 1, 1, TYPE_BULK, 0, flags);
>> + if (!dbc->ring_out)
>> + goto out_fail;
>> +
>> + /* Allocate and populate ERST: */
>> + ret = xhci_alloc_erst(xhci, dbc->ring_evt, &dbc->erst, flags);
>> + if (ret)
>> + goto erst_fail;
>> +
>> + /* Allocate context data structure: */
>> + dbc->ctx = xhci_alloc_container_ctx(xhci, XHCI_CTX_TYPE_DEVICE, flags);
>> + if (!dbc->ctx)
>> + goto ctx_fail;
>> +
>> + /* Allocate the string table: */
>> + dbc->string_size = sizeof(struct dbc_strings);
>> + dbc->string = dbc_dma_alloc_coherent(xhci,
>> + dbc->string_size,
>> + &dbc->string_dma,
>> + flags);
>> + if (!dbc->string)
>> + goto string_fail;
>> +
>> + /* Setup ERST register: */
>> + writel(dbc->erst.erst_size, &dbc->regs->ersts);
>> + xhci_write_64(xhci, dbc->erst.erst_dma_addr, &dbc->regs->erstba);
>> + deq = xhci_trb_virt_to_dma(dbc->ring_evt->deq_seg,
>> + dbc->ring_evt->dequeue);
>> + xhci_write_64(xhci, deq, &dbc->regs->erdp);
>> +
>> + /* Setup strings and contexts: */
>> + string_length = xhci_dbc_populate_strings(dbc->string);
>> + xhci_dbc_populate_contexts(xhci, string_length);
>> +
>> + mmiowb();
>> +
>> + xhci_dbc_eps_init(xhci);
>> + dbc->state = DS_INITIALIZED;
>> +
>> + return 0;
>> +
>> +string_fail:
>> + xhci_free_container_ctx(xhci, dbc->ctx);
>> + dbc->ctx = NULL;
>> +ctx_fail:
>> + xhci_free_erst(xhci, &dbc->erst);
>> +erst_fail:
>> + xhci_ring_free(xhci, dbc->ring_out);
>> + dbc->ring_out = NULL;
>> +out_fail:
>> + xhci_ring_free(xhci, dbc->ring_in);
>> + dbc->ring_in = NULL;
>> +in_fail:
>> + xhci_ring_free(xhci, dbc->ring_evt);
>> + dbc->ring_evt = NULL;
>> +evt_fail:
>> + return -ENOMEM;
>> +}
>> +
>> +static void xhci_dbc_mem_cleanup(struct xhci_hcd *xhci)
>> +{
>> + struct xhci_dbc *dbc = xhci->dbc;
>> +
>> + if (!dbc)
>> + return;
>> +
>> + xhci_dbc_eps_exit(xhci);
>> +
>> + if (dbc->string) {
>> + dbc_dma_free_coherent(xhci,
>> + dbc->string_size,
>> + dbc->string, dbc->string_dma);
>> + dbc->string = NULL;
>> + }
>> +
>> + xhci_free_container_ctx(xhci, dbc->ctx);
>> + dbc->ctx = NULL;
>> +
>> + xhci_free_erst(xhci, &dbc->erst);
>> + xhci_ring_free(xhci, dbc->ring_out);
>> + xhci_ring_free(xhci, dbc->ring_in);
>> + xhci_ring_free(xhci, dbc->ring_evt);
>> + dbc->ring_in = NULL;
>> + dbc->ring_out = NULL;
>> + dbc->ring_evt = NULL;
>> +}
>> +
>> +static void xhci_dbc_reset_debug_port(struct xhci_hcd *xhci)
>> +{
>> + u32 val;
>> + unsigned long flags;
>> + int port_index;
>> + __le32 __iomem **port_array;
>> +
>> + spin_lock_irqsave(&xhci->lock, flags);
>> +
>> + port_index = xhci->num_usb3_ports;
>> + port_array = xhci->usb3_ports;
>> + if (port_index <= 0) {
>> + spin_unlock_irqrestore(&xhci->lock, flags);
>> + return;
>> + }
>
> we could take the spin lock here, and avoid the above unlock case.
Yes, will re-factor it.
>
>> +
>> + while (port_index--) {
>> + val = readl(port_array[port_index]);
>> + if (!(val & PORT_CONNECT))
>> + writel(val | PORT_RESET, port_array[port_index]);
>> + }
>
> This resets every USB3 port that does not have a device connected.
> Shouldn't the debug port be the first USB3 port in the root hub, or do I remeber wrong?
> That should the be xhci->usb3_ports[0]. Can't we reset just that one?
Yes. I will revisit this function. I even think that we don't need it at all.
>
>> +
>> + spin_unlock_irqrestore(&xhci->lock, flags);
>> +}
>> +
>> +static int xhci_do_dbc_start(struct xhci_hcd *xhci)
>> +{
>> + int ret;
>> + u32 ctrl;
>> + struct xhci_dbc *dbc = xhci->dbc;
>> +
>> + if (dbc->state != DS_DISABLED)
>> + return -EINVAL;
>> +
>> + writel(0, &dbc->regs->control);
>> + ret = xhci_handshake(&dbc->regs->control,
>> + DBC_CTRL_DBC_ENABLE,
>> + 0, 1000);
>> + if (ret)
>> + return ret;
>> +
>> + ret = xhci_dbc_mem_init(xhci, GFP_ATOMIC);
>> + if (ret)
>> + return ret;
>> +
>> + ctrl = readl(&dbc->regs->control);
>> + writel(ctrl | DBC_CTRL_DBC_ENABLE | DBC_CTRL_PORT_ENABLE,
>> + &dbc->regs->control);
>> + ret = xhci_handshake(&dbc->regs->control,
>> + DBC_CTRL_DBC_ENABLE,
>> + DBC_CTRL_DBC_ENABLE, 1000);
>> + if (ret)
>> + return ret;
>> +
>> + xhci_dbc_reset_debug_port(xhci);
>> + dbc->state = DS_ENABLED;
>> +
>> + return 0;
>> +}
>> +
>> +static void xhci_do_dbc_stop(struct xhci_hcd *xhci)
>> +{
>> + struct xhci_dbc *dbc = xhci->dbc;
>> +
>> + if (dbc->state == DS_DISABLED)
>> + return;
>> +
>> + writel(0, &dbc->regs->control);
>> + xhci_dbc_mem_cleanup(xhci);
>> + dbc->state = DS_DISABLED;
>> +}
>> +
>> +static int xhci_dbc_start(struct xhci_hcd *xhci)
>> +{
>> + int ret;
>> + struct xhci_dbc *dbc = xhci->dbc;
>> +
>> + WARN_ON(!dbc);
>> +
>> + spin_lock(&dbc->lock);
>
> Maybe call the pm_runtime_get_sync() already here to prevent runtime pm from
> stopping the controller while starting dbc
>
> we then need to pm_runtme_put it do_dbc_start() fails
Yes. Great! We should avoid host suspending while starting DbC.
>
>> + ret = xhci_do_dbc_start(xhci);
>> + spin_unlock(&dbc->lock);
>> +
>> + if (!ret) {
>> + pm_runtime_get_sync(xhci_to_hcd(xhci)->self.controller);
>> + ret = mod_delayed_work(system_wq, &dbc->event_work, 1);
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static void xhci_dbc_stop(struct xhci_hcd *xhci)
>> +{
>> + struct xhci_dbc *dbc = xhci->dbc;
>> + struct dbc_port *port = &dbc->port;
>> +
>> + WARN_ON(!dbc);
>> +
>> + cancel_delayed_work_sync(&dbc->event_work);
>> +
>> + if (port->registered)
>> + xhci_dbc_tty_unregister_device(xhci);
>> +
>> + spin_lock(&dbc->lock);
>> + xhci_do_dbc_stop(xhci);
>> + spin_unlock(&dbc->lock);
>> +
>> + pm_runtime_put_sync(xhci_to_hcd(xhci)->self.controller);
>> +}
>> +
>> +static void
>> +dbc_handle_port_status(struct xhci_hcd *xhci, union xhci_trb *event)
>> +{
>> + u32 portsc;
>> + struct xhci_dbc *dbc = xhci->dbc;
>> +
>> + portsc = readl(&dbc->regs->portsc);
>> + if (portsc & DBC_PORTSC_CONN_CHANGE)
>> + xhci_info(xhci, "DbC port connect change\n");
>> +
>> + if (portsc & DBC_PORTSC_RESET_CHANGE)
>> + xhci_info(xhci, "DbC port reset change\n");
>> +
>> + if (portsc & DBC_PORTSC_LINK_CHANGE)
>> + xhci_info(xhci, "DbC port link status change\n");
>> +
>> + if (portsc & DBC_PORTSC_CONFIG_CHANGE)
>> + xhci_info(xhci, "DbC config error change\n");
>> +
>
> Are these messages info or debug?
For information. Users could read this in dmesg when they
connect DbC to a host.
> If these messages are the only messages seen when connecting a
> debug host (and not constantly spamming dmesg) then they are ok.
There are no spam messages. They only print when connecting to a
debug host.
>
> A bit like usb core annoncing "new usb device detected"
Yes.
>
>> + /* Port reset change bit will be cleared in other place: */
>> + writel(portsc & ~DBC_PORTSC_RESET_CHANGE, &dbc->regs->portsc);
>> +}
>> +
>> +static void dbc_handle_tx_event(struct xhci_hcd *xhci, union xhci_trb *event)
>
> Ah, here you have the opportunity to properly name this function handle_xfer_event() or
> handle_transfer_event().
>
> The handle_tx_event() we use in xhci is confusing because of the tx/rx association.
Yes, I will clean it.
>
>> +{
>> + struct dbc_ep *dep;
>> + struct xhci_ring *ring;
>> + int ep_id;
>> + int status;
>> + u32 comp_code;
>> + size_t remain_length;
>> + struct dbc_request *req = NULL, *r;
>> +
>> + comp_code = GET_COMP_CODE(le32_to_cpu(event->generic.field[2]));
>> + remain_length = EVENT_TRB_LEN(le32_to_cpu(event->generic.field[2]));
>> + ep_id = TRB_TO_EP_ID(le32_to_cpu(event->generic.field[3]));
>> + dep = (ep_id == EPID_OUT) ?
>> + get_out_ep(xhci) : get_in_ep(xhci);
>> + ring = dep->ring;
>> +
>> + switch (comp_code) {
>> + case COMP_SUCCESS:
>> + remain_length = 0;
>> + /* FALLTHROUGH */
>> + case COMP_SHORT_PACKET:
>> + status = 0;
>> + break;
>> + case COMP_TRB_ERROR:
>> + case COMP_BABBLE_DETECTED_ERROR:
>> + case COMP_USB_TRANSACTION_ERROR:
>> + case COMP_STALL_ERROR:
>> + xhci_warn(xhci, "tx error %d detected\n", comp_code);
>> + status = -comp_code;
>> + break;
>> + default:
>> + xhci_err(xhci, "unknown tx error %d\n", comp_code);
>> + status = -comp_code;
>> + break;
>> + }
>> +
>> + /* Match the pending request: */
>> + list_for_each_entry(r, &dep->list_pending, list_pending) {
>> + if (r->trb_dma == event->trans_event.buffer) {
>> + req = r;
>> + break;
>> + }
>> + }
>
> I like that it matches the request even if they are out of order.
>
> Just out of curiosiry, was there some issue with transfer event not always
> matching the next entry in dep->list_pending ?
I didn't catch such issue. The only possibilities are 1) the dep->list_pending
is not listed in the same order as trbs in ring; 2) transfer error happens and
the event trb doesn't match the next entry well. Anyway, the conservative
way is not always considering only the next entry.
>
>> +
>> + if (!req) {
>> + xhci_warn(xhci, "no matched request\n");
>> + return;
>> + }
>> +
>> + trace_xhci_dbc_handle_transfer(ring, &req->trb->generic);
>> +
>> + ring->num_trbs_free++;
>> + req->actual = req->length - remain_length;
>> + xhci_dbc_giveback(req, status);
>> +}
>> +
>> +static enum evtreturn xhci_dbc_do_handle_events(struct xhci_dbc *dbc)
>> +{
>> + dma_addr_t deq;
>> + struct dbc_ep *dep;
>> + union xhci_trb *evt;
>> + u32 ctrl, portsc;
>> + struct xhci_hcd *xhci = dbc->xhci;
>> + bool update_erdp = false;
>> +
>> + /* DbC state machine: */
>> + switch (dbc->state) {
>> + case DS_DISABLED:
>> + case DS_INITIALIZED:
>> +
>> + return EVT_ERR;
>> + case DS_ENABLED:
>> + portsc = readl(&dbc->regs->portsc);
>> + if (portsc & DBC_PORTSC_CONN_STATUS) {
>> + dbc->state = DS_CONNECTED;
>> + xhci_info(xhci, "DbC connected\n");
>> + }
>> +
>> + return EVT_DONE;
>> + case DS_CONNECTED:
>> + ctrl = readl(&dbc->regs->control);
>> + if (ctrl & DBC_CTRL_DBC_RUN) {
>> + dbc->state = DS_CONFIGURED;
>> + xhci_info(xhci, "DbC configured\n");
>> + portsc = readl(&dbc->regs->portsc);
>> + writel(portsc, &dbc->regs->portsc);
>> + return EVT_GSER;
>> + }
>> +
>> + return EVT_DONE;
>> + case DS_CONFIGURED:
>> + /* Handle cable unplug event: */
>> + portsc = readl(&dbc->regs->portsc);
>> + if (!(portsc & DBC_PORTSC_PORT_ENABLED) &&
>> + !(portsc & DBC_PORTSC_CONN_STATUS)) {
>> + xhci_info(xhci, "DbC cable unplugged\n");
>> + dbc->state = DS_ENABLED;
>> + xhci_dbc_flush_reqests(dbc);
>> +
>> + return EVT_DISC;
>> + }
>> +
>> + /* Handle debug port reset event: */
>> + if (portsc & DBC_PORTSC_RESET_CHANGE) {
>> + xhci_info(xhci, "DbC port reset\n");
>> + writel(portsc, &dbc->regs->portsc);
>> + dbc->state = DS_ENABLED;
>> + xhci_dbc_flush_reqests(dbc);
>> +
>> + return EVT_DISC;
>> + }
>> +
>> + /* Handle endpoint stall event: */
>> + ctrl = readl(&dbc->regs->control);
>> + if ((ctrl & DBC_CTRL_HALT_IN_TR) ||
>> + (ctrl & DBC_CTRL_HALT_OUT_TR)) {
>> + xhci_info(xhci, "DbC Endpoint stall\n");
>> + dbc->state = DS_STALLED;
>> +
>> + if (ctrl & DBC_CTRL_HALT_IN_TR) {
>> + dep = get_in_ep(xhci);
>> + xhci_dbc_flush_endpoint_requests(dep);
>> + }
>> +
>> + if (ctrl & DBC_CTRL_HALT_OUT_TR) {
>> + dep = get_out_ep(xhci);
>> + xhci_dbc_flush_endpoint_requests(dep);
>> + }
>> +
>> + return EVT_DONE;
>> + }
>> +
>> + /* Clear DbC run change bit: */
>> + if (ctrl & DBC_CTRL_DBC_RUN_CHANGE) {
>> + writel(ctrl, &dbc->regs->control);
>> + ctrl = readl(&dbc->regs->control);
>> + }
>> +
>> + break;
>> + case DS_STALLED:
>> + ctrl = readl(&dbc->regs->control);
>> + if (!(ctrl & DBC_CTRL_HALT_IN_TR) &&
>> + !(ctrl & DBC_CTRL_HALT_OUT_TR) &&
>> + (ctrl & DBC_CTRL_DBC_RUN)) {
>> + dbc->state = DS_CONFIGURED;
>> + break;
>> + }
>> +
>> + return EVT_DONE;
>> + default:
>> + xhci_err(xhci, "Unknown DbC state %d\n", dbc->state);
>> + break;
>> + }
>> +
>> + /* Handle the events in the event ring: */
>> + evt = dbc->ring_evt->dequeue;
>> + while ((le32_to_cpu(evt->event_cmd.flags) & TRB_CYCLE) ==
>> + dbc->ring_evt->cycle_state) {
>> + /*
>> + * Add a barrier between reading the cycle flag and any
>> + * reads of the event's flags/data below:
>> + */
>> + rmb();
>> +
>> + trace_xhci_dbc_handle_event(dbc->ring_evt, &evt->generic);
>> +
>> + switch (le32_to_cpu(evt->event_cmd.flags) & TRB_TYPE_BITMASK) {
>> + case TRB_TYPE(TRB_PORT_STATUS):
>> + dbc_handle_port_status(xhci, evt);
>> + break;
>> + case TRB_TYPE(TRB_TRANSFER):
>> + dbc_handle_tx_event(xhci, evt);
>> + break;
>> + default:
>> + break;
>> + }
>> +
>> + inc_deq(xhci, dbc->ring_evt);
>> + evt = dbc->ring_evt->dequeue;
>> + update_erdp = true;
>> + }
>> +
>> + /* Update event ring dequeue pointer: */
>> + if (update_erdp) {
>> + deq = xhci_trb_virt_to_dma(dbc->ring_evt->deq_seg,
>> + dbc->ring_evt->dequeue);
>> + xhci_write_64(xhci, deq, &dbc->regs->erdp);
>> + }
>> +
>> + return EVT_DONE;
>> +}
>> +
>> +static void xhci_dbc_handle_events(struct work_struct *work)
>> +{
>> + int ret;
>> + enum evtreturn evtr;
>> + struct xhci_dbc *dbc;
>> + struct xhci_hcd *xhci;
>> +
>> + dbc = container_of(to_delayed_work(work), struct xhci_dbc, event_work);
>> + xhci = dbc->xhci;
>> +
>> + spin_lock(&dbc->lock);
>> + evtr = xhci_dbc_do_handle_events(dbc);
>> + spin_unlock(&dbc->lock);
>> +
>> + switch (evtr) {
>> + case EVT_GSER:
>> + ret = xhci_dbc_tty_register_device(xhci);
>> + if (ret) {
>> + xhci_err(xhci, "failed to alloc tty device\n");
>> + break;
>> + }
>> +
>> + xhci_info(xhci, "DbC now attached to /dev/ttyDBC0\n");
>> + mod_delayed_work(system_wq, &dbc->event_work, 1);
>> + break;
>> + case EVT_DISC:
>> + xhci_dbc_tty_unregister_device(xhci);
>> + mod_delayed_work(system_wq, &dbc->event_work, 1);
>> + break;
>> + case EVT_DONE:
>> + mod_delayed_work(system_wq, &dbc->event_work, 1);
>> + break;
>> + default:
>> + xhci_info(xhci, "stop handling dbc events\n");
>
> return; here
>
>> + }
>
> mod_delayed_work(); here, and remove it from above cases.
Yes, will clean it.
>
>> +}
>> +
>> +static void xhci_dbc_exit(struct xhci_hcd *xhci)
>> +{
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&xhci->lock, flags);
>> + kfree(xhci->dbc);
>> + xhci->dbc = NULL;
>> + spin_unlock_irqrestore(&xhci->lock, flags);
>> +}
>> +
>> +static int xhci_dbc_init(struct xhci_hcd *xhci)
>> +{
>> + u32 reg;
>> + struct xhci_dbc *dbc;
>> + unsigned long flags;
>> + void __iomem *base;
>> + int dbc_cap_offs;
>> +
>> + base = &xhci->cap_regs->hc_capbase;
>> + dbc_cap_offs = xhci_find_next_ext_cap(base, 0, XHCI_EXT_CAPS_DEBUG);
>> + if (!dbc_cap_offs)
>> + return -ENODEV;
>> +
>> + dbc = kzalloc(sizeof(*dbc), GFP_KERNEL);
>> + if (!dbc)
>> + return -ENOMEM;
>> +
>> + dbc->regs = base + dbc_cap_offs;
>> +
>> + /* We will avoid using DbC in xhci driver if it's in use. */
>> + reg = readl(&dbc->regs->control);
>> + if (reg & DBC_CTRL_DBC_ENABLE) {
>> + kfree(dbc);
>> + return -EBUSY;
>> + }
>> +
>> + spin_lock_irqsave(&xhci->lock, flags);
>> + if (xhci->dbc) {
>> + spin_unlock_irqrestore(&xhci->lock, flags);
>> + kfree(dbc);
>> + return -EBUSY;
>> + }
>> + xhci->dbc = dbc;
>> + spin_unlock_irqrestore(&xhci->lock, flags);
>> +
>> + dbc->xhci = xhci;
>> + INIT_DELAYED_WORK(&dbc->event_work, xhci_dbc_handle_events);
>> + spin_lock_init(&dbc->lock);
>> +
>> + return 0;
>> +}
>> +
>> +static ssize_t dbc_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + const char *p;
>> + struct xhci_dbc *dbc;
>> + struct xhci_hcd *xhci;
>> +
>> + xhci = hcd_to_xhci(dev_get_drvdata(dev));
>> + dbc = xhci->dbc;
>> +
>> + switch (dbc->state) {
>> + case DS_DISABLED:
>> + p = "disabled";
>> + break;
>> + case DS_INITIALIZED:
>> + p = "initialized";
>> + break;
>> + case DS_ENABLED:
>> + p = "enabled";
>> + break;
>> + case DS_CONNECTED:
>> + p = "connected";
>> + break;
>> + case DS_CONFIGURED:
>> + p = "configured";
>> + break;
>> + case DS_STALLED:
>> + p = "stalled";
>> + break;
>> + default:
>> + p = "unknown";
>> + }
>> +
>> + return sprintf(buf, "%s\n", p);
>> +}
>> +
>> +static ssize_t dbc_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct xhci_dbc *dbc;
>> + struct xhci_hcd *xhci;
>> +
>> + xhci = hcd_to_xhci(dev_get_drvdata(dev));
>> + dbc = xhci->dbc;
>> +
>> + if (!strncmp(buf, "enable", 6))
>> + xhci_dbc_start(xhci);
>> + else if (!strncmp(buf, "disable", 7))
>> + xhci_dbc_stop(xhci);
>> + else
>> + return -EINVAL;
>> +
>> + return count;
>> +}
>> +
>> +static DEVICE_ATTR(dbc, 0644, dbc_show, dbc_store);
>> +
>> +int dbc_create_sysfs_file(struct xhci_hcd *xhci)
>> +{
>> + int ret;
>> + struct device *dev = xhci_to_hcd(xhci)->self.controller;
>> +
>> + ret = xhci_dbc_init(xhci);
>> + if (ret)
>> + return ret;
>> +
>> + xhci_dbc_tty_register_driver(xhci);
>
> The function name "dbc_create_sysfs_file()" doesn't reveal anything about
> that is also registers the tty driver
Yes. Maybe xhci_dbc_init/exit() sounds better.
I will refine it.
>
>> +
>> + return device_create_file(dev, &dev_attr_dbc);
>> +}
>> +
>> +void dbc_remove_sysfs_file(struct xhci_hcd *xhci)
>> +{
>> + struct device *dev = xhci_to_hcd(xhci)->self.controller;
>> +
>> + device_remove_file(dev, &dev_attr_dbc);
>> + xhci_dbc_tty_unregister_driver();
>> + xhci_dbc_stop(xhci);
>> + xhci_dbc_exit(xhci);
>> +}
>> +
>> +#ifdef CONFIG_PM
>> +int xhci_dbc_suspend(struct xhci_hcd *xhci)
>> +{
>> + struct xhci_dbc *dbc = xhci->dbc;
>> +
>> + if (!dbc)
>> + return 0;
>> +
>> + if (dbc->state == DS_CONFIGURED)
>> + dbc->resume_required = 1;
>> +
>> + xhci_dbc_stop(xhci);
>> +
>> + return 0;
>> +}
>> +
>> +int xhci_dbc_resume(struct xhci_hcd *xhci)
>> +{
>> + int ret = 0;
>> + struct xhci_dbc *dbc = xhci->dbc;
>> +
>> + if (!dbc)
>> + return 0;
>> +
>> + if (dbc->resume_required) {
>> + dbc->resume_required = 0;
>> + xhci_dbc_start(xhci);
>> + }
>> +
>> + return ret;
>
> So far the dbc driver looks really good, it's getting late,
> I need to reviewing the other files later.
>
> I can't say much about the TTY parts as I never needed to work on those
>
> -Mathias
Thanks for your time. I will change the code according to your comments.
The new version(v4) will come out after 4.15-rc1 is out.
Best regards,
Lu Baolu