[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <10ad0613-7e88-dbe8-c5a2-d535f8e9db03@linux.intel.com>
Date: Thu, 28 Sep 2023 13:31:09 +0300
From: Mathias Nyman <mathias.nyman@...ux.intel.com>
To: Wesley Cheng <quic_wcheng@...cinc.com>, mathias.nyman@...el.com,
gregkh@...uxfoundation.org, lgirdwood@...il.com,
broonie@...nel.org, perex@...ex.cz, tiwai@...e.com,
agross@...nel.org, andersson@...nel.org, konrad.dybcio@...aro.org,
robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
conor+dt@...nel.org, srinivas.kandagatla@...aro.org,
bgoswami@...cinc.com, Thinh.Nguyen@...opsys.com
Cc: linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
alsa-devel@...a-project.org, linux-arm-msm@...r.kernel.org,
devicetree@...r.kernel.org
Subject: Re: [PATCH v7 01/33] xhci: add support to allocate several
interrupters
On 22.9.2023 0.48, Wesley Cheng wrote:
> From: Mathias Nyman <mathias.nyman@...ux.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@...ux.intel.com>
> Co-developed-by: Wesley Cheng <quic_wcheng@...cinc.com>
> Signed-off-by: Wesley Cheng <quic_wcheng@...cinc.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 touch it */
> + 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
> +}
> +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, interrupter 0 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 running only 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 on the
> * 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.
Thanks
Mathias
Powered by blists - more mailing lists