| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6bb9b13c-2829-d7c3-872b-6e9586189300@amd.com>
Date: Fri, 13 Jan 2023 16:46:25 +0530
From: "Mukunda,Vijendar" <vijendar.mukunda@....com>
To: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
broonie@...nel.org, vkoul@...nel.org, alsa-devel@...a-project.org
Cc: Mastan.Katragadda@....com, Sunil-kumar.Dommati@....com,
open list <linux-kernel@...r.kernel.org>,
Basavaraj.Hiregoudar@....com, Takashi Iwai <tiwai@...e.com>,
Liam Girdwood <lgirdwood@...il.com>, Mario.Limonciello@....com,
arungopal.kondaveeti@....com,
Syed Saba Kareem <Syed.SabaKareem@....com>
Subject: Re: [PATCH 08/19] ASoC: amd: ps: add soundwire dma driver dma ops
On 11/01/23 21:04, Pierre-Louis Bossart wrote:
>
> On 1/11/23 03:02, Vijendar Mukunda wrote:
>> Add Soundwire DMA driver dma ops for Pink Sardine platform.
>>
>> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@....com>
>> ---
>> sound/soc/amd/ps/acp63.h | 61 ++++
>> sound/soc/amd/ps/ps-sdw-dma.c | 531 ++++++++++++++++++++++++++++++++++
>> 2 files changed, 592 insertions(+)
>>
>> diff --git a/sound/soc/amd/ps/acp63.h b/sound/soc/amd/ps/acp63.h
>> index b462320fdf2a..8963cfb6120d 100644
>> --- a/sound/soc/amd/ps/acp63.h
>> +++ b/sound/soc/amd/ps/acp63.h
>> @@ -67,6 +67,38 @@
>> #define ACP_SDW0_IRQ_MASK 21
>> #define ACP_SDW1_IRQ_MASK 2
>> #define ACP_ERROR_IRQ_MASK 29
>> +#define ACP_AUDIO_TX_THRESHOLD 28
>> +#define ACP_BT_TX_THRESHOLD 26
>> +#define ACP_HS_TX_THRESHOLD 24
>> +#define ACP_AUDIO_RX_THRESHOLD 27
>> +#define ACP_BT_RX_THRESHOLD 25
>> +#define ACP_HS_RX_THRESHOLD 23
>> +#define ACP_P1_BT_TX_THRESHOLD 6
>> +#define ACP_P1_BT_RX_THRESHOLD 5
>> +#define ACP_SDW_DMA_IRQ_MASK 0x1F800000
>> +#define ACP_P1_SDW_DMA_IRQ_MASK 0x60
>> +#define ACP63_SDW_MAX_STREAMS 8
>> +
>> +#define ACP_DELAY_US 5
>> +#define SDW_MEM_WINDOW_START 0x4800000
>> +#define ACP_SDW_SRAM_PTE_OFFSET 0x03800400
>> +#define SDW_PTE_OFFSET 0x400
>> +#define SDW_FIFO_SIZE 0x100
>> +#define SDW_DMA_SIZE 0x40
>> +#define ACP_SDW_FIFO_OFFSET 0x100
>> +#define ACP_SDW_RING_BUFF_ADDR_OFFSET (128 * 1024)
>> +
>> +#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,
>> @@ -93,6 +125,17 @@ enum acp_pdev_mask {
>> ACP63_SDW_PDM_DEV_MASK,
>> };
>>
>> +enum channel_type {
>> + ACP_SDW_AUDIO_TX = 0,
>> + ACP_SDW_BT_TX,
>> + ACP_SDW_HS_TX,
>> + ACP_SDW_AUDIO_RX,
>> + ACP_SDW_BT_RX,
>> + ACP_SDW_HS_RX,
>> + ACP_SDW1_BT_TX,
>> + ACP_SDW1_BT_RX,
>> +};
> this was defined in another patch already?
will drop this change.
>
>> +
>> struct pdm_stream_instance {
>> u16 num_pages;
>> u16 channels;
>> @@ -139,4 +182,22 @@ struct acp63_dev_data {
>> struct sdw_dma_dev_data {
>> void __iomem *acp_base;
>> struct mutex *acp_lock;
>> + struct snd_pcm_substream *sdw_stream[ACP63_SDW_MAX_STREAMS];
>> +};
>> +
>> +struct sdw_stream_instance {
> sdw_stream is already a well-defined concept. Please use sdw_dma_stream
> or something less confusing naming-wise.
will rename it.
>> + u16 num_pages;
>> + u16 channels;
>> + u32 stream_id;
>> + dma_addr_t dma_addr;
>> + u64 bytescount;
>> + void __iomem *acp_base;
>> +};
>> +
>> +union acp_sdw_dma_count {
>> + struct {
>> + u32 low;
>> + u32 high;
>> + } bcount;
>> + u64 bytescount;
>> };
>> diff --git a/sound/soc/amd/ps/ps-sdw-dma.c b/sound/soc/amd/ps/ps-sdw-dma.c
>> index 388a4b7df715..e94f76053c66 100644
>> --- a/sound/soc/amd/ps/ps-sdw-dma.c
>> +++ b/sound/soc/amd/ps/ps-sdw-dma.c
>> @@ -12,12 +12,543 @@
>> #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 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,
> is this really limited to 2ch 48kHz? This doesn't align with the
> references to Bluetooth above?
As of now we are enabling solution for 48khz, 2Ch, 16bit.
We will expand the coverage in the future.
>> + .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,
> same here?
>
>> + .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 sdw_stream_instance *sdw_ins, u32 stream_id)
>> +{
>> + u16 page_idx;
>> + u32 low, high, val;
>> + dma_addr_t addr;
>> +
>> + addr = sdw_ins->dma_addr;
>> + val = SDW_PTE_OFFSET + (stream_id * 256);
>> +
>> + /* Group Enable */
>> + acp63_writel(ACP_SDW_SRAM_PTE_OFFSET | BIT(31), sdw_ins->acp_base +
>> + ACPAXI2AXI_ATU_BASE_ADDR_GRP_2);
>> + acp63_writel(PAGE_SIZE_4K_ENABLE, sdw_ins->acp_base +
>> + ACPAXI2AXI_ATU_PAGE_SIZE_GRP_2);
>> + for (page_idx = 0; page_idx < sdw_ins->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);
>> +
>> + acp63_writel(low, sdw_ins->acp_base + ACP_SCRATCH_REG_0 + val);
>> + high |= BIT(31);
>> + acp63_writel(high, sdw_ins->acp_base + ACP_SCRATCH_REG_0 + val + 4);
>> + val += 8;
>> + addr += PAGE_SIZE;
>> + }
>> +
>> + /*cache Invalidation added for Testing */
Below register programming is required. We will remove the comment.
> /* cache
>
>> + acp63_writel(0x1, sdw_ins->acp_base + ACPAXI2AXI_ATU_CTRL);
>> +}
>> +static int acp63_sdw_dma_hw_params(struct snd_soc_component *component,
>> + struct snd_pcm_substream *substream,
>> + struct snd_pcm_hw_params *params)
>> +{
>> + struct sdw_stream_instance *sdw_stream_data;
>> + 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;
>> +
>> + stream_id = 0;
> useless initialization...
Will remove it.
>> + sdw_data = dev_get_drvdata(component->dev);
>> + sdw_stream_data = substream->runtime->private_data;
>> + if (!sdw_stream_data)
>> + return -EINVAL;
>> + stream_id = sdw_stream_data->stream_id;
> ... overriden here
>
>> + sdw_data->sdw_stream[stream_id] = substream;
>> + size = params_buffer_bytes(params);
>> + period_bytes = params_period_bytes(params);
>> + sdw_stream_data->dma_addr = substream->runtime->dma_addr;
>> + sdw_stream_data->num_pages = (PAGE_ALIGN(size) >> PAGE_SHIFT);
>> + acp63_config_dma(sdw_stream_data, stream_id);
>> + ret = acp63_configure_sdw_ringbuffer(sdw_stream_data->acp_base, stream_id, size);
>> + if (ret) {
>> + dev_err(component->dev, "Invalid channel type\n");
>> + return -EINVAL;
>> + }
>> + switch (stream_id) {
>> + case ACP_SDW_AUDIO_TX:
>> + water_mark_size_reg = ACP_AUDIO_TX_INTR_WATERMARK_SIZE;
>> + irq_mask = BIT(ACP_AUDIO_TX_THRESHOLD);
>> + acp_ext_intr_cntl_reg = ACP_EXTERNAL_INTR_CNTL;
>> + break;
> so there's ONE resource to deal with external codecs? How does this work
> if you have a headset codec and an amplifier?
Are you referring to playing a same stream over headset codec and
amplifier?
It's all about channel selection from DMA perspective.
We have tested speaker aggregation and headset playback use cases.
>> + case ACP_SDW_BT_TX:
>> + water_mark_size_reg = ACP_BT_TX_INTR_WATERMARK_SIZE;
>> + irq_mask = BIT(ACP_BT_TX_THRESHOLD);
>> + acp_ext_intr_cntl_reg = ACP_EXTERNAL_INTR_CNTL;
>> + break;
>> + case ACP_SDW_HS_TX:
>> + water_mark_size_reg = ACP_HS_TX_INTR_WATERMARK_SIZE;
>> + irq_mask = BIT(ACP_HS_TX_THRESHOLD);
>> + acp_ext_intr_cntl_reg = ACP_EXTERNAL_INTR_CNTL;
>> + break;
>> + case ACP_SDW1_BT_TX:
>> + water_mark_size_reg = ACP_P1_BT_TX_INTR_WATERMARK_SIZE;
>> + irq_mask = BIT(ACP_P1_BT_TX_THRESHOLD);
>> + acp_ext_intr_cntl_reg = ACP_EXTERNAL_INTR_CNTL1;
>> + break;
>> + case ACP_SDW_AUDIO_RX:
>> + water_mark_size_reg = ACP_AUDIO_RX_INTR_WATERMARK_SIZE;
>> + irq_mask = BIT(ACP_AUDIO_RX_THRESHOLD);
>> + acp_ext_intr_cntl_reg = ACP_EXTERNAL_INTR_CNTL;
>> + break;
>> + case ACP_SDW_BT_RX:
>> + water_mark_size_reg = ACP_BT_RX_INTR_WATERMARK_SIZE;
>> + irq_mask = BIT(ACP_BT_RX_THRESHOLD);
>> + acp_ext_intr_cntl_reg = ACP_EXTERNAL_INTR_CNTL;
>> + break;
>> + case ACP_SDW_HS_RX:
>> + water_mark_size_reg = ACP_HS_RX_INTR_WATERMARK_SIZE;
>> + irq_mask = BIT(ACP_HS_RX_THRESHOLD);
>> + acp_ext_intr_cntl_reg = ACP_EXTERNAL_INTR_CNTL;
>> + break;
>> + case ACP_SDW1_BT_RX:
>> + water_mark_size_reg = ACP_P1_BT_RX_INTR_WATERMARK_SIZE;
>> + irq_mask = BIT(ACP_P1_BT_RX_THRESHOLD);
>> + acp_ext_intr_cntl_reg = ACP_EXTERNAL_INTR_CNTL1;
>> + break;
>> + default:
>> + dev_err(component->dev, "%s: Invalid channel type\n", __func__);
>> + return -EINVAL;
>> + }
>> +
>> + ext_intr_ctrl = acp63_readl(sdw_stream_data->acp_base + acp_ext_intr_cntl_reg);
>> + ext_intr_ctrl |= irq_mask;
>> + acp63_writel(ext_intr_ctrl, sdw_stream_data->acp_base + acp_ext_intr_cntl_reg);
>> + acp63_writel(period_bytes, sdw_stream_data->acp_base + water_mark_size_reg);
>> + return 0;
>> +}
Powered by blists - more mailing lists