Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Message-ID: <e3f3c8cd-6338-da08-d988-4d2ed68280e6@quicinc.com>
Date:   Mon, 2 Oct 2023 13:07:28 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
From:   Wesley Cheng <quic_wcheng@quicinc.com>
Subject: Re: [PATCH v7 01/33] xhci: add support to allocate several
 interrupters
To:     Mathias Nyman <mathias.nyman@linux.intel.com>,
        <mathias.nyman@intel.com>, <gregkh@linuxfoundation.org>,
        <lgirdwood@gmail.com>, <broonie@kernel.org>, <perex@perex.cz>,
        <tiwai@suse.com>, <agross@kernel.org>, <andersson@kernel.org>,
        <konrad.dybcio@linaro.org>, <robh+dt@kernel.org>,
        <krzysztof.kozlowski+dt@linaro.org>, <conor+dt@kernel.org>,
        <srinivas.kandagatla@linaro.org>, <bgoswami@quicinc.com>,
        <Thinh.Nguyen@synopsys.com>
CC:     <linux-kernel@vger.kernel.org>, <linux-usb@vger.kernel.org>,
        <alsa-devel@alsa-project.org>, <linux-arm-msm@vger.kernel.org>,
        <devicetree@vger.kernel.org>
References: <20230921214843.18450-1-quic_wcheng@quicinc.com>
 <20230921214843.18450-2-quic_wcheng@quicinc.com>
 <10ad0613-7e88-dbe8-c5a2-d535f8e9db03@linux.intel.com>
Content-Language: en-US
In-Reply-To: <10ad0613-7e88-dbe8-c5a2-d535f8e9db03@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi Mathias,

On 9/28/2023 3:31 AM, Mathias Nyman wrote:
> On 22.9.2023 0.48, Wesley Cheng wrote:
>> From: Mathias Nyman <mathias.nyman@linux.intel.com>
>>
>> Modify the XHCI drivers to accommodate for handling multiple event 
>> rings in
>> case there are multiple interrupters.  Add the required APIs so 
>> clients are
>> able to allocate/request for an interrupter ring, and pass this 
>> information
>> back to the client driver.  This allows for users to handle the resource
>> accordingly, such as passing the event ring base address to an audio DSP.
>> There is no actual support for multiple MSI/MSI-X vectors.
>>
>> Factoring out XHCI interrupter APIs and structures done by Wesley 
>> Cheng, in
>> order to allow for USB class drivers to utilze them.
>>
>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>> Co-developed-by: Wesley Cheng <quic_wcheng@quicinc.com>
>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>> ---
>>   drivers/usb/host/xhci-debugfs.c |  2 +-
>>   drivers/usb/host/xhci-mem.c     | 93 ++++++++++++++++++++++++++++++---
>>   drivers/usb/host/xhci-ring.c    |  2 +-
>>   drivers/usb/host/xhci.c         | 49 ++++++++++-------
>>   drivers/usb/host/xhci.h         | 77 +--------------------------
>>   include/linux/usb/xhci-intr.h   | 86 ++++++++++++++++++++++++++++++
>>   6 files changed, 207 insertions(+), 102 deletions(-)
>>   create mode 100644 include/linux/usb/xhci-intr.h
>>
>> diff --git a/drivers/usb/host/xhci-debugfs.c 
>> b/drivers/usb/host/xhci-debugfs.c
>> index 99baa60ef50f..15a8402ee8a1 100644
>> --- a/drivers/usb/host/xhci-debugfs.c
>> +++ b/drivers/usb/host/xhci-debugfs.c
>> @@ -693,7 +693,7 @@ void xhci_debugfs_init(struct xhci_hcd *xhci)
>>                        "command-ring",
>>                        xhci->debugfs_root);
>> -    xhci_debugfs_create_ring_dir(xhci, &xhci->interrupter->event_ring,
>> +    xhci_debugfs_create_ring_dir(xhci, 
>> &xhci->interrupters[0]->event_ring,
>>                        "event-ring",
>>                        xhci->debugfs_root);
>> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
>> index 8714ab5bf04d..2f9228d7d22d 100644
>> --- a/drivers/usb/host/xhci-mem.c
>> +++ b/drivers/usb/host/xhci-mem.c
>> @@ -1837,6 +1837,26 @@ xhci_free_interrupter(struct xhci_hcd *xhci, 
>> struct xhci_interrupter *ir)
>>       kfree(ir);
>>   }
>> +void xhci_remove_secondary_interrupter(struct usb_hcd *hcd, struct 
>> xhci_interrupter *ir)
>> +{
>> +    struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>> +    unsigned int intr_num;
>> +
>> +    /* interrupter 0 is primary interrupter, don't touchit */
>> +    if (!ir || !ir->intr_num || ir->intr_num >= 
>> xhci->max_interrupters) {
>> +        xhci_dbg(xhci, "Invalid secondary interrupter, can't remove\n");
>> +        return;
>> +    }
>> +
>> +    /* fixme, should we check xhci->interrupter[intr_num] == ir */
>> +    spin_lock(&xhci->lock);
> 
> Needs to be spin_lock_irq() ir spin_lock_irqsave() as xhci->lock is used 
> in interrupt handler.
> 
> 
>> +    intr_num = ir->intr_num;
>> +    xhci_free_interrupter(xhci, ir);
>> +    xhci->interrupters[intr_num] = NULL;
>> +    spin_unlock(&xhci->lock);
> 
> likewise
> 

Let me check these again.  In general, I think I will use both the 
xhci->mutex and xhci->lock where needed, because I believe we'd run into 
sleep while atomic issues while freeing the DMA memory.  Will rework 
this and submit in the next rev.

>> +}
>> +EXPORT_SYMBOL_GPL(xhci_remove_secondary_interrupter);
>> +
>>   void xhci_mem_cleanup(struct xhci_hcd *xhci)
>>   {
>>       struct device    *dev = xhci_to_hcd(xhci)->self.sysdev;
>> @@ -1844,9 +1864,13 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
>>       cancel_delayed_work_sync(&xhci->cmd_timer);
>> -    xhci_free_interrupter(xhci, xhci->interrupter);
>> -    xhci->interrupter = NULL;
>> -    xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Freed primary event 
>> ring");
>> +    for (i = 0; i < xhci->max_interrupters; i++) {
>> +        if (xhci->interrupters[i]) {
>> +            xhci_free_interrupter(xhci, xhci->interrupters[i]);
>> +            xhci->interrupters[i] = NULL;
>> +        }
>> +    }
>> +    xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Freed interrupters");
>>       if (xhci->cmd_ring)
>>           xhci_ring_free(xhci, xhci->cmd_ring);
>> @@ -1916,6 +1940,7 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
>>       for (i = 0; i < xhci->num_port_caps; i++)
>>           kfree(xhci->port_caps[i].psi);
>>       kfree(xhci->port_caps);
>> +    kfree(xhci->interrupters);
>>       xhci->num_port_caps = 0;
>>       xhci->usb2_rhub.ports = NULL;
>> @@ -1924,6 +1949,7 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
>>       xhci->rh_bw = NULL;
>>       xhci->ext_caps = NULL;
>>       xhci->port_caps = NULL;
>> +    xhci->interrupters = NULL;
>>       xhci->page_size = 0;
>>       xhci->page_shift = 0;
>> @@ -2276,6 +2302,13 @@ xhci_add_interrupter(struct xhci_hcd *xhci, 
>> struct xhci_interrupter *ir,
>>           return -EINVAL;
>>       }
>> +    if (xhci->interrupters[intr_num]) {
>> +        xhci_warn(xhci, "Interrupter%d\n already set up", intr_num);
>> +        return -EINVAL;
>> +    }
>> +
>> +    xhci->interrupters[intr_num] = ir;
>> +    ir->intr_num = intr_num;
>>       ir->ir_set = &xhci->run_regs->ir_set[intr_num];
>>       /* set ERST count with the number of entries in the segment 
>> table */
>> @@ -2295,10 +2328,53 @@ xhci_add_interrupter(struct xhci_hcd *xhci, 
>> struct xhci_interrupter *ir,
>>       return 0;
>>   }
>> +struct xhci_interrupter *
>> +xhci_create_secondary_interrupter(struct usb_hcd *hcd)
>> +{
>> +    struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>> +    struct xhci_interrupter *ir;
>> +    unsigned int i;
>> +    int err = -ENOSPC;
>> +
>> +    if (!xhci->interrupters)
>> +        return NULL;
>> +
>> +    ir = xhci_alloc_interrupter(xhci, GFP_KERNEL);
>> +    if (!ir)
>> +        return NULL;
>> +
>> +    spin_lock_irq(&xhci->lock);
>> +
>> +    /* Find available secondary interrupter, interrupter0 is 
>> reserverd for primary */
> 
> reserved
> 
>> +    for (i = 1; i < xhci->max_interrupters; i++) {
>> +        if (xhci->interrupters[i] == NULL) {
>> +            err = xhci_add_interrupter(xhci, ir, i);
>> +            break;
>> +        }
>> +    }
>> +
>> +    spin_unlock_irq(&xhci->lock);
>> +    if (err) {
>> +        xhci_warn(xhci, "Failed to add secondary interrupter, max 
>> interrupters %d\n",
>> +            xhci->max_interrupters);
>> +        xhci_free_interrupter(xhci, ir);
>> +        ir = NULL;
>> +        goto out;
>> +    }
>> +
>> +    xhci_dbg(xhci, "Add secondary interrupter %d, max interrupters 
>> %d\n",
>> +         i, xhci->max_interrupters);
>> +
>> +out:
>> +    return ir;
>> +}
>> +EXPORT_SYMBOL_GPL(xhci_create_secondary_interrupter);
>> +
>>   int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
>>   {
>> -    dma_addr_t    dma;
>> +    struct xhci_interrupter *ir;
>>       struct device    *dev = xhci_to_hcd(xhci)->self.sysdev;
>> +    dma_addr_t    dma;
>>       unsigned int    val, val2;
>>       u64        val_64;
>>       u32        page_size, temp;
>> @@ -2422,11 +2498,14 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t 
>> flags)
>>       /* Allocate and set up primary interrupter 0 with an event ring. */
>>       xhci_dbg_trace(xhci, trace_xhci_dbg_init,
>>                  "Allocating primary event ring");
>> -    xhci->interrupter = xhci_alloc_interrupter(xhci, flags);
>> -    if (!xhci->interrupter)
>> +    xhci->interrupters = kcalloc_node(xhci->max_interrupters, 
>> sizeof(*xhci->interrupters),
>> +                      flags, dev_to_node(dev));
>> +
>> +    ir = xhci_alloc_interrupter(xhci, flags);
>> +    if (!ir)
>>           goto fail;
>> -    if (xhci_add_interrupter(xhci, xhci->interrupter, 0))
>> +    if (xhci_add_interrupter(xhci, ir, 0))
>>           goto fail;
>>       xhci->isoc_bei_interval = AVOID_BEI_INTERVAL_MAX;
>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>> index 1dde53f6eb31..93233cf5ff21 100644
>> --- a/drivers/usb/host/xhci-ring.c
>> +++ b/drivers/usb/host/xhci-ring.c
>> @@ -3074,7 +3074,7 @@ irqreturn_t xhci_irq(struct usb_hcd *hcd)
>>       writel(status, &xhci->op_regs->status);
>>       /* This is the handler of the primary interrupter */
>> -    ir = xhci->interrupter;
>> +    ir = xhci->interrupters[0];
>>       if (!hcd->msi_enabled) {
>>           u32 irq_pending;
>>           irq_pending = readl(&ir->ir_set->irq_pending);
>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>> index e1b1b64a0723..3fd2b58ee1d3 100644
>> --- a/drivers/usb/host/xhci.c
>> +++ b/drivers/usb/host/xhci.c
>> @@ -456,7 +456,7 @@ static int xhci_init(struct usb_hcd *hcd)
>>   static int xhci_run_finished(struct xhci_hcd *xhci)
>>   {
>> -    struct xhci_interrupter *ir = xhci->interrupter;
>> +    struct xhci_interrupter *ir = xhci->interrupters[0];
>>       unsigned long    flags;
>>       u32        temp;
>> @@ -508,7 +508,7 @@ int xhci_run(struct usb_hcd *hcd)
>>       u64 temp_64;
>>       int ret;
>>       struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>> -    struct xhci_interrupter *ir = xhci->interrupter;
>> +    struct xhci_interrupter *ir = xhci->interrupters[0];
>>       /* Start the xHCI host controller runningonly after the USB 2.0 
>> roothub
>>        * is setup.
>>        */
>> @@ -572,7 +572,7 @@ void xhci_stop(struct usb_hcd *hcd)
>>   {
>>       u32 temp;
>>       struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>> -    struct xhci_interrupter *ir = xhci->interrupter;
>> +    struct xhci_interrupter *ir = xhci->interrupters[0];
>>       mutex_lock(&xhci->mutex);
>> @@ -668,36 +668,49 @@ EXPORT_SYMBOL_GPL(xhci_shutdown);
>>   #ifdef CONFIG_PM
>>   static void xhci_save_registers(struct xhci_hcd *xhci)
>>   {
>> -    struct xhci_interrupter *ir = xhci->interrupter;
>> +    struct xhci_interrupter *ir;
>> +    unsigned int i;
>>       xhci->s3.command = readl(&xhci->op_regs->command);
>>       xhci->s3.dev_nt = readl(&xhci->op_regs->dev_notification);
>>       xhci->s3.dcbaa_ptr = xhci_read_64(xhci,&xhci->op_regs->dcbaa_ptr);
>>       xhci->s3.config_reg = readl(&xhci->op_regs->config_reg);
>> -    if (!ir)
>> -        return;
>> +    /* save both primary and all secondary interrupters */
>> +    for (i = 0; i < xhci->max_interrupters; i++) {
>> +        ir = xhci->interrupters[i];
>> +        if (!ir)
>> +            continue;
>> -    ir->s3_erst_size = readl(&ir->ir_set->erst_size);
>> -    ir->s3_erst_base = xhci_read_64(xhci, &ir->ir_set->erst_base);
>> -    ir->s3_erst_dequeue = xhci_read_64(xhci, &ir->ir_set->erst_dequeue);
>> -    ir->s3_irq_pending = readl(&ir->ir_set->irq_pending);
>> -    ir->s3_irq_control = readl(&ir->ir_set->irq_control);
>> +        ir->s3_erst_size = readl(&ir->ir_set->erst_size);
>> +        ir->s3_erst_base = xhci_read_64(xhci, &ir->ir_set->erst_base);
>> +        ir->s3_erst_dequeue = xhci_read_64(xhci, 
>> &ir->ir_set->erst_dequeue);
>> +        ir->s3_irq_pending = readl(&ir->ir_set->irq_pending);
>> +        ir->s3_irq_control = readl(&ir->ir_set->irq_control);
>> +    }
>>   }
>>   static void xhci_restore_registers(struct xhci_hcd *xhci)
>>   {
>> -    struct xhci_interrupter *ir = xhci->interrupter;
>> +    struct xhci_interrupter *ir;
>> +    unsigned int i;
>>       writel(xhci->s3.command, &xhci->op_regs->command);
>>       writel(xhci->s3.dev_nt, &xhci->op_regs->dev_notification);
>>       xhci_write_64(xhci, xhci->s3.dcbaa_ptr, &xhci->op_regs->dcbaa_ptr);
>>       writel(xhci->s3.config_reg, &xhci->op_regs->config_reg);
>> -    writel(ir->s3_erst_size, &ir->ir_set->erst_size);
>> -    xhci_write_64(xhci, ir->s3_erst_base, &ir->ir_set->erst_base);
>> -    xhci_write_64(xhci, ir->s3_erst_dequeue, &ir->ir_set->erst_dequeue);
>> -    writel(ir->s3_irq_pending, &ir->ir_set->irq_pending);
>> -    writel(ir->s3_irq_control, &ir->ir_set->irq_control);
>> +
>> +    for (i = 0; i < xhci->max_interrupters; i++) {
>> +        ir = xhci->interrupters[i];
>> +        if (!ir)
>> +            continue;
>> +
>> +        writel(ir->s3_erst_size, &ir->ir_set->erst_size);
>> +        xhci_write_64(xhci, ir->s3_erst_base, &ir->ir_set->erst_base);
>> +        xhci_write_64(xhci, ir->s3_erst_dequeue, 
>> &ir->ir_set->erst_dequeue);
>> +        writel(ir->s3_irq_pending, &ir->ir_set->irq_pending);
>> +        writel(ir->s3_irq_control, &ir->ir_set->irq_control);
>> +    }
>>   }
>>   static void xhci_set_cmd_ring_deq(struct xhci_hcd *xhci)
>> @@ -1059,7 +1072,7 @@ int xhci_resume(struct xhci_hcd *xhci, 
>> pm_message_t msg)
>>           xhci_dbg(xhci, "// Disabling event ring interrupts\n");
>>           temp = readl(&xhci->op_regs->status);
>>           writel((temp & ~0x1fff) | STS_EINT, &xhci->op_regs->status);
>> -        xhci_disable_interrupter(xhci->interrupter);
>> +        xhci_disable_interrupter(xhci->interrupters[0]);
>>           xhci_dbg(xhci, "cleaning up memory\n");
>>           xhci_mem_cleanup(xhci);
> 
> All code above looks like it should be its own patch.
> 
> The header shuffling below part of somethine else.
>  >> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
>> index 7e282b4522c0..d706a27ec0a3 100644
>> --- a/drivers/usb/host/xhci.h
>> +++ b/drivers/usb/host/xhci.h
>> @@ -17,6 +17,7 @@
>>   #include <linux/kernel.h>
>>   #include <linux/usb/hcd.h>
>>   #include <linux/io-64-nonatomic-lo-hi.h>
>> +#include <linux/usb/xhci-intr.h>
>>   /* Code sharing between pci-quirks and xhci hcd */
>>   #include    "xhci-ext-caps.h"
>> @@ -1541,18 +1542,6 @@ static inline const char 
>> *xhci_trb_type_string(u8 type)
>>   #define AVOID_BEI_INTERVAL_MIN    8
>>   #define AVOID_BEI_INTERVAL_MAX    32
>> -struct xhci_segment {
>> -    union xhci_trb        *trbs;
>> -    /* private to HCD */
>> -    struct xhci_segment    *next;
>> -    dma_addr_t       dma;
>> -    /* Max packet sized bounce buffer for td-fragmant alignment */
>> -    dma_addr_t       bounce_dma;
>> -    void            *bounce_buf;
>> -    unsigned int        bounce_offs;
>> -    unsigned int        bounce_len;
>> -};
>> -
>>   enum xhci_cancelled_td_status {
>>       TD_DIRTY = 0,
>>       TD_HALTED,
>> @@ -1585,16 +1574,6 @@ struct xhci_cd {
>>       union xhci_trb        *cmd_trb;
>>   };
>> -enum xhci_ring_type {
>> -    TYPE_CTRL = 0,
>> -    TYPE_ISOC,
>> -    TYPE_BULK,
>> -    TYPE_INTR,
>> -    TYPE_STREAM,
>> -    TYPE_COMMAND,
>> -    TYPE_EVENT,
>> -};
>> -
>>   static inline const char *xhci_ring_type_string(enum xhci_ring_type 
>> type)
>>   {
>>       switch (type) {
>> @@ -1615,46 +1594,6 @@ static inline const char 
>> *xhci_ring_type_string(enum xhci_ring_type type)
>>       }
>>       return "UNKNOWN";
>> -}
>> -
>> -struct xhci_ring {
>> -    struct xhci_segment    *first_seg;
>> -    struct xhci_segment    *last_seg;
>> -    union  xhci_trb        *enqueue;
>> -    struct xhci_segment    *enq_seg;
>> -    union  xhci_trb        *dequeue;
>> -    struct xhci_segment    *deq_seg;
>> -    struct list_head    td_list;
>> -    /*
>> -     * Write the cycle state into the TRB cycle field to give 
>> ownership of
>> -     * the TRB to the host controller (if we are the producer), or to 
>> check
>> -     * if we own the TRB (if we are the consumer).  See section 4.9.1.
>> -     */
>> -    u32            cycle_state;
>> -    unsigned int        stream_id;
>> -    unsigned int        num_segs;
>> -    unsigned int        num_trbs_free; /* used only by xhci DbC */
>> -    unsigned int        bounce_buf_len;
>> -    enum xhci_ring_type    type;
>> -    bool            last_td_was_short;
>> -    struct radix_tree_root    *trb_address_map;
>> -};
>> -
>> -struct xhci_erst_entry {
>> -    /* 64-bit event ring segment address */
>> -    __le64    seg_addr;
>> -    __le32    seg_size;
>> -    /* Set to zero */
>> -    __le32    rsvd;
>> -};
>> -
>> -struct xhci_erst {
>> -    struct xhci_erst_entry    *entries;
>> -    unsigned int        num_entries;
>> -    /* xhci->event_ring keeps track of segment dma addresses */
>> -    dma_addr_t       erst_dma_addr;
>> -    /* Num entries the ERST can contain */
>> -    unsigned int        erst_size;
>>   };
>>   struct xhci_scratchpad {
>> @@ -1707,18 +1646,6 @@ struct xhci_bus_state {
>>       unsigned long        resuming_ports;
>>   };
>> -struct xhci_interrupter {
>> -    struct xhci_ring    *event_ring;
>> -    struct xhci_erst    erst;
>> -    struct xhci_intr_reg __iomem *ir_set;
>> -    unsigned int        intr_num;
>> -    /* For interrupter registers save and restore over suspend/resume */
>> -    u32    s3_irq_pending;
>> -    u32    s3_irq_control;
>> -    u32    s3_erst_size;
>> -    u64    s3_erst_base;
>> -    u64    s3_erst_dequeue;
>> -};
>>   /*
>>    * It can take up to 20 ms to transition from RExit to U0 onthe
>>    * Intel Lynx Point LP xHCI host.
>> @@ -1799,7 +1726,7 @@ struct xhci_hcd {
>>       struct reset_control *reset;
>>       /* data structures */
>>       struct xhci_device_context_array *dcbaa;
>> -    struct xhci_interrupter *interrupter;
>> +    struct xhci_interrupter **interrupters;
>>       struct xhci_ring    *cmd_ring;
>>       unsigned int            cmd_ring_state;
>>   #define CMD_RING_STATE_RUNNING         (1 << 0)
>> diff --git a/include/linux/usb/xhci-intr.h 
>> b/include/linux/usb/xhci-intr.h
>> new file mode 100644
>> index 000000000000..e0091ee2c73a
>> --- /dev/null
>> +++ b/include/linux/usb/xhci-intr.h
>> @@ -0,0 +1,86 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef __LINUX_XHCI_INTR_H
>> +#define __LINUX_XHCI_INTR_H
>> +
>> +#include <linux/kernel.h>
>> +
>> +struct xhci_erst_entry {
>> +    /* 64-bit event ring segment address */
>> +    __le64    seg_addr;
>> +    __le32    seg_size;
>> +    /* Set to zero */
>> +    __le32    rsvd;
>> +};
>> +
>> +enum xhci_ring_type {
>> +    TYPE_CTRL = 0,
>> +    TYPE_ISOC,
>> +    TYPE_BULK,
>> +    TYPE_INTR,
>> +    TYPE_STREAM,
>> +    TYPE_COMMAND,
>> +    TYPE_EVENT,
>> +};
>> +
>> +struct xhci_erst {
>> +    struct xhci_erst_entry    *entries;
>> +    unsigned int        num_entries;
>> +    /* xhci->event_ring keeps track of segment dma addresses */
>> +    dma_addr_t       erst_dma_addr;
>> +    /* Num entries the ERST can contain */
>> +    unsigned int        erst_size;
>> +};
>> +
>> +struct xhci_segment {
>> +    union xhci_trb        *trbs;
>> +    /* private to HCD */
>> +    struct xhci_segment    *next;
>> +    dma_addr_t       dma;
>> +    /* Max packet sized bounce buffer for td-fragmant alignment */
>> +    dma_addr_t       bounce_dma;
>> +    void            *bounce_buf;
>> +    unsigned int        bounce_offs;
>> +    unsigned int        bounce_len;
>> +};
>> +
>> +struct xhci_ring {
>> +    struct xhci_segment    *first_seg;
>> +    struct xhci_segment    *last_seg;
>> +    union  xhci_trb        *enqueue;
>> +    struct xhci_segment    *enq_seg;
>> +    union  xhci_trb        *dequeue;
>> +    struct xhci_segment    *deq_seg;
>> +    struct list_head    td_list;
>> +    /*
>> +     * Write the cycle state into the TRB cycle field to give 
>> ownership of
>> +     * the TRB to the host controller (if we are the producer), or to 
>> check
>> +     * if we own the TRB (if we are the consumer).  See section 4.9.1.
>> +     */
>> +    u32            cycle_state;
>> +    unsigned int        stream_id;
>> +    unsigned int        num_segs;
>> +    unsigned int        num_trbs_free;
>> +    unsigned int        num_trbs_free_temp;
>> +    unsigned int        bounce_buf_len;
>> +    enum xhci_ring_type    type;
>> +    bool            last_td_was_short;
>> +    struct radix_tree_root    *trb_address_map;
>> +};
>> +
>> +struct xhci_interrupter {
>> +    struct xhci_ring    *event_ring;
>> +    struct xhci_erst    erst;
>> +    struct xhci_intr_reg __iomem *ir_set;
>> +    unsigned int        intr_num;
>> +    /* For interrupter registers save and restore over suspend/resume */
>> +    u32    s3_irq_pending;
>> +    u32    s3_irq_control;
>> +    u32    s3_erst_size;
>> +    u64    s3_erst_base;
>> +    u64    s3_erst_dequeue;
>> +};
>> +
>> +struct xhci_interrupter *
>> +xhci_create_secondary_interrupter(struct usb_hcd *hcd);
>> +void xhci_remove_secondary_interrupter(struct usb_hcd *hcd, struct 
>> xhci_interrupter *ir);
>> +#endif
>>
> 
> Not convinced we want to share all these xhci private structures in a 
> separate
> header outside of the xhci code.
> 
> As much as possible should be abstracted and added to the xhci sideband
> API in patch 3/33 instead of sharing these.

It gets a bit difficult because xhci_create_secondary_interrupter() will 
return struct xhci_interrupter, so that the class (offload) driver can 
fetch information about the event ring.  So part of that is that the 
class driver has to reference struct xhci_ring as well.

Instead of exposing all these into a header file, what do you think 
about adding the drivers/xhci path as an include directory in the class 
driver make arguments in the makefile?

Thanks
Wesley Cheng