* PXA SSP and external clocked I2S again @ 2009-03-04 20:15 Daniel Mack 2009-03-04 20:16 ` [PATCH 1/4] pxa-ssp: fix name of register bit Daniel Mack 0 siblings, 1 reply; 28+ messages in thread From: Daniel Mack @ 2009-03-04 20:15 UTC (permalink / raw) To: alsa-devel; +Cc: Tim Ruetz 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/4] pxa-ssp: fix name of register bit 2009-03-04 20:15 PXA SSP and external clocked I2S again Daniel Mack @ 2009-03-04 20:16 ` Daniel Mack 2009-03-04 20:16 ` [PATCH 2/4] soc-dai: add bitfields for hardware I2S formats Daniel Mack 2009-03-05 11:40 ` [PATCH 1/4] pxa-ssp: fix name of register bit Mark Brown 0 siblings, 2 replies; 28+ messages in thread From: Daniel Mack @ 2009-03-04 20:16 UTC (permalink / raw) To: alsa-devel; +Cc: Mark Brown, Tim Ruetz, Liam Girdwood 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; -- 1.6.1.3 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 2/4] soc-dai: add bitfields for hardware I2S formats 2009-03-04 20:16 ` [PATCH 1/4] pxa-ssp: fix name of register bit Daniel Mack @ 2009-03-04 20:16 ` Daniel Mack 2009-03-04 20:16 ` [PATCH 3/4] pxa-ssp: don't touch ssp registers when stream is running Daniel Mack 2009-03-04 22:30 ` [PATCH 2/4] soc-dai: add bitfields for hardware I2S formats Mark Brown 2009-03-05 11:40 ` [PATCH 1/4] pxa-ssp: fix name of register bit Mark Brown 1 sibling, 2 replies; 28+ messages in thread From: Daniel Mack @ 2009-03-04 20:16 UTC (permalink / raw) To: alsa-devel; +Cc: Mark Brown, Tim Ruetz, Liam Girdwood 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 -- 1.6.1.3 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 3/4] pxa-ssp: don't touch ssp registers when stream is running 2009-03-04 20:16 ` [PATCH 2/4] soc-dai: add bitfields for hardware I2S formats Daniel Mack @ 2009-03-04 20:16 ` Daniel Mack 2009-03-04 20:17 ` [PATCH 4/4] pxa-ssp: switch from network mode to psp Daniel Mack 2009-03-04 20:33 ` [PATCH 3/4] pxa-ssp: don't touch ssp registers when stream is running Mark Brown 2009-03-04 22:30 ` [PATCH 2/4] soc-dai: add bitfields for hardware I2S formats Mark Brown 1 sibling, 2 replies; 28+ messages in thread From: Daniel Mack @ 2009-03-04 20:16 UTC (permalink / raw) To: alsa-devel; +Cc: Philipp Zabel, Mark Brown, Tim Ruetz, Liam Girdwood 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) & -- 1.6.1.3 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 4/4] pxa-ssp: switch from network mode to psp 2009-03-04 20:16 ` [PATCH 3/4] pxa-ssp: don't touch ssp registers when stream is running Daniel Mack @ 2009-03-04 20:17 ` Daniel Mack 2009-03-04 20:56 ` pHilipp Zabel 2009-03-05 13:21 ` Daniel Mack 2009-03-04 20:33 ` [PATCH 3/4] pxa-ssp: don't touch ssp registers when stream is running Mark Brown 1 sibling, 2 replies; 28+ messages in thread From: Daniel Mack @ 2009-03-04 20:17 UTC (permalink / raw) To: alsa-devel; +Cc: Philipp Zabel, Mark Brown, Tim Ruetz, Liam Girdwood 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; -- 1.6.1.3 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 4/4] pxa-ssp: switch from network mode to psp 2009-03-04 20:17 ` [PATCH 4/4] pxa-ssp: switch from network mode to psp Daniel Mack @ 2009-03-04 20:56 ` pHilipp Zabel 2009-03-04 23:03 ` Daniel Mack 2009-03-05 13:21 ` Daniel Mack 1 sibling, 1 reply; 28+ messages in thread From: pHilipp Zabel @ 2009-03-04 20:56 UTC (permalink / raw) To: Daniel Mack; +Cc: alsa-devel, Mark Brown, Tim Ruetz, Liam Girdwood 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 4/4] pxa-ssp: switch from network mode to psp 2009-03-04 20:56 ` pHilipp Zabel @ 2009-03-04 23:03 ` Daniel Mack 2009-03-05 10:36 ` Daniel Mack ` (2 more replies) 0 siblings, 3 replies; 28+ messages in thread From: Daniel Mack @ 2009-03-04 23:03 UTC (permalink / raw) To: pHilipp Zabel; +Cc: alsa-devel, Mark Brown, Tim Ruetz, Liam Girdwood 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 4/4] pxa-ssp: switch from network mode to psp 2009-03-04 23:03 ` Daniel Mack @ 2009-03-05 10:36 ` Daniel Mack 2009-03-05 11:21 ` Mark Brown 2009-03-05 16:01 ` pHilipp Zabel 2 siblings, 0 replies; 28+ messages in thread From: Daniel Mack @ 2009-03-05 10:36 UTC (permalink / raw) To: pHilipp Zabel; +Cc: alsa-devel, Mark Brown, Tim Ruetz, Liam Girdwood 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 4/4] pxa-ssp: switch from network mode to psp 2009-03-04 23:03 ` Daniel Mack 2009-03-05 10:36 ` Daniel Mack @ 2009-03-05 11:21 ` Mark Brown 2009-03-05 11:26 ` Daniel Mack 2009-03-05 16:01 ` pHilipp Zabel 2 siblings, 1 reply; 28+ messages in thread From: Mark Brown @ 2009-03-05 11:21 UTC (permalink / raw) To: Daniel Mack; +Cc: alsa-devel, Tim Ruetz, Liam Girdwood, pHilipp Zabel 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 4/4] pxa-ssp: switch from network mode to psp 2009-03-05 11:21 ` Mark Brown @ 2009-03-05 11:26 ` Daniel Mack 0 siblings, 0 replies; 28+ messages in thread From: Daniel Mack @ 2009-03-05 11:26 UTC (permalink / raw) To: Mark Brown; +Cc: alsa-devel, Tim Ruetz, Liam Girdwood, pHilipp Zabel 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 4/4] pxa-ssp: switch from network mode to psp 2009-03-04 23:03 ` Daniel Mack 2009-03-05 10:36 ` Daniel Mack 2009-03-05 11:21 ` Mark Brown @ 2009-03-05 16:01 ` pHilipp Zabel 2 siblings, 0 replies; 28+ messages in thread From: pHilipp Zabel @ 2009-03-05 16:01 UTC (permalink / raw) To: Daniel Mack; +Cc: alsa-devel, Mark Brown, Tim Ruetz, Liam Girdwood 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 4/4] pxa-ssp: switch from network mode to psp 2009-03-04 20:17 ` [PATCH 4/4] pxa-ssp: switch from network mode to psp Daniel Mack 2009-03-04 20:56 ` pHilipp Zabel @ 2009-03-05 13:21 ` Daniel Mack 2009-03-05 13:34 ` Mark Brown 1 sibling, 1 reply; 28+ messages in thread From: Daniel Mack @ 2009-03-05 13:21 UTC (permalink / raw) To: alsa-devel; +Cc: Mark Brown, Tim Ruetz, Philipp Zabel, Liam Girdwood 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 >From 44ef06d5209b498249da5c0cc7e0bc86355f9904 Mon Sep 17 00:00:00 2001 From: Daniel Mack <daniel@caiaq.de> Date: Thu, 5 Mar 2009 14:13:11 +0100 Subject: [PATCH] pxa-ssp: add two more bitfields Add two more bitfields for the PSP register. As they seem to exist for PXA3xx only, define them conditionally. Signed-off-by: Daniel Mack <daniel@caiaq.de> --- arch/arm/mach-pxa/include/mach/regs-ssp.h | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-) diff --git a/arch/arm/mach-pxa/include/mach/regs-ssp.h b/arch/arm/mach-pxa/include/mach/regs-ssp.h index a9ee355..018f6d6 100644 --- a/arch/arm/mach-pxa/include/mach/regs-ssp.h +++ b/arch/arm/mach-pxa/include/mach/regs-ssp.h @@ -109,6 +109,11 @@ #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 */ -- 1.6.1.3 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 4/4] pxa-ssp: switch from network mode to psp 2009-03-05 13:21 ` Daniel Mack @ 2009-03-05 13:34 ` Mark Brown 0 siblings, 0 replies; 28+ messages in thread From: Mark Brown @ 2009-03-05 13:34 UTC (permalink / raw) To: Daniel Mack; +Cc: alsa-devel, Tim Ruetz, Philipp Zabel, Liam Girdwood 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/4] pxa-ssp: don't touch ssp registers when stream is running 2009-03-04 20:16 ` [PATCH 3/4] pxa-ssp: don't touch ssp registers when stream is running Daniel Mack 2009-03-04 20:17 ` [PATCH 4/4] pxa-ssp: switch from network mode to psp Daniel Mack @ 2009-03-04 20:33 ` Mark Brown 2009-03-04 20:39 ` Daniel Mack 1 sibling, 1 reply; 28+ messages in thread From: Mark Brown @ 2009-03-04 20:33 UTC (permalink / raw) To: Daniel Mack; +Cc: alsa-devel, Tim Ruetz, Liam Girdwood, Philipp Zabel 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/4] pxa-ssp: don't touch ssp registers when stream is running 2009-03-04 20:33 ` [PATCH 3/4] pxa-ssp: don't touch ssp registers when stream is running Mark Brown @ 2009-03-04 20:39 ` Daniel Mack 2009-03-04 20:42 ` Mark Brown 0 siblings, 1 reply; 28+ messages in thread From: Daniel Mack @ 2009-03-04 20:39 UTC (permalink / raw) To: Mark Brown; +Cc: alsa-devel, Tim Ruetz, Liam Girdwood, Philipp Zabel 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/4] pxa-ssp: don't touch ssp registers when stream is running 2009-03-04 20:39 ` Daniel Mack @ 2009-03-04 20:42 ` Mark Brown 2009-03-05 10:23 ` Daniel Mack 0 siblings, 1 reply; 28+ messages in thread From: Mark Brown @ 2009-03-04 20:42 UTC (permalink / raw) To: Daniel Mack; +Cc: alsa-devel, Tim Ruetz, Philipp Zabel, Liam Girdwood 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 :) ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/4] pxa-ssp: don't touch ssp registers when stream is running 2009-03-04 20:42 ` Mark Brown @ 2009-03-05 10:23 ` Daniel Mack 2009-03-10 15:41 ` Daniel Mack 0 siblings, 1 reply; 28+ messages in thread From: Daniel Mack @ 2009-03-05 10:23 UTC (permalink / raw) To: Mark Brown; +Cc: alsa-devel, Tim Ruetz, Philipp Zabel, Liam Girdwood 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/4] pxa-ssp: don't touch ssp registers when stream is running 2009-03-05 10:23 ` Daniel Mack @ 2009-03-10 15:41 ` Daniel Mack 2009-03-10 15:56 ` Mark Brown 0 siblings, 1 reply; 28+ messages in thread From: Daniel Mack @ 2009-03-10 15:41 UTC (permalink / raw) To: Mark Brown; +Cc: alsa-devel, Liam Girdwood, Philipp Zabel 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 >From b0399b4d42265313768044ab6bb5754162b70fd5 Mon Sep 17 00:00:00 2001 From: Daniel Mack <daniel@caiaq.de> Date: Tue, 10 Mar 2009 16:36:58 +0100 Subject: [PATCH 2/2] pxa-ssp: don't touch registers if stream is running In pxa_ssp_set_dai_fmt(), check whether there is anything to do at all. If there would be but the SSP port is in use already, bail out. Signed-off-by: Daniel Mack <daniel@caiaq.de> Cc: Philipp Zabel <philipp.zabel@gmail.com> Cc: Liam Girdwood <lrg@kernel.org> --- sound/soc/pxa/pxa-ssp.c | 11 +++++++++++ 1 files changed, 11 insertions(+), 0 deletions(-) diff --git a/sound/soc/pxa/pxa-ssp.c b/sound/soc/pxa/pxa-ssp.c index 1b3a81c..569c0a6 100644 --- a/sound/soc/pxa/pxa-ssp.c +++ b/sound/soc/pxa/pxa-ssp.c @@ -525,6 +525,17 @@ static int pxa_ssp_set_dai_fmt(struct snd_soc_dai *cpu_dai, u32 sscr1; u32 sspsp; + /* check if we need to change anything at all */ + if (priv->dai_fmt == fmt) + return 0; + + /* we can only change the settings if the port is not in use */ + if (ssp_read_reg(ssp, SSCR0) & SSCR0_SSE) { + dev_err(&ssp->pdev->dev, + "can't change hardware dai format: stream is in use"); + return -EINVAL; + } + /* reset port settings */ sscr0 = ssp_read_reg(ssp, SSCR0) & (SSCR0_ECS | SSCR0_NCS | SSCR0_MOD | SSCR0_ACS); -- 1.6.2 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 3/4] pxa-ssp: don't touch ssp registers when stream is running 2009-03-10 15:41 ` Daniel Mack @ 2009-03-10 15:56 ` Mark Brown 0 siblings, 0 replies; 28+ messages in thread From: Mark Brown @ 2009-03-10 15:56 UTC (permalink / raw) To: Daniel Mack; +Cc: alsa-devel, Philipp Zabel, Liam Girdwood On Tue, Mar 10, 2009 at 04:41:00PM +0100, Daniel Mack wrote: > Could you have a look on the patch below please - does that look better? Looks good, I'll apply this. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/4] soc-dai: add bitfields for hardware I2S formats 2009-03-04 20:16 ` [PATCH 2/4] soc-dai: add bitfields for hardware I2S formats Daniel Mack 2009-03-04 20:16 ` [PATCH 3/4] pxa-ssp: don't touch ssp registers when stream is running Daniel Mack @ 2009-03-04 22:30 ` Mark Brown 2009-03-04 23:12 ` Daniel Mack 1 sibling, 1 reply; 28+ messages in thread From: Mark Brown @ 2009-03-04 22:30 UTC (permalink / raw) To: Daniel Mack; +Cc: alsa-devel, Tim Ruetz, Liam Girdwood 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? ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/4] soc-dai: add bitfields for hardware I2S formats 2009-03-04 22:30 ` [PATCH 2/4] soc-dai: add bitfields for hardware I2S formats Mark Brown @ 2009-03-04 23:12 ` Daniel Mack 2009-03-05 10:53 ` Mark Brown 0 siblings, 1 reply; 28+ messages in thread From: Daniel Mack @ 2009-03-04 23:12 UTC (permalink / raw) To: Mark Brown; +Cc: alsa-devel, Tim Ruetz, Liam Girdwood 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/4] soc-dai: add bitfields for hardware I2S formats 2009-03-04 23:12 ` Daniel Mack @ 2009-03-05 10:53 ` Mark Brown 2009-03-05 11:31 ` Daniel Mack 0 siblings, 1 reply; 28+ messages in thread From: Mark Brown @ 2009-03-05 10:53 UTC (permalink / raw) To: Daniel Mack; +Cc: alsa-devel, Tim Ruetz, Liam Girdwood 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/4] soc-dai: add bitfields for hardware I2S formats 2009-03-05 10:53 ` Mark Brown @ 2009-03-05 11:31 ` Daniel Mack 2009-03-05 12:03 ` Mark Brown 0 siblings, 1 reply; 28+ messages in thread From: Daniel Mack @ 2009-03-05 11:31 UTC (permalink / raw) To: Mark Brown; +Cc: alsa-devel, Tim Ruetz, Liam Girdwood 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/4] soc-dai: add bitfields for hardware I2S formats 2009-03-05 11:31 ` Daniel Mack @ 2009-03-05 12:03 ` Mark Brown 2009-03-05 12:55 ` Daniel Mack 0 siblings, 1 reply; 28+ messages in thread From: Mark Brown @ 2009-03-05 12:03 UTC (permalink / raw) To: Daniel Mack; +Cc: alsa-devel, Tim Ruetz, Liam Girdwood 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/4] soc-dai: add bitfields for hardware I2S formats 2009-03-05 12:03 ` Mark Brown @ 2009-03-05 12:55 ` Daniel Mack 2009-03-05 12:57 ` Mark Brown 2009-03-05 15:58 ` pHilipp Zabel 0 siblings, 2 replies; 28+ messages in thread From: Daniel Mack @ 2009-03-05 12:55 UTC (permalink / raw) To: Mark Brown; +Cc: alsa-devel, Tim Ruetz, Liam Girdwood 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/4] soc-dai: add bitfields for hardware I2S formats 2009-03-05 12:55 ` Daniel Mack @ 2009-03-05 12:57 ` Mark Brown 2009-03-05 15:58 ` pHilipp Zabel 1 sibling, 0 replies; 28+ messages in thread From: Mark Brown @ 2009-03-05 12:57 UTC (permalink / raw) To: Daniel Mack; +Cc: alsa-devel, Tim Ruetz, Liam Girdwood 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/4] soc-dai: add bitfields for hardware I2S formats 2009-03-05 12:55 ` Daniel Mack 2009-03-05 12:57 ` Mark Brown @ 2009-03-05 15:58 ` pHilipp Zabel 1 sibling, 0 replies; 28+ messages in thread From: pHilipp Zabel @ 2009-03-05 15:58 UTC (permalink / raw) To: Daniel Mack; +Cc: alsa-devel, Tim Ruetz, Mark Brown, Liam Girdwood 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/4] pxa-ssp: fix name of register bit 2009-03-04 20:16 ` [PATCH 1/4] pxa-ssp: fix name of register bit Daniel Mack 2009-03-04 20:16 ` [PATCH 2/4] soc-dai: add bitfields for hardware I2S formats Daniel Mack @ 2009-03-05 11:40 ` Mark Brown 1 sibling, 0 replies; 28+ messages in thread From: Mark Brown @ 2009-03-05 11:40 UTC (permalink / raw) To: Daniel Mack; +Cc: alsa-devel, Tim Ruetz, Liam Girdwood 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). :) ^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2009-03-10 15:56 UTC | newest] Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-03-04 20:15 PXA SSP and external clocked I2S again Daniel Mack 2009-03-04 20:16 ` [PATCH 1/4] pxa-ssp: fix name of register bit Daniel Mack 2009-03-04 20:16 ` [PATCH 2/4] soc-dai: add bitfields for hardware I2S formats Daniel Mack 2009-03-04 20:16 ` [PATCH 3/4] pxa-ssp: don't touch ssp registers when stream is running Daniel Mack 2009-03-04 20:17 ` [PATCH 4/4] pxa-ssp: switch from network mode to psp Daniel Mack 2009-03-04 20:56 ` pHilipp Zabel 2009-03-04 23:03 ` Daniel Mack 2009-03-05 10:36 ` Daniel Mack 2009-03-05 11:21 ` Mark Brown 2009-03-05 11:26 ` Daniel Mack 2009-03-05 16:01 ` pHilipp Zabel 2009-03-05 13:21 ` Daniel Mack 2009-03-05 13:34 ` Mark Brown 2009-03-04 20:33 ` [PATCH 3/4] pxa-ssp: don't touch ssp registers when stream is running Mark Brown 2009-03-04 20:39 ` Daniel Mack 2009-03-04 20:42 ` Mark Brown 2009-03-05 10:23 ` Daniel Mack 2009-03-10 15:41 ` Daniel Mack 2009-03-10 15:56 ` Mark Brown 2009-03-04 22:30 ` [PATCH 2/4] soc-dai: add bitfields for hardware I2S formats Mark Brown 2009-03-04 23:12 ` Daniel Mack 2009-03-05 10:53 ` Mark Brown 2009-03-05 11:31 ` Daniel Mack 2009-03-05 12:03 ` Mark Brown 2009-03-05 12:55 ` Daniel Mack 2009-03-05 12:57 ` Mark Brown 2009-03-05 15:58 ` pHilipp Zabel 2009-03-05 11:40 ` [PATCH 1/4] pxa-ssp: fix name of register bit Mark Brown
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.