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

[Nicolin Chen]

On Fri, Oct 25, 2019 at 03:13:22PM +0800, Shengjiu Wang wrote:
> 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

typo: "limitaion" => "limitation"

> 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@xxxxxxx>

And some comments inline. Please add my ack once they are fixed:

Acked-by: Nicolin Chen <nicoleotsuka@xxxxxxxxx>

Thanks

> diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
> index 0bf91a6f54b9..89cf333154c7 100644
> --- a/sound/soc/fsl/fsl_asrc.c
> +++ b/sound/soc/fsl/fsl_asrc.c
> @@ -259,8 +259,11 @@ static int fsl_asrc_set_ideal_ratio(struct fsl_asrc_pair *pair,
>   * It configures those ASRC registers according to a configuration instance
>   * of struct asrc_config which includes in/output sample rate, width, channel
>   * and clock settings.
> + *
> + * Note:
> + * use_ideal_rate = true is need by some case which need higher performance.

I feel we can have a detailed one here and drop those inline comments, e.g.:

+ * Note:
+ * The ideal ratio configuration can work with a flexible clock rate setting.
+ * Using IDEAL_RATIO_RATE gives a faster converting speed but overloads ASRC.
+ * For a regular audio playback, the clock rate should not be slower than an
+ * clock rate aligning with the output sample rate; For a use case requiring
+ * faster conversion, set use_ideal_rate to have the faster speed.

> @@ -351,8 +355,10 @@ 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;
> -	if (div[IN] == 0) {
> +	clk_rate = clk_get_rate(clk);
> +	rem[IN] = do_div(clk_rate, inrate);
> +	div[IN] = (u32)clk_rate;

> +	if (div[IN] == 0 || (!ideal && (div[IN] > 1024 || rem[IN] != 0))) {

Should have some comments to explain this like:
	/*
	 * The divider range is [1, 1024], defined by the hardware. For non-
	 * ideal ratio configuration, clock rate has to be strictly aligned
	 * with the sample rate. For ideal ratio configuration, clock rates
	 * only result in different converting speeds. So remainder does not
	 * matter, as long as we keep the divider within its valid range.
	 */
>  		pair_err("failed to support input sample rate %dHz by asrck_%x\n",
>  				inrate, clk_index[ideal ? OUT : IN]);
>  		return -EINVAL;

And move the min() behind this if-condition with no more comments:
+	div[IN] = min_t(u32, 1024, div[IN]);

> @@ -360,18 +366,29 @@ 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;
> +	/*
> +	 * Output rate should be align with the out samplerate. If set too
> +	 * high output rate, there will be lots of Overload.
> +	 * But some case need higher performance, then we can use
> +	 * IDEAL_RATIO_RATE specifically for such case.
> +	 */

Can drop this since we have the detailed comments at the top.

> +	clk_rate = clk_get_rate(clk);
> +	if (ideal && use_ideal_rate)
> +		rem[OUT] = do_div(clk_rate, IDEAL_RATIO_RATE);
>  	else
> -		div[OUT] = clk_get_rate(clk) / outrate;
> +		rem[OUT] = do_div(clk_rate, outrate);
> +	div[OUT] = clk_rate;
>  
> -	if (div[OUT] == 0) {

And add before this if-condition:

	/* Output divider has the same limitation as the input one */

> +	if (div[OUT] == 0 || (!ideal && (div[OUT] > 1024 || rem[OUT] != 0))) {
>  		pair_err("failed to support output sample rate %dHz by asrck_%x\n",
>  				outrate, clk_index[OUT]);
>  		return -EINVAL;
>  	}
>  
> +	/* Divider range is [1, 1024] */

Can drop this too.

> +	div[IN] = min_t(u32, 1024, div[IN]);
> +	div[OUT] = min_t(u32, 1024, div[OUT]);