Re: [PATCH v3 08/28] ASoC: qcom: Add USB backend ASoC driver for Q6

[Srinivas Kandagatla]

On 09/03/2023 19:38, Wesley Cheng wrote:
> Hi Srinivas,
>
> On 3/9/2023 1:01 AM, Srinivas Kandagatla wrote:
> > On 08/03/2023 23:57, Wesley Cheng wrote:
> > > Create a USB BE component that will register a new USB port to the 
> > > ASoC USB
> > > framework.  This will handle determination on if the requested audio
> > > profile is supported by the USB device currently selected.
> > >
> > > Signed-off-by: Wesley Cheng <quic_wcheng@xxxxxxxxxxx>
> >
> > Thanks Wesley for the patch, I have few minor comments.
>
> Thanks for the review!
>
> > > ---
> > >   include/sound/q6usboffload.h  |  20 ++++
> > >   sound/soc/qcom/Kconfig        |   4 +
> > >   sound/soc/qcom/qdsp6/Makefile |   1 +
> > >   sound/soc/qcom/qdsp6/q6usb.c  | 208 ++++++++++++++++++++++++++++++++++
> > >   4 files changed, 233 insertions(+)
> > >   create mode 100644 include/sound/q6usboffload.h
> > >   create mode 100644 sound/soc/qcom/qdsp6/q6usb.c
> > >
> > > diff --git a/include/sound/q6usboffload.h b/include/sound/q6usboffload.h
> > > new file mode 100644
> > > index 000000000000..4fb1912d9f55
> > > --- /dev/null
> > > +++ b/include/sound/q6usboffload.h
> > > @@ -0,0 +1,20 @@
> > > +/* SPDX-License-Identifier: GPL-2.0
> > > + *
> > > + * linux/sound/q6usboffload.h -- QDSP6 USB offload
> > > + *
> > > + * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All 
> > > rights reserved.
> > > + */
> > > +
> > > +/**
> > > + * struct q6usb_offload
> > > + * @dev - dev handle to usb be
> > > + * @sid - streamID for iommu
> > > + * @intr_num - usb interrupter number
> > > + * @domain - allocated iommu domain
> > > + **/
> > > +struct q6usb_offload {
> > > +    struct device *dev;
> > > +    long long sid;
> > > +    u32 intr_num;
> > > +    struct iommu_domain *domain;
> > Why do we need to store this domain, You can remove this along with 
> > the one line that gets domain in probe function.
>
> We'll need a reference to the iommu domain, because the QC USB offload 
> driver will be the one that is going to map the XHCI interrupter and 
> transfer ring regions for the audio DSP.  This happens when a USB QMI 

this is okay, AFAIU, as long as uaudio_qdev->dev pointer is used in dma 
alloc apis like dma_map*, dma_alloc_* you would not need to handle 
iommu_domain directly like this in drivers.


--srini

> enable stream request is received in the USB offload driver.  Please 
> refer to:
>
> static int prepare_qmi_response(struct snd_usb_substream *subs,
>          struct qmi_uaudio_stream_req_msg_v01 *req_msg,
>          struct qmi_uaudio_stream_resp_msg_v01 *resp, int info_idx)
> {
> ...
>      xhci_pa = xhci_get_ir_resource(subs->dev, ir);
>      if (!xhci_pa) {
>          dev_err(uaudio_qdev->dev,
>              "failed to get sec event ring address\n");
>          ret = -ENODEV;
>          goto free_sec_ring;
>      }
> ...
>      va = uaudio_iommu_map(MEM_EVENT_RING, dma_coherent, xhci_pa, 
> PAGE_SIZE,
>              NULL);
>      if (!va) {
>          ret = -ENOMEM;
>          goto free_sec_ring;
>      }
>
> This is just an example for mapping the XHCI secondary interrupter.  We 
> will also do the same for the transfer ring.
>
> Thanks
> Wesley Cheng