PXA SSP and external clocked I2S again

PXA SSP and external clocked I2S again

[Daniel Mack]

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

[PATCH 1/4] pxa-ssp: fix name of register bit

[Daniel Mack]

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

[PATCH 2/4] soc-dai: add bitfields for hardware I2S formats

[Daniel Mack]

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

[PATCH 3/4] pxa-ssp: don't touch ssp registers when stream is running

[Daniel Mack]

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

[PATCH 4/4] pxa-ssp: switch from network mode to psp

[Daniel Mack]

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

Re: [PATCH 3/4] pxa-ssp: don't touch ssp registers when stream is running

[Mark Brown]

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.

Re: [PATCH 3/4] pxa-ssp: don't touch ssp registers when stream is running

[Daniel Mack]

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

Re: [PATCH 3/4] pxa-ssp: don't touch ssp registers when stream is running

[Mark Brown]

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 :)

Re: [PATCH 4/4] pxa-ssp: switch from network mode to psp

[pHilipp Zabel]

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

Re: [PATCH 2/4] soc-dai: add bitfields for hardware I2S formats

[Mark Brown]

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?

Re: [PATCH 4/4] pxa-ssp: switch from network mode to psp

[Daniel Mack]

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

Re: [PATCH 2/4] soc-dai: add bitfields for hardware I2S formats

[Daniel Mack]

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

Re: [PATCH 3/4] pxa-ssp: don't touch ssp registers when stream is running

[Daniel Mack]

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

Re: [PATCH 4/4] pxa-ssp: switch from network mode to psp

[Daniel Mack]

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

Re: [PATCH 2/4] soc-dai: add bitfields for hardware I2S formats

[Mark Brown]

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.

Re: [PATCH 4/4] pxa-ssp: switch from network mode to psp

[Mark Brown]

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.

Re: [PATCH 4/4] pxa-ssp: switch from network mode to psp

[Daniel Mack]

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

Re: [PATCH 2/4] soc-dai: add bitfields for hardware I2S formats

[Daniel Mack]

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

Re: [PATCH 1/4] pxa-ssp: fix name of register bit

[Mark Brown]

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

Re: [PATCH 2/4] soc-dai: add bitfields for hardware I2S formats

[Mark Brown]

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.

Re: [PATCH 2/4] soc-dai: add bitfields for hardware I2S formats

[Daniel Mack]

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

Re: [PATCH 2/4] soc-dai: add bitfields for hardware I2S formats

[Mark Brown]

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.

Re: [PATCH 4/4] pxa-ssp: switch from network mode to psp

[Daniel Mack]

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

Re: [PATCH 4/4] pxa-ssp: switch from network mode to psp

[Mark Brown]

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.

Re: [PATCH 2/4] soc-dai: add bitfields for hardware I2S formats

[pHilipp Zabel]

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

Re: [PATCH 4/4] pxa-ssp: switch from network mode to psp

[pHilipp Zabel]

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

Re: [PATCH 3/4] pxa-ssp: don't touch ssp registers when stream is running

[Daniel Mack]

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

Re: [PATCH 3/4] pxa-ssp: don't touch ssp registers when stream is running

[Mark Brown]

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.