[PATCH 1/3] ASoC: EP93xx I2S and PCM fixes

[PATCH 1/3] ASoC: EP93xx I2S and PCM fixes

[Alexander]

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 |

[PATCH 1/3] ASoC: EP93xx I2S and PCM fixes

[Mark Brown]

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.

[PATCH 1/3] ASoC: EP93xx I2S and PCM fixes

[Alexander]

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.

[PATCH 1/3] ASoC: EP93xx I2S and PCM fixes

[Mark Brown]

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.

[PATCH 1/3] ASoC: EP93xx I2S and PCM fixes

[Alexander]

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.

[PATCH 1/3] ASoC: EP93xx I2S and PCM fixes

[Mark Brown]

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.

[PATCH 1/3] ASoC: EP93xx I2S and PCM fixes

[Alexander]

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.

[PATCH 1/3] ASoC: EP93xx I2S and PCM fixes

[Mark Brown]

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.

[PATCH 1/3] ASoC: EP93xx I2S and PCM fixes

[Alexander]

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.
> 

[PATCH 1/3] ASoC: EP93xx I2S and PCM fixes

[Mark Brown]

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.

[PATCH] ASoC: EP93xx: sampling rate range extended

[Alexander]

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 |

[alsa-devel] [PATCH] ASoC: EP93xx: sampling rate range extended

[Liam Girdwood]

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>
-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

[PATCH] ASoC: EP93xx: sampling rate range extended

[Mark Brown]

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.

[PATCH] ASoC: EP93xx: fixed LRCLK rate and DMA oper. in I2S code

[Alexander]

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,

[alsa-devel] [PATCH] ASoC: EP93xx: fixed LRCLK rate and DMA oper. in I2S code

[Liam Girdwood]

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>

-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

[PATCH] ASoC: EP93xx: fixed LRCLK rate and DMA oper. in I2S code

[Alexander]

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,

[alsa-devel] [PATCH] ASoC: EP93xx: fixed LRCLK rate and DMA oper. in I2S code

[Liam Girdwood]

On Sun, 2011-01-16 at 15:48 +0300, Alexander wrote:
> 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>

Acked-by: Liam Girdwood <lrg@slimlogic.co.uk>
-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

[PATCH] ASoC: EP93xx: fixed LRCLK rate and DMA oper. in I2S code

[Mark Brown]

On Sun, Jan 16, 2011 at 03:48:05PM +0300, Alexander wrote:
> 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.

Applied, thanks.