| Message ID | 1394702320-21743-15-git-send-email-peter.ujfalusi@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/13/2014 10:18 AM, Peter Ujfalusi wrote: [...] > +static const struct snd_pcm_hardware edma_pcm_hardware = { > + .info = SNDRV_PCM_INFO_MMAP | > + SNDRV_PCM_INFO_MMAP_VALID | > + SNDRV_PCM_INFO_BATCH | > + SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME | > + SNDRV_PCM_INFO_INTERLEAVED, > + .buffer_bytes_max = 128 * 1024, > + .period_bytes_min = 32, > + .period_bytes_max = 64 * 1024, > + .periods_min = 2, > + .periods_max = 19, /* Limit by edma dmaengine driver */ > +}; The idea is that we can auto-discover all the things using the dma_slave_caps API. Too bad we removed the possibility to specify the maximum number of segments from the API. Maybe we need to add it back. Is the 19 a hard-limit or could it be worked around by software in the dmaengine driver? > + > +static const struct snd_dmaengine_pcm_config edma_dmaengine_pcm_config = { > + .pcm_hardware = &edma_pcm_hardware, > + .prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config, > + .prealloc_buffer_size = 128 * 1024, Unless there is a very good reason for exactly this size, just leave it 0 and let the generic dmaengine driver use the default. > +}; > + > +static const struct snd_dmaengine_pcm_config edma_compat_dmaengine_pcm_config = { > + .pcm_hardware = &edma_pcm_hardware, > + .prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config, > + .compat_filter_fn = edma_filter_fn, > + .prealloc_buffer_size = 128 * 1024, > +}; There is no need for different configs for DT and non-DT. > + > +int edma_pcm_platform_register(struct device *dev) > +{ > + if (dev->of_node) > + return snd_dmaengine_pcm_register(dev, > + &edma_dmaengine_pcm_config, > + SND_DMAENGINE_PCM_FLAG_NO_RESIDUE); Since the edma dmaengine driver implements the slave cap API there is no need to manually specify SND_DMAENGINE_PCM_FLAG_NO_RESIDUE manually. But since the edma driver sets the granularity to DMA_RESIDUE_GRANULARITY_DESCRIPTOR in this case the generic dmaengine will not set SND_DMAENGINE_PCM_FLAG_NO_RESIDUE automatically since it assumes that the dmaengine driver is capable of properly reporting the DMA position. > + else > + return snd_dmaengine_pcm_register(dev, > + &edma_compat_dmaengine_pcm_config, > + SND_DMAENGINE_PCM_FLAG_NO_RESIDUE | > + SND_DMAENGINE_PCM_FLAG_NO_DT | > + SND_DMAENGINE_PCM_FLAG_COMPAT); If you set the flags to just SND_DMAENGINE_PCM_FLAG_COMPAT it will do the right thing in the generic dmaengine driver depending on whether dev->of_node is set or not. There is also a devm_ version of snd_dmaengine_pcm_register() it probably makes sense to use it here.
On 03/13/2014 12:28 PM, Lars-Peter Clausen wrote: > On 03/13/2014 10:18 AM, Peter Ujfalusi wrote: [...] >> +static const struct snd_pcm_hardware edma_pcm_hardware = { + .info >> = SNDRV_PCM_INFO_MMAP | + SNDRV_PCM_INFO_MMAP_VALID | + >> SNDRV_PCM_INFO_BATCH | + SNDRV_PCM_INFO_PAUSE | >> SNDRV_PCM_INFO_RESUME | + SNDRV_PCM_INFO_INTERLEAVED, + >> .buffer_bytes_max = 128 * 1024, + .period_bytes_min = 32, + >> .period_bytes_max = 64 * 1024, + .periods_min = 2, + >> .periods_max = 19, /* Limit by edma dmaengine driver */ +}; > > The idea is that we can auto-discover all the things using the > dma_slave_caps API. Too bad we removed the possibility to specify the > maximum number of segments from the API. Maybe we need to add it back. Is > the 19 a hard-limit or could it be worked around by software in the > dmaengine driver? The edma dmaengine driver will refuse to configure more than 20 paRAM slots (1 for the channel + 19 for the periods). Depending on the SoC we might have different number of paRAM entries available. The intention with the limit was to prevent cases when we run out of paRAM entires for other devices because audio took most of them. >> + +static const struct snd_dmaengine_pcm_config edma_dmaengine_pcm_config >> = { + .pcm_hardware = &edma_pcm_hardware, + .prepare_slave_config = >> snd_dmaengine_pcm_prepare_slave_config, + .prealloc_buffer_size = 128 >> * 1024, > > Unless there is a very good reason for exactly this size, just leave it 0 > and let the generic dmaengine driver use the default. I'd rather keep this here. Since I have the .pcm_hardware the core will ignore the snd_pcm_hardware.buffer_bytes_max when preallocating and it is in fact going to go for: prealloc_buffer_size = 512 * 1024; max_buffer_size = SIZE_MAX; While I have 128 * 1024 for the snd_pcm_hardware.buffer_bytes_max. I think as long as I have the .pcm_hardware provided from the edma-pcm driver I will have the .prealloc_buffer_size as well. >> +}; + +static const struct snd_dmaengine_pcm_config >> edma_compat_dmaengine_pcm_config = { + .pcm_hardware = >> &edma_pcm_hardware, + .prepare_slave_config = >> snd_dmaengine_pcm_prepare_slave_config, + .compat_filter_fn = >> edma_filter_fn, + .prealloc_buffer_size = 128 * 1024, +}; > > There is no need for different configs for DT and non-DT. > >> + +int edma_pcm_platform_register(struct device *dev) +{ + if >> (dev->of_node) + return snd_dmaengine_pcm_register(dev, + >> &edma_dmaengine_pcm_config, + >> SND_DMAENGINE_PCM_FLAG_NO_RESIDUE); > > Since the edma dmaengine driver implements the slave cap API there is no > need to manually specify SND_DMAENGINE_PCM_FLAG_NO_RESIDUE manually. But > since the edma driver sets the granularity to > DMA_RESIDUE_GRANULARITY_DESCRIPTOR in this case the generic dmaengine will > not set SND_DMAENGINE_PCM_FLAG_NO_RESIDUE automatically since it assumes > that the dmaengine driver is capable of properly reporting the DMA > position. > >> + else + return snd_dmaengine_pcm_register(dev, + >> &edma_compat_dmaengine_pcm_config, + >> SND_DMAENGINE_PCM_FLAG_NO_RESIDUE | + >> SND_DMAENGINE_PCM_FLAG_NO_DT | + >> SND_DMAENGINE_PCM_FLAG_COMPAT); > > > If you set the flags to just SND_DMAENGINE_PCM_FLAG_COMPAT it will do the > right thing in the generic dmaengine driver depending on whether > dev->of_node is set or not. I have missed these in the core. Makes the code simpler for sure. > There is also a devm_ version of snd_dmaengine_pcm_register() it probably > makes sense to use it here. I forgot about the devm_ version. True, I'll use that instead.
On 03/13/2014 12:56 PM, Peter Ujfalusi wrote: > On 03/13/2014 12:28 PM, Lars-Peter Clausen wrote: >> On 03/13/2014 10:18 AM, Peter Ujfalusi wrote: [...] >>> +static const struct snd_pcm_hardware edma_pcm_hardware = { + .info >>> = SNDRV_PCM_INFO_MMAP | + SNDRV_PCM_INFO_MMAP_VALID | + >>> SNDRV_PCM_INFO_BATCH | + SNDRV_PCM_INFO_PAUSE | >>> SNDRV_PCM_INFO_RESUME | + SNDRV_PCM_INFO_INTERLEAVED, + >>> .buffer_bytes_max = 128 * 1024, + .period_bytes_min = 32, + >>> .period_bytes_max = 64 * 1024, + .periods_min = 2, + >>> .periods_max = 19, /* Limit by edma dmaengine driver */ +}; >> >> The idea is that we can auto-discover all the things using the >> dma_slave_caps API. Too bad we removed the possibility to specify the >> maximum number of segments from the API. Maybe we need to add it back. Is >> the 19 a hard-limit or could it be worked around by software in the >> dmaengine driver? > > The edma dmaengine driver will refuse to configure more than 20 paRAM slots (1 > for the channel + 19 for the periods). Depending on the SoC we might have > different number of paRAM entries available. The intention with the limit was > to prevent cases when we run out of paRAM entires for other devices because > audio took most of them. The reason why we removed the max_segments field from the slave_caps struct was because we though it should be possible to, even when the hardware only supports a fixed amount of segments, emulate support for more in software. If this is not the case with the edma, we need to bring this field back.
On 03/13/2014 12:28 PM, Lars-Peter Clausen wrote: >> +int edma_pcm_platform_register(struct device *dev) >> +{ >> + if (dev->of_node) >> + return snd_dmaengine_pcm_register(dev, >> + &edma_dmaengine_pcm_config, >> + SND_DMAENGINE_PCM_FLAG_NO_RESIDUE); > > Since the edma dmaengine driver implements the slave cap API there is no need > to manually specify SND_DMAENGINE_PCM_FLAG_NO_RESIDUE manually. But since the > edma driver sets the granularity to DMA_RESIDUE_GRANULARITY_DESCRIPTOR in this > case the generic dmaengine will not set SND_DMAENGINE_PCM_FLAG_NO_RESIDUE > automatically since it assumes that the dmaengine driver is capable of > properly reporting the DMA position. Hrm, I see. For eDMA I think we can support DMA_RESIDUE_GRANULARITY_SEGMENT granularity. Since according to the documentation the _SEGMENT means that the DMA position will be updated per periods, which is basically the same thing what we are doing at the moment when the granularity is DMA_RESIDUE_GRANULARITY_DESCRIPTOR. >From ALSA point of view at least they are the same: neither of them can report exact position, the DMA pointer jumps from period to period. IMHO in the generic dmaengine PCM we should set the SNDRV_PCM_INFO_BATCH for both cases.
On 03/13/2014 02:03 PM, Peter Ujfalusi wrote: > On 03/13/2014 12:28 PM, Lars-Peter Clausen wrote: >>> +int edma_pcm_platform_register(struct device *dev) >>> +{ >>> + if (dev->of_node) >>> + return snd_dmaengine_pcm_register(dev, >>> + &edma_dmaengine_pcm_config, >>> + SND_DMAENGINE_PCM_FLAG_NO_RESIDUE); >> >> Since the edma dmaengine driver implements the slave cap API there is no need >> to manually specify SND_DMAENGINE_PCM_FLAG_NO_RESIDUE manually. But since the >> edma driver sets the granularity to DMA_RESIDUE_GRANULARITY_DESCRIPTOR in this >> case the generic dmaengine will not set SND_DMAENGINE_PCM_FLAG_NO_RESIDUE >> automatically since it assumes that the dmaengine driver is capable of >> properly reporting the DMA position. > > Hrm, I see. For eDMA I think we can support DMA_RESIDUE_GRANULARITY_SEGMENT > granularity. Since according to the documentation the _SEGMENT means that the > DMA position will be updated per periods, which is basically the same thing > what we are doing at the moment when the granularity is > DMA_RESIDUE_GRANULARITY_DESCRIPTOR. > From ALSA point of view at least they are the same: neither of them can report > exact position, the DMA pointer jumps from period to period. > > IMHO in the generic dmaengine PCM we should set the SNDRV_PCM_INFO_BATCH for > both cases. > Ups, sorry mixed up DMA_RESIDUE_GRANULARITY_SEGMENT and DMA_RESIDUE_GRANULARITY_DESCRIPTOR. You can just remove the SND_DMAENGINE_PCM_FLAG_NO_RESIDUE when registering the dmaengine PCM driver and everything will still work as expected.
diff --git a/sound/soc/davinci/Kconfig b/sound/soc/davinci/Kconfig index a8ec1fc3e4d0..f9d9f748e743 100644 --- a/sound/soc/davinci/Kconfig +++ b/sound/soc/davinci/Kconfig @@ -1,6 +1,7 @@ config SND_DAVINCI_SOC tristate "SoC Audio for TI DAVINCI or AM33XX/AM43XX chips" depends on ARCH_DAVINCI || SOC_AM33XX || SOC_AM43XX + select SND_SOC_GENERIC_DMAENGINE_PCM config SND_DAVINCI_SOC_I2S tristate diff --git a/sound/soc/davinci/Makefile b/sound/soc/davinci/Makefile index 744d4d9a0184..97d05a2f1723 100644 --- a/sound/soc/davinci/Makefile +++ b/sound/soc/davinci/Makefile @@ -1,5 +1,5 @@ # DAVINCI Platform Support -snd-soc-davinci-objs := davinci-pcm.o +snd-soc-davinci-objs := davinci-pcm.o edma-pcm.o snd-soc-davinci-i2s-objs := davinci-i2s.o snd-soc-davinci-mcasp-objs:= davinci-mcasp.o snd-soc-davinci-vcif-objs:= davinci-vcif.o diff --git a/sound/soc/davinci/edma-pcm.c b/sound/soc/davinci/edma-pcm.c new file mode 100644 index 000000000000..eeed927a2657 --- /dev/null +++ b/sound/soc/davinci/edma-pcm.c @@ -0,0 +1,77 @@ +/* + * edma-pcm.c - eDMA PCM driver using dmaengine for AM3xxx, AM4xxx + * + * Copyright (C) 2014 Texas Instruments, Inc. + * + * Author: Peter Ujfalusi <peter.ujfalusi@ti.com> + * + * Based on: sound/soc/tegra/tegra_pcm.c + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + */ + +#include <linux/module.h> +#include <sound/core.h> +#include <sound/pcm.h> +#include <sound/pcm_params.h> +#include <sound/soc.h> +#include <sound/dmaengine_pcm.h> +#include <linux/edma.h> + +static const struct snd_pcm_hardware edma_pcm_hardware = { + .info = SNDRV_PCM_INFO_MMAP | + SNDRV_PCM_INFO_MMAP_VALID | + SNDRV_PCM_INFO_BATCH | + SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME | + SNDRV_PCM_INFO_INTERLEAVED, + .buffer_bytes_max = 128 * 1024, + .period_bytes_min = 32, + .period_bytes_max = 64 * 1024, + .periods_min = 2, + .periods_max = 19, /* Limit by edma dmaengine driver */ +}; + +static const struct snd_dmaengine_pcm_config edma_dmaengine_pcm_config = { + .pcm_hardware = &edma_pcm_hardware, + .prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config, + .prealloc_buffer_size = 128 * 1024, +}; + +static const struct snd_dmaengine_pcm_config edma_compat_dmaengine_pcm_config = { + .pcm_hardware = &edma_pcm_hardware, + .prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config, + .compat_filter_fn = edma_filter_fn, + .prealloc_buffer_size = 128 * 1024, +}; + +int edma_pcm_platform_register(struct device *dev) +{ + if (dev->of_node) + return snd_dmaengine_pcm_register(dev, + &edma_dmaengine_pcm_config, + SND_DMAENGINE_PCM_FLAG_NO_RESIDUE); + else + return snd_dmaengine_pcm_register(dev, + &edma_compat_dmaengine_pcm_config, + SND_DMAENGINE_PCM_FLAG_NO_RESIDUE | + SND_DMAENGINE_PCM_FLAG_NO_DT | + SND_DMAENGINE_PCM_FLAG_COMPAT); +} +EXPORT_SYMBOL_GPL(edma_pcm_platform_register); + +void edma_pcm_platform_unregister(struct device *dev) +{ + snd_dmaengine_pcm_unregister(dev); +} +EXPORT_SYMBOL_GPL(edma_pcm_platform_unregister); + +MODULE_AUTHOR("Peter Ujfalusi <peter.ujfalusi@ti.com>"); +MODULE_DESCRIPTION("eDMA PCM ASoC platform driver"); +MODULE_LICENSE("GPL"); diff --git a/sound/soc/davinci/edma-pcm.h b/sound/soc/davinci/edma-pcm.h new file mode 100644 index 000000000000..25dc8c7ff093 --- /dev/null +++ b/sound/soc/davinci/edma-pcm.h @@ -0,0 +1,26 @@ +/* + * edma-pcm.h - eDMA PCM driver using dmaengine for AM3xxx, AM4xxx + * + * Copyright (C) 2014 Texas Instruments, Inc. + * + * Author: Peter Ujfalusi <peter.ujfalusi@ti.com> + * + * Based on: sound/soc/tegra/tegra_pcm.h + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + */ + +#ifndef __EDMA_PCM_H__ +#define __EDMA_PCM_H__ + +int edma_pcm_platform_register(struct device *dev); +void edma_pcm_platform_unregister(struct device *dev); + +#endif /* __EDMA_PCM_H__ */
Platform driver glue for SoC using eDMA3 to use dmaengine PCM. The maximum number of periods need to be limited to 19 since the edma dmaengine driver limits the paRAM slot use for audio at in cyclic mode. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> --- sound/soc/davinci/Kconfig | 1 + sound/soc/davinci/Makefile | 2 +- sound/soc/davinci/edma-pcm.c | 77 ++++++++++++++++++++++++++++++++++++++++++++ sound/soc/davinci/edma-pcm.h | 26 +++++++++++++++ 4 files changed, 105 insertions(+), 1 deletion(-) create mode 100644 sound/soc/davinci/edma-pcm.c create mode 100644 sound/soc/davinci/edma-pcm.h