[RFC PATCH] Add combined clock support to Atmel SoC audio

[RFC PATCH] Add combined clock support to Atmel SoC audio

[Ryan Mallon]

The following patch is one that has been floating around in various
forms in our own internal trees for a while.

The Atmel SSC peripheral has seperate TX and RX clocks which use
separate pins from the the micro. TF (frame) and TK (clock) for transmit
and RF and RK for receive. Not all codecs have separate frame and bit
clocks for transmit and receive so we want to be able to do both
playback and capture using a single set of pins.

This patch introduces a combined clock mode for the Atmel SSC
peripheral. Which allows playback and capture to use a single set of
pins. Currently combined clock is only supported on the TF/TK pins (some
incomplete support exists for using RF/RK).

I have tested this patch on our AT91SAM9G45 + TLV320AIC26 platform.
Playback and capture work individually. Simultaneous playback and
capture have been tested by connecting a loopback cable on the linein
and lineout jacks and then doing:

  arecord -c 2 -f S16_LE -r 44100 > recording.wav &
  aplay 500hz_sine.wav

This patch is posted as RFC since the approach is incomplete and a bit
hackish. I am mostly interested in knowing if this is a sensible
approach, and could be cleaned up for mainline inclusion, or if there is
a better way to do this.

Signed-off-by: Ryan Mallon <ryan@bluewatersys.com>
---

diff --git a/sound/soc/atmel/atmel_ssc_dai.c b/sound/soc/atmel/atmel_ssc_dai.c
index 5d230ce..ee00172 100644
--- a/sound/soc/atmel/atmel_ssc_dai.c
+++ b/sound/soc/atmel/atmel_ssc_dai.c
@@ -43,6 +43,7 @@
 #include <sound/soc.h>
 
 #include <mach/hardware.h>
+#include <asm/atomic.h>
 
 #include "atmel-pcm.h"
 #include "atmel_ssc_dai.h"
@@ -246,7 +247,18 @@ static void atmel_ssc_shutdown(struct snd_pcm_substream *substream,
 	dma_params = ssc_p->dma_params[dir];
 
 	if (dma_params != NULL) {
-		ssc_writel(ssc_p->ssc->regs, CR, dma_params->mask->ssc_disable);
+		if (ssc_p->combined_clock) {
+			/*
+			 * When using a combined clock we only disable the
+			 * clock once all substreams have completed
+			 */
+			if (atomic_dec_and_test(&ssc_p->substreams_running))
+				ssc_writel(ssc_p->ssc->regs, CR,
+					   dma_params->mask->ssc_disable);
+		} else {
+			ssc_writel(ssc_p->ssc->regs, CR,
+				   dma_params->mask->ssc_disable);
+		}
 		pr_debug("atmel_ssc_shutdown: %s disabled SSC_SR=0x%08x\n",
 			(dir ? "receive" : "transmit"),
 			ssc_readl(ssc_p->ssc->regs, SR));
@@ -328,6 +340,21 @@ static int atmel_ssc_set_dai_clkdiv(struct snd_soc_dai *cpu_dai,
 	return 0;
 }
 
+void atmel_ssc_setup_combined_clock(struct atmel_ssc_info *ssc_p,
+				    int combined_clock)
+{
+	int i;
+
+	ssc_p->combined_clock = combined_clock;
+	for (i = 0; i < ARRAY_SIZE(ssc_dma_params); i++) {
+		if (ssc_p->combined_clock == ATMEL_SSC_CLOCK_TX_ON_RX)
+			ssc_dma_params[i][0].mask->ssc_enable |= SSC_BIT(CR_RXEN);
+		else if (ssc_p->combined_clock == ATMEL_SSC_CLOCK_RX_ON_TX)
+			ssc_dma_params[i][1].mask->ssc_enable |= SSC_BIT(CR_TXEN);
+	}
+}
+EXPORT_SYMBOL(atmel_ssc_setup_combined_clock);
+
 /*
  * Configure the SSC.
  */
@@ -548,6 +575,34 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream,
 			ssc_p->daifmt);
 		return -EINVAL;
 	}
+
+	if (ssc_p->combined_clock == ATMEL_SSC_CLOCK_RX_ON_TX) {
+		/* RX clock is always running */
+		rcmr &= ~SSC_BF(RCMR_CKO, 0x7);
+		rcmr |=  SSC_BF(RCMR_CKO, SSC_CKO_CONTINUOUS);
+
+		/* TX clock is always running */
+		tcmr &= ~SSC_BF(TCMR_CKO, 0x7);
+		tcmr |=  SSC_BF(TCMR_CKO, SSC_CKO_CONTINUOUS);
+
+		/* RX clock is sourced from TK pin */
+		rcmr &= ~SSC_BF(RCMR_CKS, 0x7);
+		rcmr |=  SSC_BF(RCMR_CKS, SSC_CKS_CLOCK);
+
+		/* Start RX on TX start */
+		rcmr &= ~SSC_BF(RCMR_START, 0xf);
+		rcmr |=  SSC_BF(RCMR_START, SSC_START_TX_RX);
+
+		if (dir == 1) {
+			/*
+			 * Set the TX clock period to the RX clock period
+			 * FIXME - Is this okay if we are already doing TX?
+			 */
+			tcmr &= 0x00ffffff;
+			tcmr |= rcmr & 0xff000000;
+		}
+	}
+
 	pr_debug("atmel_ssc_hw_params: "
 			"RCMR=%08x RFMR=%08x TCMR=%08x TFMR=%08x\n",
 			rcmr, rfmr, tcmr, tfmr);
@@ -581,6 +636,9 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream,
 			return ret;
 		}
 
+		if (ssc_p->combined_clock)
+			atomic_set(&ssc_p->substreams_running, 0);
+
 		ssc_p->initialized = 1;
 	}
 
@@ -616,6 +674,21 @@ static int atmel_ssc_prepare(struct snd_pcm_substream *substream,
 
 	ssc_writel(ssc_p->ssc->regs, CR, dma_params->mask->ssc_enable);
 
+	if (ssc_p->combined_clock) {
+		atomic_inc(&ssc_p->substreams_running);
+
+		if (ssc_p->combined_clock == ATMEL_SSC_CLOCK_RX_ON_TX &&
+		    dir == 1) {
+			/*
+			 * Jump start the dma for capture by loading the
+			 * transmit holding register with a dummy value.
+			 */
+			pr_debug("Jump starting SSC RX DMA\n");
+			if (!(ssc_readl(ssc_p->ssc->regs, SR) & (1 << 16)))
+				ssc_writel(ssc_p->ssc->regs, THR, 0);
+		}
+	}
+
 	pr_debug("%s enabled SSC_SR=0x%08x\n",
 			dir ? "receive" : "transmit",
 			ssc_readl(ssc_p->ssc->regs, SR));
diff --git a/sound/soc/atmel/atmel_ssc_dai.h b/sound/soc/atmel/atmel_ssc_dai.h
index 5d4f0f9..6b20ee6 100644
--- a/sound/soc/atmel/atmel_ssc_dai.h
+++ b/sound/soc/atmel/atmel_ssc_dai.h
@@ -102,6 +102,8 @@ struct atmel_ssc_state {
 	u32 ssc_imr;
 };
 
+#define ATMEL_SSC_CLOCK_RX_ON_TX	1
+#define ATMEL_SSC_CLOCK_TX_ON_RX	2
 
 struct atmel_ssc_info {
 	char *name;
@@ -115,8 +117,14 @@ struct atmel_ssc_info {
 	unsigned short rcmr_period;
 	struct atmel_pcm_dma_params *dma_params[2];
 	struct atmel_ssc_state ssc_state;
+
+	/* For combined clocks */
+	int combined_clock;
+	atomic_t substreams_running;
 };
 
 int atmel_ssc_set_audio(int ssc);
+extern void atmel_ssc_setup_combined_clock(struct atmel_ssc_info *ssc_p,
+					   int combined_clock);
 
 #endif /* _AT91_SSC_DAI_H */

Re: [RFC PATCH] Add combined clock support to Atmel SoC audio

[Mark Brown]

On Wed, Nov 24, 2010 at 11:05:11AM +1300, Ryan Mallon wrote:

> This patch is posted as RFC since the approach is incomplete and a bit
> hackish. I am mostly interested in knowing if this is a sensible
> approach, and could be cleaned up for mainline inclusion, or if there is
> a better way to do this.

This doesn't look obviously hideous.

It should set the symmetric_rates flag when going into combined clocks
mode.  A few other comments:

> +			if (atomic_dec_and_test(&ssc_p->substreams_running))
> +				ssc_writel(ssc_p->ssc->regs, CR,
> +					   dma_params->mask->ssc_disable);

It'd seem clearer to use a regular lock here; probably safer also as you
could get a race between the test and the write which tries to revert
the change - ie a:

	dec_and_test
			inc
			read
			write
	write

type pattern.

> +	if (ssc_p->combined_clock == ATMEL_SSC_CLOCK_RX_ON_TX) {

How about just calling these defines ATMEL_SSC_CLOCK_TX or _RX?  You're
identifying the clock line to use for both cases, it's probably clearer
to say "use the RX lines" or "use the TX" lines than say "do one on the
other" if you see what I mean.

> +		/* RX clock is sourced from TK pin */
> +		rcmr &= ~SSC_BF(RCMR_CKS, 0x7);
> +		rcmr |=  SSC_BF(RCMR_CKS, SSC_CKS_CLOCK);

Is this done by the driver normally, or is it done by the machine
normally?  If it's normally done by the machine perhaps it should be
moved into the driver in all cases.

> +		if (dir == 1) {
> +			/*
> +			 * Set the TX clock period to the RX clock period
> +			 * FIXME - Is this okay if we are already doing TX?
> +			 */
> +			tcmr &= 0x00ffffff;
> +			tcmr |= rcmr & 0xff000000;

Should probably enforce a constraint to stop users doing something that
forces the change?

Re: [RFC PATCH] Add combined clock support to Atmel SoC audio

[Ryan Mallon]

On 11/24/2010 12:29 PM, Mark Brown wrote:
> On Wed, Nov 24, 2010 at 11:05:11AM +1300, Ryan Mallon wrote:
> 
>> This patch is posted as RFC since the approach is incomplete and a bit
>> hackish. I am mostly interested in knowing if this is a sensible
>> approach, and could be cleaned up for mainline inclusion, or if there is
>> a better way to do this.
> 
> This doesn't look obviously hideous.

Thanks :-)

> It should set the symmetric_rates flag when going into combined clocks
> mode.  A few other comments:
> 
>> +			if (atomic_dec_and_test(&ssc_p->substreams_running))
>> +				ssc_writel(ssc_p->ssc->regs, CR,
>> +					   dma_params->mask->ssc_disable);

Okay.

> It'd seem clearer to use a regular lock here; probably safer also as you
> could get a race between the test and the write which tries to revert
> the change - ie a:
> 
> 	dec_and_test
> 			inc
> 			read
> 			write
> 	write
>
> type pattern.

Okay, regular spin_lock should be okay right?

>> +	if (ssc_p->combined_clock == ATMEL_SSC_CLOCK_RX_ON_TX) {
> 
> How about just calling these defines ATMEL_SSC_CLOCK_TX or _RX?  You're
> identifying the clock line to use for both cases, it's probably clearer
> to say "use the RX lines" or "use the TX" lines than say "do one on the
> other" if you see what I mean.

Okay, will change.

> 
>> +		/* RX clock is sourced from TK pin */
>> +		rcmr &= ~SSC_BF(RCMR_CKS, 0x7);
>> +		rcmr |=  SSC_BF(RCMR_CKS, SSC_CKS_CLOCK);
>
> Is this done by the driver normally, or is it done by the machine
> normally?  If it's normally done by the machine perhaps it should be
> moved into the driver in all cases.

Essentially this code is overriding the settings in the hw_params switch
statement for the combined clocks case. This will need to be overridden
each time hw_params is called. Doing it here seems logical since
atmel_ssc_dai:hw_params does the original setting. It keeps the machine
drivers simpler too.

>> +		if (dir == 1) {
>> +			/*
>> +			 * Set the TX clock period to the RX clock period
>> +			 * FIXME - Is this okay if we are already doing TX?
>> +			 */
>> +			tcmr &= 0x00ffffff;
>> +			tcmr |= rcmr & 0xff000000;
> 
> Should probably enforce a constraint to stop users doing something that
> forces the change?

Okay. Could you point me at an example for this please.

Thanks,
~Ryan

-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
ryan@bluewatersys.com         	PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com	New Zealand
Phone: +64 3 3779127		Freecall: Australia 1800 148 751
Fax:   +64 3 3779135			  USA 1800 261 2934

Re: [RFC PATCH] Add combined clock support to Atmel SoC audio

[Mark Brown]

On Wed, Nov 24, 2010 at 05:02:55PM +1300, Ryan Mallon wrote:

> Okay, regular spin_lock should be okay right?

I guess, though something like a mutex would seem more normal.

> > Is this done by the driver normally, or is it done by the machine
> > normally?  If it's normally done by the machine perhaps it should be
> > moved into the driver in all cases.

> Essentially this code is overriding the settings in the hw_params switch
> statement for the combined clocks case. This will need to be overridden
> each time hw_params is called. Doing it here seems logical since
> atmel_ssc_dai:hw_params does the original setting. It keeps the machine
> drivers simpler too.

That's my point - if the machine drivers normally need to do this
(sam9g20ek certainly needed to configure the pinmux) then we should just
be doing this all the time.

> >> +		if (dir == 1) {
> >> +			/*
> >> +			 * Set the TX clock period to the RX clock period
> >> +			 * FIXME - Is this okay if we are already doing TX?
> >> +			 */
> >> +			tcmr &= 0x00ffffff;
> >> +			tcmr |= rcmr & 0xff000000;

> > Should probably enforce a constraint to stop users doing something that
> > forces the change?

> Okay. Could you point me at an example for this please.

symmetric_rates probably covers it.

Re: [RFC PATCH] Add combined clock support to Atmel SoC audio

[Sergey Lapin]

On Wed, Nov 24, 2010 at 11:05:11AM +1300, Ryan Mallon wrote:
> The following patch is one that has been floating around in various
> forms in our own internal trees for a while.
> 
> The Atmel SSC peripheral has seperate TX and RX clocks which use
> separate pins from the the micro. TF (frame) and TK (clock) for transmit
> and RF and RK for receive. Not all codecs have separate frame and bit
> clocks for transmit and receive so we want to be able to do both
> playback and capture using a single set of pins.
> 
> This patch introduces a combined clock mode for the Atmel SSC
> peripheral. Which allows playback and capture to use a single set of
> pins. Currently combined clock is only supported on the TF/TK pins (some
> incomplete support exists for using RF/RK).
> 
> I have tested this patch on our AT91SAM9G45 + TLV320AIC26 platform.
> Playback and capture work individually. Simultaneous playback and
> capture have been tested by connecting a loopback cable on the linein
> and lineout jacks and then doing:
> 
>   arecord -c 2 -f S16_LE -r 44100 > recording.wav &
>   aplay 500hz_sine.wav
> 
> This patch is posted as RFC since the approach is incomplete and a bit
> hackish. I am mostly interested in knowing if this is a sensible
> approach, and could be cleaned up for mainline inclusion, or if there is
> a better way to do this.
> 
> Signed-off-by: Ryan Mallon <ryan@bluewatersys.com>
Tested-by: Sergey Lapin <slapin@ossfans.org>

Re: [RFC PATCH] Add combined clock support to Atmel SoC audio

[Sergey Lapin]

Hi, Ryan!

On Wed, Nov 24, 2010 at 11:05:11AM +1300, Ryan Mallon wrote:
> The following patch is one that has been floating around in various
> forms in our own internal trees for a while.
> 
> The Atmel SSC peripheral has seperate TX and RX clocks which use
> separate pins from the the micro. TF (frame) and TK (clock) for transmit
> and RF and RK for receive. Not all codecs have separate frame and bit
> clocks for transmit and receive so we want to be able to do both
> playback and capture using a single set of pins.
> 
> This patch introduces a combined clock mode for the Atmel SSC
> peripheral. Which allows playback and capture to use a single set of
> pins. Currently combined clock is only supported on the TF/TK pins (some
> incomplete support exists for using RF/RK).
> 
> I have tested this patch on our AT91SAM9G45 + TLV320AIC26 platform.
> Playback and capture work individually. Simultaneous playback and
> capture have been tested by connecting a loopback cable on the linein
> and lineout jacks and then doing:
> 
>   arecord -c 2 -f S16_LE -r 44100 > recording.wav &
>   aplay 500hz_sine.wav
> 
> This patch is posted as RFC since the approach is incomplete and a bit
> hackish. I am mostly interested in knowing if this is a sensible
> approach, and could be cleaned up for mainline inclusion, or if there is
> a better way to do this.
> 
> Signed-off-by: Ryan Mallon <ryan@bluewatersys.com>

I think it is also important to submit code, which uses it.
If you can't do it I might try to do this in a few days.
Also worth mentioning codec slave mode requirement for this to work.

All the best,
S.

Re: [RFC PATCH] Add combined clock support to Atmel SoC audio

[Ryan Mallon]

On 07/06/11 20:03, Sergey Lapin wrote:
> Hi, Ryan!
> 
> On Wed, Nov 24, 2010 at 11:05:11AM +1300, Ryan Mallon wrote:
>> The following patch is one that has been floating around in various
>> forms in our own internal trees for a while.
>>
>> The Atmel SSC peripheral has seperate TX and RX clocks which use
>> separate pins from the the micro. TF (frame) and TK (clock) for transmit
>> and RF and RK for receive. Not all codecs have separate frame and bit
>> clocks for transmit and receive so we want to be able to do both
>> playback and capture using a single set of pins.
>>
>> This patch introduces a combined clock mode for the Atmel SSC
>> peripheral. Which allows playback and capture to use a single set of
>> pins. Currently combined clock is only supported on the TF/TK pins (some
>> incomplete support exists for using RF/RK).
>>
>> I have tested this patch on our AT91SAM9G45 + TLV320AIC26 platform.
>> Playback and capture work individually. Simultaneous playback and
>> capture have been tested by connecting a loopback cable on the linein
>> and lineout jacks and then doing:
>>
>>   arecord -c 2 -f S16_LE -r 44100 > recording.wav &
>>   aplay 500hz_sine.wav
>>
>> This patch is posted as RFC since the approach is incomplete and a bit
>> hackish. I am mostly interested in knowing if this is a sensible
>> approach, and could be cleaned up for mainline inclusion, or if there is
>> a better way to do this.
>>
>> Signed-off-by: Ryan Mallon <ryan@bluewatersys.com>
> 
> I think it is also important to submit code, which uses it.
> If you can't do it I might try to do this in a few days.
> Also worth mentioning codec slave mode requirement for this to work.

Agreed. I don't have any hardware at the moment. I had intended to post
support for audio on the Bluewater Systems Snapper 9260 and 9G20 modules
(this patch being a precursor to that support), but I am no longer
working at Bluewater. Did you manage to get your hardware working in the
end?

I don't think the patch is ready for merging as is. It is incomplete
(though I suspect support for tx on the rx pins probably isn't needed)
and Mark also had some comments last time round: Use symmetric_rates,
replace atomic type with proper lock, etc. I think I may have done some
of this already, so can try and dig it out.

Also, this email address will cease to exist soon. Can you please use my
rmallon@gmail.com account instead. I'll have a kernel patch out soon to
fix my mail address up.

~Ryan