[alsa-devel] [PATCH 1/4] ASoC: pxa-ssp: enhance I2S and add Left_J support
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];
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
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
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.
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
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.
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
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
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?
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
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&w...
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.
On Sat, Jun 6, 2009 at 10:12 PM, Mark Brownbroonie@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
On Mon, Jun 08, 2009 at 02:12:03PM +0200, pHilipp Zabel wrote:
On Sat, Jun 6, 2009 at 10:12 PM, Mark Brownbroonie@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.
On Mon, Jun 8, 2009 at 2:40 PM, Mark Brownbroonie@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 Brownbroonie@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
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).
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.
On Mon, Jun 8, 2009 at 6:38 PM, Mark Brownbroonie@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
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.
On Mon, Jun 8, 2009 at 7:41 PM, Mark Brownbroonie@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
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.
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.
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. :)
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.
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... :)
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?
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
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.
On Tue, Jun 9, 2009 at 11:58 AM, Mark Brownbroonie@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-....
regards Philipp
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.
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.
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
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.
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;
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.
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.
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.
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.
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
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
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.
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.
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.
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)
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;
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
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;
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.
On Mon, Jun 8, 2009 at 8:12 PM, pHilipp Zabelphilipp.zabel@gmail.com wrote:
On Sat, Jun 6, 2009 at 10:12 PM, Mark Brownbroonie@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
On Mon, Jun 08, 2009 at 10:13:25PM +0800, Eric Miao wrote:
On Mon, Jun 8, 2009 at 8:12 PM, pHilipp Zabelphilipp.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.
participants (6)
-
Daniel Mack -
Daniel Ribeiro -
Eric Miao -
Mark Brown -
Paul Shen -
pHilipp Zabel