Re: [PATCH 20/25] ASoC: sun8i-codec: Protect the clock rate while streams are open

From: Maxime Ripard <maxime@cerno.tech>
To: Samuel Holland <samuel@sholland.org>
Cc: Mark Brown <broonie@kernel.org>,
	Liam Girdwood <lgirdwood@gmail.com>, Chen-Yu Tsai <wens@csie.org>,
	Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>,
	Ondrej Jirman <megous@megous.com>,
	alsa-devel@alsa-project.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 20/25] ASoC: sun8i-codec: Protect the clock rate while streams are open
Date: Thu, 8 Oct 2020 15:02:41 +0200	[thread overview]
Message-ID: <20201008130241.uigiys7iv72hca7r@gilmour.lan> (raw)
In-Reply-To: <5ef4351f-e9ed-1a38-b79e-53e62a70437e@sholland.org>

[-- Attachment #1: Type: text/plain, Size: 4055 bytes --]

On Mon, Oct 05, 2020 at 11:43:51PM -0500, Samuel Holland wrote:
> On 10/5/20 7:01 AM, Maxime Ripard wrote:
> > On Wed, Sep 30, 2020 at 09:11:43PM -0500, Samuel Holland wrote:
> >> The codec's clock input is shared among all AIFs, and shared with other
> >> audio-related hardware in the SoC, including I2S and SPDIF controllers.
> >> To ensure sample rates selected by userspace or by codec2codec DAI links
> >> are maintained, the clock rate must be protected while it is in use.
> >>
> >> Signed-off-by: Samuel Holland <samuel@sholland.org>
> >> ---
> >>  sound/soc/sunxi/sun8i-codec.c | 25 ++++++++++++++++++++++---
> >>  1 file changed, 22 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/sound/soc/sunxi/sun8i-codec.c b/sound/soc/sunxi/sun8i-codec.c
> >> index 501af64d43a0..86065bee7cd3 100644
> >> --- a/sound/soc/sunxi/sun8i-codec.c
> >> +++ b/sound/soc/sunxi/sun8i-codec.c
> >> @@ -416,27 +416,32 @@ static int sun8i_codec_get_lrck_div_order(unsigned int slots,
> >>  	unsigned int div = slots * slot_width;
> >>  
> >>  	if (div < 16 || div > 256)
> >>  		return -EINVAL;
> >>  
> >>  	return order_base_2(div);
> >>  }
> >>  
> >> +static unsigned int sun8i_codec_get_sysclk_rate(unsigned int sample_rate)
> >> +{
> >> +	return sample_rate % 4000 ? 22579200 : 24576000;
> >> +}
> >> +
> >>  static int sun8i_codec_hw_params(struct snd_pcm_substream *substream,
> >>  				 struct snd_pcm_hw_params *params,
> >>  				 struct snd_soc_dai *dai)
> >>  {
> >>  	struct sun8i_codec *scodec = snd_soc_dai_get_drvdata(dai);
> >>  	struct sun8i_codec_aif *aif = &scodec->aifs[dai->id];
> >>  	unsigned int sample_rate = params_rate(params);
> >>  	unsigned int slots = aif->slots ?: params_channels(params);
> >>  	unsigned int slot_width = aif->slot_width ?: params_width(params);
> >> -	unsigned int sysclk_rate = clk_get_rate(scodec->clk_module);
> >> -	int lrck_div_order, word_size;
> >> +	unsigned int sysclk_rate = sun8i_codec_get_sysclk_rate(sample_rate);
> >> +	int lrck_div_order, ret, word_size;
> >>  	u8 bclk_div;
> >>  
> >>  	/* word size */
> >>  	switch (params_width(params)) {
> >>  	case 8:
> >>  		word_size = 0x0;
> >>  		break;
> >>  	case 16:
> >> @@ -466,17 +471,30 @@ static int sun8i_codec_hw_params(struct snd_pcm_substream *substream,
> >>  			   (lrck_div_order - 4) << SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV);
> >>  
> >>  	/* BCLK divider (SYSCLK/BCLK ratio) */
> >>  	bclk_div = sun8i_codec_get_bclk_div(sysclk_rate, lrck_div_order, sample_rate);
> >>  	regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL,
> >>  			   SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV_MASK,
> >>  			   bclk_div << SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV);
> >>  
> >> -	if (!aif->open_streams) {
> >> +	/* SYSCLK rate */
> >> +	if (aif->open_streams) {
> >> +		ret = clk_set_rate(scodec->clk_module, sysclk_rate);
> >> +		if (ret < 0)
> >> +			return ret;
> >> +	} else {
> >> +		ret = clk_set_rate_exclusive(scodec->clk_module, sysclk_rate);
> > 
> > It's not really clear to me why we wouldn't want to always protect the
> > clock rate here?
> 
> From Documentation/sound/kernel-api/writing-an-alsa-driver.rst:
> 
>     hw_params callback
>     ...
>     Note that this and ``prepare`` callbacks may be called multiple
>     times per initialization. For example, the OSS emulation may call
>     these callbacks at each change via its ioctl.
> 
> Clock rate protection is reference counted, so we must only take one
> reference (or at least a known number of references) per stream.

Ah, right.

Can you add a comment to make that more obvious?

> >> +		if (ret == -EBUSY)
> >> +			dev_err(dai->dev, "%s: clock is busy! Sample rate %u Hz "
> >> +				"conflicts with other audio streams.\n",
> > 
> > This string creates a checkpatch warning.
> 
> I will put it on one line, though >100 columns is also a checkpatch warning.

Yeah, but in general having an error on a single line is more important.
That way you can then grep for that error message

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]