* [PATCH 0/2] ASoC: fsl_asrc: improve clock selection and quality @ 2020-07-16 14:51 Arnaud Ferraris 2020-07-16 14:52 ` [PATCH 1/2] ASoC: fsl_asrc: make sure the input and output clocks are different Arnaud Ferraris ` (2 more replies) 0 siblings, 3 replies; 18+ messages in thread From: Arnaud Ferraris @ 2020-07-16 14:51 UTC (permalink / raw) To: alsa-devel Cc: Timur Tabi, Nicolin Chen, Xiubo Li, Fabio Estevam, Shengjiu Wang, Liam Girdwood, Mark Brown, linux-kernel, kernel 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(-) ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/2] ASoC: fsl_asrc: make sure the input and output clocks are different 2020-07-16 14:51 [PATCH 0/2] ASoC: fsl_asrc: improve clock selection and quality Arnaud Ferraris @ 2020-07-16 14:52 ` Arnaud Ferraris 2020-07-16 15:07 ` Arnaud Ferraris 2020-07-16 14:52 ` [PATCH 2/2] ASoC: fsl_asrc: always use internal ratio Arnaud Ferraris 2020-07-16 15:13 ` [PATCH v2 0/2] ASoC: fsl_asrc: improve clock selection and quality Arnaud Ferraris 2 siblings, 1 reply; 18+ messages in thread From: Arnaud Ferraris @ 2020-07-16 14:52 UTC (permalink / raw) To: alsa-devel Cc: Timur Tabi, Nicolin Chen, Xiubo Li, Fabio Estevam, Shengjiu Wang, Liam Girdwood, Mark Brown, linux-kernel, kernel, 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] ASoC: fsl_asrc: make sure the input and output clocks are different 2020-07-16 14:52 ` [PATCH 1/2] ASoC: fsl_asrc: make sure the input and output clocks are different Arnaud Ferraris @ 2020-07-16 15:07 ` Arnaud Ferraris 0 siblings, 0 replies; 18+ messages in thread From: Arnaud Ferraris @ 2020-07-16 15:07 UTC (permalink / raw) To: alsa-devel Cc: Timur Tabi, Nicolin Chen, Xiubo Li, Fabio Estevam, Shengjiu Wang, Liam Girdwood, Mark Brown, linux-kernel, kernel 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2] ASoC: fsl_asrc: always use internal ratio 2020-07-16 14:51 [PATCH 0/2] ASoC: fsl_asrc: improve clock selection and quality Arnaud Ferraris 2020-07-16 14:52 ` [PATCH 1/2] ASoC: fsl_asrc: make sure the input and output clocks are different Arnaud Ferraris @ 2020-07-16 14:52 ` Arnaud Ferraris 2020-07-16 15:13 ` [PATCH v2 0/2] ASoC: fsl_asrc: improve clock selection and quality Arnaud Ferraris 2 siblings, 0 replies; 18+ messages in thread From: Arnaud Ferraris @ 2020-07-16 14:52 UTC (permalink / raw) To: alsa-devel Cc: Timur Tabi, Nicolin Chen, Xiubo Li, Fabio Estevam, Shengjiu Wang, Liam Girdwood, Mark Brown, linux-kernel, kernel, 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 0/2] ASoC: fsl_asrc: improve clock selection and quality 2020-07-16 14:51 [PATCH 0/2] ASoC: fsl_asrc: improve clock selection and quality Arnaud Ferraris 2020-07-16 14:52 ` [PATCH 1/2] ASoC: fsl_asrc: make sure the input and output clocks are different Arnaud Ferraris 2020-07-16 14:52 ` [PATCH 2/2] ASoC: fsl_asrc: always use internal ratio Arnaud Ferraris @ 2020-07-16 15:13 ` Arnaud Ferraris 2020-07-16 15:13 ` [PATCH v2 1/2] ASoC: fsl_asrc: make sure the input and output clocks are different Arnaud Ferraris 2020-07-16 15:13 ` [PATCH v2 2/2] ASoC: fsl_asrc: always use internal ratio Arnaud Ferraris 2 siblings, 2 replies; 18+ messages in thread From: Arnaud Ferraris @ 2020-07-16 15:13 UTC (permalink / raw) To: alsa-devel Cc: Timur Tabi, Nicolin Chen, Xiubo Li, Fabio Estevam, Shengjiu Wang, Liam Girdwood, Mark Brown, linux-kernel, kernel 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(-) ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 1/2] ASoC: fsl_asrc: make sure the input and output clocks are different 2020-07-16 15:13 ` [PATCH v2 0/2] ASoC: fsl_asrc: improve clock selection and quality Arnaud Ferraris @ 2020-07-16 15:13 ` Arnaud Ferraris 2020-07-16 23:20 ` Nicolin Chen 2020-07-16 15:13 ` [PATCH v2 2/2] ASoC: fsl_asrc: always use internal ratio Arnaud Ferraris 1 sibling, 1 reply; 18+ messages in thread From: Arnaud Ferraris @ 2020-07-16 15:13 UTC (permalink / raw) To: alsa-devel Cc: Timur Tabi, Nicolin Chen, Xiubo Li, Fabio Estevam, Shengjiu Wang, Liam Girdwood, Mark Brown, linux-kernel, kernel, 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/2] ASoC: fsl_asrc: make sure the input and output clocks are different 2020-07-16 15:13 ` [PATCH v2 1/2] ASoC: fsl_asrc: make sure the input and output clocks are different Arnaud Ferraris @ 2020-07-16 23:20 ` Nicolin Chen 2020-07-17 10:38 ` [PATCH v3 0/1] ASoC: fsl_asrc: always select different clocks Arnaud Ferraris 0 siblings, 1 reply; 18+ messages in thread From: Nicolin Chen @ 2020-07-16 23:20 UTC (permalink / raw) To: Arnaud Ferraris Cc: alsa-devel, Timur Tabi, Xiubo Li, Fabio Estevam, Shengjiu Wang, Liam Girdwood, Mark Brown, linux-kernel, kernel 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] ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 0/1] ASoC: fsl_asrc: always select different clocks 2020-07-16 23:20 ` Nicolin Chen @ 2020-07-17 10:38 ` Arnaud Ferraris 2020-07-17 10:38 ` [PATCH v3 1/1] ASoC: fsl_asrc: make sure the input and output clocks are different Arnaud Ferraris 2020-07-17 11:21 ` [PATCH v3 0/1] ASoC: fsl_asrc: always select different clocks Mark Brown 0 siblings, 2 replies; 18+ messages in thread From: Arnaud Ferraris @ 2020-07-17 10:38 UTC (permalink / raw) To: alsa-devel Cc: Timur Tabi, Nicolin Chen, Xiubo Li, Fabio Estevam, Shengjiu Wang, Liam Girdwood, Mark Brown, linux-kernel, kernel 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(-) ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 1/1] ASoC: fsl_asrc: make sure the input and output clocks are different 2020-07-17 10:38 ` [PATCH v3 0/1] ASoC: fsl_asrc: always select different clocks Arnaud Ferraris @ 2020-07-17 10:38 ` Arnaud Ferraris 2020-07-17 23:05 ` Nicolin Chen 2020-07-20 10:34 ` Shengjiu Wang 2020-07-17 11:21 ` [PATCH v3 0/1] ASoC: fsl_asrc: always select different clocks Mark Brown 1 sibling, 2 replies; 18+ messages in thread From: Arnaud Ferraris @ 2020-07-17 10:38 UTC (permalink / raw) To: alsa-devel Cc: Timur Tabi, Nicolin Chen, Xiubo Li, Fabio Estevam, Shengjiu Wang, Liam Girdwood, Mark Brown, linux-kernel, kernel, 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/1] ASoC: fsl_asrc: make sure the input and output clocks are different 2020-07-17 10:38 ` [PATCH v3 1/1] ASoC: fsl_asrc: make sure the input and output clocks are different Arnaud Ferraris @ 2020-07-17 23:05 ` Nicolin Chen 2020-07-20 10:34 ` Shengjiu Wang 1 sibling, 0 replies; 18+ messages in thread From: Nicolin Chen @ 2020-07-17 23:05 UTC (permalink / raw) To: Arnaud Ferraris Cc: alsa-devel, Timur Tabi, Xiubo Li, Fabio Estevam, Shengjiu Wang, Liam Girdwood, Mark Brown, linux-kernel, kernel 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 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/1] ASoC: fsl_asrc: make sure the input and output clocks are different 2020-07-17 10:38 ` [PATCH v3 1/1] ASoC: fsl_asrc: make sure the input and output clocks are different Arnaud Ferraris 2020-07-17 23:05 ` Nicolin Chen @ 2020-07-20 10:34 ` Shengjiu Wang 1 sibling, 0 replies; 18+ messages in thread From: Shengjiu Wang @ 2020-07-20 10:34 UTC (permalink / raw) To: Arnaud Ferraris Cc: Linux-ALSA, Timur Tabi, Nicolin Chen, Xiubo Li, Fabio Estevam, Liam Girdwood, Mark Brown, linux-kernel, kernel 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 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 0/1] ASoC: fsl_asrc: always select different clocks 2020-07-17 10:38 ` [PATCH v3 0/1] ASoC: fsl_asrc: always select different clocks Arnaud Ferraris 2020-07-17 10:38 ` [PATCH v3 1/1] ASoC: fsl_asrc: make sure the input and output clocks are different Arnaud Ferraris @ 2020-07-17 11:21 ` Mark Brown 2020-07-17 11:34 ` Arnaud Ferraris 1 sibling, 1 reply; 18+ messages in thread From: Mark Brown @ 2020-07-17 11:21 UTC (permalink / raw) To: Arnaud Ferraris Cc: alsa-devel, Timur Tabi, Nicolin Chen, Xiubo Li, Fabio Estevam, Shengjiu Wang, Liam Girdwood, linux-kernel, kernel [-- 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 --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 0/1] ASoC: fsl_asrc: always select different clocks 2020-07-17 11:21 ` [PATCH v3 0/1] ASoC: fsl_asrc: always select different clocks Mark Brown @ 2020-07-17 11:34 ` Arnaud Ferraris 2020-07-17 11:45 ` Mark Brown 0 siblings, 1 reply; 18+ messages in thread From: Arnaud Ferraris @ 2020-07-17 11:34 UTC (permalink / raw) To: Mark Brown Cc: alsa-devel, Timur Tabi, Nicolin Chen, Xiubo Li, Fabio Estevam, Shengjiu Wang, Liam Girdwood, linux-kernel, kernel 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 0/1] ASoC: fsl_asrc: always select different clocks 2020-07-17 11:34 ` Arnaud Ferraris @ 2020-07-17 11:45 ` Mark Brown 0 siblings, 0 replies; 18+ messages in thread From: Mark Brown @ 2020-07-17 11:45 UTC (permalink / raw) To: Arnaud Ferraris Cc: alsa-devel, Timur Tabi, Nicolin Chen, Xiubo Li, Fabio Estevam, Shengjiu Wang, Liam Girdwood, linux-kernel, kernel [-- 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 --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 2/2] ASoC: fsl_asrc: always use internal ratio 2020-07-16 15:13 ` [PATCH v2 0/2] ASoC: fsl_asrc: improve clock selection and quality Arnaud Ferraris 2020-07-16 15:13 ` [PATCH v2 1/2] ASoC: fsl_asrc: make sure the input and output clocks are different Arnaud Ferraris @ 2020-07-16 15:13 ` Arnaud Ferraris 2020-07-16 23:37 ` Nicolin Chen 1 sibling, 1 reply; 18+ messages in thread From: Arnaud Ferraris @ 2020-07-16 15:13 UTC (permalink / raw) To: alsa-devel Cc: Timur Tabi, Nicolin Chen, Xiubo Li, Fabio Estevam, Shengjiu Wang, Liam Girdwood, Mark Brown, linux-kernel, kernel, 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/2] ASoC: fsl_asrc: always use internal ratio 2020-07-16 15:13 ` [PATCH v2 2/2] ASoC: fsl_asrc: always use internal ratio Arnaud Ferraris @ 2020-07-16 23:37 ` Nicolin Chen 2020-07-17 9:58 ` Arnaud Ferraris 0 siblings, 1 reply; 18+ messages in thread From: Nicolin Chen @ 2020-07-16 23:37 UTC (permalink / raw) To: Arnaud Ferraris Cc: alsa-devel, Timur Tabi, Xiubo Li, Fabio Estevam, Shengjiu Wang, Liam Girdwood, Mark Brown, linux-kernel, kernel 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/2] ASoC: fsl_asrc: always use internal ratio 2020-07-16 23:37 ` Nicolin Chen @ 2020-07-17 9:58 ` Arnaud Ferraris 2020-07-20 10:20 ` Shengjiu Wang 0 siblings, 1 reply; 18+ messages in thread From: Arnaud Ferraris @ 2020-07-17 9:58 UTC (permalink / raw) To: Nicolin Chen Cc: alsa-devel, Timur Tabi, Xiubo Li, Fabio Estevam, Liam Girdwood, linux-kernel, Mark Brown, kernel, Shengjiu Wang 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/2] ASoC: fsl_asrc: always use internal ratio 2020-07-17 9:58 ` Arnaud Ferraris @ 2020-07-20 10:20 ` Shengjiu Wang 0 siblings, 0 replies; 18+ messages in thread From: Shengjiu Wang @ 2020-07-20 10:20 UTC (permalink / raw) To: Arnaud Ferraris Cc: Nicolin Chen, Linux-ALSA, Timur Tabi, Xiubo Li, Fabio Estevam, Liam Girdwood, linux-kernel, Mark Brown, kernel 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2020-07-20 10:34 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-16 14:51 [PATCH 0/2] ASoC: fsl_asrc: improve clock selection and quality Arnaud Ferraris 2020-07-16 14:52 ` [PATCH 1/2] ASoC: fsl_asrc: make sure the input and output clocks are different Arnaud Ferraris 2020-07-16 15:07 ` Arnaud Ferraris 2020-07-16 14:52 ` [PATCH 2/2] ASoC: fsl_asrc: always use internal ratio Arnaud Ferraris 2020-07-16 15:13 ` [PATCH v2 0/2] ASoC: fsl_asrc: improve clock selection and quality Arnaud Ferraris 2020-07-16 15:13 ` [PATCH v2 1/2] ASoC: fsl_asrc: make sure the input and output clocks are different Arnaud Ferraris 2020-07-16 23:20 ` Nicolin Chen 2020-07-17 10:38 ` [PATCH v3 0/1] ASoC: fsl_asrc: always select different clocks Arnaud Ferraris 2020-07-17 10:38 ` [PATCH v3 1/1] ASoC: fsl_asrc: make sure the input and output clocks are different Arnaud Ferraris 2020-07-17 23:05 ` Nicolin Chen 2020-07-20 10:34 ` Shengjiu Wang 2020-07-17 11:21 ` [PATCH v3 0/1] ASoC: fsl_asrc: always select different clocks Mark Brown 2020-07-17 11:34 ` Arnaud Ferraris 2020-07-17 11:45 ` Mark Brown 2020-07-16 15:13 ` [PATCH v2 2/2] ASoC: fsl_asrc: always use internal ratio Arnaud Ferraris 2020-07-16 23:37 ` Nicolin Chen 2020-07-17 9:58 ` Arnaud Ferraris 2020-07-20 10:20 ` Shengjiu Wang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).