Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Message-ID: <c72bcf47-af0b-8819-1c30-06b51358381e@quicinc.com>
Date:   Tue, 17 Oct 2023 18:45:36 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH v9 09/34] ASoC: qcom: qdsp6: Introduce USB AFE port to
 q6dsp
Content-Language: en-US
To:     Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
        <mathias.nyman@intel.com>, <gregkh@linuxfoundation.org>,
        <lgirdwood@gmail.com>, <broonie@kernel.org>, <perex@perex.cz>,
        <tiwai@suse.com>, <agross@kernel.org>, <andersson@kernel.org>,
        <konrad.dybcio@linaro.org>, <robh+dt@kernel.org>,
        <krzysztof.kozlowski+dt@linaro.org>, <conor+dt@kernel.org>,
        <srinivas.kandagatla@linaro.org>, <bgoswami@quicinc.com>,
        <Thinh.Nguyen@synopsys.com>
CC:     <linux-usb@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <alsa-devel@alsa-project.org>, <linux-arm-msm@vger.kernel.org>,
        <devicetree@vger.kernel.org>
References: <20231017200109.11407-1-quic_wcheng@quicinc.com>
 <20231017200109.11407-10-quic_wcheng@quicinc.com>
 <7aa4ea87-9d1f-400a-bcc5-b56e5b4500c6@linux.intel.com>
From:   Wesley Cheng <quic_wcheng@quicinc.com>
In-Reply-To: <7aa4ea87-9d1f-400a-bcc5-b56e5b4500c6@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Pierre,

On 10/17/2023 2:32 PM, Pierre-Louis Bossart wrote:
> 
> 
> On 10/17/23 15:00, Wesley Cheng wrote:
>> The QC ADSP is able to support USB playback endpoints, so that the main
> 
> playback only?
> 

Correct, playback only at this time.

>> application processor can be placed into lower CPU power modes.  This adds
>> the required AFE port configurations and port start command to start an
>> audio session.
>>
>> Specifically, the QC ADSP can support all potential endpoints that are
>> exposed by the audio data interface.  This includes, feedback endpoints
>> (both implicit and explicit) as well as the isochronous (data) endpoints.
> 
> implicit feedback means support for capture. This is confusing...
> 

I mean, a USB device can expose a capture path, but as of now, we won't 
enable the offloading to the audio DSP for it.  However, if we're 
executing playback, and device does support implicit feedback, we will 
pass that along to the audio DSP to utilize.

>> +static int q6usb_hw_params(struct snd_pcm_substream *substream,
>> +			   struct snd_pcm_hw_params *params,
>> +			   struct snd_soc_dai *dai)
>> +{
>> +	struct q6afe_dai_data *dai_data = dev_get_drvdata(dai->dev);
>> +	int channels = params_channels(params);
>> +	int rate = params_rate(params);
>> +	struct q6afe_usb_cfg *usb = &dai_data->port_config[dai->id].usb_audio;
>> +
>> +	usb->sample_rate = rate;
>> +	usb->num_channels = channels;
>> +
>> +	switch (params_format(params)) {
>> +	case SNDRV_PCM_FORMAT_U16_LE:
>> +	case SNDRV_PCM_FORMAT_S16_LE:
>> +	case SNDRV_PCM_FORMAT_SPECIAL:
> 
> what does FORMAT_SPECIAL mean? the only other reference I see to this is
> related to SLIMbus, not sure how this is related?
> 

Thanks for catching this.  It shouldn't be included in this path.

>> +		usb->bit_width = 16;
>> +		break;
>> +	case SNDRV_PCM_FORMAT_S24_LE:
>> +	case SNDRV_PCM_FORMAT_S24_3LE:
>> +		usb->bit_width = 24;
>> +		break;
>> +	case SNDRV_PCM_FORMAT_S32_LE:
>> +		usb->bit_width = 32;
>> +		break;
>> +	default:
>> +		dev_err(dai->dev, "%s: invalid format %d\n",
>> +			__func__, params_format(params));
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
> 
>> @@ -617,6 +655,9 @@ static const struct snd_soc_dapm_route q6afe_dapm_routes[] = {
>>   	{"TX_CODEC_DMA_TX_5", NULL, "TX_CODEC_DMA_TX_5 Capture"},
>>   	{"RX_CODEC_DMA_RX_6 Playback", NULL, "RX_CODEC_DMA_RX_6"},
>>   	{"RX_CODEC_DMA_RX_7 Playback", NULL, "RX_CODEC_DMA_RX_7"},
>> +
>> +	/* USB playback AFE port receives data for playback, hence use the RX port */
>> +	{"USB Playback", NULL, "USB_RX"},
> 
> Capture for implicit feedback?
> 

Please refer to the above comment.

>>   };
>>   
>>   static int msm_dai_q6_dai_probe(struct snd_soc_dai *dai)
>> @@ -644,6 +685,18 @@ static int msm_dai_q6_dai_remove(struct snd_soc_dai *dai)
>>   	return 0;
>>   }
>>   
>> +static const struct snd_soc_dai_ops q6usb_ops = {
>> +	.probe		= msm_dai_q6_dai_probe,
>> +	.prepare	= q6afe_dai_prepare,
>> +	.hw_params	= q6usb_hw_params,
> 
> this is rather confusing with two different layers used for hw_params
> and prepare? Additional comments or explanations wouldn't hurt.
> 

I thought this was how the ASoC design was.  Each DAI defined for a 
particular path has it own set of callbacks implemented to bring up any 
required resources for that entity.  So in this case, it initializes the 
"cpu" DAI, which is the main component that handles communication with 
the audio DSP.

>> +	.shutdown	= q6afe_dai_shutdown,
>> +	/*
>> +	 * Startup callback not needed, as AFE port start command passes the PCM
>> +	 * parameters within the AFE command, which is provided by the PCM core
>> +	 * during the prepare() stage.
> 
> This doesn't really explain why you need a shutdown?
> 
> 

Sure, I'll add a comment.  shutdown() is needed to actually issue a AFE 
port stop command to stop pumping audio data on a particular AFE port. 
This occurs when userspace closes the PCM device for the platform sound 
card, and is triggered for all linked DAIs.

>> + * struct afe_param_id_usb_audio_dev_latency_mode
>> + * @cfg_minor_version: Minor version used for tracking USB audio device
>> + * configuration.
>> + * Supported values:
>> + *     AFE_API_MINOR_VERSION_USB_AUDIO_LATENCY_MODE
>> + * @mode: latency mode for the USB audio device
> 
> what are the different latency modes? and is this related to the latency
> reporting that was added in the USB2 audio class IIRC?
> 

Must've missed removing this part during one of the earlier revision 
cleanups I had done.  We aren't setting this parameter currently on the 
AFE side, and it isn't utilized either in the audio DSP, so I will 
remove this definition.

>> +static int afe_port_send_usb_dev_param(struct q6afe_port *port, struct q6afe_usb_cfg *cfg)
>> +{
>> +	union afe_port_config *pcfg = &port->port_cfg;
>> +	struct afe_param_id_usb_audio_dev_params usb_dev;
>> +	struct afe_param_id_usb_audio_dev_lpcm_fmt lpcm_fmt;
>> +	struct afe_param_id_usb_audio_svc_interval svc_int;
>> +	int ret = 0;
> 
> useless init overridden...

Will fix this.

>> +
>> +	if (!pcfg) {
>> +		dev_err(port->afe->dev, "%s: Error, no configuration data\n", __func__);
>> +		ret = -EINVAL;
>> +		goto exit;
>> +	}
>> +
>> +	memset(&usb_dev, 0, sizeof(usb_dev));
>> +	memset(&lpcm_fmt, 0, sizeof(lpcm_fmt));
>> +	memset(&svc_int, 0, sizeof(svc_int));
>> +
>> +	usb_dev.cfg_minor_version = AFE_API_MINOR_VERSION_USB_AUDIO_CONFIG;
>> +	ret = q6afe_port_set_param_v2(port, &usb_dev,
> 
> .... here
> 
>> +				      AFE_PARAM_ID_USB_AUDIO_DEV_PARAMS,
>> +				      AFE_MODULE_AUDIO_DEV_INTERFACE, sizeof(usb_dev));
>> +	if (ret) {
>> +		dev_err(port->afe->dev, "%s: AFE device param cmd failed %d\n",
>> +			__func__, ret);
>> +		goto exit;
>> +	}
>> +
>> +	lpcm_fmt.cfg_minor_version = AFE_API_MINOR_VERSION_USB_AUDIO_CONFIG;
>> +	lpcm_fmt.endian = pcfg->usb_cfg.endian;
>> +	ret = q6afe_port_set_param_v2(port, &lpcm_fmt,
>> +				      AFE_PARAM_ID_USB_AUDIO_DEV_LPCM_FMT,
>> +				      AFE_MODULE_AUDIO_DEV_INTERFACE, sizeof(lpcm_fmt));
>> +	if (ret) {
>> +		dev_err(port->afe->dev, "%s: AFE device param cmd LPCM_FMT failed %d\n",
>> +			__func__, ret);
>> +		goto exit;
>> +	}
>> +
>> +	svc_int.cfg_minor_version = AFE_API_MINOR_VERSION_USB_AUDIO_CONFIG;
>> +	svc_int.svc_interval = pcfg->usb_cfg.service_interval;
>> +	ret = q6afe_port_set_param_v2(port, &svc_int,
>> +				      AFE_PARAM_ID_USB_AUDIO_SVC_INTERVAL,
>> +				      AFE_MODULE_AUDIO_DEV_INTERFACE, sizeof(svc_int));
>> +	if (ret)
>> +		dev_err(port->afe->dev, "%s: AFE device param cmd svc_interval failed %d\n",
>> +			__func__, ret);
>> +
>> +exit:
>> +	return ret;
>> +}
> 
>> -#define AFE_PORT_MAX		129
>> +#define AFE_PORT_MAX		137
> 
> does this mean 8 ports are reserved for USB?
> 
> Or is this 137 just a random index coming from the AFE design?
> 
> 

Its the latter.  Each port has a defined number/ID on the audio DSP AFE end.

Thanks
Wesley Cheng