| Message ID | 20200716151352.193451-3-arnaud.ferraris@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: fsl_asrc: improve clock selection and quality | expand |
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.
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
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
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);
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(-)