| Message ID | 20230921214843.18450-1-quic_wcheng@quicinc.com |
|---|---|
| Headers | show |
| Series | Introduce QC USB SND audio offloading support | expand |
On Thu, Sep 21, 2023 at 02:48:16PM -0700, Wesley Cheng wrote: > +static struct device_node *snd_soc_find_phandle(struct device *dev) > +{ > + struct device_node *node; > + > + node = of_parse_phandle(dev->of_node, "usb-soc-be", 0); Very nitpicky but this function possibly wants a _usb_ in the name, not that it *super* matters with it being static. Or it could just be inlined into the only user and not worry about the naming at all. > +/** > + * snd_soc_usb_get_priv_data() - Retrieve private data stored > + * @dev: device reference > + * > + * Fetch the private data stored in the USB SND SOC structure. > + * > + */ > +void *snd_soc_usb_get_priv_data(struct device *dev) > +{ > + struct snd_soc_usb *ctx; > + > + ctx = snd_soc_find_usb_ctx(dev); > + if (!ctx) { > + /* Check if backend device */ > + mutex_lock(&ctx_mutex); > + list_for_each_entry(ctx, &usb_ctx_list, list) { > + if (dev->of_node == ctx->dev->of_node) { > + mutex_unlock(&ctx_mutex); > + goto out; > + } > + } > + mutex_unlock(&ctx_mutex); > + ctx = NULL; > + } This seems a lot more expensive than I'd expect for a get_priv_data operation, usually it's just a container_of() or other constant time pulling out of a pointer rather than a linked list walk - the sort of thing that people put at the start of functions and do all the time. If we need this I think it needs a name that's more clearly tied to the use case. I didn't actually find the user of this though?
On Thu, Sep 21, 2023 at 02:48:19PM -0700, Wesley Cheng wrote: > For USB offloading situations, the AFE port start command will result in a > QMI handshake between the Q6DSP and the main processor. Depending on if > the USB bus is suspended, this routine would require more time to complete, > as resuming the USB bus has some overhead associated with it. Increase the > timeout to 3s to allow for sufficient time for the USB QMI stream enable > handshake to complete. ... > -#define TIMEOUT_MS 1000 > +#define TIMEOUT_MS 3000 That seems worryingly large but if it's what the hardware/firmware needs I guess there's nothing doing - even the 1s that's being replaced would be nasty if we ever actually hit it.
On Thu, Sep 21, 2023 at 02:48:39PM -0700, Wesley Cheng wrote: > Add a kcontrol to the platform sound card to fetch the current offload > status. This can allow for userspace to ensure/check which USB SND > resources are actually busy versus having to attempt opening the USB SND > devices, which will result in an error if offloading is active. > +static int q6usb_prepare(struct snd_pcm_substream *substream, > + struct snd_soc_dai *dai) > +{ > + struct q6usb_port_data *data = dev_get_drvdata(dai->dev); > + > + mutex_lock(&data->mutex); > + data->status[data->sel_card_idx].running = true; > + mutex_unlock(&data->mutex); These updates of running should really have a snd_ctl_notify() so that UIs can know to update when the value changes while they're open. > +static int q6usb_mixer_get_offload_status(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) > +{ > + running = q6usb_find_running(data); > + if (running < 0) { > + card_idx = -1; > + pcm_idx = -1; > + } else { > + card_idx = running; > + pcm_idx = data->status[running].pcm_index; > + } > + > + ucontrol->value.integer.value[0] = card_idx; > + ucontrol->value.integer.value[1] = pcm_idx; This feels a bit messy but I'm not sure what we'd do that's better so unless someone else has better ideas let's go with this. Possibly we should standardise this as a new control type for joining cards up so at least if there's further needs for this we can use the same solution?
On Thu, Sep 21, 2023 at 02:48:10PM -0700, Wesley Cheng wrote: > Several Qualcomm based chipsets can support USB audio offloading to a > dedicated audio DSP, which can take over issuing transfers to the USB > host controller. The intention is to reduce the load on the main > processors in the SoC, and allow them to be placed into lower power modes. I had a few small comments in reply to some of the patches but overall the ASoC sides of this look fine to me. I didn't really look at the USB side at all, I'm not sure I understand it enough to have any useful thoughts anyway. Thanks for taking on this work and pushing it forwards!
On Wed, Sep 27, 2023 at 05:04:06PM +0200, Mark Brown wrote: > On Thu, Sep 21, 2023 at 02:48:10PM -0700, Wesley Cheng wrote: > > Several Qualcomm based chipsets can support USB audio offloading to a > > dedicated audio DSP, which can take over issuing transfers to the USB > > host controller. The intention is to reduce the load on the main > > processors in the SoC, and allow them to be placed into lower power modes. > > I had a few small comments in reply to some of the patches but overall > the ASoC sides of this look fine to me. I didn't really look at the USB > side at all, I'm not sure I understand it enough to have any useful > thoughts anyway. Thanks for reviewing those portions, I'll look at the USB bits next week when I get back from traveling. greg k-h
Hi Mark, On 9/27/2023 7:48 AM, Mark Brown wrote: > On Thu, Sep 21, 2023 at 02:48:16PM -0700, Wesley Cheng wrote: > >> +static struct device_node *snd_soc_find_phandle(struct device *dev) >> +{ >> + struct device_node *node; >> + >> + node = of_parse_phandle(dev->of_node, "usb-soc-be", 0); > > Very nitpicky but this function possibly wants a _usb_ in the name, not > that it *super* matters with it being static. Or it could just be > inlined into the only user and not worry about the naming at all. > Thanks for the review! Sure, let me reshuffle things around a bit and just get rid of this function entirely and inline it to the API below. >> +/** >> + * snd_soc_usb_get_priv_data() - Retrieve private data stored >> + * @dev: device reference >> + * >> + * Fetch the private data stored in the USB SND SOC structure. >> + * >> + */ >> +void *snd_soc_usb_get_priv_data(struct device *dev) >> +{ >> + struct snd_soc_usb *ctx; >> + >> + ctx = snd_soc_find_usb_ctx(dev); >> + if (!ctx) { >> + /* Check if backend device */ >> + mutex_lock(&ctx_mutex); >> + list_for_each_entry(ctx, &usb_ctx_list, list) { >> + if (dev->of_node == ctx->dev->of_node) { >> + mutex_unlock(&ctx_mutex); >> + goto out; >> + } >> + } >> + mutex_unlock(&ctx_mutex); >> + ctx = NULL; >> + } > > This seems a lot more expensive than I'd expect for a get_priv_data > operation, usually it's just a container_of() or other constant time > pulling out of a pointer rather than a linked list walk - the sort of > thing that people put at the start of functions and do all the time. > If we need this I think it needs a name that's more clearly tied to the > use case. > > I didn't actually find the user of this though? So the end user of this would be the qc_audio_offload driver, within prepare_qmi_response(). This is to fetch some information about the DPCM backend during the stream enable request. Previously, I limited the # of snd_soc_usb ports to be registered to one, but that would affect the scalability of this layer. Hence, adding a list instead increased the complexity. Will rename this accordingly. Thanks Wesley Cheng
Hi Mark, On 9/27/2023 7:50 AM, Mark Brown wrote: > On Thu, Sep 21, 2023 at 02:48:19PM -0700, Wesley Cheng wrote: >> For USB offloading situations, the AFE port start command will result in a >> QMI handshake between the Q6DSP and the main processor. Depending on if >> the USB bus is suspended, this routine would require more time to complete, >> as resuming the USB bus has some overhead associated with it. Increase the >> timeout to 3s to allow for sufficient time for the USB QMI stream enable >> handshake to complete. > > ... > >> -#define TIMEOUT_MS 1000 >> +#define TIMEOUT_MS 3000 > > That seems worryingly large but if it's what the hardware/firmware needs > I guess there's nothing doing - even the 1s that's being replaced would > be nasty if we ever actually hit it. I may have gone overkill with the delay, but when I measured the duration of the AFE start command it took ~1.5-2s. It has to also account for the overhead within handling the USB QMI request in the qc_audio_offload driver. Thanks Wesley Cheng
Hi Mark, On 9/27/2023 8:02 AM, Mark Brown wrote: > On Thu, Sep 21, 2023 at 02:48:39PM -0700, Wesley Cheng wrote: > >> Add a kcontrol to the platform sound card to fetch the current offload >> status. This can allow for userspace to ensure/check which USB SND >> resources are actually busy versus having to attempt opening the USB SND >> devices, which will result in an error if offloading is active. > >> +static int q6usb_prepare(struct snd_pcm_substream *substream, >> + struct snd_soc_dai *dai) >> +{ >> + struct q6usb_port_data *data = dev_get_drvdata(dai->dev); >> + >> + mutex_lock(&data->mutex); >> + data->status[data->sel_card_idx].running = true; >> + mutex_unlock(&data->mutex); > > These updates of running should really have a snd_ctl_notify() so that > UIs can know to update when the value changes while they're open. > Sure, me review some of the APIs again and add the notify call where necessary. >> +static int q6usb_mixer_get_offload_status(struct snd_kcontrol *kcontrol, >> + struct snd_ctl_elem_value *ucontrol) >> +{ > >> + running = q6usb_find_running(data); >> + if (running < 0) { >> + card_idx = -1; >> + pcm_idx = -1; >> + } else { >> + card_idx = running; >> + pcm_idx = data->status[running].pcm_index; >> + } >> + >> + ucontrol->value.integer.value[0] = card_idx; >> + ucontrol->value.integer.value[1] = pcm_idx; > > This feels a bit messy but I'm not sure what we'd do that's better so > unless someone else has better ideas let's go with this. Possibly we > should standardise this as a new control type for joining cards up so at > least if there's further needs for this we can use the same solution? I'm all ears for any suggestions from other users :). I think its a bit difficult to tell since this is the first iteration of adding this feature. Pierre gave me some great feedback from the userspace/application level, and tried my best to accommodate for those requirements since it would be the main entity interacting with these controls. Thanks Wesley Cheng
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
On 22.9.2023 0.48, Wesley Cheng wrote: > From: Mathias Nyman <mathias.nyman@linux.intel.com> > > Expose xhci_stop_endpoint_sync() which is a synchronous variant of > xhci_queue_stop_endpoint(). This is useful for client drivers that are > using the secondary interrupters, and need to stop/clean up the current > session. The stop endpoint command handler will also take care of cleaning > up the ring. > > Modifications to repurpose the new API into existing stop endpoint > sequences was implemented by Wesley Cheng. > > 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-hub.c | 29 +++--------------- > drivers/usb/host/xhci.c | 60 +++++++++++++++++++++++++++---------- > drivers/usb/host/xhci.h | 2 ++ > 3 files changed, 50 insertions(+), 41 deletions(-) > > diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c > index 0054d02239e2..2f7309bdc922 100644 > --- a/drivers/usb/host/xhci-hub.c > +++ b/drivers/usb/host/xhci-hub.c > @@ -489,7 +489,6 @@ EXPORT_SYMBOL_GPL(xhci_find_slot_id_by_port); > static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend) > { > struct xhci_virt_device *virt_dev; > - struct xhci_command *cmd; > unsigned long flags; > int ret; > int i; > @@ -501,10 +500,6 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend) > > trace_xhci_stop_device(virt_dev); > > - cmd = xhci_alloc_command(xhci, true, GFP_NOIO); > - if (!cmd) > - return -ENOMEM; > - > spin_lock_irqsave(&xhci->lock, flags); > for (i = LAST_EP_INDEX; i > 0; i--) { > if (virt_dev->eps[i].ring && virt_dev->eps[i].ring->dequeue) { > @@ -521,7 +516,7 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend) > if (!command) { > spin_unlock_irqrestore(&xhci->lock, flags); > ret = -ENOMEM; > - goto cmd_cleanup; > + goto out; > } > > ret = xhci_queue_stop_endpoint(xhci, command, slot_id, > @@ -529,30 +524,14 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend) > if (ret) { > spin_unlock_irqrestore(&xhci->lock, flags); > xhci_free_command(xhci, command); > - goto cmd_cleanup; > + goto out; > } > } > } > - ret = xhci_queue_stop_endpoint(xhci, cmd, slot_id, 0, suspend); > - if (ret) { > - spin_unlock_irqrestore(&xhci->lock, flags); > - goto cmd_cleanup; > - } > - > - xhci_ring_cmd_db(xhci); > spin_unlock_irqrestore(&xhci->lock, flags); > + ret = xhci_stop_endpoint_sync(xhci, &virt_dev->eps[0], suspend); I didn't take this new xhci_stop_endpoint_sync() helper into use as it causes an extra xhci spinlock release and reacquire here. Also the memory allocation flags differ, GFP_NOIO is turned into GFP_KERNEL after this change. > > - /* Wait for last stop endpoint command to finish */ > - wait_for_completion(cmd->completion); > - > - if (cmd->status == COMP_COMMAND_ABORTED || > - cmd->status == COMP_COMMAND_RING_STOPPED) { > - xhci_warn(xhci, "Timeout while waiting for stop endpoint command\n"); > - ret = -ETIME; > - } > - > -cmd_cleanup: > - xhci_free_command(xhci, cmd); > +out: > return ret; > } > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > index 3fd2b58ee1d3..163d533d6200 100644 > --- a/drivers/usb/host/xhci.c > +++ b/drivers/usb/host/xhci.c > @@ -2758,6 +2758,46 @@ static int xhci_reserve_bandwidth(struct xhci_hcd *xhci, > return -ENOMEM; > } > > +/* > + * Synchronous XHCI stop endpoint helper. Issues the stop endpoint command and > + * waits for the command completion before returning. > + */ > +int xhci_stop_endpoint_sync(struct xhci_hcd *xhci, struct xhci_virt_ep *ep, int suspend) > +{ > + struct xhci_command *command; > + unsigned long flags; > + int ret; > + > + command = xhci_alloc_command(xhci, true, GFP_KERNEL); > + if (!command) > + return -ENOMEM; > + > + spin_lock_irqsave(&xhci->lock, flags); > + ret = xhci_queue_stop_endpoint(xhci, command, ep->vdev->slot_id, > + ep->ep_index, suspend); > + if (ret < 0) { > + spin_unlock_irqrestore(&xhci->lock, flags); > + goto out; > + } > + > + xhci_ring_cmd_db(xhci); > + spin_unlock_irqrestore(&xhci->lock, flags); > + > + ret = wait_for_completion_timeout(command->completion, msecs_to_jiffies(3000)); > + if (!ret) > + xhci_warn(xhci, "%s: Unable to stop endpoint.\n", > + __func__); > + > + if (command->status == COMP_COMMAND_ABORTED || > + command->status == COMP_COMMAND_RING_STOPPED) { > + xhci_warn(xhci, "Timeout while waiting for stop endpoint command\n"); > + ret = -ETIME; > + } > +out: > + xhci_free_command(xhci, command); > + > + return ret; > +} > > /* Issue a configure endpoint command or evaluate context command > * and wait for it to finish. > @@ -3078,7 +3118,7 @@ static void xhci_endpoint_reset(struct usb_hcd *hcd, > struct xhci_virt_device *vdev; > struct xhci_virt_ep *ep; > struct xhci_input_control_ctx *ctrl_ctx; > - struct xhci_command *stop_cmd, *cfg_cmd; > + struct xhci_command *cfg_cmd; > unsigned int ep_index; > unsigned long flags; > u32 ep_flag; > @@ -3118,10 +3158,6 @@ static void xhci_endpoint_reset(struct usb_hcd *hcd, > if (ep_flag == SLOT_FLAG || ep_flag == EP0_FLAG) > return; > > - stop_cmd = xhci_alloc_command(xhci, true, GFP_NOWAIT); > - if (!stop_cmd) > - return; > - > cfg_cmd = xhci_alloc_command_with_ctx(xhci, true, GFP_NOWAIT); > if (!cfg_cmd) > goto cleanup; > @@ -3144,23 +3180,16 @@ static void xhci_endpoint_reset(struct usb_hcd *hcd, > goto cleanup; > } > > - err = xhci_queue_stop_endpoint(xhci, stop_cmd, udev->slot_id, > - ep_index, 0); > + spin_unlock_irqrestore(&xhci->lock, flags); > + Same here, extra unlock -> lock, and GFP flags differ. > + err = xhci_stop_endpoint_sync(xhci, ep, 0); Thanks Mathias
Hi Mathias, On 9/28/2023 6:31 AM, Mathias Nyman wrote: > On 22.9.2023 0.48, Wesley Cheng wrote: >> From: Mathias Nyman <mathias.nyman@linux.intel.com> >> >> Expose xhci_stop_endpoint_sync() which is a synchronous variant of >> xhci_queue_stop_endpoint(). This is useful for client drivers that are >> using the secondary interrupters, and need to stop/clean up the current >> session. The stop endpoint command handler will also take care of >> cleaning >> up the ring. >> >> Modifications to repurpose the new API into existing stop endpoint >> sequences was implemented by Wesley Cheng. >> >> 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-hub.c | 29 +++--------------- >> drivers/usb/host/xhci.c | 60 +++++++++++++++++++++++++++---------- >> drivers/usb/host/xhci.h | 2 ++ >> 3 files changed, 50 insertions(+), 41 deletions(-) >> >> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c >> index 0054d02239e2..2f7309bdc922 100644 >> --- a/drivers/usb/host/xhci-hub.c >> +++ b/drivers/usb/host/xhci-hub.c >> @@ -489,7 +489,6 @@ EXPORT_SYMBOL_GPL(xhci_find_slot_id_by_port); >> static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int >> suspend) >> { >> struct xhci_virt_device *virt_dev; >> - struct xhci_command *cmd; >> unsigned long flags; >> int ret; >> int i; >> @@ -501,10 +500,6 @@ static int xhci_stop_device(struct xhci_hcd >> *xhci, int slot_id, int suspend) >> trace_xhci_stop_device(virt_dev); >> - cmd = xhci_alloc_command(xhci, true, GFP_NOIO); >> - if (!cmd) >> - return -ENOMEM; >> - >> spin_lock_irqsave(&xhci->lock, flags); >> for (i = LAST_EP_INDEX; i > 0; i--) { >> if (virt_dev->eps[i].ring && virt_dev->eps[i].ring->dequeue) { >> @@ -521,7 +516,7 @@ static int xhci_stop_device(struct xhci_hcd *xhci, >> int slot_id, int suspend) >> if (!command) { >> spin_unlock_irqrestore(&xhci->lock, flags); >> ret = -ENOMEM; >> - goto cmd_cleanup; >> + goto out; >> } >> ret = xhci_queue_stop_endpoint(xhci, command, slot_id, >> @@ -529,30 +524,14 @@ static int xhci_stop_device(struct xhci_hcd >> *xhci, int slot_id, int suspend) >> if (ret) { >> spin_unlock_irqrestore(&xhci->lock, flags); >> xhci_free_command(xhci, command); >> - goto cmd_cleanup; >> + goto out; >> } >> } >> } >> - ret = xhci_queue_stop_endpoint(xhci, cmd, slot_id, 0, suspend); >> - if (ret) { >> - spin_unlock_irqrestore(&xhci->lock, flags); >> - goto cmd_cleanup; >> - } >> - >> - xhci_ring_cmd_db(xhci); >> spin_unlock_irqrestore(&xhci->lock, flags); >> + ret = xhci_stop_endpoint_sync(xhci, &virt_dev->eps[0], suspend); > > I didn't take this new xhci_stop_endpoint_sync() helper into use as it > causes an extra > xhci spinlock release and reacquire here. > > Also the memory allocation flags differ, GFP_NOIO is turned into > GFP_KERNEL after this change. > Thanks for the review. I agree with the points made. I wasn't sure if the extra unlock/lock would cause issues if we've already queued the stop ep for the other eps used by the device. I think addressing the flags might be straightforward, we can just pass it in as an argument. At least for this change in particular, is the concern that there could be another XHCI command queued before the stop endpoint command is? >> - /* Wait for last stop endpoint command to finish */ >> - wait_for_completion(cmd->completion); >> - >> - if (cmd->status == COMP_COMMAND_ABORTED || >> - cmd->status == COMP_COMMAND_RING_STOPPED) { >> - xhci_warn(xhci, "Timeout while waiting for stop endpoint >> command\n"); >> - ret = -ETIME; >> - } >> - >> -cmd_cleanup: >> - xhci_free_command(xhci, cmd); >> +out: >> return ret; >> } >> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c >> index 3fd2b58ee1d3..163d533d6200 100644 >> --- a/drivers/usb/host/xhci.c >> +++ b/drivers/usb/host/xhci.c >> @@ -2758,6 +2758,46 @@ static int xhci_reserve_bandwidth(struct >> xhci_hcd *xhci, >> return -ENOMEM; >> } >> +/* >> + * Synchronous XHCI stop endpoint helper. Issues the stop endpoint >> command and >> + * waits for the command completion before returning. >> + */ >> +int xhci_stop_endpoint_sync(struct xhci_hcd *xhci, struct >> xhci_virt_ep *ep, int suspend) >> +{ >> + struct xhci_command *command; >> + unsigned long flags; >> + int ret; >> + >> + command = xhci_alloc_command(xhci, true, GFP_KERNEL); >> + if (!command) >> + return -ENOMEM; >> + >> + spin_lock_irqsave(&xhci->lock, flags); >> + ret = xhci_queue_stop_endpoint(xhci, command, ep->vdev->slot_id, >> + ep->ep_index, suspend); >> + if (ret < 0) { >> + spin_unlock_irqrestore(&xhci->lock, flags); >> + goto out; >> + } >> + >> + xhci_ring_cmd_db(xhci); >> + spin_unlock_irqrestore(&xhci->lock, flags); >> + >> + ret = wait_for_completion_timeout(command->completion, >> msecs_to_jiffies(3000)); >> + if (!ret) >> + xhci_warn(xhci, "%s: Unable to stop endpoint.\n", >> + __func__); >> + >> + if (command->status == COMP_COMMAND_ABORTED || >> + command->status == COMP_COMMAND_RING_STOPPED) { >> + xhci_warn(xhci, "Timeout while waiting for stop endpoint >> command\n"); >> + ret = -ETIME; >> + } >> +out: >> + xhci_free_command(xhci, command); >> + >> + return ret; >> +} >> /* Issue a configure endpoint command or evaluate context command >> * and wait for it to finish. >> @@ -3078,7 +3118,7 @@ static void xhci_endpoint_reset(struct usb_hcd >> *hcd, >> struct xhci_virt_device *vdev; >> struct xhci_virt_ep *ep; >> struct xhci_input_control_ctx *ctrl_ctx; >> - struct xhci_command *stop_cmd, *cfg_cmd; >> + struct xhci_command *cfg_cmd; >> unsigned int ep_index; >> unsigned long flags; >> u32 ep_flag; >> @@ -3118,10 +3158,6 @@ static void xhci_endpoint_reset(struct usb_hcd >> *hcd, >> if (ep_flag == SLOT_FLAG || ep_flag == EP0_FLAG) >> return; >> - stop_cmd = xhci_alloc_command(xhci, true, GFP_NOWAIT); >> - if (!stop_cmd) >> - return; >> - >> cfg_cmd = xhci_alloc_command_with_ctx(xhci, true, GFP_NOWAIT); >> if (!cfg_cmd) >> goto cleanup; >> @@ -3144,23 +3180,16 @@ static void xhci_endpoint_reset(struct usb_hcd >> *hcd, >> goto cleanup; >> } >> - err = xhci_queue_stop_endpoint(xhci, stop_cmd, udev->slot_id, >> - ep_index, 0); >> + spin_unlock_irqrestore(&xhci->lock, flags); >> + > > Same here, extra unlock -> lock, and GFP flags differ. > > My intention here (minus the GFP flags) was that the locking was mainly for setting the EP state flag -- EP_SOFT_CLEAR_TOGGLE. If that was set, then new TD queues are blocked. Seems like that was why there is a check like this afterwards: if (!list_empty(&ep->ring->td_list)) { So I believed that releasing the lock here was going to be ok, because by that point since the flag is set, nothing would be able to be added to the td_list. Thanks Wesley Cheng
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
Several Qualcomm based chipsets can support USB audio offloading to a dedicated audio DSP, which can take over issuing transfers to the USB host controller. The intention is to reduce the load on the main processors in the SoC, and allow them to be placed into lower power modes. There are several parts to this design: 1. Adding ASoC binding layer 2. Create a USB backend for Q6DSP 3. Introduce XHCI interrupter support 4. Create vendor ops for the USB SND driver USB | ASoC -------------------------------------------------------------------- | _________________________ | |sm8250 platform card | | |_________________________| | | | | ___V____ ____V____ | |Q6USB | |Q6AFE | | |"codec" | |"cpu" | | |________| |_________| | ^ ^ ^ | | |________| | ___V____ | | |SOC-USB | | ________ ________ | | | |USB SND |<--->|QC offld|<------------>|________| | |(card.c)| | |<---------- | |________| |________|___ | | | ^ ^ | | | ____________V_________ | | | | | |APR/GLINK | __ V_______________V_____ | | | |______________________| |USB SND (endpoint.c) | | | | ^ |_________________________| | | | | ^ | | | ___________V___________ | | | |->|audio DSP | ___________V_____________ | | |_______________________| |XHCI HCD |<- | |_________________________| | Adding ASoC binding layer: soc-usb: Intention is to treat a USB port similar to a headphone jack. The port is always present on the device, but cable/pin status can be enabled/disabled. Expose mechanisms for USB backend ASoC drivers to communicate with USB SND. Create a USB backend for Q6DSP: q6usb: Basic backend driver that will be responsible for maintaining the resources needed to initiate a playback stream using the Q6DSP. Will be the entity that checks to make sure the connected USB audio device supports the requested PCM format. If it does not, the PCM open call will fail, and userpsace ALSA can take action accordingly. Introduce XHCI interrupter support: XHCI HCD supports multiple interrupters, which allows for events to be routed to different event rings. This is determined by "Interrupter Target" field specified in Section "6.4.1.1 Normal TRB" of the XHCI specification. Events in the offloading case will be routed to an event ring that is assigned to the audio DSP. Create vendor ops for the USB SND driver: qc_audio_offload: This particular driver has several components associated with it: - QMI stream request handler - XHCI interrupter and resource management - audio DSP memory management When the audio DSP wants to enable a playback stream, the request is first received by the ASoC platform sound card. Depending on the selected route, ASoC will bring up the individual DAIs in the path. The Q6USB backend DAI will send an AFE port start command (with enabling the USB playback path), and the audio DSP will handle the request accordingly. Part of the AFE USB port start handling will have an exchange of control messages using the QMI protocol. The qc_audio_offload driver will populate the buffer information: - Event ring base address - EP transfer ring base address and pass it along to the audio DSP. All endpoint management will now be handed over to the DSP, and the main processor is not involved in transfers. Overall, implementing this feature will still expose separate sound card and PCM devices for both the platorm card and USB audio device: 0 [SM8250MTPWCD938]: sm8250 - SM8250-MTP-WCD9380-WSA8810-VA-D SM8250-MTP-WCD9380-WSA8810-VA-DMIC 1 [Audio ]: USB-Audio - USB Audio Generic USB Audio at usb-xhci-hcd.1.auto-1.4, high speed This is to ensure that userspace ALSA entities can decide which route to take when executing the audio playback. In the above, if card#1 is selected, then USB audio data will take the legacy path over the USB PCM drivers, etc... This feature was validated using: - tinymix: set/enable the multimedia path to route to USB backend - tinyplay: issue playback on platform card Changelog -------------------------------------------- Changes in v7: - Fixed dt check error for q6usb bindings - Updated q6usb property from qcom,usb-audio-intr-num --> qcom,usb-audio-intr-idx - Removed separate DWC3 HC interrupters num property, and place limits to XHCI one. - Modified xhci_ring_to_sgtable() to use assigned IOVA/DMA address to fetch pages, as it is not ensured event ring allocated is always done in the vmalloc range. Changes in v6: - Fixed limits and description on several DT bindings (XHCI and Q6USB) - Fixed patch subjects to follow other ALSA/ASoC notations. USB SND - Addressed devices which expose multiple audio (UAC) interfaces. These devices will create a single USB sound card with multiple audio streams, and receive multiple interface probe routines. QC offload was not properly considering cases with multiple probe calls. - Renamed offload module name and kconfig to fit within the SND domain. - Renamed attach/detach endpoint API to keep the hw_params notation. Changes in v5: - Removed some unnescessary files that were included - Fixed some typos mentioned - Addressed dt-binding issues and added hc-interrupters definition to usb-xhci.yaml XHCI: - Moved secondary skip events API to xhci-ring and updated implementation - Utilized existing XHCI APIs, such as inc_deq and xhci_update_erst_dequeue() USB SND - Renamed and reworked the APIs in "sound: usb: Export USB SND APIs for modules" patch to include suggestions to utilize snd_usb_hw_params/free and to avoid generic naming. - Added a resume_cb() op for completion sake. - Addressed some locking concerns with regards to when registering for platform hooks. - Added routine to disconnect all offloaded devices during module unbind. ASoC - Replaced individual PCM parameter arguments in snd_soc_usb_connect() with new snd_soc_usb_device structure to pass along PCM info. - Modified snd_jack set report to notify HEADPHONE event, as we do not support record path. Changes in v4: - Rebased to xhci/for-usb-next - Addressed some dt-bindings comments XHCI: - Pulled in latest changes from Mathias' feature_interrupters branch: https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/log/?h=feature_interrupters - Fixed commit text and signage for the XHCI sideband/interrupter related changes - Added some logic to address the FIXME tags mentioned throughout the commits, such as handling multi segment rings and building the SGT, locking concerns, and ep cleanup operations. - Removed some fixme tags for conditions that may not be needed/addressed. - Repurposed the new endpoint stop sync API to be utilized in other places. - Fixed potential compile issue if XHCI sideband config is not defined. ASoC: - Added sound jack control into the Q6USB driver. Allows for userpsace to know when an offload capable device is connected. USB SND: - Avoided exporting _snd_pcm_hw_param_set based on Takashi's recommendation. - Split USB QMI packet header definitions into a separate commit. This is used to properly allow the QMI interface driver to parse and route QMI packets accordingly - Added a "depends on" entry when enabling QC audio offload to avoid compile time issues. Changes in v3: - Changed prefix from RFC to PATCH - Rebased entire series to usb-next - Updated copyright years XHCI: - Rebased changes on top of XHCI changes merged into usb-next, and only added changes that were still under discussion. - Added change to read in the "num-hc-interrupters" device property. ASoC: - qusb6 USB backend - Incorporated suggestions to fetch iommu information with existing APIs - Added two new sound kcontrols to fetch offload status and offload device selection. - offload status - will return the card and pcm device in use tinymix -D 0 get 1 --> 1, 0 (offload in progress on card#1 pcm#0) - device selection - set the card and pcm device to enable offload on. Ex.: tinymix -D 0 set 1 2 0 --> sets offload on card#2 pcm#0 (this should be the USB card) USB SND: - Fixed up some locking related concerns for registering platform ops. - Moved callbacks under the register_mutex, so that - Modified APIs to properly pass more information about the USB SND device, so that the Q6USB backend can build a device list/map, in order to monitor offload status and device selection. Changes in v2: XHCI: - Replaced XHCI and HCD changes with Mathias' XHCI interrupter changes in his tree: https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/log/?h=feature_interrupters Adjustments made to Mathias' changes: - Created xhci-intr.h to export/expose interrupter APIs versus exposing xhci.h. Moved dependent structures to this file as well. (so clients can parse out information from "struct xhci_interrupter") - Added some basic locking when requesting interrupters. - Fixed up some sanity checks. - Removed clearing of the ERSTBA during freeing of the interrupter. (pending issue where SMMU fault occurs if DMA addr returned is 64b - TODO) - Clean up pending events in the XHCI secondary interrupter. While testing USB bus suspend, it was seen that on bus resume, the xHCI HC would run into a command timeout. - Added offloading APIs to xHCI to fetch transfer and event ring information. ASoC: - Modified soc-usb to allow for multiple USB port additions. For this to work, the USB offload driver has to have a reference to the USB backend by adding a "usb-soc-be" DT entry to the device saved into XHCI sysdev. - Created separate dt-bindings for defining USB_RX port. - Increased APR timeout to accommodate the situation where the AFE port start command could be delayed due to having to issue a USB bus resume while handling the QMI stream start command. USB SND: - Added a platform ops during usb_audio_suspend(). This allows for the USB offload driver to halt the audio stream when system enters PM suspend. This ensures the audio DSP is not issuing transfers on the USB bus. - Do not override platform ops if they are already populated. - Introduce a shared status variable between the USB offload and USB SND layers, to ensure that only one path is active at a time. If the USB bus is occupied, then userspace is notified that the path is busy. Mathias Nyman (3): xhci: add support to allocate several interrupters xhci: add helper to stop endpoint and wait for completion xhci: sideband: add initial api to register a sideband entity Wesley Cheng (30): usb: host: xhci-mem: Cleanup pending secondary event ring events usb: host: xhci-mem: Allow for interrupter clients to choose specific index ASoC: Add SOC USB APIs for adding an USB backend ASoC: dt-bindings: qcom,q6dsp-lpass-ports: Add USB_RX port ASoC: qcom: qdsp6: Introduce USB AFE port to q6dsp ASoC: qdsp6: q6afe: Increase APR timeout ASoC: qcom: qdsp6: Add USB backend ASoC driver for Q6 ALSA: usb-audio: Introduce USB SND platform op callbacks ALSA: usb-audio: Export USB SND APIs for modules dt-bindings: usb: dwc3: Limit num-hc-interrupters definition dt-bindings: usb: xhci: Add num-hc-interrupters definition usb: dwc3: Specify maximum number of XHCI interrupters usb: host: xhci-plat: Set XHCI max interrupters if property is present ALSA: usb-audio: qcom: Add USB QMI definitions ALSA: usb-audio: qcom: Introduce QC USB SND offloading support ALSA: usb-audio: Check for support for requested audio format ASoC: usb: Add PCM format check API for USB backend ASoC: qcom: qdsp6: Ensure PCM format is supported by USB audio device ALSA: usb-audio: Prevent starting of audio stream if in use ASoC: dt-bindings: Add Q6USB backend ASoC: dt-bindings: Update example for enabling USB offload on SM8250 ASoC: qcom: qdsp6: q6afe: Split USB AFE dev_token param into separate API ALSA: usb-audio: qcom: Populate PCM and USB chip information ASoC: qcom: qdsp6: Add support to track available USB PCM devices ASoC: qcom: qdsp6: Add SND kcontrol to select offload device ASoC: qcom: qdsp6: Add SND kcontrol for fetching offload status ASoC: qcom: qdsp6: Add headphone jack for offload connection status ALSA: usb-audio: qcom: Use card and PCM index from QMI request ALSA: usb-audio: Allow for rediscovery of connected USB SND devices ASoC: usb: Rediscover USB SND devices on USB port add .../devicetree/bindings/sound/qcom,q6usb.yaml | 55 + .../bindings/sound/qcom,sm8250.yaml | 15 + .../devicetree/bindings/usb/snps,dwc3.yaml | 4 + .../devicetree/bindings/usb/usb-xhci.yaml | 6 + drivers/usb/dwc3/core.c | 12 + drivers/usb/dwc3/core.h | 2 + drivers/usb/dwc3/host.c | 5 +- drivers/usb/host/Kconfig | 9 + drivers/usb/host/Makefile | 4 + drivers/usb/host/xhci-debugfs.c | 2 +- drivers/usb/host/xhci-hub.c | 29 +- drivers/usb/host/xhci-mem.c | 102 +- drivers/usb/host/xhci-plat.c | 2 + drivers/usb/host/xhci-ring.c | 48 +- drivers/usb/host/xhci-sideband.c | 198 ++ drivers/usb/host/xhci.c | 111 +- drivers/usb/host/xhci.h | 105 +- .../sound/qcom,q6dsp-lpass-ports.h | 1 + include/linux/usb/xhci-intr.h | 86 + include/linux/usb/xhci-sideband.h | 57 + include/sound/q6usboffload.h | 20 + include/sound/soc-usb.h | 51 + sound/soc/Makefile | 2 +- sound/soc/qcom/Kconfig | 4 + sound/soc/qcom/qdsp6/Makefile | 1 + sound/soc/qcom/qdsp6/q6afe-dai.c | 56 + sound/soc/qcom/qdsp6/q6afe.c | 206 +- sound/soc/qcom/qdsp6/q6afe.h | 36 +- sound/soc/qcom/qdsp6/q6dsp-lpass-ports.c | 23 + sound/soc/qcom/qdsp6/q6dsp-lpass-ports.h | 1 + sound/soc/qcom/qdsp6/q6routing.c | 9 + sound/soc/qcom/qdsp6/q6usb.c | 450 ++++ sound/soc/soc-usb.c | 200 ++ sound/usb/Kconfig | 15 + sound/usb/Makefile | 2 +- sound/usb/card.c | 123 ++ sound/usb/card.h | 23 + sound/usb/endpoint.c | 1 + sound/usb/helper.c | 1 + sound/usb/pcm.c | 94 +- sound/usb/pcm.h | 11 + sound/usb/qcom/Makefile | 2 + sound/usb/qcom/qc_audio_offload.c | 1854 +++++++++++++++++ sound/usb/qcom/usb_audio_qmi_v01.c | 892 ++++++++ sound/usb/qcom/usb_audio_qmi_v01.h | 162 ++ 45 files changed, 4896 insertions(+), 196 deletions(-) create mode 100644 Documentation/devicetree/bindings/sound/qcom,q6usb.yaml create mode 100644 drivers/usb/host/xhci-sideband.c create mode 100644 include/linux/usb/xhci-intr.h create mode 100644 include/linux/usb/xhci-sideband.h create mode 100644 include/sound/q6usboffload.h create mode 100644 include/sound/soc-usb.h create mode 100644 sound/soc/qcom/qdsp6/q6usb.c create mode 100644 sound/soc/soc-usb.c create mode 100644 sound/usb/qcom/Makefile create mode 100644 sound/usb/qcom/qc_audio_offload.c create mode 100644 sound/usb/qcom/usb_audio_qmi_v01.c create mode 100644 sound/usb/qcom/usb_audio_qmi_v01.h