[alsa-devel] [PATCH v2] davinci-mcasp: Add support for multichannel playback
Davinci McASP has support for I2S multichannel playback. For I2S playback/receive, each serializer is capable to play 2 channels (L/R) audio data.Serializer function (Playback-receive-none) is configured in DT, depending on hardware specification. It is possible to play less channels than configured in DT. For that purpose,only specific number of active serializers are enabled. McASP FIFO need to have DMA transfer Bcnt set to number of enabled serializers, otherwise no data are transfered to McASP and Alsa generates "DMA/IRQ playback write error (DMA or IRQ trouble?)" error.
Signed-off-by: Michal Bachraty michal.bachraty@streamunlimited.com --- sound/soc/davinci/davinci-mcasp.c | 56 ++++++++++++++++++++++++++++++++----- sound/soc/davinci/davinci-pcm.c | 16 ++++++----- sound/soc/davinci/davinci-pcm.h | 1 + 3 files changed, 59 insertions(+), 14 deletions(-)
diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c index afef3fb..b84bb73 100644 --- a/sound/soc/davinci/davinci-mcasp.c +++ b/sound/soc/davinci/davinci-mcasp.c @@ -235,6 +235,10 @@ #define DISMOD (val)(val<<2) #define TXSTATE BIT(4) #define RXSTATE BIT(5) +#define SRMOD_MASK 3 +#define SRMOD_INACTIVE 0 +#define SRMOD_TX 1 +#define SRMOD_RX 2
/* * DAVINCI_MCASP_LBCTL_REG - Loop Back Control Register Bits @@ -657,12 +661,15 @@ static int davinci_config_channel_size(struct davinci_audio_dev *dev, return 0; }
-static void davinci_hw_common_param(struct davinci_audio_dev *dev, int stream) +static int davinci_hw_common_param(struct davinci_audio_dev *dev, int stream, + int channels) { int i; u8 tx_ser = 0; u8 rx_ser = 0; - + u8 ser; + u8 slots = dev->tdm_slots; + u8 max_active_serializers = (channels + slots - 1) / slots; /* Default configuration */ mcasp_set_bits(dev->base + DAVINCI_MCASP_PWREMUMGT_REG, MCASP_SOFT);
@@ -680,16 +687,42 @@ static void davinci_hw_common_param(struct davinci_audio_dev *dev, int stream) }
for (i = 0; i < dev->num_serializer; i++) { + if (dev->serial_dir[i] == TX_MODE) + tx_ser++; + if (dev->serial_dir[i] == RX_MODE) + rx_ser++; + } + + if (stream == SNDRV_PCM_STREAM_PLAYBACK) + ser = tx_ser; + else + ser = rx_ser; + + if (ser < max_active_serializers) { + dev_warn(dev->dev, "stream has more channels (%d) than are " + "enabled in mcasp (%d)\n", channels, ser * slots); + return -EINVAL; + } + + tx_ser = 0; + rx_ser = 0; + + for (i = 0; i < dev->num_serializer; i++) { mcasp_set_bits(dev->base + DAVINCI_MCASP_XRSRCTL_REG(i), dev->serial_dir[i]); - if (dev->serial_dir[i] == TX_MODE) { + if (dev->serial_dir[i] == TX_MODE && + tx_ser < max_active_serializers) { mcasp_set_bits(dev->base + DAVINCI_MCASP_PDIR_REG, AXR(i)); tx_ser++; - } else if (dev->serial_dir[i] == RX_MODE) { + } else if (dev->serial_dir[i] == RX_MODE && + rx_ser < max_active_serializers) { mcasp_clr_bits(dev->base + DAVINCI_MCASP_PDIR_REG, AXR(i)); rx_ser++; + } else { + mcasp_mod_bits(dev->base + DAVINCI_MCASP_XRSRCTL_REG(i), + SRMOD_INACTIVE, SRMOD_MASK); } }
@@ -729,6 +762,8 @@ static void davinci_hw_common_param(struct davinci_audio_dev *dev, int stream) ((dev->rxnumevt * rx_ser) << 8), NUMEVT_MASK); } } + + return 0; }
static void davinci_hw_param(struct davinci_audio_dev *dev, int stream) @@ -812,8 +847,14 @@ static int davinci_mcasp_hw_params(struct snd_pcm_substream *substream, &dev->dma_params[substream->stream]; int word_length; u8 fifo_level; + u8 slots = dev->tdm_slots; + int channels; + struct snd_interval *pcm_channels = hw_param_interval(params, + SNDRV_PCM_HW_PARAM_CHANNELS); + channels = pcm_channels->min;
- davinci_hw_common_param(dev, substream->stream); + if (davinci_hw_common_param(dev, substream->stream, channels)) + return -EINVAL; if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) fifo_level = dev->txnumevt; else @@ -862,6 +903,7 @@ static int davinci_mcasp_hw_params(struct snd_pcm_substream *substream, dma_params->acnt = dma_params->data_type;
dma_params->fifo_level = fifo_level; + dma_params->active_serializers = (channels + slots - 1) / slots; davinci_config_channel_size(dev, word_length);
return 0; @@ -936,13 +978,13 @@ static struct snd_soc_dai_driver davinci_mcasp_dai[] = { .name = "davinci-mcasp.0", .playback = { .channels_min = 2, - .channels_max = 2, + .channels_max = 8, .rates = DAVINCI_MCASP_RATES, .formats = DAVINCI_MCASP_PCM_FMTS, }, .capture = { .channels_min = 2, - .channels_max = 2, + .channels_max = 8, .rates = DAVINCI_MCASP_RATES, .formats = DAVINCI_MCASP_PCM_FMTS, }, diff --git a/sound/soc/davinci/davinci-pcm.c b/sound/soc/davinci/davinci-pcm.c index bb57552..3af8b50 100644 --- a/sound/soc/davinci/davinci-pcm.c +++ b/sound/soc/davinci/davinci-pcm.c @@ -182,6 +182,7 @@ static void davinci_pcm_enqueue_dma(struct snd_pcm_substream *substream) unsigned short acnt; unsigned int count; unsigned int fifo_level; + unsigned char serializers = prtd->params->active_serializers;
period_size = snd_pcm_lib_period_bytes(substream); dma_offset = prtd->period * period_size; @@ -195,14 +196,14 @@ static void davinci_pcm_enqueue_dma(struct snd_pcm_substream *substream) data_type = prtd->params->data_type; count = period_size / data_type; if (fifo_level) - count /= fifo_level; + count /= fifo_level * serializers;
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { src = dma_pos; dst = prtd->params->dma_addr; src_bidx = data_type; - dst_bidx = 0; - src_cidx = data_type * fifo_level; + dst_bidx = 4; + src_cidx = data_type * fifo_level * serializers; dst_cidx = 0; } else { src = prtd->params->dma_addr; @@ -210,7 +211,7 @@ static void davinci_pcm_enqueue_dma(struct snd_pcm_substream *substream) src_bidx = 0; dst_bidx = data_type; src_cidx = 0; - dst_cidx = data_type * fifo_level; + dst_cidx = data_type * fifo_level * serializers; }
acnt = prtd->params->acnt; @@ -224,9 +225,10 @@ static void davinci_pcm_enqueue_dma(struct snd_pcm_substream *substream) edma_set_transfer_params(prtd->asp_link[0], acnt, count, 1, 0, ASYNC); else - edma_set_transfer_params(prtd->asp_link[0], acnt, fifo_level, - count, fifo_level, - ABSYNC); + edma_set_transfer_params(prtd->asp_link[0], acnt, + fifo_level * serializers, + count, fifo_level * serializers, + ABSYNC); }
static void davinci_pcm_dma_irq(unsigned link, u16 ch_status, void *data) diff --git a/sound/soc/davinci/davinci-pcm.h b/sound/soc/davinci/davinci-pcm.h index fbb710c..0d84d32 100644 --- a/sound/soc/davinci/davinci-pcm.h +++ b/sound/soc/davinci/davinci-pcm.h @@ -27,6 +27,7 @@ struct davinci_pcm_dma_params { unsigned char data_type; /* xfer data type */ unsigned char convert_mono_stereo; unsigned int fifo_level; + unsigned char active_serializers; /* num. of active audio serializers */ };
int davinci_soc_platform_register(struct device *dev);
Hi Michal,
On Wed, Feb 27, 2013 at 22:08:45, Michal Bachraty wrote:
Davinci McASP has support for I2S multichannel playback. For I2S playback/receive, each serializer is capable to play 2 channels (L/R) audio data.Serializer function (Playback-receive-none) is configured in DT, depending on hardware specification. It is possible to play less channels than configured in DT. For that purpose,only specific number of active serializers are enabled. McASP FIFO need to have DMA transfer Bcnt set to number of enabled serializers, otherwise no data are transfered to McASP and Alsa generates "DMA/IRQ playback write error (DMA or IRQ trouble?)" error.
Thanks for looking into this. Before going into details, a few generic comments. All serializers configured in Tx (or Rx) work off common clock generators and hence the serializers will be operating in sync. I assume the setup that you have matches this requirement. Based on the DMA programming assumed in the implementation the user needs to ensure that buffer has the data in the right format. Can you please describe the setup that you have and how you tested this?
Signed-off-by: Michal Bachraty michal.bachraty@streamunlimited.com
sound/soc/davinci/davinci-mcasp.c | 56 ++++++++++++++++++++++++++++++++----- sound/soc/davinci/davinci-pcm.c | 16 ++++++----- sound/soc/davinci/davinci-pcm.h | 1 + 3 files changed, 59 insertions(+), 14 deletions(-)
diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c index afef3fb..b84bb73 100644 --- a/sound/soc/davinci/davinci-mcasp.c +++ b/sound/soc/davinci/davinci-mcasp.c @@ -235,6 +235,10 @@ #define DISMOD (val)(val<<2) #define TXSTATE BIT(4) #define RXSTATE BIT(5) +#define SRMOD_MASK 3 +#define SRMOD_INACTIVE 0 +#define SRMOD_TX 1 +#define SRMOD_RX 2
I don't see SRMOD_TX/RX being used anywhere.
/*
- DAVINCI_MCASP_LBCTL_REG - Loop Back Control Register Bits
@@ -657,12 +661,15 @@ static int davinci_config_channel_size(struct davinci_audio_dev *dev, return 0; }
-static void davinci_hw_common_param(struct davinci_audio_dev *dev, int stream) +static int davinci_hw_common_param(struct davinci_audio_dev *dev, int stream,
int channels){ int i; u8 tx_ser = 0; u8 rx_ser = 0;
- u8 ser;
- u8 slots = dev->tdm_slots;
- u8 max_active_serializers = (channels + slots - 1) / slots; /* Default configuration */ mcasp_set_bits(dev->base + DAVINCI_MCASP_PWREMUMGT_REG, MCASP_SOFT);
@@ -680,16 +687,42 @@ static void davinci_hw_common_param(struct davinci_audio_dev *dev, int stream) }
for (i = 0; i < dev->num_serializer; i++) {
if (dev->serial_dir[i] == TX_MODE)tx_ser++;if (dev->serial_dir[i] == RX_MODE)rx_ser++;- }
- if (stream == SNDRV_PCM_STREAM_PLAYBACK)
ser = tx_ser;- else
ser = rx_ser;- if (ser < max_active_serializers) {
dev_warn(dev->dev, "stream has more channels (%d) than are ""enabled in mcasp (%d)\n", channels, ser * slots);return -EINVAL;- }
- tx_ser = 0;
- rx_ser = 0;
The number of active serializers is already being calculated below.
- for (i = 0; i < dev->num_serializer; i++) { mcasp_set_bits(dev->base + DAVINCI_MCASP_XRSRCTL_REG(i), dev->serial_dir[i]);
if (dev->serial_dir[i] == TX_MODE) {
if (dev->serial_dir[i] == TX_MODE &&tx_ser < max_active_serializers) { mcasp_set_bits(dev->base + DAVINCI_MCASP_PDIR_REG, AXR(i)); tx_ser++;
} else if (dev->serial_dir[i] == RX_MODE) {
} else if (dev->serial_dir[i] == RX_MODE &&rx_ser < max_active_serializers) { mcasp_clr_bits(dev->base + DAVINCI_MCASP_PDIR_REG, AXR(i)); rx_ser++;} else {mcasp_mod_bits(dev->base + DAVINCI_MCASP_XRSRCTL_REG(i),SRMOD_INACTIVE, SRMOD_MASK);
Sorry I don't follow what you are trying to do here.
}}
@@ -729,6 +762,8 @@ static void davinci_hw_common_param(struct davinci_audio_dev *dev, int stream) ((dev->rxnumevt * rx_ser) << 8), NUMEVT_MASK); } }
- return 0;
}
static void davinci_hw_param(struct davinci_audio_dev *dev, int stream) @@ -812,8 +847,14 @@ static int davinci_mcasp_hw_params(struct snd_pcm_substream *substream, &dev->dma_params[substream->stream]; int word_length; u8 fifo_level;
- u8 slots = dev->tdm_slots;
- int channels;
- struct snd_interval *pcm_channels = hw_param_interval(params,
SNDRV_PCM_HW_PARAM_CHANNELS);- channels = pcm_channels->min;
- davinci_hw_common_param(dev, substream->stream);
- if (davinci_hw_common_param(dev, substream->stream, channels))
return -EINVAL;@@ -862,6 +903,7 @@ static int davinci_mcasp_hw_params(struct snd_pcm_substream *substream, dma_params->acnt = dma_params->data_type;
dma_params->fifo_level = fifo_level;
dma_params->active_serializers = (channels + slots - 1) / slots; davinci_config_channel_size(dev, word_length);
return 0;
@@ -936,13 +978,13 @@ static struct snd_soc_dai_driver davinci_mcasp_dai[] = { .name = "davinci-mcasp.0", .playback = { .channels_min = 2,
.channels_max = 2,
.channels_max = 8,
Why are you setting this to 8?
.rates = DAVINCI_MCASP_RATES, .formats = DAVINCI_MCASP_PCM_FMTS, }, .capture = { .channels_min = 2,
.channels_max = 2,
.channels_max = 8,
Same here.
.rates = DAVINCI_MCASP_RATES, .formats = DAVINCI_MCASP_PCM_FMTS, },diff --git a/sound/soc/davinci/davinci-pcm.c b/sound/soc/davinci/davinci-pcm.c index bb57552..3af8b50 100644 --- a/sound/soc/davinci/davinci-pcm.c +++ b/sound/soc/davinci/davinci-pcm.c @@ -182,6 +182,7 @@ static void davinci_pcm_enqueue_dma(struct snd_pcm_substream *substream) unsigned short acnt; unsigned int count; unsigned int fifo_level;
- unsigned char serializers = prtd->params->active_serializers;
Can you please describe what DMA configuration you want? I think you can get rid of the active_serializers configuration by making use of tx_num_evt.
period_size = snd_pcm_lib_period_bytes(substream); dma_offset = prtd->period * period_size; @@ -195,14 +196,14 @@ static void davinci_pcm_enqueue_dma(struct snd_pcm_substream *substream) data_type = prtd->params->data_type; count = period_size / data_type; if (fifo_level)
count /= fifo_level;
count /= fifo_level * serializers;
I think there's a problem is the way in which tx_num_evt is interpreted in the current code. Instead of setting to fifo_level to tx_num_evt, if you set it to tx_num_evt * num_serializers you can get rid of the additional code to take care of the number of serializers.
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { src = dma_pos; dst = prtd->params->dma_addr; src_bidx = data_type;
dst_bidx = 0;src_cidx = data_type * fifo_level;
dst_bidx = 4;
Err.. this will most likely break other audio configurations. You should look at how to avoid this change by making use of the mask and rotation operations in the McASP code.
src_cidx = data_type * fifo_level * serializers;@@ -210,7 +211,7 @@ static void davinci_pcm_enqueue_dma(struct snd_pcm_substream *substream) src_bidx = 0; dst_bidx = data_type; src_cidx = 0;
dst_cidx = data_type * fifo_level;
dst_cidx = data_type * fifo_level * serializers;
With the change in fifo_level as described above, this won't be necessary.
}
acnt = prtd->params->acnt; @@ -224,9 +225,10 @@ static void davinci_pcm_enqueue_dma(struct snd_pcm_substream *substream) edma_set_transfer_params(prtd->asp_link[0], acnt, count, 1, 0, ASYNC); else
edma_set_transfer_params(prtd->asp_link[0], acnt, fifo_level,count, fifo_level,ABSYNC);
edma_set_transfer_params(prtd->asp_link[0], acnt,fifo_level * serializers,count, fifo_level * serializers,ABSYNC);
Same comment applies here.
Regards, Vaibhav
Hi Vaibhav,
Please look at [alsa-devel] [PATCH v3] davinci-mcasp: Add support for multichannel playback thread. There was some changes about computing number of channels. There is also described, how this driver works. Also please look at [alsa-devel] davici-mcasp: "tx-num-evt" confusion with number of serializers thread, where are discussed using "tx-num-evt".
I tested driver with 2 - 4 - 6 - 8 channnels I2S (2 slots TDM) with using 1 -2 -3 -4 serializers. and S16_LE (will do S24_3LE) audio.
Best, Michal
On Tuesday, March 05, 2013 11:06:16 Bedia, Vaibhav wrote:
Hi Michal,
On Wed, Feb 27, 2013 at 22:08:45, Michal Bachraty wrote:
Davinci McASP has support for I2S multichannel playback. For I2S playback/receive, each serializer is capable to play 2 channels (L/R) audio data.Serializer function (Playback-receive-none) is configured in DT, depending on hardware specification. It is possible to play less channels than configured in DT. For that purpose,only specific number of active serializers are enabled. McASP FIFO need to have DMA transfer Bcnt set to number of enabled serializers, otherwise no data are transfered to McASP and Alsa generates "DMA/IRQ playback write error (DMA or IRQ trouble?)" error.
Thanks for looking into this. Before going into details, a few generic comments. All serializers configured in Tx (or Rx) work off common clock generators and hence the serializers will be operating in sync. I assume the setup that you have matches this requirement. Based on the DMA programming assumed in the implementation the user needs to ensure that buffer has the data in the right format. Can you please describe the setup that you have and how you tested this?
Signed-off-by: Michal Bachraty michal.bachraty@streamunlimited.com
sound/soc/davinci/davinci-mcasp.c | 56 ++++++++++++++++++++++++++++++++----- sound/soc/davinci/davinci-pcm.c | 16 ++++++----- sound/soc/davinci/davinci-pcm.h | 1 + 3 files changed, 59 insertions(+), 14 deletions(-)
diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c index afef3fb..b84bb73 100644 --- a/sound/soc/davinci/davinci-mcasp.c +++ b/sound/soc/davinci/davinci-mcasp.c @@ -235,6 +235,10 @@
#define DISMOD (val)(val<<2) #define TXSTATE BIT(4) #define RXSTATE BIT(5)
+#define SRMOD_MASK 3 +#define SRMOD_INACTIVE 0 +#define SRMOD_TX 1 +#define SRMOD_RX 2
I don't see SRMOD_TX/RX being used anywhere.
/*
- DAVINCI_MCASP_LBCTL_REG - Loop Back Control Register Bits
@@ -657,12 +661,15 @@ static int davinci_config_channel_size(struct davinci_audio_dev *dev,> return 0;
}
-static void davinci_hw_common_param(struct davinci_audio_dev *dev, int stream) +static int davinci_hw_common_param(struct davinci_audio_dev *dev, int stream, + int channels)
{
int i; u8 tx_ser = 0; u8 rx_ser = 0;
u8 ser;
u8 slots = dev->tdm_slots;
u8 max_active_serializers = (channels + slots - 1) / slots;
/* Default configuration */ mcasp_set_bits(dev->base + DAVINCI_MCASP_PWREMUMGT_REG, MCASP_SOFT);
@@ -680,16 +687,42 @@ static void davinci_hw_common_param(struct davinci_audio_dev *dev, int stream)> }
for (i = 0; i < dev->num_serializer; i++) {
if (dev->serial_dir[i] == TX_MODE)tx_ser++;if (dev->serial_dir[i] == RX_MODE)rx_ser++;- }
- if (stream == SNDRV_PCM_STREAM_PLAYBACK)
ser = tx_ser;- else
ser = rx_ser;- if (ser < max_active_serializers) {
dev_warn(dev->dev, "stream has more channels (%d) than are ""enabled in mcasp (%d)\n", channels, ser * slots);return -EINVAL;- }
- tx_ser = 0;
- rx_ser = 0;
The number of active serializers is already being calculated below.
for (i = 0; i < dev->num_serializer; i++) {
mcasp_set_bits(dev->base + DAVINCI_MCASP_XRSRCTL_REG(i),
dev->serial_dir[i]);
if (dev->serial_dir[i] == TX_MODE) {
if (dev->serial_dir[i] == TX_MODE &&tx_ser < max_active_serializers) { mcasp_set_bits(dev->base + DAVINCI_MCASP_PDIR_REG,AXR(i)); tx_ser++;
} else if (dev->serial_dir[i] == RX_MODE) {
} else if (dev->serial_dir[i] == RX_MODE &&rx_ser < max_active_serializers) { mcasp_clr_bits(dev->base + DAVINCI_MCASP_PDIR_REG,AXR(i)); rx_ser++;
} else {mcasp_mod_bits(dev->base + DAVINCI_MCASP_XRSRCTL_REG(i),SRMOD_INACTIVE, SRMOD_MASK);Sorry I don't follow what you are trying to do here.
}}
@@ -729,6 +762,8 @@ static void davinci_hw_common_param(struct davinci_audio_dev *dev, int stream)> ((dev->rxnumevt * rx_ser) << 8), NUMEVT_MASK); }
}
- return 0;
}
static void davinci_hw_param(struct davinci_audio_dev *dev, int stream)
@@ -812,8 +847,14 @@ static int davinci_mcasp_hw_params(struct snd_pcm_substream *substream,> &dev->dma_params[substream->stream];
int word_length; u8 fifo_level;
- u8 slots = dev->tdm_slots;
- int channels;
- struct snd_interval *pcm_channels = hw_param_interval(params,
SNDRV_PCM_HW_PARAM_CHANNELS);- channels = pcm_channels->min;
- davinci_hw_common_param(dev, substream->stream);
if (davinci_hw_common_param(dev, substream->stream, channels))
return -EINVAL;if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
fifo_level = dev->txnumevt;
else
@@ -862,6 +903,7 @@ static int davinci_mcasp_hw_params(struct snd_pcm_substream *substream,> dma_params->acnt = dma_params->data_type;
dma_params->fifo_level = fifo_level;
dma_params->active_serializers = (channels + slots - 1) / slots;
davinci_config_channel_size(dev, word_length);
return 0;
@@ -936,13 +978,13 @@ static struct snd_soc_dai_driver davinci_mcasp_dai[] = {> .name = "davinci-mcasp.0", .playback = { .channels_min = 2,
.channels_max = 2,
.channels_max = 8,Why are you setting this to 8?
.rates = DAVINCI_MCASP_RATES, .formats = DAVINCI_MCASP_PCM_FMTS,}, .capture = { .channels_min = 2,
.channels_max = 2,
.channels_max = 8,Same here.
.rates = DAVINCI_MCASP_RATES, .formats = DAVINCI_MCASP_PCM_FMTS,},
diff --git a/sound/soc/davinci/davinci-pcm.c b/sound/soc/davinci/davinci-pcm.c index bb57552..3af8b50 100644 --- a/sound/soc/davinci/davinci-pcm.c +++ b/sound/soc/davinci/davinci-pcm.c @@ -182,6 +182,7 @@ static void davinci_pcm_enqueue_dma(struct snd_pcm_substream *substream)> unsigned short acnt; unsigned int count; unsigned int fifo_level;
- unsigned char serializers = prtd->params->active_serializers;
Can you please describe what DMA configuration you want? I think you can get rid of the active_serializers configuration by making use of tx_num_evt.
period_size = snd_pcm_lib_period_bytes(substream); dma_offset = prtd->period * period_size;
@@ -195,14 +196,14 @@ static void davinci_pcm_enqueue_dma(struct snd_pcm_substream *substream)> data_type = prtd->params->data_type; count = period_size / data_type; if (fifo_level)
count /= fifo_level;
count /= fifo_level * serializers;I think there's a problem is the way in which tx_num_evt is interpreted in the current code. Instead of setting to fifo_level to tx_num_evt, if you set it to tx_num_evt * num_serializers you can get rid of the additional code to take care of the number of serializers.
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
src = dma_pos; dst = prtd->params->dma_addr; src_bidx = data_type;
dst_bidx = 0;src_cidx = data_type * fifo_level;
dst_bidx = 4;Err.. this will most likely break other audio configurations. You should look at how to avoid this change by making use of the mask and rotation operations in the McASP code.
src_cidx = data_type * fifo_level * serializers;dst_cidx = 0;
} else {
src = prtd->params->dma_addr;
@@ -210,7 +211,7 @@ static void davinci_pcm_enqueue_dma(struct snd_pcm_substream *substream)> src_bidx = 0; dst_bidx = data_type; src_cidx = 0;
dst_cidx = data_type * fifo_level;
dst_cidx = data_type * fifo_level * serializers;With the change in fifo_level as described above, this won't be necessary.
}
acnt = prtd->params->acnt;
@@ -224,9 +225,10 @@ static void davinci_pcm_enqueue_dma(struct snd_pcm_substream *substream)> edma_set_transfer_params(prtd->asp_link[0], acnt, count, 1, 0, ASYNC);
else
edma_set_transfer_params(prtd->asp_link[0], acnt, fifo_level,count, fifo_level,ABSYNC);
edma_set_transfer_params(prtd->asp_link[0], acnt,fifo_level * serializers,count, fifo_level * serializers,ABSYNC);Same comment applies here.
Regards, Vaibhav
On Tue, Mar 05, 2013 at 17:11:20, Michal Bachraty wrote:
Hi Vaibhav,
Please look at [alsa-devel] [PATCH v3] davinci-mcasp: Add support for multichannel playback thread. There was some changes about computing number of channels. There is also described, how this driver works.
Oops. I somehow missed v3 :(
I just had a look at it and most the comments that I have on v2 are still applicable and IMO need to be addressed before getting merged. You also don't have a change-log in v3 so I don't understand why you made some changes.
Also please look at [alsa-devel] davici-mcasp: "tx-num-evt" confusion with number of serializers thread, where are discussed using "tx-num-evt".
Just noticed this one. Will respond to it.
I tested driver with 2 - 4 - 6 - 8 channnels I2S (2 slots TDM) with using 1 -2 -3 -4 serializers. and S16_LE (will do S24_3LE) audio.
Did you use aplay for this? If so, what were the parameters used. If not, can you describe how the userspace needs to manage the audio buffer? For anyone looking at leveraging the multi-serializer feature this information is important to highlight.
Regards, Vaibhav
On Tuesday, March 05, 2013 12:03:45 Bedia, Vaibhav wrote:
I just had a look at it and most the comments that I have on v2 are still applicable and IMO need to be addressed before getting merged. You also don't have a change-log in v3 so I don't understand why you made some changes.
I found that, there is need to upgrade for other TDM slots configs.
I tested driver with 2 - 4 - 6 - 8 channnels I2S (2 slots TDM) with using 1 -2 -3 -4 serializers. and S16_LE (will do S24_3LE) audio.
Did you use aplay for this? If so, what were the parameters used. If not, can you describe how the userspace needs to manage the audio buffer? For anyone looking at leveraging the multi-serializer feature this information is important to highlight.
Yes aplay -D "hw:1,0" -r48000 -fS16_LE -c6 < sine16_6ch.txt or similar. I hear 6ch audio on TAS5086. Also I had generated data and I saw data on oscilloscope. This patch only exteds driver for using more serializers. When you run 2ch. or one serializer), there is same configuration as was in previous driver version.
Best, Michal
Hi Vaibhav, Hi Michal,
On 05.03.2013 12:06, Bedia, Vaibhav wrote:
On Wed, Feb 27, 2013 at 22:08:45, Michal Bachraty wrote:
Davinci McASP has support for I2S multichannel playback. For I2S playback/receive, each serializer is capable to play 2 channels (L/R) audio data.Serializer function (Playback-receive-none) is configured in DT, depending on hardware specification. It is possible to play less channels than configured in DT. For that purpose,only specific number of active serializers are enabled. McASP FIFO need to have DMA transfer Bcnt set to number of enabled serializers, otherwise no data are transfered to McASP and Alsa generates "DMA/IRQ playback write error (DMA or IRQ trouble?)" error.
Thanks for looking into this. Before going into details, a few generic comments.
To explain: Michal and me are working on the same hardware - a custom made AM33xx board which has a version with multichannel output.
All serializers configured in Tx (or Rx) work off common clock generators and hence the serializers will be operating in sync. I assume the setup that you have matches this requirement. Based on the DMA programming assumed in the implementation the user needs to ensure that buffer has the data in the right format. Can you please describe the setup that you have and how you tested this?
As Michal described, we used a board with a multichannel Codec on it, which connects 3 of its I2S inputs to the AM33xx's AXR data lines.
Software wise, we tested with 'aplay -cX', and that seems to work just fine.
Signed-off-by: Michal Bachraty michal.bachraty@streamunlimited.com
sound/soc/davinci/davinci-mcasp.c | 56 ++++++++++++++++++++++++++++++++----- sound/soc/davinci/davinci-pcm.c | 16 ++++++----- sound/soc/davinci/davinci-pcm.h | 1 + 3 files changed, 59 insertions(+), 14 deletions(-)
diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c index afef3fb..b84bb73 100644 --- a/sound/soc/davinci/davinci-mcasp.c +++ b/sound/soc/davinci/davinci-mcasp.c @@ -235,6 +235,10 @@ #define DISMOD (val)(val<<2) #define TXSTATE BIT(4) #define RXSTATE BIT(5) +#define SRMOD_MASK 3 +#define SRMOD_INACTIVE 0 +#define SRMOD_TX 1 +#define SRMOD_RX 2
I don't see SRMOD_TX/RX being used anywhere.
That's true, they can be dropped.
/*
- DAVINCI_MCASP_LBCTL_REG - Loop Back Control Register Bits
@@ -657,12 +661,15 @@ static int davinci_config_channel_size(struct davinci_audio_dev *dev, return 0; }
-static void davinci_hw_common_param(struct davinci_audio_dev *dev, int stream) +static int davinci_hw_common_param(struct davinci_audio_dev *dev, int stream,
int channels){ int i; u8 tx_ser = 0; u8 rx_ser = 0;
- u8 ser;
- u8 slots = dev->tdm_slots;
- u8 max_active_serializers = (channels + slots - 1) / slots; /* Default configuration */ mcasp_set_bits(dev->base + DAVINCI_MCASP_PWREMUMGT_REG, MCASP_SOFT);
@@ -680,16 +687,42 @@ static void davinci_hw_common_param(struct davinci_audio_dev *dev, int stream) }
for (i = 0; i < dev->num_serializer; i++) {
if (dev->serial_dir[i] == TX_MODE)tx_ser++;if (dev->serial_dir[i] == RX_MODE)rx_ser++;- }
- if (stream == SNDRV_PCM_STREAM_PLAYBACK)
ser = tx_ser;- else
ser = rx_ser;- if (ser < max_active_serializers) {
dev_warn(dev->dev, "stream has more channels (%d) than are ""enabled in mcasp (%d)\n", channels, ser * slots);return -EINVAL;- }
- tx_ser = 0;
- rx_ser = 0;
The number of active serializers is already being calculated below.
True. The code can be shifted around so the calculation is only done once.
- for (i = 0; i < dev->num_serializer; i++) { mcasp_set_bits(dev->base + DAVINCI_MCASP_XRSRCTL_REG(i), dev->serial_dir[i]);
if (dev->serial_dir[i] == TX_MODE) {
if (dev->serial_dir[i] == TX_MODE &&tx_ser < max_active_serializers) { mcasp_set_bits(dev->base + DAVINCI_MCASP_PDIR_REG, AXR(i)); tx_ser++;
} else if (dev->serial_dir[i] == RX_MODE) {
} else if (dev->serial_dir[i] == RX_MODE &&rx_ser < max_active_serializers) { mcasp_clr_bits(dev->base + DAVINCI_MCASP_PDIR_REG, AXR(i)); rx_ser++;} else {mcasp_mod_bits(dev->base + DAVINCI_MCASP_XRSRCTL_REG(i),SRMOD_INACTIVE, SRMOD_MASK);Sorry I don't follow what you are trying to do here.
}}
@@ -729,6 +762,8 @@ static void davinci_hw_common_param(struct davinci_audio_dev *dev, int stream) ((dev->rxnumevt * rx_ser) << 8), NUMEVT_MASK); } }
- return 0;
}
static void davinci_hw_param(struct davinci_audio_dev *dev, int stream) @@ -812,8 +847,14 @@ static int davinci_mcasp_hw_params(struct snd_pcm_substream *substream, &dev->dma_params[substream->stream]; int word_length; u8 fifo_level;
- u8 slots = dev->tdm_slots;
- int channels;
- struct snd_interval *pcm_channels = hw_param_interval(params,
SNDRV_PCM_HW_PARAM_CHANNELS);- channels = pcm_channels->min;
- davinci_hw_common_param(dev, substream->stream);
- if (davinci_hw_common_param(dev, substream->stream, channels))
return -EINVAL;@@ -862,6 +903,7 @@ static int davinci_mcasp_hw_params(struct snd_pcm_substream *substream, dma_params->acnt = dma_params->data_type;
dma_params->fifo_level = fifo_level;
dma_params->active_serializers = (channels + slots - 1) / slots; davinci_config_channel_size(dev, word_length);
return 0;
@@ -936,13 +978,13 @@ static struct snd_soc_dai_driver davinci_mcasp_dai[] = { .name = "davinci-mcasp.0", .playback = { .channels_min = 2,
.channels_max = 2,
.channels_max = 8,Why are you setting this to 8?
Well, the ASoC core will look at this field when parsing the dai links, and will build a sound card that has min(codec_dai->channels_max, cpu_dai->channels_max) channels. Hence, this number has to reflect the maximum possible output channels for this DAI. In v3, it's actually set to 512. Or was that not your question?
I'll let Michal comment on the rest, as it's his work. Note though that v3 of this patch has already been applied to Mark's tree, but we can easily fix the details you mentioned with another patch.
Thanks a lot for your thorough review!
Daniel
.rates = DAVINCI_MCASP_RATES, .formats = DAVINCI_MCASP_PCM_FMTS, }, .capture = { .channels_min = 2,
.channels_max = 2,
.channels_max = 8,Same here.
.rates = DAVINCI_MCASP_RATES, .formats = DAVINCI_MCASP_PCM_FMTS, },diff --git a/sound/soc/davinci/davinci-pcm.c b/sound/soc/davinci/davinci-pcm.c index bb57552..3af8b50 100644 --- a/sound/soc/davinci/davinci-pcm.c +++ b/sound/soc/davinci/davinci-pcm.c @@ -182,6 +182,7 @@ static void davinci_pcm_enqueue_dma(struct snd_pcm_substream *substream) unsigned short acnt; unsigned int count; unsigned int fifo_level;
- unsigned char serializers = prtd->params->active_serializers;
Can you please describe what DMA configuration you want? I think you can get rid of the active_serializers configuration by making use of tx_num_evt.
period_size = snd_pcm_lib_period_bytes(substream); dma_offset = prtd->period * period_size; @@ -195,14 +196,14 @@ static void davinci_pcm_enqueue_dma(struct snd_pcm_substream *substream) data_type = prtd->params->data_type; count = period_size / data_type; if (fifo_level)
count /= fifo_level;
count /= fifo_level * serializers;I think there's a problem is the way in which tx_num_evt is interpreted in the current code. Instead of setting to fifo_level to tx_num_evt, if you set it to tx_num_evt * num_serializers you can get rid of the additional code to take care of the number of serializers.
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { src = dma_pos; dst = prtd->params->dma_addr; src_bidx = data_type;
dst_bidx = 0;src_cidx = data_type * fifo_level;
dst_bidx = 4;Err.. this will most likely break other audio configurations. You should look at how to avoid this change by making use of the mask and rotation operations in the McASP code.
src_cidx = data_type * fifo_level * serializers;@@ -210,7 +211,7 @@ static void davinci_pcm_enqueue_dma(struct snd_pcm_substream *substream) src_bidx = 0; dst_bidx = data_type; src_cidx = 0;
dst_cidx = data_type * fifo_level;
dst_cidx = data_type * fifo_level * serializers;With the change in fifo_level as described above, this won't be necessary.
}
acnt = prtd->params->acnt; @@ -224,9 +225,10 @@ static void davinci_pcm_enqueue_dma(struct snd_pcm_substream *substream) edma_set_transfer_params(prtd->asp_link[0], acnt, count, 1, 0, ASYNC); else
edma_set_transfer_params(prtd->asp_link[0], acnt, fifo_level,count, fifo_level,ABSYNC);
edma_set_transfer_params(prtd->asp_link[0], acnt,fifo_level * serializers,count, fifo_level * serializers,ABSYNC);Same comment applies here.
Regards, Vaibhav
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Hi Daniel,
On Tue, Mar 05, 2013 at 21:31:59, Daniel Mack wrote: [...]
As Michal described, we used a board with a multichannel Codec on it, which connects 3 of its I2S inputs to the AM33xx's AXR data lines.
Software wise, we tested with 'aplay -cX', and that seems to work just fine.
Since you are using a multichannel codec things are a lot simplified for you :) Someone might want to hook up multiple codecs to get multi-channel behavior. There will be only 1 CPU DAI but there can be upto 16 CODEC DAIs operating in sync. I haven't really followed the recent ASoC changes so I don't know whether something like this can be handled right now.
Mark, Liam,
Any suggestions?
[...]
return 0; @@ -936,13 +978,13 @@ static struct snd_soc_dai_driver davinci_mcasp_dai[] = { .name = "davinci-mcasp.0", .playback = { .channels_min = 2,
.channels_max = 2,
.channels_max = 8,Why are you setting this to 8?
Well, the ASoC core will look at this field when parsing the dai links, and will build a sound card that has min(codec_dai->channels_max, cpu_dai->channels_max) channels. Hence, this number has to reflect the maximum possible output channels for this DAI. In v3, it's actually set to 512. Or was that not your question?
32 slots on a max of 16 serializers gives the max as 512 and hence I wanted to know why 8 was selected.
Regards, Vaibhav
On Wed, Mar 06, 2013 at 10:54:52AM +0000, Bedia, Vaibhav wrote:
Since you are using a multichannel codec things are a lot simplified for you :) Someone might want to hook up multiple codecs to get multi-channel behavior. There will be only 1 CPU DAI but there can be upto 16 CODEC DAIs operating in sync. I haven't really followed the recent ASoC changes so I don't know whether something like this can be handled right now.
Mark, Liam,
Any suggestions?
Nobody with any interest in such systems has stepped up to work on the framework, and of course relatively little CPU hardware supports multiple streams on a single DAI.
On Wed, Mar 06, 2013 at 16:27:15, Mark Brown wrote:
On Wed, Mar 06, 2013 at 10:54:52AM +0000, Bedia, Vaibhav wrote:
Since you are using a multichannel codec things are a lot simplified for you :) Someone might want to hook up multiple codecs to get multi-channel behavior. There will be only 1 CPU DAI but there can be upto 16 CODEC DAIs operating in sync. I haven't really followed the recent ASoC changes so I don't know whether something like this can be handled right now.
Mark, Liam,
Any suggestions?
Nobody with any interest in such systems has stepped up to work on the framework, and of course relatively little CPU hardware supports multiple streams on a single DAI.
Ok. Thinking more on this I guess the situation I described above will indeed be a rare case. There would be additional complexity in ensuring that the codecs are always operating in sync. It would perhaps be simpler to cascade the codecs (if supported) and use only one interface to transfer the data. Or just use multichannel codec like Michal and Daniel are doing.
Regards, Vaibhav
On 03/06/2013 12:19 PM, Bedia, Vaibhav wrote:
On Wed, Mar 06, 2013 at 16:27:15, Mark Brown wrote:
On Wed, Mar 06, 2013 at 10:54:52AM +0000, Bedia, Vaibhav wrote:
Since you are using a multichannel codec things are a lot simplified for you :) Someone might want to hook up multiple codecs to get multi-channel behavior. There will be only 1 CPU DAI but there can be upto 16 CODEC DAIs operating in sync. I haven't really followed the recent ASoC changes so I don't know whether something like this can be handled right now.
Mark, Liam,
Any suggestions?
Nobody with any interest in such systems has stepped up to work on the framework, and of course relatively little CPU hardware supports multiple streams on a single DAI.
Ok. Thinking more on this I guess the situation I described above will indeed be a rare case. There would be additional complexity in ensuring that the codecs are always operating in sync. It would perhaps be simpler to cascade the codecs (if supported) and use only one interface to transfer the data. Or just use multichannel codec like Michal and Daniel are doing.
I have 9 codecs connected, and the main problem to solve was to get the system to start ALL of them instead of just one.
I solved that by creating a "multiplex" codec, which in itself doesn't do anything. It exports a mixer where you can switch on/off the codecs you want to use, and it forwards all commands (_ops) to the codecs that are enabled. The serializer settings are sent to the asp driver as if they are TDM masks (using the bits to enable serializers instead).
In userland you see 10 codecs, you can use each separately, or use the 10th codec (the multiplexer) to use any combination of other codecs.
On Fri, Mar 08, 2013 at 12:59:49, Mike Looijmans wrote:
I have 9 codecs connected, and the main problem to solve was to get the system to start ALL of them instead of just one.
I solved that by creating a "multiplex" codec, which in itself doesn't do anything. It exports a mixer where you can switch on/off the codecs you want to use, and it forwards all commands (_ops) to the codecs that are enabled. The serializer settings are sent to the asp driver as if they are TDM masks (using the bits to enable serializers instead).
In userland you see 10 codecs, you can use each separately, or use the 10th codec (the multiplexer) to use any combination of other codecs.
Interesting approach. I was thinking more on the lines of how to expose the different serializer-codec pairs to the core. Since there's only one DMA interface for the different serializers doing this in a generic manner looked complicated, at least to me.
I am aware of couple of multi-channel systems that preferred the approach that I mentioned. In one, a cascading of codecs was preferred so as to free up the extra serializer pads for different functions at the SoC level. There was a custom app to split the data coming in from different channels but since it freed up a few pads, this approach was preferred. In another, sending SPDIF data to an HDMI transmitter was preferred, again to free up some pads. It finally depends on what the use-case is and which system level constraints are more important to satisfy.
It would be good if you could post the changes that made so that we can discuss on how the approach in detail and possibly enhance the core to handle the multi-codec scenarios that use-cases like your require.
Regards, Vaibhav
On 03/11/2013 08:01 AM, Bedia, Vaibhav wrote:
On Fri, Mar 08, 2013 at 12:59:49, Mike Looijmans wrote:
I have 9 codecs connected, and the main problem to solve was to get the system to start ALL of them instead of just one.
I solved that by creating a "multiplex" codec, which in itself doesn't do anything. It exports a mixer where you can switch on/off the codecs you want to use, and it forwards all commands (_ops) to the codecs that are enabled. The serializer settings are sent to the asp driver as if they are TDM masks (using the bits to enable serializers instead).
In userland you see 10 codecs, you can use each separately, or use the 10th codec (the multiplexer) to use any combination of other codecs.
Interesting approach. I was thinking more on the lines of how to expose the different serializer-codec pairs to the core. Since there's only one DMA interface for the different serializers doing this in a generic manner looked complicated, at least to me.
I am aware of couple of multi-channel systems that preferred the approach that I mentioned. In one, a cascading of codecs was preferred so as to free up the extra serializer pads for different functions at the SoC level. There was a custom app to split the data coming in from different channels but since it freed up a few pads, this approach was preferred. In another, sending SPDIF data to an HDMI transmitter was preferred, again to free up some pads. It finally depends on what the use-case is and which system level constraints are more important to satisfy.
It would be good if you could post the changes that made so that we can discuss on how the approach in detail and possibly enhance the core to handle the multi-codec scenarios that use-cases like your require.
I'll post the code, but it's based on a 2.6.37 kernel and it's tailored to "my" platform, with numerous changes to clock settings and such. So don't expect it to be useful. It's only interesting as an "example" or for illustrating the general idea, but entirely useless for inclusion in mainstream.
On Mon, Mar 11, 2013 at 14:03:50, Mike Looijmans wrote:
I'll post the code, but it's based on a 2.6.37 kernel and it's tailored to "my" platform, with numerous changes to clock settings and such. So don't expect it to be useful. It's only interesting as an "example" or for illustrating the general idea, but entirely useless for inclusion in mainstream.
Thanks. In my opinion any proof of concept will be good to get some discussion going on this topic.
Regards, Vaibhav
If two codecs need each other for certain operations, is there a way to define DAPM links between them?
For example, codec A has an analog input "x". It can route this input via some output pin to an input of codec B. (In other words, A could be a codec that provides an SPDIF output and B could be a codec that can only handle digital inputs like the SPDIF from codec A).
Now codec B can record from input "x", but to do so, it would have to activate a path leading through codec A. (hence, codec A will need to be a bit active)
I have a suspicion that this is possible by hand-crafting some of the dapm structs and linking them together. Before I try, I would like to know if there's even a possiblity that this will work from folks more knowledgable on this topic.
My current implementation is to tell DAPM that i have an "always enabled" ADC path and just do everything myself. Which is of course wasteful.
Mike.
On 03/11/2013 09:56 AM, Bedia, Vaibhav wrote:
On Mon, Mar 11, 2013 at 14:03:50, Mike Looijmans wrote:
I'll post the code, but it's based on a 2.6.37 kernel and it's tailored to "my" platform, with numerous changes to clock settings and such. So don't expect it to be useful. It's only interesting as an "example" or for illustrating the general idea, but entirely useless for inclusion in mainstream.
Thanks. In my opinion any proof of concept will be good to get some discussion going on this topic.
It has been way to long since I last replied, and there's still much to do.
Anyway, I attached the "multiplex" codec source code. The multiplex assumes that all its child codecs share the same (cpu) dai and that the dai is capable of handling multiple codecs (either using TDM or multiple pins, or whatever combination). The machine part must take care of forwarding the "mask" setting of the multiplexer and translate it to something the CPU DAI understands. (If the codecs do not share the DAI, you don't need the multiplexer...)
The multiplex only supports capture.
To use it, compile it in (change Kconfig) and have the board file register codecs with the multiplexer. The multiplexer creates a mixer switch control for each codec. To use a set of codecs, set their switch to "on" (which is default) and record from the multiplex codec. The multiplex will control the codecs.
Codecs can also be used without the multiplex in between, no change there.
See it as a proof of concept. It works fine for me...
Mike.
On 06.03.2013 11:54, Bedia, Vaibhav wrote:
Hi Daniel,
On Tue, Mar 05, 2013 at 21:31:59, Daniel Mack wrote: [...]
As Michal described, we used a board with a multichannel Codec on it, which connects 3 of its I2S inputs to the AM33xx's AXR data lines.
Software wise, we tested with 'aplay -cX', and that seems to work just fine.
Since you are using a multichannel codec things are a lot simplified for you :) Someone might want to hook up multiple codecs to get multi-channel behavior. There will be only 1 CPU DAI but there can be upto 16 CODEC DAIs operating in sync. I haven't really followed the recent ASoC changes so I don't know whether something like this can be handled right now.
I would rather instanciate multiple McASP cores for such setups, so you can (in theory) also make them operate on different bit depths and clock dividers. You think that's possible as well?
Thanks, Daniel
On Wed, Mar 06, 2013 at 16:30:48, Daniel Mack wrote:
On 06.03.2013 11:54, Bedia, Vaibhav wrote:
Hi Daniel,
On Tue, Mar 05, 2013 at 21:31:59, Daniel Mack wrote: [...]
As Michal described, we used a board with a multichannel Codec on it, which connects 3 of its I2S inputs to the AM33xx's AXR data lines.
Software wise, we tested with 'aplay -cX', and that seems to work just fine.
Since you are using a multichannel codec things are a lot simplified for you :) Someone might want to hook up multiple codecs to get multi-channel behavior. There will be only 1 CPU DAI but there can be upto 16 CODEC DAIs operating in sync. I haven't really followed the recent ASoC changes so I don't know whether something like this can be handled right now.
I would rather instanciate multiple McASP cores for such setups, so you can (in theory) also make them operate on different bit depths and clock dividers. You think that's possible as well?
Yeah. Using multiple McASPs would be much simpler to handle both in s/w and h/w.
Regards, Vaibhav
On 03/06/2013 12:19 PM, Bedia, Vaibhav wrote:
On Wed, Mar 06, 2013 at 16:30:48, Daniel Mack wrote:
On 06.03.2013 11:54, Bedia, Vaibhav wrote:
Hi Daniel,
On Tue, Mar 05, 2013 at 21:31:59, Daniel Mack wrote: [...]
As Michal described, we used a board with a multichannel Codec on it, which connects 3 of its I2S inputs to the AM33xx's AXR data lines.
Software wise, we tested with 'aplay -cX', and that seems to work just fine.
Since you are using a multichannel codec things are a lot simplified for you :) Someone might want to hook up multiple codecs to get multi-channel behavior. There will be only 1 CPU DAI but there can be upto 16 CODEC DAIs operating in sync. I haven't really followed the recent ASoC changes so I don't know whether something like this can be handled right now.
I would rather instanciate multiple McASP cores for such setups, so you can (in theory) also make them operate on different bit depths and clock dividers. You think that's possible as well?
Yeah. Using multiple McASPs would be much simpler to handle both in s/w and h/w.
Would it?
The McASP has multiple serializers, but they all share the same clocks and format settings. The data will come in as "L1 L2 L3 .. R1 R2 R3 .." so the code would have keep track of which channels have to go where, and rearrange it to the clients. And it must be able to dynamically add/remove channels without disturbing the other streams.
So unless you want to physically add more McASP hardware to the system, I don't see how this would be "simpler" to handle than other options.
A way to "group" the codecs together is much simpler to implement (which is simply proven by the simple fact that I implemented it on my system) and it makes it very easy to control on the user side too - the selection of codecs is just a mixer control.
Mike.
On Fri, Mar 08, 2013 at 19:52:09, Mike Looijmans wrote:
On 03/06/2013 12:19 PM, Bedia, Vaibhav wrote:
On Wed, Mar 06, 2013 at 16:30:48, Daniel Mack wrote:
On 06.03.2013 11:54, Bedia, Vaibhav wrote:
Hi Daniel,
On Tue, Mar 05, 2013 at 21:31:59, Daniel Mack wrote: [...]
As Michal described, we used a board with a multichannel Codec on it, which connects 3 of its I2S inputs to the AM33xx's AXR data lines.
Software wise, we tested with 'aplay -cX', and that seems to work just fine.
Since you are using a multichannel codec things are a lot simplified for you :) Someone might want to hook up multiple codecs to get multi-channel behavior. There will be only 1 CPU DAI but there can be upto 16 CODEC DAIs operating in sync. I haven't really followed the recent ASoC changes so I don't know whether something like this can be handled right now.
I would rather instanciate multiple McASP cores for such setups, so you can (in theory) also make them operate on different bit depths and clock dividers. You think that's possible as well?
Yeah. Using multiple McASPs would be much simpler to handle both in s/w and h/w.
Would it?
The McASP has multiple serializers, but they all share the same clocks and format settings. The data will come in as "L1 L2 L3 .. R1 R2 R3 .." so the code would have keep track of which channels have to go where, and rearrange it to the clients. And it must be able to dynamically add/remove channels without disturbing the other streams.
I don't see how the grouping of codecs like you are mentioning helps add/remove channels while capture/playback in on-going. By implementing a dummy codec you can select which serializers are enabled/disabled but won't the selection have to be be before starting any streaming since you can't update the DMA controller configuration on the fly. Or maybe that's what you meant by add/remove channels without disturbing the other streams.
So unless you want to physically add more McASP hardware to the system, I don't see how this would be "simpler" to handle than other options.
A way to "group" the codecs together is much simpler to implement (which is simply proven by the simple fact that I implemented it on my system) and it makes it very easy to control on the user side too - the selection of codecs is just a mixer control.
It's good that you have working a implementation. Like I mentioned in the other thread it would be good if you can post the changes that you did so that the approach can be discussed in detail, obviously assuming you want something like to be handled in mainline code.
Regards, Vaibhav
Hi Vaibhav,
return 0;
@@ -936,13 +978,13 @@ static struct snd_soc_dai_driver davinci_mcasp_dai[] = {> >> .name = "davinci-mcasp.0", .playback = { .channels_min = 2,
.channels_max = 2,
.channels_max = 8,Why are you setting this to 8?
Well, the ASoC core will look at this field when parsing the dai links, and will build a sound card that has min(codec_dai->channels_max, cpu_dai->channels_max) channels. Hence, this number has to reflect the maximum possible output channels for this DAI. In v3, it's actually set to 512. Or was that not your question?
32 slots on a max of 16 serializers gives the max as 512 and hence I wanted to know why 8 was selected.
That number was changed in patch v3 to 512. 8 was selected only for Am335 and 2TDM slots. That number was not general and therefore it was changed. It is no need for discusion for that number, because it was changed to proper value in v3 of this patch. If not, let us know.
Michal.
On Wed, Mar 06, 2013 at 19:57:35, Michal Bachraty wrote: [...]
That number was changed in patch v3 to 512. 8 was selected only for Am335 and 2TDM slots. That number was not general and therefore it was changed. It is no need for discusion for that number, because it was changed to proper value in v3 of this patch. If not, let us know.
Yeah, that number is fine now. You can ignore this comment.
Regards, Vaibhav
Hi Vaibhav, Hi Daniel.
Daniel, thaks for comments, Vaibhav thanks for review! Please On Tuesday, March 05, 2013 17:01:59 Daniel Mack wrote:
I'll let Michal comment on the rest, as it's his work. Note though that v3 of this patch has already been applied to Mark's tree, but we can easily fix the details you mentioned with another patch.
This is part for me, so:
.rates = DAVINCI_MCASP_RATES, .formats = DAVINCI_MCASP_PCM_FMTS,}, .capture = { .channels_min = 2,
.channels_max = 2,
.channels_max = 8,Same here.
.rates = DAVINCI_MCASP_RATES, .formats = DAVINCI_MCASP_PCM_FMTS,},
diff --git a/sound/soc/davinci/davinci-pcm.c b/sound/soc/davinci/davinci-pcm.c index bb57552..3af8b50 100644 --- a/sound/soc/davinci/davinci-pcm.c +++ b/sound/soc/davinci/davinci-pcm.c @@ -182,6 +182,7 @@ static void davinci_pcm_enqueue_dma(struct snd_pcm_substream *substream)>> unsigned short acnt; unsigned int count; unsigned int fifo_level;
- unsigned char serializers = prtd->params->active_serializers;
Can you please describe what DMA configuration you want? I think you can get rid of the active_serializers configuration by making use of tx_num_evt.>
period_size = snd_pcm_lib_period_bytes(substream); dma_offset = prtd->period * period_size;
@@ -195,14 +196,14 @@ static void davinci_pcm_enqueue_dma(struct snd_pcm_substream *substream)>> data_type = prtd->params->data_type; count = period_size / data_type; if (fifo_level)
count /= fifo_level;
count /= fifo_level * serializers;I think there's a problem is the way in which tx_num_evt is interpreted in the current code. Instead of setting to fifo_level to tx_num_evt, if you set it to tx_num_evt * num_serializers you can get rid of the additional code to take care of the number of serializers.>
Yes, threre is misleading with tx_num_evt in DT. This misleading is discussed in [alsa-devel] davici-mcasp: "tx-num-evt" confusion with number of serializers. thread.
Also this code misleads fifo_level usage, so maybe that code can be upgraded.
In davinci-mcasp code, there is fifo_level defined as txnum if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) fifo_level = dev->txnumevt; else fifo_level = dev->rxnumevt;
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
src = dma_pos; dst = prtd->params->dma_addr; src_bidx = data_type;
dst_bidx = 0;src_cidx = data_type * fifo_level;
dst_bidx = 4;Err.. this will most likely break other audio configurations. You should look at how to avoid this change by making use of the mask and rotation operations in the McASP code.>
McAsp FIFO and McASP data port reg. is 32 bit wide, each new sample is copied to McASP after McAsp send DMA event. McASP also do data rotations and masking. When more serializers are enabled, each new sample for new serializer should be placed to new data room at +4 address. After that new DMA event is generated. I tested it with TDM slots = 2 and also 6 with one serializer and also two serializer enabled. Why do you think that brokes other audio configurations?
src_cidx = data_type * fifo_level * serializers;dst_cidx = 0;
} else {
src = prtd->params->dma_addr;
@@ -210,7 +211,7 @@ static void davinci_pcm_enqueue_dma(struct snd_pcm_substream *substream)>> src_bidx = 0; dst_bidx = data_type; src_cidx = 0;
dst_cidx = data_type * fifo_level;
dst_cidx = data_type * fifo_level * serializers;With the change in fifo_level as described above, this won't be necessary.
Described upper and also in Re: [alsa-devel] davici-mcasp: "tx-num-evt" confusion with number of serializers thread
Best Michal.
On Wed, Mar 06, 2013 at 20:14:59, Michal Bachraty wrote: [...]
Yes, threre is misleading with tx_num_evt in DT. This misleading is discussed in [alsa-devel] davici-mcasp: "tx-num-evt" confusion with number of serializers. thread.
I responded to that thread yesterday. Can you please check that.
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
src = dma_pos; dst = prtd->params->dma_addr; src_bidx = data_type;
dst_bidx = 0;src_cidx = data_type * fifo_level;
dst_bidx = 4;Err.. this will most likely break other audio configurations. You should look at how to avoid this change by making use of the mask and rotation operations in the McASP code.>
McAsp FIFO and McASP data port reg. is 32 bit wide, each new sample is copied to McASP after McAsp send DMA event. McASP also do data rotations and masking. When more serializers are enabled, each new sample for new serializer should be placed to new data room at +4 address. After that new DMA event is generated. I tested it with TDM slots = 2 and also 6 with one serializer and also two serializer enabled. Why do you think that brokes other audio configurations?
Refer to section 22.3.10.1.2 of the AM335x TRM.
"To access through the data port, simply have the CPU or DMA access the XRBUF through its data port location. Through the data port, the DMA/CPU can service all the serializers through a single address. The McASP automatically cycles through the appropriate serializers.
For transmit operations through the data port, the DMA/CPU should write to the same XBUF data port address to service all of the active transmit serializers. In addition, the DMA/CPU should write to the XBUF for all active transmit serializers"
The DMA controller is expected to write to the same address. I am a bit surprised the +4 thing works without issue. Still, you should not rely on that since that's not supported as per the specs.
Regards, Vaibhav
Hi Vaibhav, Hi Michal,
On 05.03.2013 12:06, Bedia, Vaibhav wrote:
On Wed, Feb 27, 2013 at 22:08:45, Michal Bachraty wrote:
Davinci McASP has support for I2S multichannel playback. For I2S playback/receive, each serializer is capable to play 2 channels (L/R) audio data.Serializer function (Playback-receive-none) is configured in DT, depending on hardware specification. It is possible to play less channels than configured in DT. For that purpose,only specific number of active serializers are enabled. McASP FIFO need to have DMA transfer Bcnt set to number of enabled serializers, otherwise no data are transfered to McASP and Alsa generates "DMA/IRQ playback write error (DMA or IRQ trouble?)" error.
Thanks for looking into this. Before going into details, a few generic comments. All serializers configured in Tx (or Rx) work off common clock generators and hence the serializers will be operating in sync. I assume the setup that you have matches this requirement. Based on the DMA programming assumed in the implementation the user needs to ensure that buffer has the data in the right format. Can you please describe the setup that you have and how you tested this?
Signed-off-by: Michal Bachraty michal.bachraty@streamunlimited.com
sound/soc/davinci/davinci-mcasp.c | 56 ++++++++++++++++++++++++++++++++----- sound/soc/davinci/davinci-pcm.c | 16 ++++++----- sound/soc/davinci/davinci-pcm.h | 1 + 3 files changed, 59 insertions(+), 14 deletions(-)
diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c index afef3fb..b84bb73 100644 --- a/sound/soc/davinci/davinci-mcasp.c +++ b/sound/soc/davinci/davinci-mcasp.c @@ -235,6 +235,10 @@ #define DISMOD (val)(val<<2) #define TXSTATE BIT(4) #define RXSTATE BIT(5) +#define SRMOD_MASK 3 +#define SRMOD_INACTIVE 0 +#define SRMOD_TX 1 +#define SRMOD_RX 2
I don't see SRMOD_TX/RX being used anywhere.
/*
- DAVINCI_MCASP_LBCTL_REG - Loop Back Control Register Bits
@@ -657,12 +661,15 @@ static int davinci_config_channel_size(struct davinci_audio_dev *dev, return 0; }
-static void davinci_hw_common_param(struct davinci_audio_dev *dev, int stream) +static int davinci_hw_common_param(struct davinci_audio_dev *dev, int stream,
int channels){ int i; u8 tx_ser = 0; u8 rx_ser = 0;
- u8 ser;
- u8 slots = dev->tdm_slots;
- u8 max_active_serializers = (channels + slots - 1) / slots; /* Default configuration */ mcasp_set_bits(dev->base + DAVINCI_MCASP_PWREMUMGT_REG, MCASP_SOFT);
@@ -680,16 +687,42 @@ static void davinci_hw_common_param(struct davinci_audio_dev *dev, int stream) }
for (i = 0; i < dev->num_serializer; i++) {
if (dev->serial_dir[i] == TX_MODE)tx_ser++;if (dev->serial_dir[i] == RX_MODE)rx_ser++;- }
- if (stream == SNDRV_PCM_STREAM_PLAYBACK)
ser = tx_ser;- else
ser = rx_ser;- if (ser < max_active_serializers) {
dev_warn(dev->dev, "stream has more channels (%d) than are ""enabled in mcasp (%d)\n", channels, ser * slots);return -EINVAL;- }
- tx_ser = 0;
- rx_ser = 0;
The number of active serializers is already being calculated below.
FWIW, I'm now sending a cleanup patch for the two details I quoted above. With those out of the way, the rest (namely the dst_bidx issue) can be solved separately.
Please have a look and tell me if you're ok with that.
Thanks for your review!
Daniel
participants (5)
-
Bedia, Vaibhav -
Daniel Mack -
Mark Brown -
Michal Bachraty -
Mike Looijmans