Re: [PATCH V2 3/8] soundwire: amd: register soundwire manager dai ops

[Mukunda,Vijendar]

On 13/02/23 23:39, Pierre-Louis Bossart wrote:
>
> On 2/13/23 03:40, Vijendar Mukunda wrote:
>> Register dai ops for soundwire manager instances.
>>
>> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@xxxxxxx>
>> ---
>>  drivers/soundwire/amd_manager.c   | 182 ++++++++++++++++++++++++++++++
>>  drivers/soundwire/amd_manager.h   |  18 +++
>>  include/linux/soundwire/sdw_amd.h |  18 +++
>>  3 files changed, 218 insertions(+)
>>
>> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
>> index cd1e5a3d5995..14c88b80ab6d 100644
>> --- a/drivers/soundwire/amd_manager.c
>> +++ b/drivers/soundwire/amd_manager.c
>> @@ -641,6 +641,182 @@ static const struct sdw_master_ops amd_sdw_ops = {
>>  	.read_ping_status = amd_sdw_read_ping_status,
>>  };
>>  
>> +static int amd_sdw_hw_params(struct snd_pcm_substream *substream,
>> +			     struct snd_pcm_hw_params *params,
>> +			     struct snd_soc_dai *dai)
>> +{
>> +	struct amd_sdw_manager *amd_manager = snd_soc_dai_get_drvdata(dai);
>> +	struct sdw_amd_dai_runtime *dai_runtime;
>> +	struct sdw_stream_config sconfig;
>> +	struct sdw_port_config *pconfig;
>> +	int ch, dir;
>> +	int ret;
>> +
>> +	dai_runtime = amd_manager->dai_runtime_array[dai->id];
>> +	if (!dai_runtime)
>> +		return -EIO;
>> +
>> +	ch = params_channels(params);
>> +	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
>> +		dir = SDW_DATA_DIR_RX;
>> +	else
>> +		dir = SDW_DATA_DIR_TX;
>> +	dev_dbg(amd_manager->dev, "%s: dir:%d dai->id:0x%x\n", __func__, dir, dai->id);
>> +
>> +	sconfig.direction = dir;
>> +	sconfig.ch_count = ch;
>> +	sconfig.frame_rate = params_rate(params);
>> +	sconfig.type = dai_runtime->stream_type;
>> +
>> +	sconfig.bps = snd_pcm_format_width(params_format(params));
>> +
>> +	/* Port configuration */
>> +	pconfig = kzalloc(sizeof(*pconfig), GFP_KERNEL);
>> +	if (!pconfig) {
>> +		ret =  -ENOMEM;
>> +		goto error;
>> +	}
>> +
>> +	pconfig->num = dai->id;
>> +	pconfig->ch_mask = (1 << ch) - 1;
>> +	ret = sdw_stream_add_master(&amd_manager->bus, &sconfig,
>> +				    pconfig, 1, dai_runtime->stream);
>> +	if (ret)
>> +		dev_err(amd_manager->dev, "add manager to stream failed:%d\n", ret);
>> +
>> +	kfree(pconfig);
>> +error:
>> +	return ret;
>> +}
>> +
>> +static int amd_sdw_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dai *dai)
>> +{
>> +	struct amd_sdw_manager *amd_manager = snd_soc_dai_get_drvdata(dai);
>> +	struct sdw_amd_dai_runtime *dai_runtime;
>> +	int ret;
>> +
>> +	dai_runtime = amd_manager->dai_runtime_array[dai->id];
>> +	if (!dai_runtime)
>> +		return -EIO;
>> +
>> +	ret = sdw_stream_remove_master(&amd_manager->bus, dai_runtime->stream);
>> +	if (ret < 0)
>> +		dev_err(dai->dev, "remove manager from stream %s failed: %d\n",
>> +			dai_runtime->stream->name, ret);
>> +	return ret;
>> +}
>> +
>> +static int amd_set_sdw_stream(struct snd_soc_dai *dai, void *stream, int direction)
>> +{
>> +	struct amd_sdw_manager *amd_manager = snd_soc_dai_get_drvdata(dai);
>> +	struct sdw_amd_dai_runtime *dai_runtime;
>> +
>> +	dai_runtime = amd_manager->dai_runtime_array[dai->id];
>> +	if (stream) {
>> +		/* first paranoia check */
>> +		if (dai_runtime) {
>> +			dev_err(dai->dev,
>> +				"dai_runtime already allocated for dai %s\n",
>> +				dai->name);
>> +			return -EINVAL;
>> +		}
>> +
>> +		/* allocate and set dai_runtime info */
>> +		dai_runtime = kzalloc(sizeof(*dai_runtime), GFP_KERNEL);
>> +		if (!dai_runtime)
>> +			return -ENOMEM;
>> +
>> +		dai_runtime->stream_type = SDW_STREAM_PCM;
>> +		dai_runtime->bus = &amd_manager->bus;
>> +		dai_runtime->stream = stream;
>> +		amd_manager->dai_runtime_array[dai->id] = dai_runtime;
>> +	} else {
>> +		/* second paranoia check */
>> +		if (!dai_runtime) {
>> +			dev_err(dai->dev,
>> +				"dai_runtime not allocated for dai %s\n",
>> +				dai->name);
>> +			return -EINVAL;
>> +		}
>> +
>> +		/* for NULL stream we release allocated dai_runtime */
>> +		kfree(dai_runtime);
>> +		amd_manager->dai_runtime_array[dai->id] = NULL;
>> +	}
>> +	return 0;
>> +}
>> +
>> +static int amd_pcm_set_sdw_stream(struct snd_soc_dai *dai, void *stream, int direction)
>> +{
>> +	return amd_set_sdw_stream(dai, stream, direction);
>> +}
>> +
>> +static void *amd_get_sdw_stream(struct snd_soc_dai *dai, int direction)
>> +{
>> +	struct amd_sdw_manager *amd_manager = snd_soc_dai_get_drvdata(dai);
>> +	struct sdw_amd_dai_runtime *dai_runtime;
>> +
>> +	dai_runtime = amd_manager->dai_runtime_array[dai->id];
>> +	if (!dai_runtime)
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	return dai_runtime->stream;
>> +}
>> +
>> +static const struct snd_soc_dai_ops amd_sdw_dai_ops = {
>> +	.hw_params = amd_sdw_hw_params,
>> +	.hw_free = amd_sdw_hw_free,
>> +	.set_stream = amd_pcm_set_sdw_stream,
>> +	.get_stream = amd_get_sdw_stream,
>> +};
>> +
>> +static const struct snd_soc_component_driver amd_sdw_dai_component = {
>> +	.name = "soundwire",
>> +};
>> +
>> +static int amd_sdw_register_dais(struct amd_sdw_manager *amd_manager)
>> +{
>> +	struct sdw_amd_dai_runtime **dai_runtime_array;
>> +	struct snd_soc_dai_driver *dais;
>> +	struct snd_soc_pcm_stream *stream;
>> +	struct device *dev;
>> +	int i, num_dais;
>> +
>> +	dev = amd_manager->dev;
>> +	num_dais = amd_manager->num_dout_ports + amd_manager->num_din_ports;
>> +	dais = devm_kcalloc(dev, num_dais, sizeof(*dais), GFP_KERNEL);
>> +	if (!dais)
>> +		return -ENOMEM;
>> +
>> +	dai_runtime_array = devm_kcalloc(dev, num_dais,
>> +					 sizeof(struct sdw_amd_dai_runtime *),
>> +					 GFP_KERNEL);
>> +	if (!dai_runtime_array)
>> +		return -ENOMEM;
>> +	amd_manager->dai_runtime_array = dai_runtime_array;
>> +	for (i = 0; i < num_dais; i++) {
>> +		dais[i].name = devm_kasprintf(dev, GFP_KERNEL, "SDW%d Pin%d", amd_manager->instance,
>> +					      i);
>> +		if (!dais[i].name)
>> +			return -ENOMEM;
>> +		if (i < amd_manager->num_dout_ports)
>> +			stream = &dais[i].playback;
>> +		else
>> +			stream = &dais[i].capture;
>> +
>> +		stream->channels_min = 2;
>> +		stream->channels_max = 2;
>> +		stream->rates = SNDRV_PCM_RATE_48000;
>> +		stream->formats = SNDRV_PCM_FMTBIT_S16_LE;
> do you have a restriction on formats here? IIRC we've removed this from
> the Intel code.
Currently, we have implemented stack for legacy driver(where DSP is disabled).
If we didn't include formats, rates, It's going to break in soc_hw_sainty_check() function
throwing config_err.





>> +
>> +		dais[i].ops = &amd_sdw_dai_ops;
>> +		dais[i].id = i;
>> +	}
>> +
>> +	return devm_snd_soc_register_component(dev, &amd_sdw_dai_component,
>> +					       dais, num_dais);
>> +}
>> +
>>  static void amd_sdw_probe_work(struct work_struct *work)
>>  {
>>  	struct amd_sdw_manager *amd_manager = container_of(work, struct amd_sdw_manager,
>> @@ -726,6 +902,12 @@ static int amd_sdw_manager_probe(struct platform_device *pdev)
>>  		dev_err(dev, "Failed to register Soundwire manager(%d)\n", ret);
>>  		return ret;
>>  	}
>> +	ret = amd_sdw_register_dais(amd_manager);
>> +	if (ret) {
>> +		dev_err(dev, "CPU DAI registration failed\n");
>> +		sdw_bus_master_delete(&amd_manager->bus);
>> +		return ret;
>> +	}
>>  	dev_set_drvdata(dev, amd_manager);
>>  	INIT_WORK(&amd_manager->probe_work, amd_sdw_probe_work);
>>  	schedule_work(&amd_manager->probe_work);
>> diff --git a/drivers/soundwire/amd_manager.h b/drivers/soundwire/amd_manager.h
>> index 811ed9ee3d86..3e1bded1e769 100644
>> --- a/drivers/soundwire/amd_manager.h
>> +++ b/drivers/soundwire/amd_manager.h
>> @@ -205,6 +205,24 @@ struct sdw_manager_dp_reg {
>>  	u32 lane_ctrl_ch_en_reg;
>>  };
>>  
>> +/*
>> + * SDW0 Manager instance registers  6 CPU DAI (3 TX & 3 RX Ports)
>> + * whereas SDW1  Manager Instance registers 2 CPU DAI (one TX & one RX port)
>> + * Below is the CPU DAI <->Manager port number mapping
>> + * i.e SDW0 Pin0 -> port number 0 -> AUDIO0 TX
>> + *     SDW0 Pin1 -> Port number 1 -> AUDIO1 TX
>> + *     SDW0 Pin2 -> Port number 2 -> AUDIO2 TX
>> + *     SDW0 Pin3 -> port number 3 -> AUDIO0 RX
>> + *     SDW0 Pin4 -> Port number 4 -> AUDIO1 RX
>> + *     SDW0 Pin5 -> Port number 5 -> AUDIO2 RX
>> + *  Whereas for SDW1 instance
>> + *  SDW1 Pin0 -> port number 0 -> AUDIO1 TX
>> + *  SDW1 Pin1 -> Port number 1 -> AUDIO1 RX
>> + *  Same mapping should be used for programming DMA controller registers in Soundwire DMA driver.
>> + * i.e if AUDIO0 TX channel is selected then we need to use AUDIO0 TX registers for DMA programming
>> + * in Soundwire DMA driver.
>> + */
>> +
>>  static struct sdw_manager_dp_reg sdw0_manager_dp_reg[AMD_SDW0_MAX_DAI] =  {
>>  	{ACP_SW_AUDIO0_TX_FRAME_FORMAT, ACP_SW_AUDIO0_TX_SAMPLEINTERVAL, ACP_SW_AUDIO0_TX_HCTRL_DP0,
>>  	 ACP_SW_AUDIO0_TX_OFFSET_DP0, ACP_SW_AUDIO0_TX_CHANNEL_ENABLE_DP0},
>> diff --git a/include/linux/soundwire/sdw_amd.h b/include/linux/soundwire/sdw_amd.h
>> index 922d30a540fd..c978cfbc0207 100644
>> --- a/include/linux/soundwire/sdw_amd.h
>> +++ b/include/linux/soundwire/sdw_amd.h
>> @@ -23,6 +23,21 @@ struct sdw_manager_reg_mask {
>>  	u32 acp_sdw_intr_mask;
>>  };
>>  
>> +/**
>> + * struct sdw_amd_dai_runtime: AMD sdw dai runtime  data
>> + *
>> + * @name: SoundWire stream name
>> + * @stream: stream runtime
>> + * @bus: Bus handle
>> + * @stream_type: Stream type
>> + */
>> +struct sdw_amd_dai_runtime {
>> +	char *name;
>> +	struct sdw_stream_runtime *stream;
>> +	struct sdw_bus *bus;
>> +	enum sdw_stream_type stream_type;
>> +};
> This seems pretty generic non-AMD code, and all the dai registration is
> also 'inspired' from the Intel code, seems like there's room for code
> sharing/reuse.
In V1 version patch set, we have used sdw_stream_runtime structure
and sdw_amd_dma_data structure. As per review comment,
we have used runtime dai array.

This can't be generic. We are going to modify this structure when we migrate
to SOF based solution. Intel code has its own structure members apart from above
mentioned fields in dai runtime structure.
>
>> +
>>  /**
>>   * struct amd_sdw_manager - amd manager driver context
>>   * @bus: bus handle
>> @@ -40,6 +55,7 @@ struct sdw_manager_reg_mask {
>>   * @quirks: soundwire manager quirks
>>   * @wake_en_mask: wake enable mask per soundwire manager
>>   * @power_mode_mask: flag interprets amd soundwire manager power mode
>> + * @dai_runtime_array: dai runtime array
>>   */
>>  struct amd_sdw_manager {
>>  	struct sdw_bus bus;
>> @@ -63,5 +79,7 @@ struct amd_sdw_manager {
>>  	u32 quirks;
>>  	u32 wake_en_mask;
>>  	u32 power_mode_mask;
>> +
>> +	struct sdw_amd_dai_runtime **dai_runtime_array;
>>  };
>>  #endif