| Message ID | a541333cf46b4c61b790df927d4b315d@EMAIL.axentia.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Peter, On 10/20/2014 09:45 PM, Peter Rosin wrote: > From 1e5621d7b9887c648d1a66238dc82d715c1e2cad Mon Sep 17 00:00:00 2001 > From: Peter Rosin <peda@axentia.se> > Date: Mon, 20 Oct 2014 14:38:04 +0200 > Subject: [PATCH] ASoC: atmel_ssc_dai: Track playback and capture CMR dividers > separately. > > The CMR divider register is shared by playback and capture. The SSC driver > therefore tries to enforce rules so that the needed register content do > not conflict during simultaneous playback/capture. However, the > implementation also prevents changing the register content in a > half-duplex scenario, which is needed when changing sample rates. I am not fully get what you mean here, do you mean: - when playback, first playback 48kHz, and then playback 8Khz, the 8Khz still playback in 48kHz mode? or other things? > Thus, keep track of the desired playback and capture clock dividers > separately, and allow changing rates without closing the stream. > > Signed-off-by: Peter Rosin <peda@axentia.se> > --- > sound/soc/atmel/atmel_ssc_dai.c | 31 ++++++++++++++++++++++--------- > sound/soc/atmel/atmel_ssc_dai.h | 10 ++++++---- > 2 files changed, 28 insertions(+), 13 deletions(-) > > diff --git a/sound/soc/atmel/atmel_ssc_dai.c b/sound/soc/atmel/atmel_ssc_dai.c > index f403f39..fec14fb 100644 > --- a/sound/soc/atmel/atmel_ssc_dai.c > +++ b/sound/soc/atmel/atmel_ssc_dai.c > @@ -277,7 +277,8 @@ static void atmel_ssc_shutdown(struct snd_pcm_substream *substream, > /* Reset the SSC */ > ssc_writel(ssc_p->ssc->regs, CR, SSC_BIT(CR_SWRST)); > /* Clear the SSC dividers */ > - ssc_p->cmr_div = ssc_p->tcmr_period = ssc_p->rcmr_period = 0; > + ssc_p->tcmr_div = ssc_p->rcmr_div = 0; > + ssc_p->tcmr_period = ssc_p->rcmr_period = 0; > } > spin_unlock_irq(&ssc_p->lock); > } > @@ -304,17 +305,27 @@ static int atmel_ssc_set_dai_clkdiv(struct snd_soc_dai *cpu_dai, > struct atmel_ssc_info *ssc_p = &ssc_info[cpu_dai->id]; > > switch (div_id) { > - case ATMEL_SSC_CMR_DIV: > + case ATMEL_SSC_TCMR_DIV: > /* > * The same master clock divider is used for both > * transmit and receive, so if a value has already > - * been set, it must match this value. > + * been set for the other direction, it must match > + * this value. > */ > - if (ssc_p->cmr_div == 0) > - ssc_p->cmr_div = div; > - else > - if (div != ssc_p->cmr_div) > - return -EBUSY; > + if (ssc_p->rcmr_div == 0) > + ssc_p->tcmr_div = div; > + else if (div != ssc_p->rcmr_div) > + return -EBUSY; > + break; > + > + case ATMEL_SSC_RCMR_DIV: > + /* > + * See ATMEL_SSC_TCMR_DIV. > + */ > + if (ssc_p->tcmr_div == 0) > + ssc_p->rcmr_div = div; > + else if (div != ssc_p->tcmr_div) > + return -EBUSY; > break; > > case ATMEL_SSC_TCMR_PERIOD: > @@ -345,6 +356,7 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream, > struct atmel_pcm_dma_params *dma_params; > int dir, channels, bits; > u32 tfmr, rfmr, tcmr, rcmr; > + u16 cmr; should be u32. > int start_event; > int ret; > int fslen, fslen_ext; > @@ -626,7 +638,8 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream, > } > > /* set SSC clock mode register */ > - ssc_writel(ssc_p->ssc->regs, CMR, ssc_p->cmr_div); > + cmr = ssc_p->tcmr_div ? ssc_p->tcmr_div : ssc_p->rcmr_div; > + ssc_writel(ssc_p->ssc->regs, CMR, cmr); > > /* set receive clock mode and format */ > ssc_writel(ssc_p->ssc->regs, RCMR, rcmr); > diff --git a/sound/soc/atmel/atmel_ssc_dai.h b/sound/soc/atmel/atmel_ssc_dai.h > index b1f08d5..a25df7a 100644 > --- a/sound/soc/atmel/atmel_ssc_dai.h > +++ b/sound/soc/atmel/atmel_ssc_dai.h > @@ -39,9 +39,10 @@ > #define ATMEL_SYSCLK_MCK 0 /* SSC uses AT91 MCK as system clock */ > > /* SSC divider ids */ > -#define ATMEL_SSC_CMR_DIV 0 /* MCK divider for BCLK */ > -#define ATMEL_SSC_TCMR_PERIOD 1 /* BCLK divider for transmit FS */ > -#define ATMEL_SSC_RCMR_PERIOD 2 /* BCLK divider for receive FS */ > +#define ATMEL_SSC_TCMR_DIV 0 /* MCK divider for transmit BCLK */ > +#define ATMEL_SSC_RCMR_DIV 1 /* MCK divider for receive BCLK */ > +#define ATMEL_SSC_TCMR_PERIOD 2 /* BCLK divider for transmit FS */ > +#define ATMEL_SSC_RCMR_PERIOD 3 /* BCLK divider for receive FS */ > /* > * SSC direction masks > */ > @@ -110,7 +111,8 @@ struct atmel_ssc_info { > unsigned short dir_mask; /* 0=unused, 1=playback, 2=capture */ > unsigned short initialized; /* true if SSC has been initialized */ > unsigned short daifmt; > - unsigned short cmr_div; > + unsigned short tcmr_div; > + unsigned short rcmr_div; > unsigned short tcmr_period; > unsigned short rcmr_period; > struct atmel_pcm_dma_params *dma_params[2]; > Best Regards, Bo Shen
Hi! (and thank you for the pointer to the example with the ssc-dai in master mode) > Hi Peter, > > On 10/20/2014 09:45 PM, Peter Rosin wrote: > > From 1e5621d7b9887c648d1a66238dc82d715c1e2cad Mon Sep 17 00:00:00 > > 2001 > > From: Peter Rosin <peda@axentia.se> > > Date: Mon, 20 Oct 2014 14:38:04 +0200 > > Subject: [PATCH] ASoC: atmel_ssc_dai: Track playback and capture CMR > dividers > > separately. > > > > The CMR divider register is shared by playback and capture. The SSC > > driver therefore tries to enforce rules so that the needed register > > content do not conflict during simultaneous playback/capture. However, > > the implementation also prevents changing the register content in a > > half-duplex scenario, which is needed when changing sample rates. > > I am not fully get what you mean here, do you mean: > - when playback, first playback 48kHz, and then playback 8Khz, the 8Khz still > playback in 48kHz mode? > > or other things? I do not know exactly why the problem occurs, but it might be connected to the library (proprietary) we are using. It apparently uses the OSS interface and somehow it opens a stream and changes the playback sample-rate a couple of time before it settles on something. I don't know why this is done, and I have no control over it. The end result is that without this patch, the ssc-dai driver returns -EBUSY when the library changes the playback sample-rate (the driver returns -EBUSY because it thinks that the new sample rate does not match a previously given capture sample-rate, but that is of course bogus when no capture is going on at all). > > Thus, keep track of the desired playback and capture clock dividers > > separately, and allow changing rates without closing the stream. > > > > Signed-off-by: Peter Rosin <peda@axentia.se> > > --- > > sound/soc/atmel/atmel_ssc_dai.c | 31 ++++++++++++++++++++++------ > --- > > sound/soc/atmel/atmel_ssc_dai.h | 10 ++++++---- > > 2 files changed, 28 insertions(+), 13 deletions(-) > > > > diff --git a/sound/soc/atmel/atmel_ssc_dai.c > > b/sound/soc/atmel/atmel_ssc_dai.c index f403f39..fec14fb 100644 > > --- a/sound/soc/atmel/atmel_ssc_dai.c > > +++ b/sound/soc/atmel/atmel_ssc_dai.c > > @@ -277,7 +277,8 @@ static void atmel_ssc_shutdown(struct > snd_pcm_substream *substream, > > /* Reset the SSC */ > > ssc_writel(ssc_p->ssc->regs, CR, SSC_BIT(CR_SWRST)); > > /* Clear the SSC dividers */ > > - ssc_p->cmr_div = ssc_p->tcmr_period = ssc_p->rcmr_period > = 0; > > + ssc_p->tcmr_div = ssc_p->rcmr_div = 0; > > + ssc_p->tcmr_period = ssc_p->rcmr_period = 0; > > } > > spin_unlock_irq(&ssc_p->lock); > > } > > @@ -304,17 +305,27 @@ static int atmel_ssc_set_dai_clkdiv(struct > snd_soc_dai *cpu_dai, > > struct atmel_ssc_info *ssc_p = &ssc_info[cpu_dai->id]; > > > > switch (div_id) { > > - case ATMEL_SSC_CMR_DIV: > > + case ATMEL_SSC_TCMR_DIV: > > /* > > * The same master clock divider is used for both > > * transmit and receive, so if a value has already > > - * been set, it must match this value. > > + * been set for the other direction, it must match > > + * this value. > > */ > > - if (ssc_p->cmr_div == 0) > > - ssc_p->cmr_div = div; > > - else > > - if (div != ssc_p->cmr_div) > > - return -EBUSY; > > + if (ssc_p->rcmr_div == 0) > > + ssc_p->tcmr_div = div; > > + else if (div != ssc_p->rcmr_div) > > + return -EBUSY; > > + break; > > + > > + case ATMEL_SSC_RCMR_DIV: > > + /* > > + * See ATMEL_SSC_TCMR_DIV. > > + */ > > + if (ssc_p->tcmr_div == 0) > > + ssc_p->rcmr_div = div; > > + else if (div != ssc_p->tcmr_div) > > + return -EBUSY; > > break; > > > > case ATMEL_SSC_TCMR_PERIOD: > > @@ -345,6 +356,7 @@ static int atmel_ssc_hw_params(struct > snd_pcm_substream *substream, > > struct atmel_pcm_dma_params *dma_params; > > int dir, channels, bits; > > u32 tfmr, rfmr, tcmr, rcmr; > > + u16 cmr; > > should be u32. Ok, I'll send an updated patch later. > > int start_event; > > int ret; > > int fslen, fslen_ext; > > @@ -626,7 +638,8 @@ static int atmel_ssc_hw_params(struct > snd_pcm_substream *substream, > > } > > > > /* set SSC clock mode register */ > > - ssc_writel(ssc_p->ssc->regs, CMR, ssc_p->cmr_div); > > + cmr = ssc_p->tcmr_div ? ssc_p->tcmr_div : ssc_p->rcmr_div; > > + ssc_writel(ssc_p->ssc->regs, CMR, cmr); > > > > /* set receive clock mode and format */ > > ssc_writel(ssc_p->ssc->regs, RCMR, rcmr); diff --git > > a/sound/soc/atmel/atmel_ssc_dai.h b/sound/soc/atmel/atmel_ssc_dai.h > > index b1f08d5..a25df7a 100644 > > --- a/sound/soc/atmel/atmel_ssc_dai.h > > +++ b/sound/soc/atmel/atmel_ssc_dai.h > > @@ -39,9 +39,10 @@ > > #define ATMEL_SYSCLK_MCK 0 /* SSC uses AT91 MCK as system > clock */ > > > > /* SSC divider ids */ > > -#define ATMEL_SSC_CMR_DIV 0 /* MCK divider for BCLK */ > > -#define ATMEL_SSC_TCMR_PERIOD 1 /* BCLK divider for transmit FS */ > > -#define ATMEL_SSC_RCMR_PERIOD 2 /* BCLK divider for receive FS */ > > +#define ATMEL_SSC_TCMR_DIV 0 /* MCK divider for transmit BCLK */ > > +#define ATMEL_SSC_RCMR_DIV 1 /* MCK divider for receive BCLK */ > > +#define ATMEL_SSC_TCMR_PERIOD 2 /* BCLK divider for transmit > FS */ > > +#define ATMEL_SSC_RCMR_PERIOD 3 /* BCLK divider for receive > FS */ > > /* > > * SSC direction masks > > */ > > @@ -110,7 +111,8 @@ struct atmel_ssc_info { > > unsigned short dir_mask; /* 0=unused, 1=playback, 2=capture > */ > > unsigned short initialized; /* true if SSC has been initialized */ > > unsigned short daifmt; > > - unsigned short cmr_div; > > + unsigned short tcmr_div; > > + unsigned short rcmr_div; > > unsigned short tcmr_period; > > unsigned short rcmr_period; > > struct atmel_pcm_dma_params *dma_params[2]; > > > > Best Regards, > Bo Shen Cheers, Peter
Hi Peter, On 10/21/2014 03:55 PM, Peter Rosin wrote: > Hi! > > (and thank you for the pointer to the example with the ssc-dai in master mode) > >> Hi Peter, >> >> On 10/20/2014 09:45 PM, Peter Rosin wrote: >>> From 1e5621d7b9887c648d1a66238dc82d715c1e2cad Mon Sep 17 00:00:00 >>> 2001 >>> From: Peter Rosin <peda@axentia.se> >>> Date: Mon, 20 Oct 2014 14:38:04 +0200 >>> Subject: [PATCH] ASoC: atmel_ssc_dai: Track playback and capture CMR >> dividers >>> separately. >>> >>> The CMR divider register is shared by playback and capture. The SSC >>> driver therefore tries to enforce rules so that the needed register >>> content do not conflict during simultaneous playback/capture. However, >>> the implementation also prevents changing the register content in a >>> half-duplex scenario, which is needed when changing sample rates. >> >> I am not fully get what you mean here, do you mean: >> - when playback, first playback 48kHz, and then playback 8Khz, the 8Khz still >> playback in 48kHz mode? >> >> or other things? > > I do not know exactly why the problem occurs, but it might be connected to > the library (proprietary) we are using. It apparently uses the OSS interface > and somehow it opens a stream and changes the playback sample-rate a > couple of time before it settles on something. I don't know why this is > done, and I have no control over it. The end result is that without this patch, > the ssc-dai driver returns -EBUSY when the library changes the playback > sample-rate (the driver returns -EBUSY because it thinks that the new > sample rate does not match a previously given capture sample-rate, but > that is of course bogus when no capture is going on at all). If this issue is caused by your proprietary library, please fix in the library. I don't know how this code can fix your issue and also there is no improvement to the code and it absolutely increase work (choose which clock to configure: tcmr or rcmr) for the SSC work in master. And also this patch may confuse the end user, let them thinking the clock for tcmr and rcmr can configure seperately. So, I think keep the code as is (without this patch applied), what's your opinion? >>> Thus, keep track of the desired playback and capture clock dividers >>> separately, and allow changing rates without closing the stream. >>> >>> Signed-off-by: Peter Rosin <peda@axentia.se> >>> --- >>> sound/soc/atmel/atmel_ssc_dai.c | 31 ++++++++++++++++++++++------ >> --- >>> sound/soc/atmel/atmel_ssc_dai.h | 10 ++++++---- >>> 2 files changed, 28 insertions(+), 13 deletions(-) >>> >>> diff --git a/sound/soc/atmel/atmel_ssc_dai.c >>> b/sound/soc/atmel/atmel_ssc_dai.c index f403f39..fec14fb 100644 >>> --- a/sound/soc/atmel/atmel_ssc_dai.c >>> +++ b/sound/soc/atmel/atmel_ssc_dai.c >>> @@ -277,7 +277,8 @@ static void atmel_ssc_shutdown(struct >> snd_pcm_substream *substream, >>> /* Reset the SSC */ >>> ssc_writel(ssc_p->ssc->regs, CR, SSC_BIT(CR_SWRST)); >>> /* Clear the SSC dividers */ >>> - ssc_p->cmr_div = ssc_p->tcmr_period = ssc_p->rcmr_period >> = 0; >>> + ssc_p->tcmr_div = ssc_p->rcmr_div = 0; >>> + ssc_p->tcmr_period = ssc_p->rcmr_period = 0; >>> } >>> spin_unlock_irq(&ssc_p->lock); >>> } >>> @@ -304,17 +305,27 @@ static int atmel_ssc_set_dai_clkdiv(struct >> snd_soc_dai *cpu_dai, >>> struct atmel_ssc_info *ssc_p = &ssc_info[cpu_dai->id]; >>> >>> switch (div_id) { >>> - case ATMEL_SSC_CMR_DIV: >>> + case ATMEL_SSC_TCMR_DIV: >>> /* >>> * The same master clock divider is used for both >>> * transmit and receive, so if a value has already >>> - * been set, it must match this value. >>> + * been set for the other direction, it must match >>> + * this value. >>> */ >>> - if (ssc_p->cmr_div == 0) >>> - ssc_p->cmr_div = div; >>> - else >>> - if (div != ssc_p->cmr_div) >>> - return -EBUSY; >>> + if (ssc_p->rcmr_div == 0) >>> + ssc_p->tcmr_div = div; >>> + else if (div != ssc_p->rcmr_div) >>> + return -EBUSY; >>> + break; >>> + >>> + case ATMEL_SSC_RCMR_DIV: >>> + /* >>> + * See ATMEL_SSC_TCMR_DIV. >>> + */ >>> + if (ssc_p->tcmr_div == 0) >>> + ssc_p->rcmr_div = div; >>> + else if (div != ssc_p->tcmr_div) >>> + return -EBUSY; >>> break; >>> >>> case ATMEL_SSC_TCMR_PERIOD: >>> @@ -345,6 +356,7 @@ static int atmel_ssc_hw_params(struct >> snd_pcm_substream *substream, >>> struct atmel_pcm_dma_params *dma_params; >>> int dir, channels, bits; >>> u32 tfmr, rfmr, tcmr, rcmr; >>> + u16 cmr; >> >> should be u32. > > Ok, I'll send an updated patch later. > >>> int start_event; >>> int ret; >>> int fslen, fslen_ext; >>> @@ -626,7 +638,8 @@ static int atmel_ssc_hw_params(struct >> snd_pcm_substream *substream, >>> } >>> >>> /* set SSC clock mode register */ >>> - ssc_writel(ssc_p->ssc->regs, CMR, ssc_p->cmr_div); >>> + cmr = ssc_p->tcmr_div ? ssc_p->tcmr_div : ssc_p->rcmr_div; >>> + ssc_writel(ssc_p->ssc->regs, CMR, cmr); >>> >>> /* set receive clock mode and format */ >>> ssc_writel(ssc_p->ssc->regs, RCMR, rcmr); diff --git >>> a/sound/soc/atmel/atmel_ssc_dai.h b/sound/soc/atmel/atmel_ssc_dai.h >>> index b1f08d5..a25df7a 100644 >>> --- a/sound/soc/atmel/atmel_ssc_dai.h >>> +++ b/sound/soc/atmel/atmel_ssc_dai.h >>> @@ -39,9 +39,10 @@ >>> #define ATMEL_SYSCLK_MCK 0 /* SSC uses AT91 MCK as system >> clock */ >>> >>> /* SSC divider ids */ >>> -#define ATMEL_SSC_CMR_DIV 0 /* MCK divider for BCLK */ >>> -#define ATMEL_SSC_TCMR_PERIOD 1 /* BCLK divider for transmit FS */ >>> -#define ATMEL_SSC_RCMR_PERIOD 2 /* BCLK divider for receive FS */ >>> +#define ATMEL_SSC_TCMR_DIV 0 /* MCK divider for transmit BCLK */ >>> +#define ATMEL_SSC_RCMR_DIV 1 /* MCK divider for receive BCLK */ >>> +#define ATMEL_SSC_TCMR_PERIOD 2 /* BCLK divider for transmit >> FS */ >>> +#define ATMEL_SSC_RCMR_PERIOD 3 /* BCLK divider for receive >> FS */ >>> /* >>> * SSC direction masks >>> */ >>> @@ -110,7 +111,8 @@ struct atmel_ssc_info { >>> unsigned short dir_mask; /* 0=unused, 1=playback, 2=capture >> */ >>> unsigned short initialized; /* true if SSC has been initialized */ >>> unsigned short daifmt; >>> - unsigned short cmr_div; >>> + unsigned short tcmr_div; >>> + unsigned short rcmr_div; >>> unsigned short tcmr_period; >>> unsigned short rcmr_period; >>> struct atmel_pcm_dma_params *dma_params[2]; >>> >> >> Best Regards, >> Bo Shen > > Cheers, > Peter > Best Regards, Bo Shen
Hi again! > Hi Peter, > > On 10/21/2014 03:55 PM, Peter Rosin wrote: > > Hi! > > > > (and thank you for the pointer to the example with the ssc-dai in > > master mode) > > > >> Hi Peter, > >> > >> On 10/20/2014 09:45 PM, Peter Rosin wrote: > >>> From 1e5621d7b9887c648d1a66238dc82d715c1e2cad Mon Sep 17 > 00:00:00 > >>> 2001 > >>> From: Peter Rosin <peda@axentia.se> > >>> Date: Mon, 20 Oct 2014 14:38:04 +0200 > >>> Subject: [PATCH] ASoC: atmel_ssc_dai: Track playback and capture CMR > >> dividers > >>> separately. > >>> > >>> The CMR divider register is shared by playback and capture. The SSC > >>> driver therefore tries to enforce rules so that the needed register > >>> content do not conflict during simultaneous playback/capture. > >>> However, the implementation also prevents changing the register > >>> content in a half-duplex scenario, which is needed when changing > sample rates. > >> > >> I am not fully get what you mean here, do you mean: > >> - when playback, first playback 48kHz, and then playback 8Khz, > >> the 8Khz still playback in 48kHz mode? > >> > >> or other things? > > > > I do not know exactly why the problem occurs, but it might be > > connected to the library (proprietary) we are using. It apparently > > uses the OSS interface and somehow it opens a stream and changes the > > playback sample-rate a couple of time before it settles on something. > > I don't know why this is done, and I have no control over it. The end > > result is that without this patch, the ssc-dai driver returns -EBUSY > > when the library changes the playback sample-rate (the driver returns > > -EBUSY because it thinks that the new sample rate does not match a > > previously given capture sample-rate, but that is of course bogus when no > capture is going on at all). > > If this issue is caused by your proprietary library, please fix in the library. Ah, but it's not "our" proprietary library to fix, it's simply a library that we are using (speech synthesis, not our area of expertise). So, I'm not in a position to simply fix it, as I have no source code. > I don't know how this code can fix your issue and also there is no > improvement to the code and it absolutely increase work (choose which > clock to configure: tcmr or rcmr) for the SSC work in master. And also this > patch may confuse the end user, let them thinking the clock for tcmr and > rcmr can configure seperately. I added some traces to our hw_params and got the following (without the patch): [ 161.170000] atmel ssc startup [ 161.170000] TFA9879: axentia_asoc_tfa9879_hw_params - mclk rate 66000000 [ 161.180000] TFA9879: axentia_asoc_tfa9879_hw_params - bclk rate 128000 [ 161.180000] TFA9879: axentia_asoc_tfa9879_hw_params - bclk divider 128 [ 161.190000] TFA9879: axentia_asoc_tfa9879_hw_params - period 7 [ 161.200000] TFA9879: axentia_asoc_tfa9879_hw_params - mclk rate 66000000 [ 161.200000] TFA9879: axentia_asoc_tfa9879_hw_params - bclk rate 128000 [ 161.210000] TFA9879: axentia_asoc_tfa9879_hw_params - bclk divider 128 [ 161.220000] TFA9879: axentia_asoc_tfa9879_hw_params - period 7 [ 161.230000] TFA9879: axentia_asoc_tfa9879_hw_params - mclk rate 66000000 [ 161.230000] TFA9879: axentia_asoc_tfa9879_hw_params - bclk rate 256000 [ 161.240000] TFA9879: axentia_asoc_tfa9879_hw_params - bclk divider 64 [ 161.250000] TFA9879: axentia_asoc_tfa9879_hw_params - Failed to set cpu dai clk divider [ 161.260000] axentia-tfa9879-audio sound.9: ASoC: machine hw_params failed: -16 > So, I think keep the code as is (without this patch applied), what's your > opinion? In that case we'll simply carry the patch ourselves. I don't know why the above happens, or if it is caused by bad behavior in the library, or what. But I do know that the library is unusable for us without some action. I thought it was an improvement, and therefore sent the patch... Cheers, Peter
Hi again, > -----Original Message----- > From: Peter Rosin > Sent: Tuesday, October 21, 2014 13:05 > To: 'Bo Shen' > Cc: Liam Girdwood; Mark Brown; Jaroslav Kysela; Takashi Iwai; 'alsa- > devel@alsa-project.org'; linux-kernel@vger.kernel.org > Subject: RE: [alsa-devel] [PATCH] ASoC: atmel_ssc_dai: Track playback and > capture CMR dividers separately. > > Hi again! > > > Hi Peter, > > > > On 10/21/2014 03:55 PM, Peter Rosin wrote: > > > Hi! > > > > > > (and thank you for the pointer to the example with the ssc-dai in > > > master mode) > > > > > >> Hi Peter, > > >> > > >> On 10/20/2014 09:45 PM, Peter Rosin wrote: > > >>> From 1e5621d7b9887c648d1a66238dc82d715c1e2cad Mon Sep 17 > > 00:00:00 > > >>> 2001 > > >>> From: Peter Rosin <peda@axentia.se> > > >>> Date: Mon, 20 Oct 2014 14:38:04 +0200 > > >>> Subject: [PATCH] ASoC: atmel_ssc_dai: Track playback and capture > CMR > > >> dividers > > >>> separately. > > >>> > > >>> The CMR divider register is shared by playback and capture. The SSC > > >>> driver therefore tries to enforce rules so that the needed register > > >>> content do not conflict during simultaneous playback/capture. > > >>> However, the implementation also prevents changing the register > > >>> content in a half-duplex scenario, which is needed when changing > > sample rates. > > >> > > >> I am not fully get what you mean here, do you mean: > > >> - when playback, first playback 48kHz, and then playback 8Khz, > > >> the 8Khz still playback in 48kHz mode? > > >> > > >> or other things? > > > > > > I do not know exactly why the problem occurs, but it might be > > > connected to the library (proprietary) we are using. It apparently > > > uses the OSS interface and somehow it opens a stream and changes the > > > playback sample-rate a couple of time before it settles on something. > > > I don't know why this is done, and I have no control over it. The end > > > result is that without this patch, the ssc-dai driver returns -EBUSY > > > when the library changes the playback sample-rate (the driver returns > > > -EBUSY because it thinks that the new sample rate does not match a > > > previously given capture sample-rate, but that is of course bogus when > no > > capture is going on at all). > > > > If this issue is caused by your proprietary library, please fix in the library. > > Ah, but it's not "our" proprietary library to fix, it's simply a library that we > are using (speech synthesis, not our area of expertise). So, I'm not in a > position to simply fix it, as I have no source code. > > > I don't know how this code can fix your issue and also there is no > > improvement to the code and it absolutely increase work (choose which > > clock to configure: tcmr or rcmr) for the SSC work in master. And also this > > patch may confuse the end user, let them thinking the clock for tcmr and > > rcmr can configure seperately. > > I added some traces to our hw_params and got the following (without the > patch): > > [ 161.170000] atmel ssc startup > [ 161.170000] TFA9879: axentia_asoc_tfa9879_hw_params - mclk rate > 66000000 > [ 161.180000] TFA9879: axentia_asoc_tfa9879_hw_params - bclk rate 128000 > [ 161.180000] TFA9879: axentia_asoc_tfa9879_hw_params - bclk divider 128 > [ 161.190000] TFA9879: axentia_asoc_tfa9879_hw_params - period 7 > [ 161.200000] TFA9879: axentia_asoc_tfa9879_hw_params - mclk rate > 66000000 > [ 161.200000] TFA9879: axentia_asoc_tfa9879_hw_params - bclk rate 128000 > [ 161.210000] TFA9879: axentia_asoc_tfa9879_hw_params - bclk divider 128 > [ 161.220000] TFA9879: axentia_asoc_tfa9879_hw_params - period 7 > [ 161.230000] TFA9879: axentia_asoc_tfa9879_hw_params - mclk rate > 66000000 > [ 161.230000] TFA9879: axentia_asoc_tfa9879_hw_params - bclk rate 256000 > [ 161.240000] TFA9879: axentia_asoc_tfa9879_hw_params - bclk divider 64 > [ 161.250000] TFA9879: axentia_asoc_tfa9879_hw_params - Failed to set cpu > dai clk divider > [ 161.260000] axentia-tfa9879-audio sound.9: ASoC: machine hw_params > failed: -16 I did some further tests, and the following program fails without the patch: #include <sys/ioctl.h> #include <unistd.h> #include <fcntl.h> #include <sys/soundcard.h> int main(void) { int fd; int format; int channels; if ((fd = open("/dev/dsp", O_WRONLY, 0)) == -1) { perror("open"); return 1; } format = AFMT_S16_LE; if (ioctl(fd, SNDCTL_DSP_SETFMT, &format) == -1) { perror("SNDCTL_DSP_SETFMT"); return 1; } channels = 2; if (ioctl(fd, SNDCTL_DSP_CHANNELS, &channels) == -1) { perror("SNDCTL_DSP_CHANNELS"); return 1; } return 0; } Output: SNDCTL_DSP_CHANNELS: Device or resource busy (I admin to having edited the above code slightly in this mail, so I might have introduced some silly bug, but you get what I mean, just open the device and request some parameters, and boom: -EBUSY) Cheers, Peter > > So, I think keep the code as is (without this patch applied), what's your > > opinion? > > In that case we'll simply carry the patch ourselves. I don't know why the > above > happens, or if it is caused by bad behavior in the library, or what. But I do > know > that the library is unusable for us without some action. > > I thought it was an improvement, and therefore sent the patch... > > Cheers, > Peter
Hi Peter, On 10/21/2014 09:05 PM, Peter Rosin wrote: > I did some further tests, and the following program fails without the patch: With the patch, it is OK? > #include <sys/ioctl.h> > #include <unistd.h> > #include <fcntl.h> > #include <sys/soundcard.h> > > int > main(void) > { > int fd; > int format; > int channels; > > if ((fd = open("/dev/dsp", O_WRONLY, 0)) == -1) { > perror("open"); > return 1; > } > format = AFMT_S16_LE; > if (ioctl(fd, SNDCTL_DSP_SETFMT, &format) == -1) { > perror("SNDCTL_DSP_SETFMT"); > return 1; > } > channels = 2; > if (ioctl(fd, SNDCTL_DSP_CHANNELS, &channels) == -1) { > perror("SNDCTL_DSP_CHANNELS"); > return 1; > } > return 0; > } > > Output: > SNDCTL_DSP_CHANNELS: Device or resource busy This return from codec or from atmel_ssc_dai? > (I admin to having edited the above code slightly in this mail, so I might have > introduced some silly bug, but you get what I mean, just open the device > and request some parameters, and boom: -EBUSY) > > Cheers, > Peter > Best Regards, Bo Shen
Hi! > Hi Peter, > > On 10/21/2014 09:05 PM, Peter Rosin wrote: > > I did some further tests, and the following program fails without the patch: > > With the patch, it is OK? Yes. > > #include <sys/ioctl.h> > > #include <unistd.h> > > #include <fcntl.h> > > #include <sys/soundcard.h> > > > > int > > main(void) > > { > > int fd; > > int format; > > int channels; > > > > if ((fd = open("/dev/dsp", O_WRONLY, 0)) == -1) { > > perror("open"); > > return 1; > > } > > format = AFMT_S16_LE; > > if (ioctl(fd, SNDCTL_DSP_SETFMT, &format) == -1) { > > perror("SNDCTL_DSP_SETFMT"); > > return 1; > > } > > channels = 2; > > if (ioctl(fd, SNDCTL_DSP_CHANNELS, &channels) == -1) { > > perror("SNDCTL_DSP_CHANNELS"); > > return 1; > > } > > return 0; > > } > > > > Output: > > SNDCTL_DSP_CHANNELS: Device or resource busy > > This return from codec or from atmel_ssc_dai? This -EBUSY definitely comes from atmel_ssc_set_dai_sysclk, when my card-driver tries to set ATMEL_SSC_CMR_DIV. With the patch, it works. (the codec is spdif-transmitter, since the i2c interface of the actual tfa9879 codec is not directly reachable from the linux cpu, but that has nothing to do with this issue). > > (I admin to having edited the above code slightly in this mail, so I s/admin/admit/ > > might have introduced some silly bug, but you get what I mean, just > > open the device and request some parameters, and boom: -EBUSY) Cheers, Peter
Hi Peter, On 10/22/2014 12:47 PM, Peter Rosin wrote: > Hi! > >> Hi Peter, >> >> On 10/21/2014 09:05 PM, Peter Rosin wrote: >>> I did some further tests, and the following program fails without the patch: >> >> With the patch, it is OK? > > Yes. > >>> #include <sys/ioctl.h> >>> #include <unistd.h> >>> #include <fcntl.h> >>> #include <sys/soundcard.h> >>> >>> int >>> main(void) >>> { >>> int fd; >>> int format; >>> int channels; >>> >>> if ((fd = open("/dev/dsp", O_WRONLY, 0)) == -1) { >>> perror("open"); >>> return 1; >>> } >>> format = AFMT_S16_LE; >>> if (ioctl(fd, SNDCTL_DSP_SETFMT, &format) == -1) { >>> perror("SNDCTL_DSP_SETFMT"); >>> return 1; >>> } >>> channels = 2; >>> if (ioctl(fd, SNDCTL_DSP_CHANNELS, &channels) == -1) { >>> perror("SNDCTL_DSP_CHANNELS"); >>> return 1; >>> } >>> return 0; >>> } >>> >>> Output: >>> SNDCTL_DSP_CHANNELS: Device or resource busy >> >> This return from codec or from atmel_ssc_dai? > > This -EBUSY definitely comes from atmel_ssc_set_dai_sysclk, when my > card-driver tries to set ATMEL_SSC_CMR_DIV. With the patch, it works. > (the codec is spdif-transmitter, since the i2c interface of the actual tfa9879 > codec is not directly reachable from the linux cpu, but that has nothing to > do with this issue). I try to reproduce it (using the code your pasted directly) on atmel sama5d3xek with wm8904 code, don't meet this error. I also go through the OSS code, I still don't find this is related with atmel_ssc_set_dai_sysclk. So, am I missing something or something else? >>> (I admin to having edited the above code slightly in this mail, so I > s/admin/admit/ >>> might have introduced some silly bug, but you get what I mean, just >>> open the device and request some parameters, and boom: -EBUSY) > > Cheers, > Peter > Best Regards, Bo Shen
Hi! > >> With the patch, it is OK? > > > > Yes. > > > >>> #include <sys/ioctl.h> > >>> #include <unistd.h> > >>> #include <fcntl.h> > >>> #include <sys/soundcard.h> > >>> > >>> int > >>> main(void) > >>> { > >>> int fd; > >>> int format; > >>> int channels; > >>> > >>> if ((fd = open("/dev/dsp", O_WRONLY, 0)) == -1) { > >>> perror("open"); > >>> return 1; > >>> } > >>> format = AFMT_S16_LE; > >>> if (ioctl(fd, SNDCTL_DSP_SETFMT, &format) == -1) { > >>> perror("SNDCTL_DSP_SETFMT"); > >>> return 1; > >>> } > >>> channels = 2; > >>> if (ioctl(fd, SNDCTL_DSP_CHANNELS, &channels) == -1) { > >>> perror("SNDCTL_DSP_CHANNELS"); > >>> return 1; > >>> } > >>> return 0; > >>> } > >>> > >>> Output: > >>> SNDCTL_DSP_CHANNELS: Device or resource busy > >> > >> This return from codec or from atmel_ssc_dai? > > > > This -EBUSY definitely comes from atmel_ssc_set_dai_sysclk, when my > > card-driver tries to set ATMEL_SSC_CMR_DIV. With the patch, it works. > > (the codec is spdif-transmitter, since the i2c interface of the actual > > tfa9879 codec is not directly reachable from the linux cpu, but that > > has nothing to do with this issue). > > I try to reproduce it (using the code your pasted directly) on atmel > sama5d3xek with wm8904 code, don't meet this error. > > I also go through the OSS code, I still don't find this is related with > atmel_ssc_set_dai_sysclk. > > So, am I missing something or something else? The sama5d3xek/wm9804 combo, as implemented in the kernel, has the ssc dai in slave mode, and therefore don't need to fiddle with any ssc dai dividers (atmel_9804.c :atmel_asoc_wm9804_hw_params() only sets things in the wm9804 codec dai driver and leaves the ssc dai to itself). Instead, try the above code on your code with the ssc dai in master mode that you pointed at previously. https://github.com/Android4SAM/linux-at91/commit/33db8ebd3e75632c482dda271340f4d2adcfd320 If that happens to not hit -EBUSY (which it might not, since the wm9804 codec will only allow stereo, so the SNDCTL_DSP_CHANNELS ioctl might not need to make any change for any ssc divider) add code to also set a non-default sample rate, e.g.: speed = 22050; if (ioctl(fd, SNDCTL_DSP_SPEED, &speed) == -1) { perror("SNDCTL_DSP_SPEED"); return 1; } Cheers, Peter
Hi Peter, On 10/22/2014 04:33 PM, Peter Rosin wrote: > The sama5d3xek/wm9804 combo, as implemented in the kernel, has the > ssc dai in slave mode, and therefore don't need to fiddle with any > ssc dai dividers (atmel_9804.c :atmel_asoc_wm9804_hw_params() only > sets things in the wm9804 codec dai driver and leaves the ssc dai to itself). > > Instead, try the above code on your code with the ssc dai in master mode > that you pointed at previously. Yes, I try the ssc working as master mode. > https://github.com/Android4SAM/linux-at91/commit/33db8ebd3e75632c482dda271340f4d2adcfd320 > > If that happens to not hit -EBUSY (which it might not, since the wm9804 codec > will only allow stereo, so the SNDCTL_DSP_CHANNELS ioctl might not need > to make any change for any ssc divider) add code to also set a non-default > sample rate, e.g.: > > speed = 22050; > if (ioctl(fd, SNDCTL_DSP_SPEED, &speed) == -1) { > perror("SNDCTL_DSP_SPEED"); > return 1; > } with this piece of code, I reproduce your issue. Now, I know the reason of this issue, work in oss mode, it will set the default clock to 8KHz, and then if change to other sample rate, for example 48KHz, the div is different, then it reports -EBUSY. So, I think we won't change the ATMEL_SSC_CMR_DIV to ATMEL_SSC_TCMR_DIV and ATMEL_SSC_RCMR_DIV, as it will affect other users. We just deal with this situation in ATMEL_SSC_CMR_DIV block, check the direction, if the same direction change the div, then accept the change, otherwise, return -EBUSY. > > Cheers, > Peter Best Regards, Bo Shen
Bo Chen wrote: > with this piece of code, I reproduce your issue. > > Now, I know the reason of this issue, work in oss mode, it will set the default > clock to 8KHz, and then if change to other sample rate, for example 48KHz, > the div is different, then it reports -EBUSY. Indeed. > So, I think we won't change the ATMEL_SSC_CMR_DIV to > ATMEL_SSC_TCMR_DIV and ATMEL_SSC_RCMR_DIV, as it will affect other > users. We just deal with this situation in ATMEL_SSC_CMR_DIV block, check > the direction, if the same direction change the div, then accept the change, > otherwise, return -EBUSY. Ok. But I'm not sure it is possible to dig out the current direction in the .set_clkdiv callback? Perhaps the correct fix is to set the bits .symmetric_rates, .symmetric_channels and .symmetric_samplebits in the atmel_ssc_dai struct when the ssc dai is master? Then I expect that other mechanisms will kick in that will render the current CMR_DIV check pointless? Is a dai driver allowed to change these symmetry bits after registration? Can they be set in the .set_sysclk callback? Perhaps in the ATMEL_SSC_CMR_DIV block itself? That callback should only be called when the dai is master, so that would be perfect... Yes, the limitation would be a little bit more strict than today, but is it really common to require different modes on playback and capture? Cheers, Peter
diff --git a/sound/soc/atmel/atmel_ssc_dai.c b/sound/soc/atmel/atmel_ssc_dai.c index f403f39..fec14fb 100644 --- a/sound/soc/atmel/atmel_ssc_dai.c +++ b/sound/soc/atmel/atmel_ssc_dai.c @@ -277,7 +277,8 @@ static void atmel_ssc_shutdown(struct snd_pcm_substream *substream, /* Reset the SSC */ ssc_writel(ssc_p->ssc->regs, CR, SSC_BIT(CR_SWRST)); /* Clear the SSC dividers */ - ssc_p->cmr_div = ssc_p->tcmr_period = ssc_p->rcmr_period = 0; + ssc_p->tcmr_div = ssc_p->rcmr_div = 0; + ssc_p->tcmr_period = ssc_p->rcmr_period = 0; } spin_unlock_irq(&ssc_p->lock); } @@ -304,17 +305,27 @@ static int atmel_ssc_set_dai_clkdiv(struct snd_soc_dai *cpu_dai, struct atmel_ssc_info *ssc_p = &ssc_info[cpu_dai->id]; switch (div_id) { - case ATMEL_SSC_CMR_DIV: + case ATMEL_SSC_TCMR_DIV: /* * The same master clock divider is used for both * transmit and receive, so if a value has already - * been set, it must match this value. + * been set for the other direction, it must match + * this value. */ - if (ssc_p->cmr_div == 0) - ssc_p->cmr_div = div; - else - if (div != ssc_p->cmr_div) - return -EBUSY; + if (ssc_p->rcmr_div == 0) + ssc_p->tcmr_div = div; + else if (div != ssc_p->rcmr_div) + return -EBUSY; + break; + + case ATMEL_SSC_RCMR_DIV: + /* + * See ATMEL_SSC_TCMR_DIV. + */ + if (ssc_p->tcmr_div == 0) + ssc_p->rcmr_div = div; + else if (div != ssc_p->tcmr_div) + return -EBUSY; break; case ATMEL_SSC_TCMR_PERIOD: @@ -345,6 +356,7 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream, struct atmel_pcm_dma_params *dma_params; int dir, channels, bits; u32 tfmr, rfmr, tcmr, rcmr; + u16 cmr; int start_event; int ret; int fslen, fslen_ext; @@ -626,7 +638,8 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream, } /* set SSC clock mode register */ - ssc_writel(ssc_p->ssc->regs, CMR, ssc_p->cmr_div); + cmr = ssc_p->tcmr_div ? ssc_p->tcmr_div : ssc_p->rcmr_div; + ssc_writel(ssc_p->ssc->regs, CMR, cmr); /* set receive clock mode and format */ ssc_writel(ssc_p->ssc->regs, RCMR, rcmr); diff --git a/sound/soc/atmel/atmel_ssc_dai.h b/sound/soc/atmel/atmel_ssc_dai.h index b1f08d5..a25df7a 100644 --- a/sound/soc/atmel/atmel_ssc_dai.h +++ b/sound/soc/atmel/atmel_ssc_dai.h @@ -39,9 +39,10 @@ #define ATMEL_SYSCLK_MCK 0 /* SSC uses AT91 MCK as system clock */ /* SSC divider ids */ -#define ATMEL_SSC_CMR_DIV 0 /* MCK divider for BCLK */ -#define ATMEL_SSC_TCMR_PERIOD 1 /* BCLK divider for transmit FS */ -#define ATMEL_SSC_RCMR_PERIOD 2 /* BCLK divider for receive FS */ +#define ATMEL_SSC_TCMR_DIV 0 /* MCK divider for transmit BCLK */ +#define ATMEL_SSC_RCMR_DIV 1 /* MCK divider for receive BCLK */ +#define ATMEL_SSC_TCMR_PERIOD 2 /* BCLK divider for transmit FS */ +#define ATMEL_SSC_RCMR_PERIOD 3 /* BCLK divider for receive FS */ /* * SSC direction masks */ @@ -110,7 +111,8 @@ struct atmel_ssc_info { unsigned short dir_mask; /* 0=unused, 1=playback, 2=capture */ unsigned short initialized; /* true if SSC has been initialized */ unsigned short daifmt; - unsigned short cmr_div; + unsigned short tcmr_div; + unsigned short rcmr_div; unsigned short tcmr_period; unsigned short rcmr_period; struct atmel_pcm_dma_params *dma_params[2];