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

[Srinivas Kandagatla <srinivas.kandagatla@linaro.org>]

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@quicinc.com>
>>
>> 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