This series fixes the automatic clock selection and enables internal
ratio in order to improve audio quality.
The clock selection patches have been set aside for now, as the
discussion is still ongoing regarding that matter.
Arnaud Ferraris(2):
ASoC: fsl_asrc: make sure the input and output clocks are different
ASoC: fsl_asrc: always use internal ratio
sound/soc/fsl/fsl_asrc.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
Even though the current driver calculates the dividers to be used
depending on the clocks and sample rates, enabling the internal ratio
can lead to noticeable improvements in the audio quality, based on my
testing.
As stated in the documentation, "When USRx=1 and IDRx=0, ASRC internal
measured ratio will be used", so setting this bit even when not in
"Ideal Ratio" mode still makes sense.
Signed-off-by: Arnaud Ferraris <[email protected]>
---
sound/soc/fsl/fsl_asrc.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index bfd35b9c0781..cc0f70c9140f 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -465,7 +465,7 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair, bool use_ideal_rate)
regmap_update_bits(asrc->regmap, REG_ASRCTR,
ASRCTR_ATSi_MASK(index), ASRCTR_ATS(index));
regmap_update_bits(asrc->regmap, REG_ASRCTR,
- ASRCTR_USRi_MASK(index), 0);
+ ASRCTR_USRi_MASK(index), ASRCTR_USR(index));
/* Set the input and output clock sources */
regmap_update_bits(asrc->regmap, REG_ASRCSR,
@@ -507,8 +507,7 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair, bool use_ideal_rate)
/* Enable Ideal Ratio mode */
regmap_update_bits(asrc->regmap, REG_ASRCTR,
- ASRCTR_IDRi_MASK(index) | ASRCTR_USRi_MASK(index),
- ASRCTR_IDR(index) | ASRCTR_USR(index));
+ ASRCTR_IDRi_MASK(index), ASRCTR_IDR(index);
fsl_asrc_sel_proc(inrate, outrate, &pre_proc, &post_proc);
--
2.27.0
The current clock selection algorithm might select the same clock for
both input and output. This can happen when, for instance, the output
sample rate is a multiple of the input rate.
This patch makes sure it always selects distinct input and output
clocks.
Signed-off-by: Arnaud Ferraris <[email protected]>
---
sound/soc/fsl/fsl_asrc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index 02c81d2e34ad..bfd35b9c0781 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -622,7 +622,8 @@ static void fsl_asrc_select_clk(struct fsl_asrc_priv *asrc_priv,
clk_rate = clk_get_rate(asrc_priv->asrck_clk[clk_index]);
/* Only match a perfect clock source with no remainder */
if (clk_rate != 0 && (clk_rate / rate[j]) <= 1024 &&
- (clk_rate % rate[j]) == 0)
+ (clk_rate % rate[j]) == 0 &&
+ (j == 0 || i != select_clk[j-1]))
break;
}
--
2.27.0
Le 16/07/2020 à 16:52, Arnaud Ferraris a écrit :
> The current clock selection algorithm might select the same clock for
> both input and output. This can happen when, for instance, the output
> sample rate is a multiple of the input rate.
>
> This patch makes sure it always selects distinct input and output
> clocks.
>
> Signed-off-by: Arnaud Ferraris <[email protected]>
> ---
> sound/soc/fsl/fsl_asrc.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
> index 02c81d2e34ad..bfd35b9c0781 100644
> --- a/sound/soc/fsl/fsl_asrc.c
> +++ b/sound/soc/fsl/fsl_asrc.c
> @@ -622,7 +622,8 @@ static void fsl_asrc_select_clk(struct fsl_asrc_priv *asrc_priv,
> clk_rate = clk_get_rate(asrc_priv->asrck_clk[clk_index]);
> /* Only match a perfect clock source with no remainder */
> if (clk_rate != 0 && (clk_rate / rate[j]) <= 1024 &&
> - (clk_rate % rate[j]) == 0)
> + (clk_rate % rate[j]) == 0 &&
> + (j == 0 || i != select_clk[j-1]))
> break;
> }
>
>
Well, it looks like I sent the wrong patch for this one, will send a v2
fixing this right now.
Sorry about the noise.
Regards,
Arnaud
This series fixes the automatic clock selection and enables internal
ratio in order to improve audio quality.
The clock selection patches have been set aside for now, as the
discussion is still ongoing regarding that matter.
v1 -> v2:
- compare clock indexes (and not the location in the clock table) to
make sure input and output clocks are different
Arnaud Ferraris(2):
ASoC: fsl_asrc: make sure the input and output clocks are different
ASoC: fsl_asrc: always use internal ratio
sound/soc/fsl/fsl_asrc.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
The current clock selection algorithm might select the same clock for
both input and output. This can happen when, for instance, the output
sample rate is a multiple of the input rate.
This patch makes sure it always selects distinct input and output
clocks.
Signed-off-by: Arnaud Ferraris <[email protected]>
---
sound/soc/fsl/fsl_asrc.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index 02c81d2e34ad..6d43cab6c885 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -608,8 +608,8 @@ static void fsl_asrc_select_clk(struct fsl_asrc_priv *asrc_priv,
{
struct fsl_asrc_pair_priv *pair_priv = pair->private;
struct asrc_config *config = pair_priv->config;
- int rate[2], select_clk[2]; /* Array size 2 means IN and OUT */
- int clk_rate, clk_index;
+ int rate[2], select_clk[2], clk_index[2]; /* Array size 2 means IN and OUT */
+ int clk_rate;
int i = 0, j = 0;
rate[IN] = in_rate;
@@ -618,11 +618,12 @@ static void fsl_asrc_select_clk(struct fsl_asrc_priv *asrc_priv,
/* Select proper clock source for internal ratio mode */
for (j = 0; j < 2; j++) {
for (i = 0; i < ASRC_CLK_MAP_LEN; i++) {
- clk_index = asrc_priv->clk_map[j][i];
- clk_rate = clk_get_rate(asrc_priv->asrck_clk[clk_index]);
+ clk_index[j] = asrc_priv->clk_map[j][i];
+ clk_rate = clk_get_rate(asrc_priv->asrck_clk[clk_index[j]]);
/* Only match a perfect clock source with no remainder */
if (clk_rate != 0 && (clk_rate / rate[j]) <= 1024 &&
- (clk_rate % rate[j]) == 0)
+ (clk_rate % rate[j]) == 0 &&
+ (j == 0 || clk_index[j] != clk_index[j-1]))
break;
}
--
2.27.0
Even though the current driver calculates the dividers to be used
depending on the clocks and sample rates, enabling the internal ratio
can lead to noticeable improvements in the audio quality, based on my
testing.
As stated in the documentation, "When USRx=1 and IDRx=0, ASRC internal
measured ratio will be used", so setting this bit even when not in
"Ideal Ratio" mode still makes sense.
Signed-off-by: Arnaud Ferraris <[email protected]>
---
sound/soc/fsl/fsl_asrc.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index 6d43cab6c885..0b79a02d0d76 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -465,7 +465,7 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair, bool use_ideal_rate)
regmap_update_bits(asrc->regmap, REG_ASRCTR,
ASRCTR_ATSi_MASK(index), ASRCTR_ATS(index));
regmap_update_bits(asrc->regmap, REG_ASRCTR,
- ASRCTR_USRi_MASK(index), 0);
+ ASRCTR_USRi_MASK(index), ASRCTR_USR(index));
/* Set the input and output clock sources */
regmap_update_bits(asrc->regmap, REG_ASRCSR,
@@ -507,8 +507,7 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair, bool use_ideal_rate)
/* Enable Ideal Ratio mode */
regmap_update_bits(asrc->regmap, REG_ASRCTR,
- ASRCTR_IDRi_MASK(index) | ASRCTR_USRi_MASK(index),
- ASRCTR_IDR(index) | ASRCTR_USR(index));
+ ASRCTR_IDRi_MASK(index), ASRCTR_IDR(index);
fsl_asrc_sel_proc(inrate, outrate, &pre_proc, &post_proc);
--
2.27.0
On Thu, Jul 16, 2020 at 05:13:52PM +0200, Arnaud Ferraris wrote:
> The current clock selection algorithm might select the same clock for
> both input and output. This can happen when, for instance, the output
> sample rate is a multiple of the input rate.
What's the issue when selecting the same clock source for both
input and output? Please explain it in the commit logs.
> This patch makes sure it always selects distinct input and output
> clocks.
>
> Signed-off-by: Arnaud Ferraris <[email protected]>
> ---
> sound/soc/fsl/fsl_asrc.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
> index 02c81d2e34ad..6d43cab6c885 100644
> --- a/sound/soc/fsl/fsl_asrc.c
> +++ b/sound/soc/fsl/fsl_asrc.c
> @@ -608,8 +608,8 @@ static void fsl_asrc_select_clk(struct fsl_asrc_priv *asrc_priv,
> {
> struct fsl_asrc_pair_priv *pair_priv = pair->private;
> struct asrc_config *config = pair_priv->config;
> - int rate[2], select_clk[2]; /* Array size 2 means IN and OUT */
> - int clk_rate, clk_index;
> + int rate[2], select_clk[2], clk_index[2]; /* Array size 2 means IN and OUT */
> + int clk_rate;
> int i = 0, j = 0;
>
> rate[IN] = in_rate;
> @@ -618,11 +618,12 @@ static void fsl_asrc_select_clk(struct fsl_asrc_priv *asrc_priv,
> /* Select proper clock source for internal ratio mode */
> for (j = 0; j < 2; j++) {
> for (i = 0; i < ASRC_CLK_MAP_LEN; i++) {
> - clk_index = asrc_priv->clk_map[j][i];
> - clk_rate = clk_get_rate(asrc_priv->asrck_clk[clk_index]);
> + clk_index[j] = asrc_priv->clk_map[j][i];
> + clk_rate = clk_get_rate(asrc_priv->asrck_clk[clk_index[j]]);
> /* Only match a perfect clock source with no remainder */
Better to update the comments here as there's a new condition.
> if (clk_rate != 0 && (clk_rate / rate[j]) <= 1024 &&
> - (clk_rate % rate[j]) == 0)
> + (clk_rate % rate[j]) == 0 &&
> + (j == 0 || clk_index[j] != clk_index[j-1]))
clk_index[j - 1]
On Thu, Jul 16, 2020 at 05:13:54PM +0200, Arnaud Ferraris wrote:
> Even though the current driver calculates the dividers to be used
> depending on the clocks and sample rates, enabling the internal ratio
> can lead to noticeable improvements in the audio quality, based on my
> testing.
>
> As stated in the documentation, "When USRx=1 and IDRx=0, ASRC internal
> measured ratio will be used", so setting this bit even when not in
> "Ideal Ratio" mode still makes sense.
>
> Signed-off-by: Arnaud Ferraris <[email protected]>
> ---
> sound/soc/fsl/fsl_asrc.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
> index 6d43cab6c885..0b79a02d0d76 100644
> --- a/sound/soc/fsl/fsl_asrc.c
> +++ b/sound/soc/fsl/fsl_asrc.c
> @@ -465,7 +465,7 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair, bool use_ideal_rate)
> regmap_update_bits(asrc->regmap, REG_ASRCTR,
> ASRCTR_ATSi_MASK(index), ASRCTR_ATS(index));
> regmap_update_bits(asrc->regmap, REG_ASRCTR,
> - ASRCTR_USRi_MASK(index), 0);
> + ASRCTR_USRi_MASK(index), ASRCTR_USR(index));
>
> /* Set the input and output clock sources */
> regmap_update_bits(asrc->regmap, REG_ASRCSR,
> @@ -507,8 +507,7 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair, bool use_ideal_rate)
>
> /* Enable Ideal Ratio mode */
The code is against the comments now -- need to update this line.
> regmap_update_bits(asrc->regmap, REG_ASRCTR,
> - ASRCTR_IDRi_MASK(index) | ASRCTR_USRi_MASK(index),
> - ASRCTR_IDR(index) | ASRCTR_USR(index));
> + ASRCTR_IDRi_MASK(index), ASRCTR_IDR(index);
The driver falls back to ideal ratio mode if there is no matched
clock source. Your change seems to apply internal ratio mode any
way? Probably would break the fallback routine.
Le 17/07/2020 à 01:37, Nicolin Chen a écrit :
>> @@ -507,8 +507,7 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair, bool use_ideal_rate)
>>
>> /* Enable Ideal Ratio mode */
>
> The code is against the comments now -- need to update this line.
It isn't, the following code still enables "Ideal Ratio" mode (see below)
>> regmap_update_bits(asrc->regmap, REG_ASRCTR,
>> - ASRCTR_IDRi_MASK(index) | ASRCTR_USRi_MASK(index),
>> - ASRCTR_IDR(index) | ASRCTR_USR(index));
>> + ASRCTR_IDRi_MASK(index), ASRCTR_IDR(index);
>
> The driver falls back to ideal ratio mode if there is no matched
> clock source. Your change seems to apply internal ratio mode any
> way? Probably would break the fallback routine.
Strictly speaking, internal ratio is only enabled when we have matched
clock sources, and is used in addition to the calculated dividers
(allows the ASRC to better adjust to drifting/inaccurate physical
clocks). "Ideal Ratio" mode is different, and still enabled as a
fallback when no clock source is matched.
Ideal ratio requires both USRi and IDRi bits to be set, and that would
still be the case if there is no matched clock source.
The only difference my patch introduces is that USRi is always set (was
previously cleared for "normal" mode), and therefore only IDRi needs to
be set in order to enable ideal ratio mode.
Regards,
Arnaud
This patch fixes the automatic clock selection so it always selects
distinct input and output clocks.
v2 -> v3:
- Update code comment, fix formatting and add more detailed explanations
in commit message
v1 -> v2:
- compare clock indexes (and not the location in the clock table) to
make sure input and output clocks are different
Arnaud Ferraris(1):
ASoC: fsl_asrc: make sure the input and output clocks are different
sound/soc/fsl/fsl_asrc.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
On Fri, Jul 17, 2020 at 12:38:56PM +0200, Arnaud Ferraris wrote:
> This patch fixes the automatic clock selection so it always selects
> distinct input and output clocks.
Please don't send new patches in reply to old ones, it buries things and
makes it hard to keep track of what the current version of a series
looks like. Just send new versions as a completely new thread.
Please don't send cover letters for single patches, if there is anything
that needs saying put it in the changelog of the patch or after the ---
if it's administrative stuff. This reduces mail volume and ensures that
any important information is recorded in the changelog rather than being
lost.
Le 17/07/2020 ? 13:21, Mark Brown a ?crit?:
> On Fri, Jul 17, 2020 at 12:38:56PM +0200, Arnaud Ferraris wrote:
>> This patch fixes the automatic clock selection so it always selects
>> distinct input and output clocks.
>
> Please don't send new patches in reply to old ones, it buries things and
> makes it hard to keep track of what the current version of a series
> looks like. Just send new versions as a completely new thread.
>
> Please don't send cover letters for single patches, if there is anything
> that needs saying put it in the changelog of the patch or after the ---
> if it's administrative stuff. This reduces mail volume and ensures that
> any important information is recorded in the changelog rather than being
> lost.
>
Understood, sorry about that. Should I do a "clean" re-send for this one?
Regards,
Arnaud
On Fri, Jul 17, 2020 at 01:34:34PM +0200, Arnaud Ferraris wrote:
> Understood, sorry about that. Should I do a "clean" re-send for this one?
It's fine, please just remember this for future submissions.
On Fri, Jul 17, 2020 at 5:58 PM Arnaud Ferraris
<[email protected]> wrote:
>
>
>
> Le 17/07/2020 à 01:37, Nicolin Chen a écrit :
> >> @@ -507,8 +507,7 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair, bool use_ideal_rate)
> >>
> >> /* Enable Ideal Ratio mode */
> >
> > The code is against the comments now -- need to update this line.
>
> It isn't, the following code still enables "Ideal Ratio" mode (see below)
>
> >> regmap_update_bits(asrc->regmap, REG_ASRCTR,
> >> - ASRCTR_IDRi_MASK(index) | ASRCTR_USRi_MASK(index),
> >> - ASRCTR_IDR(index) | ASRCTR_USR(index));
> >> + ASRCTR_IDRi_MASK(index), ASRCTR_IDR(index);
> >
> > The driver falls back to ideal ratio mode if there is no matched
> > clock source. Your change seems to apply internal ratio mode any
> > way? Probably would break the fallback routine.
>
> Strictly speaking, internal ratio is only enabled when we have matched
> clock sources, and is used in addition to the calculated dividers
> (allows the ASRC to better adjust to drifting/inaccurate physical
> clocks). "Ideal Ratio" mode is different, and still enabled as a
> fallback when no clock source is matched.
>
> Ideal ratio requires both USRi and IDRi bits to be set, and that would
> still be the case if there is no matched clock source.
>
> The only difference my patch introduces is that USRi is always set (was
> previously cleared for "normal" mode), and therefore only IDRi needs to
> be set in order to enable ideal ratio mode.
>
In my experience, the USRi = 0, no matter the value of IDRi, it is
internal ratio mode. USRi=1, IDRi=0, it is also internal ratio mode.
So original code should be ok for internal ratio mode, no need
to add this change.
could you please double check it?
best regards
wang shengjiu