Add SoundWire DMA driver dma ops for Pink Sardine platform.
Signed-off-by: Vijendar Mukunda <[email protected]>
---
sound/soc/amd/ps/acp63.h | 73 +++++++
sound/soc/amd/ps/ps-sdw-dma.c | 391 ++++++++++++++++++++++++++++++++++
2 files changed, 464 insertions(+)
diff --git a/sound/soc/amd/ps/acp63.h b/sound/soc/amd/ps/acp63.h
index c95c57970a27..5f7ddcc31842 100644
--- a/sound/soc/amd/ps/acp63.h
+++ b/sound/soc/amd/ps/acp63.h
@@ -103,6 +103,49 @@
#define ACP_SDW1_STAT BIT(2)
#define ACP_ERROR_IRQ BIT(29)
+#define ACP_AUDIO0_TX_THRESHOLD 0x1c
+#define ACP_AUDIO1_TX_THRESHOLD 0x1a
+#define ACP_AUDIO2_TX_THRESHOLD 0x18
+#define ACP_AUDIO0_RX_THRESHOLD 0x1b
+#define ACP_AUDIO1_RX_THRESHOLD 0x19
+#define ACP_AUDIO2_RX_THRESHOLD 0x17
+#define ACP_P1_AUDIO1_TX_THRESHOLD BIT(6)
+#define ACP_P1_AUDIO1_RX_THRESHOLD BIT(5)
+#define ACP_SDW_DMA_IRQ_MASK 0x1F800000
+#define ACP_P1_SDW_DMA_IRQ_MASK 0x60
+#define ACP63_SDW0_DMA_MAX_STREAMS 6
+#define ACP63_SDW1_DMA_MAX_STREAMS 2
+#define ACP_P1_AUDIO_TX_THRESHOLD 6
+#define SDW0_DMA_TX_IRQ_MASK(i) (ACP_AUDIO0_TX_THRESHOLD - (2 * (i)))
+#define SDW0_DMA_RX_IRQ_MASK(i) (ACP_AUDIO0_RX_THRESHOLD - (2 * (i)))
+#define SDW1_DMA_IRQ_MASK(i) (ACP_P1_AUDIO_TX_THRESHOLD - (i))
+
+#define ACP_DELAY_US 5
+#define ACP_SDW_RING_BUFF_ADDR_OFFSET (128 * 1024)
+#define SDW0_MEM_WINDOW_START 0x4800000
+#define ACP_SDW_SRAM_PTE_OFFSET 0x03800400
+#define SDW0_PTE_OFFSET 0x400
+#define SDW_FIFO_SIZE 0x100
+#define SDW_DMA_SIZE 0x40
+#define ACP_SDW0_FIFO_OFFSET 0x100
+#define ACP_SDW_PTE_OFFSET 0x100
+#define SDW_FIFO_OFFSET 0x100
+#define SDW_PTE_OFFSET(i) (SDW0_PTE_OFFSET + ((i) * 0x600))
+#define ACP_SDW_FIFO_OFFSET(i) (ACP_SDW0_FIFO_OFFSET + ((i) * 0x500))
+#define SDW_MEM_WINDOW_START(i) (SDW0_MEM_WINDOW_START + ((i) * 0xC0000))
+
+#define SDW_PLAYBACK_MIN_NUM_PERIODS 2
+#define SDW_PLAYBACK_MAX_NUM_PERIODS 8
+#define SDW_PLAYBACK_MAX_PERIOD_SIZE 8192
+#define SDW_PLAYBACK_MIN_PERIOD_SIZE 1024
+#define SDW_CAPTURE_MIN_NUM_PERIODS 2
+#define SDW_CAPTURE_MAX_NUM_PERIODS 8
+#define SDW_CAPTURE_MAX_PERIOD_SIZE 8192
+#define SDW_CAPTURE_MIN_PERIOD_SIZE 1024
+
+#define SDW_MAX_BUFFER (SDW_PLAYBACK_MAX_PERIOD_SIZE * SDW_PLAYBACK_MAX_NUM_PERIODS)
+#define SDW_MIN_BUFFER SDW_MAX_BUFFER
+
enum acp_config {
ACP_CONFIG_0 = 0,
ACP_CONFIG_1,
@@ -140,6 +183,36 @@ struct pdm_dev_data {
struct sdw_dma_dev_data {
void __iomem *acp_base;
struct mutex *acp_lock; /* used to protect acp common register access */
+ struct snd_pcm_substream *sdw0_dma_stream[ACP63_SDW0_DMA_MAX_STREAMS];
+ struct snd_pcm_substream *sdw1_dma_stream[ACP63_SDW1_DMA_MAX_STREAMS];
+};
+
+struct acp_sdw_dma_stream {
+ u16 num_pages;
+ u16 channels;
+ u32 stream_id;
+ u32 instance;
+ dma_addr_t dma_addr;
+ u64 bytescount;
+};
+
+union acp_sdw_dma_count {
+ struct {
+ u32 low;
+ u32 high;
+ } bcount;
+ u64 bytescount;
+};
+
+struct sdw_dma_ring_buf_reg {
+ u32 reg_dma_size;
+ u32 reg_fifo_addr;
+ u32 reg_fifo_size;
+ u32 reg_ring_buf_size;
+ u32 reg_ring_buf_addr;
+ u32 water_mark_size_reg;
+ u32 pos_low_reg;
+ u32 pos_high_reg;
};
/**
diff --git a/sound/soc/amd/ps/ps-sdw-dma.c b/sound/soc/amd/ps/ps-sdw-dma.c
index f4a8d4022dc8..1de78948f859 100644
--- a/sound/soc/amd/ps/ps-sdw-dma.c
+++ b/sound/soc/amd/ps/ps-sdw-dma.c
@@ -12,12 +12,403 @@
#include <sound/pcm_params.h>
#include <sound/soc.h>
#include <sound/soc-dai.h>
+#include <linux/soundwire/sdw_amd.h>
#include "acp63.h"
#define DRV_NAME "amd_ps_sdw_dma"
+static struct sdw_dma_ring_buf_reg sdw0_dma_ring_buf_reg[ACP63_SDW0_DMA_MAX_STREAMS] = {
+ {ACP_AUDIO0_TX_DMA_SIZE, ACP_AUDIO0_TX_FIFOADDR, ACP_AUDIO0_TX_FIFOSIZE,
+ ACP_AUDIO0_TX_RINGBUFSIZE, ACP_AUDIO0_TX_RINGBUFADDR, ACP_AUDIO0_TX_INTR_WATERMARK_SIZE,
+ ACP_AUDIO0_TX_LINEARPOSITIONCNTR_LOW, ACP_AUDIO0_TX_LINEARPOSITIONCNTR_HIGH},
+ {ACP_AUDIO1_TX_DMA_SIZE, ACP_AUDIO1_TX_FIFOADDR, ACP_AUDIO1_TX_FIFOSIZE,
+ ACP_AUDIO1_TX_RINGBUFSIZE, ACP_AUDIO1_TX_RINGBUFADDR, ACP_AUDIO1_TX_INTR_WATERMARK_SIZE,
+ ACP_AUDIO1_TX_LINEARPOSITIONCNTR_LOW, ACP_AUDIO1_TX_LINEARPOSITIONCNTR_HIGH},
+ {ACP_AUDIO2_TX_DMA_SIZE, ACP_AUDIO2_TX_FIFOADDR, ACP_AUDIO2_TX_FIFOSIZE,
+ ACP_AUDIO2_TX_RINGBUFSIZE, ACP_AUDIO2_TX_RINGBUFADDR, ACP_AUDIO2_TX_INTR_WATERMARK_SIZE,
+ ACP_AUDIO2_TX_LINEARPOSITIONCNTR_LOW, ACP_AUDIO2_TX_LINEARPOSITIONCNTR_HIGH},
+ {ACP_AUDIO0_RX_DMA_SIZE, ACP_AUDIO0_RX_FIFOADDR, ACP_AUDIO0_RX_FIFOSIZE,
+ ACP_AUDIO0_RX_RINGBUFSIZE, ACP_AUDIO0_RX_RINGBUFADDR, ACP_AUDIO0_RX_INTR_WATERMARK_SIZE,
+ ACP_AUDIO0_TX_LINEARPOSITIONCNTR_LOW, ACP_AUDIO0_TX_LINEARPOSITIONCNTR_HIGH},
+ {ACP_AUDIO1_RX_DMA_SIZE, ACP_AUDIO1_RX_FIFOADDR, ACP_AUDIO1_RX_FIFOSIZE,
+ ACP_AUDIO1_RX_RINGBUFSIZE, ACP_AUDIO1_RX_RINGBUFADDR, ACP_AUDIO1_RX_INTR_WATERMARK_SIZE,
+ ACP_AUDIO1_RX_LINEARPOSITIONCNTR_LOW, ACP_AUDIO1_RX_LINEARPOSITIONCNTR_HIGH},
+ {ACP_AUDIO2_RX_DMA_SIZE, ACP_AUDIO2_RX_FIFOADDR, ACP_AUDIO2_RX_FIFOSIZE,
+ ACP_AUDIO2_RX_RINGBUFSIZE, ACP_AUDIO2_RX_RINGBUFADDR, ACP_AUDIO2_RX_INTR_WATERMARK_SIZE,
+ ACP_AUDIO2_RX_LINEARPOSITIONCNTR_LOW, ACP_AUDIO2_RX_LINEARPOSITIONCNTR_HIGH}
+};
+
+static struct sdw_dma_ring_buf_reg sdw1_dma_ring_buf_reg[ACP63_SDW1_DMA_MAX_STREAMS] = {
+ {ACP_P1_AUDIO1_TX_DMA_SIZE, ACP_P1_AUDIO1_TX_FIFOADDR, ACP_P1_AUDIO1_TX_FIFOSIZE,
+ ACP_P1_AUDIO1_TX_RINGBUFSIZE, ACP_P1_AUDIO1_TX_RINGBUFADDR,
+ ACP_P1_AUDIO1_TX_INTR_WATERMARK_SIZE,
+ ACP_P1_AUDIO1_TX_LINEARPOSITIONCNTR_LOW, ACP_P1_AUDIO1_TX_LINEARPOSITIONCNTR_HIGH},
+ {ACP_P1_AUDIO1_RX_DMA_SIZE, ACP_P1_AUDIO1_RX_FIFOADDR, ACP_P1_AUDIO1_RX_FIFOSIZE,
+ ACP_P1_AUDIO1_RX_RINGBUFSIZE, ACP_P1_AUDIO1_RX_RINGBUFADDR,
+ ACP_P1_AUDIO1_RX_INTR_WATERMARK_SIZE,
+ ACP_P1_AUDIO1_RX_LINEARPOSITIONCNTR_LOW, ACP_P1_AUDIO1_RX_LINEARPOSITIONCNTR_HIGH},
+};
+
+static u32 sdw0_dma_enable_reg[ACP63_SDW0_DMA_MAX_STREAMS] = {
+ ACP_SW0_AUDIO0_TX_EN,
+ ACP_SW0_AUDIO1_TX_EN,
+ ACP_SW0_AUDIO2_TX_EN,
+ ACP_SW0_AUDIO0_RX_EN,
+ ACP_SW0_AUDIO1_RX_EN,
+ ACP_SW0_AUDIO2_RX_EN,
+};
+
+static u32 sdw1_dma_enable_reg[ACP63_SDW1_DMA_MAX_STREAMS] = {
+ ACP_SW1_AUDIO1_TX_EN,
+ ACP_SW1_AUDIO1_RX_EN,
+};
+
+static const struct snd_pcm_hardware acp63_sdw_hardware_playback = {
+ .info = SNDRV_PCM_INFO_INTERLEAVED |
+ SNDRV_PCM_INFO_BLOCK_TRANSFER |
+ SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID |
+ SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME,
+ .formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S8 |
+ SNDRV_PCM_FMTBIT_U8 | SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE,
+ .channels_min = 2,
+ .channels_max = 2,
+ .rates = SNDRV_PCM_RATE_48000,
+ .rate_min = 48000,
+ .rate_max = 48000,
+ .buffer_bytes_max = SDW_PLAYBACK_MAX_NUM_PERIODS * SDW_PLAYBACK_MAX_PERIOD_SIZE,
+ .period_bytes_min = SDW_PLAYBACK_MIN_PERIOD_SIZE,
+ .period_bytes_max = SDW_PLAYBACK_MAX_PERIOD_SIZE,
+ .periods_min = SDW_PLAYBACK_MIN_NUM_PERIODS,
+ .periods_max = SDW_PLAYBACK_MAX_NUM_PERIODS,
+};
+
+static const struct snd_pcm_hardware acp63_sdw_hardware_capture = {
+ .info = SNDRV_PCM_INFO_INTERLEAVED |
+ SNDRV_PCM_INFO_BLOCK_TRANSFER |
+ SNDRV_PCM_INFO_MMAP |
+ SNDRV_PCM_INFO_MMAP_VALID |
+ SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME,
+ .formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S8 |
+ SNDRV_PCM_FMTBIT_U8 | SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE,
+ .channels_min = 2,
+ .channels_max = 2,
+ .rates = SNDRV_PCM_RATE_48000,
+ .rate_min = 48000,
+ .rate_max = 48000,
+ .buffer_bytes_max = SDW_CAPTURE_MAX_NUM_PERIODS * SDW_CAPTURE_MAX_PERIOD_SIZE,
+ .period_bytes_min = SDW_CAPTURE_MIN_PERIOD_SIZE,
+ .period_bytes_max = SDW_CAPTURE_MAX_PERIOD_SIZE,
+ .periods_min = SDW_CAPTURE_MIN_NUM_PERIODS,
+ .periods_max = SDW_CAPTURE_MAX_NUM_PERIODS,
+};
+
+static void acp63_config_dma(struct acp_sdw_dma_stream *stream, void __iomem *acp_base,
+ u32 stream_id)
+{
+ u16 page_idx;
+ u32 low, high, val;
+ u32 sdw_dma_pte_offset;
+ dma_addr_t addr;
+
+ addr = stream->dma_addr;
+ sdw_dma_pte_offset = SDW_PTE_OFFSET(stream->instance);
+ val = sdw_dma_pte_offset + (stream_id * ACP_SDW_PTE_OFFSET);
+
+ /* Group Enable */
+ writel(ACP_SDW_SRAM_PTE_OFFSET | BIT(31), acp_base + ACPAXI2AXI_ATU_BASE_ADDR_GRP_2);
+ writel(PAGE_SIZE_4K_ENABLE, acp_base + ACPAXI2AXI_ATU_PAGE_SIZE_GRP_2);
+ for (page_idx = 0; page_idx < stream->num_pages; page_idx++) {
+ /* Load the low address of page int ACP SRAM through SRBM */
+ low = lower_32_bits(addr);
+ high = upper_32_bits(addr);
+
+ writel(low, acp_base + ACP_SCRATCH_REG_0 + val);
+ high |= BIT(31);
+ writel(high, acp_base + ACP_SCRATCH_REG_0 + val + 4);
+ val += 8;
+ addr += PAGE_SIZE;
+ }
+ writel(0x1, acp_base + ACPAXI2AXI_ATU_CTRL);
+}
+
+static int acp63_configure_sdw_ringbuffer(void __iomem *acp_base, u32 stream_id, u32 size,
+ u32 manager_instance)
+{
+ u32 reg_dma_size;
+ u32 reg_fifo_addr;
+ u32 reg_fifo_size;
+ u32 reg_ring_buf_size;
+ u32 reg_ring_buf_addr;
+ u32 sdw_fifo_addr;
+ u32 sdw_fifo_offset;
+ u32 sdw_ring_buf_addr;
+ u32 sdw_ring_buf_size;
+ u32 sdw_mem_window_offset;
+
+ switch (manager_instance) {
+ case ACP_SDW0:
+ reg_dma_size = sdw0_dma_ring_buf_reg[stream_id].reg_dma_size;
+ reg_fifo_addr = sdw0_dma_ring_buf_reg[stream_id].reg_fifo_addr;
+ reg_fifo_size = sdw0_dma_ring_buf_reg[stream_id].reg_fifo_size;
+ reg_ring_buf_size = sdw0_dma_ring_buf_reg[stream_id].reg_ring_buf_size;
+ reg_ring_buf_addr = sdw0_dma_ring_buf_reg[stream_id].reg_ring_buf_addr;
+ break;
+ case ACP_SDW1:
+ reg_dma_size = sdw1_dma_ring_buf_reg[stream_id].reg_dma_size;
+ reg_fifo_addr = sdw1_dma_ring_buf_reg[stream_id].reg_fifo_addr;
+ reg_fifo_size = sdw1_dma_ring_buf_reg[stream_id].reg_fifo_size;
+ reg_ring_buf_size = sdw1_dma_ring_buf_reg[stream_id].reg_ring_buf_size;
+ reg_ring_buf_addr = sdw1_dma_ring_buf_reg[stream_id].reg_ring_buf_addr;
+ break;
+ default:
+ return -EINVAL;
+ }
+ sdw_fifo_offset = ACP_SDW_FIFO_OFFSET(manager_instance);
+ sdw_mem_window_offset = SDW_MEM_WINDOW_START(manager_instance);
+ sdw_fifo_addr = sdw_fifo_offset + (stream_id * SDW_FIFO_OFFSET);
+ sdw_ring_buf_addr = sdw_mem_window_offset + (stream_id * ACP_SDW_RING_BUFF_ADDR_OFFSET);
+ sdw_ring_buf_size = size;
+ writel(sdw_ring_buf_size, acp_base + reg_ring_buf_size);
+ writel(sdw_ring_buf_addr, acp_base + reg_ring_buf_addr);
+ writel(sdw_fifo_addr, acp_base + reg_fifo_addr);
+ writel(SDW_DMA_SIZE, acp_base + reg_dma_size);
+ writel(SDW_FIFO_SIZE, acp_base + reg_fifo_size);
+ return 0;
+}
+
+static int acp63_sdw_dma_open(struct snd_soc_component *component,
+ struct snd_pcm_substream *substream)
+{
+ struct snd_pcm_runtime *runtime;
+ struct acp_sdw_dma_stream *stream;
+ struct snd_soc_dai *cpu_dai;
+ struct amd_sdw_manager *amd_manager;
+ struct snd_soc_pcm_runtime *prtd = substream->private_data;
+ int ret;
+
+ runtime = substream->runtime;
+ cpu_dai = asoc_rtd_to_cpu(prtd, 0);
+ amd_manager = snd_soc_dai_get_drvdata(cpu_dai);
+ stream = kzalloc(sizeof(*stream), GFP_KERNEL);
+ if (!stream)
+ return -ENOMEM;
+
+ if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+ runtime->hw = acp63_sdw_hardware_playback;
+ else
+ runtime->hw = acp63_sdw_hardware_capture;
+ ret = snd_pcm_hw_constraint_integer(runtime,
+ SNDRV_PCM_HW_PARAM_PERIODS);
+ if (ret < 0) {
+ dev_err(component->dev, "set integer constraint failed\n");
+ kfree(stream);
+ return ret;
+ }
+
+ stream->stream_id = cpu_dai->id;
+ stream->instance = amd_manager->instance;
+ runtime->private_data = stream;
+ return ret;
+}
+
+static int acp63_sdw_dma_hw_params(struct snd_soc_component *component,
+ struct snd_pcm_substream *substream,
+ struct snd_pcm_hw_params *params)
+{
+ struct acp_sdw_dma_stream *stream;
+ struct sdw_dma_dev_data *sdw_data;
+ u32 period_bytes;
+ u32 water_mark_size_reg;
+ u32 irq_mask, ext_intr_ctrl;
+ u64 size;
+ u32 stream_id;
+ u32 acp_ext_intr_cntl_reg;
+ int ret;
+
+ sdw_data = dev_get_drvdata(component->dev);
+ stream = substream->runtime->private_data;
+ if (!stream)
+ return -EINVAL;
+ stream_id = stream->stream_id;
+ switch (stream->instance) {
+ case ACP_SDW0:
+ sdw_data->sdw0_dma_stream[stream_id] = substream;
+ water_mark_size_reg = sdw0_dma_ring_buf_reg[stream_id].water_mark_size_reg;
+ acp_ext_intr_cntl_reg = ACP_EXTERNAL_INTR_CNTL;
+ if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+ irq_mask = BIT(SDW0_DMA_TX_IRQ_MASK(stream_id));
+ else
+ irq_mask = BIT(SDW0_DMA_RX_IRQ_MASK(stream_id));
+ break;
+ case ACP_SDW1:
+ sdw_data->sdw1_dma_stream[stream_id] = substream;
+ acp_ext_intr_cntl_reg = ACP_EXTERNAL_INTR_CNTL1;
+ water_mark_size_reg = sdw1_dma_ring_buf_reg[stream_id].water_mark_size_reg;
+ irq_mask = BIT(SDW1_DMA_IRQ_MASK(stream_id));
+ break;
+ default:
+ return -EINVAL;
+ }
+ size = params_buffer_bytes(params);
+ period_bytes = params_period_bytes(params);
+ stream->dma_addr = substream->runtime->dma_addr;
+ stream->num_pages = (PAGE_ALIGN(size) >> PAGE_SHIFT);
+ acp63_config_dma(stream, sdw_data->acp_base, stream_id);
+ ret = acp63_configure_sdw_ringbuffer(sdw_data->acp_base, stream_id, size,
+ stream->instance);
+ if (ret) {
+ dev_err(component->dev, "Invalid DMA channel\n");
+ return -EINVAL;
+ }
+ ext_intr_ctrl = readl(sdw_data->acp_base + acp_ext_intr_cntl_reg);
+ ext_intr_ctrl |= irq_mask;
+ writel(ext_intr_ctrl, sdw_data->acp_base + acp_ext_intr_cntl_reg);
+ writel(period_bytes, sdw_data->acp_base + water_mark_size_reg);
+ return 0;
+}
+
+static u64 acp63_sdw_get_byte_count(struct acp_sdw_dma_stream *stream, void __iomem *acp_base)
+{
+ union acp_sdw_dma_count byte_count;
+ u32 pos_low_reg, pos_high_reg;
+
+ byte_count.bytescount = 0;
+ switch (stream->instance) {
+ case ACP_SDW0:
+ pos_low_reg = sdw0_dma_ring_buf_reg[stream->stream_id].pos_low_reg;
+ pos_high_reg = sdw0_dma_ring_buf_reg[stream->stream_id].pos_high_reg;
+ break;
+ case ACP_SDW1:
+ pos_low_reg = sdw1_dma_ring_buf_reg[stream->stream_id].pos_low_reg;
+ pos_high_reg = sdw1_dma_ring_buf_reg[stream->stream_id].pos_high_reg;
+ break;
+ default:
+ return -EINVAL;
+ }
+ if (pos_low_reg) {
+ byte_count.bcount.high = readl(acp_base + pos_high_reg);
+ byte_count.bcount.low = readl(acp_base + pos_low_reg);
+ }
+ return byte_count.bytescount;
+}
+
+static snd_pcm_uframes_t acp63_sdw_dma_pointer(struct snd_soc_component *comp,
+ struct snd_pcm_substream *substream)
+{
+ struct sdw_dma_dev_data *sdw_data;
+ struct acp_sdw_dma_stream *stream;
+ u32 pos, buffersize;
+ u64 bytescount;
+
+ sdw_data = dev_get_drvdata(comp->dev);
+ stream = substream->runtime->private_data;
+ buffersize = frames_to_bytes(substream->runtime,
+ substream->runtime->buffer_size);
+ bytescount = acp63_sdw_get_byte_count(stream, sdw_data->acp_base);
+ if (bytescount > stream->bytescount)
+ bytescount -= stream->bytescount;
+ pos = do_div(bytescount, buffersize);
+ return bytes_to_frames(substream->runtime, pos);
+}
+
+static int acp63_sdw_dma_new(struct snd_soc_component *component,
+ struct snd_soc_pcm_runtime *rtd)
+{
+ struct device *parent = component->dev->parent;
+
+ snd_pcm_set_managed_buffer_all(rtd->pcm, SNDRV_DMA_TYPE_DEV,
+ parent, SDW_MIN_BUFFER, SDW_MAX_BUFFER);
+ return 0;
+}
+
+static int acp63_sdw_dma_close(struct snd_soc_component *component,
+ struct snd_pcm_substream *substream)
+{
+ struct sdw_dma_dev_data *sdw_data;
+ struct acp_sdw_dma_stream *stream;
+
+ sdw_data = dev_get_drvdata(component->dev);
+ stream = substream->runtime->private_data;
+ if (!stream)
+ return -EINVAL;
+ switch (stream->instance) {
+ case ACP_SDW0:
+ sdw_data->sdw0_dma_stream[stream->stream_id] = NULL;
+ break;
+ case ACP_SDW1:
+ sdw_data->sdw1_dma_stream[stream->stream_id] = NULL;
+ break;
+ default:
+ return -EINVAL;
+ }
+ kfree(stream);
+ return 0;
+}
+
+static int acp63_sdw_dma_enable(struct snd_pcm_substream *substream,
+ void __iomem *acp_base, bool sdw_dma_enable)
+{
+ struct acp_sdw_dma_stream *stream;
+ u32 stream_id;
+ u32 sdw_dma_en_reg;
+ u32 sdw_dma_en_stat_reg;
+ u32 sdw_dma_stat;
+ u32 dma_enable;
+
+ stream = substream->runtime->private_data;
+ stream_id = stream->stream_id;
+ switch (stream->instance) {
+ case ACP_SDW0:
+ sdw_dma_en_reg = sdw0_dma_enable_reg[stream_id];
+ break;
+ case ACP_SDW1:
+ sdw_dma_en_reg = sdw1_dma_enable_reg[stream_id];
+ break;
+ default:
+ return -EINVAL;
+ }
+ sdw_dma_en_stat_reg = sdw_dma_en_reg + 4;
+ dma_enable = sdw_dma_enable;
+ writel(dma_enable, acp_base + sdw_dma_en_reg);
+ return readl_poll_timeout(acp_base + sdw_dma_en_stat_reg, sdw_dma_stat,
+ (sdw_dma_stat == dma_enable), ACP_DELAY_US, ACP_COUNTER);
+}
+
+static int acp63_sdw_dma_trigger(struct snd_soc_component *comp,
+ struct snd_pcm_substream *substream,
+ int cmd)
+{
+ struct sdw_dma_dev_data *sdw_data;
+ int ret;
+
+ sdw_data = dev_get_drvdata(comp->dev);
+ switch (cmd) {
+ case SNDRV_PCM_TRIGGER_START:
+ case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+ case SNDRV_PCM_TRIGGER_RESUME:
+ ret = acp63_sdw_dma_enable(substream, sdw_data->acp_base, true);
+ break;
+ case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+ case SNDRV_PCM_TRIGGER_SUSPEND:
+ case SNDRV_PCM_TRIGGER_STOP:
+ ret = acp63_sdw_dma_enable(substream, sdw_data->acp_base, false);
+ break;
+ default:
+ ret = -EINVAL;
+ }
+ if (ret)
+ dev_err(comp->dev, "trigger %d failed: %d", cmd, ret);
+ return ret;
+}
+
static const struct snd_soc_component_driver acp63_sdw_component = {
.name = DRV_NAME,
+ .open = acp63_sdw_dma_open,
+ .close = acp63_sdw_dma_close,
+ .hw_params = acp63_sdw_dma_hw_params,
+ .trigger = acp63_sdw_dma_trigger,
+ .pointer = acp63_sdw_dma_pointer,
+ .pcm_construct = acp63_sdw_dma_new,
};
static int acp63_sdw_platform_probe(struct platform_device *pdev)
--
2.34.1
> +#define SDW_PLAYBACK_MIN_NUM_PERIODS 2
> +#define SDW_PLAYBACK_MAX_NUM_PERIODS 8
> +#define SDW_PLAYBACK_MAX_PERIOD_SIZE 8192
that's a fairly small max period. That's 21ms for 2ch S32_LE 48kHz.
Does this come from specific limitations or is this an arbitrary number?
A comment on this wouldn't hurt.
> +static u32 sdw0_dma_enable_reg[ACP63_SDW0_DMA_MAX_STREAMS] = {
> + ACP_SW0_AUDIO0_TX_EN,
> + ACP_SW0_AUDIO1_TX_EN,
> + ACP_SW0_AUDIO2_TX_EN,
> + ACP_SW0_AUDIO0_RX_EN,
> + ACP_SW0_AUDIO1_RX_EN,
> + ACP_SW0_AUDIO2_RX_EN,
> +};
> +
> +static u32 sdw1_dma_enable_reg[ACP63_SDW1_DMA_MAX_STREAMS] = {
> + ACP_SW1_AUDIO1_TX_EN,
> + ACP_SW1_AUDIO1_RX_EN,
> +};
Still no explanation as to why SDW0 indices start at zero and SDW1
indices start at one?
> +
> +static const struct snd_pcm_hardware acp63_sdw_hardware_playback = {
> + .info = SNDRV_PCM_INFO_INTERLEAVED |
> + SNDRV_PCM_INFO_BLOCK_TRANSFER |
> + SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID |
> + SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME,
> + .formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S8 |
> + SNDRV_PCM_FMTBIT_U8 | SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE,
> + .channels_min = 2,
> + .channels_max = 2,
> + .rates = SNDRV_PCM_RATE_48000,
> + .rate_min = 48000,
> + .rate_max = 48000,
> + .buffer_bytes_max = SDW_PLAYBACK_MAX_NUM_PERIODS * SDW_PLAYBACK_MAX_PERIOD_SIZE,
> + .period_bytes_min = SDW_PLAYBACK_MIN_PERIOD_SIZE,
> + .period_bytes_max = SDW_PLAYBACK_MAX_PERIOD_SIZE,
> + .periods_min = SDW_PLAYBACK_MIN_NUM_PERIODS,
> + .periods_max = SDW_PLAYBACK_MAX_NUM_PERIODS,
> +};
> +
> +static const struct snd_pcm_hardware acp63_sdw_hardware_capture = {
> + .info = SNDRV_PCM_INFO_INTERLEAVED |
> + SNDRV_PCM_INFO_BLOCK_TRANSFER |
> + SNDRV_PCM_INFO_MMAP |
> + SNDRV_PCM_INFO_MMAP_VALID |
> + SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME,
> + .formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S8 |
> + SNDRV_PCM_FMTBIT_U8 | SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE,
> + .channels_min = 2,
> + .channels_max = 2,
> + .rates = SNDRV_PCM_RATE_48000,
> + .rate_min = 48000,
> + .rate_max = 48000,
> + .buffer_bytes_max = SDW_CAPTURE_MAX_NUM_PERIODS * SDW_CAPTURE_MAX_PERIOD_SIZE,
> + .period_bytes_min = SDW_CAPTURE_MIN_PERIOD_SIZE,
> + .period_bytes_max = SDW_CAPTURE_MAX_PERIOD_SIZE,
> + .periods_min = SDW_CAPTURE_MIN_NUM_PERIODS,
> + .periods_max = SDW_CAPTURE_MAX_NUM_PERIODS,
> +};
> +static int acp63_sdw_dma_open(struct snd_soc_component *component,
> + struct snd_pcm_substream *substream)
> +{
> + struct snd_pcm_runtime *runtime;
> + struct acp_sdw_dma_stream *stream;
> + struct snd_soc_dai *cpu_dai;
> + struct amd_sdw_manager *amd_manager;
> + struct snd_soc_pcm_runtime *prtd = substream->private_data;
> + int ret;
> +
> + runtime = substream->runtime;
> + cpu_dai = asoc_rtd_to_cpu(prtd, 0);
> + amd_manager = snd_soc_dai_get_drvdata(cpu_dai);
> + stream = kzalloc(sizeof(*stream), GFP_KERNEL);
> + if (!stream)
> + return -ENOMEM;
> +
> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> + runtime->hw = acp63_sdw_hardware_playback;
> + else
> + runtime->hw = acp63_sdw_hardware_capture;
> + ret = snd_pcm_hw_constraint_integer(runtime,
> + SNDRV_PCM_HW_PARAM_PERIODS);
> + if (ret < 0) {
> + dev_err(component->dev, "set integer constraint failed\n");
> + kfree(stream);
> + return ret;
> + }
it's not clear to me why you have to add this specific constraint, isn't
this checked already with the sdw_hardware_playback information.
> +static u64 acp63_sdw_get_byte_count(struct acp_sdw_dma_stream *stream, void __iomem *acp_base)
> +{
> + union acp_sdw_dma_count byte_count;
> + u32 pos_low_reg, pos_high_reg;
> +
> + byte_count.bytescount = 0;
> + switch (stream->instance) {
> + case ACP_SDW0:
> + pos_low_reg = sdw0_dma_ring_buf_reg[stream->stream_id].pos_low_reg;
> + pos_high_reg = sdw0_dma_ring_buf_reg[stream->stream_id].pos_high_reg;
> + break;
> + case ACP_SDW1:
> + pos_low_reg = sdw1_dma_ring_buf_reg[stream->stream_id].pos_low_reg;
> + pos_high_reg = sdw1_dma_ring_buf_reg[stream->stream_id].pos_high_reg;
> + break;
> + default:
> + return -EINVAL;
returning -EINVAL as a u64 doesn't seem quite right to me?
> + }
> + if (pos_low_reg) {
> + byte_count.bcount.high = readl(acp_base + pos_high_reg);
> + byte_count.bcount.low = readl(acp_base + pos_low_reg);
> + }
> + return byte_count.bytescount;
> +}
On 12/06/23 23:36, Pierre-Louis Bossart wrote:
>> +#define SDW_PLAYBACK_MIN_NUM_PERIODS 2
>> +#define SDW_PLAYBACK_MAX_NUM_PERIODS 8
>> +#define SDW_PLAYBACK_MAX_PERIOD_SIZE 8192
> that's a fairly small max period. That's 21ms for 2ch S32_LE 48kHz.
>
> Does this come from specific limitations or is this an arbitrary number?
>
> A comment on this wouldn't hurt.
This is the initial version. We haven't exercised different sample
rates/bit depth combinations much. Currently, targeted for 2Ch, 48Khz,
16bit audio streams only with 64k as buffer size.
We will extend support for different sample rates/bit depths combinations
in the future.
>> +static u32 sdw0_dma_enable_reg[ACP63_SDW0_DMA_MAX_STREAMS] = {
>> + ACP_SW0_AUDIO0_TX_EN,
>> + ACP_SW0_AUDIO1_TX_EN,
>> + ACP_SW0_AUDIO2_TX_EN,
>> + ACP_SW0_AUDIO0_RX_EN,
>> + ACP_SW0_AUDIO1_RX_EN,
>> + ACP_SW0_AUDIO2_RX_EN,
>> +};
>> +
>> +static u32 sdw1_dma_enable_reg[ACP63_SDW1_DMA_MAX_STREAMS] = {
>> + ACP_SW1_AUDIO1_TX_EN,
>> + ACP_SW1_AUDIO1_RX_EN,
>> +};
> Still no explanation as to why SDW0 indices start at zero and SDW1
> indices start at one?
We have already provided reply in previous thread, i.e. for v3 patch set.
https://lore.kernel.org/alsa-devel/[email protected]/
>
>> +
>> +static const struct snd_pcm_hardware acp63_sdw_hardware_playback = {
>> + .info = SNDRV_PCM_INFO_INTERLEAVED |
>> + SNDRV_PCM_INFO_BLOCK_TRANSFER |
>> + SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID |
>> + SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME,
>> + .formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S8 |
>> + SNDRV_PCM_FMTBIT_U8 | SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE,
>> + .channels_min = 2,
>> + .channels_max = 2,
>> + .rates = SNDRV_PCM_RATE_48000,
>> + .rate_min = 48000,
>> + .rate_max = 48000,
>> + .buffer_bytes_max = SDW_PLAYBACK_MAX_NUM_PERIODS * SDW_PLAYBACK_MAX_PERIOD_SIZE,
>> + .period_bytes_min = SDW_PLAYBACK_MIN_PERIOD_SIZE,
>> + .period_bytes_max = SDW_PLAYBACK_MAX_PERIOD_SIZE,
>> + .periods_min = SDW_PLAYBACK_MIN_NUM_PERIODS,
>> + .periods_max = SDW_PLAYBACK_MAX_NUM_PERIODS,
>> +};
>> +
>> +static const struct snd_pcm_hardware acp63_sdw_hardware_capture = {
>> + .info = SNDRV_PCM_INFO_INTERLEAVED |
>> + SNDRV_PCM_INFO_BLOCK_TRANSFER |
>> + SNDRV_PCM_INFO_MMAP |
>> + SNDRV_PCM_INFO_MMAP_VALID |
>> + SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME,
>> + .formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S8 |
>> + SNDRV_PCM_FMTBIT_U8 | SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE,
>> + .channels_min = 2,
>> + .channels_max = 2,
>> + .rates = SNDRV_PCM_RATE_48000,
>> + .rate_min = 48000,
>> + .rate_max = 48000,
>> + .buffer_bytes_max = SDW_CAPTURE_MAX_NUM_PERIODS * SDW_CAPTURE_MAX_PERIOD_SIZE,
>> + .period_bytes_min = SDW_CAPTURE_MIN_PERIOD_SIZE,
>> + .period_bytes_max = SDW_CAPTURE_MAX_PERIOD_SIZE,
>> + .periods_min = SDW_CAPTURE_MIN_NUM_PERIODS,
>> + .periods_max = SDW_CAPTURE_MAX_NUM_PERIODS,
>> +};
>> +static int acp63_sdw_dma_open(struct snd_soc_component *component,
>> + struct snd_pcm_substream *substream)
>> +{
>> + struct snd_pcm_runtime *runtime;
>> + struct acp_sdw_dma_stream *stream;
>> + struct snd_soc_dai *cpu_dai;
>> + struct amd_sdw_manager *amd_manager;
>> + struct snd_soc_pcm_runtime *prtd = substream->private_data;
>> + int ret;
>> +
>> + runtime = substream->runtime;
>> + cpu_dai = asoc_rtd_to_cpu(prtd, 0);
>> + amd_manager = snd_soc_dai_get_drvdata(cpu_dai);
>> + stream = kzalloc(sizeof(*stream), GFP_KERNEL);
>> + if (!stream)
>> + return -ENOMEM;
>> +
>> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>> + runtime->hw = acp63_sdw_hardware_playback;
>> + else
>> + runtime->hw = acp63_sdw_hardware_capture;
>> + ret = snd_pcm_hw_constraint_integer(runtime,
>> + SNDRV_PCM_HW_PARAM_PERIODS);
>> + if (ret < 0) {
>> + dev_err(component->dev, "set integer constraint failed\n");
>> + kfree(stream);
>> + return ret;
>> + }
> it's not clear to me why you have to add this specific constraint, isn't
> this checked already with the sdw_hardware_playback information.
In above code, first we are assigning runtime->hw structures.
As per our understanding, we are not assigning any hw_constraints.
This snd_pcm_hw_constraint_integer(runtime,
SNDRV_PCM_HW_PARAM_PERIODS) constraint assures that the number
of periods is integer, hence the buffer size is aligned with the period size.
>
>> +static u64 acp63_sdw_get_byte_count(struct acp_sdw_dma_stream *stream, void __iomem *acp_base)
>> +{
>> + union acp_sdw_dma_count byte_count;
>> + u32 pos_low_reg, pos_high_reg;
>> +
>> + byte_count.bytescount = 0;
>> + switch (stream->instance) {
>> + case ACP_SDW0:
>> + pos_low_reg = sdw0_dma_ring_buf_reg[stream->stream_id].pos_low_reg;
>> + pos_high_reg = sdw0_dma_ring_buf_reg[stream->stream_id].pos_high_reg;
>> + break;
>> + case ACP_SDW1:
>> + pos_low_reg = sdw1_dma_ring_buf_reg[stream->stream_id].pos_low_reg;
>> + pos_high_reg = sdw1_dma_ring_buf_reg[stream->stream_id].pos_high_reg;
>> + break;
>> + default:
>> + return -EINVAL;
> returning -EINVAL as a u64 doesn't seem quite right to me?
Agreed. Default case needs to be corrected. In case of invalid
SDW instance, it should return default byte count which is zero
instead of returning -EINVAL.
We have identified similar fix has to be implemented in our other
dma driver as well.
https://elixir.bootlin.com/linux/v6.4-rc6/source/sound/soc/amd/acp/amd.h#L174
Will push a supplement patch to fix it at one go.
>
>> + }
>> + if (pos_low_reg) {
>> + byte_count.bcount.high = readl(acp_base + pos_high_reg);
>> + byte_count.bcount.low = readl(acp_base + pos_low_reg);
>> + }
>> + return byte_count.bytescount;
>> +}
On 13/06/23 12:30, Mukunda,Vijendar wrote:
> On 12/06/23 23:36, Pierre-Louis Bossart wrote:
>>> +#define SDW_PLAYBACK_MIN_NUM_PERIODS 2
>>> +#define SDW_PLAYBACK_MAX_NUM_PERIODS 8
>>> +#define SDW_PLAYBACK_MAX_PERIOD_SIZE 8192
>> that's a fairly small max period. That's 21ms for 2ch S32_LE 48kHz.
>>
>> Does this come from specific limitations or is this an arbitrary number?
>>
>> A comment on this wouldn't hurt.
> This is the initial version. We haven't exercised different sample
> rates/bit depth combinations much. Currently, targeted for 2Ch, 48Khz,
> 16bit audio streams only with 64k as buffer size.
>
> We will extend support for different sample rates/bit depths combinations
> in the future.
>>> +static u32 sdw0_dma_enable_reg[ACP63_SDW0_DMA_MAX_STREAMS] = {
>>> + ACP_SW0_AUDIO0_TX_EN,
>>> + ACP_SW0_AUDIO1_TX_EN,
>>> + ACP_SW0_AUDIO2_TX_EN,
>>> + ACP_SW0_AUDIO0_RX_EN,
>>> + ACP_SW0_AUDIO1_RX_EN,
>>> + ACP_SW0_AUDIO2_RX_EN,
>>> +};
>>> +
>>> +static u32 sdw1_dma_enable_reg[ACP63_SDW1_DMA_MAX_STREAMS] = {
>>> + ACP_SW1_AUDIO1_TX_EN,
>>> + ACP_SW1_AUDIO1_RX_EN,
>>> +};
>> Still no explanation as to why SDW0 indices start at zero and SDW1
>> indices start at one?
> We have already provided reply in previous thread, i.e. for v3 patch set.
>
> https://lore.kernel.org/alsa-devel/[email protected]/
>
>
>
>
>>> +
>>> +static const struct snd_pcm_hardware acp63_sdw_hardware_playback = {
>>> + .info = SNDRV_PCM_INFO_INTERLEAVED |
>>> + SNDRV_PCM_INFO_BLOCK_TRANSFER |
>>> + SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID |
>>> + SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME,
>>> + .formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S8 |
>>> + SNDRV_PCM_FMTBIT_U8 | SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE,
>>> + .channels_min = 2,
>>> + .channels_max = 2,
>>> + .rates = SNDRV_PCM_RATE_48000,
>>> + .rate_min = 48000,
>>> + .rate_max = 48000,
>>> + .buffer_bytes_max = SDW_PLAYBACK_MAX_NUM_PERIODS * SDW_PLAYBACK_MAX_PERIOD_SIZE,
>>> + .period_bytes_min = SDW_PLAYBACK_MIN_PERIOD_SIZE,
>>> + .period_bytes_max = SDW_PLAYBACK_MAX_PERIOD_SIZE,
>>> + .periods_min = SDW_PLAYBACK_MIN_NUM_PERIODS,
>>> + .periods_max = SDW_PLAYBACK_MAX_NUM_PERIODS,
>>> +};
>>> +
>>> +static const struct snd_pcm_hardware acp63_sdw_hardware_capture = {
>>> + .info = SNDRV_PCM_INFO_INTERLEAVED |
>>> + SNDRV_PCM_INFO_BLOCK_TRANSFER |
>>> + SNDRV_PCM_INFO_MMAP |
>>> + SNDRV_PCM_INFO_MMAP_VALID |
>>> + SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME,
>>> + .formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S8 |
>>> + SNDRV_PCM_FMTBIT_U8 | SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE,
>>> + .channels_min = 2,
>>> + .channels_max = 2,
>>> + .rates = SNDRV_PCM_RATE_48000,
>>> + .rate_min = 48000,
>>> + .rate_max = 48000,
>>> + .buffer_bytes_max = SDW_CAPTURE_MAX_NUM_PERIODS * SDW_CAPTURE_MAX_PERIOD_SIZE,
>>> + .period_bytes_min = SDW_CAPTURE_MIN_PERIOD_SIZE,
>>> + .period_bytes_max = SDW_CAPTURE_MAX_PERIOD_SIZE,
>>> + .periods_min = SDW_CAPTURE_MIN_NUM_PERIODS,
>>> + .periods_max = SDW_CAPTURE_MAX_NUM_PERIODS,
>>> +};
>>> +static int acp63_sdw_dma_open(struct snd_soc_component *component,
>>> + struct snd_pcm_substream *substream)
>>> +{
>>> + struct snd_pcm_runtime *runtime;
>>> + struct acp_sdw_dma_stream *stream;
>>> + struct snd_soc_dai *cpu_dai;
>>> + struct amd_sdw_manager *amd_manager;
>>> + struct snd_soc_pcm_runtime *prtd = substream->private_data;
>>> + int ret;
>>> +
>>> + runtime = substream->runtime;
>>> + cpu_dai = asoc_rtd_to_cpu(prtd, 0);
>>> + amd_manager = snd_soc_dai_get_drvdata(cpu_dai);
>>> + stream = kzalloc(sizeof(*stream), GFP_KERNEL);
>>> + if (!stream)
>>> + return -ENOMEM;
>>> +
>>> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>>> + runtime->hw = acp63_sdw_hardware_playback;
>>> + else
>>> + runtime->hw = acp63_sdw_hardware_capture;
>>> + ret = snd_pcm_hw_constraint_integer(runtime,
>>> + SNDRV_PCM_HW_PARAM_PERIODS);
>>> + if (ret < 0) {
>>> + dev_err(component->dev, "set integer constraint failed\n");
>>> + kfree(stream);
>>> + return ret;
>>> + }
>> it's not clear to me why you have to add this specific constraint, isn't
>> this checked already with the sdw_hardware_playback information.
> In above code, first we are assigning runtime->hw structures.
> As per our understanding, we are not assigning any hw_constraints.
>
> This snd_pcm_hw_constraint_integer(runtime,
> SNDRV_PCM_HW_PARAM_PERIODS) constraint assures that the number
> of periods is integer, hence the buffer size is aligned with the period size.
>
>>> +static u64 acp63_sdw_get_byte_count(struct acp_sdw_dma_stream *stream, void __iomem *acp_base)
>>> +{
>>> + union acp_sdw_dma_count byte_count;
>>> + u32 pos_low_reg, pos_high_reg;
>>> +
>>> + byte_count.bytescount = 0;
>>> + switch (stream->instance) {
>>> + case ACP_SDW0:
>>> + pos_low_reg = sdw0_dma_ring_buf_reg[stream->stream_id].pos_low_reg;
>>> + pos_high_reg = sdw0_dma_ring_buf_reg[stream->stream_id].pos_high_reg;
>>> + break;
>>> + case ACP_SDW1:
>>> + pos_low_reg = sdw1_dma_ring_buf_reg[stream->stream_id].pos_low_reg;
>>> + pos_high_reg = sdw1_dma_ring_buf_reg[stream->stream_id].pos_high_reg;
>>> + break;
>>> + default:
>>> + return -EINVAL;
>> returning -EINVAL as a u64 doesn't seem quite right to me?
> Agreed. Default case needs to be corrected. In case of invalid
> SDW instance, it should return default byte count which is zero
> instead of returning -EINVAL.
>
> We have identified similar fix has to be implemented in our other
> dma driver as well.
> https://elixir.bootlin.com/linux/v6.4-rc6/source/sound/soc/amd/acp/amd.h#L174
>
> Will push a supplement patch to fix it at one go.
>> @Bossart: Let us know, if have any futher comments for our replies.
>>> + }
>>> + if (pos_low_reg) {
>>> + byte_count.bcount.high = readl(acp_base + pos_high_reg);
>>> + byte_count.bcount.low = readl(acp_base + pos_low_reg);
>>> + }
>>> + return byte_count.bytescount;
>>> +}
On Thu, Jun 15, 2023 at 09:20:08PM +0530, Mukunda,Vijendar wrote:
> On 13/06/23 12:30, Mukunda,Vijendar wrote:
> > On 12/06/23 23:36, Pierre-Louis Bossart wrote:
> >>> +#define SDW_PLAYBACK_MIN_NUM_PERIODS 2
> >>> +#define SDW_PLAYBACK_MAX_NUM_PERIODS 8
Not seeing any new text in this mail?
On 15/06/23 21:26, Mark Brown wrote:
> On Thu, Jun 15, 2023 at 09:20:08PM +0530, Mukunda,Vijendar wrote:
>> On 13/06/23 12:30, Mukunda,Vijendar wrote:
>>> On 12/06/23 23:36, Pierre-Louis Bossart wrote:
>>>>> +#define SDW_PLAYBACK_MIN_NUM_PERIODS 2
>>>>> +#define SDW_PLAYBACK_MAX_NUM_PERIODS 8
> Not seeing any new text in this mail?
Sorry it's my bad.
My reply got mixed with previous thread comments.
On 15/06/23 21:20, Mukunda,Vijendar wrote:
> On 13/06/23 12:30, Mukunda,Vijendar wrote:
>> On 12/06/23 23:36, Pierre-Louis Bossart wrote:
>>>> +#define SDW_PLAYBACK_MIN_NUM_PERIODS 2
>>>> +#define SDW_PLAYBACK_MAX_NUM_PERIODS 8
>>>> +#define SDW_PLAYBACK_MAX_PERIOD_SIZE 8192
>>> that's a fairly small max period. That's 21ms for 2ch S32_LE 48kHz.
>>>
>>> Does this come from specific limitations or is this an arbitrary number?
>>>
>>> A comment on this wouldn't hurt.
>> This is the initial version. We haven't exercised different sample
>> rates/bit depth combinations much. Currently, targeted for 2Ch, 48Khz,
>> 16bit audio streams only with 64k as buffer size.
>>
>> We will extend support for different sample rates/bit depths combinations
>> in the future.
>>>> +static u32 sdw0_dma_enable_reg[ACP63_SDW0_DMA_MAX_STREAMS] = {
>>>> + ACP_SW0_AUDIO0_TX_EN,
>>>> + ACP_SW0_AUDIO1_TX_EN,
>>>> + ACP_SW0_AUDIO2_TX_EN,
>>>> + ACP_SW0_AUDIO0_RX_EN,
>>>> + ACP_SW0_AUDIO1_RX_EN,
>>>> + ACP_SW0_AUDIO2_RX_EN,
>>>> +};
>>>> +
>>>> +static u32 sdw1_dma_enable_reg[ACP63_SDW1_DMA_MAX_STREAMS] = {
>>>> + ACP_SW1_AUDIO1_TX_EN,
>>>> + ACP_SW1_AUDIO1_RX_EN,
>>>> +};
>>> Still no explanation as to why SDW0 indices start at zero and SDW1
>>> indices start at one?
>> We have already provided reply in previous thread, i.e. for v3 patch set.
>>
>> https://lore.kernel.org/alsa-devel/[email protected]/
>>
>>
>>
>>
>>>> +
>>>> +static const struct snd_pcm_hardware acp63_sdw_hardware_playback = {
>>>> + .info = SNDRV_PCM_INFO_INTERLEAVED |
>>>> + SNDRV_PCM_INFO_BLOCK_TRANSFER |
>>>> + SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID |
>>>> + SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME,
>>>> + .formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S8 |
>>>> + SNDRV_PCM_FMTBIT_U8 | SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE,
>>>> + .channels_min = 2,
>>>> + .channels_max = 2,
>>>> + .rates = SNDRV_PCM_RATE_48000,
>>>> + .rate_min = 48000,
>>>> + .rate_max = 48000,
>>>> + .buffer_bytes_max = SDW_PLAYBACK_MAX_NUM_PERIODS * SDW_PLAYBACK_MAX_PERIOD_SIZE,
>>>> + .period_bytes_min = SDW_PLAYBACK_MIN_PERIOD_SIZE,
>>>> + .period_bytes_max = SDW_PLAYBACK_MAX_PERIOD_SIZE,
>>>> + .periods_min = SDW_PLAYBACK_MIN_NUM_PERIODS,
>>>> + .periods_max = SDW_PLAYBACK_MAX_NUM_PERIODS,
>>>> +};
>>>> +
>>>> +static const struct snd_pcm_hardware acp63_sdw_hardware_capture = {
>>>> + .info = SNDRV_PCM_INFO_INTERLEAVED |
>>>> + SNDRV_PCM_INFO_BLOCK_TRANSFER |
>>>> + SNDRV_PCM_INFO_MMAP |
>>>> + SNDRV_PCM_INFO_MMAP_VALID |
>>>> + SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME,
>>>> + .formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S8 |
>>>> + SNDRV_PCM_FMTBIT_U8 | SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE,
>>>> + .channels_min = 2,
>>>> + .channels_max = 2,
>>>> + .rates = SNDRV_PCM_RATE_48000,
>>>> + .rate_min = 48000,
>>>> + .rate_max = 48000,
>>>> + .buffer_bytes_max = SDW_CAPTURE_MAX_NUM_PERIODS * SDW_CAPTURE_MAX_PERIOD_SIZE,
>>>> + .period_bytes_min = SDW_CAPTURE_MIN_PERIOD_SIZE,
>>>> + .period_bytes_max = SDW_CAPTURE_MAX_PERIOD_SIZE,
>>>> + .periods_min = SDW_CAPTURE_MIN_NUM_PERIODS,
>>>> + .periods_max = SDW_CAPTURE_MAX_NUM_PERIODS,
>>>> +};
>>>> +static int acp63_sdw_dma_open(struct snd_soc_component *component,
>>>> + struct snd_pcm_substream *substream)
>>>> +{
>>>> + struct snd_pcm_runtime *runtime;
>>>> + struct acp_sdw_dma_stream *stream;
>>>> + struct snd_soc_dai *cpu_dai;
>>>> + struct amd_sdw_manager *amd_manager;
>>>> + struct snd_soc_pcm_runtime *prtd = substream->private_data;
>>>> + int ret;
>>>> +
>>>> + runtime = substream->runtime;
>>>> + cpu_dai = asoc_rtd_to_cpu(prtd, 0);
>>>> + amd_manager = snd_soc_dai_get_drvdata(cpu_dai);
>>>> + stream = kzalloc(sizeof(*stream), GFP_KERNEL);
>>>> + if (!stream)
>>>> + return -ENOMEM;
>>>> +
>>>> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>>>> + runtime->hw = acp63_sdw_hardware_playback;
>>>> + else
>>>> + runtime->hw = acp63_sdw_hardware_capture;
>>>> + ret = snd_pcm_hw_constraint_integer(runtime,
>>>> + SNDRV_PCM_HW_PARAM_PERIODS);
>>>> + if (ret < 0) {
>>>> + dev_err(component->dev, "set integer constraint failed\n");
>>>> + kfree(stream);
>>>> + return ret;
>>>> + }
>>> it's not clear to me why you have to add this specific constraint, isn't
>>> this checked already with the sdw_hardware_playback information.
>> In above code, first we are assigning runtime->hw structures.
>> As per our understanding, we are not assigning any hw_constraints.
>>
>> This snd_pcm_hw_constraint_integer(runtime,
>> SNDRV_PCM_HW_PARAM_PERIODS) constraint assures that the number
>> of periods is integer, hence the buffer size is aligned with the period size.
>>
>>>> +static u64 acp63_sdw_get_byte_count(struct acp_sdw_dma_stream *stream, void __iomem *acp_base)
>>>> +{
>>>> + union acp_sdw_dma_count byte_count;
>>>> + u32 pos_low_reg, pos_high_reg;
>>>> +
>>>> + byte_count.bytescount = 0;
>>>> + switch (stream->instance) {
>>>> + case ACP_SDW0:
>>>> + pos_low_reg = sdw0_dma_ring_buf_reg[stream->stream_id].pos_low_reg;
>>>> + pos_high_reg = sdw0_dma_ring_buf_reg[stream->stream_id].pos_high_reg;
>>>> + break;
>>>> + case ACP_SDW1:
>>>> + pos_low_reg = sdw1_dma_ring_buf_reg[stream->stream_id].pos_low_reg;
>>>> + pos_high_reg = sdw1_dma_ring_buf_reg[stream->stream_id].pos_high_reg;
>>>> + break;
>>>> + default:
>>>> + return -EINVAL;
>>> returning -EINVAL as a u64 doesn't seem quite right to me?
>> Agreed. Default case needs to be corrected. In case of invalid
>> SDW instance, it should return default byte count which is zero
>> instead of returning -EINVAL.
>>
>> We have identified similar fix has to be implemented in our other
>> dma driver as well.
>> https://elixir.bootlin.com/linux/v6.4-rc6/source/sound/soc/amd/acp/amd.h#L174
>>
>> Will push a supplement patch to fix it at one go.
@Bossart: Let us know if you have any further comments for our replies.
>>> .
>>>> + }
>>>> + if (pos_low_reg) {
>>>> + byte_count.bcount.high = readl(acp_base + pos_high_reg);
>>>> + byte_count.bcount.low = readl(acp_base + pos_low_reg);
>>>> + }
>>>> + return byte_count.bytescount;
>>>> +}
On 6/13/23 09:00, Mukunda,Vijendar wrote:
> On 12/06/23 23:36, Pierre-Louis Bossart wrote:
>>> +#define SDW_PLAYBACK_MIN_NUM_PERIODS 2
>>> +#define SDW_PLAYBACK_MAX_NUM_PERIODS 8
>>> +#define SDW_PLAYBACK_MAX_PERIOD_SIZE 8192
>> that's a fairly small max period. That's 21ms for 2ch S32_LE 48kHz.
>>
>> Does this come from specific limitations or is this an arbitrary number?
>>
>> A comment on this wouldn't hurt.
> This is the initial version. We haven't exercised different sample
> rates/bit depth combinations much. Currently, targeted for 2Ch, 48Khz,
> 16bit audio streams only with 64k as buffer size.
>
> We will extend support for different sample rates/bit depths combinations
> in the future.
>>> +static u32 sdw0_dma_enable_reg[ACP63_SDW0_DMA_MAX_STREAMS] = {
>>> + ACP_SW0_AUDIO0_TX_EN,
>>> + ACP_SW0_AUDIO1_TX_EN,
>>> + ACP_SW0_AUDIO2_TX_EN,
>>> + ACP_SW0_AUDIO0_RX_EN,
>>> + ACP_SW0_AUDIO1_RX_EN,
>>> + ACP_SW0_AUDIO2_RX_EN,
>>> +};
>>> +
>>> +static u32 sdw1_dma_enable_reg[ACP63_SDW1_DMA_MAX_STREAMS] = {
>>> + ACP_SW1_AUDIO1_TX_EN,
>>> + ACP_SW1_AUDIO1_RX_EN,
>>> +};
>> Still no explanation as to why SDW0 indices start at zero and SDW1
>> indices start at one?
> We have already provided reply in previous thread, i.e. for v3 patch set.
>
> https://lore.kernel.org/alsa-devel/[email protected]/
That reply was
"
Currently, SDW0 instance uses 3 TX, 3 RX ports whereas SDW1 instance
uses 1 TX, 1 RX ports.
For SDW1 instance, It uses AUDIO1 register set as per our register spec.
We have mantained similar mapping convention here for enums as well.
"
It wouldn't hurt to add a comment in the code to remind the reviewer
that this is intentional and aligned with the hardware documentation.
>>> +static int acp63_sdw_dma_open(struct snd_soc_component *component,
>>> + struct snd_pcm_substream *substream)
>>> +{
>>> + struct snd_pcm_runtime *runtime;
>>> + struct acp_sdw_dma_stream *stream;
>>> + struct snd_soc_dai *cpu_dai;
>>> + struct amd_sdw_manager *amd_manager;
>>> + struct snd_soc_pcm_runtime *prtd = substream->private_data;
>>> + int ret;
>>> +
>>> + runtime = substream->runtime;
>>> + cpu_dai = asoc_rtd_to_cpu(prtd, 0);
>>> + amd_manager = snd_soc_dai_get_drvdata(cpu_dai);
>>> + stream = kzalloc(sizeof(*stream), GFP_KERNEL);
>>> + if (!stream)
>>> + return -ENOMEM;
>>> +
>>> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>>> + runtime->hw = acp63_sdw_hardware_playback;
>>> + else
>>> + runtime->hw = acp63_sdw_hardware_capture;
>>> + ret = snd_pcm_hw_constraint_integer(runtime,
>>> + SNDRV_PCM_HW_PARAM_PERIODS);
>>> + if (ret < 0) {
>>> + dev_err(component->dev, "set integer constraint failed\n");
>>> + kfree(stream);
>>> + return ret;
>>> + }
>> it's not clear to me why you have to add this specific constraint, isn't
>> this checked already with the sdw_hardware_playback information.
> In above code, first we are assigning runtime->hw structures.
> As per our understanding, we are not assigning any hw_constraints.
>
> This snd_pcm_hw_constraint_integer(runtime,
> SNDRV_PCM_HW_PARAM_PERIODS) constraint assures that the number
> of periods is integer, hence the buffer size is aligned with the period size.
This is surprising, I thought this was already ensured by the hw_info stuff?
>>> +static u64 acp63_sdw_get_byte_count(struct acp_sdw_dma_stream *stream, void __iomem *acp_base)
>>> +{
>>> + union acp_sdw_dma_count byte_count;
>>> + u32 pos_low_reg, pos_high_reg;
>>> +
>>> + byte_count.bytescount = 0;
>>> + switch (stream->instance) {
>>> + case ACP_SDW0:
>>> + pos_low_reg = sdw0_dma_ring_buf_reg[stream->stream_id].pos_low_reg;
>>> + pos_high_reg = sdw0_dma_ring_buf_reg[stream->stream_id].pos_high_reg;
>>> + break;
>>> + case ACP_SDW1:
>>> + pos_low_reg = sdw1_dma_ring_buf_reg[stream->stream_id].pos_low_reg;
>>> + pos_high_reg = sdw1_dma_ring_buf_reg[stream->stream_id].pos_high_reg;
>>> + break;
>>> + default:
>>> + return -EINVAL;
>> returning -EINVAL as a u64 doesn't seem quite right to me?
> Agreed. Default case needs to be corrected. In case of invalid
> SDW instance, it should return default byte count which is zero
> instead of returning -EINVAL.
>
> We have identified similar fix has to be implemented in our other
> dma driver as well.
> https://elixir.bootlin.com/linux/v6.4-rc6/source/sound/soc/amd/acp/amd.h#L174
>
> Will push a supplement patch to fix it at one go.
ok
On 15/06/23 21:51, Pierre-Louis Bossart wrote:
>
> On 6/13/23 09:00, Mukunda,Vijendar wrote:
>> On 12/06/23 23:36, Pierre-Louis Bossart wrote:
>>>> +#define SDW_PLAYBACK_MIN_NUM_PERIODS 2
>>>> +#define SDW_PLAYBACK_MAX_NUM_PERIODS 8
>>>> +#define SDW_PLAYBACK_MAX_PERIOD_SIZE 8192
>>> that's a fairly small max period. That's 21ms for 2ch S32_LE 48kHz.
>>>
>>> Does this come from specific limitations or is this an arbitrary number?
>>>
>>> A comment on this wouldn't hurt.
>> This is the initial version. We haven't exercised different sample
>> rates/bit depth combinations much. Currently, targeted for 2Ch, 48Khz,
>> 16bit audio streams only with 64k as buffer size.
>>
>> We will extend support for different sample rates/bit depths combinations
>> in the future.
>>>> +static u32 sdw0_dma_enable_reg[ACP63_SDW0_DMA_MAX_STREAMS] = {
>>>> + ACP_SW0_AUDIO0_TX_EN,
>>>> + ACP_SW0_AUDIO1_TX_EN,
>>>> + ACP_SW0_AUDIO2_TX_EN,
>>>> + ACP_SW0_AUDIO0_RX_EN,
>>>> + ACP_SW0_AUDIO1_RX_EN,
>>>> + ACP_SW0_AUDIO2_RX_EN,
>>>> +};
>>>> +
>>>> +static u32 sdw1_dma_enable_reg[ACP63_SDW1_DMA_MAX_STREAMS] = {
>>>> + ACP_SW1_AUDIO1_TX_EN,
>>>> + ACP_SW1_AUDIO1_RX_EN,
>>>> +};
>>> Still no explanation as to why SDW0 indices start at zero and SDW1
>>> indices start at one?
>> We have already provided reply in previous thread, i.e. for v3 patch set.
>>
>> https://lore.kernel.org/alsa-devel/[email protected]/
> That reply was
>
> "
> Currently, SDW0 instance uses 3 TX, 3 RX ports whereas SDW1 instance
> uses 1 TX, 1 RX ports.
>
> For SDW1 instance, It uses AUDIO1 register set as per our register spec.
> We have mantained similar mapping convention here for enums as well.
> "
>
> It wouldn't hurt to add a comment in the code to remind the reviewer
> that this is intentional and aligned with the hardware documentation.
>
>
>>>> +static int acp63_sdw_dma_open(struct snd_soc_component *component,
>>>> + struct snd_pcm_substream *substream)
>>>> +{
>>>> + struct snd_pcm_runtime *runtime;
>>>> + struct acp_sdw_dma_stream *stream;
>>>> + struct snd_soc_dai *cpu_dai;
>>>> + struct amd_sdw_manager *amd_manager;
>>>> + struct snd_soc_pcm_runtime *prtd = substream->private_data;
>>>> + int ret;
>>>> +
>>>> + runtime = substream->runtime;
>>>> + cpu_dai = asoc_rtd_to_cpu(prtd, 0);
>>>> + amd_manager = snd_soc_dai_get_drvdata(cpu_dai);
>>>> + stream = kzalloc(sizeof(*stream), GFP_KERNEL);
>>>> + if (!stream)
>>>> + return -ENOMEM;
>>>> +
>>>> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>>>> + runtime->hw = acp63_sdw_hardware_playback;
>>>> + else
>>>> + runtime->hw = acp63_sdw_hardware_capture;
>>>> + ret = snd_pcm_hw_constraint_integer(runtime,
>>>> + SNDRV_PCM_HW_PARAM_PERIODS);
>>>> + if (ret < 0) {
>>>> + dev_err(component->dev, "set integer constraint failed\n");
>>>> + kfree(stream);
>>>> + return ret;
>>>> + }
>>> it's not clear to me why you have to add this specific constraint, isn't
>>> this checked already with the sdw_hardware_playback information.
>> In above code, first we are assigning runtime->hw structures.
>> As per our understanding, we are not assigning any hw_constraints.
>>
>> This snd_pcm_hw_constraint_integer(runtime,
>> SNDRV_PCM_HW_PARAM_PERIODS) constraint assures that the number
>> of periods is integer, hence the buffer size is aligned with the period size.
> This is surprising, I thought this was already ensured by the hw_info stuff?
As per our understanding, we are populating period size range,
to ensure always, buffer size should be multiples of period size use the
constraint. Reject the buffer size if it's not multiples of period size.
snd_pcm_hw structure will only list out period_bytes_min, period_bytes_max
and buffer_size. It doesn't do any check.
Similar implementation has been observed in most of the DMA drivers open
callback () including Intel.
Failed to understand your point.
Could you please elaborate, if we miss anything?
>
>>>> +static u64 acp63_sdw_get_byte_count(struct acp_sdw_dma_stream *stream, void __iomem *acp_base)
>>>> +{
>>>> + union acp_sdw_dma_count byte_count;
>>>> + u32 pos_low_reg, pos_high_reg;
>>>> +
>>>> + byte_count.bytescount = 0;
>>>> + switch (stream->instance) {
>>>> + case ACP_SDW0:
>>>> + pos_low_reg = sdw0_dma_ring_buf_reg[stream->stream_id].pos_low_reg;
>>>> + pos_high_reg = sdw0_dma_ring_buf_reg[stream->stream_id].pos_high_reg;
>>>> + break;
>>>> + case ACP_SDW1:
>>>> + pos_low_reg = sdw1_dma_ring_buf_reg[stream->stream_id].pos_low_reg;
>>>> + pos_high_reg = sdw1_dma_ring_buf_reg[stream->stream_id].pos_high_reg;
>>>> + break;
>>>> + default:
>>>> + return -EINVAL;
>>> returning -EINVAL as a u64 doesn't seem quite right to me?
>> Agreed. Default case needs to be corrected. In case of invalid
>> SDW instance, it should return default byte count which is zero
>> instead of returning -EINVAL.
>>
>> We have identified similar fix has to be implemented in our other
>> dma driver as well.
>> https://elixir.bootlin.com/linux/v6.4-rc6/source/sound/soc/amd/acp/amd.h#L174
>>
>> Will push a supplement patch to fix it at one go.
> ok