From: Nicolin Chen <nicoleotsuka@gmail.com>
To: Shengjiu Wang <shengjiu.wang@nxp.com>
Cc: alsa-devel@alsa-project.org, timur@kernel.org,
Xiubo.Lee@gmail.com, linuxppc-dev@lists.ozlabs.org,
tiwai@suse.com, lgirdwood@gmail.com, perex@perex.cz,
broonie@kernel.org, festevam@gmail.com,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH V2] ASoC: fsl_asrc: refine the setting of internal clock divider
Date: Fri, 25 Oct 2019 14:59:20 -0700 [thread overview]
Message-ID: <20191025215919.GB15101@Asurada-Nvidia.nvidia.com> (raw)
In-Reply-To: <a0cd2ecf5e833fbdc064ba73391481d6073e7254.1571986398.git.shengjiu.wang@nxp.com>
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@nxp.com>
And some comments inline. Please add my ack once they are fixed:
Acked-by: Nicolin Chen <nicoleotsuka@gmail.com>
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]);
prev parent reply other threads:[~2019-10-25 22:01 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-25 7:13 [PATCH V2] ASoC: fsl_asrc: refine the setting of internal clock divider Shengjiu Wang
2019-10-25 21:59 ` Nicolin Chen [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191025215919.GB15101@Asurada-Nvidia.nvidia.com \
--to=nicoleotsuka@gmail.com \
--cc=Xiubo.Lee@gmail.com \
--cc=alsa-devel@alsa-project.org \
--cc=broonie@kernel.org \
--cc=festevam@gmail.com \
--cc=lgirdwood@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=perex@perex.cz \
--cc=shengjiu.wang@nxp.com \
--cc=timur@kernel.org \
--cc=tiwai@suse.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).