[PATCH 0/2] soundwire: cadence: remove dma_data
The use of dma_data is problematic for two reasons for the Cadence IP. a) the dai runtime data has nothing to do with DMAs in existing solutions b) we will use the dma_data for DMA-management in the future. We cannot share two separate pieces of information with the same dma_data pointer.
Pierre-Louis Bossart (2): soundwire: cadence: rename sdw_cdns_dai_dma_data as sdw_cdns_dai_runtime soundwire: cadence: use dai_runtime_array instead of dma_data
drivers/soundwire/cadence_master.c | 50 +++++++------ drivers/soundwire/cadence_master.h | 9 ++- drivers/soundwire/intel.c | 111 ++++++++++++++--------------- 3 files changed, 86 insertions(+), 84 deletions(-)
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
The existing 'struct sdw_cdns_dma_data' has really nothing to do with DMAs. The information is stored in the dai->dma_data, but this is really private data that should be stored in a different context.
Beyond the academic elegance discussion, using dma_data is a problem for new Intel hardware where the dma_data structure is already used for true DMA handling performed by other parts of the code.
This patch prepares a transition away from the use of dma_data, for now with a rename-only change.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Péter Ujfalusi peter.ujfalusi@linux.intel.com Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com --- drivers/soundwire/cadence_master.c | 30 +++++----- drivers/soundwire/cadence_master.h | 4 +- drivers/soundwire/intel.c | 96 +++++++++++++++--------------- 3 files changed, 65 insertions(+), 65 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 93929f19d083..235617b0542f 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -1707,40 +1707,40 @@ int cdns_set_sdw_stream(struct snd_soc_dai *dai, void *stream, int direction) { struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai); - struct sdw_cdns_dma_data *dma; + struct sdw_cdns_dai_runtime *dai_runtime;
if (stream) { /* first paranoia check */ if (direction == SNDRV_PCM_STREAM_PLAYBACK) - dma = dai->playback_dma_data; + dai_runtime = dai->playback_dma_data; else - dma = dai->capture_dma_data; + dai_runtime = dai->capture_dma_data;
- if (dma) { + if (dai_runtime) { dev_err(dai->dev, - "dma_data already allocated for dai %s\n", + "dai_runtime already allocated for dai %s\n", dai->name); return -EINVAL; }
- /* allocate and set dma info */ - dma = kzalloc(sizeof(*dma), GFP_KERNEL); - if (!dma) + /* allocate and set dai_runtime info */ + dai_runtime = kzalloc(sizeof(*dai_runtime), GFP_KERNEL); + if (!dai_runtime) return -ENOMEM;
- dma->stream_type = SDW_STREAM_PCM; + dai_runtime->stream_type = SDW_STREAM_PCM;
- dma->bus = &cdns->bus; - dma->link_id = cdns->instance; + dai_runtime->bus = &cdns->bus; + dai_runtime->link_id = cdns->instance;
- dma->stream = stream; + dai_runtime->stream = stream;
if (direction == SNDRV_PCM_STREAM_PLAYBACK) - dai->playback_dma_data = dma; + dai->playback_dma_data = dai_runtime; else - dai->capture_dma_data = dma; + dai->capture_dma_data = dai_runtime; } else { - /* for NULL stream we release allocated dma_data */ + /* for NULL stream we release allocated dai_runtime */ if (direction == SNDRV_PCM_STREAM_PLAYBACK) { kfree(dai->playback_dma_data); dai->playback_dma_data = NULL; diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h index ca9e805bab88..93f23bd46e2c 100644 --- a/drivers/soundwire/cadence_master.h +++ b/drivers/soundwire/cadence_master.h @@ -70,7 +70,7 @@ struct sdw_cdns_stream_config { };
/** - * struct sdw_cdns_dma_data: Cadence DMA data + * struct sdw_cdns_dai_runtime: Cadence DAI runtime data * * @name: SoundWire stream name * @stream: stream runtime @@ -82,7 +82,7 @@ struct sdw_cdns_stream_config { * @suspended: status set when suspended, to be used in .prepare * @paused: status set in .trigger, to be used in suspend */ -struct sdw_cdns_dma_data { +struct sdw_cdns_dai_runtime { char *name; struct sdw_stream_runtime *stream; struct sdw_cdns_pdi *pdi; diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 244209358784..1e9c6df4b62c 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -824,15 +824,15 @@ static int intel_hw_params(struct snd_pcm_substream *substream, { struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai); struct sdw_intel *sdw = cdns_to_intel(cdns); - struct sdw_cdns_dma_data *dma; + struct sdw_cdns_dai_runtime *dai_runtime; struct sdw_cdns_pdi *pdi; struct sdw_stream_config sconfig; struct sdw_port_config *pconfig; int ch, dir; int ret;
- dma = snd_soc_dai_get_dma_data(dai, substream); - if (!dma) + dai_runtime = snd_soc_dai_get_dma_data(dai, substream); + if (!dai_runtime) return -EIO;
ch = params_channels(params); @@ -854,10 +854,10 @@ static int intel_hw_params(struct snd_pcm_substream *substream, sdw_cdns_config_stream(cdns, ch, dir, pdi);
/* store pdi and hw_params, may be needed in prepare step */ - dma->paused = false; - dma->suspended = false; - dma->pdi = pdi; - dma->hw_params = params; + dai_runtime->paused = false; + dai_runtime->suspended = false; + dai_runtime->pdi = pdi; + dai_runtime->hw_params = params;
/* Inform DSP about PDI stream number */ ret = intel_params_stream(sdw, substream->stream, dai, params, @@ -869,7 +869,7 @@ static int intel_hw_params(struct snd_pcm_substream *substream, sconfig.direction = dir; sconfig.ch_count = ch; sconfig.frame_rate = params_rate(params); - sconfig.type = dma->stream_type; + sconfig.type = dai_runtime->stream_type;
sconfig.bps = snd_pcm_format_width(params_format(params));
@@ -884,7 +884,7 @@ static int intel_hw_params(struct snd_pcm_substream *substream, pconfig->ch_mask = (1 << ch) - 1;
ret = sdw_stream_add_master(&cdns->bus, &sconfig, - pconfig, 1, dma->stream); + pconfig, 1, dai_runtime->stream); if (ret) dev_err(cdns->dev, "add master to stream failed:%d\n", ret);
@@ -898,19 +898,19 @@ static int intel_prepare(struct snd_pcm_substream *substream, { struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai); struct sdw_intel *sdw = cdns_to_intel(cdns); - struct sdw_cdns_dma_data *dma; + struct sdw_cdns_dai_runtime *dai_runtime; int ch, dir; int ret = 0;
- dma = snd_soc_dai_get_dma_data(dai, substream); - if (!dma) { - dev_err(dai->dev, "failed to get dma data in %s\n", + dai_runtime = snd_soc_dai_get_dma_data(dai, substream); + if (!dai_runtime) { + dev_err(dai->dev, "failed to get dai runtime in %s\n", __func__); return -EIO; }
- if (dma->suspended) { - dma->suspended = false; + if (dai_runtime->suspended) { + dai_runtime->suspended = false;
/* * .prepare() is called after system resume, where we @@ -921,21 +921,21 @@ static int intel_prepare(struct snd_pcm_substream *substream, */
/* configure stream */ - ch = params_channels(dma->hw_params); + ch = params_channels(dai_runtime->hw_params); if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) dir = SDW_DATA_DIR_RX; else dir = SDW_DATA_DIR_TX;
- intel_pdi_shim_configure(sdw, dma->pdi); - intel_pdi_alh_configure(sdw, dma->pdi); - sdw_cdns_config_stream(cdns, ch, dir, dma->pdi); + intel_pdi_shim_configure(sdw, dai_runtime->pdi); + intel_pdi_alh_configure(sdw, dai_runtime->pdi); + sdw_cdns_config_stream(cdns, ch, dir, dai_runtime->pdi);
/* Inform DSP about PDI stream number */ ret = intel_params_stream(sdw, substream->stream, dai, - dma->hw_params, + dai_runtime->hw_params, sdw->instance, - dma->pdi->intel_alh_id); + dai_runtime->pdi->intel_alh_id); }
return ret; @@ -946,11 +946,11 @@ intel_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai); struct sdw_intel *sdw = cdns_to_intel(cdns); - struct sdw_cdns_dma_data *dma; + struct sdw_cdns_dai_runtime *dai_runtime; int ret;
- dma = snd_soc_dai_get_dma_data(dai, substream); - if (!dma) + dai_runtime = snd_soc_dai_get_dma_data(dai, substream); + if (!dai_runtime) return -EIO;
/* @@ -959,10 +959,10 @@ intel_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) * DEPREPARED for the first cpu-dai and to RELEASED for the last * cpu-dai. */ - ret = sdw_stream_remove_master(&cdns->bus, dma->stream); + ret = sdw_stream_remove_master(&cdns->bus, dai_runtime->stream); if (ret < 0) { dev_err(dai->dev, "remove master from stream %s failed: %d\n", - dma->stream->name, ret); + dai_runtime->stream->name, ret); return ret; }
@@ -972,8 +972,8 @@ intel_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) return ret; }
- dma->hw_params = NULL; - dma->pdi = NULL; + dai_runtime->hw_params = NULL; + dai_runtime->pdi = NULL;
return 0; } @@ -996,17 +996,17 @@ static int intel_pcm_set_sdw_stream(struct snd_soc_dai *dai, static void *intel_get_sdw_stream(struct snd_soc_dai *dai, int direction) { - struct sdw_cdns_dma_data *dma; + struct sdw_cdns_dai_runtime *dai_runtime;
if (direction == SNDRV_PCM_STREAM_PLAYBACK) - dma = dai->playback_dma_data; + dai_runtime = dai->playback_dma_data; else - dma = dai->capture_dma_data; + dai_runtime = dai->capture_dma_data;
- if (!dma) + if (!dai_runtime) return ERR_PTR(-EINVAL);
- return dma->stream; + return dai_runtime->stream; }
static int intel_trigger(struct snd_pcm_substream *substream, int cmd, struct snd_soc_dai *dai) @@ -1014,7 +1014,7 @@ static int intel_trigger(struct snd_pcm_substream *substream, int cmd, struct sn struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai); struct sdw_intel *sdw = cdns_to_intel(cdns); struct sdw_intel_link_res *res = sdw->link_res; - struct sdw_cdns_dma_data *dma; + struct sdw_cdns_dai_runtime *dai_runtime; int ret = 0;
/* @@ -1025,9 +1025,9 @@ static int intel_trigger(struct snd_pcm_substream *substream, int cmd, struct sn if (res->ops && res->ops->trigger) res->ops->trigger(dai, cmd, substream->stream);
- dma = snd_soc_dai_get_dma_data(dai, substream); - if (!dma) { - dev_err(dai->dev, "failed to get dma data in %s\n", + dai_runtime = snd_soc_dai_get_dma_data(dai, substream); + if (!dai_runtime) { + dev_err(dai->dev, "failed to get dai runtime in %s\n", __func__); return -EIO; } @@ -1042,17 +1042,17 @@ static int intel_trigger(struct snd_pcm_substream *substream, int cmd, struct sn * the .trigger callback is used to track the suspend case only. */
- dma->suspended = true; + dai_runtime->suspended = true;
ret = intel_free_stream(sdw, substream->stream, dai, sdw->instance); break;
case SNDRV_PCM_TRIGGER_PAUSE_PUSH: - dma->paused = true; + dai_runtime->paused = true; break; case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: - dma->paused = false; + dai_runtime->paused = false; break; default: break; @@ -1091,25 +1091,25 @@ static int intel_component_dais_suspend(struct snd_soc_component *component) for_each_component_dais(component, dai) { struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai); struct sdw_intel *sdw = cdns_to_intel(cdns); - struct sdw_cdns_dma_data *dma; + struct sdw_cdns_dai_runtime *dai_runtime; int stream; int ret;
- dma = dai->playback_dma_data; + dai_runtime = dai->playback_dma_data; stream = SNDRV_PCM_STREAM_PLAYBACK; - if (!dma) { - dma = dai->capture_dma_data; + if (!dai_runtime) { + dai_runtime = dai->capture_dma_data; stream = SNDRV_PCM_STREAM_CAPTURE; }
- if (!dma) + if (!dai_runtime) continue;
- if (dma->suspended) + if (dai_runtime->suspended) continue;
- if (dma->paused) { - dma->suspended = true; + if (dai_runtime->paused) { + dai_runtime->suspended = true;
ret = intel_free_stream(sdw, stream, dai, sdw->instance); if (ret < 0)
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
Simplify the code with a Cadence-specific dai_runtime_array, indexed with dai->id, instead of abusing dma_data.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Péter Ujfalusi peter.ujfalusi@linux.intel.com Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com --- drivers/soundwire/cadence_master.c | 30 +++++++++++++-------------- drivers/soundwire/cadence_master.h | 5 +++++ drivers/soundwire/intel.c | 33 +++++++++++++++--------------- 3 files changed, 35 insertions(+), 33 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 235617b0542f..a1de363eba3f 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -1709,13 +1709,10 @@ int cdns_set_sdw_stream(struct snd_soc_dai *dai, struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai); struct sdw_cdns_dai_runtime *dai_runtime;
+ dai_runtime = cdns->dai_runtime_array[dai->id]; + if (stream) { /* first paranoia check */ - if (direction == SNDRV_PCM_STREAM_PLAYBACK) - dai_runtime = dai->playback_dma_data; - else - dai_runtime = dai->capture_dma_data; - if (dai_runtime) { dev_err(dai->dev, "dai_runtime already allocated for dai %s\n", @@ -1734,20 +1731,21 @@ int cdns_set_sdw_stream(struct snd_soc_dai *dai, dai_runtime->link_id = cdns->instance;
dai_runtime->stream = stream; + dai_runtime->direction = direction;
- if (direction == SNDRV_PCM_STREAM_PLAYBACK) - dai->playback_dma_data = dai_runtime; - else - dai->capture_dma_data = dai_runtime; + cdns->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 */ - if (direction == SNDRV_PCM_STREAM_PLAYBACK) { - kfree(dai->playback_dma_data); - dai->playback_dma_data = NULL; - } else { - kfree(dai->capture_dma_data); - dai->capture_dma_data = NULL; - } + kfree(dai_runtime); + cdns->dai_runtime_array[dai->id] = NULL; } return 0; } diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h index 93f23bd46e2c..0434d70d4b1f 100644 --- a/drivers/soundwire/cadence_master.h +++ b/drivers/soundwire/cadence_master.h @@ -81,6 +81,7 @@ struct sdw_cdns_stream_config { * @hw_params: hw_params to be applied in .prepare step * @suspended: status set when suspended, to be used in .prepare * @paused: status set in .trigger, to be used in suspend + * @direction: stream direction */ struct sdw_cdns_dai_runtime { char *name; @@ -92,6 +93,7 @@ struct sdw_cdns_dai_runtime { struct snd_pcm_hw_params *hw_params; bool suspended; bool paused; + int direction; };
/** @@ -108,6 +110,7 @@ struct sdw_cdns_dai_runtime { * @registers: Cadence registers * @link_up: Link status * @msg_count: Messages sent on bus + * @dai_runtime_array: runtime context for each allocated DAI. */ struct sdw_cdns { struct device *dev; @@ -135,6 +138,8 @@ struct sdw_cdns { struct work_struct work;
struct list_head list; + + struct sdw_cdns_dai_runtime **dai_runtime_array; };
#define bus_to_cdns(_bus) container_of(_bus, struct sdw_cdns, bus) diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 1e9c6df4b62c..e8855a2115f6 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -831,7 +831,7 @@ static int intel_hw_params(struct snd_pcm_substream *substream, int ch, dir; int ret;
- dai_runtime = snd_soc_dai_get_dma_data(dai, substream); + dai_runtime = cdns->dai_runtime_array[dai->id]; if (!dai_runtime) return -EIO;
@@ -902,7 +902,7 @@ static int intel_prepare(struct snd_pcm_substream *substream, int ch, dir; int ret = 0;
- dai_runtime = snd_soc_dai_get_dma_data(dai, substream); + dai_runtime = cdns->dai_runtime_array[dai->id]; if (!dai_runtime) { dev_err(dai->dev, "failed to get dai runtime in %s\n", __func__); @@ -949,7 +949,7 @@ intel_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) struct sdw_cdns_dai_runtime *dai_runtime; int ret;
- dai_runtime = snd_soc_dai_get_dma_data(dai, substream); + dai_runtime = cdns->dai_runtime_array[dai->id]; if (!dai_runtime) return -EIO;
@@ -996,13 +996,10 @@ static int intel_pcm_set_sdw_stream(struct snd_soc_dai *dai, static void *intel_get_sdw_stream(struct snd_soc_dai *dai, int direction) { + struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai); struct sdw_cdns_dai_runtime *dai_runtime;
- if (direction == SNDRV_PCM_STREAM_PLAYBACK) - dai_runtime = dai->playback_dma_data; - else - dai_runtime = dai->capture_dma_data; - + dai_runtime = cdns->dai_runtime_array[dai->id]; if (!dai_runtime) return ERR_PTR(-EINVAL);
@@ -1025,7 +1022,7 @@ static int intel_trigger(struct snd_pcm_substream *substream, int cmd, struct sn if (res->ops && res->ops->trigger) res->ops->trigger(dai, cmd, substream->stream);
- dai_runtime = snd_soc_dai_get_dma_data(dai, substream); + dai_runtime = cdns->dai_runtime_array[dai->id]; if (!dai_runtime) { dev_err(dai->dev, "failed to get dai runtime in %s\n", __func__); @@ -1092,15 +1089,9 @@ static int intel_component_dais_suspend(struct snd_soc_component *component) struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai); struct sdw_intel *sdw = cdns_to_intel(cdns); struct sdw_cdns_dai_runtime *dai_runtime; - int stream; int ret;
- dai_runtime = dai->playback_dma_data; - stream = SNDRV_PCM_STREAM_PLAYBACK; - if (!dai_runtime) { - dai_runtime = dai->capture_dma_data; - stream = SNDRV_PCM_STREAM_CAPTURE; - } + dai_runtime = cdns->dai_runtime_array[dai->id];
if (!dai_runtime) continue; @@ -1111,7 +1102,7 @@ static int intel_component_dais_suspend(struct snd_soc_component *component) if (dai_runtime->paused) { dai_runtime->suspended = true;
- ret = intel_free_stream(sdw, stream, dai, sdw->instance); + ret = intel_free_stream(sdw, dai_runtime->direction, dai, sdw->instance); if (ret < 0) return ret; } @@ -1178,6 +1169,7 @@ static int intel_create_dai(struct sdw_cdns *cdns,
static int intel_register_dai(struct sdw_intel *sdw) { + struct sdw_cdns_dai_runtime **dai_runtime_array; struct sdw_cdns_stream_config config; struct sdw_cdns *cdns = &sdw->cdns; struct sdw_cdns_streams *stream; @@ -1195,6 +1187,13 @@ static int intel_register_dai(struct sdw_intel *sdw) /* DAIs are created based on total number of PDIs supported */ num_dai = cdns->pcm.num_pdi;
+ dai_runtime_array = devm_kcalloc(cdns->dev, num_dai, + sizeof(struct sdw_cdns_dai_runtime *), + GFP_KERNEL); + if (!dai_runtime_array) + return -ENOMEM; + cdns->dai_runtime_array = dai_runtime_array; + dais = devm_kcalloc(cdns->dev, num_dai, sizeof(*dais), GFP_KERNEL); if (!dais) return -ENOMEM;
On 01-11-22, 10:35, Bard Liao wrote:
The use of dma_data is problematic for two reasons for the Cadence IP. a) the dai runtime data has nothing to do with DMAs in existing solutions b) we will use the dma_data for DMA-management in the future. We cannot share two separate pieces of information with the same dma_data pointer.
Applied, thanks
participants (2)
-
Bard Liao
-
Vinod Koul