[alsa-devel] snd-soc-pxa-ssp I2C/FIFO issues
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
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
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.
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
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
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
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:
- The hardware interface format (I2S/DSP/LEFT_J/RIGHT_J/...)
- 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
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.
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
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
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
participants (4)
-
Daniel Mack -
Liam Girdwood -
Mark Brown -
pHilipp Zabel