From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> To: Ajit Kumar Pandey <AjitKumar.Pandey@amd.com>, broonie@kernel.org, alsa-devel@alsa-project.org Cc: Sunil-kumar.Dommati@amd.com, Geert Uytterhoeven <geert+renesas@glider.be>, open list <linux-kernel@vger.kernel.org>, Basavaraj.Hiregoudar@amd.com, Takashi Iwai <tiwai@suse.com>, Liam Girdwood <lgirdwood@gmail.com>, V sujith kumar Reddy <vsujithkumar.reddy@amd.com>, Arnd Bergmann <arnd@arndb.de>, Vijendar.Mukunda@amd.com, Alexander.Deucher@amd.com Subject: Re: [PATCH v2 1/6] ASoC: amd: acp: Add generic support for PDM controller on ACP Date: Thu, 13 Jan 2022 12:34:19 -0600 [thread overview] Message-ID: <7d79a8a7-b9b5-8a0c-a140-810bc647927c@linux.intel.com> (raw) In-Reply-To: <20220113163348.434108-2-AjitKumar.Pandey@amd.com> couple of nit-picks: > diff --git a/sound/soc/amd/acp/acp-pdm.c b/sound/soc/amd/acp/acp-pdm.c > new file mode 100644 > index 000000000000..cb9bbd795eee > --- /dev/null > +++ b/sound/soc/amd/acp/acp-pdm.c > @@ -0,0 +1,181 @@ > +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause) > +// > +// This file is provided under a dual BSD/GPLv2 license. When using or > +// redistributing this file, you may do so under either license. > +// > +// Copyright(c) 2021 Advanced Micro Devices, Inc. 2022? > +// > +// Authors: Ajit Kumar Pandey <AjitKumar.Pandey@amd.com> > +// Vijendar Mukunda <Vijendar.Mukunda@amd.com> > +// > + > +/* > + * Generic Hardware interface for ACP Audio PDM controller > + */ > + > +#include <linux/platform_device.h> > +#include <linux/module.h> > +#include <linux/err.h> > +#include <linux/io.h> > +#include <sound/pcm_params.h> > +#include <sound/soc.h> > +#include <sound/soc-dai.h> > +#include <linux/dma-mapping.h> alphabetical order? > + > +#include "amd.h" > + > +#define DRV_NAME "acp-pdm" > + > +#define PDM_DMA_STAT 0x10 > +#define PDM_DMA_INTR_MASK 0x10000 > +#define PDM_DEC_64 0x2 > +#define PDM_CLK_FREQ_MASK 0x07 > +#define PDM_MISC_CTRL_MASK 0x10 > +#define PDM_ENABLE 0x01 > +#define PDM_DISABLE 0x00 > +#define DMA_EN_MASK 0x02 > +#define DELAY_US 5 > +#define PDM_TIMEOUT 1000 > + > +static int acp_dmic_dai_trigger(struct snd_pcm_substream *substream, > + int cmd, struct snd_soc_dai *dai) > +{ > + struct acp_stream *stream = substream->runtime->private_data; > + struct device *dev = dai->component->dev; > + struct acp_dev_data *adata = dev_get_drvdata(dev); > + u32 physical_addr, size_dmic, period_bytes; > + unsigned int dma_enable; > + int ret = 0; > + > + period_bytes = frames_to_bytes(substream->runtime, > + substream->runtime->period_size); > + size_dmic = frames_to_bytes(substream->runtime, > + substream->runtime->buffer_size); > + > + physical_addr = stream->reg_offset + MEM_WINDOW_START; > + > + /* Init DMIC Ring buffer */ > + writel(physical_addr, adata->acp_base + ACP_WOV_RX_RINGBUFADDR); > + writel(size_dmic, adata->acp_base + ACP_WOV_RX_RINGBUFSIZE); > + writel(period_bytes, adata->acp_base + ACP_WOV_RX_INTR_WATERMARK_SIZE); > + writel(0x01, adata->acp_base + ACPAXI2AXI_ATU_CTRL); could this be done in a .prepare step? > + > + switch (cmd) { > + case SNDRV_PCM_TRIGGER_START: > + case SNDRV_PCM_TRIGGER_RESUME: > + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: > + dma_enable = readl(adata->acp_base + ACP_WOV_PDM_DMA_ENABLE); > + if (!(dma_enable & DMA_EN_MASK)) { > + writel(PDM_ENABLE, adata->acp_base + ACP_WOV_PDM_ENABLE); > + writel(PDM_ENABLE, adata->acp_base + ACP_WOV_PDM_DMA_ENABLE); > + } > + > + ret = readl_poll_timeout_atomic(adata->acp_base + ACP_WOV_PDM_DMA_ENABLE, > + dma_enable, (dma_enable & DMA_EN_MASK), > + DELAY_US, PDM_TIMEOUT); > + break; > + case SNDRV_PCM_TRIGGER_STOP: > + case SNDRV_PCM_TRIGGER_SUSPEND: > + case SNDRV_PCM_TRIGGER_PAUSE_PUSH: > + dma_enable = readl(adata->acp_base + ACP_WOV_PDM_DMA_ENABLE); > + if ((dma_enable & DMA_EN_MASK)) { > + writel(PDM_DISABLE, adata->acp_base + ACP_WOV_PDM_ENABLE); > + writel(PDM_DISABLE, adata->acp_base + ACP_WOV_PDM_DMA_ENABLE); > + > + } > + > + ret = readl_poll_timeout_atomic(adata->acp_base + ACP_WOV_PDM_DMA_ENABLE, > + dma_enable, !(dma_enable & DMA_EN_MASK), > + DELAY_US, PDM_TIMEOUT); is the _atomic needed? > + break; > + default: > + ret = -EINVAL; > + break; > + } > + > + return ret; > +} > + > +static int acp_dmic_hwparams(struct snd_pcm_substream *substream, > + struct snd_pcm_hw_params *hwparams, struct snd_soc_dai *dai) > +{ > + struct device *dev = dai->component->dev; > + struct acp_dev_data *adata = dev_get_drvdata(dev); > + unsigned int dmic_ctrl, channels, ch_mask; > + > + /* Enable default DMIC clk */ > + writel(PDM_CLK_FREQ_MASK, adata->acp_base + ACP_WOV_CLK_CTRL); > + dmic_ctrl = readl(adata->acp_base + ACP_WOV_MISC_CTRL); > + dmic_ctrl |= PDM_MISC_CTRL_MASK; > + writel(dmic_ctrl, adata->acp_base + ACP_WOV_MISC_CTRL); .hw_params can be called multiple times, should this clock handling be done in a .prepare step? Or alternatively in .startup - this doesn't seem to depend on hardware parameters? > + > + channels = params_channels(hwparams); > + switch (channels) { > + case 2: > + ch_mask = 0; > + break; > + case 4: > + ch_mask = 1; > + break; > + case 6: > + ch_mask = 2; > + break; > + default: > + dev_err(dev, "Invalid channels %d\n", channels); > + return -EINVAL; > + } > + > + if (params_format(hwparams) != SNDRV_PCM_FORMAT_S32_LE) { > + dev_err(dai->dev, "Invalid format:%d\n", params_format(hwparams)); > + return -EINVAL; > + } > + > + writel(ch_mask, adata->acp_base + ACP_WOV_PDM_NO_OF_CHANNELS); > + writel(PDM_DEC_64, adata->acp_base + ACP_WOV_PDM_DECIMATION_FACTOR); > + > + return 0; > +} > +MODULE_LICENSE("GPL v2"); "GPL" is enough
WARNING: multiple messages have this Message-ID (diff)
From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> To: Ajit Kumar Pandey <AjitKumar.Pandey@amd.com>, broonie@kernel.org, alsa-devel@alsa-project.org Cc: Sunil-kumar.Dommati@amd.com, Arnd Bergmann <arnd@arndb.de>, Geert Uytterhoeven <geert+renesas@glider.be>, Basavaraj.Hiregoudar@amd.com, Takashi Iwai <tiwai@suse.com>, Liam Girdwood <lgirdwood@gmail.com>, open list <linux-kernel@vger.kernel.org>, Alexander.Deucher@amd.com, Vijendar.Mukunda@amd.com, V sujith kumar Reddy <vsujithkumar.reddy@amd.com> Subject: Re: [PATCH v2 1/6] ASoC: amd: acp: Add generic support for PDM controller on ACP Date: Thu, 13 Jan 2022 12:34:19 -0600 [thread overview] Message-ID: <7d79a8a7-b9b5-8a0c-a140-810bc647927c@linux.intel.com> (raw) In-Reply-To: <20220113163348.434108-2-AjitKumar.Pandey@amd.com> couple of nit-picks: > diff --git a/sound/soc/amd/acp/acp-pdm.c b/sound/soc/amd/acp/acp-pdm.c > new file mode 100644 > index 000000000000..cb9bbd795eee > --- /dev/null > +++ b/sound/soc/amd/acp/acp-pdm.c > @@ -0,0 +1,181 @@ > +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause) > +// > +// This file is provided under a dual BSD/GPLv2 license. When using or > +// redistributing this file, you may do so under either license. > +// > +// Copyright(c) 2021 Advanced Micro Devices, Inc. 2022? > +// > +// Authors: Ajit Kumar Pandey <AjitKumar.Pandey@amd.com> > +// Vijendar Mukunda <Vijendar.Mukunda@amd.com> > +// > + > +/* > + * Generic Hardware interface for ACP Audio PDM controller > + */ > + > +#include <linux/platform_device.h> > +#include <linux/module.h> > +#include <linux/err.h> > +#include <linux/io.h> > +#include <sound/pcm_params.h> > +#include <sound/soc.h> > +#include <sound/soc-dai.h> > +#include <linux/dma-mapping.h> alphabetical order? > + > +#include "amd.h" > + > +#define DRV_NAME "acp-pdm" > + > +#define PDM_DMA_STAT 0x10 > +#define PDM_DMA_INTR_MASK 0x10000 > +#define PDM_DEC_64 0x2 > +#define PDM_CLK_FREQ_MASK 0x07 > +#define PDM_MISC_CTRL_MASK 0x10 > +#define PDM_ENABLE 0x01 > +#define PDM_DISABLE 0x00 > +#define DMA_EN_MASK 0x02 > +#define DELAY_US 5 > +#define PDM_TIMEOUT 1000 > + > +static int acp_dmic_dai_trigger(struct snd_pcm_substream *substream, > + int cmd, struct snd_soc_dai *dai) > +{ > + struct acp_stream *stream = substream->runtime->private_data; > + struct device *dev = dai->component->dev; > + struct acp_dev_data *adata = dev_get_drvdata(dev); > + u32 physical_addr, size_dmic, period_bytes; > + unsigned int dma_enable; > + int ret = 0; > + > + period_bytes = frames_to_bytes(substream->runtime, > + substream->runtime->period_size); > + size_dmic = frames_to_bytes(substream->runtime, > + substream->runtime->buffer_size); > + > + physical_addr = stream->reg_offset + MEM_WINDOW_START; > + > + /* Init DMIC Ring buffer */ > + writel(physical_addr, adata->acp_base + ACP_WOV_RX_RINGBUFADDR); > + writel(size_dmic, adata->acp_base + ACP_WOV_RX_RINGBUFSIZE); > + writel(period_bytes, adata->acp_base + ACP_WOV_RX_INTR_WATERMARK_SIZE); > + writel(0x01, adata->acp_base + ACPAXI2AXI_ATU_CTRL); could this be done in a .prepare step? > + > + switch (cmd) { > + case SNDRV_PCM_TRIGGER_START: > + case SNDRV_PCM_TRIGGER_RESUME: > + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: > + dma_enable = readl(adata->acp_base + ACP_WOV_PDM_DMA_ENABLE); > + if (!(dma_enable & DMA_EN_MASK)) { > + writel(PDM_ENABLE, adata->acp_base + ACP_WOV_PDM_ENABLE); > + writel(PDM_ENABLE, adata->acp_base + ACP_WOV_PDM_DMA_ENABLE); > + } > + > + ret = readl_poll_timeout_atomic(adata->acp_base + ACP_WOV_PDM_DMA_ENABLE, > + dma_enable, (dma_enable & DMA_EN_MASK), > + DELAY_US, PDM_TIMEOUT); > + break; > + case SNDRV_PCM_TRIGGER_STOP: > + case SNDRV_PCM_TRIGGER_SUSPEND: > + case SNDRV_PCM_TRIGGER_PAUSE_PUSH: > + dma_enable = readl(adata->acp_base + ACP_WOV_PDM_DMA_ENABLE); > + if ((dma_enable & DMA_EN_MASK)) { > + writel(PDM_DISABLE, adata->acp_base + ACP_WOV_PDM_ENABLE); > + writel(PDM_DISABLE, adata->acp_base + ACP_WOV_PDM_DMA_ENABLE); > + > + } > + > + ret = readl_poll_timeout_atomic(adata->acp_base + ACP_WOV_PDM_DMA_ENABLE, > + dma_enable, !(dma_enable & DMA_EN_MASK), > + DELAY_US, PDM_TIMEOUT); is the _atomic needed? > + break; > + default: > + ret = -EINVAL; > + break; > + } > + > + return ret; > +} > + > +static int acp_dmic_hwparams(struct snd_pcm_substream *substream, > + struct snd_pcm_hw_params *hwparams, struct snd_soc_dai *dai) > +{ > + struct device *dev = dai->component->dev; > + struct acp_dev_data *adata = dev_get_drvdata(dev); > + unsigned int dmic_ctrl, channels, ch_mask; > + > + /* Enable default DMIC clk */ > + writel(PDM_CLK_FREQ_MASK, adata->acp_base + ACP_WOV_CLK_CTRL); > + dmic_ctrl = readl(adata->acp_base + ACP_WOV_MISC_CTRL); > + dmic_ctrl |= PDM_MISC_CTRL_MASK; > + writel(dmic_ctrl, adata->acp_base + ACP_WOV_MISC_CTRL); .hw_params can be called multiple times, should this clock handling be done in a .prepare step? Or alternatively in .startup - this doesn't seem to depend on hardware parameters? > + > + channels = params_channels(hwparams); > + switch (channels) { > + case 2: > + ch_mask = 0; > + break; > + case 4: > + ch_mask = 1; > + break; > + case 6: > + ch_mask = 2; > + break; > + default: > + dev_err(dev, "Invalid channels %d\n", channels); > + return -EINVAL; > + } > + > + if (params_format(hwparams) != SNDRV_PCM_FORMAT_S32_LE) { > + dev_err(dai->dev, "Invalid format:%d\n", params_format(hwparams)); > + return -EINVAL; > + } > + > + writel(ch_mask, adata->acp_base + ACP_WOV_PDM_NO_OF_CHANNELS); > + writel(PDM_DEC_64, adata->acp_base + ACP_WOV_PDM_DECIMATION_FACTOR); > + > + return 0; > +} > +MODULE_LICENSE("GPL v2"); "GPL" is enough
next prev parent reply other threads:[~2022-01-13 18:38 UTC|newest] Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-01-13 16:33 [PATCH v2 0/6] ASOC: amd: acp: Add generic PDM and PCI driver support for ACP Ajit Kumar Pandey 2022-01-13 16:33 ` [PATCH v2 1/6] ASoC: amd: acp: Add generic support for PDM controller on ACP Ajit Kumar Pandey 2022-01-13 16:33 ` Ajit Kumar Pandey 2022-01-13 18:34 ` Pierre-Louis Bossart [this message] 2022-01-13 18:34 ` Pierre-Louis Bossart 2022-01-17 11:48 ` Ajit Kumar Pandey 2022-01-17 11:48 ` Ajit Kumar Pandey 2022-01-13 16:33 ` [PATCH v2 2/6] ASoC: amd: acp: Add PDM controller based dmic dai for Renoir Ajit Kumar Pandey 2022-01-13 16:33 ` Ajit Kumar Pandey 2022-01-13 16:33 ` [PATCH v2 3/6] ASoC: amd: acp: Add generic PCI driver module for ACP device Ajit Kumar Pandey 2022-01-13 16:33 ` Ajit Kumar Pandey 2022-01-13 18:36 ` Pierre-Louis Bossart 2022-01-13 18:36 ` Pierre-Louis Bossart 2022-01-13 16:33 ` [PATCH v2 4/6] ASoC: amd: acp: Add ACP init()/deinit() callback for Renoir Ajit Kumar Pandey 2022-01-13 16:33 ` Ajit Kumar Pandey 2022-01-14 9:01 ` Amadeusz Sławiński 2022-01-14 9:01 ` Amadeusz Sławiński 2022-01-17 11:50 ` Ajit Kumar Pandey 2022-01-17 11:50 ` Ajit Kumar Pandey 2022-01-13 16:33 ` [PATCH v2 5/6] ASoC: amd: acp: acp-legacy: Add DMIC dai link support " Ajit Kumar Pandey 2022-01-13 16:33 ` Ajit Kumar Pandey 2022-01-13 18:38 ` Pierre-Louis Bossart 2022-01-13 18:38 ` Pierre-Louis Bossart 2022-01-13 16:33 ` [PATCH v2 6/6] ASoC: amd: renoir: Add check for acp configuration flags Ajit Kumar Pandey 2022-01-13 16:33 ` Ajit Kumar Pandey 2022-01-16 22:55 ` kernel test robot 2022-01-17 5:12 ` kernel test robot
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=7d79a8a7-b9b5-8a0c-a140-810bc647927c@linux.intel.com \ --to=pierre-louis.bossart@linux.intel.com \ --cc=AjitKumar.Pandey@amd.com \ --cc=Alexander.Deucher@amd.com \ --cc=Basavaraj.Hiregoudar@amd.com \ --cc=Sunil-kumar.Dommati@amd.com \ --cc=Vijendar.Mukunda@amd.com \ --cc=alsa-devel@alsa-project.org \ --cc=arnd@arndb.de \ --cc=broonie@kernel.org \ --cc=geert+renesas@glider.be \ --cc=lgirdwood@gmail.com \ --cc=linux-kernel@vger.kernel.org \ --cc=tiwai@suse.com \ --cc=vsujithkumar.reddy@amd.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.