[alsa-devel] [RFC PATCH] Add combined clock support to Atmel SoC audio
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 */
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?
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
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.
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
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.
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
participants (3)
-
Mark Brown -
Ryan Mallon -
Sergey Lapin