[alsa-devel] PXA SSP and external clocked I2S again
Hi,
as denoted yesterday, we've spend some more time on the PXA/SSP/I2S issue and would like to share conclusions about out utterly frustrating trial-and-error sessions during the last days, especially as reference for everyone who tries to get a similar setup running.
The situation on our board is: we have a tunable master clock generator, a CS4270 codec and an PXA303 connected to each other. In order to use the master clock, the PXA needs to be set to an external clock mode and for the CS4270 to operate properly, we need to provide a full 64 bits I2S stream, even though not all of the data bits (in fact, currently only 16 of them per channel) carry data.
The above thing was not possible with the pxa-ssp's current approach as it entirely relyed on the network mode and the time slots mechanism which is - according to the datasheets - supposed to do exactly this, but which simply doesn't work at all. It might work for existing boards without our constrains, but that's more or less due to coincidence.
Hence, we switched over to non-network mode and fiddled around with the PSP bits a lot until we found a mode that fits our needs, at least as long as we let the PXA be master in the game (which we did for test purposes).
As soon as the clock direction changes (codec takes over control for LRCLK and bitclk), the PSP feature fails badly again. The final solution is now to never ever set the PXA to a real slave mode (DAIFMT_CBM_CFM) but only provide the master clock to its input pin (SSPEXTCLK), set the clock config to SSP_CLK_EXT and let the PXA derive the other clocks from that one internally.
Another thing is: ALSA doesn't currently provide a way to configure a DAI format where the I2S protocol has more clock bits than data bits. One of the upcoming series of 4 patches adds them.
Best regards, Daniel
A bit in PXA's SSCR0 register was erroneously named ADC but its name is in fact ADC (audio clock select).
Signed-off-by: Daniel Mack daniel@caiaq.de Cc: Mark Brown broonie@opensource.wolfsonmicro.com Cc: Liam Girdwood lrg@kernel.org Cc: Tim Ruetz tim@caiaq.de --- arch/arm/mach-pxa/include/mach/regs-ssp.h | 2 +- sound/soc/pxa/pxa-ssp.c | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/arm/mach-pxa/include/mach/regs-ssp.h b/arch/arm/mach-pxa/include/mach/regs-ssp.h index 34eda82..70e0b81 100644 --- a/arch/arm/mach-pxa/include/mach/regs-ssp.h +++ b/arch/arm/mach-pxa/include/mach/regs-ssp.h @@ -50,7 +50,7 @@ #define SSCR0_TUM (1 << 23) /* Transmit FIFO underrun interrupt mask */ #define SSCR0_FRDC (0x07000000) /* Frame rate divider control (mask) */ #define SSCR0_SlotsPerFrm(x) (((x) - 1) << 24) /* Time slots per frame [1..8] */ -#define SSCR0_ADC (1 << 30) /* Audio clock select */ +#define SSCR0_ACS (1 << 30) /* Audio clock select */ #define SSCR0_MOD (1 << 31) /* Mode (normal or network) */ #endif
diff --git a/sound/soc/pxa/pxa-ssp.c b/sound/soc/pxa/pxa-ssp.c index c49bb12..7fc13f0 100644 --- a/sound/soc/pxa/pxa-ssp.c +++ b/sound/soc/pxa/pxa-ssp.c @@ -300,7 +300,7 @@ static int pxa_ssp_set_dai_sysclk(struct snd_soc_dai *cpu_dai, int val;
u32 sscr0 = ssp_read_reg(ssp, SSCR0) & - ~(SSCR0_ECS | SSCR0_NCS | SSCR0_MOD | SSCR0_ADC); + ~(SSCR0_ECS | SSCR0_NCS | SSCR0_MOD | SSCR0_ACS);
dev_dbg(&ssp->pdev->dev, "pxa_ssp_set_dai_sysclk id: %d, clk_id %d, freq %d\n", @@ -328,7 +328,7 @@ static int pxa_ssp_set_dai_sysclk(struct snd_soc_dai *cpu_dai, case PXA_SSP_CLK_AUDIO: priv->sysclk = 0; ssp_set_scr(&priv->dev, 1); - sscr0 |= SSCR0_ADC; + sscr0 |= SSCR0_ACS; break; default: return -ENODEV; @@ -524,7 +524,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_ADC); + (SSCR0_ECS | SSCR0_NCS | SSCR0_MOD | SSCR0_ACS); sscr1 = SSCR1_RxTresh(8) | SSCR1_TxTresh(7); sspsp = 0;
This patch adds bitfields for I2S serial formats that differ from the amount of data actually sent on the line. Some codecs (namely the cs4270) require 64 sysclks being sent during each frame period, even though the number of acutal data bits might be less.
Signed-off-by: Daniel Mack daniel@caiaq.de Cc: Mark Brown broonie@opensource.wolfsonmicro.com Cc: Liam Girdwood lrg@kernel.org Cc: Tim Ruetz tim@caiaq.de --- include/sound/soc-dai.h | 20 ++++++++++++++++---- 1 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h index 24247f7..83b617f 100644 --- a/include/sound/soc-dai.h +++ b/include/sound/soc-dai.h @@ -85,10 +85,22 @@ struct snd_pcm_substream; #define SND_SOC_DAIFMT_CBM_CFS (2 << 12) /* codec clk master & frame slave */ #define SND_SOC_DAIFMT_CBS_CFS (3 << 12) /* codec clk & frm slave */
-#define SND_SOC_DAIFMT_FORMAT_MASK 0x000f -#define SND_SOC_DAIFMT_CLOCK_MASK 0x00f0 -#define SND_SOC_DAIFMT_INV_MASK 0x0f00 -#define SND_SOC_DAIFMT_MASTER_MASK 0xf000 +/* + * DAI hardware frame formats. + * + * Independently, these bits the define the audio format on the + * pysical channel in case it differs from the audio sample the + * DAI is configured to + */ +#define SND_SOC_DAIFMT_FF_SAMPLE (0 << 16) +#define SND_SOC_DAIFMT_FF_I2S_16 (1 << 16) +#define SND_SOC_DAIFMT_FF_I2S_32 (2 << 16) + +#define SND_SOC_DAIFMT_FORMAT_MASK 0x0000f +#define SND_SOC_DAIFMT_CLOCK_MASK 0x000f0 +#define SND_SOC_DAIFMT_INV_MASK 0x00f00 +#define SND_SOC_DAIFMT_MASTER_MASK 0x0f000 +#define SND_SOC_DAIFMT_FRAME_FORMAT_MASK 0xf0000
/* * Master Clock Directions
In pxa_ssp_set_dai_fmt(), don't modify the SSP registers in case the stream is already running. With that patch applied, loop-thru tests like 'acrecord -f cd | aplay -f cd' succeed.
Signed-off-by: Daniel Mack daniel@caiaq.de Cc: Mark Brown broonie@opensource.wolfsonmicro.com Cc: Liam Girdwood lrg@kernel.org Cc: Philipp Zabel philipp.zabel@gmail.com Cc: Tim Ruetz tim@caiaq.de --- sound/soc/pxa/pxa-ssp.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/sound/soc/pxa/pxa-ssp.c b/sound/soc/pxa/pxa-ssp.c index 7fc13f0..45fb600 100644 --- a/sound/soc/pxa/pxa-ssp.c +++ b/sound/soc/pxa/pxa-ssp.c @@ -521,6 +521,10 @@ static int pxa_ssp_set_dai_fmt(struct snd_soc_dai *cpu_dai, u32 sscr0; u32 sscr1; u32 sspsp; + + /* we can only change the settings if the port is not in use */ + if (ssp_read_reg(ssp, SSCR0) & SSCR0_SSE) + return 0;
/* reset port settings */ sscr0 = ssp_read_reg(ssp, SSCR0) &
This patch uses the SSP's PSP functionality to provide I2S timings. The particular problem is that even though the datasheets state it should be possbible, there is no mode which uses the network feature with its associated time slots in a sane way to do what we need.
Hence, in order to have full 64 bit I2S on the wire, we need to fiddle around with the SSP and the timing paramters a lot. There are some constants left in the code which can't be replaced by names because the true meaning of their registers remains nebulous.
Signed-off-by: Daniel Mack daniel@caiaq.de Signed-off-by: Tim Ruetz tim@caiaq.de Cc: Mark Brown broonie@opensource.wolfsonmicro.com Cc: Liam Girdwood lrg@kernel.org Cc: Philipp Zabel philipp.zabel@gmail.com --- sound/soc/pxa/pxa-ssp.c | 39 +++++++++++++++++++++------------------ 1 files changed, 21 insertions(+), 18 deletions(-)
diff --git a/sound/soc/pxa/pxa-ssp.c b/sound/soc/pxa/pxa-ssp.c index 45fb600..97f11d6 100644 --- a/sound/soc/pxa/pxa-ssp.c +++ b/sound/soc/pxa/pxa-ssp.c @@ -319,6 +319,7 @@ static int pxa_ssp_set_dai_sysclk(struct snd_soc_dai *cpu_dai, break; case PXA_SSP_CLK_EXT: priv->sysclk = freq; + ssp_set_scr(&priv->dev, 4); sscr0 |= SSCR0_ECS; break; case PXA_SSP_CLK_NET: @@ -551,17 +552,13 @@ 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_MOD | SSCR0_PSP; + sscr0 |= SSCR0_PSP; sscr1 |= SSCR1_RWOT | SSCR1_TRAIL;
switch (fmt & SND_SOC_DAIFMT_INV_MASK) { case SND_SOC_DAIFMT_NB_NF: - sspsp |= SSPSP_FSRT; break; case SND_SOC_DAIFMT_NB_IF: - sspsp |= SSPSP_SFRMP | SSPSP_FSRT; - break; - case SND_SOC_DAIFMT_IB_IF: sspsp |= SSPSP_SFRMP; break; default: @@ -652,33 +649,39 @@ static int pxa_ssp_hw_params(struct snd_pcm_substream *substream, break; case SNDRV_PCM_FORMAT_S24_LE: sscr0 |= (SSCR0_EDSS | SSCR0_DataSize(8)); - /* we must be in network mode (2 slots) for 24 bit stereo */ break; case SNDRV_PCM_FORMAT_S32_LE: sscr0 |= (SSCR0_EDSS | SSCR0_DataSize(16)); - /* we must be in network mode (2 slots) for 32 bit stereo */ break; } ssp_write_reg(ssp, SSCR0, sscr0);
switch (priv->dai_fmt & SND_SOC_DAIFMT_FORMAT_MASK) { case SND_SOC_DAIFMT_I2S: - /* Cleared when the DAI format is set */ - sspsp = ssp_read_reg(ssp, SSPSP) | SSPSP_SFRMWDTH(width); + sspsp = ssp_read_reg(ssp, SSPSP); + + switch (priv->dai_fmt & SND_SOC_DAIFMT_FRAME_FORMAT_MASK) { + case SND_SOC_DAIFMT_FF_I2S_32: + /* These values are all found out by trying and + * failing a lot. PXA's SSP is all black magic and + * does not work like described in any datasheet. + */ + sspsp |= SSPSP_SFRMWDTH(32); + sspsp |= SSPSP_SFRMDLY(32 * 2); + sspsp |= SSPSP_EDMYSTOP(3); + sspsp |= SSPSP_DMYSTOP(3); + sspsp |= SSPSP_DMYSTRT(1); + break; + default: + /* Cleared when the DAI format is set */ + sspsp |= SSPSP_SFRMWDTH(width); + break; + } ssp_write_reg(ssp, SSPSP, sspsp); - break; default: break; }
- /* We always use a network mode so we always require TDM slots - * - complain loudly and fail if they've not been set up yet. - */ - if (!(ssp_read_reg(ssp, SSTSA) & 0xf)) { - dev_err(&ssp->pdev->dev, "No TDM timeslot configured\n"); - return -EINVAL; - } - dump_registers(ssp);
return 0;
On Wed, Mar 4, 2009 at 9:17 PM, Daniel Mack daniel@caiaq.de wrote:
This patch uses the SSP's PSP functionality to provide I2S timings. The particular problem is that even though the datasheets state it should be possbible, there is no mode which uses the network feature with its associated time slots in a sane way to do what we need.
Hence, in order to have full 64 bit I2S on the wire, we need to fiddle around with the SSP and the timing paramters a lot. There are some constants left in the code which can't be replaced by names because the true meaning of their registers remains nebulous.
Signed-off-by: Daniel Mack daniel@caiaq.de Signed-off-by: Tim Ruetz tim@caiaq.de Cc: Mark Brown broonie@opensource.wolfsonmicro.com Cc: Liam Girdwood lrg@kernel.org Cc: Philipp Zabel philipp.zabel@gmail.com
sound/soc/pxa/pxa-ssp.c | 39 +++++++++++++++++++++------------------ 1 files changed, 21 insertions(+), 18 deletions(-)
diff --git a/sound/soc/pxa/pxa-ssp.c b/sound/soc/pxa/pxa-ssp.c index 45fb600..97f11d6 100644 --- a/sound/soc/pxa/pxa-ssp.c +++ b/sound/soc/pxa/pxa-ssp.c @@ -319,6 +319,7 @@ static int pxa_ssp_set_dai_sysclk(struct snd_soc_dai *cpu_dai, break; case PXA_SSP_CLK_EXT: priv->sysclk = freq;
- ssp_set_scr(&priv->dev, 4);
Shouldn't this be somehow set by set_dai_clkdiv instead?
sscr0 |= SSCR0_ECS; break; case PXA_SSP_CLK_NET: @@ -551,17 +552,13 @@ 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_MOD | SSCR0_PSP;
- sscr0 |= SSCR0_PSP;
sscr1 |= SSCR1_RWOT | SSCR1_TRAIL;
switch (fmt & SND_SOC_DAIFMT_INV_MASK) { case SND_SOC_DAIFMT_NB_NF:
- sspsp |= SSPSP_FSRT;
break; case SND_SOC_DAIFMT_NB_IF:
- sspsp |= SSPSP_SFRMP | SSPSP_FSRT;
- break;
- case SND_SOC_DAIFMT_IB_IF:
sspsp |= SSPSP_SFRMP; break;
Removal of SSPSP_FSRT from NB/IB selection seems to be correct from the docs. Can you check if IB could be properly handled by setting SCMODE(1)?
default: @@ -652,33 +649,39 @@ static int pxa_ssp_hw_params(struct snd_pcm_substream *substream, break; case SNDRV_PCM_FORMAT_S24_LE: sscr0 |= (SSCR0_EDSS | SSCR0_DataSize(8));
- /* we must be in network mode (2 slots) for 24 bit stereo */
This is still dubious ... S24_LE is 24-bit sound LSB-aligned in 32-bit frames, so DataSize should be 32 here.
break; case SNDRV_PCM_FORMAT_S32_LE: sscr0 |= (SSCR0_EDSS | SSCR0_DataSize(16));
- /* we must be in network mode (2 slots) for 32 bit stereo */
How is it possible to send 64bit in one frame otherwise?
break; } ssp_write_reg(ssp, SSCR0, sscr0);
switch (priv->dai_fmt & SND_SOC_DAIFMT_FORMAT_MASK) { case SND_SOC_DAIFMT_I2S:
- /* Cleared when the DAI format is set */
- sspsp = ssp_read_reg(ssp, SSPSP) | SSPSP_SFRMWDTH(width);
- sspsp = ssp_read_reg(ssp, SSPSP);
- switch (priv->dai_fmt & SND_SOC_DAIFMT_FRAME_FORMAT_MASK) {
- case SND_SOC_DAIFMT_FF_I2S_32:
- /* These values are all found out by trying and
- * failing a lot. PXA's SSP is all black magic and
- * does not work like described in any datasheet.
- */
- sspsp |= SSPSP_SFRMWDTH(32);
- sspsp |= SSPSP_SFRMDLY(32 * 2);
- sspsp |= SSPSP_EDMYSTOP(3);
- sspsp |= SSPSP_DMYSTOP(3);
- sspsp |= SSPSP_DMYSTRT(1);
Wha?! Amazing. And this really works? How the hell can this result in 16 bits of data followed by 16 bits of zeroes, twice :)
- break;
- default:
- /* Cleared when the DAI format is set */
- sspsp |= SSPSP_SFRMWDTH(width);
Not good for DSP_A/B.
- break;
- }
ssp_write_reg(ssp, SSPSP, sspsp);
- break;
default: break; }
- /* We always use a network mode so we always require TDM slots
- * - complain loudly and fail if they've not been set up yet.
- */
- if (!(ssp_read_reg(ssp, SSTSA) & 0xf)) {
- dev_err(&ssp->pdev->dev, "No TDM timeslot configured\n");
- return -EINVAL;
- }
dump_registers(ssp);
return 0;
1.6.1.3
regards Philipp
Hi Philipp,
On Wed, Mar 04, 2009 at 09:56:01PM +0100, pHilipp Zabel wrote:
diff --git a/sound/soc/pxa/pxa-ssp.c b/sound/soc/pxa/pxa-ssp.c index 45fb600..97f11d6 100644 --- a/sound/soc/pxa/pxa-ssp.c +++ b/sound/soc/pxa/pxa-ssp.c @@ -319,6 +319,7 @@ static int pxa_ssp_set_dai_sysclk(struct snd_soc_dai *cpu_dai, break; case PXA_SSP_CLK_EXT: priv->sysclk = freq;
- ssp_set_scr(&priv->dev, 4);
Shouldn't this be somehow set by set_dai_clkdiv instead?
This is actually a very common case - MCLK is BCLK*4, so I put it here. We could still move it to some more configurable place if it turns out we need to configure it per board.
switch (fmt & SND_SOC_DAIFMT_INV_MASK) { case SND_SOC_DAIFMT_NB_NF:
- sspsp |= SSPSP_FSRT;
break; case SND_SOC_DAIFMT_NB_IF:
- sspsp |= SSPSP_SFRMP | SSPSP_FSRT;
- break;
- case SND_SOC_DAIFMT_IB_IF:
sspsp |= SSPSP_SFRMP; break;
Removal of SSPSP_FSRT from NB/IB selection seems to be correct from the docs.
SSPSP_FSRT has a totally different meaning according to the PXA3xx docs, but I'll have a look at the PXA2x specs - maybe we need a special case here. Unfortunately, I'm not able to quote from PXA3x specs here due to NDA restrictions.
Can you check if IB could be properly handled by setting SCMODE(1)?
Can't follow that - what are you referring to here?
default: @@ -652,33 +649,39 @@ static int pxa_ssp_hw_params(struct snd_pcm_substream *substream, break; case SNDRV_PCM_FORMAT_S24_LE: sscr0 |= (SSCR0_EDSS | SSCR0_DataSize(8));
- /* we must be in network mode (2 slots) for 24 bit stereo */
This is still dubious ... S24_LE is 24-bit sound LSB-aligned in 32-bit frames, so DataSize should be 32 here.
I didn't test that, and I didn't change it either - I just removed the comment, as we're not running in network mode anymore.
break; case SNDRV_PCM_FORMAT_S32_LE: sscr0 |= (SSCR0_EDSS | SSCR0_DataSize(16));
- /* we must be in network mode (2 slots) for 32 bit stereo */
How is it possible to send 64bit in one frame otherwise?
We're not running in network mode anymore - things are different now :)
- switch (priv->dai_fmt & SND_SOC_DAIFMT_FRAME_FORMAT_MASK) {
- case SND_SOC_DAIFMT_FF_I2S_32:
- /* These values are all found out by trying and
- * failing a lot. PXA's SSP is all black magic and
- * does not work like described in any datasheet.
- */
- sspsp |= SSPSP_SFRMWDTH(32);
- sspsp |= SSPSP_SFRMDLY(32 * 2);
- sspsp |= SSPSP_EDMYSTOP(3);
- sspsp |= SSPSP_DMYSTOP(3);
- sspsp |= SSPSP_DMYSTRT(1);
Wha?! Amazing. And this really works? How the hell can this result in 16 bits of data followed by 16 bits of zeroes, twice :)
Don't ask :) If we could explain in detail what these registers mean, I'd rather make them macro values, but unfortunately, the comment is correct ...
- break;
- default:
- /* Cleared when the DAI format is set */
- sspsp |= SSPSP_SFRMWDTH(width);
Not good for DSP_A/B.
This is the SND_SOC_DAIFMT_I2S case, so DSP modes will be unaffected.
Could you try the patches on your board?
Thanks, Daniel
On Thu, Mar 05, 2009 at 12:03:01AM +0100, Daniel Mack wrote:
switch (fmt & SND_SOC_DAIFMT_INV_MASK) { case SND_SOC_DAIFMT_NB_NF:
- sspsp |= SSPSP_FSRT;
break; case SND_SOC_DAIFMT_NB_IF:
- sspsp |= SSPSP_SFRMP | SSPSP_FSRT;
- break;
- case SND_SOC_DAIFMT_IB_IF:
sspsp |= SSPSP_SFRMP; break;
Removal of SSPSP_FSRT from NB/IB selection seems to be correct from the docs.
SSPSP_FSRT has a totally different meaning according to the PXA3xx docs, but I'll have a look at the PXA2x specs - maybe we need a special case here.
Sorry, got you wrong. I compared that to the PXA27x spec and it turns out that this bit has no different meaning there. So it is definitely wrong to use it for inverting the bitclk, no special case needed.
Daniel
On Thu, Mar 05, 2009 at 12:03:01AM +0100, Daniel Mack wrote:
On Wed, Mar 04, 2009 at 09:56:01PM +0100, pHilipp Zabel wrote:
Shouldn't this be somehow set by set_dai_clkdiv instead?
This is actually a very common case - MCLK is BCLK*4, so I put it here. We could still move it to some more configurable place if it turns out we need to configure it per board.
It's a common option but far from universal - apart from anything else normally the MCLK requirement is a multiple of the sampling frequency rather than the bitclock rate so the ratio with BCLK will often vary with sample size. I've seen devices able to operate as low as 64fs and IIRC I've seen things up to 512fs, though 256fs is more common.
On Thu, Mar 05, 2009 at 11:21:33AM +0000, Mark Brown wrote:
Shouldn't this be somehow set by set_dai_clkdiv instead?
This is actually a very common case - MCLK is BCLK*4, so I put it here. We could still move it to some more configurable place if it turns out we need to configure it per board.
It's a common option but far from universal - apart from anything else normally the MCLK requirement is a multiple of the sampling frequency rather than the bitclock rate so the ratio with BCLK will often vary with sample size. I've seen devices able to operate as low as 64fs and IIRC I've seen things up to 512fs, though 256fs is more common.
Ok, agreed. I moved that to my board file (using DIV_SCR) and will drop that piece in the next patches.
Daniel
On Thu, Mar 5, 2009 at 12:03 AM, Daniel Mack daniel@caiaq.de wrote:
Hi Philipp,
On Wed, Mar 04, 2009 at 09:56:01PM +0100, pHilipp Zabel wrote:
diff --git a/sound/soc/pxa/pxa-ssp.c b/sound/soc/pxa/pxa-ssp.c index 45fb600..97f11d6 100644 --- a/sound/soc/pxa/pxa-ssp.c +++ b/sound/soc/pxa/pxa-ssp.c @@ -319,6 +319,7 @@ static int pxa_ssp_set_dai_sysclk(struct snd_soc_dai *cpu_dai, break; case PXA_SSP_CLK_EXT: priv->sysclk = freq;
- ssp_set_scr(&priv->dev, 4);
Shouldn't this be somehow set by set_dai_clkdiv instead?
This is actually a very common case - MCLK is BCLK*4, so I put it here. We could still move it to some more configurable place if it turns out we need to configure it per board.
switch (fmt & SND_SOC_DAIFMT_INV_MASK) { case SND_SOC_DAIFMT_NB_NF:
- sspsp |= SSPSP_FSRT;
break; case SND_SOC_DAIFMT_NB_IF:
- sspsp |= SSPSP_SFRMP | SSPSP_FSRT;
- break;
- case SND_SOC_DAIFMT_IB_IF:
sspsp |= SSPSP_SFRMP; break;
Removal of SSPSP_FSRT from NB/IB selection seems to be correct from the docs.
SSPSP_FSRT has a totally different meaning according to the PXA3xx docs, but I'll have a look at the PXA2x specs - maybe we need a special case here. Unfortunately, I'm not able to quote from PXA3x specs here due to NDA restrictions.
Can you check if IB could be properly handled by setting SCMODE(1)?
Can't follow that - what are you referring to here?
default: @@ -652,33 +649,39 @@ static int pxa_ssp_hw_params(struct snd_pcm_substream *substream, break; case SNDRV_PCM_FORMAT_S24_LE: sscr0 |= (SSCR0_EDSS | SSCR0_DataSize(8));
- /* we must be in network mode (2 slots) for 24 bit stereo */
This is still dubious ... S24_LE is 24-bit sound LSB-aligned in 32-bit frames, so DataSize should be 32 here.
I didn't test that, and I didn't change it either - I just removed the comment, as we're not running in network mode anymore.
break; case SNDRV_PCM_FORMAT_S32_LE: sscr0 |= (SSCR0_EDSS | SSCR0_DataSize(16));
- /* we must be in network mode (2 slots) for 32 bit stereo */
How is it possible to send 64bit in one frame otherwise?
We're not running in network mode anymore - things are different now :)
- switch (priv->dai_fmt & SND_SOC_DAIFMT_FRAME_FORMAT_MASK) {
- case SND_SOC_DAIFMT_FF_I2S_32:
- /* These values are all found out by trying and
- * failing a lot. PXA's SSP is all black magic and
- * does not work like described in any datasheet.
- */
- sspsp |= SSPSP_SFRMWDTH(32);
- sspsp |= SSPSP_SFRMDLY(32 * 2);
- sspsp |= SSPSP_EDMYSTOP(3);
- sspsp |= SSPSP_DMYSTOP(3);
- sspsp |= SSPSP_DMYSTRT(1);
Wha?! Amazing. And this really works? How the hell can this result in 16 bits of data followed by 16 bits of zeroes, twice :)
Don't ask :) If we could explain in detail what these registers mean, I'd rather make them macro values, but unfortunately, the comment is correct ...
- break;
- default:
- /* Cleared when the DAI format is set */
- sspsp |= SSPSP_SFRMWDTH(width);
Not good for DSP_A/B.
This is the SND_SOC_DAIFMT_I2S case, so DSP modes will be unaffected.
Could you try the patches on your board?
Done. They don't interfere with my setup.
regards Philipp
On Wed, Mar 04, 2009 at 09:17:00PM +0100, Daniel Mack wrote:
switch (priv->dai_fmt & SND_SOC_DAIFMT_FRAME_FORMAT_MASK) {case SND_SOC_DAIFMT_FF_I2S_32:/* These values are all found out by trying and* failing a lot. PXA's SSP is all black magic and* does not work like described in any datasheet.*/sspsp |= SSPSP_SFRMWDTH(32);sspsp |= SSPSP_SFRMDLY(32 * 2);sspsp |= SSPSP_EDMYSTOP(3);sspsp |= SSPSP_DMYSTOP(3);sspsp |= SSPSP_DMYSTRT(1);break;
*sigh*
I just saw that one of these bit fields we write to here is only available on PXA3xx - so we probably need the whole logic to be different for PXA2xx which makes everthing a lot more complicated.
Anyway, could the patch below be taken?
Thanks, Daniel
On Thu, Mar 05, 2009 at 02:21:26PM +0100, Daniel Mack wrote:
I just saw that one of these bit fields we write to here is only available on PXA3xx - so we probably need the whole logic to be different for PXA2xx which makes everthing a lot more complicated.
At some point it's going to be better to just split the function using these into PXA2xx and PXA3xx variants.
Anyway, could the patch below be taken?
Yes, sure. Will apply.
On Wed, Mar 04, 2009 at 09:16:59PM +0100, Daniel Mack wrote:
In pxa_ssp_set_dai_fmt(), don't modify the SSP registers in case the stream is already running. With that patch applied, loop-thru tests like 'acrecord -f cd | aplay -f cd' succeed.
- /* we can only change the settings if the port is not in use */
- if (ssp_read_reg(ssp, SSCR0) & SSCR0_SSE)
return 0;
I'd expect an error to be reported here - if we needed to change the settings and can't things could go wrong. Ideally it'd check to see if the DAI format was being changed and only error if it was.
On Wed, Mar 04, 2009 at 08:33:52PM +0000, Mark Brown wrote:
In pxa_ssp_set_dai_fmt(), don't modify the SSP registers in case the stream is already running. With that patch applied, loop-thru tests like 'acrecord -f cd | aplay -f cd' succeed.
- /* we can only change the settings if the port is not in use */
- if (ssp_read_reg(ssp, SSCR0) & SSCR0_SSE)
return 0;I'd expect an error to be reported here - if we needed to change the settings and can't things could go wrong. Ideally it'd check to see if the DAI format was being changed and only error if it was.
In my tests, pxa_ssp_set_dai_fmt() was simply called twice, once for the input, once for the ouput. Hence I believed a silent exit in such a case fine. Does 'arecord -f cd | aplay -f cd' work for you?
Daniel
On Wed, Mar 04, 2009 at 09:39:11PM +0100, Daniel Mack wrote:
On Wed, Mar 04, 2009 at 08:33:52PM +0000, Mark Brown wrote:
I'd expect an error to be reported here - if we needed to change the settings and can't things could go wrong. Ideally it'd check to see if the DAI format was being changed and only error if it was.
In my tests, pxa_ssp_set_dai_fmt() was simply called twice, once for the input, once for the ouput. Hence I believed a silent exit in such a case
That'll be because you weren't changing the DAI format - hence my comment about only erroring if the format is changed.
fine. Does 'arecord -f cd | aplay -f cd' work for you?
It won't, but that's because I don't have hardware that can play back cd format audio through the SSP port :)
On Wed, Mar 04, 2009 at 08:42:22PM +0000, Mark Brown wrote:
On Wed, Mar 04, 2009 at 09:39:11PM +0100, Daniel Mack wrote:
On Wed, Mar 04, 2009 at 08:33:52PM +0000, Mark Brown wrote:
I'd expect an error to be reported here - if we needed to change the settings and can't things could go wrong. Ideally it'd check to see if the DAI format was being changed and only error if it was.
In my tests, pxa_ssp_set_dai_fmt() was simply called twice, once for the input, once for the ouput. Hence I believed a silent exit in such a case
That'll be because you weren't changing the DAI format - hence my comment about only erroring if the format is changed.
Ok, that makes sense. We should probably do something silimar in set_hw_params() were we also currently exit silently in case of running streams.
I'll post a new set soon when we sorted out more things on the other patches.
Daniel
Hi Mark,
On Thu, Mar 05, 2009 at 11:23:37AM +0100, Daniel Mack wrote:
On Wed, Mar 04, 2009 at 08:42:22PM +0000, Mark Brown wrote:
On Wed, Mar 04, 2009 at 09:39:11PM +0100, Daniel Mack wrote:
On Wed, Mar 04, 2009 at 08:33:52PM +0000, Mark Brown wrote:
I'd expect an error to be reported here - if we needed to change the settings and can't things could go wrong. Ideally it'd check to see if the DAI format was being changed and only error if it was.
In my tests, pxa_ssp_set_dai_fmt() was simply called twice, once for the input, once for the ouput. Hence I believed a silent exit in such a case
That'll be because you weren't changing the DAI format - hence my comment about only erroring if the format is changed.
Ok, that makes sense. We should probably do something silimar in set_hw_params() were we also currently exit silently in case of running streams.
Could you have a look on the patch below please - does that look better?
Thanks, Daniel
On Wed, Mar 04, 2009 at 09:16:58PM +0100, Daniel Mack wrote:
This patch adds bitfields for I2S serial formats that differ from the amount of data actually sent on the line. Some codecs (namely the cs4270) require 64 sysclks being sent during each frame period, even though the number of acutal data bits might be less.
Hrm. This is normally handled via the clock divider interface - it's often much more straightforward to work with when dealing with devices with very flexible clocking, especially if there are more than two devices on the link. Have you considered handling this through that, perhaps through adding a virtual thing to configure (eg, a PXA_SSP_FRAME_CLOCKS)? Normally this would be a network mode but it seems clear that that has some issues on this hardware and an alternative solution is required.
The other downside of this approach is that it makes all existing drivers theoretically instabuggy in that they don't reject invalid configurations, although that's not such a big issue since it really only makes life easier when writing board drivers.
I guess cs4270.c ought to be enforcing this if it's added...
+#define SND_SOC_DAIFMT_FF_SAMPLE (0 << 16) +#define SND_SOC_DAIFMT_FF_I2S_16 (1 << 16) +#define SND_SOC_DAIFMT_FF_I2S_32 (2 << 16)
FF_SAMPLE should probably be FF_UNSPEC or something to preserve the existing semantics.
FF_I2S should probably be something else since something might need this for non-I2S devices - _BIT, perhaps?
Hi Mark,
On Wed, Mar 04, 2009 at 10:30:58PM +0000, Mark Brown wrote:
On Wed, Mar 04, 2009 at 09:16:58PM +0100, Daniel Mack wrote:
This patch adds bitfields for I2S serial formats that differ from the amount of data actually sent on the line. Some codecs (namely the cs4270) require 64 sysclks being sent during each frame period, even though the number of acutal data bits might be less.
Hrm. This is normally handled via the clock divider interface - it's often much more straightforward to work with when dealing with devices with very flexible clocking, especially if there are more than two devices on the link. Have you considered handling this through that, perhaps through adding a virtual thing to configure (eg, a PXA_SSP_FRAME_CLOCKS)?
I thought about that but then I condidered it's actually the wrong place for such a setting. I see two statements the board file has to make:
1) "We need to have 64 bits per frame for the codec" 2) "The sample data you'll be finding in the FIFO is 16 bits"
It could be possible to express that in clk division rations, but it is actually a lot more straight forward to make that setting once rather then changing it over and over again.
Normally this would be a network mode but it seems clear that that has some issues on this hardware and an alternative solution is required.
The other downside of this approach is that it makes all existing drivers theoretically instabuggy in that they don't reject invalid configurations, although that's not such a big issue since it really only makes life easier when writing board drivers.
Well, it won't break things - the worst thing that can happen is that things are silently ignored. That bitfield was meant to be extended, wasn't it ;)
I guess cs4270.c ought to be enforcing this if it's added...
cs4270.c would need that flag to be set or fail otherwise. Don't know if that really makes life easier for board support files that are not mainline. Want me to do that?
+#define SND_SOC_DAIFMT_FF_SAMPLE (0 << 16) +#define SND_SOC_DAIFMT_FF_I2S_16 (1 << 16) +#define SND_SOC_DAIFMT_FF_I2S_32 (2 << 16)
FF_SAMPLE should probably be FF_UNSPEC or something to preserve the existing semantics.
Could do that, yes.
FF_I2S should probably be something else since something might need this for non-I2S devices - _BIT, perhaps?
For non-I2S devices, we could re-use the same bit space as they will never appear in the same format word anyway, right?
Anyway, I'm totally open to any other approach, this one just seems most straight forward :)
Thanks, Daniel
On Thu, Mar 05, 2009 at 12:12:23AM +0100, Daniel Mack wrote:
On Wed, Mar 04, 2009 at 10:30:58PM +0000, Mark Brown wrote:
devices on the link. Have you considered handling this through that, perhaps through adding a virtual thing to configure (eg, a PXA_SSP_FRAME_CLOCKS)?
I thought about that but then I condidered it's actually the wrong place for such a setting. I see two statements the board file has to make:
The other option would be to use the TDM mode interface to set this up since that's roughly what it is; my main concern here is that we already have two ways of doing this and I'd like to try to keep the number down for consistency.
It could be possible to express that in clk division rations, but it is actually a lot more straight forward to make that setting once rather then changing it over and over again.
I think it's all much of a muchness in terms of complexity whatever option is chosen.
I guess cs4270.c ought to be enforcing this if it's added...
cs4270.c would need that flag to be set or fail otherwise. Don't know if that really makes life easier for board support files that are not mainline. Want me to do that?
If this gets merged it's probably for the best (at least when it's in slave mode). Or add a hint to that effect.
FF_I2S should probably be something else since something might need this for non-I2S devices - _BIT, perhaps?
For non-I2S devices, we could re-use the same bit space as they will never appear in the same format word anyway, right?
Yes, I'm just thinking we should be able to use the same name for this with every format.
On Thu, Mar 05, 2009 at 10:53:57AM +0000, Mark Brown wrote:
On Wed, Mar 04, 2009 at 10:30:58PM +0000, Mark Brown wrote:
devices on the link. Have you considered handling this through that, perhaps through adding a virtual thing to configure (eg, a PXA_SSP_FRAME_CLOCKS)?
I thought about that but then I condidered it's actually the wrong place for such a setting. I see two statements the board file has to make:
The other option would be to use the TDM mode interface to set this up since that's roughly what it is; my main concern here is that we already have two ways of doing this and I'd like to try to keep the number down for consistency.
Probably a matter of taste, but IMO TDM mode is not the place either. Userspace could decide sending out 24bit samples out of a sudden (which the CPU DAI might accept) and in this case, you'd need some special logic in the board file again to set up deviders, time slots etc, right?
I guess cs4270.c ought to be enforcing this if it's added...
cs4270.c would need that flag to be set or fail otherwise. Don't know if that really makes life easier for board support files that are not mainline. Want me to do that?
If this gets merged it's probably for the best (at least when it's in slave mode). Or add a hint to that effect.
It a hard requirement for the master mode only. I'll add a warning for this case.
For non-I2S devices, we could re-use the same bit space as they will never appear in the same format word anyway, right?
Yes, I'm just thinking we should be able to use the same name for this with every format.
Ok, I now call them SND_SOC_DAIFMT_FF_{UNSPEC,16BIT,24BIT,32BIT}. Are you fine with these names?
Daniel
On Thu, Mar 05, 2009 at 12:31:41PM +0100, Daniel Mack wrote:
On Thu, Mar 05, 2009 at 10:53:57AM +0000, Mark Brown wrote:
The other option would be to use the TDM mode interface to set this up since that's roughly what it is; my main concern here is that we already have two ways of doing this and I'd like to try to keep the number down for consistency.
Probably a matter of taste, but IMO TDM mode is not the place either. Userspace could decide sending out 24bit samples out of a sudden (which the CPU DAI might accept) and in this case, you'd need some special logic in the board file again to set up deviders, time slots etc, right?
That's why the clocking interface is generally used here - it's more orthogonal.
Yes, I'm just thinking we should be able to use the same name for this with every format.
Ok, I now call them SND_SOC_DAIFMT_FF_{UNSPEC,16BIT,24BIT,32BIT}. Are you fine with these names?
Yes, though I'd still prefer to use an existing interface rather than add a new one.
On Thu, Mar 05, 2009 at 12:03:35PM +0000, Mark Brown wrote:
The other option would be to use the TDM mode interface to set this up since that's roughly what it is; my main concern here is that we already have two ways of doing this and I'd like to try to keep the number down for consistency.
Probably a matter of taste, but IMO TDM mode is not the place either. Userspace could decide sending out 24bit samples out of a sudden (which the CPU DAI might accept) and in this case, you'd need some special logic in the board file again to set up deviders, time slots etc, right?
That's why the clocking interface is generally used here - it's more orthogonal.
Hmm, how would you express 24 sample bits with 32 clocks per channel? I might not have gotten your point yet - could you provide an example?
Daniel
On Thu, Mar 05, 2009 at 01:55:21PM +0100, Daniel Mack wrote:
On Thu, Mar 05, 2009 at 12:03:35PM +0000, Mark Brown wrote:
That's why the clocking interface is generally used here - it's more orthogonal.
Hmm, how would you express 24 sample bits with 32 clocks per channel? I might not have gotten your point yet - could you provide an example?
You could add a "clock" which is the number of clocks per channel. It doesn't need to correspond to a literal clock in the hardware, it'd just provide a way of setting this number.
On Thu, Mar 5, 2009 at 1:55 PM, Daniel Mack daniel@caiaq.de wrote:
On Thu, Mar 05, 2009 at 12:03:35PM +0000, Mark Brown wrote:
The other option would be to use the TDM mode interface to set this up since that's roughly what it is; my main concern here is that we already have two ways of doing this and I'd like to try to keep the number down for consistency.
Probably a matter of taste, but IMO TDM mode is not the place either. Userspace could decide sending out 24bit samples out of a sudden (which the CPU DAI might accept) and in this case, you'd need some special logic in the board file again to set up deviders, time slots etc, right?
That's why the clocking interface is generally used here - it's more orthogonal.
Hmm, how would you express 24 sample bits with 32 clocks per channel? I might not have gotten your point yet - could you provide an example?
I'm still a bit confused by the alsa formats and how they interact with the SSP formats.
Do I understand correctly that S24_3LE (24-bit packed in 3 bytes) can't be supported due to limitations in (at least PXA27x's) DMA engine? So the remaining 24-bit formats are S24_LE and S32_LE anyway, both of which are 24-bit packed in 32 bits (either LSB or MSB aligned).
regards Philipp
On Wed, Mar 04, 2009 at 09:16:57PM +0100, Daniel Mack wrote:
A bit in PXA's SSCR0 register was erroneously named ADC but its name is in fact ADC (audio clock select).
Applied, but I think I spot a typo in the changelog (which I've fixed). :)
participants (4)
-
Daniel Mack -
Mark Brown -
Mark Brown -
pHilipp Zabel