[alsa-devel] [RFC 0/7] ASoC: Introduce dmaengine pcm helper functions
Same series as send out yesterday (sans the cleanup patches which already have been applied), since I forgot to Cc alsa-devel. Sorry for the noice if you got the series twice.
This patch series introduces a new set of helper functions for dmaengine based PCM drivers. Currently we have 6 different dmaengine based PCM drivers which are all more or less similar in structure, but all have also individual requirements which can't easily be generalized. So that's why this series does not introduce a generic dmaengine PCM driver but rather a set of helper functions which can be used to implement the common bits between such dmaengine based PCM drivers.
The helper functions only provide infrastructure code for dmaengines which provide thed evice_prep_dma_cyclic. While it is possible to add support for dmaengine, which only provide device_prep_slave_sg, it is in my opinion a better idea to either implement the missing functionality in the dmaengine driver or provide a emulation layer for cyclic transfer within the dmaengine framework itself. This has the advantage that it is also available outside of ALSA.
This series converts the three PCM drivers which already use the circular dmaengine prepare function. Since I don't have access to hardware using the converted drivers the patches have only been compile tested. But the helper functions introduced in this series have been runtime tested with a platform which is not upstream yet.
The first two patches restructure the iMX and MXS drivers to request their DMA channel in the PCM open callback instead of the hwparams callback. This is done to make their structure more similar to what it will be when using the helper functions and make the actual patch converting them to the new dmaengine PCM helper functions less intrusive. The next patch introduces the helper functions and the last three patches respectively convert the imx, mxs and ep39xx dmaengine based PCM drivers to use the new set of helper functions.
The set of helper functions consists of a open, a close, a trigger and a pointer callback and also a function to convert hwparams to a dma_slave_config. The trigger and pointer callbacks can usually be used directly for the pcm_ops callbacks. The open callback wants to be called from a driver specific open function which may do some driver specific setup tasks and provide the helper open callback with a dma filter function. If the driver specific open functions allocates resources they need to be freed in a driver specific close function which has to call the helper close callback. If not the helper close callback can be assigned directly to the pcm_ops struct.
If a driver needs to keep additional driver specific data around it can be done by using snd_dmaengine_pcm_{get,set}_data. Normally a driver should not require to keep additional data around, but all of the converted drivers need this, because of this horrible abusive pattern where the dma channels private field gets assigned configuration data in the filter callback. We should hopefully be able to get rid of this pattern with Guennadi Liakhovetski's patches[1], which add a slave configuration parameter to dma_request_channel, and with it the need for storing extra private data.
While the DMA helper functions only use generic ALSA functions and nothing ASoC specific I've placed the, under ASoC for now since the only users are ASoC driver.
- Lars
[1] https://lkml.org/lkml/2012/2/1/227
Lars-Peter Clausen (7): ASoC: imx-ssi: Set dma data early ASoC: imx-pcm: Request DMA channel early ASoC: mxs-pcm: Request DMA channel early ASoC: Add dmaengine PCM helper functions ASoC: imx-pcm-dma: Use dmaengine PCM helper functions ASoC: mxs-pcm: Use dmaengine PCM helper functions ASoC: ep93xx-pcm: Use dmaengine PCM helper functions
include/sound/dmaengine_pcm.h | 49 +++++++ sound/soc/Kconfig | 3 + sound/soc/Makefile | 3 + sound/soc/ep93xx/Kconfig | 1 + sound/soc/ep93xx/ep93xx-pcm.c | 148 +++----------------- sound/soc/imx/Kconfig | 1 + sound/soc/imx/imx-pcm-dma-mx2.c | 196 ++++---------------------- sound/soc/imx/imx-ssi.c | 28 +++- sound/soc/mxs/Kconfig | 1 + sound/soc/mxs/mxs-pcm.c | 152 ++++----------------- sound/soc/mxs/mxs-pcm.h | 12 -- sound/soc/soc-dmaengine-pcm.c | 287 +++++++++++++++++++++++++++++++++++++++ 12 files changed, 443 insertions(+), 438 deletions(-) create mode 100644 include/sound/dmaengine_pcm.h create mode 100644 sound/soc/soc-dmaengine-pcm.c
Move the call to snd_soc_dai_set_dma_data from the hw_params callback to the startup callback. This allows us to use the dma data in the pcm driver's open callback.
Signed-off-by: Lars-Peter Clausen lars@metafoo.de Tested-by: Shawn Guo shawn.guo@linaro.org --- sound/soc/imx/imx-ssi.c | 28 ++++++++++++++++++++-------- 1 files changed, 20 insertions(+), 8 deletions(-)
diff --git a/sound/soc/imx/imx-ssi.c b/sound/soc/imx/imx-ssi.c index 01d1f74..dbf43f5 100644 --- a/sound/soc/imx/imx-ssi.c +++ b/sound/soc/imx/imx-ssi.c @@ -233,6 +233,23 @@ static int imx_ssi_set_dai_clkdiv(struct snd_soc_dai *cpu_dai, return 0; }
+static int imx_ssi_startup(struct snd_pcm_substream *substream, + struct snd_soc_dai *cpu_dai) +{ + struct imx_ssi *ssi = snd_soc_dai_get_drvdata(cpu_dai); + struct imx_pcm_dma_params *dma_data; + + /* Tx/Rx config */ + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) + dma_data = &ssi->dma_params_tx; + else + dma_data = &ssi->dma_params_rx; + + snd_soc_dai_set_dma_data(cpu_dai, substream, dma_data); + + return 0; +} + /* * Should only be called when port is inactive (i.e. SSIEN = 0), * although can be called multiple times by upper layers. @@ -242,23 +259,17 @@ static int imx_ssi_hw_params(struct snd_pcm_substream *substream, struct snd_soc_dai *cpu_dai) { struct imx_ssi *ssi = snd_soc_dai_get_drvdata(cpu_dai); - struct imx_pcm_dma_params *dma_data; u32 reg, sccr;
/* Tx/Rx config */ - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) reg = SSI_STCCR; - dma_data = &ssi->dma_params_tx; - } else { + else reg = SSI_SRCCR; - dma_data = &ssi->dma_params_rx; - }
if (ssi->flags & IMX_SSI_SYN) reg = SSI_STCCR;
- snd_soc_dai_set_dma_data(cpu_dai, substream, dma_data); - sccr = readl(ssi->base + reg) & ~SSI_STCCR_WL_MASK;
/* DAI data (word) size */ @@ -343,6 +354,7 @@ static int imx_ssi_trigger(struct snd_pcm_substream *substream, int cmd, }
static const struct snd_soc_dai_ops imx_ssi_pcm_dai_ops = { + .startup = imx_ssi_startup, .hw_params = imx_ssi_hw_params, .set_fmt = imx_ssi_set_dai_fmt, .set_clkdiv = imx_ssi_set_dai_clkdiv,
On Wed, Feb 22, 2012 at 10:49:05AM +0100, Lars-Peter Clausen wrote:
Move the call to snd_soc_dai_set_dma_data from the hw_params callback to the startup callback. This allows us to use the dma data in the pcm driver's open callback.
Applied, thanks.
Request the DMA channel in the pcm open callback. This allows us to let open fail if there is no dma channel available.
Signed-off-by: Lars-Peter Clausen lars@metafoo.de Tested-by: Shawn Guo shawn.guo@linaro.org --- sound/soc/imx/imx-pcm-dma-mx2.c | 78 ++++++++++++++++----------------------- 1 files changed, 32 insertions(+), 46 deletions(-)
diff --git a/sound/soc/imx/imx-pcm-dma-mx2.c b/sound/soc/imx/imx-pcm-dma-mx2.c index ec13944..f974e61 100644 --- a/sound/soc/imx/imx-pcm-dma-mx2.c +++ b/sound/soc/imx/imx-pcm-dma-mx2.c @@ -65,17 +65,13 @@ static bool filter(struct dma_chan *chan, void *param) return true; }
-static int imx_ssi_dma_alloc(struct snd_pcm_substream *substream, - struct snd_pcm_hw_params *params) +static int imx_ssi_dma_alloc(struct snd_pcm_substream *substream) { struct snd_soc_pcm_runtime *rtd = substream->private_data; struct imx_pcm_dma_params *dma_params; struct snd_pcm_runtime *runtime = substream->runtime; struct imx_pcm_runtime_data *iprtd = runtime->private_data; - struct dma_slave_config slave_config; dma_cap_mask_t mask; - enum dma_slave_buswidth buswidth; - int ret;
dma_params = snd_soc_dai_get_dma_data(rtd->cpu_dai, substream);
@@ -84,13 +80,29 @@ static int imx_ssi_dma_alloc(struct snd_pcm_substream *substream, iprtd->dma_data.dma_request = dma_params->dma;
/* Try to grab a DMA channel */ - if (!iprtd->dma_chan) { - dma_cap_zero(mask); - dma_cap_set(DMA_SLAVE, mask); - iprtd->dma_chan = dma_request_channel(mask, filter, iprtd); - if (!iprtd->dma_chan) - return -EINVAL; - } + dma_cap_zero(mask); + dma_cap_set(DMA_SLAVE, mask); + iprtd->dma_chan = dma_request_channel(mask, filter, iprtd); + if (!iprtd->dma_chan) + return -EINVAL; + + return 0; +} + +static int snd_imx_pcm_hw_params(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct snd_pcm_runtime *runtime = substream->runtime; + struct imx_pcm_runtime_data *iprtd = runtime->private_data; + struct dma_chan *chan = iprtd->dma_chan; + struct imx_pcm_dma_params *dma_params; + struct dma_slave_config slave_config; + enum dma_slave_buswidth buswidth; + unsigned long dma_addr; + int ret; + + dma_params = snd_soc_dai_get_dma_data(rtd->cpu_dai, substream);
switch (params_format(params)) { case SNDRV_PCM_FORMAT_S16_LE: @@ -116,29 +128,10 @@ static int imx_ssi_dma_alloc(struct snd_pcm_substream *substream, slave_config.src_maxburst = dma_params->burstsize; }
- ret = dmaengine_slave_config(iprtd->dma_chan, &slave_config); + ret = dmaengine_slave_config(chan, &slave_config); if (ret) return ret;
- return 0; -} - -static int snd_imx_pcm_hw_params(struct snd_pcm_substream *substream, - struct snd_pcm_hw_params *params) -{ - struct snd_soc_pcm_runtime *rtd = substream->private_data; - struct snd_pcm_runtime *runtime = substream->runtime; - struct imx_pcm_runtime_data *iprtd = runtime->private_data; - unsigned long dma_addr; - struct dma_chan *chan; - struct imx_pcm_dma_params *dma_params; - int ret; - - dma_params = snd_soc_dai_get_dma_data(rtd->cpu_dai, substream); - ret = imx_ssi_dma_alloc(substream, params); - if (ret) - return ret; - chan = iprtd->dma_chan;
iprtd->periods = params_periods(params); iprtd->period_bytes = params_period_bytes(params); @@ -164,19 +157,6 @@ static int snd_imx_pcm_hw_params(struct snd_pcm_substream *substream, return 0; }
-static int snd_imx_pcm_hw_free(struct snd_pcm_substream *substream) -{ - struct snd_pcm_runtime *runtime = substream->runtime; - struct imx_pcm_runtime_data *iprtd = runtime->private_data; - - if (iprtd->dma_chan) { - dma_release_channel(iprtd->dma_chan); - iprtd->dma_chan = NULL; - } - - return 0; -} - static int snd_imx_pcm_trigger(struct snd_pcm_substream *substream, int cmd) { struct snd_pcm_runtime *runtime = substream->runtime; @@ -251,6 +231,12 @@ static int snd_imx_open(struct snd_pcm_substream *substream) return ret; }
+ ret = imx_ssi_dma_alloc(substream); + if (ret < 0) { + kfree(iprtd); + return ret; + } + snd_soc_set_runtime_hwparams(substream, &snd_imx_hardware);
return 0; @@ -261,6 +247,7 @@ static int snd_imx_close(struct snd_pcm_substream *substream) struct snd_pcm_runtime *runtime = substream->runtime; struct imx_pcm_runtime_data *iprtd = runtime->private_data;
+ dma_release_channel(iprtd->dma_chan); kfree(iprtd);
return 0; @@ -271,7 +258,6 @@ static struct snd_pcm_ops imx_pcm_ops = { .close = snd_imx_close, .ioctl = snd_pcm_lib_ioctl, .hw_params = snd_imx_pcm_hw_params, - .hw_free = snd_imx_pcm_hw_free, .trigger = snd_imx_pcm_trigger, .pointer = snd_imx_pcm_pointer, .mmap = snd_imx_pcm_mmap,
On Wed, Feb 22, 2012 at 10:49:06AM +0100, Lars-Peter Clausen wrote:
Request the DMA channel in the pcm open callback. This allows us to let open fail if there is no dma channel available.
Applied, thanks.
Request the DMA channel in the PCM open callback instead of the hwparams callback, this allows us to let open fail if no dma channel is available. This also fixes a bug where the channel will be requested multiple times if hwparams is called multiple times.
Signed-off-by: Lars-Peter Clausen lars@metafoo.de Tested-by: Shawn Guo shawn.guo@linaro.org --- sound/soc/mxs/mxs-pcm.c | 28 ++++++++-------------------- 1 files changed, 8 insertions(+), 20 deletions(-)
diff --git a/sound/soc/mxs/mxs-pcm.c b/sound/soc/mxs/mxs-pcm.c index 06c18ec..5b8c8d3 100644 --- a/sound/soc/mxs/mxs-pcm.c +++ b/sound/soc/mxs/mxs-pcm.c @@ -85,8 +85,7 @@ static bool filter(struct dma_chan *chan, void *param) return true; }
-static int mxs_dma_alloc(struct snd_pcm_substream *substream, - struct snd_pcm_hw_params *params) +static int mxs_dma_alloc(struct snd_pcm_substream *substream) { struct snd_soc_pcm_runtime *rtd = substream->private_data; struct snd_pcm_runtime *runtime = substream->runtime; @@ -112,11 +111,7 @@ static int snd_mxs_pcm_hw_params(struct snd_pcm_substream *substream, struct mxs_pcm_runtime_data *iprtd = runtime->private_data; unsigned long dma_addr; struct dma_chan *chan; - int ret;
- ret = mxs_dma_alloc(substream, params); - if (ret) - return ret; chan = iprtd->dma_chan;
iprtd->periods = params_periods(params); @@ -143,19 +138,6 @@ static int snd_mxs_pcm_hw_params(struct snd_pcm_substream *substream, return 0; }
-static int snd_mxs_pcm_hw_free(struct snd_pcm_substream *substream) -{ - struct snd_pcm_runtime *runtime = substream->runtime; - struct mxs_pcm_runtime_data *iprtd = runtime->private_data; - - if (iprtd->dma_chan) { - dma_release_channel(iprtd->dma_chan); - iprtd->dma_chan = NULL; - } - - return 0; -} - static int snd_mxs_pcm_trigger(struct snd_pcm_substream *substream, int cmd) { struct snd_pcm_runtime *runtime = substream->runtime; @@ -208,6 +190,12 @@ static int snd_mxs_open(struct snd_pcm_substream *substream) return ret; }
+ ret = mxs_dma_alloc(substream); + if (ret) { + kfree(iprtd); + return ret; + } + snd_soc_set_runtime_hwparams(substream, &snd_mxs_hardware);
return 0; @@ -218,6 +206,7 @@ static int snd_mxs_close(struct snd_pcm_substream *substream) struct snd_pcm_runtime *runtime = substream->runtime; struct mxs_pcm_runtime_data *iprtd = runtime->private_data;
+ dma_release_channel(iprtd->dma_chan); kfree(iprtd);
return 0; @@ -239,7 +228,6 @@ static struct snd_pcm_ops mxs_pcm_ops = { .close = snd_mxs_close, .ioctl = snd_pcm_lib_ioctl, .hw_params = snd_mxs_pcm_hw_params, - .hw_free = snd_mxs_pcm_hw_free, .trigger = snd_mxs_pcm_trigger, .pointer = snd_mxs_pcm_pointer, .mmap = snd_mxs_pcm_mmap,
On Wed, Feb 22, 2012 at 10:49:07AM +0100, Lars-Peter Clausen wrote:
Request the DMA channel in the PCM open callback instead of the hwparams callback, this allows us to let open fail if no dma channel is available. This also fixes a bug where the channel will be requested multiple times if hwparams is called multiple times.
Applied, thanks.
This patch adds a set of functions which are intended to be used when implementing a dmaengine based sound PCM driver.
Signed-off-by: Lars-Peter Clausen lars@metafoo.de Tested-by: Shawn Guo shawn.guo@linaro.org --- include/sound/dmaengine_pcm.h | 49 +++++++ sound/soc/Kconfig | 3 + sound/soc/Makefile | 3 + sound/soc/soc-dmaengine-pcm.c | 287 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 342 insertions(+), 0 deletions(-) create mode 100644 include/sound/dmaengine_pcm.h create mode 100644 sound/soc/soc-dmaengine-pcm.c
diff --git a/include/sound/dmaengine_pcm.h b/include/sound/dmaengine_pcm.h new file mode 100644 index 0000000..a8fcaa6 --- /dev/null +++ b/include/sound/dmaengine_pcm.h @@ -0,0 +1,49 @@ +/* + * Copyright (C) 2012, Analog Devices Inc. + * Author: Lars-Peter Clausen lars@metafoo.de + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or (at your + * option) any later version. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 675 Mass Ave, Cambridge, MA 02139, USA. + * + */ +#ifndef __SOUND_DMAENGINE_PCM_H__ +#define __SOUND_DMAENGINE_PCM_H__ + +#include <sound/pcm.h> +#include <linux/dmaengine.h> + +/** + * snd_pcm_substream_to_dma_direction - Get dma_transfer_direction for a PCM + * substream + * @substream: PCM substream + */ +static inline enum dma_transfer_direction +snd_pcm_substream_to_dma_direction(const struct snd_pcm_substream *substream) +{ + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) + return DMA_MEM_TO_DEV; + else + return DMA_DEV_TO_MEM; +} + +void snd_dmaengine_pcm_set_data(struct snd_pcm_substream *substream, void *data); +void *snd_dmaengine_pcm_get_data(struct snd_pcm_substream *substream); + +int snd_hwparams_to_dma_slave_config(const struct snd_pcm_substream *substream, + const struct snd_pcm_hw_params *params, struct dma_slave_config *slave_config); +int snd_dmaengine_pcm_trigger(struct snd_pcm_substream *substream, int cmd); +snd_pcm_uframes_t snd_dmaengine_pcm_pointer(struct snd_pcm_substream *substream); + +int snd_dmaengine_pcm_open(struct snd_pcm_substream *substream, + dma_filter_fn filter_fn, void *filter_data); +int snd_dmaengine_pcm_close(struct snd_pcm_substream *substream); + +struct dma_chan *snd_dmaengine_pcm_get_chan(struct snd_pcm_substream *substream); + +#endif diff --git a/sound/soc/Kconfig b/sound/soc/Kconfig index 35e662d..91c9855 100644 --- a/sound/soc/Kconfig +++ b/sound/soc/Kconfig @@ -25,6 +25,9 @@ if SND_SOC config SND_SOC_AC97_BUS bool
+config SND_SOC_DMAENGINE_PCM + bool + # All the supported SoCs source "sound/soc/atmel/Kconfig" source "sound/soc/au1x/Kconfig" diff --git a/sound/soc/Makefile b/sound/soc/Makefile index 9ea8ac8..2feaf37 100644 --- a/sound/soc/Makefile +++ b/sound/soc/Makefile @@ -1,6 +1,9 @@ snd-soc-core-objs := soc-core.o soc-dapm.o soc-jack.o soc-cache.o soc-utils.o snd-soc-core-objs += soc-pcm.o soc-io.o
+snd-soc-dmaengine-pcm-objs := soc-dmaengine-pcm.o +obj-$(CONFIG_SND_SOC_DMAENGINE_PCM) += snd-soc-dmaengine-pcm.o + obj-$(CONFIG_SND_SOC) += snd-soc-core.o obj-$(CONFIG_SND_SOC) += codecs/ obj-$(CONFIG_SND_SOC) += atmel/ diff --git a/sound/soc/soc-dmaengine-pcm.c b/sound/soc/soc-dmaengine-pcm.c new file mode 100644 index 0000000..0526cf8 --- /dev/null +++ b/sound/soc/soc-dmaengine-pcm.c @@ -0,0 +1,287 @@ +/* + * Copyright (C) 2012, Analog Devices Inc. + * Author: Lars-Peter Clausen lars@metafoo.de + * + * Based on: + * imx-pcm-dma-mx2.c, Copyright 2009 Sascha Hauer s.hauer@pengutronix.de + * mxs-pcm.c, Copyright (C) 2011 Freescale Semiconductor, Inc. + * ep93xx-pcm.c, Copyright (C) 2006 Lennert Buytenhek buytenh@wantstofly.org + * Copyright (C) 2006 Applied Data Systems + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or (at your + * option) any later version. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 675 Mass Ave, Cambridge, MA 02139, USA. + * + */ +#include <linux/module.h> +#include <linux/init.h> +#include <linux/dmaengine.h> +#include <linux/slab.h> +#include <sound/pcm.h> +#include <sound/pcm_params.h> +#include <sound/soc.h> + +#include <sound/dmaengine_pcm.h> + +struct dmaengine_pcm_runtime_data { + struct dma_chan *dma_chan; + + unsigned int pos; + + void *data; +}; + +static inline struct dmaengine_pcm_runtime_data *substream_to_prtd( + const struct snd_pcm_substream *substream) +{ + return substream->runtime->private_data; +} + +/** + * snd_dmaengine_pcm_set_data - Set dmaengine substream private data + * @substream: PCM substream + * @data: Data to set + */ +void snd_dmaengine_pcm_set_data(struct snd_pcm_substream *substream, void *data) +{ + struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream); + + prtd->data = data; +} +EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_set_data); + +/** + * snd_dmaengine_pcm_get_data - Get dmaeinge substream private data + * @substream: PCM substream + * + * Returns the data previously set with snd_dmaengine_pcm_set_data + */ +void *snd_dmaengine_pcm_get_data(struct snd_pcm_substream *substream) +{ + struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream); + + return prtd->data; +} +EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_get_data); + +struct dma_chan *snd_dmaengine_pcm_get_chan(struct snd_pcm_substream *substream) +{ + struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream); + + return prtd->dma_chan; +} +EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_get_chan); + +/** + * snd_hwparams_to_dma_slave_config - Convert hw_params to dma_slave_config + * @substream: PCM substream + * @params: hw_params + * @slave_config: DMA slave config + * + * This function can be used to initialize a dma_slave_config from a substream + * and hw_params in a dmaengine based PCM driver implementation. + */ +int snd_hwparams_to_dma_slave_config(const struct snd_pcm_substream *substream, + const struct snd_pcm_hw_params *params, + struct dma_slave_config *slave_config) +{ + enum dma_slave_buswidth buswidth; + + switch (params_format(params)) { + case SNDRV_PCM_FORMAT_S8: + buswidth = DMA_SLAVE_BUSWIDTH_1_BYTE; + break; + case SNDRV_PCM_FORMAT_S16_LE: + buswidth = DMA_SLAVE_BUSWIDTH_2_BYTES; + break; + case SNDRV_PCM_FORMAT_S18_3LE: + case SNDRV_PCM_FORMAT_S20_3LE: + case SNDRV_PCM_FORMAT_S24_LE: + case SNDRV_PCM_FORMAT_S32_LE: + buswidth = DMA_SLAVE_BUSWIDTH_4_BYTES; + break; + default: + return -EINVAL; + } + + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { + slave_config->direction = DMA_MEM_TO_DEV; + slave_config->dst_addr_width = buswidth; + } else { + slave_config->direction = DMA_DEV_TO_MEM; + slave_config->src_addr_width = buswidth; + } + + return 0; +} +EXPORT_SYMBOL_GPL(snd_hwparams_to_dma_slave_config); + +static void dmaengine_pcm_dma_complete(void *arg) +{ + struct snd_pcm_substream *substream = arg; + struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream); + + prtd->pos += snd_pcm_lib_period_bytes(substream); + if (prtd->pos >= snd_pcm_lib_buffer_bytes(substream)) + prtd->pos = 0; + + snd_pcm_period_elapsed(substream); +} + +static int dmaengine_pcm_prepare_and_submit(struct snd_pcm_substream *substream) +{ + struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream); + struct dma_chan *chan = prtd->dma_chan; + struct dma_async_tx_descriptor *desc; + enum dma_transfer_direction direction; + + direction = snd_pcm_substream_to_dma_direction(substream); + + desc = chan->device->device_prep_dma_cyclic(chan, + substream->runtime->dma_addr, + snd_pcm_lib_buffer_bytes(substream), + snd_pcm_lib_period_bytes(substream), direction); + + if (!desc) + return -ENOMEM; + + desc->callback = dmaengine_pcm_dma_complete; + desc->callback_param = substream; + dmaengine_submit(desc); + + return 0; +} + +/** + * snd_dmaengine_pcm_trigger - dmaengine based PCM trigger implementation + * @substream: PCM substream + * @cmd: Trigger command + * + * Returns 0 on success, a negative error code otherwise. + * + * This function can be used as the PCM trigger callback for dmaengine based PCM + * driver implementations. + */ +int snd_dmaengine_pcm_trigger(struct snd_pcm_substream *substream, int cmd) +{ + struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream); + int ret; + + switch (cmd) { + case SNDRV_PCM_TRIGGER_START: + ret = dmaengine_pcm_prepare_and_submit(substream); + if (ret) + return ret; + dma_async_issue_pending(prtd->dma_chan); + break; + case SNDRV_PCM_TRIGGER_RESUME: + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: + dmaengine_resume(prtd->dma_chan); + break; + case SNDRV_PCM_TRIGGER_SUSPEND: + case SNDRV_PCM_TRIGGER_PAUSE_PUSH: + dmaengine_pause(prtd->dma_chan); + break; + case SNDRV_PCM_TRIGGER_STOP: + dmaengine_terminate_all(prtd->dma_chan); + break; + default: + return -EINVAL; + } + + return 0; +} +EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_trigger); + +/** + * snd_dmaengine_pcm_pointer - dmaengine based PCM pointer implementation + * @substream: PCM substream + * + * This function can be used as the PCM pointer callback for dmaengine based PCM + * driver implementations. + */ +snd_pcm_uframes_t snd_dmaengine_pcm_pointer(struct snd_pcm_substream *substream) +{ + struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream); + return bytes_to_frames(substream->runtime, prtd->pos); +} +EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_pointer); + +static int dmaengine_pcm_request_channel(struct dmaengine_pcm_runtime_data *prtd, + dma_filter_fn filter_fn, void *filter_data) +{ + dma_cap_mask_t mask; + + dma_cap_zero(mask); + dma_cap_set(DMA_SLAVE, mask); + dma_cap_set(DMA_CYCLIC, mask); + prtd->dma_chan = dma_request_channel(mask, filter_fn, filter_data); + + if (!prtd->dma_chan) + return -ENXIO; + + return 0; +} + +/** + * snd_dmaengine_pcm_open - Open a dmaengine based PCM substream + * @substream: PCM substream + * @filter_fn: Filter function used to request the DMA channel + * @filter_data: Data passed to the DMA filter function + * + * Returns 0 on success, a negative error code otherwise. + * + * This function will request a DMA channel using the passed filter function and + * data. The function should usually be called from the pcm open callback. + * + * Note that this function will use private_data field of the substream's + * runtime. So it is not availabe to your pcm driver implementation. If you need + * to keep additional data attached to a substream use + * snd_dmaeinge_pcm_{set,get}_data. + */ +int snd_dmaengine_pcm_open(struct snd_pcm_substream *substream, + dma_filter_fn filter_fn, void *filter_data) +{ + struct dmaengine_pcm_runtime_data *prtd; + int ret; + + ret = snd_pcm_hw_constraint_integer(substream->runtime, + SNDRV_PCM_HW_PARAM_PERIODS); + if (ret < 0) + return ret; + + prtd = kzalloc(sizeof(*prtd), GFP_KERNEL); + if (!prtd) + return -ENOMEM; + + ret = dmaengine_pcm_request_channel(prtd, filter_fn, filter_data); + if (ret < 0) { + kfree(prtd); + return ret; + } + + substream->runtime->private_data = prtd; + + return 0; +} +EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_open); + +/** + * snd_dmaengine_pcm_close - Close a dmaengine based PCM substream + * @substream: PCM substream + */ +int snd_dmaengine_pcm_close(struct snd_pcm_substream *substream) +{ + struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream); + + dma_release_channel(prtd->dma_chan); + kfree(prtd); + + return 0; +} +EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_close);
On Wed, Feb 22, 2012 at 10:49:08AM +0100, Lars-Peter Clausen wrote:
This patch adds a set of functions which are intended to be used when implementing a dmaengine based sound PCM driver.
This isn't going to be usable on sa11x0 platforms, because we need to know the struct device underlying the DMA device before we allocate the buffers.
On 02/22/2012 11:01 AM, Russell King - ARM Linux wrote:
On Wed, Feb 22, 2012 at 10:49:08AM +0100, Lars-Peter Clausen wrote:
This patch adds a set of functions which are intended to be used when implementing a dmaengine based sound PCM driver.
This isn't going to be usable on sa11x0 platforms, because we need to know the struct device underlying the DMA device before we allocate the buffers.
Which means the dma channel has to be requested before allocating the buffers. Which can be done. Not with the current code but it can be implemented on top of it.
I had a look at your 'generic' dmaengine PCM driver patch. And it looks like you went to opposite way as I did in this patch. You implemented one driver which is supposed to work for all cases, while my patch provides a set of helper functions which can be used to implement a PCM driver. Your patch only supports non-cyclic transfers, mine only supports cyclic transfers.
Your driver emulates cyclic transfers using non-cyclic transfers, in my opinion it is better to add such a emulation layer to the dmaengine core itself. This will allow other users to benefit from this as well and it doesn't have to be reimplemented in other subsystems/driver. Also users don't have to be updated if a dmaengine driver gets native support for cyclic transfers. But, well, the code exists and it is a step in the right direction so we should probably use it.
I suppose we could rearrange the code so that we can share most of it between the cyclic and non-cyclic case. The non-cyclic case needs special handling everywhere, which can be made conditional.
- Lars
On Wed, Feb 22, 2012 at 10:49:08AM +0100, Lars-Peter Clausen wrote:
This patch adds a set of functions which are intended to be used when implementing a dmaengine based sound PCM driver.
Adding Morimoto-san as he has just implemented dmaengine support for fsi which should also be able to use the same framework. Not cutting any of the text so he can see it.
Signed-off-by: Lars-Peter Clausen lars@metafoo.de Tested-by: Shawn Guo shawn.guo@linaro.org
include/sound/dmaengine_pcm.h | 49 +++++++ sound/soc/Kconfig | 3 + sound/soc/Makefile | 3 + sound/soc/soc-dmaengine-pcm.c | 287 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 342 insertions(+), 0 deletions(-) create mode 100644 include/sound/dmaengine_pcm.h create mode 100644 sound/soc/soc-dmaengine-pcm.c
diff --git a/include/sound/dmaengine_pcm.h b/include/sound/dmaengine_pcm.h new file mode 100644 index 0000000..a8fcaa6 --- /dev/null +++ b/include/sound/dmaengine_pcm.h @@ -0,0 +1,49 @@ +/*
- Copyright (C) 2012, Analog Devices Inc.
- Author: Lars-Peter Clausen lars@metafoo.de
- This program is free software; you can redistribute it and/or modify it
- under the terms of the GNU General Public License as published by the
- Free Software Foundation; either version 2 of the License, or (at your
- option) any later version.
- You should have received a copy of the GNU General Public License along
- with this program; if not, write to the Free Software Foundation, Inc.,
- 675 Mass Ave, Cambridge, MA 02139, USA.
- */
+#ifndef __SOUND_DMAENGINE_PCM_H__ +#define __SOUND_DMAENGINE_PCM_H__
+#include <sound/pcm.h> +#include <linux/dmaengine.h>
+/**
- snd_pcm_substream_to_dma_direction - Get dma_transfer_direction for a PCM
- substream
- @substream: PCM substream
- */
+static inline enum dma_transfer_direction +snd_pcm_substream_to_dma_direction(const struct snd_pcm_substream *substream) +{
- if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
return DMA_MEM_TO_DEV;- else
return DMA_DEV_TO_MEM;+}
+void snd_dmaengine_pcm_set_data(struct snd_pcm_substream *substream, void *data); +void *snd_dmaengine_pcm_get_data(struct snd_pcm_substream *substream);
+int snd_hwparams_to_dma_slave_config(const struct snd_pcm_substream *substream,
- const struct snd_pcm_hw_params *params, struct dma_slave_config *slave_config);
+int snd_dmaengine_pcm_trigger(struct snd_pcm_substream *substream, int cmd); +snd_pcm_uframes_t snd_dmaengine_pcm_pointer(struct snd_pcm_substream *substream);
+int snd_dmaengine_pcm_open(struct snd_pcm_substream *substream,
- dma_filter_fn filter_fn, void *filter_data);
+int snd_dmaengine_pcm_close(struct snd_pcm_substream *substream);
+struct dma_chan *snd_dmaengine_pcm_get_chan(struct snd_pcm_substream *substream);
+#endif diff --git a/sound/soc/Kconfig b/sound/soc/Kconfig index 35e662d..91c9855 100644 --- a/sound/soc/Kconfig +++ b/sound/soc/Kconfig @@ -25,6 +25,9 @@ if SND_SOC config SND_SOC_AC97_BUS bool
+config SND_SOC_DMAENGINE_PCM
- bool
# All the supported SoCs source "sound/soc/atmel/Kconfig" source "sound/soc/au1x/Kconfig" diff --git a/sound/soc/Makefile b/sound/soc/Makefile index 9ea8ac8..2feaf37 100644 --- a/sound/soc/Makefile +++ b/sound/soc/Makefile @@ -1,6 +1,9 @@ snd-soc-core-objs := soc-core.o soc-dapm.o soc-jack.o soc-cache.o soc-utils.o snd-soc-core-objs += soc-pcm.o soc-io.o
+snd-soc-dmaengine-pcm-objs := soc-dmaengine-pcm.o +obj-$(CONFIG_SND_SOC_DMAENGINE_PCM) += snd-soc-dmaengine-pcm.o
obj-$(CONFIG_SND_SOC) += snd-soc-core.o obj-$(CONFIG_SND_SOC) += codecs/ obj-$(CONFIG_SND_SOC) += atmel/ diff --git a/sound/soc/soc-dmaengine-pcm.c b/sound/soc/soc-dmaengine-pcm.c new file mode 100644 index 0000000..0526cf8 --- /dev/null +++ b/sound/soc/soc-dmaengine-pcm.c @@ -0,0 +1,287 @@ +/*
- Copyright (C) 2012, Analog Devices Inc.
- Author: Lars-Peter Clausen lars@metafoo.de
- Based on:
- imx-pcm-dma-mx2.c, Copyright 2009 Sascha Hauer s.hauer@pengutronix.de
- mxs-pcm.c, Copyright (C) 2011 Freescale Semiconductor, Inc.
- ep93xx-pcm.c, Copyright (C) 2006 Lennert Buytenhek buytenh@wantstofly.org
Copyright (C) 2006 Applied Data Systems
- This program is free software; you can redistribute it and/or modify it
- under the terms of the GNU General Public License as published by the
- Free Software Foundation; either version 2 of the License, or (at your
- option) any later version.
- You should have received a copy of the GNU General Public License along
- with this program; if not, write to the Free Software Foundation, Inc.,
- 675 Mass Ave, Cambridge, MA 02139, USA.
- */
+#include <linux/module.h> +#include <linux/init.h> +#include <linux/dmaengine.h> +#include <linux/slab.h> +#include <sound/pcm.h> +#include <sound/pcm_params.h> +#include <sound/soc.h>
+#include <sound/dmaengine_pcm.h>
+struct dmaengine_pcm_runtime_data {
- struct dma_chan *dma_chan;
- unsigned int pos;
- void *data;
+};
+static inline struct dmaengine_pcm_runtime_data *substream_to_prtd(
- const struct snd_pcm_substream *substream)
+{
- return substream->runtime->private_data;
+}
+/**
- snd_dmaengine_pcm_set_data - Set dmaengine substream private data
- @substream: PCM substream
- @data: Data to set
- */
+void snd_dmaengine_pcm_set_data(struct snd_pcm_substream *substream, void *data) +{
- struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream);
- prtd->data = data;
+} +EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_set_data);
+/**
- snd_dmaengine_pcm_get_data - Get dmaeinge substream private data
- @substream: PCM substream
- Returns the data previously set with snd_dmaengine_pcm_set_data
- */
+void *snd_dmaengine_pcm_get_data(struct snd_pcm_substream *substream) +{
- struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream);
- return prtd->data;
+} +EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_get_data);
+struct dma_chan *snd_dmaengine_pcm_get_chan(struct snd_pcm_substream *substream) +{
- struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream);
- return prtd->dma_chan;
+} +EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_get_chan);
+/**
- snd_hwparams_to_dma_slave_config - Convert hw_params to dma_slave_config
- @substream: PCM substream
- @params: hw_params
- @slave_config: DMA slave config
- This function can be used to initialize a dma_slave_config from a substream
- and hw_params in a dmaengine based PCM driver implementation.
- */
+int snd_hwparams_to_dma_slave_config(const struct snd_pcm_substream *substream,
- const struct snd_pcm_hw_params *params,
- struct dma_slave_config *slave_config)
+{
- enum dma_slave_buswidth buswidth;
- switch (params_format(params)) {
- case SNDRV_PCM_FORMAT_S8:
buswidth = DMA_SLAVE_BUSWIDTH_1_BYTE;break;- case SNDRV_PCM_FORMAT_S16_LE:
buswidth = DMA_SLAVE_BUSWIDTH_2_BYTES;break;- case SNDRV_PCM_FORMAT_S18_3LE:
- case SNDRV_PCM_FORMAT_S20_3LE:
- case SNDRV_PCM_FORMAT_S24_LE:
- case SNDRV_PCM_FORMAT_S32_LE:
buswidth = DMA_SLAVE_BUSWIDTH_4_BYTES;break;- default:
return -EINVAL;- }
- if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
slave_config->direction = DMA_MEM_TO_DEV;slave_config->dst_addr_width = buswidth;- } else {
slave_config->direction = DMA_DEV_TO_MEM;slave_config->src_addr_width = buswidth;- }
- return 0;
+} +EXPORT_SYMBOL_GPL(snd_hwparams_to_dma_slave_config);
+static void dmaengine_pcm_dma_complete(void *arg) +{
- struct snd_pcm_substream *substream = arg;
- struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream);
- prtd->pos += snd_pcm_lib_period_bytes(substream);
- if (prtd->pos >= snd_pcm_lib_buffer_bytes(substream))
prtd->pos = 0;- snd_pcm_period_elapsed(substream);
+}
+static int dmaengine_pcm_prepare_and_submit(struct snd_pcm_substream *substream) +{
- struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream);
- struct dma_chan *chan = prtd->dma_chan;
- struct dma_async_tx_descriptor *desc;
- enum dma_transfer_direction direction;
- direction = snd_pcm_substream_to_dma_direction(substream);
- desc = chan->device->device_prep_dma_cyclic(chan,
substream->runtime->dma_addr,snd_pcm_lib_buffer_bytes(substream),snd_pcm_lib_period_bytes(substream), direction);- if (!desc)
return -ENOMEM;- desc->callback = dmaengine_pcm_dma_complete;
- desc->callback_param = substream;
- dmaengine_submit(desc);
- return 0;
+}
+/**
- snd_dmaengine_pcm_trigger - dmaengine based PCM trigger implementation
- @substream: PCM substream
- @cmd: Trigger command
- Returns 0 on success, a negative error code otherwise.
- This function can be used as the PCM trigger callback for dmaengine based PCM
- driver implementations.
- */
+int snd_dmaengine_pcm_trigger(struct snd_pcm_substream *substream, int cmd) +{
- struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream);
- int ret;
- switch (cmd) {
- case SNDRV_PCM_TRIGGER_START:
ret = dmaengine_pcm_prepare_and_submit(substream);if (ret)return ret;dma_async_issue_pending(prtd->dma_chan);break;- case SNDRV_PCM_TRIGGER_RESUME:
- case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
dmaengine_resume(prtd->dma_chan);break;- case SNDRV_PCM_TRIGGER_SUSPEND:
- case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
dmaengine_pause(prtd->dma_chan);break;- case SNDRV_PCM_TRIGGER_STOP:
dmaengine_terminate_all(prtd->dma_chan);break;- default:
return -EINVAL;- }
- return 0;
+} +EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_trigger);
+/**
- snd_dmaengine_pcm_pointer - dmaengine based PCM pointer implementation
- @substream: PCM substream
- This function can be used as the PCM pointer callback for dmaengine based PCM
- driver implementations.
- */
+snd_pcm_uframes_t snd_dmaengine_pcm_pointer(struct snd_pcm_substream *substream) +{
- struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream);
- return bytes_to_frames(substream->runtime, prtd->pos);
+} +EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_pointer);
+static int dmaengine_pcm_request_channel(struct dmaengine_pcm_runtime_data *prtd,
- dma_filter_fn filter_fn, void *filter_data)
+{
- dma_cap_mask_t mask;
- dma_cap_zero(mask);
- dma_cap_set(DMA_SLAVE, mask);
- dma_cap_set(DMA_CYCLIC, mask);
- prtd->dma_chan = dma_request_channel(mask, filter_fn, filter_data);
- if (!prtd->dma_chan)
return -ENXIO;- return 0;
+}
+/**
- snd_dmaengine_pcm_open - Open a dmaengine based PCM substream
- @substream: PCM substream
- @filter_fn: Filter function used to request the DMA channel
- @filter_data: Data passed to the DMA filter function
- Returns 0 on success, a negative error code otherwise.
- This function will request a DMA channel using the passed filter function and
- data. The function should usually be called from the pcm open callback.
- Note that this function will use private_data field of the substream's
- runtime. So it is not availabe to your pcm driver implementation. If you need
- to keep additional data attached to a substream use
- snd_dmaeinge_pcm_{set,get}_data.
- */
+int snd_dmaengine_pcm_open(struct snd_pcm_substream *substream,
- dma_filter_fn filter_fn, void *filter_data)
+{
- struct dmaengine_pcm_runtime_data *prtd;
- int ret;
- ret = snd_pcm_hw_constraint_integer(substream->runtime,
SNDRV_PCM_HW_PARAM_PERIODS);- if (ret < 0)
return ret;- prtd = kzalloc(sizeof(*prtd), GFP_KERNEL);
- if (!prtd)
return -ENOMEM;- ret = dmaengine_pcm_request_channel(prtd, filter_fn, filter_data);
- if (ret < 0) {
kfree(prtd);return ret;- }
- substream->runtime->private_data = prtd;
- return 0;
+} +EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_open);
+/**
- snd_dmaengine_pcm_close - Close a dmaengine based PCM substream
- @substream: PCM substream
- */
+int snd_dmaengine_pcm_close(struct snd_pcm_substream *substream) +{
- struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream);
- dma_release_channel(prtd->dma_chan);
- kfree(prtd);
- return 0;
+}
+EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_close);
1.7.9
On Wed, Feb 22, 2012 at 10:49:08AM +0100, Lars-Peter Clausen wrote:
This patch adds a set of functions which are intended to be used when implementing a dmaengine based sound PCM driver.
Looks good - if you need to resend then:
- Note that this function will use private_data field of the substream's
- runtime. So it is not availabe to your pcm driver implementation. If you need
- to keep additional data attached to a substream use
- snd_dmaeinge_pcm_{set,get}_data.
there's a typo here but no need to resend just for that.
I'd like to see some review from both Morimoto-san as we should convert fsi over to this too, Vinod I guess you're also pretty much happy given your comments on the previous version?
For the non-cyclic DMAs the idea of emulating at the dmaengine layer does seem very sensible but if that's hard then having the code at the ASoC level and pushing it down later seems fine. We do have several platforms with non-cyclic DMA so it's a general need.
On Wed, 2012-02-22 at 13:21 +0000, Mark Brown wrote:
On Wed, Feb 22, 2012 at 10:49:08AM +0100, Lars-Peter Clausen wrote:
This patch adds a set of functions which are intended to be used when implementing a dmaengine based sound PCM driver.
Looks good - if you need to resend then:
- Note that this function will use private_data field of the substream's
- runtime. So it is not availabe to your pcm driver implementation. If you need
- to keep additional data attached to a substream use
- snd_dmaeinge_pcm_{set,get}_data.
there's a typo here but no need to resend just for that.
I'd like to see some review from both Morimoto-san as we should convert fsi over to this too, Vinod I guess you're also pretty much happy given your comments on the previous version?
Sure, Acked-by :Vinod Koul vinod.koul@linux.intel.com
For the non-cyclic DMAs the idea of emulating at the dmaengine layer does seem very sensible but if that's hard then having the code at the ASoC level and pushing it down later seems fine. We do have several platforms with non-cyclic DMA so it's a general need.
I think this should be pushed to dmaengine rather than in ASoC. That way ASoC can always request circular and we emulate the behavior in dmaengine for dmacs who don't support this
On Wed, Feb 22, 2012 at 07:00:09PM +0530, Vinod Koul wrote:
On Wed, 2012-02-22 at 13:21 +0000, Mark Brown wrote:
For the non-cyclic DMAs the idea of emulating at the dmaengine layer does seem very sensible but if that's hard then having the code at the ASoC level and pushing it down later seems fine. We do have several platforms with non-cyclic DMA so it's a general need.
I think this should be pushed to dmaengine rather than in ASoC. That way ASoC can always request circular and we emulate the behavior in dmaengine for dmacs who don't support this
That's what I just said. I'm just saying that if for some reason it's going to be difficult to implement I don't mind carrying code at the ASoC layer while that work is done.
On Wed, Feb 22, 2012 at 01:21:08PM +0000, Mark Brown wrote:
On Wed, Feb 22, 2012 at 10:49:08AM +0100, Lars-Peter Clausen wrote:
This patch adds a set of functions which are intended to be used when implementing a dmaengine based sound PCM driver.
Looks good - if you need to resend then:
- Note that this function will use private_data field of the substream's
- runtime. So it is not availabe to your pcm driver implementation. If you need
- to keep additional data attached to a substream use
- snd_dmaeinge_pcm_{set,get}_data.
there's a typo here but no need to resend just for that.
I'd like to see some review from both Morimoto-san as we should convert fsi over to this too, Vinod I guess you're also pretty much happy given your comments on the previous version?
For the non-cyclic DMAs the idea of emulating at the dmaengine layer does seem very sensible but if that's hard then having the code at the ASoC level and pushing it down later seems fine. We do have several platforms with non-cyclic DMA so it's a general need.
I think you're making the assumption that other people need cyclic transfers. I've seen little evidence that anyone other than sound needs such things, so I don't think there's justification to push this code into every DMA engine driver.
Remember, there's no common code to a DMA engine driver at all, everyone implements their own way with their own bugs. I would agree with you if there was some decent DMA engine infrastructure to abstract out such things, but there isn't.
So what you're asking is for N different ways of doing this, instead of having one centralized way.
On Wed, Feb 22, 2012 at 01:32:07PM +0000, Russell King - ARM Linux wrote:
I think you're making the assumption that other people need cyclic transfers. I've seen little evidence that anyone other than sound needs such things, so I don't think there's justification to push this code into every DMA engine driver.
The idea was to have the library code to do this in the dmaengine core rather in drivers. Doing it in the drivers would be silly, it should be in library code somewhere. If we are going to librify it then pushing the code as far up the stack as we can seems like a smart move.
Remember, there's no common code to a DMA engine driver at all, everyone implements their own way with their own bugs. I would agree with you if there was some decent DMA engine infrastructure to abstract out such things, but there isn't.
So what you're asking is for N different ways of doing this, instead of having one centralized way.
Well, Vinod seemed to also think that dmaengine ought to be able to cope with emulating this and I really don't see why it shouldn't be. It does know the capabilities of the underlying driver and it gets to look at all the calls going into the driver before the driver does. If ASoC can interpose itself I don't see why dmaengine can't.
On Wed, Feb 22, 2012 at 01:39:39PM +0000, Mark Brown wrote:
Well, Vinod seemed to also think that dmaengine ought to be able to cope with emulating this and I really don't see why it shouldn't be. It does know the capabilities of the underlying driver and it gets to look at all the calls going into the driver before the driver does. If ASoC can interpose itself I don't see why dmaengine can't.
Have you ever looked at the DMA engine drivers? It's a total mess. Even something as basic as the DMA engine driver assigning a cookie to the descriptor is implemented in each driver in their own unique way.
Completion of DMA descriptors is a similar thing - every driver implements this in their own unique way.
The only really common thing in the DMA engine drivers is that they all share the same API. Nothing much more than that is shared.
It's something I have been chipping away at for a while, trying to extract out common parts into a library, but it's really not that easy.
So, there's no way in hell that I want to see any more stuff pushed down into the DMA engine drivers until we have a proper DMA engine library in place to stop all this idiotic code duplication throughout every driver. Especially if it means everyone will be implementing their own cyclic emulation in their individual drivers.
And lets not forget that it's not just a question of "oh we can just queue up buffer after buffer". Doing it properly for audio does need to ensure that the DMA engine driver is capable of getting the next buffer chained into the hardware before the previous buffer has completed, so the transfer continues across a buffer boundary without needing the intervention of an interrupt handler. Otherwise you get glitches in the audio.
So, there's a lot more here that DMA engine stuff needs to get fixed than just a simple "layer on top".
On Wed, Feb 22, 2012 at 02:22:36PM +0000, Russell King - ARM Linux wrote:
On Wed, Feb 22, 2012 at 01:39:39PM +0000, Mark Brown wrote:
Well, Vinod seemed to also think that dmaengine ought to be able to cope with emulating this and I really don't see why it shouldn't be. It does know the capabilities of the underlying driver and it gets to look at all the calls going into the driver before the driver does. If ASoC can interpose itself I don't see why dmaengine can't.
Have you ever looked at the DMA engine drivers? It's a total mess.
Not in enormous detail, no.
Even something as basic as the DMA engine driver assigning a cookie to the descriptor is implemented in each driver in their own unique way.
Completion of DMA descriptors is a similar thing - every driver implements this in their own unique way.
The only really common thing in the DMA engine drivers is that they all share the same API. Nothing much more than that is shared.
Plus they're going through a common vtable and accessors which is the main thing here I expect.
So, there's no way in hell that I want to see any more stuff pushed down into the DMA engine drivers until we have a proper DMA engine library in place to stop all this idiotic code duplication throughout every driver. Especially if it means everyone will be implementing their own cyclic emulation in their individual drivers.
I've not seen any suggestion from anyone that that's a good idea.
So, there's a lot more here that DMA engine stuff needs to get fixed than just a simple "layer on top".
Well, if we can't do it with a layer in the dmaengine code then I don't see how we could do it in ASoC either, presumably the issues that block doing it in the dmaengine core code would also cause issues doing it in ASoC?
I'm not saying there's nothing that needs fixing in the dmaengine code, and like I say I'm not too worried about carrying things in ASoC if the dmaengine code is intractably difficult, but I'm not sure that the issues you're rasing aren't orthogonal to the issues with emulating cyclic transfers.
On Wed, Feb 22, 2012 at 03:04:56PM +0000, Mark Brown wrote:
Well, if we can't do it with a layer in the dmaengine code then I don't see how we could do it in ASoC either, presumably the issues that block doing it in the dmaengine core code would also cause issues doing it in ASoC?
I'm not saying there's nothing that needs fixing in the dmaengine code, and like I say I'm not too worried about carrying things in ASoC if the dmaengine code is intractably difficult, but I'm not sure that the issues you're rasing aren't orthogonal to the issues with emulating cyclic transfers.
Look, I spent a number of weeks working on the SA11x0 DMA engine code and ALSA side to get something which worked reliably, and a simple solution doesn't work. I've been looking at the DMA engine code probably for longer than Vinod has been involved with it. I've been working on cleaning up all the DMA engine drivers extracting some of the common bits from them. I'm very familiar with level of crap in the DMA engine stuff. I know what I'm talking about.
The point that I'm making is that there's more to this than just adding a layer. If you think that's all that there is, then you haven't properly understood the SA11x0 audio support patch set that I have, and the interactions between the individual patches. That's not surprising because I haven't posted the patches yet, and I haven't explained them either.
And since I'm spending today working on x86 kernel instability (thanks to Linux for taking out my laptop last night) I've little time to spend discussing the fine points of this, let alone any other ARM stuff.
On Wed, Feb 22, 2012 at 03:23:55PM +0000, Russell King - ARM Linux wrote:
Look, I spent a number of weeks working on the SA11x0 DMA engine code and ALSA side to get something which worked reliably, and a simple solution doesn't work. I've been looking at the DMA engine code probably for longer than Vinod has been involved with it. I've been working on cleaning up all the DMA engine drivers extracting some of the common bits from them. I'm very familiar with level of crap in the DMA engine stuff. I know what I'm talking about.
OK, fair enough - like I say the thing I'm not getting is why this is resolvable in a dmaengine client but not in dmaengine itself. If nothing else I'd expect the core to be able to insert a proxy client.
The point that I'm making is that there's more to this than just adding a layer. If you think that's all that there is, then you haven't properly understood the SA11x0 audio support patch set that I have, and the interactions between the individual patches. That's not surprising because I haven't posted the patches yet, and I haven't explained them either.
Indeed, I've not really looked at the code in any great detail at your code as you didn't post it (I only looked at all because it showed up in -next) and there were some issues at the subsystem level so I stopped pretty quickly.
On Wed, 2012-02-22 at 14:22 +0000, Russell King - ARM Linux wrote:
On Wed, Feb 22, 2012 at 01:39:39PM +0000, Mark Brown wrote:
Well, Vinod seemed to also think that dmaengine ought to be able to cope with emulating this and I really don't see why it shouldn't be. It does know the capabilities of the underlying driver and it gets to look at all the calls going into the driver before the driver does. If ASoC can interpose itself I don't see why dmaengine can't.
Have you ever looked at the DMA engine drivers? It's a total mess. Even something as basic as the DMA engine driver assigning a cookie to the descriptor is implemented in each driver in their own unique way.
Completion of DMA descriptors is a similar thing - every driver implements this in their own unique way.
The only really common thing in the DMA engine drivers is that they all share the same API. Nothing much more than that is shared.
It's something I have been chipping away at for a while, trying to extract out common parts into a library, but it's really not that easy.
So, there's no way in hell that I want to see any more stuff pushed down into the DMA engine drivers until we have a proper DMA engine library in place to stop all this idiotic code duplication throughout every driver. Especially if it means everyone will be implementing their own cyclic emulation in their individual drivers.
And lets not forget that it's not just a question of "oh we can just queue up buffer after buffer". Doing it properly for audio does need to ensure that the DMA engine driver is capable of getting the next buffer chained into the hardware before the previous buffer has completed, so the transfer continues across a buffer boundary without needing the intervention of an interrupt handler. Otherwise you get glitches in the audio.
So, there's a lot more here that DMA engine stuff needs to get fixed than just a simple "layer on top".
I would agree with Russell that some of dma drivers are messy, but then were also a victim of ambiguous API. Now at least we have a clear direction of what is expected out of dma driver and what is not. Except for channel mapping stuff, rest has a clarity.
So the new drivers and fixes now are going in the right direction (hopefully you will agree with me on this) and we are frankly in much better state then we were one year back.
Said that, we still have lot of work to do to improve :) And I see this as another opportunity. Having a common library for these kind of common subsystem operations forces people to write proper driver and forces them to fix existing issue. Now, new support for DMAengine in asoc wont come without using this library and if it does work on a particular driver that is precondition for fixes as well.
Similarly I would really love to see such libraries in other subsystems which would help us in making the usage of dmaengine and dmac standard and straight forward for users.
On Wed, Feb 22, 2012 at 10:49:08AM +0100, Lars-Peter Clausen wrote:
This patch adds a set of functions which are intended to be used when implementing a dmaengine based sound PCM driver.
Having this framework is a substantial win and I got bored waiting for the repost of this and the subsequent patches (especially given that the merge window is drawing nearer). Since the fix for ep93xx is so simple and it works on i.MX I went ahead and applied everything except the ep93xx conversion. Thanks!
Please resend the ep93xx conversion along with the tweak to the core so we can get that working. It would also be good to update FSI to use this library as it's also using dmaengine (the patch having been merged just before this one it got missed in the updates I expect).
On 03/02/2012 02:59 PM, Mark Brown wrote:
On Wed, Feb 22, 2012 at 10:49:08AM +0100, Lars-Peter Clausen wrote:
This patch adds a set of functions which are intended to be used when implementing a dmaengine based sound PCM driver.
Having this framework is a substantial win and I got bored waiting for the repost of this and the subsequent patches (especially given that the merge window is drawing nearer). Since the fix for ep93xx is so simple and it works on i.MX I went ahead and applied everything except the ep93xx conversion. Thanks!
Please resend the ep93xx conversion along with the tweak to the core so we can get that working.
I was at trade show for the rest of last week, without much time for anything else. I will submit the missing patches in a moment.
It would also be good to update FSI to use this library as it's also using dmaengine (the patch having been merged just before this one it got missed in the updates I expect).
I've had a look at the FSI dmaengine driver and it is not straight forward to convert. For one it does not use cyclic dma transfers so it is more something for Russel's patch and on the other hand the FSI PCM driver introduces another layer of indirection which is used to be able to switch between PIO and DMA. But as a result the functions the dmaengine PCM framework provides don't really fit anymore.
- Lars
On Mon, Mar 05, 2012 at 01:30:42PM +0100, Lars-Peter Clausen wrote:
On 03/02/2012 02:59 PM, Mark Brown wrote:
Please resend the ep93xx conversion along with the tweak to the core so we can get that working.
I was at trade show for the rest of last week, without much time for anything else. I will submit the missing patches in a moment.
No problem, I figured it was something like that and didn't see any reason not to get the changes into 3.4 - it's a substantial win all round and the fixes are pretty minior.
It would also be good to update FSI to use this library as it's also using dmaengine (the patch having been merged just before this one it got missed in the updates I expect).
I've had a look at the FSI dmaengine driver and it is not straight forward to convert. For one it does not use cyclic dma transfers so it is more something for Russel's patch and on the other hand the FSI PCM driver introduces another layer of indirection which is used to be able to switch between PIO and DMA. But as a result the functions the dmaengine PCM framework provides don't really fit anymore.
OK, fair enough.
Hi Lars, Mark
It would also be good to update FSI to use this library as it's also using dmaengine (the patch having been merged just before this one it got missed in the updates I expect).
I've had a look at the FSI dmaengine driver and it is not straight forward to convert. For one it does not use cyclic dma transfers so it is more something for Russel's patch and on the other hand the FSI PCM driver introduces another layer of indirection which is used to be able to switch between PIO and DMA. But as a result the functions the dmaengine PCM framework provides don't really fit anymore.
OK, fair enough.
FSI can switch PIO/DMA, but it will be decided on _probe() timing, not runtime switch. PIO/DMA is depend on SuperH CPU/platform supporting status.
And yes, it doesn't use cyclic dma transfer since I don't know how to use it :)
I forgot detail, but I used sound/soc/sh/siu_pcm.c and sound/soc/txx9/txx9aclc.c as sample code when I created FSI DMAEngine support.
If ASoC layer supports dmaengine helper functions, I can switch to it by my incremental patch.
Best regards --- Kuninori Morimoto
On Tue, Mar 06, 2012 at 04:38:30PM -0800, Kuninori Morimoto wrote:
And yes, it doesn't use cyclic dma transfer since I don't know how to use it :)
I forgot detail, but I used sound/soc/sh/siu_pcm.c and sound/soc/txx9/txx9aclc.c as sample code when I created FSI DMAEngine support.
If ASoC layer supports dmaengine helper functions, I can switch to it by my incremental patch.
OK, great - if the hardware supports cyclic mode then I guess the best thing is to enable that in dmaengine then switch over to using the new library once that's merged (unless someone comes along and implements cyclic emulation support first).
Signed-off-by: Lars-Peter Clausen lars@metafoo.de Tested-by: Shawn Guo shawn.guo@linaro.org --- sound/soc/imx/Kconfig | 1 + sound/soc/imx/imx-pcm-dma-mx2.c | 176 ++++++--------------------------------- 2 files changed, 28 insertions(+), 149 deletions(-)
diff --git a/sound/soc/imx/Kconfig b/sound/soc/imx/Kconfig index 91b66ef..08c4b9b 100644 --- a/sound/soc/imx/Kconfig +++ b/sound/soc/imx/Kconfig @@ -14,6 +14,7 @@ config SND_MXC_SOC_FIQ tristate
config SND_MXC_SOC_MX2 + select SND_SOC_DMAENGINE_PCM tristate
config SND_MXC_SOC_WM1133_EV1 diff --git a/sound/soc/imx/imx-pcm-dma-mx2.c b/sound/soc/imx/imx-pcm-dma-mx2.c index f974e61..4cd5462 100644 --- a/sound/soc/imx/imx-pcm-dma-mx2.c +++ b/sound/soc/imx/imx-pcm-dma-mx2.c @@ -27,104 +27,42 @@ #include <sound/pcm.h> #include <sound/pcm_params.h> #include <sound/soc.h> +#include <sound/dmaengine_pcm.h>
#include <mach/dma.h>
#include "imx-ssi.h"
-struct imx_pcm_runtime_data { - int period_bytes; - int periods; - unsigned long offset; - struct dma_async_tx_descriptor *desc; - struct dma_chan *dma_chan; - struct imx_dma_data dma_data; -}; - -static void audio_dma_irq(void *data) -{ - struct snd_pcm_substream *substream = (struct snd_pcm_substream *)data; - struct snd_pcm_runtime *runtime = substream->runtime; - struct imx_pcm_runtime_data *iprtd = runtime->private_data; - - iprtd->offset += iprtd->period_bytes; - iprtd->offset %= iprtd->period_bytes * iprtd->periods; - - snd_pcm_period_elapsed(substream); -} - static bool filter(struct dma_chan *chan, void *param) { - struct imx_pcm_runtime_data *iprtd = param; - if (!imx_dma_is_general_purpose(chan)) return false;
- chan->private = &iprtd->dma_data; + chan->private = param;
- return true; -} - -static int imx_ssi_dma_alloc(struct snd_pcm_substream *substream) -{ - struct snd_soc_pcm_runtime *rtd = substream->private_data; - struct imx_pcm_dma_params *dma_params; - struct snd_pcm_runtime *runtime = substream->runtime; - struct imx_pcm_runtime_data *iprtd = runtime->private_data; - dma_cap_mask_t mask; - - dma_params = snd_soc_dai_get_dma_data(rtd->cpu_dai, substream); - - iprtd->dma_data.peripheral_type = IMX_DMATYPE_SSI; - iprtd->dma_data.priority = DMA_PRIO_HIGH; - iprtd->dma_data.dma_request = dma_params->dma; - - /* Try to grab a DMA channel */ - dma_cap_zero(mask); - dma_cap_set(DMA_SLAVE, mask); - iprtd->dma_chan = dma_request_channel(mask, filter, iprtd); - if (!iprtd->dma_chan) - return -EINVAL; - - return 0; + return true; }
static int snd_imx_pcm_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params) { struct snd_soc_pcm_runtime *rtd = substream->private_data; - struct snd_pcm_runtime *runtime = substream->runtime; - struct imx_pcm_runtime_data *iprtd = runtime->private_data; - struct dma_chan *chan = iprtd->dma_chan; + struct dma_chan *chan = snd_dmaengine_pcm_get_chan(substream); struct imx_pcm_dma_params *dma_params; struct dma_slave_config slave_config; - enum dma_slave_buswidth buswidth; - unsigned long dma_addr; int ret;
dma_params = snd_soc_dai_get_dma_data(rtd->cpu_dai, substream);
- switch (params_format(params)) { - case SNDRV_PCM_FORMAT_S16_LE: - buswidth = DMA_SLAVE_BUSWIDTH_2_BYTES; - break; - case SNDRV_PCM_FORMAT_S20_3LE: - case SNDRV_PCM_FORMAT_S24_LE: - buswidth = DMA_SLAVE_BUSWIDTH_4_BYTES; - break; - default: - return 0; - } + ret = snd_hwparams_to_dma_slave_config(substream, params, &slave_config); + if (ret) + return ret;
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { - slave_config.direction = DMA_MEM_TO_DEV; slave_config.dst_addr = dma_params->dma_addr; - slave_config.dst_addr_width = buswidth; slave_config.dst_maxburst = dma_params->burstsize; } else { - slave_config.direction = DMA_DEV_TO_MEM; slave_config.src_addr = dma_params->dma_addr; - slave_config.src_addr_width = buswidth; slave_config.src_maxburst = dma_params->burstsize; }
@@ -132,68 +70,11 @@ static int snd_imx_pcm_hw_params(struct snd_pcm_substream *substream, if (ret) return ret;
- - iprtd->periods = params_periods(params); - iprtd->period_bytes = params_period_bytes(params); - iprtd->offset = 0; - snd_pcm_set_runtime_buffer(substream, &substream->dma_buffer);
- dma_addr = runtime->dma_addr; - - iprtd->desc = chan->device->device_prep_dma_cyclic(chan, dma_addr, - iprtd->period_bytes * iprtd->periods, - iprtd->period_bytes, - substream->stream == SNDRV_PCM_STREAM_PLAYBACK ? - DMA_MEM_TO_DEV : DMA_DEV_TO_MEM); - if (!iprtd->desc) { - dev_err(&chan->dev->device, "cannot prepare slave dma\n"); - return -EINVAL; - } - - iprtd->desc->callback = audio_dma_irq; - iprtd->desc->callback_param = substream; - - return 0; -} - -static int snd_imx_pcm_trigger(struct snd_pcm_substream *substream, int cmd) -{ - struct snd_pcm_runtime *runtime = substream->runtime; - struct imx_pcm_runtime_data *iprtd = runtime->private_data; - - switch (cmd) { - case SNDRV_PCM_TRIGGER_START: - case SNDRV_PCM_TRIGGER_RESUME: - case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: - dmaengine_submit(iprtd->desc); - - break; - - case SNDRV_PCM_TRIGGER_STOP: - case SNDRV_PCM_TRIGGER_SUSPEND: - case SNDRV_PCM_TRIGGER_PAUSE_PUSH: - dmaengine_terminate_all(iprtd->dma_chan); - - break; - default: - return -EINVAL; - } - return 0; }
-static snd_pcm_uframes_t snd_imx_pcm_pointer(struct snd_pcm_substream *substream) -{ - struct snd_pcm_runtime *runtime = substream->runtime; - struct imx_pcm_runtime_data *iprtd = runtime->private_data; - - pr_debug("%s: %ld %ld\n", __func__, iprtd->offset, - bytes_to_frames(substream->runtime, iprtd->offset)); - - return bytes_to_frames(substream->runtime, iprtd->offset); -} - static struct snd_pcm_hardware snd_imx_hardware = { .info = SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER | @@ -215,40 +96,37 @@ static struct snd_pcm_hardware snd_imx_hardware = {
static int snd_imx_open(struct snd_pcm_substream *substream) { - struct snd_pcm_runtime *runtime = substream->runtime; - struct imx_pcm_runtime_data *iprtd; + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct imx_pcm_dma_params *dma_params; + struct imx_dma_data *dma_data; int ret;
- iprtd = kzalloc(sizeof(*iprtd), GFP_KERNEL); - if (iprtd == NULL) - return -ENOMEM; - runtime->private_data = iprtd; + snd_soc_set_runtime_hwparams(substream, &snd_imx_hardware);
- ret = snd_pcm_hw_constraint_integer(substream->runtime, - SNDRV_PCM_HW_PARAM_PERIODS); - if (ret < 0) { - kfree(iprtd); - return ret; - } + dma_params = snd_soc_dai_get_dma_data(rtd->cpu_dai, substream);
- ret = imx_ssi_dma_alloc(substream); - if (ret < 0) { - kfree(iprtd); - return ret; + dma_data = kzalloc(sizeof(*dma_data), GFP_KERNEL); + dma_data->peripheral_type = IMX_DMATYPE_SSI; + dma_data->priority = DMA_PRIO_HIGH; + dma_data->dma_request = dma_params->dma; + + ret = snd_dmaengine_pcm_open(substream, filter, dma_data); + if (ret) { + kfree(dma_data); + return 0; }
- snd_soc_set_runtime_hwparams(substream, &snd_imx_hardware); + snd_dmaengine_pcm_set_data(substream, dma_data);
return 0; }
static int snd_imx_close(struct snd_pcm_substream *substream) { - struct snd_pcm_runtime *runtime = substream->runtime; - struct imx_pcm_runtime_data *iprtd = runtime->private_data; + struct imx_dma_data *dma_data = snd_dmaengine_pcm_get_data(substream);
- dma_release_channel(iprtd->dma_chan); - kfree(iprtd); + snd_dmaengine_pcm_close(substream); + kfree(dma_data);
return 0; } @@ -258,8 +136,8 @@ static struct snd_pcm_ops imx_pcm_ops = { .close = snd_imx_close, .ioctl = snd_pcm_lib_ioctl, .hw_params = snd_imx_pcm_hw_params, - .trigger = snd_imx_pcm_trigger, - .pointer = snd_imx_pcm_pointer, + .trigger = snd_dmaengine_pcm_trigger, + .pointer = snd_dmaengine_pcm_pointer, .mmap = snd_imx_pcm_mmap, };
Signed-off-by: Lars-Peter Clausen lars@metafoo.de Tested-by: Shawn Guo shawn.guo@linaro.org --- sound/soc/mxs/Kconfig | 1 + sound/soc/mxs/mxs-pcm.c | 136 ++++++++-------------------------------------- sound/soc/mxs/mxs-pcm.h | 12 ---- 3 files changed, 25 insertions(+), 124 deletions(-)
diff --git a/sound/soc/mxs/Kconfig b/sound/soc/mxs/Kconfig index 21d20f3..99a997f 100644 --- a/sound/soc/mxs/Kconfig +++ b/sound/soc/mxs/Kconfig @@ -1,6 +1,7 @@ menuconfig SND_MXS_SOC tristate "SoC Audio for Freescale MXS CPUs" depends on ARCH_MXS + select SND_SOC_DMAENGINE_PCM help Say Y or M if you want to add support for codecs attached to the MXS SAIF interface. diff --git a/sound/soc/mxs/mxs-pcm.c b/sound/soc/mxs/mxs-pcm.c index 5b8c8d3..6ca1f46 100644 --- a/sound/soc/mxs/mxs-pcm.c +++ b/sound/soc/mxs/mxs-pcm.c @@ -34,10 +34,16 @@ #include <sound/pcm.h> #include <sound/pcm_params.h> #include <sound/soc.h> +#include <sound/dmaengine_pcm.h>
#include <mach/dma.h> #include "mxs-pcm.h"
+struct mxs_pcm_dma_data { + struct mxs_dma_data dma_data; + struct mxs_pcm_dma_params *dma_params; +}; + static struct snd_pcm_hardware snd_mxs_hardware = { .info = SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID | @@ -58,21 +64,10 @@ static struct snd_pcm_hardware snd_mxs_hardware = {
};
-static void audio_dma_irq(void *data) -{ - struct snd_pcm_substream *substream = (struct snd_pcm_substream *)data; - struct snd_pcm_runtime *runtime = substream->runtime; - struct mxs_pcm_runtime_data *iprtd = runtime->private_data; - - iprtd->offset += iprtd->period_bytes; - iprtd->offset %= iprtd->period_bytes * iprtd->periods; - snd_pcm_period_elapsed(substream); -} - static bool filter(struct dma_chan *chan, void *param) { - struct mxs_pcm_runtime_data *iprtd = param; - struct mxs_pcm_dma_params *dma_params = iprtd->dma_params; + struct mxs_pcm_dma_data *pcm_dma_data = param; + struct mxs_pcm_dma_params *dma_params = pcm_dma_data->dma_params;
if (!mxs_dma_is_apbx(chan)) return false; @@ -80,134 +75,51 @@ static bool filter(struct dma_chan *chan, void *param) if (chan->chan_id != dma_params->chan_num) return false;
- chan->private = &iprtd->dma_data; + chan->private = &pcm_dma_data->dma_data;
return true; }
-static int mxs_dma_alloc(struct snd_pcm_substream *substream) -{ - struct snd_soc_pcm_runtime *rtd = substream->private_data; - struct snd_pcm_runtime *runtime = substream->runtime; - struct mxs_pcm_runtime_data *iprtd = runtime->private_data; - dma_cap_mask_t mask; - - iprtd->dma_params = snd_soc_dai_get_dma_data(rtd->cpu_dai, substream); - - dma_cap_zero(mask); - dma_cap_set(DMA_SLAVE, mask); - iprtd->dma_data.chan_irq = iprtd->dma_params->chan_irq; - iprtd->dma_chan = dma_request_channel(mask, filter, iprtd); - if (!iprtd->dma_chan) - return -EINVAL; - - return 0; -} - static int snd_mxs_pcm_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params) { - struct snd_pcm_runtime *runtime = substream->runtime; - struct mxs_pcm_runtime_data *iprtd = runtime->private_data; - unsigned long dma_addr; - struct dma_chan *chan; - - chan = iprtd->dma_chan; - - iprtd->periods = params_periods(params); - iprtd->period_bytes = params_period_bytes(params); - iprtd->offset = 0; - snd_pcm_set_runtime_buffer(substream, &substream->dma_buffer);
- dma_addr = runtime->dma_addr; - - iprtd->desc = chan->device->device_prep_dma_cyclic(chan, dma_addr, - iprtd->period_bytes * iprtd->periods, - iprtd->period_bytes, - substream->stream == SNDRV_PCM_STREAM_PLAYBACK ? - DMA_MEM_TO_DEV : DMA_DEV_TO_MEM); - if (!iprtd->desc) { - dev_err(&chan->dev->device, "cannot prepare slave dma\n"); - return -EINVAL; - } - - iprtd->desc->callback = audio_dma_irq; - iprtd->desc->callback_param = substream; - return 0; }
-static int snd_mxs_pcm_trigger(struct snd_pcm_substream *substream, int cmd) -{ - struct snd_pcm_runtime *runtime = substream->runtime; - struct mxs_pcm_runtime_data *iprtd = runtime->private_data; - - switch (cmd) { - case SNDRV_PCM_TRIGGER_START: - case SNDRV_PCM_TRIGGER_RESUME: - case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: - dmaengine_submit(iprtd->desc); - - break; - case SNDRV_PCM_TRIGGER_STOP: - case SNDRV_PCM_TRIGGER_SUSPEND: - case SNDRV_PCM_TRIGGER_PAUSE_PUSH: - dmaengine_terminate_all(iprtd->dma_chan); - - break; - default: - return -EINVAL; - } - - return 0; -} - -static snd_pcm_uframes_t snd_mxs_pcm_pointer( - struct snd_pcm_substream *substream) -{ - struct snd_pcm_runtime *runtime = substream->runtime; - struct mxs_pcm_runtime_data *iprtd = runtime->private_data; - - return bytes_to_frames(substream->runtime, iprtd->offset); -} - static int snd_mxs_open(struct snd_pcm_substream *substream) { - struct snd_pcm_runtime *runtime = substream->runtime; - struct mxs_pcm_runtime_data *iprtd; + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct mxs_pcm_dma_data *pcm_dma_data; int ret;
- iprtd = kzalloc(sizeof(*iprtd), GFP_KERNEL); - if (iprtd == NULL) + pcm_dma_data = kzalloc(sizeof(*pcm_dma_data), GFP_KERNEL); + if (pcm_dma_data == NULL) return -ENOMEM; - runtime->private_data = iprtd;
- ret = snd_pcm_hw_constraint_integer(substream->runtime, - SNDRV_PCM_HW_PARAM_PERIODS); - if (ret < 0) { - kfree(iprtd); - return ret; - } + pcm_dma_data->dma_params = snd_soc_dai_get_dma_data(rtd->cpu_dai, substream); + pcm_dma_data->dma_data.chan_irq = pcm_dma_data->dma_params->chan_irq;
- ret = mxs_dma_alloc(substream); + ret = snd_dmaengine_pcm_open(substream, filter, pcm_dma_data); if (ret) { - kfree(iprtd); + kfree(pcm_dma_data); return ret; }
snd_soc_set_runtime_hwparams(substream, &snd_mxs_hardware);
+ snd_dmaengine_pcm_set_data(substream, pcm_dma_data); + return 0; }
static int snd_mxs_close(struct snd_pcm_substream *substream) { - struct snd_pcm_runtime *runtime = substream->runtime; - struct mxs_pcm_runtime_data *iprtd = runtime->private_data; + struct mxs_pcm_dma_data *pcm_dma_data = snd_dmaengine_pcm_get_data(substream);
- dma_release_channel(iprtd->dma_chan); - kfree(iprtd); + snd_dmaengine_pcm_close(substream); + kfree(pcm_dma_data);
return 0; } @@ -228,8 +140,8 @@ static struct snd_pcm_ops mxs_pcm_ops = { .close = snd_mxs_close, .ioctl = snd_pcm_lib_ioctl, .hw_params = snd_mxs_pcm_hw_params, - .trigger = snd_mxs_pcm_trigger, - .pointer = snd_mxs_pcm_pointer, + .trigger = snd_dmaengine_pcm_trigger, + .pointer = snd_dmaengine_pcm_pointer, .mmap = snd_mxs_pcm_mmap, };
diff --git a/sound/soc/mxs/mxs-pcm.h b/sound/soc/mxs/mxs-pcm.h index ba75e10..5f01a91 100644 --- a/sound/soc/mxs/mxs-pcm.h +++ b/sound/soc/mxs/mxs-pcm.h @@ -19,21 +19,9 @@ #ifndef _MXS_PCM_H #define _MXS_PCM_H
-#include <mach/dma.h> - struct mxs_pcm_dma_params { int chan_irq; int chan_num; };
-struct mxs_pcm_runtime_data { - int period_bytes; - int periods; - unsigned long offset; - struct dma_async_tx_descriptor *desc; - struct dma_chan *dma_chan; - struct mxs_dma_data dma_data; - struct mxs_pcm_dma_params *dma_params; -}; - #endif
Signed-off-by: Lars-Peter Clausen lars@metafoo.de --- sound/soc/ep93xx/Kconfig | 1 + sound/soc/ep93xx/ep93xx-pcm.c | 148 ++++++----------------------------------- 2 files changed, 23 insertions(+), 126 deletions(-)
diff --git a/sound/soc/ep93xx/Kconfig b/sound/soc/ep93xx/Kconfig index 91a28de..88143db 100644 --- a/sound/soc/ep93xx/Kconfig +++ b/sound/soc/ep93xx/Kconfig @@ -1,6 +1,7 @@ config SND_EP93XX_SOC tristate "SoC Audio support for the Cirrus Logic EP93xx series" depends on ARCH_EP93XX && SND_SOC + select SND_SOC_DMAENGINE_PCM help Say Y or M if you want to add support for codecs attached to the EP93xx I2S or AC97 interfaces. diff --git a/sound/soc/ep93xx/ep93xx-pcm.c b/sound/soc/ep93xx/ep93xx-pcm.c index 32adca3..162dbb7 100644 --- a/sound/soc/ep93xx/ep93xx-pcm.c +++ b/sound/soc/ep93xx/ep93xx-pcm.c @@ -23,6 +23,7 @@ #include <sound/pcm.h> #include <sound/pcm_params.h> #include <sound/soc.h> +#include <sound/dmaengine_pcm.h>
#include <mach/dma.h> #include <mach/hardware.h> @@ -52,26 +53,6 @@ static const struct snd_pcm_hardware ep93xx_pcm_hardware = { .fifo_size = 32, };
-struct ep93xx_runtime_data -{ - int pointer_bytes; - int periods; - int period_bytes; - struct dma_chan *dma_chan; - struct ep93xx_dma_data dma_data; -}; - -static void ep93xx_pcm_dma_callback(void *data) -{ - struct snd_pcm_substream *substream = data; - struct ep93xx_runtime_data *rtd = substream->runtime->private_data; - - rtd->pointer_bytes += rtd->period_bytes; - rtd->pointer_bytes %= rtd->period_bytes * rtd->periods; - - snd_pcm_period_elapsed(substream); -} - static bool ep93xx_pcm_dma_filter(struct dma_chan *chan, void *filter_param) { struct ep93xx_dma_data *data = filter_param; @@ -86,98 +67,48 @@ static bool ep93xx_pcm_dma_filter(struct dma_chan *chan, void *filter_param)
static int ep93xx_pcm_open(struct snd_pcm_substream *substream) { - struct snd_soc_pcm_runtime *soc_rtd = substream->private_data; - struct snd_soc_dai *cpu_dai = soc_rtd->cpu_dai; + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct snd_soc_dai *cpu_dai = rtd->cpu_dai; struct ep93xx_pcm_dma_params *dma_params; - struct ep93xx_runtime_data *rtd; - dma_cap_mask_t mask; + struct ep93xx_dma_data *dma_data; int ret;
- ret = snd_pcm_hw_constraint_integer(substream->runtime, - SNDRV_PCM_HW_PARAM_PERIODS); - if (ret < 0) - return ret; - snd_soc_set_runtime_hwparams(substream, &ep93xx_pcm_hardware);
- rtd = kmalloc(sizeof(*rtd), GFP_KERNEL); - if (!rtd) + dma_data = kmalloc(sizeof(*dma_data), GFP_KERNEL); + if (!dma_data) return -ENOMEM;
- dma_cap_zero(mask); - dma_cap_set(DMA_SLAVE, mask); - dma_cap_set(DMA_CYCLIC, mask); - dma_params = snd_soc_dai_get_dma_data(cpu_dai, substream); - rtd->dma_data.port = dma_params->dma_port; - rtd->dma_data.name = dma_params->name; - - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) - rtd->dma_data.direction = DMA_MEM_TO_DEV; - else - rtd->dma_data.direction = DMA_DEV_TO_MEM; - - rtd->dma_chan = dma_request_channel(mask, ep93xx_pcm_dma_filter, - &rtd->dma_data); - if (!rtd->dma_chan) { - kfree(rtd); - return -EINVAL; - } - - substream->runtime->private_data = rtd; - return 0; -} + dma_data->port = dma_params->dma_port; + dma_data->name = dma_params->name; + dma_data->direction = snd_pcm_substream_to_dma_direction(substream);
-static int ep93xx_pcm_close(struct snd_pcm_substream *substream) -{ - struct ep93xx_runtime_data *rtd = substream->runtime->private_data; + ret = snd_dmaengine_pcm_open(substream, ep93xx_pcm_dma_filter, dma_data); + if (ret) { + kfree(dma_data); + return ret; + }
- dma_release_channel(rtd->dma_chan); - kfree(rtd); - return 0; -} + snd_dmaengine_pcm_set_data(substream, dma_data);
-static int ep93xx_pcm_dma_submit(struct snd_pcm_substream *substream) -{ - struct snd_pcm_runtime *runtime = substream->runtime; - struct ep93xx_runtime_data *rtd = runtime->private_data; - struct dma_chan *chan = rtd->dma_chan; - struct dma_device *dma_dev = chan->device; - struct dma_async_tx_descriptor *desc; - - rtd->pointer_bytes = 0; - desc = dma_dev->device_prep_dma_cyclic(chan, runtime->dma_addr, - rtd->period_bytes * rtd->periods, - rtd->period_bytes, - rtd->dma_data.direction); - if (!desc) - return -EINVAL; - - desc->callback = ep93xx_pcm_dma_callback; - desc->callback_param = substream; - - dmaengine_submit(desc); return 0; }
-static void ep93xx_pcm_dma_flush(struct snd_pcm_substream *substream) +static int ep93xx_pcm_close(struct snd_pcm_substream *substream) { - struct snd_pcm_runtime *runtime = substream->runtime; - struct ep93xx_runtime_data *rtd = runtime->private_data; + struct dma_data *dma_data = snd_dmaengine_pcm_get_data(substream);
- dmaengine_terminate_all(rtd->dma_chan); + snd_dmaengine_pcm_close(substream); + kfree(dma_data); + return 0; }
static int ep93xx_pcm_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params) { - struct snd_pcm_runtime *runtime = substream->runtime; - struct ep93xx_runtime_data *rtd = runtime->private_data; - snd_pcm_set_runtime_buffer(substream, &substream->dma_buffer);
- rtd->periods = params_periods(params); - rtd->period_bytes = params_period_bytes(params); return 0; }
@@ -187,41 +118,6 @@ static int ep93xx_pcm_hw_free(struct snd_pcm_substream *substream) return 0; }
-static int ep93xx_pcm_trigger(struct snd_pcm_substream *substream, int cmd) -{ - int ret; - - ret = 0; - switch (cmd) { - case SNDRV_PCM_TRIGGER_START: - case SNDRV_PCM_TRIGGER_RESUME: - case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: - ret = ep93xx_pcm_dma_submit(substream); - break; - - case SNDRV_PCM_TRIGGER_STOP: - case SNDRV_PCM_TRIGGER_SUSPEND: - case SNDRV_PCM_TRIGGER_PAUSE_PUSH: - ep93xx_pcm_dma_flush(substream); - break; - - default: - ret = -EINVAL; - break; - } - - return ret; -} - -static snd_pcm_uframes_t ep93xx_pcm_pointer(struct snd_pcm_substream *substream) -{ - struct snd_pcm_runtime *runtime = substream->runtime; - struct ep93xx_runtime_data *rtd = substream->runtime->private_data; - - /* FIXME: implement this with sub-period granularity */ - return bytes_to_frames(runtime, rtd->pointer_bytes); -} - static int ep93xx_pcm_mmap(struct snd_pcm_substream *substream, struct vm_area_struct *vma) { @@ -239,8 +135,8 @@ static struct snd_pcm_ops ep93xx_pcm_ops = { .ioctl = snd_pcm_lib_ioctl, .hw_params = ep93xx_pcm_hw_params, .hw_free = ep93xx_pcm_hw_free, - .trigger = ep93xx_pcm_trigger, - .pointer = ep93xx_pcm_pointer, + .trigger = snd_dmaengine_pcm_trigger, + .pointer = snd_dmaengine_pcm_pointer, .mmap = ep93xx_pcm_mmap, };
On Wed, Feb 22, 2012 at 10:49:11AM +0100, Lars-Peter Clausen wrote:
Signed-off-by: Lars-Peter Clausen lars@metafoo.de
For some reason, this doesn't work on my ep93xx based Sim.One board. On playback with mpg123 when I press stop, it continues to play whatever was on the ring-buffer forever. Without the patches it works fine.
I'll try to find some time to debug this further.
On 02/27/2012 09:19 AM, Mika Westerberg wrote:
On Wed, Feb 22, 2012 at 10:49:11AM +0100, Lars-Peter Clausen wrote:
Signed-off-by: Lars-Peter Clausen lars@metafoo.de
For some reason, this doesn't work on my ep93xx based Sim.One board. On playback with mpg123 when I press stop, it continues to play whatever was on the ring-buffer forever. Without the patches it works fine.
I'll try to find some time to debug this further.
Hm, that’s interesting. The original ep93xx pcm driver was almost identical to what the common helper functions do. The only difference I can spot right now is, that it doesn't call dma_issue_pending after submitting the descriptor. Could you try to comment out the dma_issue_pending in soc-dmaengine-pcm.c and test whether it makes a difference?
Thanks, - Lars
On Mon, Feb 27, 2012 at 09:51:57AM +0100, Lars-Peter Clausen wrote:
On 02/27/2012 09:19 AM, Mika Westerberg wrote:
On Wed, Feb 22, 2012 at 10:49:11AM +0100, Lars-Peter Clausen wrote:
Signed-off-by: Lars-Peter Clausen lars@metafoo.de
For some reason, this doesn't work on my ep93xx based Sim.One board. On playback with mpg123 when I press stop, it continues to play whatever was on the ring-buffer forever. Without the patches it works fine.
I'll try to find some time to debug this further.
Hm, that’s interesting. The original ep93xx pcm driver was almost identical to what the common helper functions do. The only difference I can spot right now is, that it doesn't call dma_issue_pending after submitting the descriptor. Could you try to comment out the dma_issue_pending in soc-dmaengine-pcm.c and test whether it makes a difference?
I did try that but there was no effect, unfortunately.
However, I noticed that prtd->pos was never set to zero as it was in the original ep93xx-pcm driver. With following addition to your patch, playback works fine.
diff --git a/sound/soc/soc-dmaengine-pcm.c b/sound/soc/soc-dmaengine-pcm.c index 0526cf8..4420b70 100644 --- a/sound/soc/soc-dmaengine-pcm.c +++ b/sound/soc/soc-dmaengine-pcm.c @@ -142,6 +142,7 @@ static int dmaengine_pcm_prepare_and_submit(struct snd_pcm_substream *substream)
direction = snd_pcm_substream_to_dma_direction(substream);
+ prtd->pos = 0; desc = chan->device->device_prep_dma_cyclic(chan, substream->runtime->dma_addr, snd_pcm_lib_buffer_bytes(substream),
On 02/27/2012 08:01 PM, Mika Westerberg wrote:
On Mon, Feb 27, 2012 at 09:51:57AM +0100, Lars-Peter Clausen wrote:
On 02/27/2012 09:19 AM, Mika Westerberg wrote:
On Wed, Feb 22, 2012 at 10:49:11AM +0100, Lars-Peter Clausen wrote:
Signed-off-by: Lars-Peter Clausen lars@metafoo.de
For some reason, this doesn't work on my ep93xx based Sim.One board. On playback with mpg123 when I press stop, it continues to play whatever was on the ring-buffer forever. Without the patches it works fine.
I'll try to find some time to debug this further.
Hm, that’s interesting. The original ep93xx pcm driver was almost identical to what the common helper functions do. The only difference I can spot right now is, that it doesn't call dma_issue_pending after submitting the descriptor. Could you try to comment out the dma_issue_pending in soc-dmaengine-pcm.c and test whether it makes a difference?
I did try that but there was no effect, unfortunately.
However, I noticed that prtd->pos was never set to zero as it was in the original ep93xx-pcm driver. With following addition to your patch, playback works fine.
Ah, nice :)
Thanks, - Lars
diff --git a/sound/soc/soc-dmaengine-pcm.c b/sound/soc/soc-dmaengine-pcm.c index 0526cf8..4420b70 100644 --- a/sound/soc/soc-dmaengine-pcm.c +++ b/sound/soc/soc-dmaengine-pcm.c @@ -142,6 +142,7 @@ static int dmaengine_pcm_prepare_and_submit(struct snd_pcm_substream *substream)
direction = snd_pcm_substream_to_dma_direction(substream);
- prtd->pos = 0; desc = chan->device->device_prep_dma_cyclic(chan, substream->runtime->dma_addr, snd_pcm_lib_buffer_bytes(substream),
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Tue, Feb 28, 2012 at 09:47:50AM +0100, Lars-Peter Clausen wrote:
On 02/27/2012 08:01 PM, Mika Westerberg wrote:
However, I noticed that prtd->pos was never set to zero as it was in the original ep93xx-pcm driver. With following addition to your patch, playback works fine.
Ah, nice :)
Looking good. Can you please resend the series with this fix applied and all the acks you've collected?
On 02/28/2012 01:52 PM, Mark Brown wrote:
On Tue, Feb 28, 2012 at 09:47:50AM +0100, Lars-Peter Clausen wrote:
On 02/27/2012 08:01 PM, Mika Westerberg wrote:
However, I noticed that prtd->pos was never set to zero as it was in the original ep93xx-pcm driver. With following addition to your patch, playback works fine.
Ah, nice :)
Looking good. Can you please resend the series with this fix applied and all the acks you've collected?
I don't think there were any Acked-bys, just a Tested-by from Shawn Guo.
Mika, can I get a Tested-by or Acked-by for this patch from you?
Thanks, - Lars
On Tue, Feb 28, 2012 at 03:17:06PM +0100, Lars-Peter Clausen wrote:
On 02/28/2012 01:52 PM, Mark Brown wrote:
Looking good. Can you please resend the series with this fix applied and all the acks you've collected?
I don't think there were any Acked-bys, just a Tested-by from Shawn Guo.
I think Vinod acked it, ICBW though.
On Tue, Feb 28, 2012 at 03:17:06PM +0100, Lars-Peter Clausen wrote:
On 02/28/2012 01:52 PM, Mark Brown wrote:
On Tue, Feb 28, 2012 at 09:47:50AM +0100, Lars-Peter Clausen wrote:
On 02/27/2012 08:01 PM, Mika Westerberg wrote:
However, I noticed that prtd->pos was never set to zero as it was in the original ep93xx-pcm driver. With following addition to your patch, playback works fine.
Ah, nice :)
Looking good. Can you please resend the series with this fix applied and all the acks you've collected?
I don't think there were any Acked-bys, just a Tested-by from Shawn Guo.
Mika, can I get a Tested-by or Acked-by for this patch from you?
Sure,
Tested-by: Mika Westerberg mika.westerberg@iki.fi
participants (6)
-
Kuninori Morimoto -
Lars-Peter Clausen -
Mark Brown -
Mika Westerberg -
Russell King - ARM Linux -
Vinod Koul