snd-soc-pxa-ssp I2C/FIFO issues

snd-soc-pxa-ssp I2C/FIFO issues

[pHilipp Zabel]

Ok,

I'm not sure how to handle this correctly.

Magician needs stereo S16_LE input data from alsa, DAIFMT_DSP_A on the
wire with a frame size of 16 bit (one frame for each channel) and thus
2-byte DMA transfers to the FIFO (currently called 'mono' DMA params).
Zylonite's current code uses 32 bit frame size and 4-byte DMA
transfers to emulate I2S (S16_LE,stereo). Network mode seems to work,
so it could be changed to use two 16 bit slots instead with 2-byte DMA
transfers (which would make Magician happy). But...
Daniel's board needs very strange settings because network mode
doesn't work, so I guess he can't use 2-byte DMA transfers. Is that
correct?

If we can't find a way to have all machines use the mono DMA params,
there has to be some kind of interface to set the width of the data in
the FIFO independently from the Alsa format. How should that interface
look like?

regards
Philipp

Re: snd-soc-pxa-ssp I2C/FIFO issues

[Liam Girdwood]

On Mon, 2009-03-09 at 15:47 +0100, pHilipp Zabel wrote:
> Ok,
> 
> I'm not sure how to handle this correctly.
> 
> Magician needs stereo S16_LE input data from alsa, DAIFMT_DSP_A on the
> wire with a frame size of 16 bit (one frame for each channel) and thus
> 2-byte DMA transfers to the FIFO (currently called 'mono' DMA params).
> Zylonite's current code uses 32 bit frame size and 4-byte DMA
> transfers to emulate I2S (S16_LE,stereo). Network mode seems to work,
> so it could be changed to use two 16 bit slots instead with 2-byte DMA
> transfers (which would make Magician happy). But...
> Daniel's board needs very strange settings because network mode
> doesn't work, so I guess he can't use 2-byte DMA transfers. Is that
> correct?
> 
> If we can't find a way to have all machines use the mono DMA params,
> there has to be some kind of interface to set the width of the data in
> the FIFO independently from the Alsa format. How should that interface
> look like?

Btw, what pxa processor variants are you and Daniel using ?

Iirc, PXA3x SSP is quite different in terms of "usability" compared to
to PXA2x SSP. It might be worth diverging the SSP code a little here if
your CPUs are different.

Liam

Re: snd-soc-pxa-ssp I2C/FIFO issues

[Mark Brown]

On Mon, Mar 09, 2009 at 03:03:06PM +0000, Liam Girdwood wrote:

> Iirc, PXA3x SSP is quite different in terms of "usability" compared to
> to PXA2x SSP. It might be worth diverging the SSP code a little here if
> your CPUs are different.

The PXA3xx variant is certainly rather more featureful in terms of the
clocking, though that one is already handled in the code.  For any
divergance that's needed in the rest of it I'd hope we'll be able to
keep the one driver but only replace some of the operations at runtime -
from memory hw_params() is the most likely place to need it.

Re: snd-soc-pxa-ssp I2C/FIFO issues

[Daniel Mack]

Hi Philipp,

On Mon, Mar 09, 2009 at 03:47:35PM +0100, pHilipp Zabel wrote:
> Magician needs stereo S16_LE input data from alsa, DAIFMT_DSP_A on the
> wire with a frame size of 16 bit (one frame for each channel) and thus
> 2-byte DMA transfers to the FIFO (currently called 'mono' DMA params).
> Zylonite's current code uses 32 bit frame size and 4-byte DMA
> transfers to emulate I2S (S16_LE,stereo). Network mode seems to work,
> so it could be changed to use two 16 bit slots instead with 2-byte DMA
> transfers (which would make Magician happy). But...

That's just one mode the driver has to support, which it currently does
already, right?

> Daniel's board needs very strange settings because network mode
> doesn't work, so I guess he can't use 2-byte DMA transfers. Is that
> correct?

It's not that strange actually. All it needs to have is 64 bitclk cycles
per frame with 32 bits of data per frame. See the CS4270 datasheet[1],
Figure 10 on page 19. At least when operating in master mode, the codec
needs to have bitclk = 64fs.

> If we can't find a way to have all machines use the mono DMA params,
> there has to be some kind of interface to set the width of the data in
> the FIFO independently from the Alsa format. How should that interface
> look like?

IMO, we need to pass three informations to the DAIs:

1) The hardware interface format (I2S/DSP/LEFT_J/RIGHT_J/...)
2) The pyhsical frame width (ie, how many data is actually used per
   frame, which then also tells us how to use the FIFOs)
3) The data alignment (ie, how data is sent within the frames)

As I see these informations beloning together, I would have said (3)
should be defined in a similar way than (1) and (2) and hence find a
nice place to live in the dai_fmt integer bits, as we can call the CPU's
set_dai_fmt() function from the board file.

Mark says he'd rather like me to use/abuse the set_clk_div() interface
for that but IMO that's an evil hack. The next CPU would need a similar
thing to be used with this codec, so there should be a clean way to
achive that.

So - no worries. Whatever way we choose, we'll avoid regressions for the
Magician board, but I need your help in testing this as I don't have
anything else than our own hardware :)

Daniel


[1] http://www.cirrus.com/en/pubs/proDatasheet/CS4270_PP1.pdf

Re: snd-soc-pxa-ssp I2C/FIFO issues

[pHilipp Zabel]

On Mon, Mar 9, 2009 at 4:36 PM, Daniel Mack <daniel@caiaq.de> wrote:
> Hi Philipp,
>
> On Mon, Mar 09, 2009 at 03:47:35PM +0100, pHilipp Zabel wrote:
>> Magician needs stereo S16_LE input data from alsa, DAIFMT_DSP_A on the
>> wire with a frame size of 16 bit (one frame for each channel) and thus
>> 2-byte DMA transfers to the FIFO (currently called 'mono' DMA params).
>> Zylonite's current code uses 32 bit frame size and 4-byte DMA
>> transfers to emulate I2S (S16_LE,stereo). Network mode seems to work,
>> so it could be changed to use two 16 bit slots instead with 2-byte DMA
>> transfers (which would make Magician happy). But...
>
> That's just one mode the driver has to support, which it currently does
> already, right?

The DAI mode (DSP_A, 16 bit width) is supported just fine. But the
current code always uses 4-byte DMA for stereo and 2-byte DMA for
mono. It operates under the assumption that both 16-bit stereo
channels are transmitted in one 32-bit frame.
If I send two 16-bit frames per sample, the DMA shovels 4 bytes into
the FIFO twice, and I'm losing half the data.

>> Daniel's board needs very strange settings because network mode
>> doesn't work, so I guess he can't use 2-byte DMA transfers. Is that
>> correct?
>
> It's not that strange actually. All it needs to have is 64 bitclk cycles
> per frame with 32 bits of data per frame. See the CS4270 datasheet[1],
> Figure 10 on page 19. At least when operating in master mode, the codec
> needs to have bitclk = 64fs.

IMHO, the physical DAI format isn't strange, but SSP parameters you
can use to achieve that are.

>> If we can't find a way to have all machines use the mono DMA params,
>> there has to be some kind of interface to set the width of the data in
>> the FIFO independently from the Alsa format. How should that interface
>> look like?
>
> IMO, we need to pass three informations to the DAIs:
>
> 1) The hardware interface format (I2S/DSP/LEFT_J/RIGHT_J/...)
> 2) The pyhsical frame width (ie, how many data is actually used per
>   frame, which then also tells us how to use the FIFOs)
> 3) The data alignment (ie, how data is sent within the frames)
>
> As I see these informations beloning together, I would have said (3)
> should be defined in a similar way than (1) and (2) and hence find a
> nice place to live in the dai_fmt integer bits, as we can call the CPU's
> set_dai_fmt() function from the board file.

Would work for me.

> Mark says he'd rather like me to use/abuse the set_clk_div() interface
> for that but IMO that's an evil hack. The next CPU would need a similar
> thing to be used with this codec, so there should be a clean way to
> achive that.

And it can't be used to set the DMA/FIFO width.

> So - no worries. Whatever way we choose, we'll avoid regressions for the
> Magician board, but I need your help in testing this as I don't have
> anything else than our own hardware :)

I'm not worried about regressions. Magician board support isn't even
submitted yet.
I need a clean way to do 2) first. I just kind of hoped it could make
it into 2.6.30 :)

regards
Philipp

Re: snd-soc-pxa-ssp I2C/FIFO issues

[pHilipp Zabel]

On Mon, Mar 9, 2009 at 4:03 PM, Liam Girdwood <lrg@kernel.org> wrote:
> On Mon, 2009-03-09 at 15:47 +0100, pHilipp Zabel wrote:
>> Ok,
>>
>> I'm not sure how to handle this correctly.
>>
>> Magician needs stereo S16_LE input data from alsa, DAIFMT_DSP_A on the
>> wire with a frame size of 16 bit (one frame for each channel) and thus
>> 2-byte DMA transfers to the FIFO (currently called 'mono' DMA params).
>> Zylonite's current code uses 32 bit frame size and 4-byte DMA
>> transfers to emulate I2S (S16_LE,stereo). Network mode seems to work,
>> so it could be changed to use two 16 bit slots instead with 2-byte DMA
>> transfers (which would make Magician happy). But...
>> Daniel's board needs very strange settings because network mode
>> doesn't work, so I guess he can't use 2-byte DMA transfers. Is that
>> correct?
>>
>> If we can't find a way to have all machines use the mono DMA params,
>> there has to be some kind of interface to set the width of the data in
>> the FIFO independently from the Alsa format. How should that interface
>> look like?
>
> Btw, what pxa processor variants are you and Daniel using ?
>
> Iirc, PXA3x SSP is quite different in terms of "usability" compared to
> to PXA2x SSP. It might be worth diverging the SSP code a little here if
> your CPUs are different.

PXA272 in my case, but I share Mark's hope of keeping it unified.

regards
Philipp

Re: snd-soc-pxa-ssp I2C/FIFO issues

[Daniel Mack]

On Mon, Mar 09, 2009 at 03:03:06PM +0000, Liam Girdwood wrote:
> On Mon, 2009-03-09 at 15:47 +0100, pHilipp Zabel wrote:
> > Ok,
> > 
> > I'm not sure how to handle this correctly.
> > 
> > Magician needs stereo S16_LE input data from alsa, DAIFMT_DSP_A on the
> > wire with a frame size of 16 bit (one frame for each channel) and thus
> > 2-byte DMA transfers to the FIFO (currently called 'mono' DMA params).
> > Zylonite's current code uses 32 bit frame size and 4-byte DMA
> > transfers to emulate I2S (S16_LE,stereo). Network mode seems to work,
> > so it could be changed to use two 16 bit slots instead with 2-byte DMA
> > transfers (which would make Magician happy). But...
> > Daniel's board needs very strange settings because network mode
> > doesn't work, so I guess he can't use 2-byte DMA transfers. Is that
> > correct?
> > 
> > If we can't find a way to have all machines use the mono DMA params,
> > there has to be some kind of interface to set the width of the data in
> > the FIFO independently from the Alsa format. How should that interface
> > look like?
> 
> Btw, what pxa processor variants are you and Daniel using ?

PXA300 in my case.

> Iirc, PXA3x SSP is quite different in terms of "usability" compared to
> to PXA2x SSP. It might be worth diverging the SSP code a little here if
> your CPUs are different.

The SSP registers are indeed different (or at least PXA3xx has features
PXA2xx lacks), so it could be we end up with some special cases for the
format I described in my other mail. Maybe PXA2xx's SSPs won't support
that format at all or the need some special treatmeant with other
register magic to be found out by more nasty trial-and-error.

My plan is to only conditionally support that format my board needs for
PXA3s now until someone finds some time to test that on a different
processor. Now the only remaining question is how to pass that
information to the CPU DAI ;)

Daniel

Re: snd-soc-pxa-ssp I2C/FIFO issues

[Mark Brown]

On Mon, Mar 09, 2009 at 04:36:29PM +0100, Daniel Mack wrote:

> Mark says he'd rather like me to use/abuse the set_clk_div() interface
> for that but IMO that's an evil hack. The next CPU would need a similar
> thing to be used with this codec, so there should be a clean way to
> achive that.

The point here is that this is already fairly widely supported with some
combination of that approach and network mode and adding a third method
only makes things more complex, especially with more flexibile devices
which are capable of supporting combinations of these options - what
happens if the DAC and ADC clocks are separate and the user wants to
use that, for example?  How exactly does it play with TDM mode?

Worst case codec devices for this sort of stuff have clocking trees of
equivalent complexity to a SoC (eg, multiple roots, ability to select
between then at various points for various functions), multiple DAIs and
support a wide range of configurations on each DAI.

In your system a big part of the problem appears to be that you've got
two devices that have fairly exacting requirements with regard to clock
configuration (the CS4270 can only support one configuration involving
unused clock cycles while the PXA needs to know *exactly* what is being
input to it due to not directly implementing I2S).

I'm also slightly concerned with the use of a bitmask here since it
limits the set of clock counts that can be supported to a predefined set
of values but that's fairly minor.

It's probably worth pointing out that originally ASoC did have much more
complete clock handling in the core but this was removed due to the
difficulty in automatically working out exactly how to set things up in
complex configurations - it proved much more practical to have machine
drivers manually set everything up rather than try to tell the core
about all the clock trees.

Re: snd-soc-pxa-ssp I2C/FIFO issues

[Daniel Mack]

On Mon, Mar 09, 2009 at 05:44:46PM +0000, Mark Brown wrote:
> On Mon, Mar 09, 2009 at 04:36:29PM +0100, Daniel Mack wrote:
> 
> > Mark says he'd rather like me to use/abuse the set_clk_div() interface
> > for that but IMO that's an evil hack. The next CPU would need a similar
> > thing to be used with this codec, so there should be a clean way to
> > achive that.
> 
> The point here is that this is already fairly widely supported with some
> combination of that approach and network mode and adding a third method
> only makes things more complex, especially with more flexibile devices
> which are capable of supporting combinations of these options

Ok, there might be an even more straight-forward solution for this
problem: As we know already from the DIV_SCR divider what our
BCLK/LRCLK ratio is, we can add a special case for the mode I need, as
implemented in the patch below.

Philipp, could you test that again on your board, please?
Applies on top of the other one ("pxa-ssp: don't touch ssp registers
...") I just posted.

Thanks,
Daniel


>From cbd130dfeca4d65acd34cd6a3ca6d1a45885985f Mon Sep 17 00:00:00 2001
From: Daniel Mack <daniel@caiaq.de>
Date: Tue, 10 Mar 2009 16:33:35 +0100
Subject: [PATCH 1/2] pxa-ssp: switch from network mode to PSP

This switches the pxa ssp port usage from network mode to PSP mode.
Removed some comments and checks for configured TDM channels.
A special case is added to support configuration where BCLK = 64fs. We
need to do some black magic in this case which doesn't look nice but
there is unfortunately no other option than that.

Signed-off-by: Daniel Mack <daniel@caiaq.de>
---
 sound/soc/pxa/pxa-ssp.c |   56 ++++++++++++++++++++++++++++++----------------
 1 files changed, 36 insertions(+), 20 deletions(-)

diff --git a/sound/soc/pxa/pxa-ssp.c b/sound/soc/pxa/pxa-ssp.c
index 7fc13f0..1b3a81c 100644
--- a/sound/soc/pxa/pxa-ssp.c
+++ b/sound/soc/pxa/pxa-ssp.c
@@ -45,6 +45,7 @@
 struct ssp_priv {
 	struct ssp_dev dev;
 	unsigned int sysclk;
+	unsigned int scr_div;
 	int dai_fmt;
 #ifdef CONFIG_PM
 	struct ssp_state state;
@@ -281,11 +282,13 @@ static int pxa_ssp_resume(struct snd_soc_dai *cpu_dai)
  * ssp_set_clkdiv - set SSP clock divider
  * @div: serial clock rate divider
  */
-static void ssp_set_scr(struct ssp_dev *dev, u32 div)
+static void ssp_set_scr(struct ssp_priv *priv, u32 div)
 {
+	struct ssp_dev *dev = &priv->dev;
 	struct ssp_device *ssp = dev->ssp;
 	u32 sscr0 = ssp_read_reg(dev->ssp, SSCR0) & ~SSCR0_SCR;
 
+	priv->scr_div = div;
 	ssp_write_reg(ssp, SSCR0, (sscr0 | SSCR0_SerClkDiv(div)));
 }
 
@@ -327,7 +330,7 @@ static int pxa_ssp_set_dai_sysclk(struct snd_soc_dai *cpu_dai,
 		break;
 	case PXA_SSP_CLK_AUDIO:
 		priv->sysclk = 0;
-		ssp_set_scr(&priv->dev, 1);
+		ssp_set_scr(priv, 1);
 		sscr0 |= SSCR0_ACS;
 		break;
 	default:
@@ -388,7 +391,7 @@ static int pxa_ssp_set_dai_clkdiv(struct snd_soc_dai *cpu_dai,
 		ssp_write_reg(ssp, SSACD, val);
 		break;
 	case PXA_SSP_DIV_SCR:
-		ssp_set_scr(&priv->dev, div);
+		ssp_set_scr(priv, div);
 		break;
 	default:
 		return -ENODEV;
@@ -547,18 +550,17 @@ 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;
+			sspsp |= SSPSP_SFRMP;
 			break;
 		case SND_SOC_DAIFMT_IB_IF:
-			sspsp |= SSPSP_SFRMP;
+			sspsp |= SSPSP_SFRMP | SSPSP_SCMODE(3);
 			break;
 		default:
 			return -EINVAL;
@@ -644,37 +646,51 @@ static int pxa_ssp_hw_params(struct snd_pcm_substream *substream,
 			sscr0 |= SSCR0_FPCKE;
 #endif
 		sscr0 |= SSCR0_DataSize(16);
-		/* use network mode (2 slots) for 16 bit stereo */
 		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);
+
+		if ((priv->scr_div == 4) && (width == 16)) {
+			/* This is a special case where the bitclk is 64fs
+			 * and we're not dealing with 2*32 bits of audio
+			 * samples.
+			 *
+			 * The SSP values used for that are all found out by
+			 * trying and failing a lot; some of the registers
+			 * needed for that mode are only available on PXA3xx.
+			 */
+
+#ifdef CONFIG_PXA3xx
+			if (!cpu_is_pxa3xx())
+				return -EINVAL;
+
+			sspsp |= SSPSP_SFRMWDTH(width * 2);
+			sspsp |= SSPSP_SFRMDLY(width * 4);
+			sspsp |= SSPSP_EDMYSTOP(3);
+			sspsp |= SSPSP_DMYSTOP(3);
+			sspsp |= SSPSP_DMYSTRT(1);
+#else
+			return -EINVAL;
+#endif
+		} else
+			sspsp |= SSPSP_SFRMWDTH(width);
+
 		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.2

Re: snd-soc-pxa-ssp I2C/FIFO issues

[pHilipp Zabel]

Hi,

On Tue, Mar 10, 2009 at 4:47 PM, Daniel Mack <daniel@caiaq.de> wrote:
> On Mon, Mar 09, 2009 at 05:44:46PM +0000, Mark Brown wrote:
>> On Mon, Mar 09, 2009 at 04:36:29PM +0100, Daniel Mack wrote:
>>
>> > Mark says he'd rather like me to use/abuse the set_clk_div() interface
>> > for that but IMO that's an evil hack. The next CPU would need a similar
>> > thing to be used with this codec, so there should be a clean way to
>> > achive that.
>>
>> The point here is that this is already fairly widely supported with some
>> combination of that approach and network mode and adding a third method
>> only makes things more complex, especially with more flexibile devices
>> which are capable of supporting combinations of these options
>
> Ok, there might be an even more straight-forward solution for this
> problem: As we know already from the DIV_SCR divider what our
> BCLK/LRCLK ratio is, we can add a special case for the mode I need, as
> implemented in the patch below.
>
> Philipp, could you test that again on your board, please?
> Applies on top of the other one ("pxa-ssp: don't touch ssp registers
> ...") I just posted.

Tested-by me. Of course this still doesn't help me to enable 16-bit
DMA transfers for stereo, but it doesn't hurt either.

> Thanks,
> Daniel
>
>
> From cbd130dfeca4d65acd34cd6a3ca6d1a45885985f Mon Sep 17 00:00:00 2001
> From: Daniel Mack <daniel@caiaq.de>
> Date: Tue, 10 Mar 2009 16:33:35 +0100
> Subject: [PATCH 1/2] pxa-ssp: switch from network mode to PSP
>
> This switches the pxa ssp port usage from network mode to PSP mode.
> Removed some comments and checks for configured TDM channels.
> A special case is added to support configuration where BCLK = 64fs. We
> need to do some black magic in this case which doesn't look nice but
> there is unfortunately no other option than that.
>
> Signed-off-by: Daniel Mack <daniel@caiaq.de>
> ---
>  sound/soc/pxa/pxa-ssp.c |   56 ++++++++++++++++++++++++++++++----------------
>  1 files changed, 36 insertions(+), 20 deletions(-)
>
> diff --git a/sound/soc/pxa/pxa-ssp.c b/sound/soc/pxa/pxa-ssp.c
> index 7fc13f0..1b3a81c 100644
> --- a/sound/soc/pxa/pxa-ssp.c
> +++ b/sound/soc/pxa/pxa-ssp.c
> @@ -45,6 +45,7 @@
>  struct ssp_priv {
>        struct ssp_dev dev;
>        unsigned int sysclk;
> +       unsigned int scr_div;

Really needed? Or could we just check SSCR0 below?

>        int dai_fmt;
>  #ifdef CONFIG_PM
>        struct ssp_state state;
> @@ -281,11 +282,13 @@ static int pxa_ssp_resume(struct snd_soc_dai *cpu_dai)
>  * ssp_set_clkdiv - set SSP clock divider
>  * @div: serial clock rate divider
>  */
> -static void ssp_set_scr(struct ssp_dev *dev, u32 div)
> +static void ssp_set_scr(struct ssp_priv *priv, u32 div)
>  {
> +       struct ssp_dev *dev = &priv->dev;
>        struct ssp_device *ssp = dev->ssp;
>        u32 sscr0 = ssp_read_reg(dev->ssp, SSCR0) & ~SSCR0_SCR;
>
> +       priv->scr_div = div;
>        ssp_write_reg(ssp, SSCR0, (sscr0 | SSCR0_SerClkDiv(div)));
>  }
>
> @@ -327,7 +330,7 @@ static int pxa_ssp_set_dai_sysclk(struct snd_soc_dai *cpu_dai,
>                break;
>        case PXA_SSP_CLK_AUDIO:
>                priv->sysclk = 0;
> -               ssp_set_scr(&priv->dev, 1);
> +               ssp_set_scr(priv, 1);
>                sscr0 |= SSCR0_ACS;
>                break;
>        default:
> @@ -388,7 +391,7 @@ static int pxa_ssp_set_dai_clkdiv(struct snd_soc_dai *cpu_dai,
>                ssp_write_reg(ssp, SSACD, val);
>                break;
>        case PXA_SSP_DIV_SCR:
> -               ssp_set_scr(&priv->dev, div);
> +               ssp_set_scr(priv, div);
>                break;
>        default:
>                return -ENODEV;
> @@ -547,18 +550,17 @@ 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;
> +                       sspsp |= SSPSP_SFRMP;
>                        break;
>                case SND_SOC_DAIFMT_IB_IF:
> -                       sspsp |= SSPSP_SFRMP;
> +                       sspsp |= SSPSP_SFRMP | SSPSP_SCMODE(3);
>                        break;
>                default:
>                        return -EINVAL;
> @@ -644,37 +646,51 @@ static int pxa_ssp_hw_params(struct snd_pcm_substream *substream,
>                        sscr0 |= SSCR0_FPCKE;
>  #endif
>                sscr0 |= SSCR0_DataSize(16);
> -               /* use network mode (2 slots) for 16 bit stereo */
>                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);
> +
> +               if ((priv->scr_div == 4) && (width == 16)) {

sscr0 & SSCR0_SCR == SSCR0_SerClkDiv(4) instead?

Probably not, it just replaces data bloat by code bloat.

> +                       /* This is a special case where the bitclk is 64fs
> +                        * and we're not dealing with 2*32 bits of audio
> +                        * samples.
> +                        *
> +                        * The SSP values used for that are all found out by
> +                        * trying and failing a lot; some of the registers
> +                        * needed for that mode are only available on PXA3xx.
> +                        */
> +
> +#ifdef CONFIG_PXA3xx
> +                       if (!cpu_is_pxa3xx())
> +                               return -EINVAL;
> +
> +                       sspsp |= SSPSP_SFRMWDTH(width * 2);
> +                       sspsp |= SSPSP_SFRMDLY(width * 4);
> +                       sspsp |= SSPSP_EDMYSTOP(3);
> +                       sspsp |= SSPSP_DMYSTOP(3);
> +                       sspsp |= SSPSP_DMYSTRT(1);
> +#else
> +                       return -EINVAL;
> +#endif
> +               } else
> +                       sspsp |= SSPSP_SFRMWDTH(width);
> +
>                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;
> -       }
> -

I wonder if this check was useful in some case? If so, we could
replace it with something like this:

@@ -667,10 +667,11 @@ static int pxa_ssp_hw_params(struct
snd_pcm_substream *substream,
 		break;
 	}

-	/* We always use a network mode so we always require TDM slots
+	/* If we are using a network mode, require TDM slots
 	 * - complain loudly and fail if they've not been set up yet.
 	 */
-	if (!(ssp_read_reg(ssp, SSTSA) & 0xf)) {
+	if ((ssp_read_reg(ssp, SSCR0) & SSCR0_MOD) &&
+	    !(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.2
>
>

regards
Philipp

Re: snd-soc-pxa-ssp I2C/FIFO issues

[Daniel Mack]

On Wed, Mar 11, 2009 at 07:10:28PM +0100, pHilipp Zabel wrote:
> > Philipp, could you test that again on your board, please?
> > Applies on top of the other one ("pxa-ssp: don't touch ssp registers
> > ...") I just posted.
> 
> Tested-by me. Of course this still doesn't help me to enable 16-bit
> DMA transfers for stereo, but it doesn't hurt either.

Ok, but good to know this does not break your config.

> >  struct ssp_priv {
> >        struct ssp_dev dev;
> >        unsigned int sysclk;
> > +       unsigned int scr_div;
> 
> Really needed? Or could we just check SSCR0 below?

Well, SSCR0_SerClkDiv() is defined as follows:

#define SSCR0_SCR       (0x000fff00)    /* Serial Clock Rate (mask) */
#define SSCR0_SerClkDiv(x) (((x) - 1) << 8) /* Divisor [1..4096] */

I thought about adding a reverse macro just for that case, but that
seemed a lot more intrusive.

> >        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);
> > +
> > +               if ((priv->scr_div == 4) && (width == 16)) {
> 
> sscr0 & SSCR0_SCR == SSCR0_SerClkDiv(4) instead?

Yes, could do that, but I doubt it is more readable? But we would save
one entry in the priv struct, so I'll do it.

> > -       /* 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;
> > -       }
> > -
> 
> I wonder if this check was useful in some case? If so, we could
> replace it with something like this:

Yep, we didn't kill the network mode entirely, you're right. Will fix
this in a follow-up.

Thanks,
Daniel

Re: snd-soc-pxa-ssp I2C/FIFO issues

[Daniel Mack]

On Wed, Mar 11, 2009 at 07:10:28PM +0100, pHilipp Zabel wrote:
> Tested-by me. Of course this still doesn't help me to enable 16-bit
> DMA transfers for stereo, but it doesn't hurt either.

Ok, new version below.

Daniel


>From ad8734e93eed130a55482b0e937729578e6d93c8 Mon Sep 17 00:00:00 2001
From: Daniel Mack <daniel@caiaq.de>
Date: Wed, 11 Mar 2009 19:38:15 +0100
Subject: [PATCH] pxa-ssp: switch from network mode to PSP

This switches the pxa ssp port usage from network mode to PSP mode.
Removed some comments and checks for configured TDM channels.
A special case is added to support configuration where BCLK = 64fs. We
need to do some black magic in this case which doesn't look nice but
there is unfortunately no other option than that.

Signed-off-by: Daniel Mack <daniel@caiaq.de>
---
 sound/soc/pxa/pxa-ssp.c |   44 +++++++++++++++++++++++++++++++++-----------
 1 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/sound/soc/pxa/pxa-ssp.c b/sound/soc/pxa/pxa-ssp.c
index 52d97c4..3cde686 100644
--- a/sound/soc/pxa/pxa-ssp.c
+++ b/sound/soc/pxa/pxa-ssp.c
@@ -558,18 +558,17 @@ 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;
+			sspsp |= SSPSP_SFRMP;
 			break;
 		case SND_SOC_DAIFMT_IB_IF:
-			sspsp |= SSPSP_SFRMP;
+			sspsp |= SSPSP_SFRMP | SSPSP_SCMODE(3);
 			break;
 		default:
 			return -EINVAL;
@@ -655,33 +654,56 @@ static int pxa_ssp_hw_params(struct snd_pcm_substream *substream,
 			sscr0 |= SSCR0_FPCKE;
 #endif
 		sscr0 |= SSCR0_DataSize(16);
-		/* use network mode (2 slots) for 16 bit stereo */
 		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);
+
+		if (((sscr0 & SSCR0_SCR) == SSCR0_SerClkDiv(4)) &&
+		     (width == 16)) {
+			/* This is a special case where the bitclk is 64fs
+			* and we're not dealing with 2*32 bits of audio
+			* samples.
+			*
+			* The SSP values used for that are all found out by
+			* trying and failing a lot; some of the registers
+			* needed for that mode are only available on PXA3xx.
+			*/
+
+#ifdef CONFIG_PXA3xx
+			if (!cpu_is_pxa3xx())
+				return -EINVAL;
+
+			sspsp |= SSPSP_SFRMWDTH(width * 2);
+			sspsp |= SSPSP_SFRMDLY(width * 4);
+			sspsp |= SSPSP_EDMYSTOP(3);
+			sspsp |= SSPSP_DMYSTOP(3);
+			sspsp |= SSPSP_DMYSTRT(1);
+#else
+			return -EINVAL;
+#endif
+		} else
+			sspsp |= SSPSP_SFRMWDTH(width);
+
 		ssp_write_reg(ssp, SSPSP, sspsp);
 		break;
 	default:
 		break;
 	}
 
-	/* We always use a network mode so we always require TDM slots
+	/* When we use a network mode, we always require TDM slots
 	 * - complain loudly and fail if they've not been set up yet.
 	 */
-	if (!(ssp_read_reg(ssp, SSTSA) & 0xf)) {
+	if ((sscr0 & SSCR0_MOD) && !(ssp_read_reg(ssp, SSTSA) & 0xf)) {
 		dev_err(&ssp->pdev->dev, "No TDM timeslot configured\n");
 		return -EINVAL;
 	}
-- 
1.6.2