[alsa-devel] [PATCH 1/3] ASoC: EP93xx I2S and PCM fixes
From: Alexander Sverdlin subaparts@yandex.ru
Changes to I2S code: - MCLK is turned on on driver probe. This is done for several codecs that need this for registers access (like CS4271). - SCLK and LRCLK rates are corrected, assuming we always send 32 bits * 2 channels to codec. - Formats list shortened to just S32_LE, this makes all the DMA transactions right, while ALSA will do all sample format translation for us. Changes to both I2S and PCM code: - Rates list extended up to 96kHz, it's tested on EDB9302 and works for both capture and playback.
Signed-off-by: Alexander Sverdlin subaparts@yandex.ru ---
sound/soc/ep93xx/ep93xx-i2s.c | 37 +++++++++++++++++++++++-------------- sound/soc/ep93xx/ep93xx-pcm.c | 4 ++-- 2 files changed, 25 insertions(+), 16 deletions(-)
diff --git a/sound/soc/ep93xx/ep93xx-i2s.c b/sound/soc/ep93xx/ep93xx-i2s.c index 4f48733..e5e9b9a 100644 --- a/sound/soc/ep93xx/ep93xx-i2s.c +++ b/sound/soc/ep93xx/ep93xx-i2s.c @@ -98,7 +98,6 @@ static void ep93xx_i2s_enable(struct ep93xx_i2s_info *info, int stream) if ((ep93xx_i2s_read_reg(info, EP93XX_I2S_TX0EN) & 0x1) == 0 && (ep93xx_i2s_read_reg(info, EP93XX_I2S_RX0EN) & 0x1) == 0) { /* Enable clocks */ - clk_enable(info->mclk); clk_enable(info->sclk); clk_enable(info->lrclk);
@@ -136,7 +135,6 @@ static void ep93xx_i2s_disable(struct ep93xx_i2s_info *info, int stream) /* Disable clocks */ clk_disable(info->lrclk); clk_disable(info->sclk); - clk_disable(info->mclk); } }
@@ -267,14 +265,16 @@ static int ep93xx_i2s_hw_params(struct snd_pcm_substream *substream, ep93xx_i2s_write_reg(info, EP93XX_I2S_RXWRDLEN, word_len);
/* - * Calculate the sdiv (bit clock) and lrdiv (left/right clock) values. - * If the lrclk is pulse length is larger than the word size, then the - * bit clock will be gated for the unused bits. + * EP93xx I2S module can be setup so SCLK / LRCLK value can be + * 32, 64, 128. MCLK / SCLK value can be 2 and 4. + * We set LRCLK equal to `rate' and minimum SCLK / LRCLK + * value is 64, because our sample size is 32 bit * 2 channels. + * I2S standard permits us to transmit more bits than + * the codec uses. */ - div = (clk_get_rate(info->mclk) / params_rate(params)) * - params_channels(params); + div = clk_get_rate(info->mclk) / params_rate(params); for (sdiv = 2; sdiv <= 4; sdiv += 2) - for (lrdiv = 32; lrdiv <= 128; lrdiv <<= 1) + for (lrdiv = 64; lrdiv <= 128; lrdiv <<= 1) if (sdiv * lrdiv == div) { found = 1; goto out; @@ -316,6 +316,7 @@ static int ep93xx_i2s_suspend(struct snd_soc_dai *dai)
ep93xx_i2s_disable(info, SNDRV_PCM_STREAM_PLAYBACK); ep93xx_i2s_disable(info, SNDRV_PCM_STREAM_CAPTURE); + clk_disable(info->mclk); }
static int ep93xx_i2s_resume(struct snd_soc_dai *dai) @@ -325,6 +326,7 @@ static int ep93xx_i2s_resume(struct snd_soc_dai *dai) if (!dai->active) return;
+ clk_enable(info->mclk); ep93xx_i2s_enable(info, SNDRV_PCM_STREAM_PLAYBACK); ep93xx_i2s_enable(info, SNDRV_PCM_STREAM_CAPTURE); } @@ -341,9 +343,7 @@ static struct snd_soc_dai_ops ep93xx_i2s_dai_ops = { .set_fmt = ep93xx_i2s_set_dai_fmt, };
-#define EP93XX_I2S_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | \ - SNDRV_PCM_FMTBIT_S24_LE | \ - SNDRV_PCM_FMTBIT_S32_LE) +#define EP93XX_I2S_FORMATS (SNDRV_PCM_FMTBIT_S32_LE)
static struct snd_soc_dai_driver ep93xx_i2s_dai = { .symmetric_rates= 1, @@ -352,13 +352,13 @@ static struct snd_soc_dai_driver ep93xx_i2s_dai = { .playback = { .channels_min = 2, .channels_max = 2, - .rates = SNDRV_PCM_RATE_8000_48000, + .rates = SNDRV_PCM_RATE_8000_96000, .formats = EP93XX_I2S_FORMATS, }, .capture = { .channels_min = 2, .channels_max = 2, - .rates = SNDRV_PCM_RATE_8000_48000, + .rates = SNDRV_PCM_RATE_8000_96000, .formats = EP93XX_I2S_FORMATS, }, .ops = &ep93xx_i2s_dai_ops, @@ -404,10 +404,16 @@ static int ep93xx_i2s_probe(struct platform_device *pdev) goto fail_unmap_mem; }
+ /* Minimal MCLK freq to enable some codecs */ + err = clk_set_rate(info->mclk, 8000 * 64 * 4); + if (err) + goto fail_put_mclk; + clk_enable(info->mclk); + info->sclk = clk_get(&pdev->dev, "sclk"); if (IS_ERR(info->sclk)) { err = PTR_ERR(info->sclk); - goto fail_put_mclk; + goto fail_disable_mclk; }
info->lrclk = clk_get(&pdev->dev, "lrclk"); @@ -426,6 +432,8 @@ fail_put_lrclk: clk_put(info->lrclk); fail_put_sclk: clk_put(info->sclk); +fail_disable_mclk: + clk_disable(info->mclk); fail_put_mclk: clk_put(info->mclk); fail_unmap_mem: @@ -444,6 +452,7 @@ static int __devexit ep93xx_i2s_remove(struct platform_device *pdev) snd_soc_unregister_dai(&pdev->dev); clk_put(info->lrclk); clk_put(info->sclk); + clk_disable(info->mclk); clk_put(info->mclk); iounmap(info->regs); release_mem_region(info->mem->start, resource_size(info->mem)); diff --git a/sound/soc/ep93xx/ep93xx-pcm.c b/sound/soc/ep93xx/ep93xx-pcm.c index 2f121dd..0667077 100644 --- a/sound/soc/ep93xx/ep93xx-pcm.c +++ b/sound/soc/ep93xx/ep93xx-pcm.c @@ -35,9 +35,9 @@ static const struct snd_pcm_hardware ep93xx_pcm_hardware = { SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER), - .rates = SNDRV_PCM_RATE_8000_48000, + .rates = SNDRV_PCM_RATE_8000_96000, .rate_min = SNDRV_PCM_RATE_8000, - .rate_max = SNDRV_PCM_RATE_48000, + .rate_max = SNDRV_PCM_RATE_96000, .formats = (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE |
On Wed, Dec 08, 2010 at 03:01:33PM +0300, Alexander wrote:
Changes to I2S code:
This should be split into a patch series, there's a bunch of changes here that are pretty much unrelated. Having everything in one patch makes review harder as more changes have to be thought about simultaneously and means that the different changes get tied up with each other and a problem with one can hold up others.
- MCLK is turned on on driver probe. This is done for several codecs that need this for registers access (like CS4271).
This seems like a retrograde step for systems that don't need it. Would it not be better to make this configurable by the machine driver?
- SCLK and LRCLK rates are corrected, assuming we always send 32 bits * 2 channels to codec.
What was the problem?
- Formats list shortened to just S32_LE, this makes all the DMA transactions right, while ALSA will do all sample format translation for us.
Again, what was the actual problem? 32 bit samples seem very large if the hardware is capable of other formats, especially given that things like MP3 tend to produce 16 bit data.
On Wed, 2010-12-08 at 12:46 +0000, Mark Brown wrote:
On Wed, Dec 08, 2010 at 03:01:33PM +0300, Alexander wrote:
Changes to I2S code:
This should be split into a patch series, there's a bunch of changes here that are pretty much unrelated. Having everything in one patch makes review harder as more changes have to be thought about simultaneously and means that the different changes get tied up with each other and a problem with one can hold up others.
I'll split it.
- SCLK and LRCLK rates are corrected, assuming we always send 32 bits * 2 channels to codec.
What was the problem?
- Formats list shortened to just S32_LE, this makes all the DMA transactions right, while ALSA will do all sample format translation for us.
Again, what was the actual problem? 32 bit samples seem very large if the hardware is capable of other formats, especially given that things like MP3 tend to produce 16 bit data.
It seems that EP93xx DMA could only operate 32 bit words. So the I2S module is always feed by 32 bit samples. Incorrect setting of LRCLK (2 times slower) in original ep93xx-i2s.c masks the problem. DMA takes two 16 bit samples instead of one, overall sound speed seems to be normal, but you get actually 4000 sampling rate instead of requested 8000 and therefore some noise... This is also the reason why the capture function not worked at all in this driver...
Best regards, Alexander A. Sverdlin.
On Thu, Dec 09, 2010 at 03:37:31AM +0300, Alexander wrote:
- Formats list shortened to just S32_LE, this makes all the DMA transactions right, while ALSA will do all sample format translation for us.
Again, what was the actual problem? 32 bit samples seem very large if the hardware is capable of other formats, especially given that things like MP3 tend to produce 16 bit data.
It seems that EP93xx DMA could only operate 32 bit words. So the I2S module is always feed by 32 bit samples. Incorrect setting of LRCLK (2 times slower) in original ep93xx-i2s.c masks the problem. DMA takes two 16 bit samples instead of one, overall sound speed seems to be normal, but you get actually 4000 sampling rate instead of requested 8000 and therefore some noise... This is also the reason why the capture function not worked at all in this driver...
The approach taken by the original code (while it sounds like it still has issues is a pretty standard way to deal with limitations in DMA controllers like this. You often end up programming the DMA controller to transfer 32 bit chunks and the I2S controller to transfer two 16 bit samples then let the FIFOs on the edge of the I2S controller sort out the difference. This means that the DMA controller transfers a stereo pair of samples at a go, which works well enough. Some of the hardware configuration may be technically incorrect according to the spec but so long as the externally observable behaviour is OK that's not an issue.
It may be that there's limitations in the hardware that prevent such a configuration but I'd like to see more analysis of what's going on here.
Dear Mark,
On Thu, 2010-12-09 at 10:54 +0000, Mark Brown wrote:
On Thu, Dec 09, 2010 at 03:37:31AM +0300, Alexander wrote:
- Formats list shortened to just S32_LE, this makes all the DMA transactions right, while ALSA will do all sample format translation for us.
Again, what was the actual problem? 32 bit samples seem very large if the hardware is capable of other formats, especially given that things like MP3 tend to produce 16 bit data.
It seems that EP93xx DMA could only operate 32 bit words. So the I2S module is always feed by 32 bit samples. Incorrect setting of LRCLK (2 times slower) in original ep93xx-i2s.c masks the problem. DMA takes two 16 bit samples instead of one, overall sound speed seems to be normal, but you get actually 4000 sampling rate instead of requested 8000 and therefore some noise... This is also the reason why the capture function not worked at all in this driver...
The approach taken by the original code (while it sounds like it still has issues is a pretty standard way to deal with limitations in DMA controllers like this. You often end up programming the DMA controller to transfer 32 bit chunks and the I2S controller to transfer two 16 bit samples then let the FIFOs on the edge of the I2S controller sort out the difference. This means that the DMA controller transfers a stereo pair of samples at a go, which works well enough. Some of the hardware configuration may be technically incorrect according to the spec but so long as the externally observable behaviour is OK that's not an issue.
I2S controller always takes 2 samples (2 ch) from DMA, DMA is always 32 bit, so if you will feed it with 16 samples, you will get 2 times faster sound.
It may be that there's limitations in the hardware that prevent such a configuration but I'd like to see more analysis of what's going on here.
If we take a look into I2S specification, we will figure that LRCLK MUST be equal to sample rate, if we are talking about stereo (in mono too, but it's not our case at all). So it doesnt seems strange to you, ep93xx-i2s.c produces LRCLK two times slower than it MUST be? It's a bug, making DMA bug less visible...
Always using 32 bit chunks is not a problem for I2S, the codec I use uses less bits too (24), it's permitted by I2S standard.
On Thu, Dec 09, 2010 at 03:17:41PM +0300, Alexander wrote:
On Thu, 2010-12-09 at 10:54 +0000, Mark Brown wrote:
pair of samples at a go, which works well enough. Some of the hardware configuration may be technically incorrect according to the spec but so long as the externally observable behaviour is OK that's not an issue.
I2S controller always takes 2 samples (2 ch) from DMA, DMA is always 32 bit, so if you will feed it with 16 samples, you will get 2 times faster sound.
How exactly does it "take" the samples? You're really not being clear about what the hardware is doing here. Please be specific about the interaction between the I2S controller and the DMA controller - for example, is there a FIFO on the edge of the I2S controller, is there any mediation of the data words anywhere in the process?
I *think* what you're trying to say above is that the I2S controller reads each sample as an individual word directly from the DMA controller so the DMA word size must match exactly the I2S word size but you're really not being clear.
It may be that there's limitations in the hardware that prevent such a configuration but I'd like to see more analysis of what's going on here.
If we take a look into I2S specification, we will figure that LRCLK MUST be equal to sample rate, if we are talking about stereo (in mono too, but it's not our case at all). So it doesnt seems strange to you, ep93xx-i2s.c produces LRCLK two times slower than it MUST be? It's a bug, making DMA bug less visible...
Right, but the DMA controller can (ignoring any restrictions in your hardware) happily transfer data in any size and this needn't have anything to do with what the I2S controller is emitting. You need to look at the configuration of the interaction between the I2S and DMA controllers rather than the external interface when looking at this stuff.
This is all broadly orthogonal to the external BCLK and LRCLK rates, it's all about how the data gets between the I2S and DMA controllers.
Always using 32 bit chunks is not a problem for I2S, the codec I use uses less bits too (24), it's permitted by I2S standard.
It's a problem because it means that the data needs to be laid out in memory in 32 bit words which means that a lot of data is going to need to be repacked from 16 bit to 32 bit samples which is wasteful. If the hardware can be persuaded to avoid this then that's much better.
Dear Mark,
On Thu, 2010-12-09 at 12:34 +0000, Mark Brown wrote:
Always using 32 bit chunks is not a problem for I2S, the codec I use uses less bits too (24), it's permitted by I2S standard.
It's a problem because it means that the data needs to be laid out in memory in 32 bit words which means that a lot of data is going to need to be repacked from 16 bit to 32 bit samples which is wasteful. If the hardware can be persuaded to avoid this then that's much better.
BTW, it's how original Cirrus's sound driver had done it's work. Cirrus programmers had not found a way to overcome this. The datasheets for EP93xx series cover everything only briefly... The dumbness of EP93xx's DMA is also the reason why we do not have DMA in serial ports and SSP...
Here we have attached original Cirrus driver: http://arm.cirrus.com/forum/viewtopic.php?t=3517 Here it is: http://arm.cirrus.com/forum/download.php?id=240
The function I'm talking about is snd_ep93xx_dma2usr_ratio(), as told in comments "For audio playback, we convert samples of arbitrary format to be 32 bit for our hardware".
Best regards, Alexander A. Sverdlin.
On Fri, Dec 10, 2010 at 12:14:18AM +0300, Alexander wrote:
BTW, it's how original Cirrus's sound driver had done it's work. Cirrus programmers had not found a way to overcome this. The datasheets for EP93xx series cover everything only briefly... The dumbness of EP93xx's DMA is also the reason why we do not have DMA in serial ports and SSP...
The function I'm talking about is snd_ep93xx_dma2usr_ratio(), as told in comments "For audio playback, we convert samples of arbitrary format to be 32 bit for our hardware".
This doesn't really answer any of my technical questions about what's going on here.
Please resubmit with a changelog explaining what the limitations are on both sides (DMA seems clear but the I2S also needs to be covered) and makes it clear why the functionality is being reduced like this. This will ensure that users understand why the change has been made - right now it looks like a serious functionality regression is being introduced so we really should make it clear why this is being done.
Dear Mark,
Well, now I have time to deal with this again, sorry for long pause. I've found, that author of code, on which modern ep93xx-i2s.c and ep93xx-pcm.c are based, had faced this problem also in 2007: http://blog.gmane.org/gmane.linux.ports.arm.cirrus/month=20070101/page=3
Now SoC code uses his developments, but not overcomes the hardware issues. Some details from EP93xx users guide:
Both I2S transmitter and receiver have similar 16x32bit FIFO, where they store 8 samples for both left and right channels. The FIFO is always 32bit wide and should be properly aligned if you use samples of other width. Transmitter and receiver have configuration registers for selection of I2S word length (16, 24, 32). They are I2STXWrdLen and I2SRXWrdLen.
Yes, EP93xx DMA can do byte, word and quad-word transfers. The width for transfers to and from peripherals is selected by particular module configuration. Lucky AC97 module has such configuration: AC97RXCRx registers, bit CM (Compact mode enable) switches between 16 and 32 bit samples. AC97TXCRx registers have the same bits for transmitters. ep93xx-ac97.c enables this compact mode and so has all the rights to use S16_LE format. No one has found such a configuration in I2S module until now in any Cirrus manuals. I2S module always feeds it's 32bit wide FIFO with 32bit samples consecutively for left and right channels. You cannot use 32-bit DMA transfers to transfer two 16-bit samples.
So we can use two formats for AC97, but should remove all but S32_LE for I2S.
Should I resend my patches now?
Best regards, Alexander A. Sverdlin.
On Fri, 2010-12-10 at 15:07 +0000, Mark Brown wrote:
On Fri, Dec 10, 2010 at 12:14:18AM +0300, Alexander wrote:
BTW, it's how original Cirrus's sound driver had done it's work. Cirrus programmers had not found a way to overcome this. The datasheets for EP93xx series cover everything only briefly... The dumbness of EP93xx's DMA is also the reason why we do not have DMA in serial ports and SSP...
The function I'm talking about is snd_ep93xx_dma2usr_ratio(), as told in comments "For audio playback, we convert samples of arbitrary format to be 32 bit for our hardware".
This doesn't really answer any of my technical questions about what's going on here.
Please resubmit with a changelog explaining what the limitations are on both sides (DMA seems clear but the I2S also needs to be covered) and makes it clear why the functionality is being reduced like this. This will ensure that users understand why the change has been made - right now it looks like a serious functionality regression is being introduced so we really should make it clear why this is being done.
On Sun, Jan 16, 2011 at 02:21:02PM +0300, Alexander wrote:
Please don't top post.
So we can use two formats for AC97, but should remove all but S32_LE for I2S.
Should I resend my patches now?
Yes. Like I said the important thing is to have a changelog which makes it clear what is going on.
From: Alexander Sverdlin subaparts@yandex.ru
Changes to both I2S and PCM code: - Rates list extended up to 96kHz, it's tested on EDB9302 and works for both capture and playback.
Signed-off-by: Alexander Sverdlin subaparts@yandex.ru --- sound/soc/ep93xx/ep93xx-i2s.c | 4 ++-- sound/soc/ep93xx/ep93xx-pcm.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/sound/soc/ep93xx/ep93xx-i2s.c b/sound/soc/ep93xx/ep93xx-i2s.c index 4f48733..9ac93f6 100644 --- a/sound/soc/ep93xx/ep93xx-i2s.c +++ b/sound/soc/ep93xx/ep93xx-i2s.c @@ -352,13 +352,13 @@ static struct snd_soc_dai_driver ep93xx_i2s_dai = { .playback = { .channels_min = 2, .channels_max = 2, - .rates = SNDRV_PCM_RATE_8000_48000, + .rates = SNDRV_PCM_RATE_8000_96000, .formats = EP93XX_I2S_FORMATS, }, .capture = { .channels_min = 2, .channels_max = 2, - .rates = SNDRV_PCM_RATE_8000_48000, + .rates = SNDRV_PCM_RATE_8000_96000, .formats = EP93XX_I2S_FORMATS, }, .ops = &ep93xx_i2s_dai_ops, diff --git a/sound/soc/ep93xx/ep93xx-pcm.c b/sound/soc/ep93xx/ep93xx-pcm.c index 2f121dd..0667077 100644 --- a/sound/soc/ep93xx/ep93xx-pcm.c +++ b/sound/soc/ep93xx/ep93xx-pcm.c @@ -35,9 +35,9 @@ static const struct snd_pcm_hardware ep93xx_pcm_hardware = { SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER), - .rates = SNDRV_PCM_RATE_8000_48000, + .rates = SNDRV_PCM_RATE_8000_96000, .rate_min = SNDRV_PCM_RATE_8000, - .rate_max = SNDRV_PCM_RATE_48000, + .rate_max = SNDRV_PCM_RATE_96000, .formats = (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE |
On Thu, 2010-12-09 at 03:43 +0300, Alexander wrote:
From: Alexander Sverdlin subaparts@yandex.ru
Changes to both I2S and PCM code:
- Rates list extended up to 96kHz, it's tested on EDB9302 and works for both capture and playback.
Signed-off-by: Alexander Sverdlin subaparts@yandex.ru
Acked-by: Liam Girdwood lrg@slimlogic.co.uk
On Thu, Dec 09, 2010 at 03:43:49AM +0300, Alexander wrote:
From: Alexander Sverdlin subaparts@yandex.ru
Changes to both I2S and PCM code:
- Rates list extended up to 96kHz, it's tested on EDB9302 and works for both capture and playback.
Signed-off-by: Alexander Sverdlin subaparts@yandex.ru
Applied, thanks.
From: Alexander Sverdlin subaparts@yandex.ru
Changes to I2S code: - SCLK and LRCLK rates are corrected, assuming we always send 32 bits * 2 channels to codec. - Formats list shortened to just S32_LE, this makes all the DMA transactions right, while ALSA will do all sample format translation for us.
Signed-off-by: Alexander Sverdlin subaparts@yandex.ru ---
sound/soc/ep93xx/ep93xx-i2s.c | 18 +++++++++--------- 1 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/sound/soc/ep93xx/ep93xx-i2s.c b/sound/soc/ep93xx/ep93xx-i2s.c index 4f48733..3e4c3d9 100644 --- a/sound/soc/ep93xx/ep93xx-i2s.c +++ b/sound/soc/ep93xx/ep93xx-i2s.c @@ -267,14 +267,16 @@ static int ep93xx_i2s_hw_params(struct snd_pcm_substream *substream, ep93xx_i2s_write_reg(info, EP93XX_I2S_RXWRDLEN, word_len);
/* - * Calculate the sdiv (bit clock) and lrdiv (left/right clock) values. - * If the lrclk is pulse length is larger than the word size, then the - * bit clock will be gated for the unused bits. + * EP93xx I2S module can be setup so SCLK / LRCLK value can be + * 32, 64, 128. MCLK / SCLK value can be 2 and 4. + * We set LRCLK equal to `rate' and minimum SCLK / LRCLK + * value is 64, because our sample size is 32 bit * 2 channels. + * I2S standard permits us to transmit more bits than + * the codec uses. */ - div = (clk_get_rate(info->mclk) / params_rate(params)) * - params_channels(params); + div = clk_get_rate(info->mclk) / params_rate(params); for (sdiv = 2; sdiv <= 4; sdiv += 2) - for (lrdiv = 32; lrdiv <= 128; lrdiv <<= 1) + for (lrdiv = 64; lrdiv <= 128; lrdiv <<= 1) if (sdiv * lrdiv == div) { found = 1; goto out; @@ -341,9 +343,7 @@ static struct snd_soc_dai_ops ep93xx_i2s_dai_ops = { .set_fmt = ep93xx_i2s_set_dai_fmt, };
-#define EP93XX_I2S_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | \ - SNDRV_PCM_FMTBIT_S24_LE | \ - SNDRV_PCM_FMTBIT_S32_LE) +#define EP93XX_I2S_FORMATS (SNDRV_PCM_FMTBIT_S32_LE)
static struct snd_soc_dai_driver ep93xx_i2s_dai = { .symmetric_rates= 1,
On Thu, 2010-12-09 at 03:59 +0300, Alexander wrote:
From: Alexander Sverdlin subaparts@yandex.ru
Changes to I2S code:
- SCLK and LRCLK rates are corrected, assuming we always send 32 bits * 2 channels to codec.
- Formats list shortened to just S32_LE, this makes all the DMA transactions right, while ALSA will do all sample format translation for us.
Signed-off-by: Alexander Sverdlin subaparts@yandex.ru
Acked-by: Liam Girdwood lrg@slimlogic.co.uk
From: Alexander Sverdlin subaparts@yandex.ru
Changelog: 1. I2S module of EP93xx should be feed by 32bit DMA transfers. This is hardware limitation and that's the way original Cirrus's driver worked. This will fix distorted sound playback and make capture actually work in present ep93xx drivers.
I've found, that author of code, on which modern ep93xx-i2s.c and ep93xx-pcm.c are based, had faced this problem also in 2007: http://blog.gmane.org/gmane.linux.ports.arm.cirrus/month=20070101/page=3
Now SoC code uses his developments, but not overcomes the hardware issues. Some details from EP93xx users guide:
Both I2S transmitter and receiver have similar 16x32bit FIFO, where they store 8 samples for both left and right channels. The FIFO is always 32bit wide and should be properly aligned if you use samples of other width. Transmitter and receiver have configuration registers for selection of I2S word length (16, 24, 32). They are I2STXWrdLen and I2SRXWrdLen.
Yes, EP93xx DMA can do byte, word and quad-word transfers. The width for transfers to and from peripherals is selected by particular module configuration. Lucky AC97 module has such configuration: AC97RXCRx registers, bit CM (Compact mode enable) switches between 16 and 32 bit samples. AC97TXCRx registers have the same bits for transmitters. ep93xx-ac97.c enables this compact mode and so has all the rights to use S16_LE format. No one has found such a configuration in I2S module until now in any Cirrus manuals. I2S module always feeds it's 32bit wide FIFO with 32bit samples consecutively for left and right channels. You cannot use 32-bit DMA transfers to transfer two 16-bit samples.
So we can use two formats for AC97, but should remove all but S32_LE for I2S. Always using 32 bit chunks is not a problem for I2S, the codec I use uses less bits too (24), it's permitted by I2S standard.
In proposed patch formats list shortened to just S32_LE, this makes all the DMA transactions right, while ALSA will do all sample format translation for us.
2. Incorrect setting of LRCLK (2 times slower) in original ep93xx-i2s.c masks the first problem.
DMA takes two 16 bit samples instead of one, overall sound speed seems to be normal, but you get actually 4000 sampling rate instead of requested 8000 and therefore some noise... This is also the reason why the capture function not worked at all in this driver...
If we take a look into I2S specification, we will figure that LRCLK MUST be equal to sample rate, if we are talking about stereo (in mono too, but it's not our case at all).
In proposed patch SCLK and LRCLK rates are corrected, assuming we always send 32 bits * 2 channels to codec.
Signed-off-by: Alexander Sverdlin subaparts@yandex.ru ---
sound/soc/ep93xx/ep93xx-i2s.c | 18 +++++++++--------- 1 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/sound/soc/ep93xx/ep93xx-i2s.c b/sound/soc/ep93xx/ep93xx-i2s.c index 4f48733..3e4c3d9 100644 --- a/sound/soc/ep93xx/ep93xx-i2s.c +++ b/sound/soc/ep93xx/ep93xx-i2s.c @@ -267,14 +267,16 @@ static int ep93xx_i2s_hw_params(struct snd_pcm_substream *substream, ep93xx_i2s_write_reg(info, EP93XX_I2S_RXWRDLEN, word_len);
/* - * Calculate the sdiv (bit clock) and lrdiv (left/right clock) values. - * If the lrclk is pulse length is larger than the word size, then the - * bit clock will be gated for the unused bits. + * EP93xx I2S module can be setup so SCLK / LRCLK value can be + * 32, 64, 128. MCLK / SCLK value can be 2 and 4. + * We set LRCLK equal to `rate' and minimum SCLK / LRCLK + * value is 64, because our sample size is 32 bit * 2 channels. + * I2S standard permits us to transmit more bits than + * the codec uses. */ - div = (clk_get_rate(info->mclk) / params_rate(params)) * - params_channels(params); + div = clk_get_rate(info->mclk) / params_rate(params); for (sdiv = 2; sdiv <= 4; sdiv += 2) - for (lrdiv = 32; lrdiv <= 128; lrdiv <<= 1) + for (lrdiv = 64; lrdiv <= 128; lrdiv <<= 1) if (sdiv * lrdiv == div) { found = 1; goto out; @@ -341,9 +343,7 @@ static struct snd_soc_dai_ops ep93xx_i2s_dai_ops = { .set_fmt = ep93xx_i2s_set_dai_fmt, };
-#define EP93XX_I2S_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | \ - SNDRV_PCM_FMTBIT_S24_LE | \ - SNDRV_PCM_FMTBIT_S32_LE) +#define EP93XX_I2S_FORMATS (SNDRV_PCM_FMTBIT_S32_LE)
static struct snd_soc_dai_driver ep93xx_i2s_dai = { .symmetric_rates= 1,
On Sun, 2011-01-16 at 15:48 +0300, Alexander wrote:
From: Alexander Sverdlin subaparts@yandex.ru
Changelog:
- I2S module of EP93xx should be feed by 32bit DMA transfers. This is
hardware limitation and that's the way original Cirrus's driver worked. This will fix distorted sound playback and make capture actually work in present ep93xx drivers.
I've found, that author of code, on which modern ep93xx-i2s.c and ep93xx-pcm.c are based, had faced this problem also in 2007: http://blog.gmane.org/gmane.linux.ports.arm.cirrus/month=20070101/page=3
Now SoC code uses his developments, but not overcomes the hardware issues. Some details from EP93xx users guide:
Both I2S transmitter and receiver have similar 16x32bit FIFO, where they store 8 samples for both left and right channels. The FIFO is always 32bit wide and should be properly aligned if you use samples of other width. Transmitter and receiver have configuration registers for selection of I2S word length (16, 24, 32). They are I2STXWrdLen and I2SRXWrdLen.
Yes, EP93xx DMA can do byte, word and quad-word transfers. The width for transfers to and from peripherals is selected by particular module configuration. Lucky AC97 module has such configuration: AC97RXCRx registers, bit CM (Compact mode enable) switches between 16 and 32 bit samples. AC97TXCRx registers have the same bits for transmitters. ep93xx-ac97.c enables this compact mode and so has all the rights to use S16_LE format. No one has found such a configuration in I2S module until now in any Cirrus manuals. I2S module always feeds it's 32bit wide FIFO with 32bit samples consecutively for left and right channels. You cannot use 32-bit DMA transfers to transfer two 16-bit samples.
So we can use two formats for AC97, but should remove all but S32_LE for I2S. Always using 32 bit chunks is not a problem for I2S, the codec I use uses less bits too (24), it's permitted by I2S standard.
In proposed patch formats list shortened to just S32_LE, this makes all the DMA transactions right, while ALSA will do all sample format translation for us.
- Incorrect setting of LRCLK (2 times slower) in original ep93xx-i2s.c
masks the first problem.
DMA takes two 16 bit samples instead of one, overall sound speed seems to be normal, but you get actually 4000 sampling rate instead of requested 8000 and therefore some noise... This is also the reason why the capture function not worked at all in this driver...
If we take a look into I2S specification, we will figure that LRCLK MUST be equal to sample rate, if we are talking about stereo (in mono too, but it's not our case at all).
In proposed patch SCLK and LRCLK rates are corrected, assuming we always send 32 bits * 2 channels to codec.
Signed-off-by: Alexander Sverdlin subaparts@yandex.ru
Acked-by: Liam Girdwood lrg@slimlogic.co.uk
On Sun, Jan 16, 2011 at 03:48:05PM +0300, Alexander wrote:
From: Alexander Sverdlin subaparts@yandex.ru
Changelog:
- I2S module of EP93xx should be feed by 32bit DMA transfers. This is
hardware limitation and that's the way original Cirrus's driver worked.
Applied, thanks.
participants (3)
-
Alexander -
Liam Girdwood -
Mark Brown