[v3,1/1] ASoC: fsl_asrc: make sure the input and output clocks are different
diff mbox series

Message ID 20200717103857.31877-2-arnaud.ferraris@collabora.com
State New
Headers show
Series
  • ASoC: fsl_asrc: always select different clocks
Related show

Commit Message

Arnaud Ferraris July 17, 2020, 10:38 a.m. UTC
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(-)

Comments

Nicolin Chen July 17, 2020, 11:05 p.m. UTC | #1
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
>
Shengjiu Wang July 20, 2020, 10:34 a.m. UTC | #2
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
>

Patch
diff mbox series

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;
 		}