[alsa-devel] [PATCH V5 3/3] ASoC: AMD: add AMD ASoC ACP-I2S driver
From: Maruthi Srinivas Bayyavarapu Maruthi.Bayyavarapu@amd.com
ACP IP block consists of dedicated DMA and I2S blocks. The PCM driver provides the DMA and CPU DAI components to ALSA core.
Signed-off-by: Maruthi Bayyavarapu maruthi.bayyavarapu@amd.com Reviewed-by: Alex Deucher alexander.deucher@amd.com Reviewed-by: Murali Krishna Vemuri murali-krishna.vemuri@amd.com Signed-off-by: Alex Deucher alexander.deucher@amd.com ---
v2: squash in Kconfig fix v3: squash additional commits, convert to mfd, drop rt286 changes v4: squash in naming fixes v5: move patch versioning out of commit message
The module licensing follows the model used in the drm drivers.
sound/soc/Kconfig | 1 + sound/soc/Makefile | 1 + sound/soc/amd/Kconfig | 5 + sound/soc/amd/Makefile | 3 + sound/soc/amd/acp-pcm-dma.c | 676 ++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 686 insertions(+) create mode 100644 sound/soc/amd/Kconfig create mode 100644 sound/soc/amd/Makefile create mode 100644 sound/soc/amd/acp-pcm-dma.c
diff --git a/sound/soc/Kconfig b/sound/soc/Kconfig index 2ae9619..1b9ce31 100644 --- a/sound/soc/Kconfig +++ b/sound/soc/Kconfig @@ -32,6 +32,7 @@ config SND_SOC_GENERIC_DMAENGINE_PCM
# All the supported SoCs source "sound/soc/adi/Kconfig" +source "sound/soc/amd/Kconfig" source "sound/soc/atmel/Kconfig" source "sound/soc/au1x/Kconfig" source "sound/soc/bcm/Kconfig" diff --git a/sound/soc/Makefile b/sound/soc/Makefile index e189903..a6cbb99 100644 --- a/sound/soc/Makefile +++ b/sound/soc/Makefile @@ -14,6 +14,7 @@ obj-$(CONFIG_SND_SOC) += snd-soc-core.o obj-$(CONFIG_SND_SOC) += codecs/ obj-$(CONFIG_SND_SOC) += generic/ obj-$(CONFIG_SND_SOC) += adi/ +obj-$(CONFIG_SND_SOC) += amd/ obj-$(CONFIG_SND_SOC) += atmel/ obj-$(CONFIG_SND_SOC) += au1x/ obj-$(CONFIG_SND_SOC) += bcm/ diff --git a/sound/soc/amd/Kconfig b/sound/soc/amd/Kconfig new file mode 100644 index 0000000..ce39979 --- /dev/null +++ b/sound/soc/amd/Kconfig @@ -0,0 +1,5 @@ +config SND_SOC_AMD_ACP + tristate "AMD Audio Coprocessor support" + depends on MFD_CORE + help + This option enables ACP support (DMA,I2S) on AMD platforms. diff --git a/sound/soc/amd/Makefile b/sound/soc/amd/Makefile new file mode 100644 index 0000000..1a66ec0 --- /dev/null +++ b/sound/soc/amd/Makefile @@ -0,0 +1,3 @@ +snd-soc-acp-pcm-objs := acp-pcm-dma.o + +obj-$(CONFIG_SND_SOC_AMD_ACP) += snd-soc-acp-pcm.o diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c new file mode 100644 index 0000000..18cc60c --- /dev/null +++ b/sound/soc/amd/acp-pcm-dma.c @@ -0,0 +1,676 @@ +/* + * AMD ALSA SoC PCM Driver + * + * Copyright 2014-2015 Advanced Micro Devices, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR + * OTHER DEALINGS IN THE SOFTWARE. + * + * + */ + +#include <linux/interrupt.h> +#include <linux/platform_device.h> +#include <linux/dma-mapping.h> +#include <linux/slab.h> +#include <linux/module.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/pci.h> +#include <linux/pm_runtime.h> +#include <linux/mfd/amd_acp.h> + +#include <sound/pcm.h> +#include <sound/pcm_params.h> +#include <sound/soc.h> + +#define PLAYBACK_MIN_NUM_PERIODS 2 +#define PLAYBACK_MAX_NUM_PERIODS 2 +#define PLAYBACK_MAX_PERIOD_SIZE 16384 +#define PLAYBACK_MIN_PERIOD_SIZE 1024 +#define CAPTURE_MIN_NUM_PERIODS 2 +#define CAPTURE_MAX_NUM_PERIODS 2 +#define CAPTURE_MAX_PERIOD_SIZE 16384 +#define CAPTURE_MIN_PERIOD_SIZE 1024 + +#define NUM_DSCRS_PER_CHANNEL 2 + +#define MAX_BUFFER (PLAYBACK_MAX_PERIOD_SIZE * PLAYBACK_MAX_NUM_PERIODS) +#define MIN_BUFFER MAX_BUFFER + +#define TWO_CHANNEL_SUPPORT 2 /* up to 2.0 */ +#define FOUR_CHANNEL_SUPPORT 4 /* up to 3.1 */ +#define SIX_CHANNEL_SUPPORT 6 /* up to 5.1 */ +#define EIGHT_CHANNEL_SUPPORT 8 /* up to 7.1 */ + + +static const struct snd_pcm_hardware acp_pcm_hardware_playback = { + .info = SNDRV_PCM_INFO_INTERLEAVED | + SNDRV_PCM_INFO_BLOCK_TRANSFER | SNDRV_PCM_INFO_MMAP | + SNDRV_PCM_INFO_MMAP_VALID | SNDRV_PCM_INFO_BATCH | + SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME, + /* formats,rates,channels based on i2s doc. */ + .formats = SNDRV_PCM_FMTBIT_S16_LE | + SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE, + .channels_min = 1, + .channels_max = 8, + .rates = SNDRV_PCM_RATE_8000_96000, + .rate_min = 8000, + .rate_max = 96000, + .buffer_bytes_max = PLAYBACK_MAX_NUM_PERIODS * PLAYBACK_MAX_PERIOD_SIZE, + .period_bytes_min = PLAYBACK_MIN_PERIOD_SIZE, + .period_bytes_max = PLAYBACK_MAX_PERIOD_SIZE, + .periods_min = PLAYBACK_MIN_NUM_PERIODS, + .periods_max = PLAYBACK_MAX_NUM_PERIODS, +}; + +static const struct snd_pcm_hardware acp_pcm_hardware_capture = { + .info = SNDRV_PCM_INFO_INTERLEAVED | + SNDRV_PCM_INFO_BLOCK_TRANSFER | SNDRV_PCM_INFO_MMAP | + SNDRV_PCM_INFO_MMAP_VALID | SNDRV_PCM_INFO_BATCH | + SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME, + /* formats,rates,channels based on i2s doc. */ + .formats = SNDRV_PCM_FMTBIT_S16_LE | + SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE, + .channels_min = 1, + .channels_max = 2, + .rates = SNDRV_PCM_RATE_8000_48000, + .rate_min = 8000, + .rate_max = 48000, + .buffer_bytes_max = CAPTURE_MAX_NUM_PERIODS * CAPTURE_MAX_PERIOD_SIZE, + .period_bytes_min = CAPTURE_MIN_PERIOD_SIZE, + .period_bytes_max = CAPTURE_MAX_PERIOD_SIZE, + .periods_min = CAPTURE_MIN_NUM_PERIODS, + .periods_max = CAPTURE_MAX_NUM_PERIODS, +}; + +struct audio_drv_data { + struct snd_pcm_substream *play_stream; + struct snd_pcm_substream *capture_stream; + struct amd_acp_device *acp_dev; + struct acp_irq_prv *iprv; +}; + +static const struct snd_soc_component_driver dw_i2s_component = { + .name = "dw-i2s", +}; + +static void acp_pcm_period_elapsed(struct device *dev, u16 play_intr, + u16 capture_intr) +{ + struct snd_pcm_substream *substream; + struct audio_drv_data *irq_data = dev_get_drvdata(dev); + + /* Inform ALSA about the period elapsed (one out of two periods) */ + if (play_intr) + substream = irq_data->play_stream; + else + substream = irq_data->capture_stream; + + if (substream->runtime && snd_pcm_running(substream)) + snd_pcm_period_elapsed(substream); +} + +static int acp_dma_open(struct snd_pcm_substream *substream) +{ + int ret = 0; + struct snd_pcm_runtime *runtime = substream->runtime; + struct snd_soc_pcm_runtime *prtd = substream->private_data; + struct audio_drv_data *intr_data = dev_get_drvdata(prtd->platform->dev); + + struct audio_substream_data *adata = + kzalloc(sizeof(struct audio_substream_data), GFP_KERNEL); + if (adata == NULL) + return -ENOMEM; + + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) + runtime->hw = acp_pcm_hardware_playback; + else + runtime->hw = acp_pcm_hardware_capture; + + ret = snd_pcm_hw_constraint_integer(runtime, + SNDRV_PCM_HW_PARAM_PERIODS); + if (ret < 0) { + dev_err(prtd->platform->dev, "set integer constraint failed\n"); + return ret; + } + + adata->acp_dev = intr_data->acp_dev; + runtime->private_data = adata; + + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) + intr_data->play_stream = substream; + else + intr_data->capture_stream = substream; + + return 0; +} + +static int acp_dma_hw_params(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params) +{ + int status; + uint64_t size; + struct snd_dma_buffer *dma_buffer; + struct page *pg; + u16 num_of_pages; + struct snd_pcm_runtime *runtime; + struct audio_substream_data *rtd; + struct amd_acp_device *acp_dev; + + dma_buffer = &substream->dma_buffer; + + runtime = substream->runtime; + rtd = runtime->private_data; + + if (WARN_ON(!rtd)) + return -EINVAL; + acp_dev = rtd->acp_dev; + + size = params_buffer_bytes(params); + status = snd_pcm_lib_malloc_pages(substream, size); + if (status < 0) + return status; + + memset(substream->runtime->dma_area, 0, params_buffer_bytes(params)); + pg = virt_to_page(substream->dma_buffer.area); + + if (NULL != pg) { + /* Save for runtime private data */ + rtd->pg = pg; + rtd->order = get_order(size); + + /* Let ACP know the Allocated memory */ + num_of_pages = PAGE_ALIGN(size) >> PAGE_SHIFT; + + /* Fill the page table entries in ACP SRAM */ + rtd->pg = pg; + rtd->size = size; + rtd->num_of_pages = num_of_pages; + rtd->direction = substream->stream; + + acp_dev->config_dma(acp_dev, rtd); + status = 0; + } else { + status = -ENOMEM; + } + return status; +} + +static int acp_dma_hw_free(struct snd_pcm_substream *substream) +{ + return snd_pcm_lib_free_pages(substream); +} + +static snd_pcm_uframes_t acp_dma_pointer(struct snd_pcm_substream *substream) +{ + u32 pos = 0; + struct snd_pcm_runtime *runtime = substream->runtime; + struct audio_substream_data *rtd = runtime->private_data; + struct amd_acp_device *acp_dev = rtd->acp_dev; + + pos = acp_dev->update_dma_pointer(acp_dev, substream->stream, + frames_to_bytes(runtime, runtime->period_size)); + return bytes_to_frames(runtime, pos); + +} + +static int acp_dma_mmap(struct snd_pcm_substream *substream, + struct vm_area_struct *vma) +{ + return snd_pcm_lib_default_mmap(substream, vma); +} + +static int acp_dma_prepare(struct snd_pcm_substream *substream) +{ + int ret; + struct snd_pcm_runtime *runtime = substream->runtime; + struct audio_substream_data *rtd = runtime->private_data; + struct amd_acp_device *acp_dev = rtd->acp_dev; + + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { + + acp_dev->config_dma_channel(acp_dev, SYSRAM_TO_ACP_CH_NUM, + PLAYBACK_START_DMA_DESCR_CH12, + NUM_DSCRS_PER_CHANNEL, 0); + acp_dev->config_dma_channel(acp_dev, ACP_TO_I2S_DMA_CH_NUM, + PLAYBACK_START_DMA_DESCR_CH13, + NUM_DSCRS_PER_CHANNEL, 0); + /* Fill ACP SRAM (2 periods) with zeros from System RAM + * which is zero-ed in hw_params */ + ret = acp_dev->dma_start(rtd->acp_dev, + SYSRAM_TO_ACP_CH_NUM, false); + if (ret < 0) + ret = -EFAULT; + + /* Now configure DMA to transfer only first half of System RAM + * buffer before playback is triggered. This will overwrite + * zero-ed second half of SRAM buffer */ + acp_dev->config_dma_channel(acp_dev, SYSRAM_TO_ACP_CH_NUM, + PLAYBACK_START_DMA_DESCR_CH12, + 1, 0); + } else { + acp_dev->config_dma_channel(acp_dev, ACP_TO_SYSRAM_CH_NUM, + CAPTURE_START_DMA_DESCR_CH14, + NUM_DSCRS_PER_CHANNEL, 0); + acp_dev->config_dma_channel(acp_dev, I2S_TO_ACP_DMA_CH_NUM, + CAPTURE_START_DMA_DESCR_CH15, + NUM_DSCRS_PER_CHANNEL, 0); + } + return 0; +} + +static int acp_dma_trigger(struct snd_pcm_substream *substream, int cmd) +{ + struct snd_pcm_runtime *runtime = substream->runtime; + struct audio_substream_data *rtd = runtime->private_data; + struct amd_acp_device *acp_dev = rtd->acp_dev; + int ret; + + if (!rtd) + return -EINVAL; + switch (cmd) { + case SNDRV_PCM_TRIGGER_START: + case SNDRV_PCM_TRIGGER_RESUME: + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { + ret = acp_dev->dma_start(rtd->acp_dev, + SYSRAM_TO_ACP_CH_NUM, false); + if (ret < 0) + return -EFAULT; + acp_dev->prebuffer_audio(rtd->acp_dev); + + ret = acp_dev->dma_start(acp_dev, + ACP_TO_I2S_DMA_CH_NUM, true); + } else { + ret = acp_dev->dma_start(acp_dev, + I2S_TO_ACP_DMA_CH_NUM, true); + } + break; + case SNDRV_PCM_TRIGGER_STOP: + case SNDRV_PCM_TRIGGER_SUSPEND: + case SNDRV_PCM_TRIGGER_PAUSE_PUSH: + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { + ret = acp_dev->dma_stop(acp_dev, SYSRAM_TO_ACP_CH_NUM); + if (0 == ret) + ret = acp_dev->dma_stop(acp_dev, + ACP_TO_I2S_DMA_CH_NUM); + } else { + ret = acp_dev->dma_stop(acp_dev, I2S_TO_ACP_DMA_CH_NUM); + if (0 == ret) + ret = acp_dev->dma_stop(acp_dev, + ACP_TO_SYSRAM_CH_NUM); + } + break; + default: + ret = -EINVAL; + + } + return ret; +} + +static int acp_dma_new(struct snd_soc_pcm_runtime *rtd) +{ + int ret; + struct snd_pcm *pcm; + + pcm = rtd->pcm; + ret = snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_DEV, + NULL, MIN_BUFFER, MAX_BUFFER); + return ret; +} + +static int acp_dma_close(struct snd_pcm_substream *substream) +{ + struct snd_pcm_runtime *runtime = substream->runtime; + struct audio_substream_data *rtd = runtime->private_data; + struct snd_soc_pcm_runtime *prtd = substream->private_data; + + kfree(rtd); + + pm_runtime_mark_last_busy(prtd->platform->dev); + return 0; +} + +static int acp_dai_i2s_hwparams(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params, + struct snd_soc_dai *dai) +{ + struct snd_pcm_runtime *runtime = substream->runtime; + struct snd_soc_pcm_runtime *prtd = substream->private_data; + struct audio_substream_data *rtd = runtime->private_data; + struct amd_acp_device *acp_dev = rtd->acp_dev; + struct device *dev = prtd->platform->dev; + u32 chan_nr; + + switch (params_format(params)) { + case SNDRV_PCM_FORMAT_S16_LE: + rtd->xfer_resolution = 0x02; + break; + + case SNDRV_PCM_FORMAT_S24_LE: + rtd->xfer_resolution = 0x04; + break; + + case SNDRV_PCM_FORMAT_S32_LE: + rtd->xfer_resolution = 0x05; + break; + + default: + dev_err(dev, "unsuppted PCM fmt : %d\n", params_format(params)); + return -EINVAL; + } + + chan_nr = params_channels(params); + + switch (chan_nr) { + case EIGHT_CHANNEL_SUPPORT: + rtd->ch_reg = 3; + break; + case SIX_CHANNEL_SUPPORT: + rtd->ch_reg = 2; + break; + case FOUR_CHANNEL_SUPPORT: + rtd->ch_reg = 1; + break; + case TWO_CHANNEL_SUPPORT: + rtd->ch_reg = 0; + break; + default: + dev_err(dev, "channel not supported : %d\n", chan_nr); + return -EINVAL; + } + + rtd->direction = substream->stream; + + acp_dev->config_i2s(acp_dev, rtd); + + return 0; +} + +static int acp_dai_i2s_trigger(struct snd_pcm_substream *substream, + int cmd, struct snd_soc_dai *dai) +{ + struct snd_pcm_runtime *runtime = substream->runtime; + struct audio_substream_data *rtd = runtime->private_data; + struct amd_acp_device *acp_dev = rtd->acp_dev; + int ret; + + switch (cmd) { + case SNDRV_PCM_TRIGGER_START: + case SNDRV_PCM_TRIGGER_RESUME: + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: + acp_dev->i2s_start(acp_dev, substream->stream); + ret = 0; + break; + + case SNDRV_PCM_TRIGGER_STOP: + case SNDRV_PCM_TRIGGER_SUSPEND: + case SNDRV_PCM_TRIGGER_PAUSE_PUSH: + acp_dev->i2s_stop(acp_dev, substream->stream); + ret = 0; + break; + default: + ret = -EINVAL; + break; + } + + return ret; +} + +static int acp_dai_i2s_prepare(struct snd_pcm_substream *substream, + struct snd_soc_dai *dai) +{ + struct snd_pcm_runtime *runtime = substream->runtime; + struct audio_substream_data *rtd = runtime->private_data; + struct amd_acp_device *acp_dev = rtd->acp_dev; + + acp_dev->i2s_reset(acp_dev, substream->stream); + return 0; +} + +static struct snd_pcm_ops acp_dma_ops = { + .open = acp_dma_open, + .close = acp_dma_close, + .ioctl = snd_pcm_lib_ioctl, + .hw_params = acp_dma_hw_params, + .hw_free = acp_dma_hw_free, + .trigger = acp_dma_trigger, + .pointer = acp_dma_pointer, + .mmap = acp_dma_mmap, + .prepare = acp_dma_prepare, +}; + +static struct snd_soc_dai_ops acp_dai_i2s_ops = { + .prepare = acp_dai_i2s_prepare, + .hw_params = acp_dai_i2s_hwparams, + .trigger = acp_dai_i2s_trigger, +}; + +static struct snd_soc_dai_driver i2s_dai_driver = { + .playback = { + .stream_name = "I2S Playback", + .channels_min = 2, + .channels_max = 2, + .rates = SNDRV_PCM_RATE_8000_96000, + .formats = SNDRV_PCM_FMTBIT_S24_LE | + SNDRV_PCM_FMTBIT_S32_LE, + + .rate_min = 8000, + .rate_max = 96000, + }, + .capture = { + .stream_name = "I2S Capture", + .channels_min = 2, + .channels_max = 2, + .rates = SNDRV_PCM_RATE_8000_48000, + .formats = SNDRV_PCM_FMTBIT_S24_LE | + SNDRV_PCM_FMTBIT_S32_LE, + .rate_min = 8000, + .rate_max = 48000, + }, + .ops = &acp_dai_i2s_ops, +}; + +static struct snd_soc_platform_driver acp_asoc_platform = { + .ops = &acp_dma_ops, + .pcm_new = acp_dma_new, +}; + +static int acp_alsa_register(struct device *dev, struct amd_acp_device *acp_dev, + struct platform_device *pdev) +{ + int status; + + status = snd_soc_register_platform(dev, &acp_asoc_platform); + if (0 != status) { + dev_err(dev, "Unable to register ALSA platform device\n"); + goto exit_platform; + } else { + status = snd_soc_register_component(dev, + &dw_i2s_component, + &i2s_dai_driver, 1); + + if (0 != status) { + dev_err(dev, "Unable to register i2s dai\n"); + goto exit_dai; + } else { + dev_info(dev, "ACP device registered with ALSA\n"); + return status; + } + } + +exit_dai: + snd_soc_unregister_platform(dev); +exit_platform: + acp_dev->fini(acp_dev); + return status; +} + +static int acp_audio_probe(struct platform_device *pdev) +{ + int status; + struct audio_drv_data *audio_drv_data; + struct amd_acp_device *acp_dev = dev_get_platdata(&pdev->dev); + + audio_drv_data = devm_kzalloc(&pdev->dev, sizeof(struct audio_drv_data), + GFP_KERNEL); + if (audio_drv_data == NULL) + return -ENOMEM; + + audio_drv_data->iprv = devm_kzalloc(&pdev->dev, + sizeof(struct acp_irq_prv), + GFP_KERNEL); + if (audio_drv_data->iprv == NULL) + return -ENOMEM; + + /* The following members gets populated in device 'open' + * function. Till then interrupts are disabled in 'acp_hw_init' + * and device doesn't generate any interrupts. + */ + + audio_drv_data->play_stream = NULL; + audio_drv_data->capture_stream = NULL; + audio_drv_data->acp_dev = acp_dev; + + audio_drv_data->iprv->dev = &pdev->dev; + audio_drv_data->iprv->acp_dev = acp_dev; + audio_drv_data->iprv->set_elapsed = acp_pcm_period_elapsed; + + dev_set_drvdata(&pdev->dev, audio_drv_data); + + /* Initialize the ACP */ + status = acp_dev->init(acp_dev, audio_drv_data->iprv); + + if (0 == status) + status = acp_alsa_register(&pdev->dev, acp_dev, pdev); + else + dev_err(&pdev->dev, "ACP initialization Failed\n"); + + pm_runtime_set_autosuspend_delay(&pdev->dev, 10000); + pm_runtime_use_autosuspend(&pdev->dev); + pm_runtime_enable(&pdev->dev); + + return status; +} + +static int acp_audio_remove(struct platform_device *pdev) +{ + struct amd_acp_device *acp_dev = dev_get_platdata(&pdev->dev); + + snd_soc_unregister_component(&pdev->dev); + snd_soc_unregister_platform(&pdev->dev); + + acp_dev->fini(acp_dev); + pm_runtime_disable(&pdev->dev); + + return 0; +} + +static int acp_pcm_suspend(struct device *dev) +{ + bool pm_rts; + struct audio_drv_data *adata = dev_get_drvdata(dev); + + pm_rts = pm_runtime_status_suspended(dev); + if (pm_rts == false) + adata->acp_dev->fini(adata->acp_dev); + + return 0; +} + +static int acp_pcm_resume(struct device *dev) +{ + bool pm_rts; + struct snd_pcm_substream *pstream, *cstream; + struct snd_pcm_runtime *prtd, *crtd; + struct audio_substream_data *rtd; + struct audio_drv_data *adata = dev_get_drvdata(dev); + + pm_rts = pm_runtime_status_suspended(dev); + if (pm_rts == true) { + /* Resumed from system wide suspend and there is + * no pending audio activity to resume. */ + pm_runtime_disable(dev); + pm_runtime_set_active(dev); + pm_runtime_enable(dev); + + goto out; + } + + pstream = adata->play_stream; + prtd = pstream ? pstream->runtime : NULL; + if (prtd != NULL) { + /* Resume playback stream from a suspended state */ + rtd = prtd->private_data; + + adata->acp_dev->config_dma(adata->acp_dev, rtd); + adata->acp_dev->config_i2s(adata->acp_dev, rtd); + } + + cstream = adata->capture_stream; + crtd = cstream ? cstream->runtime : NULL; + if (crtd != NULL) { + /* Resume capture stream from a suspended state */ + rtd = crtd->private_data; + + adata->acp_dev->config_dma(adata->acp_dev, rtd); + adata->acp_dev->config_i2s(adata->acp_dev, rtd); + } +out: + return 0; +} + +static int acp_pcm_runtime_suspend(struct device *dev) +{ + struct audio_drv_data *adata = dev_get_drvdata(dev); + + adata->acp_dev->acp_suspend(adata->acp_dev); + return 0; +} + +static int acp_pcm_runtime_resume(struct device *dev) +{ + struct audio_drv_data *adata = dev_get_drvdata(dev); + + adata->acp_dev->acp_resume(adata->acp_dev); + return 0; +} + +static const struct dev_pm_ops acp_pm_ops = { + .suspend = acp_pcm_suspend, + .resume = acp_pcm_resume, + .runtime_suspend = acp_pcm_runtime_suspend, + .runtime_resume = acp_pcm_runtime_resume, +}; + +static struct platform_driver acp_dma_driver = { + .probe = acp_audio_probe, + .remove = acp_audio_remove, + .driver = { + .name = "acp-i2s-audio", + .pm = &acp_pm_ops, + }, +}; + +module_platform_driver(acp_dma_driver); + +MODULE_AUTHOR("Maruthi.Bayyavarapu@amd.com"); +MODULE_DESCRIPTION("AMD ACP PCM Driver"); +MODULE_LICENSE("GPL and additional rights"); +MODULE_ALIAS("platform:acp-i2s-audio");
On Thu, Aug 20, 2015 at 05:36:34PM -0400, Alex Deucher wrote:
From: Maruthi Srinivas Bayyavarapu Maruthi.Bayyavarapu@amd.com
ACP IP block consists of dedicated DMA and I2S blocks. The PCM driver provides the DMA and CPU DAI components to ALSA core.
This is flagged as patch 3/3 but appears to be sent as a single patch. Please don't do this, the purpose of numbering patches is to help people tell which order they are in. Numbering only makes sense when you send more than one patch, if you are sending a single patch there is no need to number.
+++ b/sound/soc/amd/Kconfig @@ -0,0 +1,5 @@ +config SND_SOC_AMD_ACP
- tristate "AMD Audio Coprocessor support"
- depends on MFD_CORE
What is the dependency on the MFD core?
+/*
- AMD ALSA SoC PCM Driver
- Copyright 2014-2015 Advanced Micro Devices, Inc.
- Permission is hereby granted, free of charge, to any person obtaining a
- copy of this software and associated documentation files (the "Software"),
- to deal in the Software without restriction, including without limitation
- the rights to use, copy, modify, merge, publish, distribute, sublicense,
- and/or sell copies of the Software, and to permit persons to whom the
- Software is furnished to do so, subject to the following conditions:
- The above copyright notice and this permission notice shall be included in
- all copies or substantial portions of the Software.
- THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
- THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
- OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
- ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
- OTHER DEALINGS IN THE SOFTWARE.
Still not happy with the licensing.
+static const struct snd_soc_component_driver dw_i2s_component = {
- .name = "dw-i2s",
+};
We already have a driver for the DesignWare I2S controller. To repeat the concern I raised in a slightly different bit of the code last time:
| This doesn't appear to be a designware-i2s driver, we already have one | of those?
Like I say please stop ignoring review comments. The hw_params() certainly seems to bear more than a passing resemblance.
+static void acp_pcm_period_elapsed(struct device *dev, u16 play_intr,
u16 capture_intr)+{
- struct snd_pcm_substream *substream;
- struct audio_drv_data *irq_data = dev_get_drvdata(dev);
- /* Inform ALSA about the period elapsed (one out of two periods) */
- if (play_intr)
substream = irq_data->play_stream;- else
substream = irq_data->capture_stream;
So you've replaced the two if statements with an if/else which means subsstream is now guaranteed to be initialized but perhaps not in the way the caller intended... I did find the caller (which got moved into another patch in this version) and it appears that the two u16s are actually used to pass boolean flags. The whole interface here now seems odd, what is going on here?
- if (NULL != pg) {
We still normally write these checks more like (pg != NULL) or even just (pg).
/* Save for runtime private data */rtd->pg = pg;rtd->order = get_order(size);/* Let ACP know the Allocated memory */num_of_pages = PAGE_ALIGN(size) >> PAGE_SHIFT;/* Fill the page table entries in ACP SRAM */rtd->pg = pg;rtd->size = size;rtd->num_of_pages = num_of_pages;rtd->direction = substream->stream;acp_dev->config_dma(acp_dev, rtd);status = 0;- } else {
status = -ENOMEM;- }
- return status;
+}
+static int acp_dma_hw_free(struct snd_pcm_substream *substream) +{
- return snd_pcm_lib_free_pages(substream);
+}
hw_free() doesn't seem to undo everything that we did on allocation?
/* Now configure DMA to transfer only first half of System RAM* buffer before playback is triggered. This will overwrite* zero-ed second half of SRAM buffer */acp_dev->config_dma_channel(acp_dev, SYSRAM_TO_ACP_CH_NUM,PLAYBACK_START_DMA_DESCR_CH12,1, 0);
| Why? The comments describe what's happening but it's not clear why it's | happening.
+static struct snd_soc_dai_driver i2s_dai_driver = {
- .playback = {
.stream_name = "I2S Playback",.channels_min = 2,.channels_max = 2,
Elsewhere support for 8 channels was declared and handled.
.rates = SNDRV_PCM_RATE_8000_96000,.formats = SNDRV_PCM_FMTBIT_S24_LE |SNDRV_PCM_FMTBIT_S32_LE,
Elsewhere there was 16 bit support declared and handled.
- pm_rts = pm_runtime_status_suspended(dev);
- if (pm_rts == true) {
There seem to be a lot of variables in here that have only one assignment and one user immediately next to them. I'm not sure it's adding much.
On Thu, Aug 20, 2015 at 7:18 PM, Mark Brown broonie@kernel.org wrote:
On Thu, Aug 20, 2015 at 05:36:34PM -0400, Alex Deucher wrote:
From: Maruthi Srinivas Bayyavarapu Maruthi.Bayyavarapu@amd.com
ACP IP block consists of dedicated DMA and I2S blocks. The PCM driver provides the DMA and CPU DAI components to ALSA core.
This is flagged as patch 3/3 but appears to be sent as a single patch. Please don't do this, the purpose of numbering patches is to help people tell which order they are in. Numbering only makes sense when you send more than one patch, if you are sending a single patch there is no need to number.
Sorry, I just saw this reply now. I just resent the patch with an improved change log. You an ignore that one.
Maruthi, can you comment on the points below and respin the patches as necessary?
Thanks,
Alex
+++ b/sound/soc/amd/Kconfig @@ -0,0 +1,5 @@ +config SND_SOC_AMD_ACP
tristate "AMD Audio Coprocessor support"depends on MFD_COREWhat is the dependency on the MFD core?
+/*
- AMD ALSA SoC PCM Driver
- Copyright 2014-2015 Advanced Micro Devices, Inc.
- Permission is hereby granted, free of charge, to any person obtaining a
- copy of this software and associated documentation files (the "Software"),
- to deal in the Software without restriction, including without limitation
- the rights to use, copy, modify, merge, publish, distribute, sublicense,
- and/or sell copies of the Software, and to permit persons to whom the
- Software is furnished to do so, subject to the following conditions:
- The above copyright notice and this permission notice shall be included in
- all copies or substantial portions of the Software.
- THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
- THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
- OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
- ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
- OTHER DEALINGS IN THE SOFTWARE.
Still not happy with the licensing.
+static const struct snd_soc_component_driver dw_i2s_component = {
.name = "dw-i2s",+};
We already have a driver for the DesignWare I2S controller. To repeat the concern I raised in a slightly different bit of the code last time:
| This doesn't appear to be a designware-i2s driver, we already have one | of those?
Like I say please stop ignoring review comments. The hw_params() certainly seems to bear more than a passing resemblance.
+static void acp_pcm_period_elapsed(struct device *dev, u16 play_intr,
u16 capture_intr)+{
struct snd_pcm_substream *substream;struct audio_drv_data *irq_data = dev_get_drvdata(dev);/* Inform ALSA about the period elapsed (one out of two periods) */if (play_intr)substream = irq_data->play_stream;elsesubstream = irq_data->capture_stream;So you've replaced the two if statements with an if/else which means subsstream is now guaranteed to be initialized but perhaps not in the way the caller intended... I did find the caller (which got moved into another patch in this version) and it appears that the two u16s are actually used to pass boolean flags. The whole interface here now seems odd, what is going on here?
if (NULL != pg) {We still normally write these checks more like (pg != NULL) or even just (pg).
/* Save for runtime private data */rtd->pg = pg;rtd->order = get_order(size);/* Let ACP know the Allocated memory */num_of_pages = PAGE_ALIGN(size) >> PAGE_SHIFT;/* Fill the page table entries in ACP SRAM */rtd->pg = pg;rtd->size = size;rtd->num_of_pages = num_of_pages;rtd->direction = substream->stream;acp_dev->config_dma(acp_dev, rtd);status = 0;} else {status = -ENOMEM;}return status;+}
+static int acp_dma_hw_free(struct snd_pcm_substream *substream) +{
return snd_pcm_lib_free_pages(substream);+}
hw_free() doesn't seem to undo everything that we did on allocation?
/* Now configure DMA to transfer only first half of System RAM* buffer before playback is triggered. This will overwrite* zero-ed second half of SRAM buffer */acp_dev->config_dma_channel(acp_dev, SYSRAM_TO_ACP_CH_NUM,PLAYBACK_START_DMA_DESCR_CH12,1, 0);| Why? The comments describe what's happening but it's not clear why it's | happening.
+static struct snd_soc_dai_driver i2s_dai_driver = {
.playback = {.stream_name = "I2S Playback",.channels_min = 2,.channels_max = 2,Elsewhere support for 8 channels was declared and handled.
.rates = SNDRV_PCM_RATE_8000_96000,.formats = SNDRV_PCM_FMTBIT_S24_LE |SNDRV_PCM_FMTBIT_S32_LE,Elsewhere there was 16 bit support declared and handled.
pm_rts = pm_runtime_status_suspended(dev);if (pm_rts == true) {There seem to be a lot of variables in here that have only one assignment and one user immediately next to them. I'm not sure it's adding much.
On Fri, Aug 21, 2015 at 4:48 AM, Mark Brown broonie@kernel.org wrote:
On Thu, Aug 20, 2015 at 05:36:34PM -0400, Alex Deucher wrote:
From: Maruthi Srinivas Bayyavarapu Maruthi.Bayyavarapu@amd.com
ACP IP block consists of dedicated DMA and I2S blocks. The PCM driver provides the DMA and CPU DAI components to ALSA core.
This is flagged as patch 3/3 but appears to be sent as a single patch. Please don't do this, the purpose of numbering patches is to help people tell which order they are in. Numbering only makes sense when you send more than one patch, if you are sending a single patch there is no need to number.
Ok, sorry. I will be more careful next time. Actually, two patches are sent - one for DRM and other ASoC. It should have been represented as 2/2.
+++ b/sound/soc/amd/Kconfig @@ -0,0 +1,5 @@ +config SND_SOC_AMD_ACP
tristate "AMD Audio Coprocessor support"depends on MFD_COREWhat is the dependency on the MFD core?
None. Sorry, I forgot to remove this. Actually, AMD DRM driver depends on MFD_CORE to create a platform device for ASoC. This is already present in DRM patch.
+/*
- AMD ALSA SoC PCM Driver
- Copyright 2014-2015 Advanced Micro Devices, Inc.
- Permission is hereby granted, free of charge, to any person obtaining a
- copy of this software and associated documentation files (the "Software"),
- to deal in the Software without restriction, including without limitation
- the rights to use, copy, modify, merge, publish, distribute, sublicense,
- and/or sell copies of the Software, and to permit persons to whom the
- Software is furnished to do so, subject to the following conditions:
- The above copyright notice and this permission notice shall be included in
- all copies or substantial portions of the Software.
- THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
- THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
- OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
- ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
- OTHER DEALINGS IN THE SOFTWARE.
Still not happy with the licensing.
I will contact our internal teams and get back on this.
+static const struct snd_soc_component_driver dw_i2s_component = {
.name = "dw-i2s",+};
We already have a driver for the DesignWare I2S controller. To repeat the concern I raised in a slightly different bit of the code last time:
| This doesn't appear to be a designware-i2s driver, we already have one | of those?
Our IP block includes few AMD IPs along with DesignWare I2S IP. I have reused code from Designware I2S controller from sound/soc/dwc/designware_i2s.c and because of the way IPs are coupled together, I couldn't use existing Designware I2S driver as is. I have given credit to the original author in DRM patch copyright header, where register I2S read/writes are made. Do I need to add the same header in ASoC driver too ?
Like I say please stop ignoring review comments. The hw_params() certainly seems to bear more than a passing resemblance.
Iam sorry, this mistake won't be repeated.
+static void acp_pcm_period_elapsed(struct device *dev, u16 play_intr,
u16 capture_intr)+{
struct snd_pcm_substream *substream;struct audio_drv_data *irq_data = dev_get_drvdata(dev);/* Inform ALSA about the period elapsed (one out of two periods) */if (play_intr)substream = irq_data->play_stream;elsesubstream = irq_data->capture_stream;So you've replaced the two if statements with an if/else which means subsstream is now guaranteed to be initialized but perhaps not in the way the caller intended... I did find the caller (which got moved into another patch in this version) and it appears that the two u16s are actually used to pass boolean flags. The whole interface here now seems odd, what is going on here?
Agreed. I will modify this interface to use a bool, in next revision
if (NULL != pg) {We still normally write these checks more like (pg != NULL) or even just (pg).
Ok, will modify accordingly in next revision.
/* Save for runtime private data */rtd->pg = pg;rtd->order = get_order(size);/* Let ACP know the Allocated memory */num_of_pages = PAGE_ALIGN(size) >> PAGE_SHIFT;/* Fill the page table entries in ACP SRAM */rtd->pg = pg;rtd->size = size;rtd->num_of_pages = num_of_pages;rtd->direction = substream->stream;acp_dev->config_dma(acp_dev, rtd);status = 0;} else {status = -ENOMEM;}return status;+}
+static int acp_dma_hw_free(struct snd_pcm_substream *substream) +{
return snd_pcm_lib_free_pages(substream);+}
hw_free() doesn't seem to undo everything that we did on allocation?
Oh! I freed in dma_close(), that were allocated in dma_open(). Rest of them used devm_*
/* Now configure DMA to transfer only first half of System RAM* buffer before playback is triggered. This will overwrite* zero-ed second half of SRAM buffer */acp_dev->config_dma_channel(acp_dev, SYSRAM_TO_ACP_CH_NUM,PLAYBACK_START_DMA_DESCR_CH12,1, 0);| Why? The comments describe what's happening but it's not clear why it's | happening.
|------------|----------| | A | B | DRAM |------------|----------| \ / \ / / DMA /\ |---------/-|---------| | C / | \ D | SRAM ---DMA ---------> I2S |-----------|----------|
buffer in System memory(DRAM) is divided into 2 periods - say A,B. internal memory inside ACP IP (SRAM) is also divided in similar way - say C,D. In hw_params() A and B are filled with zeros. In prepare() , content of A and B are DMA'ed to D and C respectively (as per DMA configuration). When ALSA core fills A and B with audio content (start threshold = A+B size), trigger() is called and content of A is transferred to D. C still holds zeros by now. The internal buffer (SRAM) which holds C+D will be DMA'ed to I2S in cyclic way starting from C.
At the end of C or D's DMA to I2S, irq is generated. In irq handler DMA is done from B to C or A to D depending on C or D' DMA completion respectively and snd_pcm_period_elapsed() is called.
The reason for doing this is : When C completes rendering, calls period_elapsed() and informs ALSA core there is free space to fill new data to system memory. In the same irq handler, B is DMA'ed to C to be ready by the time D completes rendering with prefetched data (in trigger()). when D completes rendering, new data fetched by ALSA core can be DMA'ed from A to D and rendering is continued with C. This is done cyclically.
If I don’t do A->D, B->C and instead do A->C, B->D, there would be timing problem to have new data ready to be rendered. The alternate approach would be implement 'copy' callback of 'snd_pcm_ops' in pcm driver. In that callback, I can use copy_from_user() to get new data and then only issue DMA transfers (A->C / B->D). This can solve the timing issue mentioned, but then I can't handle MMAP based playback/capture. Existing design can handle non-mmap and mmap based transfers, but the only issue(?) would be 1 period size of zeros will be played initially for any new playback usecase.
+static struct snd_soc_dai_driver i2s_dai_driver = {
.playback = {.stream_name = "I2S Playback",.channels_min = 2,.channels_max = 2,Elsewhere support for 8 channels was declared and handled.
The board for which driver is developed, doesn't support more than 2 channels.
.rates = SNDRV_PCM_RATE_8000_96000,.formats = SNDRV_PCM_FMTBIT_S24_LE |SNDRV_PCM_FMTBIT_S32_LE,Elsewhere there was 16 bit support declared and handled.
There is a bug in our I2S IP, which wrongly handles 16 bit and lower resolutions. So, I removed the support. All the players using the driver, will handle unsupported formats with the help of pulseaudio conversions.
pm_rts = pm_runtime_status_suspended(dev);if (pm_rts == true) {There seem to be a lot of variables in here that have only one assignment and one user immediately next to them. I'm not sure it's adding much.
Ok, I will reuse variables, where required and remove extra ones in the next version.
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Fri, Aug 21, 2015 at 05:21:07PM +0530, maruthi srinivas wrote:
On Fri, Aug 21, 2015 at 4:48 AM, Mark Brown broonie@kernel.org wrote:
We already have a driver for the DesignWare I2S controller. To repeat the concern I raised in a slightly different bit of the code last time:
| This doesn't appear to be a designware-i2s driver, we already have one | of those?
Our IP block includes few AMD IPs along with DesignWare I2S IP. I have reused code from Designware I2S controller from sound/soc/dwc/designware_i2s.c and because of the way IPs are coupled together, I couldn't use existing Designware I2S driver as is.
Could you be more specific about the way in which the IPs are coupled and the problems this causes please?
I have given credit to the original author in DRM patch copyright header, where register I2S read/writes are made. Do I need to add the same header in ASoC driver too ?
What I'm looking for is actual code sharing where we use the same code for the I2S controller block or a clear and documented understanding of why it is not possible to share things.
Oh! I freed in dma_close(), that were allocated in dma_open(). Rest of them used devm_*
OK.
/* Now configure DMA to transfer only first half of System RAM* buffer before playback is triggered. This will overwrite* zero-ed second half of SRAM buffer */acp_dev->config_dma_channel(acp_dev, SYSRAM_TO_ACP_CH_NUM,PLAYBACK_START_DMA_DESCR_CH12,1, 0);| Why? The comments describe what's happening but it's not clear why it's | happening.
The reason for doing this is : When C completes rendering, calls period_elapsed() and informs ALSA core there is free space to fill new data to system memory. In the same irq handler, B is DMA'ed to C to be ready by the time D completes rendering with prefetched data (in trigger()). when D completes rendering, new data fetched by ALSA core can be DMA'ed from A to D and rendering is continued with C. This is done cyclically.
My point here is that the internal documentation in the driver should be improved so that someone reading the code can tell why this is being done. It doesn't need to be this full explanation but at least enough for people to be aware of the general issue.
+static struct snd_soc_dai_driver i2s_dai_driver = {
.playback = {.stream_name = "I2S Playback",.channels_min = 2,.channels_max = 2,
Elsewhere support for 8 channels was declared and handled.
The board for which driver is developed, doesn't support more than 2 channels.
This is a driver for the IP, not for the board - you may not be able to test everything but code should try to be as general as it can be.
On Fri, Aug 21, 2015 at 12:17 PM, Mark Brown broonie@kernel.org wrote:
On Fri, Aug 21, 2015 at 05:21:07PM +0530, maruthi srinivas wrote:
On Fri, Aug 21, 2015 at 4:48 AM, Mark Brown broonie@kernel.org wrote:
We already have a driver for the DesignWare I2S controller. To repeat the concern I raised in a slightly different bit of the code last time:
| This doesn't appear to be a designware-i2s driver, we already have one | of those?
Our IP block includes few AMD IPs along with DesignWare I2S IP. I have reused code from Designware I2S controller from sound/soc/dwc/designware_i2s.c and because of the way IPs are coupled together, I couldn't use existing Designware I2S driver as is.
Could you be more specific about the way in which the IPs are coupled and the problems this causes please?
I have given credit to the original author in DRM patch copyright header, where register I2S read/writes are made. Do I need to add the same header in ASoC driver too ?
What I'm looking for is actual code sharing where we use the same code for the I2S controller block or a clear and documented understanding of why it is not possible to share things.
Maruthi can clarify further, but it's not possible to use the designware driver directly because: 1. The i2s registers are within the same MMIO aperture as our other GPU registers. Our GPU driver is designed in such a way that the specific IP modules don't have direct access to the MMIO aperture. They use functions provided by the core driver to access registers. Thus the ACP IP module within the GPU driver does not have direct access to the mmio base pointer in order to pass it on. 2. The designware driver depends on the CLKDEV framework which we don't currently support. 3. Our hardware does not support S16_LE
Alex
Oh! I freed in dma_close(), that were allocated in dma_open(). Rest of them used devm_*
OK.
/* Now configure DMA to transfer only first half of System RAM* buffer before playback is triggered. This will overwrite* zero-ed second half of SRAM buffer */acp_dev->config_dma_channel(acp_dev, SYSRAM_TO_ACP_CH_NUM,PLAYBACK_START_DMA_DESCR_CH12,1, 0);| Why? The comments describe what's happening but it's not clear why it's | happening.
The reason for doing this is : When C completes rendering, calls period_elapsed() and informs ALSA core there is free space to fill new data to system memory. In the same irq handler, B is DMA'ed to C to be ready by the time D completes rendering with prefetched data (in trigger()). when D completes rendering, new data fetched by ALSA core can be DMA'ed from A to D and rendering is continued with C. This is done cyclically.
My point here is that the internal documentation in the driver should be improved so that someone reading the code can tell why this is being done. It doesn't need to be this full explanation but at least enough for people to be aware of the general issue.
+static struct snd_soc_dai_driver i2s_dai_driver = {
.playback = {.stream_name = "I2S Playback",.channels_min = 2,.channels_max = 2,Elsewhere support for 8 channels was declared and handled.
The board for which driver is developed, doesn't support more than 2 channels.
This is a driver for the IP, not for the board - you may not be able to test everything but code should try to be as general as it can be.
On Mon, Aug 24, 2015 at 04:08:31PM -0400, Alex Deucher wrote:
On Fri, Aug 21, 2015 at 12:17 PM, Mark Brown broonie@kernel.org wrote:
What I'm looking for is actual code sharing where we use the same code for the I2S controller block or a clear and documented understanding of why it is not possible to share things.
Maruthi can clarify further, but it's not possible to use the designware driver directly because:
- The i2s registers are within the same MMIO aperture as our other
GPU registers. Our GPU driver is designed in such a way that the specific IP modules don't have direct access to the MMIO aperture. They use functions provided by the core driver to access registers. Thus the ACP IP module within the GPU driver does not have direct access to the mmio base pointer in order to pass it on.
Please explain this in more detail, shared register ranges are very common and are the sort of things MFDs are supposed to help with.
- The designware driver depends on the CLKDEV framework which we
don't currently support.
You need to support the clock API, it's very easy to do so so there is no excuse for doing something custom here.
- Our hardware does not support S16_LE
If you have modified the designware IP to remove this support (why would anyone do that?) it's a trivial quirk, if the restriction comes from some other part of the system like the DMA driver then the constraint will come from that part of the system.
On Tue, Aug 25, 2015 at 11:36 AM, Mark Brown broonie@kernel.org wrote:
On Mon, Aug 24, 2015 at 04:08:31PM -0400, Alex Deucher wrote:
On Fri, Aug 21, 2015 at 12:17 PM, Mark Brown broonie@kernel.org wrote:
What I'm looking for is actual code sharing where we use the same code for the I2S controller block or a clear and documented understanding of why it is not possible to share things.
Maruthi can clarify further, but it's not possible to use the designware driver directly because:
- The i2s registers are within the same MMIO aperture as our other
GPU registers. Our GPU driver is designed in such a way that the specific IP modules don't have direct access to the MMIO aperture. They use functions provided by the core driver to access registers. Thus the ACP IP module within the GPU driver does not have direct access to the mmio base pointer in order to pass it on.
Please explain this in more detail, shared register ranges are very common and are the sort of things MFDs are supposed to help with.
In our case, ACP I2S driver need not do a 'devm_ioremap_resource' to get mmio base. ACP audio IP (DMA + I2S+ Others) registers can be accessed, using GPU's MMIO base. During GPU driver design, it was decided that all the register access for entire GPU MMIO aperture (includes ACP and others) to be done in GPU module only. This is implemented in another patch in this patch series using a abstraction layer. A set of functions were defined for audio DMA and I2S functionality within GPU driver module and those function pointers were passed as platform data to ALSA PCM driver to handle both DMA and I2S.
- The designware driver depends on the CLKDEV framework which we
don't currently support.
You need to support the clock API, it's very easy to do so so there is no excuse for doing something custom here.
Codec acts as master in our case to provide clock to i2s controller and there wasn't a need to use clock APIs unlike in existing designware i2s driver. There is no custom implementation.
- Our hardware does not support S16_LE
If you have modified the designware IP to remove this support (why would anyone do that?) it's a trivial quirk, if the restriction comes from some other part of the system like the DMA driver then the constraint will come from that part of the system.
There is a bug in ACP SoC implementation (which combines internal DMA, designware I2S and other blocks) for 16bit and lower resolution. I felt , it would be better to limit functionality in I2S DAI capabilities. I will put this limitation in DMA driver capabilities, to represent overall sound card capabilities, if you suggest.
On Tue, Aug 25, 2015 at 03:26:54PM +0530, maruthi srinivas wrote:
On Tue, Aug 25, 2015 at 11:36 AM, Mark Brown broonie@kernel.org wrote:
Please explain this in more detail, shared register ranges are very common and are the sort of things MFDs are supposed to help with.
In our case, ACP I2S driver need not do a 'devm_ioremap_resource' to get mmio base.
That sounds like a MFD type problem...
ACP audio IP (DMA + I2S+ Others) registers can be accessed, using GPU's MMIO base. During GPU driver design, it was decided that all the register access for entire GPU MMIO aperture (includes ACP and others) to be done in GPU module only. This is implemented in another patch in this patch series using a abstraction layer.
That sounds like converting the Designware driver to use regmap and providing a regmap would enable code sharing (you can provide a regmap for accessors if you don't use it in the main driver).
- The designware driver depends on the CLKDEV framework which we
don't currently support.
You need to support the clock API, it's very easy to do so so there is no excuse for doing something custom here.
Codec acts as master in our case to provide clock to i2s controller and there wasn't a need to use clock APIs unlike in existing designware i2s driver. There is no custom implementation.
So you just need to add slave mode support to the driver. Again not a reason to just copy the code.
- Our hardware does not support S16_LE
If you have modified the designware IP to remove this support (why would anyone do that?) it's a trivial quirk, if the restriction comes from some other part of the system like the DMA driver then the constraint will come from that part of the system.
There is a bug in ACP SoC implementation (which combines internal DMA, designware I2S and other blocks) for 16bit and lower resolution. I felt , it would be better to limit functionality in I2S DAI capabilities. I will put this limitation in DMA driver capabilities, to represent overall sound card capabilities, if you suggest.
A quirk would also do the job.
participants (3)
-
Alex Deucher -
Mark Brown -
maruthi srinivas