[PATCH] ASoC: fsl_asrc: refine the setting of internal clock divider

[PATCH] ASoC: fsl_asrc: refine the setting of internal clock divider

[Shengjiu Wang]

For P2P output, the output divider should align with the output sample
rate, if use ideal sample rate, there will be a lot of overload, which
would cause underrun.

The maximum divider of asrc clock is 1024, but there is no judgement
for this limitaion in driver, which may cause the divider setting not
correct.

For non-ideal ratio mode, the clock rate should divide the sample rate
with no remainder, and the quotient should be less than 1024.

Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
 sound/soc/fsl/fsl_asrc.c | 40 +++++++++++++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 9 deletions(-)

diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index 0bf91a6f54b9..44d05ec28bd3 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -260,7 +260,7 @@ static int fsl_asrc_set_ideal_ratio(struct fsl_asrc_pair *pair,
  * of struct asrc_config which includes in/output sample rate, width, channel
  * and clock settings.
  */
-static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair)
+static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair, bool p2p)
 {
 	struct asrc_config *config = pair->config;
 	struct fsl_asrc *asrc_priv = pair->asrc_priv;
@@ -268,7 +268,8 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair)
 	enum asrc_word_width input_word_width;
 	enum asrc_word_width output_word_width;
 	u32 inrate, outrate, indiv, outdiv;
-	u32 clk_index[2], div[2];
+	u32 clk_index[2], div[2], rem[2];
+	u64 clk_rate;
 	int in, out, channels;
 	int pre_proc, post_proc;
 	struct clk *clk;
@@ -351,7 +352,9 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair)
 	/* We only have output clock for ideal ratio mode */
 	clk = asrc_priv->asrck_clk[clk_index[ideal ? OUT : IN]];
 
-	div[IN] = clk_get_rate(clk) / inrate;
+	clk_rate = clk_get_rate(clk);
+	rem[IN] = do_div(clk_rate, inrate);
+	div[IN] = (u32)clk_rate;
 	if (div[IN] == 0) {
 		pair_err("failed to support input sample rate %dHz by asrck_%x\n",
 				inrate, clk_index[ideal ? OUT : IN]);
@@ -360,11 +363,20 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair)
 
 	clk = asrc_priv->asrck_clk[clk_index[OUT]];
 
-	/* Use fixed output rate for Ideal Ratio mode (INCLK_NONE) */
-	if (ideal)
-		div[OUT] = clk_get_rate(clk) / IDEAL_RATIO_RATE;
-	else
-		div[OUT] = clk_get_rate(clk) / outrate;
+	/*
+	 * When P2P mode, output rate should align with the out samplerate.
+	 * if set too high output rate, there will be lots of Overload.
+	 * When M2M mode, output rate should also need to align with the out
+	 * samplerate, but M2M must use less time to achieve good performance.
+	 */
+	clk_rate = clk_get_rate(clk);
+	if (p2p || !ideal) {
+		rem[OUT] = do_div(clk_rate, outrate);
+		div[OUT] = clk_rate;
+	} else {
+		rem[OUT] = do_div(clk_rate, IDEAL_RATIO_RATE);
+		div[OUT] = clk_rate;
+	}
 
 	if (div[OUT] == 0) {
 		pair_err("failed to support output sample rate %dHz by asrck_%x\n",
@@ -372,6 +384,16 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair)
 		return -EINVAL;
 	}
 
+	if (!ideal && (div[IN] > 1024 || div[OUT] > 1024 ||
+		       rem[IN] != 0 || rem[OUT] != 0)) {
+		pair_err("The divider can't be used for non ideal mode\n");
+		return -EINVAL;
+	}
+
+	/* Divider range is [1, 1024] */
+	div[IN] = min_t(u32, 1024, div[IN]);
+	div[OUT] = min_t(u32, 1024, div[OUT]);
+
 	/* Set the channel number */
 	channels = config->channel_num;
 
@@ -560,7 +582,7 @@ static int fsl_asrc_dai_hw_params(struct snd_pcm_substream *substream,
 		config.output_sample_rate = rate;
 	}
 
-	ret = fsl_asrc_config_pair(pair);
+	ret = fsl_asrc_config_pair(pair, true);
 	if (ret) {
 		dev_err(dai->dev, "fail to config asrc pair\n");
 		return ret;
-- 
2.21.0

Re: [PATCH] ASoC: fsl_asrc: refine the setting of internal clock divider

[Nicolin Chen]

Hello Shengjiu,

On Thu, Oct 17, 2019 at 02:21:08PM +0800, Shengjiu Wang wrote:
> For P2P output, the output divider should align with the output sample

I think we should avoid "P2P" (or "M2M") keyword in the mainline
code as we know M2M will never get merged while somebody working
with the mainline and caring about new feature might be confused.

> rate, if use ideal sample rate, there will be a lot of overload, which
> would cause underrun.

If I understand it correctly, setting to ideal ratio provides a
faster converting speed but increases the load of the processor
of ASRC. So we choose a slower converting speed here since real-
time playback mode doesn't really need a faster conversion?

It makes sense to me, yet I feel that the delay at the beginning
of the audio playback might be longer as a compromise. I am okay
with this decision though...

> The maximum divider of asrc clock is 1024, but there is no judgement
> for this limitaion in driver, which may cause the divider setting not
> correct.
> 
> For non-ideal ratio mode, the clock rate should divide the sample rate
> with no remainder, and the quotient should be less than 1024.
> 
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> ---
>  sound/soc/fsl/fsl_asrc.c | 40 +++++++++++++++++++++++++++++++---------
>  1 file changed, 31 insertions(+), 9 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
> index 0bf91a6f54b9..44d05ec28bd3 100644
> --- a/sound/soc/fsl/fsl_asrc.c
> +++ b/sound/soc/fsl/fsl_asrc.c
> @@ -260,7 +260,7 @@ static int fsl_asrc_set_ideal_ratio(struct fsl_asrc_pair *pair,
>   * of struct asrc_config which includes in/output sample rate, width, channel
>   * and clock settings.
>   */
> -static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair)
> +static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair, bool p2p)
>  {
>  	struct asrc_config *config = pair->config;
>  	struct fsl_asrc *asrc_priv = pair->asrc_priv;
> @@ -268,7 +268,8 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair)
>  	enum asrc_word_width input_word_width;
>  	enum asrc_word_width output_word_width;
>  	u32 inrate, outrate, indiv, outdiv;
> -	u32 clk_index[2], div[2];
> +	u32 clk_index[2], div[2], rem[2];
> +	u64 clk_rate;
>  	int in, out, channels;
>  	int pre_proc, post_proc;
>  	struct clk *clk;
> @@ -351,7 +352,9 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair)
>  	/* We only have output clock for ideal ratio mode */
>  	clk = asrc_priv->asrck_clk[clk_index[ideal ? OUT : IN]];
>  
> -	div[IN] = clk_get_rate(clk) / inrate;
> +	clk_rate = clk_get_rate(clk);

The fsl_asrc.c file has config.inclk being set to INCLK_NONE and
this sets the "ideal" in this function to true. So, although we
tend to not use ideal ratio setting for p2p cases, yet the input
clock is still not physically connected, so we still use output
clock for div[IN] calculation?

I am thinking something simplier: if we decided not to use ideal
ratio for "P2P", instead of adding "bool p2p" with the confusing
"ideal" in this function, could we just set config.inclk to the
same clock as the output one for "P2P"? By doing so, "P2P" won't
go through ideal ratio mode while still having a clock rate from
the output clock for div[IN] calculation here.

> +	rem[IN] = do_div(clk_rate, inrate);
> +	div[IN] = (u32)clk_rate;
>  	if (div[IN] == 0) {

Could we check div[IN] and rem[IN] here? Like:
  	if (div[IN] == 0 || div[IN] > 1024) {
		pair_err();
		goto out;
	}

	if (!ideal && rem[IN]) {
		pair_err();
		goto out;
	}

According to your commit log, I think the max-1024 limitation
should be applied to all cases, not confined to "!ideal" cases
right? And we should add some comments also, indicating it is
limited by hardware.

>  		pair_err("failed to support input sample rate %dHz by asrck_%x\n",
>  				inrate, clk_index[ideal ? OUT : IN]);
> @@ -360,11 +363,20 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair)
>  
>  	clk = asrc_priv->asrck_clk[clk_index[OUT]];
>  
> -	/* Use fixed output rate for Ideal Ratio mode (INCLK_NONE) */
> -	if (ideal)
> -		div[OUT] = clk_get_rate(clk) / IDEAL_RATIO_RATE;
> -	else
> -		div[OUT] = clk_get_rate(clk) / outrate;
> +	/*
> +	 * When P2P mode, output rate should align with the out samplerate.
> +	 * if set too high output rate, there will be lots of Overload.
> +	 * When M2M mode, output rate should also need to align with the out

For this "should", do you actually mean "M2M could also"? Sorry,
I'm just trying to understand everyting here, not intentionally
being picky at words. My understanding is that we still keep the
ideal ratio setting because "M2M" still uses it.

> +	 * samplerate, but M2M must use less time to achieve good performance.
> +	 */
> +	clk_rate = clk_get_rate(clk);
> +	if (p2p || !ideal) {
> +		rem[OUT] = do_div(clk_rate, outrate);
> +		div[OUT] = clk_rate;
> +	} else {
> +		rem[OUT] = do_div(clk_rate, IDEAL_RATIO_RATE);
> +		div[OUT] = clk_rate;
> +	}
>  
>  	if (div[OUT] == 0) {
>  		pair_err("failed to support output sample rate %dHz by asrck_%x\n",
> @@ -372,6 +384,16 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair)
>  		return -EINVAL;
>  	}
>  
> +	if (!ideal && (div[IN] > 1024 || div[OUT] > 1024 ||
> +		       rem[IN] != 0 || rem[OUT] != 0)) {
> +	if (!ideal && (div[IN] > 1024 || div[OUT] > 1024 || rem[IN] || rem[OUT] != 0)) {

So for ideal == true, these limitaions are not applied any more?
Remember that the "ideal" is true for "p2p == true" cases here.

> +		pair_err("The divider can't be used for non ideal mode\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Divider range is [1, 1024] */
> +	div[IN] = min_t(u32, 1024, div[IN]);
> +	div[OUT] = min_t(u32, 1024, div[OUT]);

Hmm, this looks like we want to allow ideal ratio cases and p2p
cases to operate any way, even if the divider wasn't within the
range to get the in/out rates from the output clock?

Re: [PATCH] ASoC: fsl_asrc: refine the setting of internal clock divider

[Nicolin Chen]

On Fri, Oct 25, 2019 at 05:33:17AM +0000, S.j. Wang wrote:
> > > > > +             pair_err("The divider can't be used for non ideal mode\n");
> > > > > +             return -EINVAL;
> > > > > +     }
> > > > > +
> > > > > +     /* Divider range is [1, 1024] */
> > > > > +     div[IN] = min_t(u32, 1024, div[IN]);
> > > > > +     div[OUT] = min_t(u32, 1024, div[OUT]);
> > > >
> > > > Hmm, this looks like we want to allow ideal ratio cases and p2p
> > > > cases to operate any way, even if the divider wasn't within the
> > > > range to get the in/out rates from the output clock?
> > >
> > > Yes. We still allow the p2p = true,  ideal = false.  Note that p2p is
> > > not Equal to ideal.
> > 
> > Got it.
> > 
> > Overall, I feel it's better to have a naming to state the purpose of using
> > ideal configurations without the IDEAL_RATIO_RATE setup.
> >         bool use_ideal_rate;
> > And we can put into the asrc_config structure if there's no major problem.
> > 
> 
> Asrc_config may exposed to user, I don't think user need to care about
> The using of ideal rate or not. 

Given that M2M could use output rate instead of ideal ratio rate
as well, it could be a configuration from my point of view. Yet,
we may just add it as a function parameter like you did, for now
to ease the situation, until we have such a need someday.

> 
> > So the condition check for the calculation would be:
> > +       if (ideal && config->use_ideal_rate)
> > +               rem[OUT] = do_div(clk_rate, IDEAL_RATIO_RATE);
> > +       else
> > +               rem[OUT] = do_div(clk_rate, outrate);
> > +       div[OUT] = clk_rate;
> > 
> > And for that if (!ideal && div[IN]....rem[OUT]), I feel it would be clear to
> > have them separately, as the existing "div[IN] == 0"
> > and "div[OUT] == 0" checks, so that we can tell users which side of the
> > divider is out of range and what the sample rate and clock rate are.
> > 
> Do you mean need to combine this judgement with "div[IN] == 0"
> Or "div[OUT] == 0"?

Not necessarily. Could put in the else path so its error message
would be more ideal ratio configuration specific.

Thanks

Re: [PATCH] ASoC: fsl_asrc: refine the setting of internal clock divider

[S.j. Wang]

Hi
> 
> On Wed, Oct 23, 2019 at 06:25:20AM +0000, S.j. Wang wrote:
> > > On Thu, Oct 17, 2019 at 02:21:08PM +0800, Shengjiu Wang wrote:
> > > > For P2P output, the output divider should align with the output
> > > > sample
> > >
> > > I think we should avoid "P2P" (or "M2M") keyword in the mainline
> > > code as we know M2M will never get merged while somebody working
> > > with the mainline and caring about new feature might be confused.
> >
> > Ok. But we still curious that is there a way to upstream m2m?
> 
> Hmm..I would love to see that happening. Here is an old discussion that
> you may want to take a look:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail
> man.alsa-project.org%2Fpipermail%2Falsa-devel%2F2014-
> May%2F076797.html&data=02%7C01%7Cshengjiu.wang%40nxp.com%7
> Ce902d2bac4254d2faa0f08d757ecac0e%7C686ea1d3bc2b4c6fa92cd99c5c301
> 635%7C0%7C0%7C637074546320396681&sdata=bg%2BLwRQnUPhW8f
> mE972O%2F53MyVftJkK140PSnmC%2FDKQ%3D&reserved=0
> 
> > > It makes sense to me, yet I feel that the delay at the beginning of
> > > the audio playback might be longer as a compromise. I am okay with
> > > this decision though...
> > >
> > > > The maximum divider of asrc clock is 1024, but there is no
> > > > judgement for this limitaion in driver, which may cause the
> > > > divider setting not correct.
> > > >
> > > > For non-ideal ratio mode, the clock rate should divide the sample
> > > > rate with no remainder, and the quotient should be less than 1024.
> > > >
> > > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> 
> > > > @@ -351,7 +352,9 @@ static int fsl_asrc_config_pair(struct
> > > > fsl_asrc_pair
> > > *pair)
> > > >       /* We only have output clock for ideal ratio mode */
> > > >       clk = asrc_priv->asrck_clk[clk_index[ideal ? OUT : IN]];
> > > >
> > > > -     div[IN] = clk_get_rate(clk) / inrate;
> > > > +     clk_rate = clk_get_rate(clk);
> > >
> > > The fsl_asrc.c file has config.inclk being set to INCLK_NONE and
> > > this sets the "ideal" in this function to true. So, although we tend
> > > to not use ideal ratio setting for p2p cases, yet the input clock is
> > > still not physically connected, so we still use output clock for div[IN]
> calculation?
> >
> > For p2p case, it can be ideal or non-ideal.  For non-ideal, we still
> > use Output clock for div calculation.
> >
> > >
> > > I am thinking something simplier: if we decided not to use ideal
> > > ratio for "P2P", instead of adding "bool p2p" with the confusing
> > > "ideal" in this function, could we just set config.inclk to the same
> > > clock as the output one for "P2P"? By doing so, "P2P" won't go
> > > through ideal ratio mode while still having a clock rate from the output
> clock for div[IN] calculation here.
> >
> > Bool p2p is to force output rate to be sample rate, no impact to ideal
> > Ratio mode.
> 
> I just realized that the function has a bottom part for ideal mode
> exclusively -- if we treat p2p as !ideal, those configurations will be missing.
> So you're right, should have an extra boolean variable.
> 
> > >
> > > > +     rem[IN] = do_div(clk_rate, inrate);
> > > > +     div[IN] = (u32)clk_rate;
> > > >       if (div[IN] == 0) {
> > >
> > > Could we check div[IN] and rem[IN] here? Like:
> > >         if (div[IN] == 0 || div[IN] > 1024) {
> > >                 pair_err();
> > >                 goto out;
> > >         }
> > >
> > >         if (!ideal && rem[IN]) {
> > >                 pair_err();
> > >                 goto out;
> > >         }
> > >
> > > According to your commit log, I think the max-1024 limitation should
> > > be applied to all cases, not confined to "!ideal" cases right? And
> > > we should add some comments also, indicating it is limited by hardware.
> >
> > For ideal mode,  my test result is  the divider not impact the output result.
> > Which means it is ok for ideal mode even divider is not correct...
> 
> OK.
> 
> > >
> > > >               pair_err("failed to support input sample rate %dHz
> > > > by
> > > asrck_%x\n",
> > > >                               inrate, clk_index[ideal ? OUT :
> > > > IN]); @@
> > > > -360,11 +363,20 @@ static int fsl_asrc_config_pair(struct
> > > > fsl_asrc_pair *pair)
> > > >
> > > >       clk = asrc_priv->asrck_clk[clk_index[OUT]];
> > > >
> > > > -     /* Use fixed output rate for Ideal Ratio mode (INCLK_NONE) */
> > > > -     if (ideal)
> > > > -             div[OUT] = clk_get_rate(clk) / IDEAL_RATIO_RATE;
> > > > -     else
> > > > -             div[OUT] = clk_get_rate(clk) / outrate;
> > > > +     /*
> > > > +      * When P2P mode, output rate should align with the out
> samplerate.
> > > > +      * if set too high output rate, there will be lots of Overload.
> > > > +      * When M2M mode, output rate should also need to align with
> > > > + the out
> > >
> > > For this "should", do you actually mean "M2M could also"? Sorry, I'm
> > > just trying to understand everyting here, not intentionally being picky at
> words.
> > > My understanding is that we still keep the ideal ratio setting
> > > because "M2M" still uses it.
> >
> > We use IDEAL_RATIO_RATE as output rate for m2m mode, it likes a Tricky
> > operation, in order to improve the performance. I think The correct
> > operation is to use the real output rate, but the performance Is bad.
> > So it is a compromise.
> 
> I see.
> 
> > >
> > > > +      * samplerate, but M2M must use less time to achieve good
> > > performance.
> > > > +      */
> > > > +     clk_rate = clk_get_rate(clk);
> > > > +     if (p2p || !ideal) {
> > > > +             rem[OUT] = do_div(clk_rate, outrate);
> > > > +             div[OUT] = clk_rate;
> > > > +     } else {
> > > > +             rem[OUT] = do_div(clk_rate, IDEAL_RATIO_RATE);
> > > > +             div[OUT] = clk_rate;
> > > > +     }
> > > >
> > > >       if (div[OUT] == 0) {
> > > >               pair_err("failed to support output sample rate %dHz
> > > > by asrck_%x\n", @@ -372,6 +384,16 @@ static int
> > > > fsl_asrc_config_pair(struct
> > > fsl_asrc_pair *pair)
> > > >               return -EINVAL;
> > > >       }
> > > >
> > > > +     if (!ideal && (div[IN] > 1024 || div[OUT] > 1024 ||
> > > > +                    rem[IN] != 0 || rem[OUT] != 0)) {
> > > > +     if (!ideal && (div[IN] > 1024 || div[OUT] > 1024 || rem[IN]
> > > > + || rem[OUT] != 0)) {
> > >
> > > So for ideal == true, these limitaions are not applied any more?
> > > Remember that the "ideal" is true for "p2p == true" cases here.
> >
> > No, not applied.  for ideal, the div don't impact the output result
> > Even it is not accurate.
> 
> I see.
> 
> > >
> > > > +             pair_err("The divider can't be used for non ideal mode\n");
> > > > +             return -EINVAL;
> > > > +     }
> > > > +
> > > > +     /* Divider range is [1, 1024] */
> > > > +     div[IN] = min_t(u32, 1024, div[IN]);
> > > > +     div[OUT] = min_t(u32, 1024, div[OUT]);
> > >
> > > Hmm, this looks like we want to allow ideal ratio cases and p2p
> > > cases to operate any way, even if the divider wasn't within the
> > > range to get the in/out rates from the output clock?
> >
> > Yes. We still allow the p2p = true,  ideal = false.  Note that p2p is
> > not Equal to ideal.
> 
> Got it.
> 
> Overall, I feel it's better to have a naming to state the purpose of using
> ideal configurations without the IDEAL_RATIO_RATE setup.
>         bool use_ideal_rate;
> And we can put into the asrc_config structure if there's no major problem.
> 

Asrc_config may exposed to user, I don't think user need to care about
The using of ideal rate or not. 

> So the condition check for the calculation would be:
> +       if (ideal && config->use_ideal_rate)
> +               rem[OUT] = do_div(clk_rate, IDEAL_RATIO_RATE);
> +       else
> +               rem[OUT] = do_div(clk_rate, outrate);
> +       div[OUT] = clk_rate;
> 
> And for that if (!ideal && div[IN]....rem[OUT]), I feel it would be clear to
> have them separately, as the existing "div[IN] == 0"
> and "div[OUT] == 0" checks, so that we can tell users which side of the
> divider is out of range and what the sample rate and clock rate are.
> 
Do you mean need to combine this judgement with "div[IN] == 0"
Or "div[OUT] == 0"?

Best regards
Wang shengjiu

Re: [PATCH] ASoC: fsl_asrc: refine the setting of internal clock divider

[Nicolin Chen]

On Wed, Oct 23, 2019 at 06:25:20AM +0000, S.j. Wang wrote:
> > On Thu, Oct 17, 2019 at 02:21:08PM +0800, Shengjiu Wang wrote:
> > > For P2P output, the output divider should align with the output sample
> > 
> > I think we should avoid "P2P" (or "M2M") keyword in the mainline code as
> > we know M2M will never get merged while somebody working with the
> > mainline and caring about new feature might be confused.
> 
> Ok. But we still curious that is there a way to upstream m2m?

Hmm..I would love to see that happening. Here is an old discussion
that you may want to take a look:
https://mailman.alsa-project.org/pipermail/alsa-devel/2014-May/076797.html

> > It makes sense to me, yet I feel that the delay at the beginning of the audio
> > playback might be longer as a compromise. I am okay with this decision
> > though...
> > 
> > > The maximum divider of asrc clock is 1024, but there is no judgement
> > > for this limitaion in driver, which may cause the divider setting not
> > > correct.
> > >
> > > For non-ideal ratio mode, the clock rate should divide the sample rate
> > > with no remainder, and the quotient should be less than 1024.
> > >
> > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>

> > > @@ -351,7 +352,9 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair
> > *pair)
> > >       /* We only have output clock for ideal ratio mode */
> > >       clk = asrc_priv->asrck_clk[clk_index[ideal ? OUT : IN]];
> > >
> > > -     div[IN] = clk_get_rate(clk) / inrate;
> > > +     clk_rate = clk_get_rate(clk);
> > 
> > The fsl_asrc.c file has config.inclk being set to INCLK_NONE and this sets the
> > "ideal" in this function to true. So, although we tend to not use ideal ratio
> > setting for p2p cases, yet the input clock is still not physically connected, so
> > we still use output clock for div[IN] calculation?
> 
> For p2p case, it can be ideal or non-ideal.  For non-ideal, we still use
> Output clock for div calculation.
> 
> > 
> > I am thinking something simplier: if we decided not to use ideal ratio for
> > "P2P", instead of adding "bool p2p" with the confusing "ideal" in this
> > function, could we just set config.inclk to the same clock as the output one
> > for "P2P"? By doing so, "P2P" won't go through ideal ratio mode while still
> > having a clock rate from the output clock for div[IN] calculation here.
> 
> Bool p2p is to force output rate to be sample rate, no impact to ideal
> Ratio mode.

I just realized that the function has a bottom part for ideal mode
exclusively -- if we treat p2p as !ideal, those configurations will
be missing. So you're right, should have an extra boolean variable.

> > 
> > > +     rem[IN] = do_div(clk_rate, inrate);
> > > +     div[IN] = (u32)clk_rate;
> > >       if (div[IN] == 0) {
> > 
> > Could we check div[IN] and rem[IN] here? Like:
> >         if (div[IN] == 0 || div[IN] > 1024) {
> >                 pair_err();
> >                 goto out;
> >         }
> > 
> >         if (!ideal && rem[IN]) {
> >                 pair_err();
> >                 goto out;
> >         }
> > 
> > According to your commit log, I think the max-1024 limitation should be
> > applied to all cases, not confined to "!ideal" cases right? And we should
> > add some comments also, indicating it is limited by hardware.
> 
> For ideal mode,  my test result is  the divider not impact the output result.
> Which means it is ok for ideal mode even divider is not correct... 

OK.

> > 
> > >               pair_err("failed to support input sample rate %dHz by
> > asrck_%x\n",
> > >                               inrate, clk_index[ideal ? OUT : IN]); @@
> > > -360,11 +363,20 @@ static int fsl_asrc_config_pair(struct
> > > fsl_asrc_pair *pair)
> > >
> > >       clk = asrc_priv->asrck_clk[clk_index[OUT]];
> > >
> > > -     /* Use fixed output rate for Ideal Ratio mode (INCLK_NONE) */
> > > -     if (ideal)
> > > -             div[OUT] = clk_get_rate(clk) / IDEAL_RATIO_RATE;
> > > -     else
> > > -             div[OUT] = clk_get_rate(clk) / outrate;
> > > +     /*
> > > +      * When P2P mode, output rate should align with the out samplerate.
> > > +      * if set too high output rate, there will be lots of Overload.
> > > +      * When M2M mode, output rate should also need to align with the
> > > + out
> > 
> > For this "should", do you actually mean "M2M could also"? Sorry, I'm just
> > trying to understand everyting here, not intentionally being picky at words.
> > My understanding is that we still keep the ideal ratio setting because
> > "M2M" still uses it.
> 
> We use IDEAL_RATIO_RATE as output rate for m2m mode, it likes a
> Tricky operation, in order to improve the performance. I think
> The correct operation is to use the real output rate, but the performance
> Is bad.  So it is a compromise.

I see.

> > 
> > > +      * samplerate, but M2M must use less time to achieve good
> > performance.
> > > +      */
> > > +     clk_rate = clk_get_rate(clk);
> > > +     if (p2p || !ideal) {
> > > +             rem[OUT] = do_div(clk_rate, outrate);
> > > +             div[OUT] = clk_rate;
> > > +     } else {
> > > +             rem[OUT] = do_div(clk_rate, IDEAL_RATIO_RATE);
> > > +             div[OUT] = clk_rate;
> > > +     }
> > >
> > >       if (div[OUT] == 0) {
> > >               pair_err("failed to support output sample rate %dHz by
> > > asrck_%x\n", @@ -372,6 +384,16 @@ static int fsl_asrc_config_pair(struct
> > fsl_asrc_pair *pair)
> > >               return -EINVAL;
> > >       }
> > >
> > > +     if (!ideal && (div[IN] > 1024 || div[OUT] > 1024 ||
> > > +                    rem[IN] != 0 || rem[OUT] != 0)) {
> > > +     if (!ideal && (div[IN] > 1024 || div[OUT] > 1024 || rem[IN] ||
> > > + rem[OUT] != 0)) {
> > 
> > So for ideal == true, these limitaions are not applied any more?
> > Remember that the "ideal" is true for "p2p == true" cases here.
> 
> No, not applied.  for ideal, the div don't impact the output result
> Even it is not accurate.

I see.

> > 
> > > +             pair_err("The divider can't be used for non ideal mode\n");
> > > +             return -EINVAL;
> > > +     }
> > > +
> > > +     /* Divider range is [1, 1024] */
> > > +     div[IN] = min_t(u32, 1024, div[IN]);
> > > +     div[OUT] = min_t(u32, 1024, div[OUT]);
> > 
> > Hmm, this looks like we want to allow ideal ratio cases and p2p cases to
> > operate any way, even if the divider wasn't within the range to get the
> > in/out rates from the output clock?
> 
> Yes. We still allow the p2p = true,  ideal = false.  Note that p2p is not
> Equal to ideal.

Got it.

Overall, I feel it's better to have a naming to state the purpose
of using ideal configurations without the IDEAL_RATIO_RATE setup.
	bool use_ideal_rate;
And we can put into the asrc_config structure if there's no major
problem.

So the condition check for the calculation would be:
+	if (ideal && config->use_ideal_rate)
+		rem[OUT] = do_div(clk_rate, IDEAL_RATIO_RATE);
+	else
+		rem[OUT] = do_div(clk_rate, outrate);
+	div[OUT] = clk_rate;

And for that if (!ideal && div[IN]....rem[OUT]), I feel it would
be clear to have them separately, as the existing "div[IN] == 0"
and "div[OUT] == 0" checks, so that we can tell users which side
of the divider is out of range and what the sample rate and clock
rate are.

Thanks
Nicolin

Re: [PATCH] ASoC: fsl_asrc: refine the setting of internal clock divider

[S.j. Wang]

Hi
> 
> On Thu, Oct 17, 2019 at 02:21:08PM +0800, Shengjiu Wang wrote:
> > For P2P output, the output divider should align with the output sample
> 
> I think we should avoid "P2P" (or "M2M") keyword in the mainline code as
> we know M2M will never get merged while somebody working with the
> mainline and caring about new feature might be confused.

Ok. But we still curious that is there a way to upstream m2m?

> 
> > rate, if use ideal sample rate, there will be a lot of overload, which
> > would cause underrun.
> 
> If I understand it correctly, setting to ideal ratio provides a faster converting
> speed but increases the load of the processor of ASRC. So we choose a
> slower converting speed here since real- time playback mode doesn't really
> need a faster conversion?

Yes.  Slower speed is enough for real-time playback

> 
> It makes sense to me, yet I feel that the delay at the beginning of the audio
> playback might be longer as a compromise. I am okay with this decision
> though...
> 
> > The maximum divider of asrc clock is 1024, but there is no judgement
> > for this limitaion in driver, which may cause the divider setting not
> > correct.
> >
> > For non-ideal ratio mode, the clock rate should divide the sample rate
> > with no remainder, and the quotient should be less than 1024.
> >
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > ---
> >  sound/soc/fsl/fsl_asrc.c | 40
> > +++++++++++++++++++++++++++++++---------
> >  1 file changed, 31 insertions(+), 9 deletions(-)
> >
> > diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c index
> > 0bf91a6f54b9..44d05ec28bd3 100644
> > --- a/sound/soc/fsl/fsl_asrc.c
> > +++ b/sound/soc/fsl/fsl_asrc.c
> > @@ -260,7 +260,7 @@ static int fsl_asrc_set_ideal_ratio(struct
> fsl_asrc_pair *pair,
> >   * of struct asrc_config which includes in/output sample rate, width,
> channel
> >   * and clock settings.
> >   */
> > -static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair)
> > +static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair, bool p2p)
> >  {
> >       struct asrc_config *config = pair->config;
> >       struct fsl_asrc *asrc_priv = pair->asrc_priv; @@ -268,7 +268,8
> > @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair)
> >       enum asrc_word_width input_word_width;
> >       enum asrc_word_width output_word_width;
> >       u32 inrate, outrate, indiv, outdiv;
> > -     u32 clk_index[2], div[2];
> > +     u32 clk_index[2], div[2], rem[2];
> > +     u64 clk_rate;
> >       int in, out, channels;
> >       int pre_proc, post_proc;
> >       struct clk *clk;
> > @@ -351,7 +352,9 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair
> *pair)
> >       /* We only have output clock for ideal ratio mode */
> >       clk = asrc_priv->asrck_clk[clk_index[ideal ? OUT : IN]];
> >
> > -     div[IN] = clk_get_rate(clk) / inrate;
> > +     clk_rate = clk_get_rate(clk);
> 
> The fsl_asrc.c file has config.inclk being set to INCLK_NONE and this sets the
> "ideal" in this function to true. So, although we tend to not use ideal ratio
> setting for p2p cases, yet the input clock is still not physically connected, so
> we still use output clock for div[IN] calculation?

For p2p case, it can be ideal or non-ideal.  For non-ideal, we still use
Output clock for div calculation.

> 
> I am thinking something simplier: if we decided not to use ideal ratio for
> "P2P", instead of adding "bool p2p" with the confusing "ideal" in this
> function, could we just set config.inclk to the same clock as the output one
> for "P2P"? By doing so, "P2P" won't go through ideal ratio mode while still
> having a clock rate from the output clock for div[IN] calculation here.

Bool p2p is to force output rate to be sample rate, no impact to ideal
Ratio mode.

> 
> > +     rem[IN] = do_div(clk_rate, inrate);
> > +     div[IN] = (u32)clk_rate;
> >       if (div[IN] == 0) {
> 
> Could we check div[IN] and rem[IN] here? Like:
>         if (div[IN] == 0 || div[IN] > 1024) {
>                 pair_err();
>                 goto out;
>         }
> 
>         if (!ideal && rem[IN]) {
>                 pair_err();
>                 goto out;
>         }
> 
> According to your commit log, I think the max-1024 limitation should be
> applied to all cases, not confined to "!ideal" cases right? And we should
> add some comments also, indicating it is limited by hardware.

For ideal mode,  my test result is  the divider not impact the output result.
Which means it is ok for ideal mode even divider is not correct... 

> 
> >               pair_err("failed to support input sample rate %dHz by
> asrck_%x\n",
> >                               inrate, clk_index[ideal ? OUT : IN]); @@
> > -360,11 +363,20 @@ static int fsl_asrc_config_pair(struct
> > fsl_asrc_pair *pair)
> >
> >       clk = asrc_priv->asrck_clk[clk_index[OUT]];
> >
> > -     /* Use fixed output rate for Ideal Ratio mode (INCLK_NONE) */
> > -     if (ideal)
> > -             div[OUT] = clk_get_rate(clk) / IDEAL_RATIO_RATE;
> > -     else
> > -             div[OUT] = clk_get_rate(clk) / outrate;
> > +     /*
> > +      * When P2P mode, output rate should align with the out samplerate.
> > +      * if set too high output rate, there will be lots of Overload.
> > +      * When M2M mode, output rate should also need to align with the
> > + out
> 
> For this "should", do you actually mean "M2M could also"? Sorry, I'm just
> trying to understand everyting here, not intentionally being picky at words.
> My understanding is that we still keep the ideal ratio setting because
> "M2M" still uses it.

We use IDEAL_RATIO_RATE as output rate for m2m mode, it likes a
Tricky operation, in order to improve the performance. I think
The correct operation is to use the real output rate, but the performance
Is bad.  So it is a compromise.

> 
> > +      * samplerate, but M2M must use less time to achieve good
> performance.
> > +      */
> > +     clk_rate = clk_get_rate(clk);
> > +     if (p2p || !ideal) {
> > +             rem[OUT] = do_div(clk_rate, outrate);
> > +             div[OUT] = clk_rate;
> > +     } else {
> > +             rem[OUT] = do_div(clk_rate, IDEAL_RATIO_RATE);
> > +             div[OUT] = clk_rate;
> > +     }
> >
> >       if (div[OUT] == 0) {
> >               pair_err("failed to support output sample rate %dHz by
> > asrck_%x\n", @@ -372,6 +384,16 @@ static int fsl_asrc_config_pair(struct
> fsl_asrc_pair *pair)
> >               return -EINVAL;
> >       }
> >
> > +     if (!ideal && (div[IN] > 1024 || div[OUT] > 1024 ||
> > +                    rem[IN] != 0 || rem[OUT] != 0)) {
> > +     if (!ideal && (div[IN] > 1024 || div[OUT] > 1024 || rem[IN] ||
> > + rem[OUT] != 0)) {
> 
> So for ideal == true, these limitaions are not applied any more?
> Remember that the "ideal" is true for "p2p == true" cases here.

No, not applied.  for ideal, the div don't impact the output result
Even it is not accurate.

> 
> > +             pair_err("The divider can't be used for non ideal mode\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     /* Divider range is [1, 1024] */
> > +     div[IN] = min_t(u32, 1024, div[IN]);
> > +     div[OUT] = min_t(u32, 1024, div[OUT]);
> 
> Hmm, this looks like we want to allow ideal ratio cases and p2p cases to
> operate any way, even if the divider wasn't within the range to get the
> in/out rates from the output clock?

Yes. We still allow the p2p = true,  ideal = false.  Note that p2p is not
Equal to ideal.


Best regards
Wang shengjiu