[PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[Eric Miao]

Make the pxa I2S configuration generic, add support for Left_J, add
support for variable frame width like 32fs, 48fs, 64fs and 96fs

Signed-off-by: Paul Shen <bshen9@marvell.com>
Signed-off-by: Eric Miao <eric.miao@marvell.com>
Cc: Daniel Mack <daniel@caiaq.de>
---
 arch/arm/mach-pxa/include/mach/regs-ssp.h |   14 +++---
 sound/soc/pxa/pxa-ssp.c                   |   62 ++++++++++++++--------------
 sound/soc/pxa/pxa-ssp.h                   |    9 ++++
 3 files changed, 47 insertions(+), 38 deletions(-)

diff --git a/arch/arm/mach-pxa/include/mach/regs-ssp.h
b/arch/arm/mach-pxa/include/mach/regs-ssp.h
index 6a2ed35..27f0cd4 100644
--- a/arch/arm/mach-pxa/include/mach/regs-ssp.h
+++ b/arch/arm/mach-pxa/include/mach/regs-ssp.h
@@ -108,21 +108,21 @@
 #define SSSR_TINT		(1 << 19)	/* Receiver Time-out Interrupt */
 #define SSSR_PINT		(1 << 18)	/* Peripheral Trailing Byte Interrupt */

-#if defined(CONFIG_PXA3xx)
-#define SSPSP_EDMYSTOP(x)	((x) << 28)     /* Extended Dummy Stop */
-#define SSPSP_EDMYSTRT(x)	((x) << 26)     /* Extended Dummy Start */
-#endif
-
 #define SSPSP_FSRT		(1 << 25)	/* Frame Sync Relative Timing */
-#define SSPSP_DMYSTOP(x)	((x) << 23)	/* Dummy Stop */
 #define SSPSP_SFRMWDTH(x)	((x) << 16)	/* Serial Frame Width */
 #define SSPSP_SFRMDLY(x)	((x) << 9)	/* Serial Frame Delay */
-#define SSPSP_DMYSTRT(x)	((x) << 7)	/* Dummy Start */
 #define SSPSP_STRTDLY(x)	((x) << 4)	/* Start Delay */
 #define SSPSP_ETDS		(1 << 3)	/* End of Transfer data State */
 #define SSPSP_SFRMP		(1 << 2)	/* Serial Frame Polarity */
 #define SSPSP_SCMODE(x)		((x) << 0)	/* Serial Bit Rate Clock Mode */

+/* NOTE: PXA3xx extends the bit number of dummy start and stop, the macros
+ * below are compatible with PXA25x/27x as long as the parameter is within
+ * the correct limits, driver code has to take care of this.
+ */
+#define SSPSP_DMYSTRT(x)	((((x) & 3) << 7)  | ((((x) >> 2) & 3) << 26))
+#define SSPSP_DMYSTOP(x)	((((x) & 3) << 23) | ((((x) >> 2) & 7) << 28))
+
 #define SSACD_SCDB		(1 << 3)	/* SSPSYSCLK Divider Bypass */
 #define SSACD_ACPS(x)		((x) << 4)	/* Audio clock PLL select */
 #define SSACD_ACDS(x)		((x) << 0)	/* Audio clock divider select */
diff --git a/sound/soc/pxa/pxa-ssp.c b/sound/soc/pxa/pxa-ssp.c
index 6fc7876..2831c16 100644
--- a/sound/soc/pxa/pxa-ssp.c
+++ b/sound/soc/pxa/pxa-ssp.c
@@ -463,7 +463,8 @@ static int pxa_ssp_set_dai_fmt(struct snd_soc_dai *cpu_dai,

 	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
 	case SND_SOC_DAIFMT_I2S:
-		sscr0 |= SSCR0_PSP;
+	case SND_SOC_DAIFMT_LEFT_J:
+		sscr0 |= SSCR0_PSP | SSCR0_MOD;
 		sscr1 |= SSCR1_RWOT | SSCR1_TRAIL;

 		/* See hw_params() */
@@ -541,6 +542,7 @@ static int pxa_ssp_hw_params(struct
snd_pcm_substream *substream,
 	int chn = params_channels(params);
 	u32 sscr0;
 	u32 sspsp;
+	int frame_width;
 	int width = snd_pcm_format_physical_width(params_format(params));
 	int ttsa = ssp_read_reg(ssp, SSTSA) & 0xf;

@@ -585,40 +587,38 @@ static int pxa_ssp_hw_params(struct
snd_pcm_substream *substream,

 	switch (priv->dai_fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
 	case SND_SOC_DAIFMT_I2S:
-	       sspsp = ssp_read_reg(ssp, SSPSP);
-
-		if ((ssp_get_scr(ssp) == 4) && (width == 16)) {
-			/* This is a special case where the bitclk is 64fs
-			* and we're not dealing with 2*32 bits of audio
-			* samples.
-			*
-			* The SSP values used for that are all found out by
-			* trying and failing a lot; some of the registers
-			* needed for that mode are only available on PXA3xx.
-			*/
+		sspsp = ssp_read_reg(ssp, SSPSP);
+		frame_width = PXA_SSP_FRM_WIDTH(priv->dai_fmt);

-#ifdef CONFIG_PXA3xx
-			if (!cpu_is_pxa3xx())
-				return -EINVAL;
-
-			sspsp |= SSPSP_SFRMWDTH(width * 2);
-			sspsp |= SSPSP_SFRMDLY(width * 4);
-			sspsp |= SSPSP_EDMYSTOP(3);
-			sspsp |= SSPSP_DMYSTOP(3);
-			sspsp |= SSPSP_DMYSTRT(1);
-#else
+		if (frame_width < width * 2)
 			return -EINVAL;
-#endif
-		} else {
-			/* The frame width is the width the LRCLK is
-			 * asserted for; the delay is expressed in
-			 * half cycle units.  We need the extra cycle
-			 * because the data starts clocking out one BCLK
-			 * after LRCLK changes polarity.
+
+		if (frame_width == width * 2)
+			/* frame width is exactly double of data sample width,
+			 * use FSRT instead
 			 */
-			sspsp |= SSPSP_SFRMWDTH(width + 1);
-			sspsp |= SSPSP_SFRMDLY((width + 1) * 2);
+			sspsp |= SSPSP_FSRT | SSPSP_SFRMWDTH(width);
+		else {
 			sspsp |= SSPSP_DMYSTRT(1);
+			sspsp |= SSPSP_DMYSTOP((frame_width / 2 - width - 1));
+			sspsp |= SSPSP_SFRMWDTH(frame_width / 2);
+		}
+
+		ssp_write_reg(ssp, SSPSP, sspsp);
+		break;
+
+	case SND_SOC_DAIFMT_LEFT_J:
+		sspsp = ssp_read_reg(ssp, SSPSP);
+		frame_width = PXA_SSP_FRM_WIDTH(priv->dai_fmt);
+
+		if (frame_width < width * 2)
+			return -EINVAL;
+
+		if (frame_width == width * 2)
+			sspsp |= SSPSP_SFRMWDTH(width);
+		else {
+			sspsp |= SSPSP_DMYSTOP((frame_width / 2 - width));
+			sspsp |= SSPSP_SFRMWDTH(frame_width / 2);
 		}

 		ssp_write_reg(ssp, SSPSP, sspsp);
diff --git a/sound/soc/pxa/pxa-ssp.h b/sound/soc/pxa/pxa-ssp.h
index 91deadd..fda51d0 100644
--- a/sound/soc/pxa/pxa-ssp.h
+++ b/sound/soc/pxa/pxa-ssp.h
@@ -40,6 +40,15 @@
 #define PXA_SSP_CLK_SCDB_1		1
 #define PXA_SSP_CLK_SCDB_8		2

+/* frame size definitions for I2S and Left_J formats - default is
+ * 32fs, other possibilities are 48fs, 64fs and 96fs
+ */
+#define PXA_SSP_FRM_32FS	(0 << 16)
+#define PXA_SSP_FRM_48FS	(1 << 16)
+#define PXA_SSP_FRM_64FS	(2 << 16)
+#define PXA_SSP_FRM_96FS	(3 << 16)
+#define PXA_SSP_FRM_WIDTH(x)	(((((x) >> 16) & 0x3) + 2) << 4)
+
 #define PXA_SSP_PLL_OUT  0

 extern struct snd_soc_dai pxa_ssp_dai[4];
-- 
1.6.0.4

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[Daniel Mack]

On Wed, Jun 03, 2009 at 08:33:42PM +0800, Eric Miao wrote:
> Make the pxa I2S configuration generic, add support for Left_J, add
> support for variable frame width like 32fs, 48fs, 64fs and 96fs
> 
> Signed-off-by: Paul Shen <bshen9@marvell.com>
> Signed-off-by: Eric Miao <eric.miao@marvell.com>
> Cc: Daniel Mack <daniel@caiaq.de>
> ---
>  arch/arm/mach-pxa/include/mach/regs-ssp.h |   14 +++---
>  sound/soc/pxa/pxa-ssp.c                   |   62 ++++++++++++++--------------
>  sound/soc/pxa/pxa-ssp.h                   |    9 ++++
>  3 files changed, 47 insertions(+), 38 deletions(-)

Ok, I tried that code on my board and this is what I had to change
there:

The tdm time slot configuration needs to be set again in my board support
code just like in your example: snd_soc_set_tdm_slot(cpu_dai, 3, 2). And
the PXA_SSP_DIV_SCR value needed to be doubled from 4 to 8.

With that changes, LRCLK is 44100Hz when configured to 44100Hz. But the
bitclk is not 64fs anymore but 32fs only (1.41Mhz). Is there any
implementation details I miss? What does your codec clock config look
like?

Another small thing:

  CC      sound/soc/pxa/pxa-ssp.o
  sound/soc/pxa/pxa-ssp.c:186: warning: 'ssp_get_scr' defined but not used
 
Daniel

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[Eric Miao]

On Wed, Jun 3, 2009 at 9:18 PM, Daniel Mack <daniel@caiaq.de> wrote:
> On Wed, Jun 03, 2009 at 08:33:42PM +0800, Eric Miao wrote:
>> Make the pxa I2S configuration generic, add support for Left_J, add
>> support for variable frame width like 32fs, 48fs, 64fs and 96fs
>>
>> Signed-off-by: Paul Shen <bshen9@marvell.com>
>> Signed-off-by: Eric Miao <eric.miao@marvell.com>
>> Cc: Daniel Mack <daniel@caiaq.de>
>> ---
>>  arch/arm/mach-pxa/include/mach/regs-ssp.h |   14 +++---
>>  sound/soc/pxa/pxa-ssp.c                   |   62 ++++++++++++++--------------
>>  sound/soc/pxa/pxa-ssp.h                   |    9 ++++
>>  3 files changed, 47 insertions(+), 38 deletions(-)
>
> Ok, I tried that code on my board and this is what I had to change
> there:
>
> The tdm time slot configuration needs to be set again in my board support
> code just like in your example: snd_soc_set_tdm_slot(cpu_dai, 3, 2).

Mmm.... what would be the preferred way, to let board specific
code do this or the I2S configuration code?

> And
> the PXA_SSP_DIV_SCR value needed to be doubled from 4 to 8.

OK, I'll check with SCR, since we used the dithering clock
SSACD and SSACDD for the clock.

>
> With that changes, LRCLK is 44100Hz when configured to 44100Hz. But the
> bitclk is not 64fs anymore but 32fs only (1.41Mhz). Is there any
> implementation details I miss? What does your codec clock config look
> like?
>
> Another small thing:
>
>  CC      sound/soc/pxa/pxa-ssp.o
>  sound/soc/pxa/pxa-ssp.c:186: warning: 'ssp_get_scr' defined but not used
>

OK, will get rid of that.

> Daniel
>
>



-- 
Cheers
- eric
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[Mark Brown]

On Wed, Jun 03, 2009 at 10:22:17PM +0800, Eric Miao wrote:
> On Wed, Jun 3, 2009 at 9:18 PM, Daniel Mack <daniel@caiaq.de> wrote:

> > The tdm time slot configuration needs to be set again in my board support
> > code just like in your example: snd_soc_set_tdm_slot(cpu_dai, 3, 2).

> Mmm.... what would be the preferred way, to let board specific
> code do this or the I2S configuration code?

TDM would normally be board-specific.

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[Daniel Mack]

On Wed, Jun 03, 2009 at 10:22:17PM +0800, Eric Miao wrote:
> > The tdm time slot configuration needs to be set again in my board support
> > code just like in your example: snd_soc_set_tdm_slot(cpu_dai, 3, 2).
> 
> Mmm.... what would be the preferred way, to let board specific
> code do this or the I2S configuration code?

It is a configuration details that is up to the user to set up. Hence
the right place is the board specific code - I just wanted to let you
know the interface differences from a board support code point of view.

Daniel

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[Paul Shen]

Hi Daniel,

This is Paul Shen , I will substitute boshen9@gmail.com  for
bshen9@marvell.com to answer open source mails.


> From: Daniel Mack <daniel@caiaq.de>
> Date: Wed, Jun 3, 2009 at 9:18 PM
> Subject: Re: [alsa-devel] [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support
> To: Eric Miao <eric.y.miao@gmail.com>
> Cc: alsa-devel@alsa-project.org, linux-arm-kernel <linux-arm-kernel@lists.arm.linux.org.uk>, Mark Brown <broonie@sirena.org.uk>
>
>
> On Wed, Jun 03, 2009 at 08:33:42PM +0800, Eric Miao wrote:
> > Make the pxa I2S configuration generic, add support for Left_J, add
> > support for variable frame width like 32fs, 48fs, 64fs and 96fs
> >
> > Signed-off-by: Paul Shen <bshen9@marvell.com>
> > Signed-off-by: Eric Miao <eric.miao@marvell.com>
> > Cc: Daniel Mack <daniel@caiaq.de>
> > ---
> >  arch/arm/mach-pxa/include/mach/regs-ssp.h |   14 +++---
> >  sound/soc/pxa/pxa-ssp.c                   |   62 ++++++++++++++--------------
> >  sound/soc/pxa/pxa-ssp.h                   |    9 ++++
> >  3 files changed, 47 insertions(+), 38 deletions(-)
>
> Ok, I tried that code on my board and this is what I had to change
> there:
>
> The tdm time slot configuration needs to be set again in my board support
> code just like in your example: snd_soc_set_tdm_slot(cpu_dai, 3, 2). And
> the PXA_SSP_DIV_SCR value needed to be doubled from 4 to 8.
>
I tested  with below  codes to set the cpu_dai  :

format = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF |
                         SND_SOC_DAIFMT_CBS_CFS | PXA_SSP_FRM_64FS;

snd_soc_dai_set_fmt(cpu_dai, format);
snd_soc_dai_set_tdm_slot(cpu_dai, 3, 2);

snd_soc_dai_set_sysclk(cpu_dai, PXA_SSP_CLK_EXT, clk, 0);
snd_soc_dai_set_clkdiv(cpu_dai, PXA_SSP_DIV_SCR, 4);

It is no need to "snd_soc_dai_set_pll(cpu_dai, 0, 0, clk)" , for  your
case ,no ssp-on-chip pll configuration is needed.

On my littleton platform,it is ok, and  I did not modify the
PXA_SSP_DIV_SCR from 4 to 8.

I dumped the ssp register:
SSCR0 0xa10003ff
SSCR1 0x00e01d80
SSTO 0x00000000
SSPSP 0x31a00084
SSSR 0x0000f0fc
SSACD 0x00000000
SSACDD 0x00000000

How about your registers ?

> With that changes, LRCLK is 44100Hz when configured to 44100Hz. But the
> bitclk is not 64fs anymore but 32fs only (1.41Mhz). Is there any
> implementation details I miss? What does your codec clock config look
> like?
>
Do you mean it only down to 32fs when LRCLK is 44100HZ ?
And when LRCLK is 48000HZ, all is OK ?

> Another small thing:
>
>  CC      sound/soc/pxa/pxa-ssp.o
>  sound/soc/pxa/pxa-ssp.c:186: warning: 'ssp_get_scr' defined but not used
>
> Daniel
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
>

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[Daniel Mack]

Hi Paul,

sorry for the delay.

On Thu, Jun 04, 2009 at 05:44:55PM +0800, Paul Shen wrote:
> > The tdm time slot configuration needs to be set again in my board support
> > code just like in your example: snd_soc_set_tdm_slot(cpu_dai, 3, 2). And
> > the PXA_SSP_DIV_SCR value needed to be doubled from 4 to 8.
> >
> I tested  with below  codes to set the cpu_dai  :
> 
> format = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF |
>                          SND_SOC_DAIFMT_CBS_CFS | PXA_SSP_FRM_64FS;

Ah - the PXA_SSP_FRM_64FS is the detail I missed. With that option set,
my system works well, so I'm generally fine with the changes.

However, I proposed a generic way (not pxa/ssp specific) to let the
format flags carry the information about such frame format details some
months ago and the approach was rejected because there are already too
many ways to let the DAI know about this. In particular, the DIV_SCR
(serial clock rate divider) is used to propagate the base frequency
ratio and together with other options, you can find out the desired
frame format.

In my implementation, the '32 bit over 64fs case' was detected with this
condition in pxa_ssp_hw_params():

  (scr == 4) && (snd_pcm_format_physical_width(params_format(params)) == 16)

Now, SCR isn't used at all any more but instead PXA_SSP_FRM_WIDTH()
handles the case. Eventually this might be a matter of taste, but if we
go this way, why shouldn't that definitions be in ASoC's generic
headers?

Anyway, the code looks much cleaner now :)

Thanks,
Daniel

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[Mark Brown]

On Fri, Jun 05, 2009 at 07:26:30PM +0200, Daniel Mack wrote:

> Ah - the PXA_SSP_FRM_64FS is the detail I missed. With that option set,
> my system works well, so I'm generally fine with the changes.
> 
> However, I proposed a generic way (not pxa/ssp specific) to let the
> format flags carry the information about such frame format details some
> months ago and the approach was rejected because there are already too
> many ways to let the DAI know about this.

Indeed, and there's some potential lack of clarity with the interaction
of settings in the DAI format with these other settings.

>                                           In particular, the DIV_SCR
> (serial clock rate divider) is used to propagate the base frequency
> ratio and together with other options, you can find out the desired
> frame format.

There's that, and there's also the TDM settings which provide
essentially the same information - an overclocked bit clock is
equivalent to a TDM mode where the first slot is in use.  It's not
immediately clear to me what a combination of the two options ought to
do.

It's also a logically different bit of configuration - for most devices
it only makes a difference to the device that's generating the clocks
and wouldn't need to be specified for the other devices since they'd
just ignore the extra clocks.  The DAI format specifies the format for
the clocks, primarily the frame clocks, but does not currently handle
their rates.

> handles the case. Eventually this might be a matter of taste, but if we
> go this way, why shouldn't that definitions be in ASoC's generic
> headers?

Definitely.

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[Mark Brown]

On Wed, Jun 03, 2009 at 08:33:42PM +0800, Eric Miao wrote:
> Make the pxa I2S configuration generic, add support for Left_J, add
> support for variable frame width like 32fs, 48fs, 64fs and 96fs

I'm OK with this from a code review point of view providing Daniel is
happy from a testing point of view.  I'll need to fire up my Zylonite
and test on that as well - I'll update the machine driver as required
assuming everything works OK.

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[Daniel Mack]

On Thu, Jun 04, 2009 at 01:36:32PM +0100, Mark Brown wrote:
> On Wed, Jun 03, 2009 at 08:33:42PM +0800, Eric Miao wrote:
> > Make the pxa I2S configuration generic, add support for Left_J, add
> > support for variable frame width like 32fs, 48fs, 64fs and 96fs
> 
> I'm OK with this from a code review point of view providing Daniel is
> happy from a testing point of view.  I'll need to fire up my Zylonite
> and test on that as well - I'll update the machine driver as required
> assuming everything works OK.

I'll need some time to test that. Hope to get back to you soon.

Daniel

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[Daniel Ribeiro]

[-- Attachment #1.1: Type: text/plain, Size: 6599 bytes --]

Em Qua, 2009-06-03 às 20:33 +0800, Eric Miao escreveu:
> Make the pxa I2S configuration generic, add support for Left_J, add
> support for variable frame width like 32fs, 48fs, 64fs and 96fs
> 
> Signed-off-by: Paul Shen <bshen9@marvell.com>
> Signed-off-by: Eric Miao <eric.miao@marvell.com>
> Cc: Daniel Mack <daniel@caiaq.de>
> ---
>  arch/arm/mach-pxa/include/mach/regs-ssp.h |   14 +++---
>  sound/soc/pxa/pxa-ssp.c                   |   62 ++++++++++++++--------------
>  sound/soc/pxa/pxa-ssp.h                   |    9 ++++
>  3 files changed, 47 insertions(+), 38 deletions(-)
> 
> diff --git a/arch/arm/mach-pxa/include/mach/regs-ssp.h
> b/arch/arm/mach-pxa/include/mach/regs-ssp.h
> index 6a2ed35..27f0cd4 100644
> --- a/arch/arm/mach-pxa/include/mach/regs-ssp.h
> +++ b/arch/arm/mach-pxa/include/mach/regs-ssp.h
> @@ -108,21 +108,21 @@
>  #define SSSR_TINT		(1 << 19)	/* Receiver Time-out Interrupt */
>  #define SSSR_PINT		(1 << 18)	/* Peripheral Trailing Byte Interrupt */
> 
> -#if defined(CONFIG_PXA3xx)
> -#define SSPSP_EDMYSTOP(x)	((x) << 28)     /* Extended Dummy Stop */
> -#define SSPSP_EDMYSTRT(x)	((x) << 26)     /* Extended Dummy Start */
> -#endif
> -
>  #define SSPSP_FSRT		(1 << 25)	/* Frame Sync Relative Timing */
> -#define SSPSP_DMYSTOP(x)	((x) << 23)	/* Dummy Stop */
>  #define SSPSP_SFRMWDTH(x)	((x) << 16)	/* Serial Frame Width */
>  #define SSPSP_SFRMDLY(x)	((x) << 9)	/* Serial Frame Delay */
> -#define SSPSP_DMYSTRT(x)	((x) << 7)	/* Dummy Start */
>  #define SSPSP_STRTDLY(x)	((x) << 4)	/* Start Delay */
>  #define SSPSP_ETDS		(1 << 3)	/* End of Transfer data State */
>  #define SSPSP_SFRMP		(1 << 2)	/* Serial Frame Polarity */
>  #define SSPSP_SCMODE(x)		((x) << 0)	/* Serial Bit Rate Clock Mode */
> 
> +/* NOTE: PXA3xx extends the bit number of dummy start and stop, the macros
> + * below are compatible with PXA25x/27x as long as the parameter is within
> + * the correct limits, driver code has to take care of this.
> + */
> +#define SSPSP_DMYSTRT(x)	((((x) & 3) << 7)  | ((((x) >> 2) & 3) << 26))
> +#define SSPSP_DMYSTOP(x)	((((x) & 3) << 23) | ((((x) >> 2) & 7) << 28))
> +
>  #define SSACD_SCDB		(1 << 3)	/* SSPSYSCLK Divider Bypass */
>  #define SSACD_ACPS(x)		((x) << 4)	/* Audio clock PLL select */
>  #define SSACD_ACDS(x)		((x) << 0)	/* Audio clock divider select */
> diff --git a/sound/soc/pxa/pxa-ssp.c b/sound/soc/pxa/pxa-ssp.c
> index 6fc7876..2831c16 100644
> --- a/sound/soc/pxa/pxa-ssp.c
> +++ b/sound/soc/pxa/pxa-ssp.c
> @@ -463,7 +463,8 @@ static int pxa_ssp_set_dai_fmt(struct snd_soc_dai *cpu_dai,
> 
>  	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
>  	case SND_SOC_DAIFMT_I2S:
> -		sscr0 |= SSCR0_PSP;
> +	case SND_SOC_DAIFMT_LEFT_J:
> +		sscr0 |= SSCR0_PSP | SSCR0_MOD;

Why do you enforce network mode here?

>  		sscr1 |= SSCR1_RWOT | SSCR1_TRAIL;
> 
>  		/* See hw_params() */
> @@ -541,6 +542,7 @@ static int pxa_ssp_hw_params(struct
> snd_pcm_substream *substream,
>  	int chn = params_channels(params);
>  	u32 sscr0;
>  	u32 sspsp;
> +	int frame_width;
>  	int width = snd_pcm_format_physical_width(params_format(params));
>  	int ttsa = ssp_read_reg(ssp, SSTSA) & 0xf;
> 
> @@ -585,40 +587,38 @@ static int pxa_ssp_hw_params(struct
> snd_pcm_substream *substream,
> 
>  	switch (priv->dai_fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
>  	case SND_SOC_DAIFMT_I2S:
> -	       sspsp = ssp_read_reg(ssp, SSPSP);
> -
> -		if ((ssp_get_scr(ssp) == 4) && (width == 16)) {
> -			/* This is a special case where the bitclk is 64fs
> -			* and we're not dealing with 2*32 bits of audio
> -			* samples.
> -			*
> -			* The SSP values used for that are all found out by
> -			* trying and failing a lot; some of the registers
> -			* needed for that mode are only available on PXA3xx.
> -			*/
> +		sspsp = ssp_read_reg(ssp, SSPSP);
> +		frame_width = PXA_SSP_FRM_WIDTH(priv->dai_fmt);

I would expect FRM_WIDTH to also change SSCR0_EDSS and SSCR0_DataSize

> 
> -#ifdef CONFIG_PXA3xx
> -			if (!cpu_is_pxa3xx())
> -				return -EINVAL;
> -
> -			sspsp |= SSPSP_SFRMWDTH(width * 2);
> -			sspsp |= SSPSP_SFRMDLY(width * 4);
> -			sspsp |= SSPSP_EDMYSTOP(3);
> -			sspsp |= SSPSP_DMYSTOP(3);
> -			sspsp |= SSPSP_DMYSTRT(1);
> -#else
> +		if (frame_width < width * 2)
>  			return -EINVAL;
> -#endif
> -		} else {
> -			/* The frame width is the width the LRCLK is
> -			 * asserted for; the delay is expressed in
> -			 * half cycle units.  We need the extra cycle
> -			 * because the data starts clocking out one BCLK
> -			 * after LRCLK changes polarity.
> +
> +		if (frame_width == width * 2)
> +			/* frame width is exactly double of data sample width,
> +			 * use FSRT instead
>  			 */
> -			sspsp |= SSPSP_SFRMWDTH(width + 1);
> -			sspsp |= SSPSP_SFRMDLY((width + 1) * 2);
> +			sspsp |= SSPSP_FSRT | SSPSP_SFRMWDTH(width);
> +		else {
>  			sspsp |= SSPSP_DMYSTRT(1);
> +			sspsp |= SSPSP_DMYSTOP((frame_width / 2 - width - 1));
> +			sspsp |= SSPSP_SFRMWDTH(frame_width / 2);
> +		}
> +
> +		ssp_write_reg(ssp, SSPSP, sspsp);
> +		break;
> +
> +	case SND_SOC_DAIFMT_LEFT_J:
> +		sspsp = ssp_read_reg(ssp, SSPSP);
> +		frame_width = PXA_SSP_FRM_WIDTH(priv->dai_fmt);
> +
> +		if (frame_width < width * 2)
> +			return -EINVAL;
> +
> +		if (frame_width == width * 2)
> +			sspsp |= SSPSP_SFRMWDTH(width);
> +		else {
> +			sspsp |= SSPSP_DMYSTOP((frame_width / 2 - width));
> +			sspsp |= SSPSP_SFRMWDTH(frame_width / 2);
>  		}
> 
>  		ssp_write_reg(ssp, SSPSP, sspsp);
> diff --git a/sound/soc/pxa/pxa-ssp.h b/sound/soc/pxa/pxa-ssp.h
> index 91deadd..fda51d0 100644
> --- a/sound/soc/pxa/pxa-ssp.h
> +++ b/sound/soc/pxa/pxa-ssp.h
> @@ -40,6 +40,15 @@
>  #define PXA_SSP_CLK_SCDB_1		1
>  #define PXA_SSP_CLK_SCDB_8		2
> 
> +/* frame size definitions for I2S and Left_J formats - default is
> + * 32fs, other possibilities are 48fs, 64fs and 96fs
> + */
> +#define PXA_SSP_FRM_32FS	(0 << 16)
> +#define PXA_SSP_FRM_48FS	(1 << 16)
> +#define PXA_SSP_FRM_64FS	(2 << 16)
> +#define PXA_SSP_FRM_96FS	(3 << 16)
> +#define PXA_SSP_FRM_WIDTH(x)	(((((x) >> 16) & 0x3) + 2) << 4)
> +
>  #define PXA_SSP_PLL_OUT  0
> 
>  extern struct snd_soc_dai pxa_ssp_dai[4];

I am testing this patch with PXA272 slave of clock and frame,
DAIFMT_LEFT_J, tdm_slot(3,2), and it causes my audio to play with double
speed. (with tdm_slot(1,1) it plays at half speed).

Values that are known to work fine for my board are:
SSCR0 = 0x1000bf
SSPSP = 0x100002


-- 
Daniel Ribeiro

[-- Attachment #1.2: Esta é uma parte de mensagem assinada digitalmente --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[Paul Shen]

Hi Daniel,

Would you please give some informations about your platform ?
Thus I can test the patches with your method.

2009/6/6 Daniel Ribeiro <drwyrm@gmail.com>:
> Em Qua, 2009-06-03 às 20:33 +0800, Eric Miao escreveu:
>> Make the pxa I2S configuration generic, add support for Left_J, add
>> support for variable frame width like 32fs, 48fs, 64fs and 96fs
>>
>> Signed-off-by: Paul Shen <bshen9@marvell.com>
>> Signed-off-by: Eric Miao <eric.miao@marvell.com>
>> Cc: Daniel Mack <daniel@caiaq.de>
>> ---
>>  arch/arm/mach-pxa/include/mach/regs-ssp.h |   14 +++---
>>  sound/soc/pxa/pxa-ssp.c                   |   62 ++++++++++++++--------------
>>  sound/soc/pxa/pxa-ssp.h                   |    9 ++++
>>  3 files changed, 47 insertions(+), 38 deletions(-)
>>
>> diff --git a/arch/arm/mach-pxa/include/mach/regs-ssp.h
>> b/arch/arm/mach-pxa/include/mach/regs-ssp.h
>> index 6a2ed35..27f0cd4 100644
>> --- a/arch/arm/mach-pxa/include/mach/regs-ssp.h
>> +++ b/arch/arm/mach-pxa/include/mach/regs-ssp.h
>> @@ -108,21 +108,21 @@
>>  #define SSSR_TINT            (1 << 19)       /* Receiver Time-out Interrupt */
>>  #define SSSR_PINT            (1 << 18)       /* Peripheral Trailing Byte Interrupt */
>>
>> -#if defined(CONFIG_PXA3xx)
>> -#define SSPSP_EDMYSTOP(x)    ((x) << 28)     /* Extended Dummy Stop */
>> -#define SSPSP_EDMYSTRT(x)    ((x) << 26)     /* Extended Dummy Start */
>> -#endif
>> -
>>  #define SSPSP_FSRT           (1 << 25)       /* Frame Sync Relative Timing */
>> -#define SSPSP_DMYSTOP(x)     ((x) << 23)     /* Dummy Stop */
>>  #define SSPSP_SFRMWDTH(x)    ((x) << 16)     /* Serial Frame Width */
>>  #define SSPSP_SFRMDLY(x)     ((x) << 9)      /* Serial Frame Delay */
>> -#define SSPSP_DMYSTRT(x)     ((x) << 7)      /* Dummy Start */
>>  #define SSPSP_STRTDLY(x)     ((x) << 4)      /* Start Delay */
>>  #define SSPSP_ETDS           (1 << 3)        /* End of Transfer data State */
>>  #define SSPSP_SFRMP          (1 << 2)        /* Serial Frame Polarity */
>>  #define SSPSP_SCMODE(x)              ((x) << 0)      /* Serial Bit Rate Clock Mode */
>>
>> +/* NOTE: PXA3xx extends the bit number of dummy start and stop, the macros
>> + * below are compatible with PXA25x/27x as long as the parameter is within
>> + * the correct limits, driver code has to take care of this.
>> + */
>> +#define SSPSP_DMYSTRT(x)     ((((x) & 3) << 7)  | ((((x) >> 2) & 3) << 26))
>> +#define SSPSP_DMYSTOP(x)     ((((x) & 3) << 23) | ((((x) >> 2) & 7) << 28))
>> +
>>  #define SSACD_SCDB           (1 << 3)        /* SSPSYSCLK Divider Bypass */
>>  #define SSACD_ACPS(x)                ((x) << 4)      /* Audio clock PLL select */
>>  #define SSACD_ACDS(x)                ((x) << 0)      /* Audio clock divider select */
>> diff --git a/sound/soc/pxa/pxa-ssp.c b/sound/soc/pxa/pxa-ssp.c
>> index 6fc7876..2831c16 100644
>> --- a/sound/soc/pxa/pxa-ssp.c
>> +++ b/sound/soc/pxa/pxa-ssp.c
>> @@ -463,7 +463,8 @@ static int pxa_ssp_set_dai_fmt(struct snd_soc_dai *cpu_dai,
>>
>>       switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
>>       case SND_SOC_DAIFMT_I2S:
>> -             sscr0 |= SSCR0_PSP;
>> +     case SND_SOC_DAIFMT_LEFT_J:
>> +             sscr0 |= SSCR0_PSP | SSCR0_MOD;
>
> Why do you enforce network mode here?
>
>>               sscr1 |= SSCR1_RWOT | SSCR1_TRAIL;
>>
>>               /* See hw_params() */
>> @@ -541,6 +542,7 @@ static int pxa_ssp_hw_params(struct
>> snd_pcm_substream *substream,
>>       int chn = params_channels(params);
>>       u32 sscr0;
>>       u32 sspsp;
>> +     int frame_width;
>>       int width = snd_pcm_format_physical_width(params_format(params));
>>       int ttsa = ssp_read_reg(ssp, SSTSA) & 0xf;
>>
>> @@ -585,40 +587,38 @@ static int pxa_ssp_hw_params(struct
>> snd_pcm_substream *substream,
>>
>>       switch (priv->dai_fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
>>       case SND_SOC_DAIFMT_I2S:
>> -            sspsp = ssp_read_reg(ssp, SSPSP);
>> -
>> -             if ((ssp_get_scr(ssp) == 4) && (width == 16)) {
>> -                     /* This is a special case where the bitclk is 64fs
>> -                     * and we're not dealing with 2*32 bits of audio
>> -                     * samples.
>> -                     *
>> -                     * The SSP values used for that are all found out by
>> -                     * trying and failing a lot; some of the registers
>> -                     * needed for that mode are only available on PXA3xx.
>> -                     */
>> +             sspsp = ssp_read_reg(ssp, SSPSP);
>> +             frame_width = PXA_SSP_FRM_WIDTH(priv->dai_fmt);
>
> I would expect FRM_WIDTH to also change SSCR0_EDSS and SSCR0_DataSize
>
>>
>> -#ifdef CONFIG_PXA3xx
>> -                     if (!cpu_is_pxa3xx())
>> -                             return -EINVAL;
>> -
>> -                     sspsp |= SSPSP_SFRMWDTH(width * 2);
>> -                     sspsp |= SSPSP_SFRMDLY(width * 4);
>> -                     sspsp |= SSPSP_EDMYSTOP(3);
>> -                     sspsp |= SSPSP_DMYSTOP(3);
>> -                     sspsp |= SSPSP_DMYSTRT(1);
>> -#else
>> +             if (frame_width < width * 2)
>>                       return -EINVAL;
>> -#endif
>> -             } else {
>> -                     /* The frame width is the width the LRCLK is
>> -                      * asserted for; the delay is expressed in
>> -                      * half cycle units.  We need the extra cycle
>> -                      * because the data starts clocking out one BCLK
>> -                      * after LRCLK changes polarity.
>> +
>> +             if (frame_width == width * 2)
>> +                     /* frame width is exactly double of data sample width,
>> +                      * use FSRT instead
>>                        */
>> -                     sspsp |= SSPSP_SFRMWDTH(width + 1);
>> -                     sspsp |= SSPSP_SFRMDLY((width + 1) * 2);
>> +                     sspsp |= SSPSP_FSRT | SSPSP_SFRMWDTH(width);
>> +             else {
>>                       sspsp |= SSPSP_DMYSTRT(1);
>> +                     sspsp |= SSPSP_DMYSTOP((frame_width / 2 - width - 1));
>> +                     sspsp |= SSPSP_SFRMWDTH(frame_width / 2);
>> +             }
>> +
>> +             ssp_write_reg(ssp, SSPSP, sspsp);
>> +             break;
>> +
>> +     case SND_SOC_DAIFMT_LEFT_J:
>> +             sspsp = ssp_read_reg(ssp, SSPSP);
>> +             frame_width = PXA_SSP_FRM_WIDTH(priv->dai_fmt);
>> +
>> +             if (frame_width < width * 2)
>> +                     return -EINVAL;
>> +
>> +             if (frame_width == width * 2)
>> +                     sspsp |= SSPSP_SFRMWDTH(width);
>> +             else {
>> +                     sspsp |= SSPSP_DMYSTOP((frame_width / 2 - width));
>> +                     sspsp |= SSPSP_SFRMWDTH(frame_width / 2);
>>               }
>>
>>               ssp_write_reg(ssp, SSPSP, sspsp);
>> diff --git a/sound/soc/pxa/pxa-ssp.h b/sound/soc/pxa/pxa-ssp.h
>> index 91deadd..fda51d0 100644
>> --- a/sound/soc/pxa/pxa-ssp.h
>> +++ b/sound/soc/pxa/pxa-ssp.h
>> @@ -40,6 +40,15 @@
>>  #define PXA_SSP_CLK_SCDB_1           1
>>  #define PXA_SSP_CLK_SCDB_8           2
>>
>> +/* frame size definitions for I2S and Left_J formats - default is
>> + * 32fs, other possibilities are 48fs, 64fs and 96fs
>> + */
>> +#define PXA_SSP_FRM_32FS     (0 << 16)
>> +#define PXA_SSP_FRM_48FS     (1 << 16)
>> +#define PXA_SSP_FRM_64FS     (2 << 16)
>> +#define PXA_SSP_FRM_96FS     (3 << 16)
>> +#define PXA_SSP_FRM_WIDTH(x) (((((x) >> 16) & 0x3) + 2) << 4)
>> +
>>  #define PXA_SSP_PLL_OUT  0
>>
>>  extern struct snd_soc_dai pxa_ssp_dai[4];
>
> I am testing this patch with PXA272 slave of clock and frame,
> DAIFMT_LEFT_J, tdm_slot(3,2), and it causes my audio to play with double
> speed. (with tdm_slot(1,1) it plays at half speed).
>
> Values that are known to work fine for my board are:
> SSCR0 = 0x1000bf
> SSPSP = 0x100002
>
>
> --
> Daniel Ribeiro
>


Would you please give me code extract about your SSP, codec
configurations and clock setting?

-- 
Paul Shen

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[Daniel Mack]

On Tue, Jun 09, 2009 at 05:39:53PM +0800, Paul Shen wrote:
> Would you please give some informations about your platform ?
> Thus I can test the patches with your method.

Sure, here we go.

We're using the following setup:

 - PXA in master mode to BITCLK and LRCLK but external SSP clock
 - 64fs frame format
 - CS4270 codec

Here's the code sniplet I use to configure the port using your latest
patch set:


	fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF |
		SND_SOC_DAIFMT_CBS_CFS | PXA_SSP_FRM_64FS;

	/* setup the CODEC DAI */
        ret = snd_soc_dai_set_fmt(codec_dai, fmt);
        if (ret < 0)
		return ret;

	ret = snd_soc_dai_set_sysclk(codec_dai, 0, clk, 0);
	if (ret < 0)
		return ret;

	/* setup the CPU DAI */
	ret = snd_soc_dai_set_pll(cpu_dai, 0, 0, clk);
	if (ret < 0)
		return ret;

        ret = snd_soc_dai_set_fmt(cpu_dai, fmt);
        if (ret < 0)
                return ret;

	ret = snd_soc_dai_set_clkdiv(cpu_dai, PXA_SSP_DIV_SCR, 4);
        if (ret < 0)
                return ret;

	ret = snd_soc_dai_set_sysclk(cpu_dai, PXA_SSP_CLK_EXT, 0, 1);
        if (ret < 0)
                return ret;

	snd_soc_dai_set_tdm_slot(cpu_dai, 3, 2);

That gives me the following register values:

	SSCR0 0xa10003ff
	SSCR1 0x00e01d80
	SSPSP 0x31a00084

... which works fine for me. But the PXA is not really in a typical
slave configuration, mostly because we had lots of strange issues when
we tried that.

Any more things I can provide?

Thanks,
Daniel

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[Daniel Ribeiro]

[-- Attachment #1.1: Type: text/plain, Size: 1380 bytes --]

Em Ter, 2009-06-09 às 17:39 +0800, Paul Shen escreveu:
> Hi Daniel,
> 
> Would you please give some informations about your platform ?
> Thus I can test the patches with your method.
...
> Would you please give me code extract about your SSP, codec
> configurations and clock setting?

Hi Paul! :)

Im working to get Motorola EZX Phones[1] supported by mainline linux.

Our audio codec is a proprietary PMIC, manufactured by TI for Motorola,
named PCAP2(PTWL93017).

The audio codec is connected to PXA SSP2, PXA SSP3, Baseband Processor
and BCM20x5(bluetooth) in network mode.

PXA is always the _slave_ of bitclock and frame, so I don't have to care
about clock settings on PXA side, the codec always provides me the
correct clock for the selected rate.

You can find our code on git://git.openezx.org/openezx.git, ezx/current
branch. (sound/soc/pxa/ezx.c and sound/soc/codecs/pcap2.c).

Most of our work is based on the 2.4 kernel source releases by
motorola[2], and on the atlas(MC13783) PMIC documentation[3] from
Freescale (used by Motorola on newer(MAGX) phones), as we don't have
documentation for PCAP2.

[1]http://wiki.openezx.org/Project_devices
[2]https://opensource.motorola.com/
[3]http://www.freescale.com/webapp/sps/site/prod_summary.jsp?code=MC13783&webpageId=1098824671538699514881&nodeId=01435974404881

-- 
Daniel Ribeiro

[-- Attachment #1.2: Esta é uma parte de mensagem assinada digitalmente --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[Mark Brown]

On Wed, Jun 03, 2009 at 08:33:42PM +0800, Eric Miao wrote:

> +/* frame size definitions for I2S and Left_J formats - default is
> + * 32fs, other possibilities are 48fs, 64fs and 96fs
> + */
> +#define PXA_SSP_FRM_32FS	(0 << 16)
> +#define PXA_SSP_FRM_48FS	(1 << 16)
> +#define PXA_SSP_FRM_64FS	(2 << 16)
> +#define PXA_SSP_FRM_96FS	(3 << 16)
> +#define PXA_SSP_FRM_WIDTH(x)	(((((x) >> 16) & 0x3) + 2) << 4)
> +

I still haven't checked this on Zylonite but I wanted to mention these
new DAI format bits just now - as previously discussed I'm not
enthusiastic about this.  As well as the previous issues I raised with
this approach I also meant to mention is that the use of a bitfield will
inevitably restrict what can be expressed, causing real problems for
some systems.  For example, the WM9081 supports systems using up to 3
stereo TDM slots and up to 32 bit samples so could be used in a system
which needs a 192fs bit clock.

If we did decide to adopt this approach then these defines should be in
the ASoC headers rather than private to this driver - apart from
anything else, there's every chance that additions to the standard bits
could end up colliding with this.

I'm still not sure what the best way to handle this is due to the
interaction with TDM mode.  The fact that TDM mode really wants to
always explicitly specify the frame width in order to allow the sample
size in each slot to vary suggests that one option would be to add a
sample width parameter to set_tdm_slot() - given the very small number
of in tree users it'd cause little disruption.  A set_sample_width()
operation could also be added.

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[pHilipp Zabel]

On Sat, Jun 6, 2009 at 10:12 PM, Mark
Brown<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, Jun 03, 2009 at 08:33:42PM +0800, Eric Miao wrote:
>
>> +/* frame size definitions for I2S and Left_J formats - default is
>> + * 32fs, other possibilities are 48fs, 64fs and 96fs
>> + */
>> +#define PXA_SSP_FRM_32FS     (0 << 16)
>> +#define PXA_SSP_FRM_48FS     (1 << 16)
>> +#define PXA_SSP_FRM_64FS     (2 << 16)
>> +#define PXA_SSP_FRM_96FS     (3 << 16)
>> +#define PXA_SSP_FRM_WIDTH(x) (((((x) >> 16) & 0x3) + 2) << 4)
>> +
>
> I still haven't checked this on Zylonite but I wanted to mention these
> new DAI format bits just now - as previously discussed I'm not
> enthusiastic about this.

Agreed. Also, if I had to use this to configure magician with DSP_A
(right now it says I2S and LEFT_J only, but why limit it to that?),
I'd need an additional PXA_SSP_FRM_16FS.

> As well as the previous issues I raised with
> this approach I also meant to mention is that the use of a bitfield will
> inevitably restrict what can be expressed, causing real problems for
> some systems.  For example, the WM9081 supports systems using up to 3
> stereo TDM slots and up to 32 bit samples so could be used in a system
> which needs a 192fs bit clock.
>
> If we did decide to adopt this approach then these defines should be in
> the ASoC headers rather than private to this driver - apart from
> anything else, there's every chance that additions to the standard bits
> could end up colliding with this.
>
> I'm still not sure what the best way to handle this is due to the
> interaction with TDM mode.  The fact that TDM mode really wants to
> always explicitly specify the frame width in order to allow the sample
> size in each slot to vary suggests that one option would be to add a
> sample width parameter to set_tdm_slot() - given the very small number
> of in tree users it'd cause little disruption.  A set_sample_width()
> operation could also be added.

set_sample_width or set_frame_width?
I'd prefer a separate operation over a parameter to set_tdm_slot because
in my setup I just need to force the (SSP) frame width to 16 bits.
Enabling network mode and calling set_tdm_slot(..,1,1) wouldn't be
necessary at all, then.

regards
Philipp

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[Mark Brown]

On Mon, Jun 08, 2009 at 02:12:03PM +0200, pHilipp Zabel wrote:
> On Sat, Jun 6, 2009 at 10:12 PM, Mark
> Brown<broonie@opensource.wolfsonmicro.com> wrote:

OOI which MUA are you using?  I've noticed several people with this very
odd word wrapping the past day.

> > of in tree users it'd cause little disruption. ?A set_sample_width()
> > operation could also be added.

> set_sample_width or set_frame_width?

I'm inclined to go with sample here since it seems harder for the
machine drivers to get wrong but I've not really thought it through yet?

> I'd prefer a separate operation over a parameter to set_tdm_slot because
> in my setup I just need to force the (SSP) frame width to 16 bits.
> Enabling network mode and calling set_tdm_slot(..,1,1) wouldn't be
> necessary at all, then.

OTOH I don't really see much difference between the two cases - it's
just an extra couple of parameters on the end of the call.  That said,
it'd be much nicer if the driver were able to just do the right thing
for DSP mode unless explicitly configured, there's not the issue you
have with I2S where extra clocks would change the offset of the second
channel within a stereo frame so it's much easier.

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[pHilipp Zabel]

On Mon, Jun 8, 2009 at 2:40 PM, Mark
Brown<broonie@opensource.wolfsonmicro.com> wrote:
> On Mon, Jun 08, 2009 at 02:12:03PM +0200, pHilipp Zabel wrote:
>> On Sat, Jun 6, 2009 at 10:12 PM, Mark
>> Brown<broonie@opensource.wolfsonmicro.com> wrote:
>
> OOI which MUA are you using?  I've noticed several people with this very
> odd word wrapping the past day.

This mail was sent with the Google Mail web interface.

>> > of in tree users it'd cause little disruption. ?A set_sample_width()
>> > operation could also be added.
>
>> set_sample_width or set_frame_width?
>
> I'm inclined to go with sample here since it seems harder for the
> machine drivers to get wrong but I've not really thought it through yet?

I thought sample width is determined by the snd_pcm_hw_params. But
maybe I'm mixing up alsa sample width vs sample width on the wire?
I'm leaning towards set_frame_width because that's directly what I
want to do: override pxa_ssp_hw_params' standard decision to use
32-bit frames for S16_LE stereo and set 16-bit frames instead.

>> I'd prefer a separate operation over a parameter to set_tdm_slot because
>> in my setup I just need to force the (SSP) frame width to 16 bits.
>> Enabling network mode and calling set_tdm_slot(..,1,1) wouldn't be
>> necessary at all, then.
>
> OTOH I don't really see much difference between the two cases - it's
> just an extra couple of parameters on the end of the call.

Technically there isn't. It just seems much more obvious to me to
write something like:
    /* nonstandard: 16-bit frames, even for 2x 16-bit stereo */
    if (params_format(params) == ..._S16_LE)
        set_frame_size(16);
in the machine driver instead of:
    /* nonstandard: 16-bit frames, even for 2x 16-bit stereo
     * pxa_ssp_hw_params will check for TTSA==1
     * and then set the frame size accordingly
     */
    set_tdm_slot(1,1);
especially as I don't really need network mode at all.

> That said,
> it'd be much nicer if the driver were able to just do the right thing
> for DSP mode unless explicitly configured, there's not the issue you
> have with I2S where extra clocks would change the offset of the second
> channel within a stereo frame so it's much easier.

Yes. Only for strange nonstandard setups like a flip-flop in the LRCLK
line even DSP mode has to be told that we need two frame clock pulses
per stereo sample.

regards
Philipp

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[Daniel Ribeiro]

[-- Attachment #1.1: Type: text/plain, Size: 590 bytes --]

Em Seg, 2009-06-08 às 17:58 +0200, pHilipp Zabel escreveu:
> I thought sample width is determined by the snd_pcm_hw_params. But
> maybe I'm mixing up alsa sample width vs sample width on the wire?
> I'm leaning towards set_frame_width because that's directly what I
> want to do: override pxa_ssp_hw_params' standard decision to use
> 32-bit frames for S16_LE stereo and set 16-bit frames instead.

Actually, the current code is not doing this. It is using 32bit DMA, but
with 16bit frame size on the wire for S16_LE stereo (unless you setup 2
timeslots).

-- 
Daniel Ribeiro

[-- Attachment #1.2: Esta é uma parte de mensagem assinada digitalmente --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[Mark Brown]

On Mon, Jun 08, 2009 at 05:58:54PM +0200, pHilipp Zabel wrote:
> On Mon, Jun 8, 2009 at 2:40 PM, Mark

> > OOI which MUA are you using?  I've noticed several people with this very
> > odd word wrapping the past day.

> This mail was sent with the Google Mail web interface.

Ah.  Why am I not surprised. :/

> > I'm inclined to go with sample here since it seems harder for the
> > machine drivers to get wrong but I've not really thought it through yet?

> I thought sample width is determined by the snd_pcm_hw_params. But
> maybe I'm mixing up alsa sample width vs sample width on the wire?

Essentially what we're doing here is providing a mechanism to specify a
separate wire format.

> I'm leaning towards set_frame_width because that's directly what I
> want to do: override pxa_ssp_hw_params' standard decision to use
> 32-bit frames for S16_LE stereo and set 16-bit frames instead.

Hrm.  Now I remember what you're doing - you're trying to essentially
send your stereo stream as mono data due to your hardware either having
a flip flop or an entertainingly non-standard CODEC which needs a frame
sync per sample rather than per frame.

> > OTOH I don't really see much difference between the two cases - it's
> > just an extra couple of parameters on the end of the call.

> Technically there isn't. It just seems much more obvious to me to
> write something like:
>     /* nonstandard: 16-bit frames, even for 2x 16-bit stereo */
>     if (params_format(params) == ..._S16_LE)
>         set_frame_size(16);

Thing is, I'd expect this would be just as likely to cause the CPU to
discard every other sample since it doesn't have enough clocks to clock
out a stereo sample in the frame.

It occurs to me that this is something that it might be better to work
around this with a user space plugin which rewrites the sample formats
on the way down to the driver so that it claims the stream is configured
as mono even if it's stereo :/  Not sure how practical that is or if
there's a sensible way to do that in kernel space.

> in the machine driver instead of:
>     /* nonstandard: 16-bit frames, even for 2x 16-bit stereo
>      * pxa_ssp_hw_params will check for TTSA==1
>      * and then set the frame size accordingly
>      */
>     set_tdm_slot(1,1);
> especially as I don't really need network mode at all.

Same issue with "it's a surprise it works" applies here.

That TDM configuration ought to disable network mode if the driver
doesn't need it.

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[pHilipp Zabel]

On Mon, Jun 8, 2009 at 6:38 PM, Mark
Brown<broonie@opensource.wolfsonmicro.com> wrote:
> On Mon, Jun 08, 2009 at 05:58:54PM +0200, pHilipp Zabel wrote:
>> On Mon, Jun 8, 2009 at 2:40 PM, Mark
>
>> > OOI which MUA are you using?  I've noticed several people with this very
>> > odd word wrapping the past day.
>
>> This mail was sent with the Google Mail web interface.
>
> Ah.  Why am I not surprised. :/
>
>> > I'm inclined to go with sample here since it seems harder for the
>> > machine drivers to get wrong but I've not really thought it through yet?
>
>> I thought sample width is determined by the snd_pcm_hw_params. But
>> maybe I'm mixing up alsa sample width vs sample width on the wire?
>
> Essentially what we're doing here is providing a mechanism to specify a
> separate wire format.
>
>> I'm leaning towards set_frame_width because that's directly what I
>> want to do: override pxa_ssp_hw_params' standard decision to use
>> 32-bit frames for S16_LE stereo and set 16-bit frames instead.
>
> Hrm.  Now I remember what you're doing - you're trying to essentially
> send your stereo stream as mono data due to your hardware either having
> a flip flop [...]

Exactly.

>> > OTOH I don't really see much difference between the two cases - it's
>> > just an extra couple of parameters on the end of the call.
>
>> Technically there isn't. It just seems much more obvious to me to
>> write something like:
>>     /* nonstandard: 16-bit frames, even for 2x 16-bit stereo */
>>     if (params_format(params) == ..._S16_LE)
>>         set_frame_size(16);
>
> Thing is, I'd expect this would be just as likely to cause the CPU to
> discard every other sample since it doesn't have enough clocks to clock
> out a stereo sample in the frame.

Ah right, and it would. That's why I set up DMA to only transfer 16
bits in this case. It's the reason for the existance of this snippet
in pxa-ssp:

        /* Network mode with one active slot (ttsa == 1) can be used
         * to force 16-bit frame width on the wire (for S16_LE), even
         * with two channels. Use 16-bit DMA transfers for this case.
         */
        if (((chn == 2) && (ttsa != 1)) || (width == 32))
                dma += 2; /* 32-bit DMA offset is 2, 16-bit is 0 */

Which makes me wonder, whether it wouldn't generally be more accurate
to calculate the DMA transfer size as ssp_framewidth *
number_of_active_slots. Decreasing the DMA transfer size instead of
throwing away half the data (whether it is due to misconfiguration or
due to a strange special case like mine) should be a sane default?

> It occurs to me that this is something that it might be better to work
> around this with a user space plugin which rewrites the sample formats
> on the way down to the driver so that it claims the stream is configured
> as mono even if it's stereo :/  Not sure how practical that is or if
> there's a sensible way to do that in kernel space.

Please, don't do this to me :)
As it works the way it is now, I don't see how moving this to
userspace improves things (except that you'd get rid of the two code
lines quoted above).

>> in the machine driver instead of:
>>     /* nonstandard: 16-bit frames, even for 2x 16-bit stereo
>>      * pxa_ssp_hw_params will check for TTSA==1
>>      * and then set the frame size accordingly
>>      */
>>     set_tdm_slot(1,1);
>> especially as I don't really need network mode at all.
>
> Same issue with "it's a surprise it works" applies here.

See above, reducing the DMA transfer size works as expected.

> That TDM configuration ought to disable network mode if the driver
> doesn't need it.

Ok.

regards
Philipp

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[Mark Brown]

On Mon, Jun 08, 2009 at 07:18:16PM +0200, pHilipp Zabel wrote:
> On Mon, Jun 8, 2009 at 6:38 PM, Mark

> Which makes me wonder, whether it wouldn't generally be more accurate
> to calculate the DMA transfer size as ssp_framewidth *
> number_of_active_slots. Decreasing the DMA transfer size instead of
> throwing away half the data (whether it is due to misconfiguration or
> due to a strange special case like mine) should be a sane default?

Yeah, it's kind of random and depends on what the effect you're trying
to achieve is - do you want to preserve the data or the sample rate?
Neither is immediately obviously the correct thing in the general case.

> > It occurs to me that this is something that it might be better to work
> > around this with a user space plugin which rewrites the sample formats

> Please, don't do this to me :)
> As it works the way it is now, I don't see how moving this to
> userspace improves things (except that you'd get rid of the two code
> lines quoted above).

It's not so much the move to user space as the bit where we're able to
say to the CPU driver "treat this data as a mono stream rather than
stereo" which makes the problem a whole lot easier, I think?  At the
minute user space is the only place that has hooks to do that.

I'll have a think about this and see if I can come up with a clean way
of doing that in kernel.

> >> ? ? set_tdm_slot(1,1);
> >> especially as I don't really need network mode at all.

> > Same issue with "it's a surprise it works" applies here.

> See above, reducing the DMA transfer size works as expected.

Yeah, I'm thinking in terms of the generic interface rather than the
behaviour of the specific hardware - I'd expect something doing I2S
natively would be more likely to fail the other way.

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[pHilipp Zabel]

On Mon, Jun 8, 2009 at 7:41 PM, Mark
Brown<broonie@opensource.wolfsonmicro.com> wrote:
> On Mon, Jun 08, 2009 at 07:18:16PM +0200, pHilipp Zabel wrote:
>> On Mon, Jun 8, 2009 at 6:38 PM, Mark
>
>> Which makes me wonder, whether it wouldn't generally be more accurate
>> to calculate the DMA transfer size as ssp_framewidth *
>> number_of_active_slots. Decreasing the DMA transfer size instead of
>> throwing away half the data (whether it is due to misconfiguration or
>> due to a strange special case like mine) should be a sane default?
>
> Yeah, it's kind of random and depends on what the effect you're trying
> to achieve is - do you want to preserve the data or the sample rate?
> Neither is immediately obviously the correct thing in the general case.

Point taken.

My preference for data preservation stems from the fact that (with
pxa-ssp) I had no direct influence on the DMA transfer width from the
machine driver whereas adjusting the sample rate was easily possible.
It took me quite some time to notice that half the data was gone, but
it was always quite obvious whether the sample rate was too high or
too low. But I certainly hope that this kind of trial-and-error
programming is not the general case :)

>> > It occurs to me that this is something that it might be better to work
>> > around this with a user space plugin which rewrites the sample formats
>
>> Please, don't do this to me :)
>> As it works the way it is now, I don't see how moving this to
>> userspace improves things (except that you'd get rid of the two code
>> lines quoted above).
>
> It's not so much the move to user space as the bit where we're able to
> say to the CPU driver "treat this data as a mono stream rather than
> stereo" which makes the problem a whole lot easier, I think?  At the
> minute user space is the only place that has hooks to do that.
>
> I'll have a think about this and see if I can come up with a clean way
> of doing that in kernel.

Much appreciated.

Although I've still not quite understood why set_frame_width(16)
wouldn't be an acceptable interface. Nobody in their right mind would
do that if they don't mean it.

>> >> ? ? set_tdm_slot(1,1);
>> >> especially as I don't really need network mode at all.
>
>> > Same issue with "it's a surprise it works" applies here.
>
>> See above, reducing the DMA transfer size works as expected.
>
> Yeah, I'm thinking in terms of the generic interface rather than the
> behaviour of the specific hardware - [...]

I know. My POV is a bit limited to pxa-ssp.

regards
Philipp

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[Daniel Ribeiro]

[-- Attachment #1.1: Type: text/plain, Size: 2506 bytes --]

Em Seg, 2009-06-08 às 13:40 +0100, Mark Brown escreveu:
> > set_sample_width or set_frame_width?
> 
> I'm inclined to go with sample here since it seems harder for the
> machine drivers to get wrong but I've not really thought it through yet?

But sample wouldn't cover all cases. eg, magician and ezx uses the same
sample size, but magician needs 16bit frames and ezx 32bits frames.

Just to confirm that I'm understanding the terminology here..
Sample size is SNDRV_PCM_FORMAT_*, and frame size is the actual distance
in clocks between each SSPFRM assertion. Is this correct?

> > I'd prefer a separate operation over a parameter to set_tdm_slot because
> > in my setup I just need to force the (SSP) frame width to 16 bits.
> > Enabling network mode and calling set_tdm_slot(..,1,1) wouldn't be
> > necessary at all, then.
> 
> OTOH I don't really see much difference between the two cases - it's
> just an extra couple of parameters on the end of the call.  That said,
> it'd be much nicer if the driver were able to just do the right thing
> for DSP mode unless explicitly configured, there's not the issue you
> have with I2S where extra clocks would change the offset of the second
> channel within a stereo frame so it's much easier.

I agree with pHilipp, I don't need to set network mode for normal
operation, but i want to be able to use real network mode.

Using set_tdm_slot to setup this also seems awkward, because the dma
configuration to use is also affected by the frame size.


And finally, set_tdm_mode doesn't cover all use cases, I may want to
receive on a timeslot and keep the TX line high impedance, or I may want
to use a different timeslot for TX...

This is used on ezx to record GSM audio. I want the TX line to remain
high impedance, while I sniff BP <-> codec data.

This also brings another issue.. We are enforcing network mode on ALL
modes (with Eric's recent I2S patch), and at the same time we want
RWOT(Receive Without Transmit) always enabled. But the manual says(Table
185): "RWOT must not be used when SSCR0_x[MOD] is set"


My suggestion here is to make a clear distinction between normal mode
and network mode. We should not abuse SSCR0_MOD when we don't need it.
We should not require the machine driver to call set_tdm_slot when it
doesn't need to. And for frame sizes that really need SSCR0_MOD to be
set we should issue a warning telling that network mode was
"automatically" enabled.

-- 
Daniel Ribeiro

[-- Attachment #1.2: Esta é uma parte de mensagem assinada digitalmente --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[Mark Brown]

On Mon, Jun 08, 2009 at 01:03:20PM -0300, Daniel Ribeiro wrote:
> Em Seg, 2009-06-08 às 13:40 +0100, Mark Brown escreveu:

> > I'm inclined to go with sample here since it seems harder for the
> > machine drivers to get wrong but I've not really thought it through yet?

> But sample wouldn't cover all cases. eg, magician and ezx uses the same
> sample size, but magician needs 16bit frames and ezx 32bits frames.

See my reply to Philip - even with frame size his case is going to be
very hard to cover in a standard fashion since it's not clear what to do
when your frame clock is run too fast relative to your bit clock.  On
the receive side a lot of devices are just going to discard incomplete
stereo frames.

> Just to confirm that I'm understanding the terminology here..
> Sample size is SNDRV_PCM_FORMAT_*, and frame size is the actual distance
> in clocks between each SSPFRM assertion. Is this correct?

That's pretty much my understanding - sample size is the number of bits
clocked per mono sample on the wire and frame size is the number of bits
clocked per cycle of the frame clock.

> > OTOH I don't really see much difference between the two cases - it's
> > just an extra couple of parameters on the end of the call.  That said,

> I agree with pHilipp, I don't need to set network mode for normal
> operation, but i want to be able to use real network mode.

Network mode is just a detail of the implementation of the PXA here - it
should not be visible outside the pxa-ssp driver.  Or to put it another
way setting a tdm_slot of 1, 1 ought to result in the same behaviour as
disabling TDM as far as the user is concerned.

> Using set_tdm_slot to setup this also seems awkward, because the dma
> configuration to use is also affected by the frame size.

I'd not expect it to be?

> And finally, set_tdm_mode doesn't cover all use cases, I may want to
> receive on a timeslot and keep the TX line high impedance, or I may want
> to use a different timeslot for TX...

Yes, set_tdm_mode() needs separate RX and TX configuration regardless -
if we added the framing configuration in there we should definitely make
that change at that point.

> My suggestion here is to make a clear distinction between normal mode
> and network mode. We should not abuse SSCR0_MOD when we don't need it.
> We should not require the machine driver to call set_tdm_slot when it
> doesn't need to. And for frame sizes that really need SSCR0_MOD to be
> set we should issue a warning telling that network mode was
> "automatically" enabled.

I'd like to see all these details handled within the driver - knowing if
and when network mode are to be set up is the sort of thing that users
ought to be able to rely on the driver for.

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[Daniel Ribeiro]

[-- Attachment #1.1: Type: text/plain, Size: 2624 bytes --]

Em Seg, 2009-06-08 às 17:53 +0100, Mark Brown escreveu:
> > But sample wouldn't cover all cases. eg, magician and ezx uses the same
> > sample size, but magician needs 16bit frames and ezx 32bits frames.
> 
> See my reply to Philip - even with frame size his case is going to be
> very hard to cover in a standard fashion since it's not clear what to do
> when your frame clock is run too fast relative to your bit clock.  On
> the receive side a lot of devices are just going to discard incomplete
> stereo frames.

I don't see why. If we provide an API to setup the frame size this is
all magician needs.

If we assume that the standard is frame_size = sample_size * channels,
then there is no need for a frame size API at all. But then magician
case will not be supported correctly.

> Network mode is just a detail of the implementation of the PXA here - it
> should not be visible outside the pxa-ssp driver.  Or to put it another
> way setting a tdm_slot of 1, 1 ought to result in the same behaviour as
> disabling TDM as far as the user is concerned.

But for pxa-ssp this is not the current behaviour.

Currently, the code uses frame_size = sample_size unless the machine
driver explicitly calls set_tdm_slots.

I believe that the correct behaviour would be frame_size = channels *
sample size.

Regarding tdm_slot(1, 1), it does not disable network mode, it setups
DMA to 16bits regardless of 2 channels.

> > Using set_tdm_slot to setup this also seems awkward, because the dma
> > configuration to use is also affected by the frame size.
> 
> I'd not expect it to be?

If we want to support magician, then set_frame_size(16) would also setup
DMA for 16bits even for stereo audio.

> > My suggestion here is to make a clear distinction between normal mode
> > and network mode. We should not abuse SSCR0_MOD when we don't need it.
> > We should not require the machine driver to call set_tdm_slot when it
> > doesn't need to. And for frame sizes that really need SSCR0_MOD to be
> > set we should issue a warning telling that network mode was
> > "automatically" enabled.
> 
> I'd like to see all these details handled within the driver - knowing if
> and when network mode are to be set up is the sort of thing that users
> ought to be able to rely on the driver for.

I can cook a patch.. All I need to know is:

Does it needs to support magician non-standard format? Or this will be
handled by userspace?

I think we can support magician case too, we just need to provide the
set_frame_size api that was initially proposed. :)

-- 
Daniel Ribeiro

[-- Attachment #1.2: Esta é uma parte de mensagem assinada digitalmente --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[Mark Brown]

On Mon, Jun 08, 2009 at 02:26:32PM -0300, Daniel Ribeiro wrote:
> Em Seg, 2009-06-08 ??s 17:53 +0100, Mark Brown escreveu:

> > See my reply to Philip - even with frame size his case is going to be
> > very hard to cover in a standard fashion since it's not clear what to do

> I don't see why. If we provide an API to setup the frame size this is
> all magician needs.

That depends on what you do with the excess data in a frame - what
magician needs to do is to set the frame clock up to run at double rate
compared to standard one.  Doing this by specifying the frame size means
ignoring the rate of the incoming data rather than paying attention to
the rate the data is supposed to have and therefore skipping the second
channel but either of those two options would be a reasonable response.

> If we assume that the standard is frame_size = sample_size * channels,
> then there is no need for a frame size API at all. But then magician
> case will not be supported correctly.

Think about TDM mode for a minute here - there a separate configuration
for the sample size on the wire opens the way to using a lower sample
size in a given timeslot than the timeslot supports, reducing the need
for the CPU to rewrite data.  Or to put it another way, I can't see TDM
mode working unless the sample size is constrained to always be exactly
that desired so it seems sensible to have a standard way of doing that.

> > Network mode is just a detail of the implementation of the PXA here - it
> > should not be visible outside the pxa-ssp driver.  Or to put it another
> > way setting a tdm_slot of 1, 1 ought to result in the same behaviour as
> > disabling TDM as far as the user is concerned.

> But for pxa-ssp this is not the current behaviour.

I think we can all agree that the current behaviour is fairly broken,
it's kind of orphaned at the minute since nothing is using it.

> > I'd like to see all these details handled within the driver - knowing if
> > and when network mode are to be set up is the sort of thing that users
> > ought to be able to rely on the driver for.

> I can cook a patch.. All I need to know is:

> Does it needs to support magician non-standard format? Or this will be
> handled by userspace?

I'd rather come up with a cleaner way of configuring the magician case
that's explicit about what it's trying to achieve.  It doesn't need to
be in user space, though.

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[Daniel Ribeiro]

[-- Attachment #1.1: Type: text/plain, Size: 1066 bytes --]

Em Seg, 2009-06-08 às 19:06 +0100, Mark Brown escreveu:
> Think about TDM mode for a minute here - there a separate configuration
> for the sample size on the wire opens the way to using a lower sample
> size in a given timeslot than the timeslot supports, reducing the need
> for the CPU to rewrite data.  Or to put it another way, I can't see TDM
> mode working unless the sample size is constrained to always be exactly
> that desired so it seems sensible to have a standard way of doing that.

Hum... Now I understood this. If i want to use network mode with 2
different codecs, and they differ on the expected frame_size i have to
use the smaller frame_size for both codecs.

And indeed, in this case the best place to setup the frame size would be
on set_tdm_slot().

> I'd rather come up with a cleaner way of configuring the magician case
> that's explicit about what it's trying to achieve.  It doesn't need to
> be in user space, though.

I dont know how to do this other than just changing the frame size... :)

-- 
Daniel Ribeiro

[-- Attachment #1.2: Esta é uma parte de mensagem assinada digitalmente --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[Eric Miao]

2009/6/9 Daniel Ribeiro <drwyrm@gmail.com>:
> Em Seg, 2009-06-08 às 19:06 +0100, Mark Brown escreveu:
>> Think about TDM mode for a minute here - there a separate configuration
>> for the sample size on the wire opens the way to using a lower sample
>> size in a given timeslot than the timeslot supports, reducing the need
>> for the CPU to rewrite data.  Or to put it another way, I can't see TDM
>> mode working unless the sample size is constrained to always be exactly
>> that desired so it seems sensible to have a standard way of doing that.
>
> Hum... Now I understood this. If i want to use network mode with 2
> different codecs, and they differ on the expected frame_size i have to
> use the smaller frame_size for both codecs.
>
> And indeed, in this case the best place to setup the frame size would be
> on set_tdm_slot().
>
>> I'd rather come up with a cleaner way of configuring the magician case
>> that's explicit about what it's trying to achieve.  It doesn't need to
>> be in user space, though.
>
> I dont know how to do this other than just changing the frame size... :)
>

OK, not having enough time to read all this thread, let's make sure
first we are on the same floor:

frame_width  = number of bit clocks per frame
sample_width = number of bits per sample

And the assumption that

frame_width = sample_width * channel ( = LRCLK for I2S )

is no longer (and ever) correct.

E.g. the frame width could be 64fs, meaning a whole frame is consisting
of 64 bitclks - that's saying, sample width from 1 - 64 are possible,
For typical 16-bit sample width I2S format, the envelope for each sample
is 32 bitclks, and offseting by 1 bitclk starts the 16-bit sample.

And the TDM mode is actually special for PXA-SSP to emulate the I2S
protocol, it's no way generic TDM in a common sense. So talking about
set_tdm_slots(), I'd really like to hide into the format setting code
of I2S/Left_J to avoid further confusion.

And I still didn't quite capture the issue of magician. Phillip, do
you have any HW reference to the audio codec and the connection on
magician?
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[Eric Miao]

On Tue, Jun 9, 2009 at 5:39 PM, Eric Miao <eric.y.miao@gmail.com> wrote:
>
>
> 2009/6/9 Daniel Ribeiro <drwyrm@gmail.com>:
> > Em Seg, 2009-06-08 às 19:06 +0100, Mark Brown escreveu:
> >> Think about TDM mode for a minute here - there a separate configuration
> >> for the sample size on the wire opens the way to using a lower sample
> >> size in a given timeslot than the timeslot supports, reducing the need
> >> for the CPU to rewrite data.  Or to put it another way, I can't see TDM
> >> mode working unless the sample size is constrained to always be exactly
> >> that desired so it seems sensible to have a standard way of doing that.
> >
> > Hum... Now I understood this. If i want to use network mode with 2
> > different codecs, and they differ on the expected frame_size i have to
> > use the smaller frame_size for both codecs.
> >
> > And indeed, in this case the best place to setup the frame size would be
> > on set_tdm_slot().
> >
> >> I'd rather come up with a cleaner way of configuring the magician case
> >> that's explicit about what it's trying to achieve.  It doesn't need to
> >> be in user space, though.
> >
> > I dont know how to do this other than just changing the frame size... :)
> >
>
> OK, not having enough time to read all this thread, let's make sure
> first we are on the same floor:
>
> frame_width  = number of bit clocks per frame
> sample_width = number of bits per sample
>
> And the assumption that
>
> frame_width = sample_width * channel ( = LRCLK for I2S )
>
> is no longer (and ever) correct.
>
> E.g. the frame width could be 64fs, meaning a whole frame is consisting
> of 64 bitclks - that's saying, sample width from 1 - 64 are possible,

sorry for the HTML mail, and here is a typo: sample width should be within 1-32

> For typical 16-bit sample width I2S format, the envelope for each sample
> is 32 bitclks, and offseting by 1 bitclk starts the 16-bit sample.
>
> And the TDM mode is actually special for PXA-SSP to emulate the I2S
> protocol, it's no way generic TDM in a common sense. So talking about
> set_tdm_slots(), I'd really like to hide into the format setting code
> of I2S/Left_J to avoid further confusion.
>
> And I still didn't quite capture the issue of magician. Phillip, do
> you have any HW reference to the audio codec and the connection on
> magician?



--
Cheers
- eric
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[Mark Brown]

On Tue, Jun 09, 2009 at 05:39:18PM +0800, Eric Miao wrote:

> OK, not having enough time to read all this thread, let's make sure
> first we are on the same floor:

> frame_width  = number of bit clocks per frame
> sample_width = number of bits per sample

Yes.

> And the assumption that

> frame_width = sample_width * channel ( = LRCLK for I2S )

> is no longer (and ever) correct.

Ish.  That's always been a minimum for most frame formats, though with
programmable ports like the PXA SSP ports I2S normally makes it an exact
requirement.  At the minute the magician is abusing a low frame width to
produce a double rate frame clock but this is an abuse.

> And the TDM mode is actually special for PXA-SSP to emulate the I2S
> protocol, it's no way generic TDM in a common sense. So talking about
> set_tdm_slots(), I'd really like to hide into the format setting code
> of I2S/Left_J to avoid further confusion.

In terms of the ASoC APIs TDM mode should only be required in order to
configure actual TDM.  Ideally the PXA driver should only ever require
set_tdm_slot() for actual TDM configurations and setting up a single
slot for TDM should be equivalent to never having called it at all.

It's use in the PXA code has always been a wart.

> And I still didn't quite capture the issue of magician. Phillip, do
> you have any HW reference to the audio codec and the connection on
> magician?

Essentially his hardware ends up requiring one frame clock per sample in
a stereo stream - ie, a double rate frame clock.  I strongly expect that
it is actually running in a DSP mode with one frame sync per sample
rather than per frame.

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[pHilipp Zabel]

On Tue, Jun 9, 2009 at 11:58 AM, Mark
Brown<broonie@opensource.wolfsonmicro.com> wrote:
> On Tue, Jun 09, 2009 at 05:39:18PM +0800, Eric Miao wrote:
>
>> OK, not having enough time to read all this thread, let's make sure
>> first we are on the same floor:
>
>> frame_width  = number of bit clocks per frame
>> sample_width = number of bits per sample
>
> Yes.
>
>> And the assumption that
>
>> frame_width = sample_width * channel ( = LRCLK for I2S )
>
>> is no longer (and ever) correct.
>
> Ish.  That's always been a minimum for most frame formats, though with
> programmable ports like the PXA SSP ports I2S normally makes it an exact
> requirement.  At the minute the magician is abusing a low frame width to
> produce a double rate frame clock but this is an abuse.
>
>> And the TDM mode is actually special for PXA-SSP to emulate the I2S
>> protocol, it's no way generic TDM in a common sense. So talking about
>> set_tdm_slots(), I'd really like to hide into the format setting code
>> of I2S/Left_J to avoid further confusion.
>
> In terms of the ASoC APIs TDM mode should only be required in order to
> configure actual TDM.  Ideally the PXA driver should only ever require
> set_tdm_slot() for actual TDM configurations and setting up a single
> slot for TDM should be equivalent to never having called it at all.
>
> It's use in the PXA code has always been a wart.
>
>> And I still didn't quite capture the issue of magician. Phillip, do
>> you have any HW reference to the audio codec and the connection on
>> magician?
>
> Essentially his hardware ends up requiring one frame clock per sample in
> a stereo stream - ie, a double rate frame clock.  I strongly expect that
> it is actually running in a DSP mode with one frame sync per sample
> rather than per frame.

Correct. A flip-flop between the PXA frame clock output and the
UDA1380 codec's LRCLK input turns the double rate DSP mode pulses into
an I2S style LRCLK signal.

I don't have any references for this, but the behaviour is consistent
(for example L/R channels are switched if I restart playback after
sending an odd number of samples without powering down first) and
suitable flip-flops appear in the HTC Blueangel list of parts at
http://www2.electronicproducts.com/HTC_XDA_III_Pocket_PC_Phone_-whatsinside-42.aspx.

regards
Philipp

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[Daniel Ribeiro]

[-- Attachment #1.1: Type: text/plain, Size: 1354 bytes --]

Hi Eric,

Em Ter, 2009-06-09 às 17:39 +0800, Eric Miao escreveu:
> frame_width = sample_width * channel ( = LRCLK for I2S )
> 
> is no longer (and ever) correct.
> 
> E.g. the frame width could be 64fs, meaning a whole frame is
> consisting
> of 64 bitclks - that's saying, sample width from 1 - 64 are possible,
> For typical 16-bit sample width I2S format, the envelope for each
> sample
> is 32 bitclks, and offseting by 1 bitclk starts the 16-bit sample.

I'm working on supporting I2S right now, and I found some issues with
this enveloping thing.

First, it doesn't work on pxa2xx. DMYSTOP on pxa2xx is only 0-3.

Second, is enveloping needed at all? The patch you sent supports 32bits
frames for 2*16bits samples or 64bits frames for 2*32bits samples using
FSRT.

We already use FSRT for DSP_A, and if this works on littleton I2S we
should just stick with FSRT (and frame_width = sample_width * channels)
to keep the code simple.

> And the TDM mode is actually special for PXA-SSP to emulate the I2S
> protocol, it's no way generic TDM in a common sense. So talking about
> set_tdm_slots(), I'd really like to hide into the format setting code
> of I2S/Left_J to avoid further confusion.

TDM is needed for frames larger than 32 bits on any dai format. Its not
something specific to I2S.

-- 
Daniel Ribeiro

[-- Attachment #1.2: Esta é uma parte de mensagem assinada digitalmente --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[Mark Brown]

On Wed, Jun 10, 2009 at 07:24:09PM -0300, Daniel Ribeiro wrote:

> Second, is enveloping needed at all? The patch you sent supports 32bits
> frames for 2*16bits samples or 64bits frames for 2*32bits samples using
> FSRT.

There are apparently devices that require 64fs BCLK with I2S even though
they only handle sample sizes up to 16 bits (IIRC Daniel Mack was
working with one).  This is pretty much TDM mode with 2 slots and only
the first one active.

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[Daniel Mack]

On Thu, Jun 11, 2009 at 10:00:45AM +0100, Mark Brown wrote:
> > Second, is enveloping needed at all? The patch you sent supports 32bits
> > frames for 2*16bits samples or 64bits frames for 2*32bits samples using
> > FSRT.
> 
> There are apparently devices that require 64fs BCLK with I2S even though
> they only handle sample sizes up to 16 bits (IIRC Daniel Mack was
> working with one). 

Exactly.

> This is pretty much TDM mode with 2 slots and only
> the first one active.

I'll try the patches you applied and see if it still works. I must admit
I lost track in this thread.

Daniel

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[Eric Miao]

2009/6/11 Daniel Ribeiro <drwyrm@gmail.com>:
> Hi Eric,
>
> Em Ter, 2009-06-09 às 17:39 +0800, Eric Miao escreveu:
>> frame_width = sample_width * channel ( = LRCLK for I2S )
>>
>> is no longer (and ever) correct.
>>
>> E.g. the frame width could be 64fs, meaning a whole frame is
>> consisting
>> of 64 bitclks - that's saying, sample width from 1 - 64 are possible,
>> For typical 16-bit sample width I2S format, the envelope for each
>> sample
>> is 32 bitclks, and offseting by 1 bitclk starts the 16-bit sample.
>
> I'm working on supporting I2S right now, and I found some issues with
> this enveloping thing.
>
> First, it doesn't work on pxa2xx. DMYSTOP on pxa2xx is only 0-3.

Well, there is a dedicated I2S controller on pxa{25x,27x}, so I'd expect
if it's required for DMYSTOP > 3, the HW engineer would just resort to
the I2S controller.

>
> Second, is enveloping needed at all? The patch you sent supports 32bits
> frames for 2*16bits samples or 64bits frames for 2*32bits samples using
> FSRT.
>

There are some codecs requiring this.

> We already use FSRT for DSP_A, and if this works on littleton I2S we
> should just stick with FSRT (and frame_width = sample_width * channels)
> to keep the code simple.
>

I hope so, but the assumption of frame_width == sample_width * 2 should
hold true first.

>> And the TDM mode is actually special for PXA-SSP to emulate the I2S
>> protocol, it's no way generic TDM in a common sense. So talking about
>> set_tdm_slots(), I'd really like to hide into the format setting code
>> of I2S/Left_J to avoid further confusion.
>
> TDM is needed for frames larger than 32 bits on any dai format. Its not
> something specific to I2S.
>

Well, emulating I2S with SSP has to use TDM mode so that a single
frame can include two samples.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[RFC] I2S and LEFT_J (was: ASoC: pxa-ssp: enhance I2S and add Left_J support)

[Daniel Ribeiro]

[-- Attachment #1.1: Type: text/plain, Size: 9460 bytes --]

Em Qui, 2009-06-11 às 21:34 +0800, Eric Miao escreveu:
> > We already use FSRT for DSP_A, and if this works on littleton I2S we
> > should just stick with FSRT (and frame_width = sample_width * channels)
> > to keep the code simple.
> >
> 
> I hope so, but the assumption of frame_width == sample_width * 2 should
> hold true first.

Ok, here is what I think that should work for I2S after my 2 patches to
sort the TDM thing.

2 Patches are inline, first version assumes that frame_width =
sample_width * 2(channels), and just increases the SFRM duration to
emulate the LRCLK.

Second version uses Eric and Paul code only for pxa3xx. In this version,
frame_width = sample_width * 2(channels) * 2(envelope).

This was only compile tested, I dont have PXA3XX hardware to test this.
It applies after the 3 patches I sent earlier.

First version:
---
 sound/soc/pxa/pxa-ssp.c |   76 ++++++++++++++++-------------------------------
 1 files changed, 26 insertions(+), 50 deletions(-)

diff --git a/sound/soc/pxa/pxa-ssp.c b/sound/soc/pxa/pxa-ssp.c
index 7e72c41..631eca4 100644
--- a/sound/soc/pxa/pxa-ssp.c
+++ b/sound/soc/pxa/pxa-ssp.c
@@ -487,17 +487,14 @@ static int pxa_ssp_set_dai_fmt(struct snd_soc_dai *cpu_dai,
 	}
 
 	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
-	case SND_SOC_DAIFMT_I2S:
-		sscr0 |= SSCR0_PSP;
-		sscr1 |= SSCR1_RWOT | SSCR1_TRAIL;
-		/* See hw_params() */
-		break;
-
 	case SND_SOC_DAIFMT_DSP_A:
+	case SND_SOC_DAIFMT_I2S:
 		sspsp |= SSPSP_FSRT;
 	case SND_SOC_DAIFMT_DSP_B:
+	case SND_SOC_DAIFMT_LEFT_J:
 		sscr0 |= SSCR0_PSP;
 		sscr1 |= SSCR1_TRAIL | SSCR1_RWOT;
+		/* See hw_params() for I2S and LEFT_J */
 		break;
 
 	default:
@@ -565,6 +562,29 @@ static int pxa_ssp_hw_params(struct snd_pcm_substream *substream,
 		sscr0 |= SSCR0_FPCKE;
 #endif
 
+	switch (priv->dai_fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
+	case SND_SOC_DAIFMT_I2S:
+	case SND_SOC_DAIFMT_LEFT_J:
+		/*
+		 * We can't support network mode with I2S or LEFT_J,
+		 * SSPFRM is asserted only for the first slot.
+		 */
+		if (frame_width == 0 || chn > 2)
+			return -EINVAL;
+
+		/*
+		 * I2S and LEFT_J are stereo only, we have to send data for
+		 * both channels.
+		 */
+		if (chn == 1)
+			frame_width *= 2;
+
+		sspsp |= SSPSP_SFRMWDTH(frame_width / 2);
+		break;
+	default:
+		break;
+	}
+
 	if (frame_width > 0) {
 		/* Not using network mode */
 		if (frame_width > 16)
@@ -602,50 +622,6 @@ static int pxa_ssp_hw_params(struct snd_pcm_substream *substream,
 
 	ssp_write_reg(ssp, SSCR0, sscr0);
 
-	switch (priv->dai_fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
-	case SND_SOC_DAIFMT_I2S:
-	       sspsp = ssp_read_reg(ssp, SSPSP);
-
-		if ((ssp_get_scr(ssp) == 4) && (width == 16)) {
-			/* This is a special case where the bitclk is 64fs
-			* and we're not dealing with 2*32 bits of audio
-			* samples.
-			*
-			* The SSP values used for that are all found out by
-			* trying and failing a lot; some of the registers
-			* needed for that mode are only available on PXA3xx.
-			*/
-
-#ifdef CONFIG_PXA3xx
-			if (!cpu_is_pxa3xx())
-				return -EINVAL;
-
-			sspsp |= SSPSP_SFRMWDTH(width * 2);
-			sspsp |= SSPSP_SFRMDLY(width * 4);
-			sspsp |= SSPSP_EDMYSTOP(3);
-			sspsp |= SSPSP_DMYSTOP(3);
-			sspsp |= SSPSP_DMYSTRT(1);
-#else
-			return -EINVAL;
-#endif
-		} else {
-			/* The frame width is the width the LRCLK is
-			 * asserted for; the delay is expressed in
-			 * half cycle units.  We need the extra cycle
-			 * because the data starts clocking out one BCLK
-			 * after LRCLK changes polarity.
-			 */
-			sspsp |= SSPSP_SFRMWDTH(width + 1);
-			sspsp |= SSPSP_SFRMDLY((width + 1) * 2);
-			sspsp |= SSPSP_DMYSTRT(1);
-		}
-
-		ssp_write_reg(ssp, SSPSP, sspsp);
-		break;
-	default:
-		break;
-	}
-
 	dump_registers(ssp);
 
 	return 0;
-- 
tg: (99ef28c..) asoc/leftj-and-i2s (depends on: asoc/ssp-internals)


Second version:
---
 arch/arm/mach-pxa/include/mach/regs-ssp.h |   14 ++--
 sound/soc/pxa/pxa-ssp.c                   |   99 ++++++++++++++---------------
 2 files changed, 56 insertions(+), 57 deletions(-)

diff --git a/arch/arm/mach-pxa/include/mach/regs-ssp.h b/arch/arm/mach-pxa/include/mach/regs-ssp.h
index 6a2ed35..060e23b 100644
--- a/arch/arm/mach-pxa/include/mach/regs-ssp.h
+++ b/arch/arm/mach-pxa/include/mach/regs-ssp.h
@@ -108,21 +108,21 @@
 #define SSSR_TINT		(1 << 19)	/* Receiver Time-out Interrupt */
 #define SSSR_PINT		(1 << 18)	/* Peripheral Trailing Byte Interrupt */
 
-#if defined(CONFIG_PXA3xx)
-#define SSPSP_EDMYSTOP(x)	((x) << 28)     /* Extended Dummy Stop */
-#define SSPSP_EDMYSTRT(x)	((x) << 26)     /* Extended Dummy Start */
-#endif
-
 #define SSPSP_FSRT		(1 << 25)	/* Frame Sync Relative Timing */
-#define SSPSP_DMYSTOP(x)	((x) << 23)	/* Dummy Stop */
 #define SSPSP_SFRMWDTH(x)	((x) << 16)	/* Serial Frame Width */
 #define SSPSP_SFRMDLY(x)	((x) << 9)	/* Serial Frame Delay */
-#define SSPSP_DMYSTRT(x)	((x) << 7)	/* Dummy Start */
 #define SSPSP_STRTDLY(x)	((x) << 4)	/* Start Delay */
 #define SSPSP_ETDS		(1 << 3)	/* End of Transfer data State */
 #define SSPSP_SFRMP		(1 << 2)	/* Serial Frame Polarity */
 #define SSPSP_SCMODE(x)		((x) << 0)	/* Serial Bit Rate Clock Mode */
 
+/* NOTE: PXA3xx extends the bit number of dummy start and stop, the macros
+ * below are compatible with PXA25x/27x as long as the parameter is within
+ * the correct limits, driver code has to take care of this.
+ */
+#define SSPSP_DMYSTRT(x)       ((((x) & 3) << 7)  | ((((x) >> 2) & 3) << 26))
+#define SSPSP_DMYSTOP(x)       ((((x) & 3) << 23) | ((((x) >> 2) & 7) << 28))
+
 #define SSACD_SCDB		(1 << 3)	/* SSPSYSCLK Divider Bypass */
 #define SSACD_ACPS(x)		((x) << 4)	/* Audio clock PLL select */
 #define SSACD_ACDS(x)		((x) << 0)	/* Audio clock divider select */
diff --git a/sound/soc/pxa/pxa-ssp.c b/sound/soc/pxa/pxa-ssp.c
index 7e72c41..a14ce77 100644
--- a/sound/soc/pxa/pxa-ssp.c
+++ b/sound/soc/pxa/pxa-ssp.c
@@ -487,17 +487,14 @@ static int pxa_ssp_set_dai_fmt(struct snd_soc_dai *cpu_dai,
 	}
 
 	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
-	case SND_SOC_DAIFMT_I2S:
-		sscr0 |= SSCR0_PSP;
-		sscr1 |= SSCR1_RWOT | SSCR1_TRAIL;
-		/* See hw_params() */
-		break;
-
 	case SND_SOC_DAIFMT_DSP_A:
 		sspsp |= SSPSP_FSRT;
 	case SND_SOC_DAIFMT_DSP_B:
+	case SND_SOC_DAIFMT_LEFT_J:
+	case SND_SOC_DAIFMT_I2S:
 		sscr0 |= SSCR0_PSP;
 		sscr1 |= SSCR1_TRAIL | SSCR1_RWOT;
+		/* See hw_params() for I2S and LEFT_J */
 		break;
 
 	default:
@@ -565,6 +562,52 @@ static int pxa_ssp_hw_params(struct snd_pcm_substream *substream,
 		sscr0 |= SSCR0_FPCKE;
 #endif
 
+	switch (priv->dai_fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
+	case SND_SOC_DAIFMT_I2S:
+		/*
+		 * We can't support network mode with I2S or LEFT_J,
+		 * SSPFRM is asserted only for the first slot.
+		 */
+		if (frame_width == 0 || chn > 2)
+			return -EINVAL;
+
+		/*
+		 * I2S and LEFT_J are stereo only, we have to send data for
+		 * both channels.
+		 */
+		if (chn == 1)
+			frame_width *= 2;
+
+		/* For pxa2xx we have to stick with FSRT */
+		if (cpu_is_pxa25x() || cpu_is_pxa27x())
+			sspsp |= SSPSP_FSRT;
+
+		/* For pxa3xx we use Paul's code */
+		if (cpu_is_pxa3xx()) {
+			/* We double the frame_width to envelope the sample */
+			frame_width *= 2;
+
+			sspsp |= SSPSP_DMYSTRT(1);
+			sspsp |= SSPSP_DMYSTOP(frame_width / 2 - width - 1);
+			sspsp |= SSPSP_SFRMWDTH(frame_width / 2);
+		}
+
+		break;
+
+	case SND_SOC_DAIFMT_LEFT_J:
+		if (frame_width == 0 || chn > 2)
+			return -EINVAL;
+
+		if (chn == 1)
+			frame_width *= 2;
+
+		/* No need to envelope the frame for LEFT_J */
+		sspsp |= SSPSP_SFRMWDTH(frame_width / 2);
+		break;
+	default:
+		break;
+	}
+
 	if (frame_width > 0) {
 		/* Not using network mode */
 		if (frame_width > 16)
@@ -602,50 +645,6 @@ static int pxa_ssp_hw_params(struct snd_pcm_substream *substream,
 
 	ssp_write_reg(ssp, SSCR0, sscr0);
 
-	switch (priv->dai_fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
-	case SND_SOC_DAIFMT_I2S:
-	       sspsp = ssp_read_reg(ssp, SSPSP);
-
-		if ((ssp_get_scr(ssp) == 4) && (width == 16)) {
-			/* This is a special case where the bitclk is 64fs
-			* and we're not dealing with 2*32 bits of audio
-			* samples.
-			*
-			* The SSP values used for that are all found out by
-			* trying and failing a lot; some of the registers
-			* needed for that mode are only available on PXA3xx.
-			*/
-
-#ifdef CONFIG_PXA3xx
-			if (!cpu_is_pxa3xx())
-				return -EINVAL;
-
-			sspsp |= SSPSP_SFRMWDTH(width * 2);
-			sspsp |= SSPSP_SFRMDLY(width * 4);
-			sspsp |= SSPSP_EDMYSTOP(3);
-			sspsp |= SSPSP_DMYSTOP(3);
-			sspsp |= SSPSP_DMYSTRT(1);
-#else
-			return -EINVAL;
-#endif
-		} else {
-			/* The frame width is the width the LRCLK is
-			 * asserted for; the delay is expressed in
-			 * half cycle units.  We need the extra cycle
-			 * because the data starts clocking out one BCLK
-			 * after LRCLK changes polarity.
-			 */
-			sspsp |= SSPSP_SFRMWDTH(width + 1);
-			sspsp |= SSPSP_SFRMDLY((width + 1) * 2);
-			sspsp |= SSPSP_DMYSTRT(1);
-		}
-
-		ssp_write_reg(ssp, SSPSP, sspsp);
-		break;
-	default:
-		break;
-	}
-
 	dump_registers(ssp);
 
 	return 0;
-- 
tg: (99ef28c..) asoc/leftj-and-i2s (depends on: asoc/ssp-internals)


-- 
Daniel Ribeiro

[-- Attachment #1.2: Esta é uma parte de mensagem assinada digitalmente --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [RFC] I2S and LEFT_J (was: ASoC: pxa-ssp: enhance I2S and add Left_J support)

[Eric Miao]

2009/6/11 Daniel Ribeiro <drwyrm@gmail.com>:
> Em Qui, 2009-06-11 às 21:34 +0800, Eric Miao escreveu:
>> > We already use FSRT for DSP_A, and if this works on littleton I2S we
>> > should just stick with FSRT (and frame_width = sample_width * channels)
>> > to keep the code simple.
>> >
>>
>> I hope so, but the assumption of frame_width == sample_width * 2 should
>> hold true first.
>
> Ok, here is what I think that should work for I2S after my 2 patches to
> sort the TDM thing.
>
> 2 Patches are inline, first version assumes that frame_width =
> sample_width * 2(channels), and just increases the SFRM duration to
> emulate the LRCLK.
>
> Second version uses Eric and Paul code only for pxa3xx. In this version,
> frame_width = sample_width * 2(channels) * 2(envelope).
>
> This was only compile tested, I dont have PXA3XX hardware to test this.
> It applies after the 3 patches I sent earlier.
>

Well, I'm completely lost in this thread. Can anyone give a summary
on this issue? And it looks like set_tdm_slot() is used to generalize
the issue of envelop (or the actual frame/sample width), and the DAI
format setting code here will be generalized??

Daniel,

Could you please send out all the four patches? Sorry late on this,
busy with the merging stuff.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [RFC] I2S and LEFT_J (was: ASoC: pxa-ssp: enhance I2S and add Left_J support)

[Daniel Ribeiro]

[-- Attachment #1.1: Type: text/plain, Size: 2389 bytes --]

Em Seg, 2009-06-15 às 16:45 +0800, Eric Miao escreveu:
> Well, I'm completely lost in this thread. Can anyone give a summary
> on this issue? And it looks like set_tdm_slot() is used to generalize
> the issue of envelop (or the actual frame/sample width), and the DAI
> format setting code here will be generalized??

The patches fixes a number of issues on pxa-ssp, and extends
set_tdm_slot()

1. No abuse of SSCR0_MOD.

Currently pxa-ssp requires SSCR0_MOD to work, but this should only be
needed if you need a frame width larger than 32 bits or if you are
really using network mode.


2. Frame width is set wrong for 2*16 bits format.

Currently it sets 32bits DMA but 16bits frame width for stereo S16_LE
audio. It currently "works" because people set network mode with 2
active slots.


3. set_tdm_slot should be only for real network mode.

Currently set_tdm_slot is always required. After the patches the users
only need to call set_tdm_slot if they are really using network mode. 

For frame widths > 32bits (SSCR0_MOD is needed to support these cases),
a "fake" network mode is automatically set.


4. Extends set_tdm_slot to include the desired frame width

It is needed to support real network mode. As the code is currently, the
frame width is set based on the the pcm format, so you cant have network
mode running if the devices uses 2 different pcm formats.


5. I2S and LEFT_J

I have 2 versions of this patch, first doesn't do the enveloping, and
just uses 32bits frames for 2*16bits I2S samples. The other does the
enveloping on 64bits frames for 2*16bits I2S samples (this can only work
on pxa3xx). I need somebody to test the first version on pxa3xx, as it
is much simpler and doesn't waste 32 bitclocks for each frame.

Having the start of the sample offset by 1 bitclk is not something
specific to I2S, its how DSP_A works too, and I believe that we
shouldn't make I2S a special case (vs DSP_A).

For what it matters, the only difference on I2S/LEFT_J vs DSP_A/DSP_B
should be the SSPSFRM duration as it is needed to emulate the LRCLK.
(and of course, the fact that I2S/LEFT_J are stereo only and that
network mode can't be supported)

> Could you please send out all the four patches? Sorry late on this,
> busy with the merging stuff.
Yes, i will send the patches again later today.

-- 
Daniel Ribeiro

[-- Attachment #1.2: Esta é uma parte de mensagem assinada digitalmente --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 271 bytes --]

-------------------------------------------------------------------
List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel
FAQ:        http://www.arm.linux.org.uk/mailinglists/faq.php
Etiquette:  http://www.arm.linux.org.uk/mailinglists/etiquette.php

Re: [RFC] I2S and LEFT_J (was: ASoC: pxa-ssp: enhance I2S and add Left_J support)

[Mark Brown]

On Mon, Jun 15, 2009 at 11:57:04AM -0300, Daniel Ribeiro wrote:

> For what it matters, the only difference on I2S/LEFT_J vs DSP_A/DSP_B
> should be the SSPSFRM duration as it is needed to emulate the LRCLK.

Assuming no extra bit clocks.  Extra bit clocks do something different
in I2S due to the fact that LRCLK falling edge is signifigcant.

> (and of course, the fact that I2S/LEFT_J are stereo only and that
> network mode can't be supported)

You should be able to support at least modes using the first TDM
timeslot I'd have thought.

Re: [RFC] I2S and LEFT_J (was: ASoC: pxa-ssp: enhance I2S and add Left_J support)

[Daniel Ribeiro]

[-- Attachment #1.1: Type: text/plain, Size: 3218 bytes --]

Em Seg, 2009-06-15 às 16:04 +0100, Mark Brown escreveu:
> > For what it matters, the only difference on I2S/LEFT_J vs DSP_A/DSP_B
> > should be the SSPSFRM duration as it is needed to emulate the LRCLK.
> 
> Assuming no extra bit clocks.  Extra bit clocks do something different
> in I2S due to the fact that LRCLK falling edge is signifigcant.

Yes, assuming that we will not do the envelope thing.

But you have to consider that a codec that _requires_ 64 bits frames for
2*16bits I2S audio is not exactly I2S compliant.

Quoting the specifications:

"It isn't necessary for the transmitter to know how many bits the
receiver can handle, nor does the receiver need to know how many bits
are being transmitted.

When the system word length is is greater than the transmitter word
length, the word is truncated (least significant data bits are set to
'0') for data transmission. If the receiver is sent more bits than its
word length, the bits after the LSB are ignored. On the other hand, if
the receiver is sent fewer bits than its word length, the missing bits
are set to zero internally. And so, the MSB has a fixed position,
whereas the position of the LSB depends on the word length. The
transmitter always sends the MSB of the next word one clock period after
the WS changes."


Anyway, my interpretation of the I2S specifications, is that we don't
need to do this enveloping thing at all. Codecs that requires this are
simply broken, and are _not_ considering LRCLK edges as they are
supposed to.

And finally, if the codec does this enveloping thing, it can't work if
PXA is slave of LRCLK. The PXA-SSP port is definitely not I2S compliant,
as it just ignores the LRCLK falling edge. We can workaround this using
network mode with 4 slots and with slots 1|3 active, but this implies
knowing the sample/frame width in advance, which by itself is an I2S
spec violation.

I have hope that Daniel Mack's codec, which supposedly only works with
64 bits frames _is_ I2S compliant, and he just got to the wrong
conclusion that it needs 64 bits frames after a lot of trial and error,
and failing to fix the real issue. (setting 16bits frames(DataSize(16))
for 2*16bit samples is simply wrong).

> > (and of course, the fact that I2S/LEFT_J are stereo only and that
> > network mode can't be supported)
> 
> You should be able to support at least modes using the first TDM
> timeslot I'd have thought.

Right, I2S/LEFT_J can be networked with DSP_A/DSP_B pcm formats, but
I2S/LEFT_J has to be the first slot.


But...

Transmitter is sending 16 bits samples, and the receiver expecting 32
bits samples. This is perfectly valid according to I2S, the receiver
would append zeros to the LSBs of each sample.

Now, what if the transmitter is using network mode? The receiver would
interpret the 16 MSBs of the second slot as the 16 LSBs of the second
channel on the first slot. This would be noisy. ;)

I prefer to just not support network mode with I2S. I2S was designed to
have a single transmitter and N receivers on the bus, or, in case of an
I2S network have LRCLK assertion on all slots, which the PXA-SSP port
can't do.

-- 
Daniel Ribeiro

[-- Attachment #1.2: Esta é uma parte de mensagem assinada digitalmente --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [RFC] I2S and LEFT_J (was: ASoC: pxa-ssp: enhance I2S and add Left_J support)

[Daniel Mack]

On Mon, Jun 15, 2009 at 02:20:20PM -0300, Daniel Ribeiro wrote:
> But you have to consider that a codec that _requires_ 64 bits frames for
> 2*16bits I2S audio is not exactly I2S compliant.
> 
> Quoting the specifications:

[...]

Might be, but it's still up to the codec which further constrains it has
for the digital side :)

> Anyway, my interpretation of the I2S specifications, is that we don't
> need to do this enveloping thing at all. Codecs that requires this are
> simply broken, and are _not_ considering LRCLK edges as they are
> supposed to.

Quoting from the CS4270 datasheet:

5.1.2 Master/Slave Mode 
  The CS4270 supports operation in either Master Mode or Slave Mode. 

  In Master Mode, LRCK and SCLK are outputs and are synchronously
  generated on-chip. LRCK is equal to Fs and SCLK is equal to 64x Fs. 

  In Slave Mode, LRCK and SCLK are inputs, requiring external generation
  that is synchronous to MCLK. It is recommended that SCLK be 48x or 64x
  Fs to maximize system performance. 

I can only guess what really happens internally, but the most obvious
thing is that they need the additional clock cycles for internal
real-time processing.

> And finally, if the codec does this enveloping thing, it can't work if
> PXA is slave of LRCLK. The PXA-SSP port is definitely not I2S compliant,
> as it just ignores the LRCLK falling edge. We can workaround this using
> network mode with 4 slots and with slots 1|3 active, but this implies
> knowing the sample/frame width in advance, which by itself is an I2S
> spec violation.

Yes, that's true. For that very reason, we run the PXA in master mode,
but clock the whole SSP engine from the SSPEXTCLK pin. So the PXA is
master to LRCLK and BITCLK, but eventually it's still slave to an
external and tunable clock.

> I have hope that Daniel Mack's codec, which supposedly only works with
> 64 bits frames _is_ I2S compliant, and he just got to the wrong
> conclusion that it needs 64 bits frames after a lot of trial and error,
> and failing to fix the real issue. (setting 16bits frames(DataSize(16))
> for 2*16bit samples is simply wrong).

I'm willing to give that another try now that the codec is in slave
mode. But I fear there is some impact on the audio quality which I can't
quickly measure here, so I'd really vote for offering the mode I'm
currently using. Forbidding it due to API clearance even though it's
supported by the hardware is a step backwards.

Daniel

Re: [RFC] I2S and LEFT_J (was: ASoC: pxa-ssp: enhance I2S and add Left_J support)

[Daniel Ribeiro]

[-- Attachment #1.1: Type: text/plain, Size: 1468 bytes --]

Em Seg, 2009-06-15 às 19:40 +0200, Daniel Mack escreveu:
> Forbidding it due to API clearance even though it's
> supported by the hardware is a step backwards.

I wouldn't consider this API clearance. If you look at the pxa-ssp.c
history you will see that the first version of this driver that hit
mainline used to work for the standard 32 bits frame_width for 2*16 bits
samples, for DSP_A/DSP_B and I2S.

(Tough I2S was broken for other reasons if you used
SND_SOC_DAIFMT_IB_IF, and required a set_tdm_slot() call)


Commit aa4ef01de5f2e7ed948b88f9f1cfc93c8e0c3f25 broke DSP_A and DSP_B
for 2*16bits samples, if you don't make a dummy set_tdm_slot() call.

Commit 72d7466468b471f99cefae3c5f4a414bbbaa0bdd broke I2S mode for
pxa2xx, even with set_tdm_slot(). And introduced a relation between
SerClkDiv and sample_width that shouldn't exist.

Commit 92429069d0fc9f52d436c9067c5b5c392e3f8876 extended the breakage of
2*16bits samples on 32bit frames for all modes, unless you call
set_tdm_slot(2, 3).

So, mostly, I'm fixing regressions. ;)

Of course I want the driver to work with all the weird non-standard
formats that hardware may require, but the current code is broken on
standards compliant hardware unless you do some set_tdm_slot black
magic. Doing set_tdm_slot hacks is acceptable for non-standard devices,
but is not a nice thing if your hardware is standard or if you want to
use _real_ network mode.

--
Daniel Ribeiro

[-- Attachment #1.2: Esta é uma parte de mensagem assinada digitalmente --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [RFC] I2S and LEFT_J (was: ASoC: pxa-ssp: enhance I2S and add Left_J support)

[Mark Brown]

On Mon, Jun 15, 2009 at 02:20:20PM -0300, Daniel Ribeiro wrote:

> But you have to consider that a codec that _requires_ 64 bits frames for
> 2*16bits I2S audio is not exactly I2S compliant.

Implementors in "not having fully read standard" shocker!  :)

Actually, I suspect what's happened here is that whoever was designing
the chip needed a synced clock at 64fs for their DAC or ADC, noticed
that they could run the bitclock over speed to get that clock and
decided to use that.  Most devices use a separate MCLK.

> Anyway, my interpretation of the I2S specifications, is that we don't
> need to do this enveloping thing at all. Codecs that requires this are
> simply broken, and are _not_ considering LRCLK edges as they are
> supposed to.

Broken and nonexistant are unfortunately not synonyms.  Like I say,
they're just using a subset of TDM mode configuration so it's not like
there's no possible use for this outside of workarounds anyway.

> But...

> Transmitter is sending 16 bits samples, and the receiver expecting 32
> bits samples. This is perfectly valid according to I2S, the receiver
> would append zeros to the LSBs of each sample.

> Now, what if the transmitter is using network mode? The receiver would
> interpret the 16 MSBs of the second slot as the 16 LSBs of the second
> channel on the first slot. This would be noisy. ;)

> I prefer to just not support network mode with I2S. I2S was designed to
> have a single transmitter and N receivers on the bus, or, in case of an
> I2S network have LRCLK assertion on all slots, which the PXA-SSP port
> can't do.

There's plenty of ways to mess up your audio if you do something broken,
I'm not overly concerned about this particular one over any of the
others so preserving the option to set it up seems useful.  As far as
your patch goes I think so long as it's clear how one would implement
I2S TDM it's fine, I don't think it's essential to implement it in your
patch since we don't have anything in-tree which needs it.

Personally I don't need this mode, the systems I'm working with all have
very much standards compliant CODECs which can be configured to use DSP
modes if desired.

Re: [RFC] I2S and LEFT_J

[Eric Miao]

Daniel Ribeiro wrote:
> Em Seg, 2009-06-15 às 16:04 +0100, Mark Brown escreveu:
>>> For what it matters, the only difference on I2S/LEFT_J vs DSP_A/DSP_B
>>> should be the SSPSFRM duration as it is needed to emulate the LRCLK.
>> Assuming no extra bit clocks.  Extra bit clocks do something different
>> in I2S due to the fact that LRCLK falling edge is signifigcant.
> 
> Yes, assuming that we will not do the envelope thing.
> 
> But you have to consider that a codec that _requires_ 64 bits frames for
> 2*16bits I2S audio is not exactly I2S compliant.
> 
> Quoting the specifications:
> 
> "It isn't necessary for the transmitter to know how many bits the
> receiver can handle, nor does the receiver need to know how many bits
> are being transmitted.
> 
> When the system word length is is greater than the transmitter word
> length, the word is truncated (least significant data bits are set to
> '0') for data transmission. If the receiver is sent more bits than its
> word length, the bits after the LSB are ignored. On the other hand, if
> the receiver is sent fewer bits than its word length, the missing bits
> are set to zero internally. And so, the MSB has a fixed position,
> whereas the position of the LSB depends on the word length. The
> transmitter always sends the MSB of the next word one clock period after
> the WS changes."
> 
> 
> Anyway, my interpretation of the I2S specifications, is that we don't
> need to do this enveloping thing at all. Codecs that requires this are
> simply broken, and are _not_ considering LRCLK edges as they are
> supposed to.
> 
> And finally, if the codec does this enveloping thing, it can't work if
> PXA is slave of LRCLK. The PXA-SSP port is definitely not I2S compliant,
> as it just ignores the LRCLK falling edge. We can workaround this using
> network mode with 4 slots and with slots 1|3 active, but this implies
> knowing the sample/frame width in advance, which by itself is an I2S
> spec violation.
> 
> I have hope that Daniel Mack's codec, which supposedly only works with
> 64 bits frames _is_ I2S compliant, and he just got to the wrong
> conclusion that it needs 64 bits frames after a lot of trial and error,
> and failing to fix the real issue. (setting 16bits frames(DataSize(16))
> for 2*16bit samples is simply wrong).
> 
>>> (and of course, the fact that I2S/LEFT_J are stereo only and that
>>> network mode can't be supported)
>> You should be able to support at least modes using the first TDM
>> timeslot I'd have thought.
> 
> Right, I2S/LEFT_J can be networked with DSP_A/DSP_B pcm formats, but
> I2S/LEFT_J has to be the first slot.
> 
> 
> But...
> 
> Transmitter is sending 16 bits samples, and the receiver expecting 32
> bits samples. This is perfectly valid according to I2S, the receiver
> would append zeros to the LSBs of each sample.

Daniel,

Could you give me an example of how can I setup the I2S in-compatible
mode with S_16LE, 64bitfs with your current patch? Thanks.

Re: [RFC] I2S and LEFT_J

[Daniel Ribeiro]

[-- Attachment #1.1: Type: text/plain, Size: 806 bytes --]

Em Qui, 2009-06-18 às 15:58 +0800, Eric Miao escreveu:
> Daniel,
> 
> Could you give me an example of how can I setup the I2S in-compatible
> mode with S_16LE, 64bitfs with your current patch? Thanks.

Hi Eric,

If your codec can work with S16LE and 32bitfs, then i suggest you to use
this mode. If not, then you need to setup TDM.

For 2*16 on 32bitfs:
Don't call set_tdm_slot().

For 2*16 on 64bitfs:
Call set_tdm_slot(5, 5, 4, 16).

For 2*16 on 128bitfs:
Call set_tdm_slot(0x11, 0x11, 8, 16).

For 2*32 on 64bitfs:
Don't call set_tdm_slot().

For 2*32 on 128bitfs:
Call set_tdm_slot(5, 5, 4, 32).

Please note that I have _not_ tested the sample_width * channels !=
frame_width cases(only possible on PXA3XX), and maybe we still need to
amend these.

-- 
Daniel Ribeiro

[-- Attachment #1.2: Esta é uma parte de mensagem assinada digitalmente --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [RFC] I2S and LEFT_J

[Daniel Mack]

Hi Daniel,

On Thu, Jun 18, 2009 at 09:30:58AM -0300, Daniel Ribeiro wrote:
> If your codec can work with S16LE and 32bitfs, then i suggest you to use
> this mode. If not, then you need to setup TDM.
> 
> For 2*16 on 64bitfs:
> Call set_tdm_slot(5, 5, 4, 16).

I tried your three patches now, and it doesn't seem to work for me.
Using the mode above, I get the following register values:

  SSCR0 0xa30003ff
  SSCR1 0x00601d80
  SSPSP 0x31a00084 
  SSTSA 0x00000005
  SSRSA 0x00000005

And on the oscilloscope, I see an asynchronous LRCLK[1].

set_tdm_slot(3, 3, 2, 16) gave me slightly better results, but the PSP
values are stil bogus (SFRMWDTH=0x10 and EDMYSTOP=0x7).

When manually forcing (E)DMYSTOP=0xf and SFRMWDTH=0x20, the signal looks
correct at first[2], but the audio material is played back at half speed.

This is where I stopped for now. I can just tell that I've spent many
hours playing with these bits and never found a fully working networked
mode based setting for that kind of signal output.

What's worth mentioning is this quote from the PXA datasheet - the code
does not currently follow that rule:

  "When using Programmable Serial Protocol (PSP) format in network
  mode, the parameters SFRMDLY, STRTDLY, DMYSTP, EDMYSTP, DMYSTRT, and
  EDMYSTRT must be set to 0b0; the other parameters SFRMP, SCMODE, FSRT,
  and SFRMWDTH are programmable."
  (4.5.8 SSP Programmable Serial Protocol Registers (SSPSP_x))

AFAIK, Eric and Paul seem to have the exactly same requirements, so
maybe they can test and get back with more results?

Without network mode, these are the register values that do what I need:

  SSCR0 0x2100037F
  SSCR1 0x00C01d08
  SSPSP 0x31a08084
  (SSTSA/SSRSA don't matter in this case)

Let me know if I can provide any more feedback :)

Thanks,
Daniel

[1] http://caiaq.de/download/tmp/1.png
[2] http://caiaq.de/download/tmp/2.png
(2: i2s_txd, 1:i2s_frame, 3:i2s_clk)

Re: [RFC] I2S and LEFT_J

[Daniel Ribeiro]

[-- Attachment #1.1: Type: text/plain, Size: 7669 bytes --]

Hi Daniel, sorry for the delay on this.

Em Ter, 2009-06-23 às 00:14 +0200, Daniel Mack escreveu:
> And on the oscilloscope, I see an asynchronous LRCLK[1].

> What's worth mentioning is this quote from the PXA datasheet - the code
> does not currently follow that rule:
> 
>   "When using Programmable Serial Protocol (PSP) format in network
>   mode, the parameters SFRMDLY, STRTDLY, DMYSTP, EDMYSTP, DMYSTRT, and
>   EDMYSTRT must be set to 0b0; the other parameters SFRMP, SCMODE, FSRT,
>   and SFRMWDTH are programmable."
>   (4.5.8 SSP Programmable Serial Protocol Registers (SSPSP_x))

Yes, and very likely this is what caused the asynchronous LRCLK.

> Without network mode, these are the register values that do what I need:
> 
>   SSCR0 0x2100037F
>   SSCR1 0x00C01d08
>   SSPSP 0x31a08084
>   (SSTSA/SSRSA don't matter in this case)

Thanks for the testing. If you don't mind, can you please replace the
third patch, and try this one instead?

Use set_tdm_slot(5, 5, 4, 16), if this works for you we can just drop
the pxa3xx special case and use SSCR0_FSRT and network mode for both
pxa2xx and pxa3xx.

If not, use set_tdm_slot(1, 1, 1, 16), and tweak the
(slots == 1 && slot_width == 16) special case as needed. This condition
should give you a config without SSCR0_MOD, and you should be able to
match the register values above.

---
 arch/arm/mach-pxa/include/mach/regs-ssp.h |   14 ++--
 sound/soc/pxa/pxa-ssp.c                   |  125 ++++++++++++++---------------
 2 files changed, 67 insertions(+), 72 deletions(-)

diff --git a/arch/arm/mach-pxa/include/mach/regs-ssp.h b/arch/arm/mach-pxa/include/mach/regs-ssp.h
index 6a2ed35..060e23b 100644
--- a/arch/arm/mach-pxa/include/mach/regs-ssp.h
+++ b/arch/arm/mach-pxa/include/mach/regs-ssp.h
@@ -108,21 +108,21 @@
 #define SSSR_TINT		(1 << 19)	/* Receiver Time-out Interrupt */
 #define SSSR_PINT		(1 << 18)	/* Peripheral Trailing Byte Interrupt */
 
-#if defined(CONFIG_PXA3xx)
-#define SSPSP_EDMYSTOP(x)	((x) << 28)     /* Extended Dummy Stop */
-#define SSPSP_EDMYSTRT(x)	((x) << 26)     /* Extended Dummy Start */
-#endif
-
 #define SSPSP_FSRT		(1 << 25)	/* Frame Sync Relative Timing */
-#define SSPSP_DMYSTOP(x)	((x) << 23)	/* Dummy Stop */
 #define SSPSP_SFRMWDTH(x)	((x) << 16)	/* Serial Frame Width */
 #define SSPSP_SFRMDLY(x)	((x) << 9)	/* Serial Frame Delay */
-#define SSPSP_DMYSTRT(x)	((x) << 7)	/* Dummy Start */
 #define SSPSP_STRTDLY(x)	((x) << 4)	/* Start Delay */
 #define SSPSP_ETDS		(1 << 3)	/* End of Transfer data State */
 #define SSPSP_SFRMP		(1 << 2)	/* Serial Frame Polarity */
 #define SSPSP_SCMODE(x)		((x) << 0)	/* Serial Bit Rate Clock Mode */
 
+/* NOTE: PXA3xx extends the bit number of dummy start and stop, the macros
+ * below are compatible with PXA25x/27x as long as the parameter is within
+ * the correct limits, driver code has to take care of this.
+ */
+#define SSPSP_DMYSTRT(x)       ((((x) & 3) << 7)  | ((((x) >> 2) & 3) << 26))
+#define SSPSP_DMYSTOP(x)       ((((x) & 3) << 23) | ((((x) >> 2) & 7) << 28))
+
 #define SSACD_SCDB		(1 << 3)	/* SSPSYSCLK Divider Bypass */
 #define SSACD_ACPS(x)		((x) << 4)	/* Audio clock PLL select */
 #define SSACD_ACDS(x)		((x) << 0)	/* Audio clock divider select */
diff --git a/sound/soc/pxa/pxa-ssp.c b/sound/soc/pxa/pxa-ssp.c
index d60492e..6efead5 100644
--- a/sound/soc/pxa/pxa-ssp.c
+++ b/sound/soc/pxa/pxa-ssp.c
@@ -179,21 +179,6 @@ static void ssp_set_scr(struct ssp_device *ssp, u32 div)
 	ssp_write_reg(ssp, SSCR0, sscr0);
 }
 
-/**
- * ssp_get_clkdiv - get SSP clock divider
- */
-static u32 ssp_get_scr(struct ssp_device *ssp)
-{
-	u32 sscr0 = ssp_read_reg(ssp, SSCR0);
-	u32 div;
-
-	if (cpu_is_pxa25x() && ssp->type == PXA25x_SSP)
-		div = ((sscr0 >> 8) & 0xff) * 2 + 2;
-	else
-		div = ((sscr0 >> 8) & 0xfff) + 1;
-	return div;
-}
-
 /*
  * Set the SSP ports SYSCLK.
  */
@@ -487,17 +472,14 @@ static int pxa_ssp_set_dai_fmt(struct snd_soc_dai *cpu_dai,
 	}
 
 	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
-	case SND_SOC_DAIFMT_I2S:
-		sscr0 |= SSCR0_PSP;
-		sscr1 |= SSCR1_RWOT | SSCR1_TRAIL;
-		/* See hw_params() */
-		break;
-
 	case SND_SOC_DAIFMT_DSP_A:
 		sspsp |= SSPSP_FSRT;
 	case SND_SOC_DAIFMT_DSP_B:
+	case SND_SOC_DAIFMT_LEFT_J:
+	case SND_SOC_DAIFMT_I2S:
 		sscr0 |= SSCR0_PSP;
 		sscr1 |= SSCR1_TRAIL | SSCR1_RWOT;
+		/* See hw_params() for I2S and LEFT_J */
 		break;
 
 	default:
@@ -561,6 +543,63 @@ static int pxa_ssp_hw_params(struct snd_pcm_substream *substream,
 		sscr0 |= SSCR0_FPCKE;
 #endif
 
+	sspsp = ssp_read_reg(ssp, SSPSP);
+	switch (priv->dai_fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
+	case SND_SOC_DAIFMT_I2S:
+		/*
+		 * I2S and LEFT_J are stereo only, we have to send data for
+		 * both channels.
+		 */
+		if (chn == 1)
+			frame_width *= 2;
+
+		/*
+		 * If the user did not use network mode, we assume the codec
+		 * is I2S compliant.
+		 */
+		if (frame_width > 0) {
+			sspsp |= SSPSP_SFRMWDTH(frame_width / 2);
+			sspsp |= SSPSP_FSRT;
+		} else {
+			/*
+			 * Otherwise we assume that it is a single TDM slot, and
+			 * the user is abusing set_tdm_slot to support an
+			 * out of spec codec.
+			 */
+			int slots = ((sscr0 & SSCR0_SlotsPerFrm(8)) >> 24) + 1;
+
+			if (slots == 1 && slot_width == 16) {
+				if (!cpu_is_pxa3xx())
+					return -EINVAL;
+
+				sspsp |= SSPSP_DMYSTRT(1);
+				sspsp |= SSPSP_DMYSTOP(
+						slot_width * 2 - width - 1);
+				sspsp |= SSPSP_SFRMWDTH(slot_width * 2);
+			} else {
+				sspsp |= SSPSP_SFRMWDTH(slot_width * slots / 2);
+				sspsp |= SSPSP_FSRT;
+			}
+		}
+		break;
+
+	case SND_SOC_DAIFMT_LEFT_J:
+		if (chn == 1)
+			frame_width *= 2;
+
+		if (frame_width > 0) {
+			sspsp |= SSPSP_SFRMWDTH(frame_width / 2);
+		} else {
+			int slots = ((sscr0 & SSCR0_SlotsPerFrm(8)) >> 24) + 1;
+
+			sspsp |= SSPSP_SFRMWDTH((slot_width * slots) / 2);
+		}
+		break;
+	default:
+		break;
+	}
+	ssp_write_reg(ssp, SSPSP, sspsp);
+
 	if (frame_width > 0) {
 		/* Not using network mode */
 		if (frame_width > 16)
@@ -598,50 +637,6 @@ static int pxa_ssp_hw_params(struct snd_pcm_substream *substream,
 
 	ssp_write_reg(ssp, SSCR0, sscr0);
 
-	switch (priv->dai_fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
-	case SND_SOC_DAIFMT_I2S:
-	       sspsp = ssp_read_reg(ssp, SSPSP);
-
-		if ((ssp_get_scr(ssp) == 4) && (width == 16)) {
-			/* This is a special case where the bitclk is 64fs
-			* and we're not dealing with 2*32 bits of audio
-			* samples.
-			*
-			* The SSP values used for that are all found out by
-			* trying and failing a lot; some of the registers
-			* needed for that mode are only available on PXA3xx.
-			*/
-
-#ifdef CONFIG_PXA3xx
-			if (!cpu_is_pxa3xx())
-				return -EINVAL;
-
-			sspsp |= SSPSP_SFRMWDTH(width * 2);
-			sspsp |= SSPSP_SFRMDLY(width * 4);
-			sspsp |= SSPSP_EDMYSTOP(3);
-			sspsp |= SSPSP_DMYSTOP(3);
-			sspsp |= SSPSP_DMYSTRT(1);
-#else
-			return -EINVAL;
-#endif
-		} else {
-			/* The frame width is the width the LRCLK is
-			 * asserted for; the delay is expressed in
-			 * half cycle units.  We need the extra cycle
-			 * because the data starts clocking out one BCLK
-			 * after LRCLK changes polarity.
-			 */
-			sspsp |= SSPSP_SFRMWDTH(width + 1);
-			sspsp |= SSPSP_SFRMDLY((width + 1) * 2);
-			sspsp |= SSPSP_DMYSTRT(1);
-		}
-
-		ssp_write_reg(ssp, SSPSP, sspsp);
-		break;
-	default:
-		break;
-	}
-
 	dump_registers(ssp);
 
 	return 0;
-- 
tg: (2d97ff5..) asoc/leftj-and-i2s (depends on: asoc/ssp-internals)


-- 
Daniel Ribeiro

[-- Attachment #1.2: Esta é uma parte de mensagem assinada digitalmente --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [RFC] I2S and LEFT_J

[Daniel Mack]

Hi Daniel,

On Fri, Jun 26, 2009 at 09:28:28PM -0300, Daniel Ribeiro wrote:
> Em Ter, 2009-06-23 às 00:14 +0200, Daniel Mack escreveu:
> > Without network mode, these are the register values that do what I need:
> > 
> >   SSCR0 0x2100037F
> >   SSCR1 0x00C01d08
> >   SSPSP 0x31a08084
> >   (SSTSA/SSRSA don't matter in this case)
> 
> Thanks for the testing. If you don't mind, can you please replace the
> third patch, and try this one instead?

Jup, applied now.

> Use set_tdm_slot(5, 5, 4, 16), if this works for you we can just drop
> the pxa3xx special case and use SSCR0_FSRT and network mode for both
> pxa2xx and pxa3xx.

That gives an asynchronous LRCLK again.

> If not, use set_tdm_slot(1, 1, 1, 16), and tweak the
> (slots == 1 && slot_width == 16) special case as needed. This condition
> should give you a config without SSCR0_MOD, and you should be able to
> match the register values above.

Unfortunately it doesn't. The resulting register values are

  SSCR0 0x200003ff
  SSCR1 0x00e01d80
  SSPSP 0x31a00084

and the LRCLK does not toggle any more at all, and hence DMA is not
happening. I couldn't do more investigation on this right now, but the
value dump might help. Let me know if I can do more tests.

Thanks,
Daniel

[RFC] Auto setup TDM when needed. Add frame_width and rx/tx masks to set_tdm_slots.

[Daniel Ribeiro]

[-- Attachment #1.1: Type: text/plain, Size: 7664 bytes --]

Em Seg, 2009-06-08 às 17:53 +0100, Mark Brown escreveu:
> I'd like to see all these details handled within the driver - knowing if
> and when network mode are to be set up is the sort of thing that users
> ought to be able to rely on the driver for.

Untested, RFC only version of patch.
(plus some minor copynpaste and whitespace fixes)

Am I on the right direction? :)

 include/sound/soc-dai.h |    3 +
 sound/soc/pxa/pxa-ssp.c |   86 +++++++++++++++++++++++++++++-------------------
 sound/soc/soc-core.c    |   13 ++++---
 3 files changed, 62 insertions(+), 40 deletions(-)
--
diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
index 1367647..0bf9502 100644
--- a/include/sound/soc-dai.h
+++ b/include/sound/soc-dai.h
@@ -154,7 +154,8 @@ struct snd_soc_dai_ops {
 	 */
 	int (*set_fmt)(struct snd_soc_dai *dai, unsigned int fmt);
 	int (*set_tdm_slot)(struct snd_soc_dai *dai,
-		unsigned int mask, int slots);
+		unsigned int tx_mask, unsigned int rx_mask,
+		int slots, int frame_width);
 	int (*set_tristate)(struct snd_soc_dai *dai, int tristate);
 
 	/*
diff --git a/sound/soc/pxa/pxa-ssp.c b/sound/soc/pxa/pxa-ssp.c
index 92838af..a0b7bad 100644
--- a/sound/soc/pxa/pxa-ssp.c
+++ b/sound/soc/pxa/pxa-ssp.c
@@ -490,21 +490,31 @@ static int pxa_ssp_set_dai_pll(struct snd_soc_dai *cpu_dai,
  * Set the active slots in TDM/Network mode
  */
 static int pxa_ssp_set_dai_tdm_slot(struct snd_soc_dai *cpu_dai,
-	unsigned int mask, int slots)
+	unsigned int tx_mask, unsigned int rx_mask, int slots, int frame_width)
 {
 	struct ssp_priv *priv = cpu_dai->private_data;
 	struct ssp_device *ssp = priv->dev.ssp;
 	u32 sscr0;
 
-	sscr0 = ssp_read_reg(ssp, SSCR0) & ~SSCR0_SlotsPerFrm(7);
+	sscr0 = ssp_read_reg(ssp, SSCR0);
+	sscr0 &= ~(SSCR0_SlotsPerFrm(7) | SSCR0_EDSS | SSCR0_DSS);
+
+	/* enable network mode */
+	sscr0 |= SSCR0_MOD;
+
+	/* set frame width */
+	if (frame_width > 16)
+		sscr0 |= SSCR0_EDSS | SSCR0_DataSize(frame_width - 16);
+	else
+		sscr0 |= SSCR0_DataSize(frame_width);
 
 	/* set number of active slots */
 	sscr0 |= SSCR0_SlotsPerFrm(slots);
 	ssp_write_reg(ssp, SSCR0, sscr0);
 
 	/* set active slot mask */
-	ssp_write_reg(ssp, SSTSA, mask);
-	ssp_write_reg(ssp, SSRSA, mask);
+	ssp_write_reg(ssp, SSTSA, tx_mask);
+	ssp_write_reg(ssp, SSRSA, rx_mask);
 	return 0;
 }
 
@@ -555,7 +565,7 @@ static int pxa_ssp_set_dai_fmt(struct snd_soc_dai *cpu_dai,
 
 	/* reset port settings */
 	sscr0 = ssp_read_reg(ssp, SSCR0) &
-		(SSCR0_ECS |  SSCR0_NCS | SSCR0_MOD | SSCR0_ACS);
+		(SSCR0_ECS | SSCR0_NCS | SSCR0_MOD | SSCR0_ACS);
 	sscr1 = SSCR1_RxTresh(8) | SSCR1_TxTresh(7);
 	sspsp = 0;
 
@@ -602,7 +612,7 @@ static int pxa_ssp_set_dai_fmt(struct snd_soc_dai *cpu_dai,
 	case SND_SOC_DAIFMT_DSP_A:
 		sspsp |= SSPSP_FSRT;
 	case SND_SOC_DAIFMT_DSP_B:
-		sscr0 |= SSCR0_MOD | SSCR0_PSP;
+		sscr0 |= SSCR0_PSP;
 		sscr1 |= SSCR1_TRAIL | SSCR1_RWOT;
 
 		switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
@@ -652,10 +662,10 @@ static int pxa_ssp_hw_params(struct snd_pcm_substream *substream,
 	struct ssp_priv *priv = cpu_dai->private_data;
 	struct ssp_device *ssp = priv->dev.ssp;
 	int dma = 0, chn = params_channels(params);
-	u32 sscr0;
+	u32 sscr0, sscr1;
 	u32 sspsp;
 	int width = snd_pcm_format_physical_width(params_format(params));
-	int ttsa = ssp_read_reg(ssp, SSTSA) & 0xf;
+	int frame_width = width * chn;
 
 	/* select correct DMA params */
 	if (substream->stream != SNDRV_PCM_STREAM_PLAYBACK)
@@ -664,7 +674,7 @@ static int pxa_ssp_hw_params(struct snd_pcm_substream *substream,
 	 * to force 16-bit frame width on the wire (for S16_LE), even
 	 * with two channels. Use 16-bit DMA transfers for this case.
 	 */
-	if (((chn == 2) && (ttsa != 1)) || (width == 32))
+	if (frame_width >= 32)
 		dma += 2; /* 32-bit DMA offset is 2, 16-bit is 0 */
 
 	cpu_dai->dma_data = ssp_dma_params[cpu_dai->id][dma];
@@ -675,32 +685,48 @@ static int pxa_ssp_hw_params(struct snd_pcm_substream *substream,
 	if (ssp_read_reg(ssp, SSCR0) & SSCR0_SSE)
 		return 0;
 
-	/* clear selected SSP bits */
-	sscr0 = ssp_read_reg(ssp, SSCR0) & ~(SSCR0_DSS | SSCR0_EDSS);
-	ssp_write_reg(ssp, SSCR0, sscr0);
+	/* frame width */
+	sscr0 = ssp_read_reg(ssp, SSCR0) & ~(SSCR0_EDSS | SSCR0_DSS);
 
-	/* bit size */
-	sscr0 = ssp_read_reg(ssp, SSCR0);
-	switch (params_format(params)) {
-	case SNDRV_PCM_FORMAT_S16_LE:
+	/* FIXME: Is this needed? */
 #ifdef CONFIG_PXA3xx
-		if (cpu_is_pxa3xx())
-			sscr0 |= SSCR0_FPCKE;
+	if (width == 16 && cpu_is_pxa3xx())
+		sscr0 |= SSCR0_FPCKE;
 #endif
+
+	if (!(sscr0 & SSCR0_MOD)) {
+
+		/* Not on network mode, setup the frame width */
 		sscr0 |= SSCR0_DataSize(16);
-		break;
-	case SNDRV_PCM_FORMAT_S24_LE:
-		sscr0 |= (SSCR0_EDSS | SSCR0_DataSize(8));
-		break;
-	case SNDRV_PCM_FORMAT_S32_LE:
-		sscr0 |= (SSCR0_EDSS | SSCR0_DataSize(16));
-		break;
+		if (frame_width >= 32)
+			sscr0 |= SSCR0_EDSS;
+
+		if (frame_width > 32) {
+			/*
+			 * Network mode is needed to support this frame_width
+			 * We assume that the wire is not networked and setup
+			 * a "fake" network mode here.
+			 */
+			int slots = frame_width / 32;
+
+			sscr0 |= SSCR0_MOD;
+			sscr0 |= SSCR0_SlotsPerFrm(slots);
+			ssp_write_reg(ssp, SSTSA, slots - 1);
+			ssp_write_reg(ssp, SSRSA, slots - 1);
+		}
+	}
+
+	/* If SSCR0_MOD is set we can't use SSCR1_RWOT */
+	if (sscr0 & SSCR0_MOD) {
+		sscr1 = ssp_read_reg(ssp, SSCR1);
+		ssp_write_reg(ssp, SSCR1, sscr1 & ~SSCR1_RWOT);
 	}
+
 	ssp_write_reg(ssp, SSCR0, sscr0);
 
 	switch (priv->dai_fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
 	case SND_SOC_DAIFMT_I2S:
-	       sspsp = ssp_read_reg(ssp, SSPSP);
+		sspsp = ssp_read_reg(ssp, SSPSP);
 
 		if ((ssp_get_scr(ssp) == 4) && (width == 16)) {
 			/* This is a special case where the bitclk is 64fs
@@ -742,14 +768,6 @@ static int pxa_ssp_hw_params(struct snd_pcm_substream *substream,
 		break;
 	}
 
-	/* When we use a network mode, we always require TDM slots
-	 * - complain loudly and fail if they've not been set up yet.
-	 */
-	if ((sscr0 & SSCR0_MOD) && !ttsa) {
-		dev_err(&ssp->pdev->dev, "No TDM timeslot configured\n");
-		return -EINVAL;
-	}
-
 	dump_registers(ssp);
 
 	return 0;
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index c9f19d0..ce13b6d 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -2130,17 +2130,20 @@ EXPORT_SYMBOL_GPL(snd_soc_dai_set_fmt);
 /**
  * snd_soc_dai_set_tdm_slot - configure DAI TDM.
  * @dai: DAI
- * @mask: DAI specific mask representing used slots.
+ * @tx_mask: DAI specific mask representing TX slots.
+ * @rx_mask: DAI specific mask representing RX slots.
  * @slots: Number of slots in use.
+ * @frame_width: Width in bits for each slot.
  *
  * Configures a DAI for TDM operation. Both mask and slots are codec and DAI
  * specific.
  */
 int snd_soc_dai_set_tdm_slot(struct snd_soc_dai *dai,
-	unsigned int mask, int slots)
+	unsigned int tx_mask, unsigned int rx_mask, int slots, int frame_width)
 {
-	if (dai->ops->set_sysclk)
-		return dai->ops->set_tdm_slot(dai, mask, slots);
+	if (dai->ops->set_tdm_slot)
+		return dai->ops->set_tdm_slot(dai, tx_mask, rx_mask,
+				slots, frame_width);
 	else
 		return -EINVAL;
 }
@@ -2155,7 +2158,7 @@ EXPORT_SYMBOL_GPL(snd_soc_dai_set_tdm_slot);
  */
 int snd_soc_dai_set_tristate(struct snd_soc_dai *dai, int tristate)
 {
-	if (dai->ops->set_sysclk)
+	if (dai->ops->set_tristate)
 		return dai->ops->set_tristate(dai, tristate);
 	else
 		return -EINVAL;

-- 
Daniel Ribeiro

[-- Attachment #1.2: Esta é uma parte de mensagem assinada digitalmente --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [RFC] Auto setup TDM when needed. Add frame_width and rx/tx masks to set_tdm_slots.

[Mark Brown]

On Mon, Jun 08, 2009 at 06:07:24PM -0300, Daniel Ribeiro wrote:

> Untested, RFC only version of patch.
> (plus some minor copynpaste and whitespace fixes)

> Am I on the right direction? :)

Yes, looks sensible.  I've not checked the driver code at all.

> +++ b/sound/soc/soc-core.c
> @@ -2130,17 +2130,20 @@ EXPORT_SYMBOL_GPL(snd_soc_dai_set_fmt);
>  /**
>   * snd_soc_dai_set_tdm_slot - configure DAI TDM.
>   * @dai: DAI
> - * @mask: DAI specific mask representing used slots.
> + * @tx_mask: DAI specific mask representing TX slots.
> + * @rx_mask: DAI specific mask representing RX slots.

I'd change the wording to be something like "bitmask representing active
slots".

>  {
> -	if (dai->ops->set_sysclk)
> -		return dai->ops->set_tdm_slot(dai, mask, slots);
> +	if (dai->ops->set_tdm_slot)

Should check that there are ops too.

> +		return dai->ops->set_tdm_slot(dai, tx_mask, rx_mask,
> +				slots, frame_width);

It might be nice to provide a default implementation for non-TDM if no
implementation is provided but that could be done later if it makes
sense.  I don't think it'll make much difference, though.

>  int snd_soc_dai_set_tristate(struct snd_soc_dai *dai, int tristate)
>  {
> -	if (dai->ops->set_sysclk)
> +	if (dai->ops->set_tristate)

Separate patch, please.  This is fixed already, though.

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[Eric Miao]

On Mon, Jun 8, 2009 at 8:12 PM, pHilipp Zabel<philipp.zabel@gmail.com> wrote:
> On Sat, Jun 6, 2009 at 10:12 PM, Mark
> Brown<broonie@opensource.wolfsonmicro.com> wrote:
>> On Wed, Jun 03, 2009 at 08:33:42PM +0800, Eric Miao wrote:
>>
>>> +/* frame size definitions for I2S and Left_J formats - default is
>>> + * 32fs, other possibilities are 48fs, 64fs and 96fs
>>> + */
>>> +#define PXA_SSP_FRM_32FS     (0 << 16)
>>> +#define PXA_SSP_FRM_48FS     (1 << 16)
>>> +#define PXA_SSP_FRM_64FS     (2 << 16)
>>> +#define PXA_SSP_FRM_96FS     (3 << 16)
>>> +#define PXA_SSP_FRM_WIDTH(x) (((((x) >> 16) & 0x3) + 2) << 4)
>>> +
>>
>> I still haven't checked this on Zylonite but I wanted to mention these
>> new DAI format bits just now - as previously discussed I'm not
>> enthusiastic about this.
>
> Agreed. Also, if I had to use this to configure magician with DSP_A
> (right now it says I2S and LEFT_J only, but why limit it to that?),
> I'd need an additional PXA_SSP_FRM_16FS.
>

That shouldn't be a problem - it can just be added.

>> As well as the previous issues I raised with
>> this approach I also meant to mention is that the use of a bitfield will
>> inevitably restrict what can be expressed, causing real problems for
>> some systems.  For example, the WM9081 supports systems using up to 3
>> stereo TDM slots and up to 32 bit samples so could be used in a system
>> which needs a 192fs bit clock.
>>
>> If we did decide to adopt this approach then these defines should be in
>> the ASoC headers rather than private to this driver - apart from
>> anything else, there's every chance that additions to the standard bits
>> could end up colliding with this.
>>
>> I'm still not sure what the best way to handle this is due to the
>> interaction with TDM mode.  The fact that TDM mode really wants to
>> always explicitly specify the frame width in order to allow the sample
>> size in each slot to vary suggests that one option would be to add a
>> sample width parameter to set_tdm_slot() - given the very small number
>> of in tree users it'd cause little disruption.  A set_sample_width()
>> operation could also be added.

Well, the real problem is that it's quite difficult to abstract all
the different formats into a single structure. I'd prefer to have
something specific at the begining.

>
> set_sample_width or set_frame_width?
> I'd prefer a separate operation over a parameter to set_tdm_slot because
> in my setup I just need to force the (SSP) frame width to 16 bits.
> Enabling network mode and calling set_tdm_slot(..,1,1) wouldn't be
> necessary at all, then.

As said, if we can invent something like a single structure
making all formats happy, it will be good. However, before
that's possible, I'd really prefer less API for the formats being
further introduced, so we go no further from that goal.

>
> regards
> Philipp
>



-- 
Cheers
- eric
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support

[Mark Brown]

On Mon, Jun 08, 2009 at 10:13:25PM +0800, Eric Miao wrote:
> On Mon, Jun 8, 2009 at 8:12 PM, pHilipp Zabel<philipp.zabel@gmail.com> wrote:

> > Agreed. Also, if I had to use this to configure magician with DSP_A
> > (right now it says I2S and LEFT_J only, but why limit it to that?),
> > I'd need an additional PXA_SSP_FRM_16FS.

> That shouldn't be a problem - it can just be added.

That won't scale in a bitmask - as you get more and more slots in use on
a TDM system more and more slots are going to need to extend the range
of values you can set in a bitmask.  Consider what happens when people
start hanging 5.1 CODECs off TDM buses with single data lines, for
example.

> >> sample width parameter to set_tdm_slot() - given the very small number
> >> of in tree users it'd cause little disruption.  A set_sample_width()
> >> operation could also be added.

> Well, the real problem is that it's quite difficult to abstract all
> the different formats into a single structure. I'd prefer to have
> something specific at the begining.

I don't see much difference between any of the proposals here (or any
problem with format-specificness) - my main thing here is to pull the
frame width configuration out of the DAI format.

> > set_sample_width or set_frame_width?
> > I'd prefer a separate operation over a parameter to set_tdm_slot because
> > in my setup I just need to force the (SSP) frame width to 16 bits.
> > Enabling network mode and calling set_tdm_slot(..,1,1) wouldn't be
> > necessary at all, then.

> As said, if we can invent something like a single structure
> making all formats happy, it will be good. However, before
> that's possible, I'd really prefer less API for the formats being
> further introduced, so we go no further from that goal.

Well, if we want to avoid introducing a new API at all then the PXA SSP
driver is going to need to figure out the bit clock rate automatically
from the TDM and sample size configuration - if it needs to be
explicitly specified then it's a new API and once we introduce one way
of doing this it'll probably stick.

TDM really needs some sort of frame/sample width configuration to work
well so I think we're better off adding a standard API now.