[PATCH 0/2] ASoC: fsl_asrc: improve clock selection and quality

[PATCH 0/2] ASoC: fsl_asrc: improve clock selection and quality

[Arnaud Ferraris]

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(-)

[PATCH 1/2] ASoC: fsl_asrc: make sure the input and output clocks are different

[Arnaud Ferraris]

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 <arnaud.ferraris@collabora.com>
---
 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

Re: [PATCH 1/2] ASoC: fsl_asrc: make sure the input and output clocks are different

[Arnaud Ferraris]

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 <arnaud.ferraris@collabora.com>
> ---
>  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

[PATCH 2/2] ASoC: fsl_asrc: always use internal ratio

[Arnaud Ferraris]

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 <arnaud.ferraris@collabora.com>
---
 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

[PATCH v2 0/2] ASoC: fsl_asrc: improve clock selection and quality

[Arnaud Ferraris]

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(-)

[PATCH v2 1/2] ASoC: fsl_asrc: make sure the input and output clocks are different

[Arnaud Ferraris]

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 <arnaud.ferraris@collabora.com>
---
 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

Re: [PATCH v2 1/2] ASoC: fsl_asrc: make sure the input and output clocks are different

[Nicolin Chen]

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 <arnaud.ferraris@collabora.com>
> ---
>  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]

[PATCH v3 0/1] ASoC: fsl_asrc: always select different clocks

[Arnaud Ferraris]

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(-)

[PATCH v3 1/1] ASoC: fsl_asrc: make sure the input and output clocks are different

[Arnaud Ferraris]

The current clock selection algorithm might select the same clock for
both input and output when, for instance, the output sample rate is a
multiple of the input rate.

In that case, both selectable clocks will be multiples of both the input
and output sample rates, and therefore the first of these clocks will be
selected for both input and output, leading to miscalculation of the
dividers for either the input or output side.

Example:
  Input uses clock A (512kHz) and has a sample rate of 8kHz
  Output uses clock B (1024kHz) and has a sample rate of 16kHz

  In this case, the algorithm will select clock A for both input and
  output: the input divider will therefore be calculated properly
  (512 / 8 => 64), but the output divider's value will be only half
  the expected value (512 / 16 => 32 instead of 1024 / 16 => 64).

This patch makes sure it always selects distinct input and output
clocks.

Signed-off-by: Arnaud Ferraris <arnaud.ferraris@collabora.com>
---
 sound/soc/fsl/fsl_asrc.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index 02c81d2e34ad..de10c208d3c8 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,15 @@ 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]);
-			/* Only match a perfect clock source with no remainder */
+			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
+			 * and make sure the input & output clocks are different
+			 */
 			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

Re: [PATCH v3 1/1] ASoC: fsl_asrc: make sure the input and output clocks are different

[Nicolin Chen]

On Fri, Jul 17, 2020 at 12:38:58PM +0200, Arnaud Ferraris wrote:
> The current clock selection algorithm might select the same clock for
> both input and output when, for instance, the output sample rate is a
> multiple of the input rate.
> 
> In that case, both selectable clocks will be multiples of both the input
> and output sample rates, and therefore the first of these clocks will be
> selected for both input and output, leading to miscalculation of the
> dividers for either the input or output side.
> 
> Example:
>   Input uses clock A (512kHz) and has a sample rate of 8kHz
>   Output uses clock B (1024kHz) and has a sample rate of 16kHz
> 
>   In this case, the algorithm will select clock A for both input and
>   output: the input divider will therefore be calculated properly
>   (512 / 8 => 64), but the output divider's value will be only half
>   the expected value (512 / 16 => 32 instead of 1024 / 16 => 64).
> 
> This patch makes sure it always selects distinct input and output
> clocks.

Please allow me to take some time to review the use case and
the changes. And I'd love to wait for a review from Shengjiu
also, as he introduced this auto-selection function and he's
more familiar with internal ratio mode.

Thanks

> Signed-off-by: Arnaud Ferraris <arnaud.ferraris@collabora.com>
> ---
>  sound/soc/fsl/fsl_asrc.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
> index 02c81d2e34ad..de10c208d3c8 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,15 @@ 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]);
> -			/* Only match a perfect clock source with no remainder */
> +			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
> +			 * and make sure the input & output clocks are different
> +			 */
>  			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
> 

Re: [PATCH v3 1/1] ASoC: fsl_asrc: make sure the input and output clocks are different

[Shengjiu Wang]

On Fri, Jul 17, 2020 at 6:40 PM Arnaud Ferraris
<arnaud.ferraris@collabora.com> wrote:
>
> The current clock selection algorithm might select the same clock for
> both input and output when, for instance, the output sample rate is a
> multiple of the input rate.
>
> In that case, both selectable clocks will be multiples of both the input
> and output sample rates, and therefore the first of these clocks will be
> selected for both input and output, leading to miscalculation of the
> dividers for either the input or output side.
>
> Example:
>   Input uses clock A (512kHz) and has a sample rate of 8kHz
>   Output uses clock B (1024kHz) and has a sample rate of 16kHz
>
>   In this case, the algorithm will select clock A for both input and
>   output: the input divider will therefore be calculated properly
>   (512 / 8 => 64), but the output divider's value will be only half
>   the expected value (512 / 16 => 32 instead of 1024 / 16 => 64).
>
(input divider, output divider) = (64, 32) for the same clock source
(512kHz) looks no problem.  could you explain more detail why
(64, 32) can't work?


> This patch makes sure it always selects distinct input and output
> clocks.

There should be no such constraint for this IP, do you have any
evidence for we should use distinct input and output clocks?

>
> Signed-off-by: Arnaud Ferraris <arnaud.ferraris@collabora.com>
> ---
>  sound/soc/fsl/fsl_asrc.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
> index 02c81d2e34ad..de10c208d3c8 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,15 @@ 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]);
> -                       /* Only match a perfect clock source with no remainder */
> +                       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
> +                        * and make sure the input & output clocks are different
> +                        */
>                         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
>

Re: [PATCH v3 0/1] ASoC: fsl_asrc: always select different clocks

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 690 bytes --]

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. 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 0/1] ASoC: fsl_asrc: always select different clocks

[Arnaud Ferraris]

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

Re: [PATCH v3 0/1] ASoC: fsl_asrc: always select different clocks

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 204 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v2 2/2] ASoC: fsl_asrc: always use internal ratio

[Arnaud Ferraris]

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 <arnaud.ferraris@collabora.com>
---
 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

Re: [PATCH v2 2/2] ASoC: fsl_asrc: always use internal ratio

[Nicolin Chen]

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 <arnaud.ferraris@collabora.com>
> ---
>  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.

Re: [PATCH v2 2/2] ASoC: fsl_asrc: always use internal ratio

[Arnaud Ferraris]

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

Re: [PATCH v2 2/2] ASoC: fsl_asrc: always use internal ratio

[Shengjiu Wang]

On Fri, Jul 17, 2020 at 5:58 PM Arnaud Ferraris
<arnaud.ferraris@collabora.com> 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