[alsa-devel] [PATCH 1/4] ASoC: Use network mode with 2 slots for 16-bit stereo in pxa-ssp/Zylonite
For consistency with 24-bit and 32-bit modes, don't send 16-bit stereo in one 32-bit transfer. Use 2 slots instead on Zylonite. It should result in exactly the same behaviour. Now it is possible to use 16-bit single slot transfers in pxa-ssp, which are needed for Magician to get two frame clock pulses per sample (one for each channel).
Signed-off-by: Philipp Zabel philipp.zabel@gmail.com --- sound/soc/pxa/pxa-ssp.c | 3 +-- sound/soc/pxa/zylonite.c | 7 +++++-- 2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/sound/soc/pxa/pxa-ssp.c b/sound/soc/pxa/pxa-ssp.c index 4a973ab..c49bb12 100644 --- a/sound/soc/pxa/pxa-ssp.c +++ b/sound/soc/pxa/pxa-ssp.c @@ -644,8 +644,7 @@ static int pxa_ssp_hw_params(struct snd_pcm_substream *substream, sscr0 |= SSCR0_FPCKE; #endif sscr0 |= SSCR0_DataSize(16); - if (params_channels(params) > 1) - sscr0 |= SSCR0_EDSS; + /* use network mode (2 slots) for 16 bit stereo */ break; case SNDRV_PCM_FORMAT_S24_LE: sscr0 |= (SSCR0_EDSS | SSCR0_DataSize(8)); diff --git a/sound/soc/pxa/zylonite.c b/sound/soc/pxa/zylonite.c index 0140a25..9f6116e 100644 --- a/sound/soc/pxa/zylonite.c +++ b/sound/soc/pxa/zylonite.c @@ -127,8 +127,11 @@ static int zylonite_voice_hw_params(struct snd_pcm_substream *substream, if (ret < 0) return ret;
- /* We're not really in network mode but the emulation wants this. */ - ret = snd_soc_dai_set_tdm_slot(cpu_dai, 1, 1); + /* Use network mode for stereo, one slot per channel. */ + if (params_channels(params) > 1) + ret = snd_soc_dai_set_tdm_slot(cpu_dai, 0x3, 2); + else + ret = snd_soc_dai_set_tdm_slot(cpu_dai, 1, 1); if (ret < 0) return ret;
Only allow SND_SOC_DAIFMT_CBS_CFS for the playback DAI.
Signed-off-by: Philipp Zabel philipp.zabel@gmail.com --- sound/soc/codecs/uda1380.c | 9 +++++++-- 1 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/uda1380.c b/sound/soc/codecs/uda1380.c index 5242b81..1e3769d 100644 --- a/sound/soc/codecs/uda1380.c +++ b/sound/soc/codecs/uda1380.c @@ -377,8 +377,9 @@ static int uda1380_set_dai_fmt_both(struct snd_soc_dai *codec_dai, iface |= R01_SFORI_MSB | R01_SFORO_MSB; }
- if ((fmt & SND_SOC_DAIFMT_MASTER_MASK) == SND_SOC_DAIFMT_CBM_CFM) - iface |= R01_SIM; + /* DATAI is slave only, so in single-link mode, this has to be slave */ + if ((fmt & SND_SOC_DAIFMT_MASTER_MASK) != SND_SOC_DAIFMT_CBS_CFS) + return -EINVAL;
uda1380_write(codec, UDA1380_IFACE, iface);
@@ -406,6 +407,10 @@ static int uda1380_set_dai_fmt_playback(struct snd_soc_dai *codec_dai, iface |= R01_SFORI_MSB; }
+ /* DATAI is slave only, so this has to be slave */ + if ((fmt & SND_SOC_DAIFMT_MASTER_MASK) != SND_SOC_DAIFMT_CBS_CFS) + return -EINVAL; + uda1380_write(codec, UDA1380_IFACE, iface);
return 0;
On Tue, Mar 03, 2009 at 04:10:52PM +0100, Philipp Zabel wrote:
- if ((fmt & SND_SOC_DAIFMT_MASTER_MASK) == SND_SOC_DAIFMT_CBM_CFM)
iface |= R01_SIM;
- /* DATAI is slave only, so in single-link mode, this has to be slave */
- if ((fmt & SND_SOC_DAIFMT_MASTER_MASK) != SND_SOC_DAIFMT_CBS_CFS)
return -EINVAL;
Hrm, is R01_SIM not required to be set? I've no idea what it does so quite possibly.
On Tue, Mar 3, 2009 at 4:50 PM, Mark Brown broonie@opensource.wolfsonmicro.com wrote:
On Tue, Mar 03, 2009 at 04:10:52PM +0100, Philipp Zabel wrote:
- if ((fmt & SND_SOC_DAIFMT_MASTER_MASK) == SND_SOC_DAIFMT_CBM_CFM)
- iface |= R01_SIM;
- /* DATAI is slave only, so in single-link mode, this has to be slave */
- if ((fmt & SND_SOC_DAIFMT_MASTER_MASK) != SND_SOC_DAIFMT_CBS_CFS)
- return -EINVAL;
Hrm, is R01_SIM not required to be set? I've no idea what it does so quite possibly.
R01_SIM sets the UDA1380's digital output interface to master. This is not allowed if both input and output use a single DAI (uda1380_set_dai_fmt_both), because the input interface is slave only.
Only uda1380_set_dai_fmt_capture should accept the CBM_CFM setting.
regards Philipp
On Tue, Mar 03, 2009 at 04:56:58PM +0100, pHilipp Zabel wrote:
R01_SIM sets the UDA1380's digital output interface to master. This is not allowed if both input and output use a single DAI (uda1380_set_dai_fmt_both), because the input interface is slave only.
Only uda1380_set_dai_fmt_capture should accept the CBM_CFM setting.
OK, applied this patch too - thanks!
Signed-off-by: Philipp Zabel philipp.zabel@gmail.com --- sound/soc/codecs/uda1380.c | 4 ---- 1 files changed, 0 insertions(+), 4 deletions(-)
diff --git a/sound/soc/codecs/uda1380.c b/sound/soc/codecs/uda1380.c index 1e3769d..a6987d7 100644 --- a/sound/soc/codecs/uda1380.c +++ b/sound/soc/codecs/uda1380.c @@ -35,8 +35,6 @@
#include "uda1380.h"
-#define UDA1380_VERSION "0.6" - /* * uda1380 register cache */ @@ -831,8 +829,6 @@ static int uda1380_probe(struct platform_device *pdev) struct snd_soc_codec *codec; int ret;
- pr_info("UDA1380 Audio Codec %s", UDA1380_VERSION); - setup = socdev->codec_data; codec = kzalloc(sizeof(struct snd_soc_codec), GFP_KERNEL); if (codec == NULL)
On Tue, Mar 03, 2009 at 04:10:53PM +0100, Philipp Zabel wrote:
Signed-off-by: Philipp Zabel philipp.zabel@gmail.com
Applied this and your final patch, thanks.
If the UDA1380's interpolator or decimator are set to be clocked from the WSPLL (which syncs to the WSI signal), the DAI link must be running to change the interpolator/decimator registers (which include volume controls and digital mute setting).
* Queue work in the alsa PCM_START .trigger to flush registers as soon as the link is running. This replaces the .prepare and .digital_mute callbacks. * Use the SILENCE override instead of MTM for muting and remove its alsa control to avoid confusion.
Signed-off-by: Philipp Zabel philipp.zabel@gmail.com --- sound/soc/codecs/uda1380.c | 102 +++++++++++++++++++++----------------------- 1 files changed, 48 insertions(+), 54 deletions(-)
diff --git a/sound/soc/codecs/uda1380.c b/sound/soc/codecs/uda1380.c index a6987d7..1b10f48 100644 --- a/sound/soc/codecs/uda1380.c +++ b/sound/soc/codecs/uda1380.c @@ -25,6 +25,7 @@ #include <linux/ioctl.h> #include <linux/delay.h> #include <linux/i2c.h> +#include <linux/workqueue.h> #include <sound/core.h> #include <sound/control.h> #include <sound/initval.h> @@ -35,6 +36,9 @@
#include "uda1380.h"
+static struct work_struct uda1380_work; +static struct snd_soc_codec *uda1380_codec; + /* * uda1380 register cache */ @@ -50,6 +54,8 @@ static const u16 uda1380_reg[UDA1380_CACHEREGNUM] = { 0x0000, 0x8000, 0x0002, 0x0000, };
+static unsigned long uda1380_cache_dirty; + /* * read uda1380 register cache */ @@ -71,8 +77,11 @@ static inline void uda1380_write_reg_cache(struct snd_soc_codec *codec, u16 reg, unsigned int value) { u16 *cache = codec->reg_cache; + if (reg >= UDA1380_CACHEREGNUM) return; + if ((reg >= 0x10) && (cache[reg] != value)) + set_bit(reg - 0x10, &uda1380_cache_dirty); cache[reg] = value; }
@@ -111,6 +120,8 @@ static int uda1380_write(struct snd_soc_codec *codec, unsigned int reg, (data[0]<<8) | data[1]); return -EIO; } + if (reg >= 0x10) + clear_bit(reg - 0x10, &uda1380_cache_dirty); return 0; } else return -EIO; @@ -118,6 +129,20 @@ static int uda1380_write(struct snd_soc_codec *codec, unsigned int reg,
#define uda1380_reset(c) uda1380_write(c, UDA1380_RESET, 0)
+static void uda1380_flush_work(struct work_struct *work) +{ + int bit, reg; + + for_each_bit(bit, &uda1380_cache_dirty, UDA1380_CACHEREGNUM - 0x10) { + reg = 0x10 + bit; + pr_debug("uda1380: flush reg %x val %x:\n", reg, + uda1380_read_reg_cache(uda1380_codec, reg)); + uda1380_write(uda1380_codec, reg, + uda1380_read_reg_cache(uda1380_codec, reg)); + clear_bit(bit, &uda1380_cache_dirty); + } +} + /* declarations of ALSA reg_elem_REAL controls */ static const char *uda1380_deemp[] = { "None", @@ -252,7 +277,6 @@ static const struct snd_kcontrol_new uda1380_snd_controls[] = { SOC_SINGLE("DAC Polarity inverting Switch", UDA1380_MIXER, 15, 1, 0), /* DA_POL_INV */ SOC_ENUM("Noise Shaper", uda1380_sel_ns_enum), /* SEL_NS */ SOC_ENUM("Digital Mixer Signal Control", uda1380_mix_enum), /* MIX_POS, MIX */ - SOC_SINGLE("Silence Switch", UDA1380_MIXER, 7, 1, 0), /* SILENCE, force DAC output to silence */ SOC_SINGLE("Silence Detector Switch", UDA1380_MIXER, 6, 1, 0), /* SDET_ON */ SOC_ENUM("Silence Detector Setting", uda1380_sdet_enum), /* SD_VALUE */ SOC_ENUM("Oversampling Input", uda1380_os_enum), /* OS */ @@ -443,41 +467,28 @@ static int uda1380_set_dai_fmt_capture(struct snd_soc_dai *codec_dai, return 0; }
-/* - * Flush reg cache - * We can only write the interpolator and decimator registers - * when the DAI is being clocked by the CPU DAI. It's up to the - * machine and cpu DAI driver to do this before we are called. - */ -static int uda1380_pcm_prepare(struct snd_pcm_substream *substream, - struct snd_soc_dai *dai) +static int uda1380_trigger(struct snd_pcm_substream *substream, int cmd, + struct snd_soc_dai *dai) { struct snd_soc_pcm_runtime *rtd = substream->private_data; struct snd_soc_device *socdev = rtd->socdev; struct snd_soc_codec *codec = socdev->card->codec; - int reg, reg_start, reg_end, clk; - - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { - reg_start = UDA1380_MVOL; - reg_end = UDA1380_MIXER; - } else { - reg_start = UDA1380_DEC; - reg_end = UDA1380_AGC; - } - - /* FIXME disable DAC_CLK */ - clk = uda1380_read_reg_cache(codec, UDA1380_CLK); - uda1380_write(codec, UDA1380_CLK, clk & ~R00_DAC_CLK); - - for (reg = reg_start; reg <= reg_end; reg++) { - pr_debug("uda1380: flush reg %x val %x:", reg, - uda1380_read_reg_cache(codec, reg)); - uda1380_write(codec, reg, uda1380_read_reg_cache(codec, reg)); + int mixer = uda1380_read_reg_cache(codec, UDA1380_MIXER); + + switch (cmd) { + case SNDRV_PCM_TRIGGER_START: + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: + uda1380_write_reg_cache(codec, UDA1380_MIXER, + mixer & ~R14_SILENCE); + schedule_work(&uda1380_work); + break; + case SNDRV_PCM_TRIGGER_STOP: + case SNDRV_PCM_TRIGGER_PAUSE_PUSH: + uda1380_write_reg_cache(codec, UDA1380_MIXER, + mixer | R14_SILENCE); + schedule_work(&uda1380_work); + break; } - - /* FIXME restore DAC_CLK */ - uda1380_write(codec, UDA1380_CLK, clk); - return 0; }
@@ -543,24 +554,6 @@ static void uda1380_pcm_shutdown(struct snd_pcm_substream *substream, uda1380_write(codec, UDA1380_CLK, clk); }
-static int uda1380_mute(struct snd_soc_dai *codec_dai, int mute) -{ - struct snd_soc_codec *codec = codec_dai->codec; - u16 mute_reg = uda1380_read_reg_cache(codec, UDA1380_DEEMP) & ~R13_MTM; - - /* FIXME: mute(codec,0) is called when the magician clock is already - * set to WSPLL, but for some unknown reason writing to interpolator - * registers works only when clocked by SYSCLK */ - u16 clk = uda1380_read_reg_cache(codec, UDA1380_CLK); - uda1380_write(codec, UDA1380_CLK, ~R00_DAC_CLK & clk); - if (mute) - uda1380_write(codec, UDA1380_DEEMP, mute_reg | R13_MTM); - else - uda1380_write(codec, UDA1380_DEEMP, mute_reg); - uda1380_write(codec, UDA1380_CLK, clk); - return 0; -} - static int uda1380_set_bias_level(struct snd_soc_codec *codec, enum snd_soc_bias_level level) { @@ -602,10 +595,9 @@ struct snd_soc_dai uda1380_dai[] = { .rates = UDA1380_RATES, .formats = SNDRV_PCM_FMTBIT_S16_LE,}, .ops = { + .trigger = uda1380_trigger, .hw_params = uda1380_pcm_hw_params, .shutdown = uda1380_pcm_shutdown, - .prepare = uda1380_pcm_prepare, - .digital_mute = uda1380_mute, .set_fmt = uda1380_set_dai_fmt_both, }, }, @@ -619,10 +611,9 @@ struct snd_soc_dai uda1380_dai[] = { .formats = SNDRV_PCM_FMTBIT_S16_LE, }, .ops = { + .trigger = uda1380_trigger, .hw_params = uda1380_pcm_hw_params, .shutdown = uda1380_pcm_shutdown, - .prepare = uda1380_pcm_prepare, - .digital_mute = uda1380_mute, .set_fmt = uda1380_set_dai_fmt_playback, }, }, @@ -636,9 +627,9 @@ struct snd_soc_dai uda1380_dai[] = { .formats = SNDRV_PCM_FMTBIT_S16_LE, }, .ops = { + .trigger = uda1380_trigger, .hw_params = uda1380_pcm_hw_params, .shutdown = uda1380_pcm_shutdown, - .prepare = uda1380_pcm_prepare, .set_fmt = uda1380_set_dai_fmt_capture, }, }, @@ -697,6 +688,9 @@ static int uda1380_init(struct snd_soc_device *socdev, int dac_clk) codec->reg_cache_step = 1; uda1380_reset(codec);
+ uda1380_codec = codec; + INIT_WORK(&uda1380_work, uda1380_flush_work); + /* register pcms */ ret = snd_soc_new_pcms(socdev, SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1); if (ret < 0) {
On Tue, Mar 03, 2009 at 04:10:51PM +0100, Philipp Zabel wrote:
For consistency with 24-bit and 32-bit modes, don't send 16-bit stereo in one 32-bit transfer. Use 2 slots instead on Zylonite. It should result in exactly the same behaviour.
I've tested, it does indeed.
Applied, thanks.
Hi Phillip,
glad to see someone is wasting time with that issue, too :)
On Tue, Mar 03, 2009 at 04:10:51PM +0100, Philipp Zabel wrote:
For consistency with 24-bit and 32-bit modes, don't send 16-bit stereo in one 32-bit transfer. Use 2 slots instead on Zylonite. It should result in exactly the same behaviour. Now it is possible to use 16-bit single slot transfers in pxa-ssp, which are needed for Magician to get two frame clock pulses per sample (one for each channel).
We've been fiddling around with these modes and registers for another two days in a row now and figured out that literally all the documentation about these registers is entirly bogus. In particular, the network mode with the associated timeslots simply does not work at all, according to our measurements.
Hence the question: how does your I2S signal look like at the moment? How many clocks do you measure in one frame cylce, how many of them are actually filled with data? And which format does the userspace use to send samples in?
We finally got a mode now that outputs 2x16 bit on a 2x32 bits I2S frame, padded with zeros. But that is in non-network modes, more about that tomorrow ...
Thanks, Daniel
On Wed, Mar 04, 2009 at 02:34:24AM +0100, Daniel Mack wrote:
Hence the question: how does your I2S signal look like at the moment? How many clocks do you measure in one frame cylce, how many of them are actually filled with data? And which format does the userspace use to send samples in?
We finally got a mode now that outputs 2x16 bit on a 2x32 bits I2S frame, padded with zeros. But that is in non-network modes, more about that tomorrow ...
Zylonite generates I2S signals which look OK but which don't have any padding.
On Wed, Mar 4, 2009 at 2:34 AM, Daniel Mack daniel@caiaq.de wrote:
Hi Phillip,
glad to see someone is wasting time with that issue, too :)
On Tue, Mar 03, 2009 at 04:10:51PM +0100, Philipp Zabel wrote:
For consistency with 24-bit and 32-bit modes, don't send 16-bit stereo in one 32-bit transfer. Use 2 slots instead on Zylonite. It should result in exactly the same behaviour. Now it is possible to use 16-bit single slot transfers in pxa-ssp, which are needed for Magician to get two frame clock pulses per sample (one for each channel).
We've been fiddling around with these modes and registers for another two days in a row now and figured out that literally all the documentation about these registers is entirly bogus. In particular, the network mode with the associated timeslots simply does not work at all, according to our measurements.
Hence the question: how does your I2S signal look like at the moment? How many clocks do you measure in one frame cylce, how many of them are actually filled with data? And which format does the userspace use to send samples in?
Unfortunately, I can't measure anything. I don't own an oscilloscope and I don't even know if there are useful test points on the phone's mainboard. Also, I've come to the conclusion that on Magician there has to be some kind of flip-flop between the PXA272's SSPFRMCLK and the codec's LRCLK input. I'm not using I2S mode at all, but TISSP (or PSP with DAIFMT_DSP_A) and two full frames per sample. I can more or less handle S16_LE and S32_LE from userspace now. Using S24_LE doesn't work because the codec supports LSB aligned signals only up to 20-bit. S24_3LE I didn't try yet, but the PXA DMA can't advance the source pointer by 3 bytes per transfer, so that would be ugly anyway (and only possible if network mode works).
We finally got a mode now that outputs 2x16 bit on a 2x32 bits I2S frame, padded with zeros. But that is in non-network modes, more about that tomorrow ...
Strange, interesting ...
regards Philipp
On Wed, Mar 4, 2009 at 9:45 PM, pHilipp Zabel philipp.zabel@gmail.com wrote:
On Wed, Mar 4, 2009 at 2:34 AM, Daniel Mack daniel@caiaq.de wrote:
Hi Phillip,
glad to see someone is wasting time with that issue, too :)
On Tue, Mar 03, 2009 at 04:10:51PM +0100, Philipp Zabel wrote:
For consistency with 24-bit and 32-bit modes, don't send 16-bit stereo in one 32-bit transfer. Use 2 slots instead on Zylonite. It should result in exactly the same behaviour. Now it is possible to use 16-bit single slot transfers in pxa-ssp, which are needed for Magician to get two frame clock pulses per sample (one for each channel).
We've been fiddling around with these modes and registers for another two days in a row now and figured out that literally all the documentation about these registers is entirly bogus. In particular, the network mode with the associated timeslots simply does not work at all, according to our measurements.
Hence the question: how does your I2S signal look like at the moment? How many clocks do you measure in one frame cylce, how many of them are actually filled with data? And which format does the userspace use to send samples in?
Unfortunately, I can't measure anything. I don't own an oscilloscope and I don't even know if there are useful test points on the phone's mainboard. Also, I've come to the conclusion that on Magician there has to be some kind of flip-flop between the PXA272's SSPFRMCLK and the codec's LRCLK input.
[...]
Unfortunately, this means that if I play sound for an odd number of frame clocks, the next playback will have left and right channels switched. I checked this by measuring the number of frames sent with OSCR8. Is there a way to force pxa-ssp to only ever stop playback after an even number of frames?
regards Philipp
participants (4)
-
Daniel Mack -
Mark Brown -
Mark Brown -
Philipp Zabel