Message-ID: <3795f87d-ae1b-bc58-79b7-56c67329de02@linaro.org>
Date:   Fri, 10 Mar 2023 07:21:20 +0000
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH v3 08/28] ASoC: qcom: Add USB backend ASoC driver for Q6
Content-Language: en-US
To:     Wesley Cheng <quic_wcheng@quicinc.com>, mathias.nyman@intel.com,
        perex@perex.cz, broonie@kernel.org, lgirdwood@gmail.com,
        krzysztof.kozlowski+dt@linaro.org, agross@kernel.org,
        Thinh.Nguyen@synopsys.com, bgoswami@quicinc.com,
        andersson@kernel.org, robh+dt@kernel.org,
        gregkh@linuxfoundation.org, tiwai@suse.com
Cc:     linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        alsa-devel@alsa-project.org, devicetree@vger.kernel.org,
        linux-usb@vger.kernel.org, quic_jackp@quicinc.com,
        quic_plai@quicinc.com
References: <20230308235751.495-1-quic_wcheng@quicinc.com>
 <20230308235751.495-9-quic_wcheng@quicinc.com>
 <ad7dd26d-0ee6-675f-72a5-a93bb0240121@linaro.org>
 <64fc529a-3250-e655-e06e-952f25b1ac2b@quicinc.com>
From:   Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
In-Reply-To: <64fc529a-3250-e655-e06e-952f25b1ac2b@quicinc.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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