[PATCH] ALSA: soc - fsl_ssi.c fix audio capture

[PATCH] ALSA: soc - fsl_ssi.c fix audio capture

[Anton Vorontsov]

Since we're using SSI in synchronous mode, the STCCR register controls
both the receive and transmit sections. So, when we're trying to record
anything, stccr register does not get initialized, thus the output file
filled with the white noise.

Fix this by initializing the STCCR for both playback and capture. If
TX/RX widths don't match, return that we're busy at the moment.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 sound/soc/fsl/fsl_ssi.c |   21 +++++++++++++++------
 1 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index f588545..31f689e 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -372,16 +372,24 @@ static int fsl_ssi_prepare(struct snd_pcm_substream *substream)
 
 	struct ccsr_ssi __iomem *ssi = ssi_private->ssi;
 	u32 wl;
+	u32 cur_wl;
 
 	wl = CCSR_SSI_SxCCR_WL(snd_pcm_format_width(runtime->format));
 
-	clrbits32(&ssi->scr, CCSR_SSI_SCR_SSIEN);
-
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
-		clrsetbits_be32(&ssi->stccr, CCSR_SSI_SxCCR_WL_MASK, wl);
-	else
-		clrsetbits_be32(&ssi->srccr, CCSR_SSI_SxCCR_WL_MASK, wl);
+	/*
+	 * MPC8610 spec says: "The STCCR register is dedicated to the transmit
+	 * section, and the SRCCR register is dedicated to the receive section
+	 * except in Synchronous mode, in which the STCCR register controls
+	 * both the receive and transmit sections."
+	 * So, the width for TX and RX should match, otherwise we're busy with
+	 * either TX or RX.
+	 */
+	cur_wl = in_be32(&ssi->stccr) & CCSR_SSI_SxCCR_WL_MASK;
+	if (cur_wl && cur_wl != wl)
+		return -EBUSY;
 
+	clrbits32(&ssi->scr, CCSR_SSI_SCR_SSIEN);
+	clrsetbits_be32(&ssi->stccr, CCSR_SSI_SxCCR_WL_MASK, wl);
 	setbits32(&ssi->scr, CCSR_SSI_SCR_SSIEN);
 
 	return 0;
@@ -460,6 +468,7 @@ static void fsl_ssi_shutdown(struct snd_pcm_substream *substream)
 		struct ccsr_ssi __iomem *ssi = ssi_private->ssi;
 
 		clrbits32(&ssi->scr, CCSR_SSI_SCR_SSIEN);
+		clrsetbits_be32(&ssi->stccr, CCSR_SSI_SxCCR_WL_MASK, 0);
 
 		free_irq(ssi_private->irq, ssi_private);
 	}
-- 
1.5.5.4

Re: [PATCH] ALSA: soc - fsl_ssi.c fix audio capture

[Mark Brown]

On Thu, Jul 03, 2008 at 06:37:14PM +0400, Anton Vorontsov wrote:
> Since we're using SSI in synchronous mode, the STCCR register controls
> both the receive and transmit sections. So, when we're trying to record
> anything, stccr register does not get initialized, thus the output file
> filled with the white noise.

> Fix this by initializing the STCCR for both playback and capture. If
> TX/RX widths don't match, return that we're busy at the moment.

> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>

Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

but Timur needs to ack it since I don't have any particular knowledge of
the hardware.

Re: [PATCH] ALSA: soc - fsl_ssi.c fix audio capture

[Mark Brown]

On Fri, Jul 04, 2008 at 07:02:17AM -0400, Timur Tabi wrote:

> Anton, are you sure returning -EBUSY is the right fix?  Would this make 
> applications like mplayer detect the problem and automatically pick a 
> matching sample format that does work?

It's probably worth testing with OSS emulation too, though you could
decide that you don't want to support that.

Re: [PATCH] ALSA: soc - fsl_ssi.c fix audio capture

[Timur Tabi]

On Jul 3, 2008, at 10:42 AM, Mark Brown wrote:

> On Thu, Jul 03, 2008 at 06:37:14PM +0400, Anton Vorontsov wrote:
>> Since we're using SSI in synchronous mode, the STCCR register  
>> controls
>> both the receive and transmit sections. So, when we're trying to  
>> record
>> anything, stccr register does not get initialized, thus the output  
>> file
>> filled with the white noise.
>
>> Fix this by initializing the STCCR for both playback and capture. If
>> TX/RX widths don't match, return that we're busy at the moment.
>
>> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
>
> Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
>
> but Timur needs to ack it since I don't have any particular  
> knowledge of
> the hardware.

Anton has the right idea, but I'm not sure the fix is the best.  I was  
planning on posting my fix after I got back from vacation.

Part of the problem is that STCK and SRCK can be wired together, which  
means that even though you're not in synchronous mode, the sample  
rates have to match.  For the 8610 HPCD, this isn't a problem, but the  
SSI driver is supposed to support more than just that board.  We need  
device tree additions to cover all cases.

Anton, are you sure returning -EBUSY is the right fix?  Would this  
make applications like mplayer detect the problem and automatically  
pick a matching sample format that does work?

Re: [PATCH] ALSA: soc - fsl_ssi.c fix audio capture

[Timur Tabi]

On Jul 4, 2008, at 7:01 AM, Mark Brown wrote:

> It's probably worth testing with OSS emulation too, though you could
> decide that you don't want to support that.

Sadly, we have a customer that insists on OSS support, so that is a  
hard requirement.

Re: [PATCH] ALSA: soc - fsl_ssi.c fix audio capture

[Takashi Iwai]

At Fri, 4 Jul 2008 07:02:17 -0400,
Timur Tabi wrote:
> 
> 
> On Jul 3, 2008, at 10:42 AM, Mark Brown wrote:
> 
> > On Thu, Jul 03, 2008 at 06:37:14PM +0400, Anton Vorontsov wrote:
> >> Since we're using SSI in synchronous mode, the STCCR register  
> >> controls
> >> both the receive and transmit sections. So, when we're trying to  
> >> record
> >> anything, stccr register does not get initialized, thus the output  
> >> file
> >> filled with the white noise.
> >
> >> Fix this by initializing the STCCR for both playback and capture. If
> >> TX/RX widths don't match, return that we're busy at the moment.
> >
> >> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> >
> > Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> >
> > but Timur needs to ack it since I don't have any particular  
> > knowledge of
> > the hardware.
> 
> Anton has the right idea, but I'm not sure the fix is the best.  I was  
> planning on posting my fix after I got back from vacation.
> 
> Part of the problem is that STCK and SRCK can be wired together, which  
> means that even though you're not in synchronous mode, the sample  
> rates have to match.  For the 8610 HPCD, this isn't a problem, but the  
> SSI driver is supposed to support more than just that board.  We need  
> device tree additions to cover all cases.
> 
> Anton, are you sure returning -EBUSY is the right fix?  Would this  
> make applications like mplayer detect the problem and automatically  
> pick a matching sample format that does work?

I guess it won't work in such a way.  But, at least, you can avoid
unexpected machine state, which resulted in white noise.

Since you will post another patch (I suppose it's with hw_constraint
coupling), I'll postpone this patch as now.


thanks,

Takashi

Re: [PATCH] ALSA: soc - fsl_ssi.c fix audio capture

[Anton Vorontsov]

On Fri, Jul 04, 2008 at 07:02:17AM -0400, Timur Tabi wrote:
>
> On Jul 3, 2008, at 10:42 AM, Mark Brown wrote:
>
>> On Thu, Jul 03, 2008 at 06:37:14PM +0400, Anton Vorontsov wrote:
>>> Since we're using SSI in synchronous mode, the STCCR register  
>>> controls
>>> both the receive and transmit sections. So, when we're trying to  
>>> record
>>> anything, stccr register does not get initialized, thus the output  
>>> file
>>> filled with the white noise.
>>
>>> Fix this by initializing the STCCR for both playback and capture. If
>>> TX/RX widths don't match, return that we're busy at the moment.
>>
>>> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
>>
>> Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
>>
>> but Timur needs to ack it since I don't have any particular knowledge 
>> of
>> the hardware.
>
> Anton has the right idea, but I'm not sure the fix is the best.  I was  
> planning on posting my fix after I got back from vacation.
>
> Part of the problem is that STCK and SRCK can be wired together, which  
> means that even though you're not in synchronous mode, the sample rates 
> have to match.  For the 8610 HPCD, this isn't a problem, but the SSI 
> driver is supposed to support more than just that board.  We need device 
> tree additions to cover all cases.

Ok, we can set both.

> Anton, are you sure returning -EBUSY is the right fix? 

Since we're using prepare(), yes. But we probably should use
hw_params() and return -EINVAL.

> Would this make 
> applications like mplayer detect the problem and automatically pick a 
> matching sample format that does work?

I don't think that anyone tries to negotiate if/when prepare() failed.
But if hw_params() failed, then yes. Here is updated patch.

Also new patch for the codec used on the HPCD: it seem has the similar
issue, but wrt sampling rate.

On Fri, Jul 04, 2008 at 04:49:29PM +0200, Takashi Iwai wrote:
> I guess it won't work in such a way.  But, at least, you can avoid
> unexpected machine state, which resulted in white noise.
> Since you will post another patch (I suppose it's with hw_constraint
> coupling), I'll postpone this patch as now.

Hm... Not sure hw_constraints are appropriate in these cases, as see
it, they all called in the open routines, while we set up things much
later -- in the hw_params, so we want "dynamic" constraints (please
take a look at the second patch, it is simpler).

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

[PATCH 1/2] ALSA: soc - fsl_ssi.c fix audio capture

[Anton Vorontsov]

Since we're using SSI in synchronous mode, the STCCR register controls
both the receive and transmit sections. So, when we're trying to record
anything, stccr register does not get initialized, thus the output file
filled with the white noise.

Fix this by initializing the STCCR for both playback and capture.

Also use hw_params and hw_free callbacks, so that we won't fail at the
last moment, thus applications could negotiate.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 sound/soc/fsl/fsl_ssi.c |   69 ++++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 59 insertions(+), 10 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 145ad13..07b19ed 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -69,6 +69,8 @@
  * @ssi_phys: physical address of the SSI registers
  * @irq: IRQ of this SSI
  * @dev: struct device pointer
+ * @master_substream: substream that owns hw params
+ * @width: format width that master substream has configured
  * @playback: the number of playback streams opened
  * @capture: the number of capture streams opened
  * @cpu_dai: the CPU DAI for this device
@@ -81,6 +83,8 @@ struct fsl_ssi_private {
 	dma_addr_t ssi_phys;
 	unsigned int irq;
 	struct device *dev;
+	struct snd_pcm_substream *master_substream;
+	unsigned int width;
 	unsigned int playback;
 	unsigned int capture;
 	struct snd_soc_cpu_dai cpu_dai;
@@ -367,22 +371,24 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream)
  */
 static int fsl_ssi_prepare(struct snd_pcm_substream *substream)
 {
-	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
 	struct fsl_ssi_private *ssi_private = rtd->dai->cpu_dai->private_data;
 
 	struct ccsr_ssi __iomem *ssi = ssi_private->ssi;
-	u32 wl;
-
-	wl = CCSR_SSI_SxCCR_WL(snd_pcm_format_width(runtime->format));
+	u32 wl = CCSR_SSI_SxCCR_WL(ssi_private->width);
 
 	clrbits32(&ssi->scr, CCSR_SSI_SCR_SSIEN);
-
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
-		clrsetbits_be32(&ssi->stccr, CCSR_SSI_SxCCR_WL_MASK, wl);
-	else
-		clrsetbits_be32(&ssi->srccr, CCSR_SSI_SxCCR_WL_MASK, wl);
-
+	/*
+	 * MPC8610 spec says: "The STCCR register is dedicated to the transmit
+	 * section, and the SRCCR register is dedicated to the receive section
+	 * except in Synchronous mode, in which the STCCR register controls
+	 * both the receive and transmit sections."
+	 * So, the width for TX and RX should match, otherwise we're busy with
+	 * either TX or RX. Also, STCK and SRCK lines could be wired together,
+	 * so we also program the SRCCR, so that TX and RX will not conflict.
+	 */
+	clrsetbits_be32(&ssi->stccr, CCSR_SSI_SxCCR_WL_MASK, wl);
+	clrsetbits_be32(&ssi->srccr, CCSR_SSI_SxCCR_WL_MASK, wl);
 	setbits32(&ssi->scr, CCSR_SSI_SCR_SSIEN);
 
 	return 0;
@@ -461,6 +467,8 @@ static void fsl_ssi_shutdown(struct snd_pcm_substream *substream)
 		struct ccsr_ssi __iomem *ssi = ssi_private->ssi;
 
 		clrbits32(&ssi->scr, CCSR_SSI_SCR_SSIEN);
+		clrsetbits_be32(&ssi->stccr, CCSR_SSI_SxCCR_WL_MASK, 0);
+		clrsetbits_be32(&ssi->srccr, CCSR_SSI_SxCCR_WL_MASK, 0);
 
 		free_irq(ssi_private->irq, ssi_private);
 	}
@@ -503,6 +511,45 @@ static int fsl_ssi_set_fmt(struct snd_soc_cpu_dai *cpu_dai, unsigned int format)
 	return (format == SND_SOC_DAIFMT_I2S) ? 0 : -EINVAL;
 }
 
+static int fsl_ssi_hw_params(struct snd_pcm_substream *substream,
+			    struct snd_pcm_hw_params *params)
+{
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct fsl_ssi_private *ssi_private = rtd->dai->cpu_dai->private_data;
+	unsigned int width = snd_pcm_format_width(params_format(params));
+
+	/*
+	 * If there is no hw_params' master substream, first one becomes
+	 * master. All other substreams must comply with the format width
+	 * of the master substream.
+	 */
+	if (!ssi_private->master_substream)
+		ssi_private->master_substream = substream;
+	else if (ssi_private->master_substream != substream &&
+			ssi_private->width != width)
+		return -EINVAL;
+	/*
+	 * hw_params callback can be issued multiple times with different
+	 * params, for example when an application negotiates the parameters.
+	 */
+	ssi_private->width = width;
+	return 0;
+}
+
+static int fsl_ssi_hw_free(struct snd_pcm_substream *substream)
+{
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct fsl_ssi_private *ssi_private = rtd->dai->cpu_dai->private_data;
+
+	/*
+	 * Master substream don't want to own the hw params anymore, so from
+	 * now on other streams could do their own params.
+	 */
+	if (ssi_private->master_substream == substream)
+		ssi_private->master_substream = NULL;
+	return 0;
+}
+
 /**
  * fsl_ssi_dai_template: template CPU DAI for the SSI
  */
@@ -522,6 +569,8 @@ static struct snd_soc_cpu_dai fsl_ssi_dai_template = {
 	},
 	.ops = {
 		.startup = fsl_ssi_startup,
+		.hw_params = fsl_ssi_hw_params,
+		.hw_free = fsl_ssi_hw_free,
 		.prepare = fsl_ssi_prepare,
 		.shutdown = fsl_ssi_shutdown,
 		.trigger = fsl_ssi_trigger,
-- 
1.5.5.4

[PATCH 2/2] ALSA: soc - cs4270: fix ADC/DAC rate matching

[Anton Vorontsov]

CS4270 is unable to handle different rates separately for ADC and DAC,
so the rates should always match. This patch fixes it so that the first
substream that configured the codec will own hardware parameters, thus
subsequent substreams should comply with existing parameters or bail out.

This patch fixes the issue when capture substream may ruin the playback,
and vice versa.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 sound/soc/codecs/cs4270.c |   30 ++++++++++++++++++++++++++++++
 1 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/sound/soc/codecs/cs4270.c b/sound/soc/codecs/cs4270.c
index d250491..d5a7a00 100644
--- a/sound/soc/codecs/cs4270.c
+++ b/sound/soc/codecs/cs4270.c
@@ -45,6 +45,8 @@
 struct cs4270_private {
 	unsigned int mclk; /* Input frequency of the MCLK pin */
 	unsigned int mode; /* The mode (I2S or left-justified) */
+	struct snd_pcm_substream *master_substream; /* Owns hw_params */
+	unsigned int rate; /* Rate that master substream has configured */
 };
 
 /*
@@ -383,6 +385,19 @@ static int cs4270_hw_params(struct snd_pcm_substream *substream,
 	rate = params_rate(params);	/* Sampling rate, in Hz */
 	ratio = cs4270->mclk / rate;	/* MCLK/LRCK ratio */
 
+	/*
+	 * CODEC is using the same rate for ADC and DAC. If, for example,
+	 * we're currently playing something with one rate, we must use the
+	 * same rate for recording. The substream that did hw_params first
+	 * wins.
+	 */
+	if (!cs4270->master_substream)
+		cs4270->master_substream = substream;
+	else if (cs4270->master_substream != substream &&
+			cs4270->rate != rate)
+		return -EINVAL;
+	cs4270->rate = rate;
+
 	for (i = 0; i < NUM_MCLK_RATIOS; i++) {
 		if (cs4270_mode_ratios[i].ratio == ratio)
 			break;
@@ -462,6 +477,20 @@ static int cs4270_hw_params(struct snd_pcm_substream *substream,
 	return ret;
 }
 
+static int cs4270_hw_free(struct snd_pcm_substream *substream)
+{
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct cs4270_private *cs4270 = rtd->socdev->codec->private_data;
+
+	/*
+	 * Master substream don't want to own the hw params anymore, so from
+	 * now on other streams could do their own params.
+	 */
+	if (cs4270->master_substream == substream)
+		cs4270->master_substream = NULL;
+	return 0;
+}
+
 #ifdef CONFIG_SND_SOC_CS4270_HWMUTE
 
 /*
@@ -745,6 +774,7 @@ static int cs4270_probe(struct platform_device *pdev)
 	if (codec->control_data) {
 		/* Initialize codec ops */
 		cs4270_dai.ops.hw_params = cs4270_hw_params;
+		cs4270_dai.ops.hw_free = cs4270_hw_free;
 		cs4270_dai.dai_ops.set_sysclk = cs4270_set_dai_sysclk;
 		cs4270_dai.dai_ops.set_fmt = cs4270_set_dai_fmt;
 #ifdef CONFIG_SND_SOC_CS4270_HWMUTE
-- 
1.5.5.4

Re: [PATCH] ALSA: soc - fsl_ssi.c fix audio capture

[Anton Vorontsov]

On Fri, Jul 04, 2008 at 07:43:13PM +0400, Anton Vorontsov wrote:
[...]
> >>> Since we're using SSI in synchronous mode, the STCCR register  
> >>> controls
> >>> both the receive and transmit sections. So, when we're trying to  
> >>> record
> >>> anything, stccr register does not get initialized, thus the output  
> >>> file
> >>> filled with the white noise.
> >>
> >>> Fix this by initializing the STCCR for both playback and capture. If
> >>> TX/RX widths don't match, return that we're busy at the moment.
> >>
> >>> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> >>
> >> Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> >>
> >> but Timur needs to ack it since I don't have any particular knowledge 
> >> of
> >> the hardware.
> >
> > Anton has the right idea, but I'm not sure the fix is the best.  I was  
> > planning on posting my fix after I got back from vacation.
> >
> > Part of the problem is that STCK and SRCK can be wired together, which  
> > means that even though you're not in synchronous mode, the sample rates 
> > have to match.  For the 8610 HPCD, this isn't a problem, but the SSI 
> > driver is supposed to support more than just that board.  We need device 
> > tree additions to cover all cases.
> 
> Ok, we can set both.
> 
> > Anton, are you sure returning -EBUSY is the right fix? 
> 
> Since we're using prepare(), yes. But we probably should use
> hw_params() and return -EINVAL.
> 
> > Would this make 
> > applications like mplayer detect the problem and automatically pick a 
> > matching sample format that does work?
> 
> I don't think that anyone tries to negotiate if/when prepare() failed.
> But if hw_params() failed, then yes. Here is updated patch.
> 
> Also new patch for the codec used on the HPCD: it seem has the similar
> issue, but wrt sampling rate.

Timur, have you had a chance to look into these two patches?

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH] ALSA: soc - fsl_ssi.c fix audio capture

[Timur Tabi]

Anton Vorontsov wrote:

> Timur, have you had a chance to look into these two patches?

No, sorry.  Unfortunately, I have a more critical issue with the SSI that I'm
debugging now, so it's holding up other work.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH 2/2] ALSA: soc - cs4270: fix ADC/DAC rate matching

[Timur Tabi]

Anton Vorontsov wrote:

> +	if (!cs4270->master_substream)
> +		cs4270->master_substream = substream;

This should be done in a _startup function, not here.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH 1/2] ALSA: soc - fsl_ssi.c fix audio capture

[Timur Tabi]

Anton Vorontsov wrote:
> Since we're using SSI in synchronous mode, the STCCR register controls
> both the receive and transmit sections. So, when we're trying to record
> anything, stccr register does not get initialized, thus the output file
> filled with the white noise.
> 
> Fix this by initializing the STCCR for both playback and capture.
> 
> Also use hw_params and hw_free callbacks, so that we won't fail at the
> last moment, thus applications could negotiate.

I already have a patch that fixes this problem, I'm just not ready to post it.

I really do appreciate your efforts, Anton, but you have to give me a chance to
do my job!

-- 
Timur Tabi
Linux kernel developer at Freescale