| Message ID | 20230921214843.18450-2-quic_wcheng@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce QC USB SND audio offloading support | expand |
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 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
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
On 2.10.2023 23.07, Wesley Cheng wrote: > 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. >>> >>> } >>> +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. > Maybe we need to split xhci_free_interrupter() into separate remove and free functions Did some work on this, and on the sideband api in general. Code still has a lot of FIXMEs, and it's completely untested, but to avoid us from doing duplicate work I pushed it to my feature_interrupters branch anyway git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git feature_interrupters https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/log/?h=feature_interrupters Thanks -Mathias
Hi Mathias, On 10/4/2023 7:02 AM, Mathias Nyman wrote: > On 2.10.2023 23.07, Wesley Cheng wrote: >> 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. >>>> >>>> } >>>> +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. >> > > Maybe we need to split xhci_free_interrupter() into separate remove and > free functions > Thanks for sharing the work you've been doing. Yes, I did something similar as well on my end, but will refactor in your code and re-test. > Did some work on this, and on the sideband api in general. > > Code still has a lot of FIXMEs, and it's completely untested, but to > avoid us > from doing duplicate work I pushed it to my feature_interrupters branch > anyway > > git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git > feature_interrupters > https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/log/?h=feature_interrupters > Ok. Initial look at it seems like it will be fine, but will integrate and make changes where needed. Thanks Wesley Cheng
Hi Mathias, On 10/4/2023 11:35 AM, Wesley Cheng wrote: > Hi Mathias, > > On 10/4/2023 7:02 AM, Mathias Nyman wrote: >> On 2.10.2023 23.07, Wesley Cheng wrote: >>> 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. >>>>> >>>>> } >>>>> +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. >>> >> >> Maybe we need to split xhci_free_interrupter() into separate remove >> and free functions >> > > Thanks for sharing the work you've been doing. Yes, I did something > similar as well on my end, but will refactor in your code and re-test. > >> Did some work on this, and on the sideband api in general. >> >> Code still has a lot of FIXMEs, and it's completely untested, but to >> avoid us >> from doing duplicate work I pushed it to my feature_interrupters >> branch anyway >> >> git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git >> feature_interrupters >> https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/log/?h=feature_interrupters >> > > Ok. Initial look at it seems like it will be fine, but will integrate > and make changes where needed. > Had to make some minor tweaks here and there, but nothing major. Was able to validate the changes on my end, and they look good. Will test a bit more, and include these in my next submission. Will try to address your FIXME tags as well. Thanks Wesley Cheng
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); + intr_num = ir->intr_num; + xhci_free_interrupter(xhci, ir); + xhci->interrupters[intr_num] = NULL; + spin_unlock(&xhci->lock); +} +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 */ + 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); 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