The current ASRC driver hardcodes the input and output clocks used for
sample rate conversions. In order to allow greater flexibility and to
cover more use cases, it would be preferable to select the clocks using
device-tree properties.
This series also fix register configuration and clock assignment so
conversion can be conducted effectively in both directions with a good
quality.
Arnaud Ferraris (4):
dt-bindings: sound: fsl,asrc: add properties to select in/out clocks
ASoC: fsl_asrc: allow using arbitrary input and output clocks
ASoC: fsl_asrc: always use ratio for conversion
ASoC: fsl_asrc: swap input and output clocks in capture mode
Documentation/devicetree/bindings/sound/fsl,asrc.txt | 8 ++++++++
sound/soc/fsl/fsl_asrc.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
sound/soc/fsl/fsl_asrc_common.h | 3 +++
3 files changed, 75 insertions(+), 5 deletions(-)
fsl_asrc currently uses hardcoded input and output clocks, preventing
its use for anything other than S/PDIF output.
This patch adds the ability to select any clock as input or output (by
using new DT properties), making it possible to use this peripheral in a
more advanced way.
Signed-off-by: Arnaud Ferraris <[email protected]>
---
sound/soc/fsl/fsl_asrc.c | 18 ++++++++++++++++--
sound/soc/fsl/fsl_asrc_common.h | 3 +++
2 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index 95f6a9617b0b..75df220e4b51 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -605,8 +605,8 @@ static int fsl_asrc_dai_hw_params(struct snd_pcm_substream *substream,
config.pair = pair->index;
config.channel_num = channels;
- config.inclk = INCLK_NONE;
- config.outclk = OUTCLK_ASRCK1_CLK;
+ config.inclk = asrc->inclk;
+ config.outclk = asrc->outclk;
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
config.input_format = params_format(params);
@@ -1067,6 +1067,20 @@ static int fsl_asrc_probe(struct platform_device *pdev)
asrc->channel_avail = 10;
+ ret = of_property_read_u32(np, "fsl,asrc-input-clock",
+ &asrc->inclk);
+ if (ret) {
+ dev_info(&pdev->dev, "no input clock specified, using none\n");
+ asrc->inclk = INCLK_NONE;
+ }
+
+ ret = of_property_read_u32(np, "fsl,asrc-output-clock",
+ &asrc->outclk);
+ if (ret) {
+ dev_info(&pdev->dev, "no output clock specified, using default\n");
+ asrc->outclk = OUTCLK_ASRCK1_CLK;
+ }
+
ret = of_property_read_u32(np, "fsl,asrc-rate",
&asrc->asrc_rate);
if (ret) {
diff --git a/sound/soc/fsl/fsl_asrc_common.h b/sound/soc/fsl/fsl_asrc_common.h
index 7e1c13ca37f1..1468878fbaca 100644
--- a/sound/soc/fsl/fsl_asrc_common.h
+++ b/sound/soc/fsl/fsl_asrc_common.h
@@ -89,6 +89,9 @@ struct fsl_asrc {
struct fsl_asrc_pair *pair[PAIR_CTX_NUM];
unsigned int channel_avail;
+ enum asrc_inclk inclk;
+ enum asrc_outclk outclk;
+
int asrc_rate;
snd_pcm_format_t asrc_format;
bool use_edma;
--
2.27.0
Hi Arnaud,
On Thu, Jul 02, 2020 at 04:22:31PM +0200, Arnaud Ferraris wrote:
> The current ASRC driver hardcodes the input and output clocks used for
> sample rate conversions. In order to allow greater flexibility and to
> cover more use cases, it would be preferable to select the clocks using
> device-tree properties.
We recent just merged a new change that auto-selecting internal
clocks based on sample rates as the first option -- ideal ratio
mode is the fallback mode now. Please refer to:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20200702&id=d0250cf4f2abfbea64ed247230f08f5ae23979f0
Having a quick review at your changes, I think the DT part may
not be necessary as it's more likely a software configuration.
I personally like the new auto-selecting solution more.
> This series also fix register configuration and clock assignment so
> conversion can be conducted effectively in both directions with a good
> quality.
If there's any further change that you feel you can improve on
the top of mentioned change after rebasing, I'd like to review.
Thanks
Nic
Hi Nic,
Le 02/07/2020 à 20:42, Nicolin Chen a écrit :
> Hi Arnaud,
>
> On Thu, Jul 02, 2020 at 04:22:31PM +0200, Arnaud Ferraris wrote:
>> The current ASRC driver hardcodes the input and output clocks used for
>> sample rate conversions. In order to allow greater flexibility and to
>> cover more use cases, it would be preferable to select the clocks using
>> device-tree properties.
>
> We recent just merged a new change that auto-selecting internal
> clocks based on sample rates as the first option -- ideal ratio
> mode is the fallback mode now. Please refer to:
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20200702&id=d0250cf4f2abfbea64ed247230f08f5ae23979f0
That looks interesting, thanks for pointing this out!
I'll rebase and see how it works for my use-case, will keep you informed.
Regards,
Arnaud
Hi Nic,
Le 03/07/2020 à 11:38, Arnaud Ferraris a écrit :
> Hi Nic,
>
> Le 02/07/2020 à 20:42, Nicolin Chen a écrit :
>> Hi Arnaud,
>>
>> On Thu, Jul 02, 2020 at 04:22:31PM +0200, Arnaud Ferraris wrote:
>>> The current ASRC driver hardcodes the input and output clocks used for
>>> sample rate conversions. In order to allow greater flexibility and to
>>> cover more use cases, it would be preferable to select the clocks using
>>> device-tree properties.
>>
>> We recent just merged a new change that auto-selecting internal
>> clocks based on sample rates as the first option -- ideal ratio
>> mode is the fallback mode now. Please refer to:
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20200702&id=d0250cf4f2abfbea64ed247230f08f5ae23979f0
>
> That looks interesting, thanks for pointing this out!
> I'll rebase and see how it works for my use-case, will keep you informed.
>
> Regards,
> Arnaud
>
I finally got some time to test and debug clock auto-selection on my
system, and unfortunately couldn't get it to work.
Here's some background about my use case: the i.MX6 board acts as a
Bluetooth proxy between a phone and a headset. It has 2 Bluetooth
modules (one for each connected device), with audio connected to SSI1 &
SSI2. Audio sample rate can be either 8 or 16kHz, and bclk can be either
512 or 1024kHz, all depending of the capabilities of the headset and phone.
In our case we want SSI2 to be the input clock to the ASRC and SSI1 the
output clock, but there is no way to force that with auto-selection:
both clocks are multiples of both 8k and 16k, so the algorithm will
always select the SSI1 clock.
I don't think auto-selection can be made smart enough to cover this
case, which is why I believe we still need a way to manually setup the
input and output clocks to be used by ASRC, falling back to
auto-selecting the clocks if not setup manually.
If not using DT bindings, what do you think would be the best way to
implement that?
Regards,
Arnaud
On Tue, Jul 14, 2020 at 06:20:32PM +0200, Arnaud Ferraris wrote:
> >>> The current ASRC driver hardcodes the input and output clocks used for
> >>> sample rate conversions. In order to allow greater flexibility and to
> >>> cover more use cases, it would be preferable to select the clocks using
> >>> device-tree properties.
> >>
> >> We recent just merged a new change that auto-selecting internal
> >> clocks based on sample rates as the first option -- ideal ratio
> >> mode is the fallback mode now. Please refer to:
> >> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20200702&id=d0250cf4f2abfbea64ed247230f08f5ae23979f0
> I finally got some time to test and debug clock auto-selection on my
> system, and unfortunately couldn't get it to work.
>
> Here's some background about my use case: the i.MX6 board acts as a
> Bluetooth proxy between a phone and a headset. It has 2 Bluetooth
> modules (one for each connected device), with audio connected to SSI1 &
> SSI2. Audio sample rate can be either 8 or 16kHz, and bclk can be either
> 512 or 1024kHz, all depending of the capabilities of the headset and phone.
> In our case we want SSI2 to be the input clock to the ASRC and SSI1 the
> output clock, but there is no way to force that with auto-selection:
> both clocks are multiples of both 8k and 16k, so the algorithm will
> always select the SSI1 clock.
Anything wrong with ASRC selecting SSI1 clock for both cases? The
driver calculates the divisors based on the given clock rate, so
the final internal rate should be the same. If there's a problem,
I feel that's a separate bug.
On Tue, Jul 14, 2020 at 01:15:45PM -0700, Nicolin Chen wrote:
> On Tue, Jul 14, 2020 at 06:20:32PM +0200, Arnaud Ferraris wrote:
> > Here's some background about my use case: the i.MX6 board acts as a
> > Bluetooth proxy between a phone and a headset. It has 2 Bluetooth
> > modules (one for each connected device), with audio connected to SSI1 &
> > SSI2. Audio sample rate can be either 8 or 16kHz, and bclk can be either
> > 512 or 1024kHz, all depending of the capabilities of the headset and phone.
> > In our case we want SSI2 to be the input clock to the ASRC and SSI1 the
> > output clock, but there is no way to force that with auto-selection:
> > both clocks are multiples of both 8k and 16k, so the algorithm will
> > always select the SSI1 clock.
> Anything wrong with ASRC selecting SSI1 clock for both cases? The
> driver calculates the divisors based on the given clock rate, so
> the final internal rate should be the same. If there's a problem,
> I feel that's a separate bug.
The nominal rate might be the same but if they're in different clock
domains then the actual rates might be different (hence the desire for
an ASRC I guess). I can see the system wanting to choose one clock or
the other on the basis of some system specific property (quality of the
clock sources, tolerances of the devices involved or something) though
it's a rather fun edge case configuration :/ .
Hi Mark,
On Tue, Jul 14, 2020 at 09:27:53PM +0100, Mark Brown wrote:
> On Tue, Jul 14, 2020 at 01:15:45PM -0700, Nicolin Chen wrote:
> > On Tue, Jul 14, 2020 at 06:20:32PM +0200, Arnaud Ferraris wrote:
>
> > > Here's some background about my use case: the i.MX6 board acts as a
> > > Bluetooth proxy between a phone and a headset. It has 2 Bluetooth
> > > modules (one for each connected device), with audio connected to SSI1 &
> > > SSI2. Audio sample rate can be either 8 or 16kHz, and bclk can be either
> > > 512 or 1024kHz, all depending of the capabilities of the headset and phone.
> > > In our case we want SSI2 to be the input clock to the ASRC and SSI1 the
> > > output clock, but there is no way to force that with auto-selection:
> > > both clocks are multiples of both 8k and 16k, so the algorithm will
> > > always select the SSI1 clock.
>
> > Anything wrong with ASRC selecting SSI1 clock for both cases? The
> > driver calculates the divisors based on the given clock rate, so
> > the final internal rate should be the same. If there's a problem,
> > I feel that's a separate bug.
>
> The nominal rate might be the same but if they're in different clock
> domains then the actual rates might be different (hence the desire for
> an ASRC I guess). I can see the system wanting to choose one clock or
> the other on the basis of some system specific property (quality of the
> clock sources, tolerances of the devices involved or something) though
> it's a rather fun edge case configuration :/ .
Thanks for the input. Fox i.MX6, I don't feel it would be that
drastically different though. And both SSI1 and SSI2 can simply
select the same root clock source to avoid that happen.
Yet, in case that we need to support such an edge case, what's
a relatively common practice to allow system select the clock
source now?
On Tue, Jul 14, 2020 at 01:50:50PM -0700, Nicolin Chen wrote:
> On Tue, Jul 14, 2020 at 09:27:53PM +0100, Mark Brown wrote:
> > The nominal rate might be the same but if they're in different clock
> > domains then the actual rates might be different (hence the desire for
> > an ASRC I guess). I can see the system wanting to choose one clock or
> > the other on the basis of some system specific property (quality of the
> > clock sources, tolerances of the devices involved or something) though
> > it's a rather fun edge case configuration :/ .
> Thanks for the input. Fox i.MX6, I don't feel it would be that
> drastically different though. And both SSI1 and SSI2 can simply
> select the same root clock source to avoid that happen.
If you've got two radios that both need to sync to some radio derived
frequency it gets a bit more entertaining.
> Yet, in case that we need to support such an edge case, what's
> a relatively common practice to allow system select the clock
> source now?
Honestly for anything that fun it tends to be a custom machine driver.
A property would seem reasonable though.
Hi,
Le 15/07/2020 ? 16:05, Mark Brown a ?crit?:
> On Tue, Jul 14, 2020 at 01:50:50PM -0700, Nicolin Chen wrote:
> Anything wrong with ASRC selecting SSI1 clock for both cases? The
> driver calculates the divisors based on the given clock rate, so
> the final internal rate should be the same. If there's a problem,
> I feel that's a separate bug.
Calculations are indeed good, but then the clock selection setting in
the ASRCSR register would also use SSI1 as the input clock, which
doesn't work in our case.
>> On Tue, Jul 14, 2020 at 09:27:53PM +0100, Mark Brown wrote:
>
> If you've got two radios that both need to sync to some radio derived
> frequency it gets a bit more entertaining.
It was indeed a fun ride trying to make it work for all the use-cases we
were targetting.
>
>> Yet, in case that we need to support such an edge case, what's
>> a relatively common practice to allow system select the clock
>> source now?
>
> Honestly for anything that fun it tends to be a custom machine driver.
> A property would seem reasonable though.
I think so, does my initial implementation of the properties look
sensible to you? ("fsl,asrc-input-clock" & "fsl,asrc-output-clock")
Regards,
Arnaud
On Wed, Jul 15, 2020 at 06:18:38PM +0200, Arnaud Ferraris wrote:
> Le 15/07/2020 ? 16:05, Mark Brown a ?crit?:
> > Honestly for anything that fun it tends to be a custom machine driver.
> > A property would seem reasonable though.
> I think so, does my initial implementation of the properties look
> sensible to you? ("fsl,asrc-input-clock" & "fsl,asrc-output-clock")
Those look reasonable enough off the top of my head, not that I've
really looked at the hardware.
Le 15/07/2020 ? 18:22, Mark Brown a ?crit?:
>
> Those look reasonable enough off the top of my head, not that I've
> really looked at the hardware.
>
Alright, thanks for your feedback! Unless Nic has other comments I'll
submit a v2 tomorrow then.
Regards,
Arnaud
On Wed, Jul 15, 2020 at 06:18:38PM +0200, Arnaud Ferraris wrote:
> Hi,
>
> Le 15/07/2020 ? 16:05, Mark Brown a ?crit?:
> > On Tue, Jul 14, 2020 at 01:50:50PM -0700, Nicolin Chen wrote:
> > Anything wrong with ASRC selecting SSI1 clock for both cases? The
> > driver calculates the divisors based on the given clock rate, so
> > the final internal rate should be the same. If there's a problem,
> > I feel that's a separate bug.
>
> Calculations are indeed good, but then the clock selection setting in
> the ASRCSR register would also use SSI1 as the input clock, which
> doesn't work in our case.
Could you please elaborate why it doesn't work?
On Wed, Jul 15, 2020 at 03:05:19PM +0100, Mark Brown wrote:
> On Tue, Jul 14, 2020 at 01:50:50PM -0700, Nicolin Chen wrote:
> > On Tue, Jul 14, 2020 at 09:27:53PM +0100, Mark Brown wrote:
>
> > > The nominal rate might be the same but if they're in different clock
> > > domains then the actual rates might be different (hence the desire for
> > > an ASRC I guess). I can see the system wanting to choose one clock or
> > > the other on the basis of some system specific property (quality of the
> > > clock sources, tolerances of the devices involved or something) though
> > > it's a rather fun edge case configuration :/ .
>
> > Thanks for the input. Fox i.MX6, I don't feel it would be that
> > drastically different though. And both SSI1 and SSI2 can simply
> > select the same root clock source to avoid that happen.
>
> If you've got two radios that both need to sync to some radio derived
> frequency it gets a bit more entertaining.
I'm simply curious what could be a problem. Do you mind educating
me a bit? And ASRC here isn't a radio but a sample rate converter
working as a BE in DPCM setup, using radio-capture for example...
Le 15/07/2020 à 22:46, Nicolin Chen a écrit :
>> Calculations are indeed good, but then the clock selection setting in
>> the ASRCSR register would also use SSI1 as the input clock, which
>> doesn't work in our case.
>
> Could you please elaborate why it doesn't work?
>
Hmm, actually it was a userspace issue, kernel-wise it could work fine
with SSI1 as input clock, so please ignore this latest comment.
Regards,
Arnaud
On Wed, Jul 15, 2020 at 02:03:08PM -0700, Nicolin Chen wrote:
> On Wed, Jul 15, 2020 at 03:05:19PM +0100, Mark Brown wrote:
> > On Tue, Jul 14, 2020 at 01:50:50PM -0700, Nicolin Chen wrote:
> > > Thanks for the input. Fox i.MX6, I don't feel it would be that
> > > drastically different though. And both SSI1 and SSI2 can simply
> > > select the same root clock source to avoid that happen.
> > If you've got two radios that both need to sync to some radio derived
> > frequency it gets a bit more entertaining.
> I'm simply curious what could be a problem. Do you mind educating
> me a bit? And ASRC here isn't a radio but a sample rate converter
> working as a BE in DPCM setup, using radio-capture for example...
My understanding was that this application was using the ASRC to convert
between the sample rates of two different radios - the rates may be
nominaly the same but in practice different so the audio will glitch
after a while when the clocks drift far enough apart.
Le 16/07/2020 ? 14:18, Mark Brown a ?crit?:
> On Wed, Jul 15, 2020 at 02:03:08PM -0700, Nicolin Chen wrote:
>> On Wed, Jul 15, 2020 at 03:05:19PM +0100, Mark Brown wrote:
>>> On Tue, Jul 14, 2020 at 01:50:50PM -0700, Nicolin Chen wrote:
>
>>>> Thanks for the input. Fox i.MX6, I don't feel it would be that
>>>> drastically different though. And both SSI1 and SSI2 can simply
>>>> select the same root clock source to avoid that happen.
>
>>> If you've got two radios that both need to sync to some radio derived
>>> frequency it gets a bit more entertaining.
>
>> I'm simply curious what could be a problem. Do you mind educating
>> me a bit? And ASRC here isn't a radio but a sample rate converter
>> working as a BE in DPCM setup, using radio-capture for example...
>
> My understanding was that this application was using the ASRC to convert
> between the sample rates of two different radios - the rates may be
> nominaly the same but in practice different so the audio will glitch
> after a while when the clocks drift far enough apart.
That's part of the issues we had to solve, yes. The other part is more
traditional sample rate conversion on an as-needed basis, as we can't
assume which rate will be used (iPhone's use 16kHz, Android phones stick
to 8kHz, and headsets can use both depending on their capabilities).
Hi Nic,
Le 02/07/2020 à 20:42, Nicolin Chen a écrit :
> Hi Arnaud,
>
> On Thu, Jul 02, 2020 at 04:22:31PM +0200, Arnaud Ferraris wrote:
>> The current ASRC driver hardcodes the input and output clocks used for
>> sample rate conversions. In order to allow greater flexibility and to
>> cover more use cases, it would be preferable to select the clocks using
>> device-tree properties.
>
> We recent just merged a new change that auto-selecting internal
> clocks based on sample rates as the first option -- ideal ratio
> mode is the fallback mode now. Please refer to:
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20200702&id=d0250cf4f2abfbea64ed247230f08f5ae23979f0
While working on fixing the automatic clock selection (see my v3), I
came across another potential issue, which would be better explained
with an example:
- Input has sample rate 8kHz and uses clock SSI1 with rate 512kHz
- Output has sample rate 16kHz and uses clock SSI2 with rate 1024kHz
Let's say my v3 patch is merged, then the selected input clock will be
SSI1, while the selected output clock will be SSI2. In that case, it's
all good, as the driver will calculate the dividers right.
Now, suppose a similar board has the input wired to SSI2 and output to
SSI1, meaning we're now in the following case:
- Input has sample rate 8kHz and uses clock SSI2 with rate 512kHz
- Output has sample rate 16kHz and uses clock SSI1 with rate 1024kHz
(the same result is achieved during capture with the initial example
setup, as input and output properties are then swapped)
In that case, the selected clocks will still be SSI1 for input (just
because it appears first in the clock table), and SSI2 for output,
meaning the calculated dividers will be:
- input: 512 / 16 => 32 (should be 64)
- output: 1024 / 8 => 128 (should be 64 here too)
---
I can't see how the clock selection algorithm could be made smart enough
to cover cases such as this one, as it would need to be aware of the
exact relationship between the sample rate and the clock rate (my
example demonstrates a case where the "sample rate to clock rate"
multiplier is identical for both input and output, but this can't be
assumed to be always the case).
Therefore, I still believe being able to force clock selection using
optional DT properties would make sense, while still using the
auto-selection by default.
Regards,
Arnaud
>
> Having a quick review at your changes, I think the DT part may
> not be necessary as it's more likely a software configuration.
> I personally like the new auto-selecting solution more.
>
>> This series also fix register configuration and clock assignment so
>> conversion can be conducted effectively in both directions with a good
>> quality.
>
> If there's any further change that you feel you can improve on
> the top of mentioned change after rebasing, I'd like to review.
>
> Thanks
> Nic
>
On Fri, Jul 17, 2020 at 01:16:42PM +0200, Arnaud Ferraris wrote:
> Hi Nic,
>
> Le 02/07/2020 ? 20:42, Nicolin Chen a ?crit?:
> > Hi Arnaud,
> >
> > On Thu, Jul 02, 2020 at 04:22:31PM +0200, Arnaud Ferraris wrote:
> >> The current ASRC driver hardcodes the input and output clocks used for
> >> sample rate conversions. In order to allow greater flexibility and to
> >> cover more use cases, it would be preferable to select the clocks using
> >> device-tree properties.
> >
> > We recent just merged a new change that auto-selecting internal
> > clocks based on sample rates as the first option -- ideal ratio
> > mode is the fallback mode now. Please refer to:
> > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20200702&id=d0250cf4f2abfbea64ed247230f08f5ae23979f0
>
> While working on fixing the automatic clock selection (see my v3), I
> came across another potential issue, which would be better explained
> with an example:
> - Input has sample rate 8kHz and uses clock SSI1 with rate 512kHz
> - Output has sample rate 16kHz and uses clock SSI2 with rate 1024kHz
>
> Let's say my v3 patch is merged, then the selected input clock will be
> SSI1, while the selected output clock will be SSI2. In that case, it's
> all good, as the driver will calculate the dividers right.
>
> Now, suppose a similar board has the input wired to SSI2 and output to
> SSI1, meaning we're now in the following case:
> - Input has sample rate 8kHz and uses clock SSI2 with rate 512kHz
> - Output has sample rate 16kHz and uses clock SSI1 with rate 1024kHz
> (the same result is achieved during capture with the initial example
> setup, as input and output properties are then swapped)
>
> In that case, the selected clocks will still be SSI1 for input (just
> because it appears first in the clock table), and SSI2 for output,
> meaning the calculated dividers will be:
> - input: 512 / 16 => 32 (should be 64)
> - output: 1024 / 8 => 128 (should be 64 here too)
I don't get the 32, 128 and 64 parts. Would you please to elaborate
a bit? What you said sounds to me like the driver calculates wrong
dividers?