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

[Samuel Holland]

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@xxxxxxxxxxxx>
>> ---
>>  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.

>> +		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.

> Maxime

Cheers,
Samuel