[v5,1/9] ASoC: jz4740-i2s: Handle independent FIFO flush bits

On the JZ4740, there is a single bit that flushes (empties) both
the transmit and receive FIFO. Later SoCs have independent flush
bits for each FIFO.

Independent FIFOs can be flushed before the snd_soc_dai_active()
check because it won't disturb other active streams. This ensures
that the FIFO we're about to use is always flushed before starting
up. With shared FIFOs we can't do that because if another substream
is active, flushing its FIFO would cause underrun errors.

This also fixes a bug: since we were only setting the JZ4740's
flush bit, which corresponds to the TX FIFO flush bit on other
SoCs, other SoCs were not having their RX FIFO flushed at all.

Fixes: 967beb2e8777 ("ASoC: jz4740: Add jz4780 support")
Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
 sound/soc/jz4740/jz4740-i2s.c | 39 ++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

Hi Aidan,

Le sam. 22 oct. 2022 à 20:13:00 +0100, Aidan MacDonald 
<aidanmacdonald.0x0@gmail.com> a écrit :
> On the JZ4740, there is a single bit that flushes (empties) both
> the transmit and receive FIFO. Later SoCs have independent flush
> bits for each FIFO.
> 
> Independent FIFOs can be flushed before the snd_soc_dai_active()
> check because it won't disturb other active streams. This ensures
> that the FIFO we're about to use is always flushed before starting
> up. With shared FIFOs we can't do that because if another substream
> is active, flushing its FIFO would cause underrun errors.
> 
> This also fixes a bug: since we were only setting the JZ4740's
> flush bit, which corresponds to the TX FIFO flush bit on other
> SoCs, other SoCs were not having their RX FIFO flushed at all.
> 
> Fixes: 967beb2e8777 ("ASoC: jz4740: Add jz4780 support")
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>

If you have a Fixes: tag you need to Cc linux-stable as well.
See "option 1" of 
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

Also, a cover letter with a description of the changes is good, but you 
should still add a changelog per patch (below the --- line so that it 
doesn't end up in the commit message). That makes it much easier to 
review.

With the Cc line added:
Reviewed-by: Paul Cercueil <paul@crapouillou.net>

Cheers,
-Paul

> ---
>  sound/soc/jz4740/jz4740-i2s.c | 39 
> ++++++++++++++++++++++++++++++-----
>  1 file changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/sound/soc/jz4740/jz4740-i2s.c 
> b/sound/soc/jz4740/jz4740-i2s.c
> index c4c1e89b47c1..83cb81999c6f 100644
> --- a/sound/soc/jz4740/jz4740-i2s.c
> +++ b/sound/soc/jz4740/jz4740-i2s.c
> @@ -55,7 +55,8 @@
>  #define JZ_AIC_CTRL_MONO_TO_STEREO BIT(11)
>  #define JZ_AIC_CTRL_SWITCH_ENDIANNESS BIT(10)
>  #define JZ_AIC_CTRL_SIGNED_TO_UNSIGNED BIT(9)
> -#define JZ_AIC_CTRL_FLUSH		BIT(8)
> +#define JZ_AIC_CTRL_TFLUSH		BIT(8)
> +#define JZ_AIC_CTRL_RFLUSH		BIT(7)
>  #define JZ_AIC_CTRL_ENABLE_ROR_INT BIT(6)
>  #define JZ_AIC_CTRL_ENABLE_TUR_INT BIT(5)
>  #define JZ_AIC_CTRL_ENABLE_RFS_INT BIT(4)
> @@ -90,6 +91,8 @@ enum jz47xx_i2s_version {
>  struct i2s_soc_info {
>  	enum jz47xx_i2s_version version;
>  	struct snd_soc_dai_driver *dai;
> +
> +	bool shared_fifo_flush;
>  };
> 
>  struct jz4740_i2s {
> @@ -116,19 +119,44 @@ static inline void jz4740_i2s_write(const 
> struct jz4740_i2s *i2s,
>  	writel(value, i2s->base + reg);
>  }
> 
> +static inline void jz4740_i2s_set_bits(const struct jz4740_i2s *i2s,
> +	unsigned int reg, uint32_t bits)
> +{
> +	uint32_t value = jz4740_i2s_read(i2s, reg);
> +	value |= bits;
> +	jz4740_i2s_write(i2s, reg, value);
> +}
> +
>  static int jz4740_i2s_startup(struct snd_pcm_substream *substream,
>  	struct snd_soc_dai *dai)
>  {
>  	struct jz4740_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> -	uint32_t conf, ctrl;
> +	uint32_t conf;
>  	int ret;
> 
> +	/*
> +	 * When we can flush FIFOs independently, only flush the FIFO
> +	 * that is starting up. We can do this when the DAI is active
> +	 * because it does not disturb other active substreams.
> +	 */
> +	if (!i2s->soc_info->shared_fifo_flush) {
> +		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> +			jz4740_i2s_set_bits(i2s, JZ_REG_AIC_CTRL, JZ_AIC_CTRL_TFLUSH);
> +		else
> +			jz4740_i2s_set_bits(i2s, JZ_REG_AIC_CTRL, JZ_AIC_CTRL_RFLUSH);
> +	}
> +
>  	if (snd_soc_dai_active(dai))
>  		return 0;
> 
> -	ctrl = jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL);
> -	ctrl |= JZ_AIC_CTRL_FLUSH;
> -	jz4740_i2s_write(i2s, JZ_REG_AIC_CTRL, ctrl);
> +	/*
> +	 * When there is a shared flush bit for both FIFOs, the TFLUSH
> +	 * bit flushes both FIFOs. Flushing while the DAI is active would
> +	 * cause FIFO underruns in other active substreams so we have to
> +	 * guard this behind the snd_soc_dai_active() check.
> +	 */
> +	if (i2s->soc_info->shared_fifo_flush)
> +		jz4740_i2s_set_bits(i2s, JZ_REG_AIC_CTRL, JZ_AIC_CTRL_TFLUSH);
> 
>  	ret = clk_prepare_enable(i2s->clk_i2s);
>  	if (ret)
> @@ -443,6 +471,7 @@ static struct snd_soc_dai_driver jz4740_i2s_dai = 
> {
>  static const struct i2s_soc_info jz4740_i2s_soc_info = {
>  	.version = JZ_I2S_JZ4740,
>  	.dai = &jz4740_i2s_dai,
> +	.shared_fifo_flush = true,
>  };
> 
>  static const struct i2s_soc_info jz4760_i2s_soc_info = {
> --
> 2.38.1
>

Paul Cercueil <paul@crapouillou.net> writes:

> Hi Aidan,
>
> Le sam. 22 oct. 2022 à 20:13:00 +0100, Aidan MacDonald
> <aidanmacdonald.0x0@gmail.com> a écrit :
>> On the JZ4740, there is a single bit that flushes (empties) both
>> the transmit and receive FIFO. Later SoCs have independent flush
>> bits for each FIFO.
>> Independent FIFOs can be flushed before the snd_soc_dai_active()
>> check because it won't disturb other active streams. This ensures
>> that the FIFO we're about to use is always flushed before starting
>> up. With shared FIFOs we can't do that because if another substream
>> is active, flushing its FIFO would cause underrun errors.
>> This also fixes a bug: since we were only setting the JZ4740's
>> flush bit, which corresponds to the TX FIFO flush bit on other
>> SoCs, other SoCs were not having their RX FIFO flushed at all.
>> Fixes: 967beb2e8777 ("ASoC: jz4740: Add jz4780 support")
>> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
>
> If you have a Fixes: tag you need to Cc linux-stable as well.
> See "option 1" of
> https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

Then I'll resend with Cc: stable.

> Also, a cover letter with a description of the changes is good, but you should
> still add a changelog per patch (below the --- line so that it doesn't end up
> in the commit message). That makes it much easier to review.

Thank you, I'll keep that in mind next time.

> With the Cc line added:
> Reviewed-by: Paul Cercueil <paul@crapouillou.net>
>
> Cheers,
> -Paul
>
>> ---
>>  sound/soc/jz4740/jz4740-i2s.c | 39 ++++++++++++++++++++++++++++++-----
>>  1 file changed, 34 insertions(+), 5 deletions(-)
>> diff --git a/sound/soc/jz4740/jz4740-i2s.c b/sound/soc/jz4740/jz4740-i2s.c
>> index c4c1e89b47c1..83cb81999c6f 100644
>> --- a/sound/soc/jz4740/jz4740-i2s.c
>> +++ b/sound/soc/jz4740/jz4740-i2s.c
>> @@ -55,7 +55,8 @@
>>  #define JZ_AIC_CTRL_MONO_TO_STEREO BIT(11)
>>  #define JZ_AIC_CTRL_SWITCH_ENDIANNESS BIT(10)
>>  #define JZ_AIC_CTRL_SIGNED_TO_UNSIGNED BIT(9)
>> -#define JZ_AIC_CTRL_FLUSH		BIT(8)
>> +#define JZ_AIC_CTRL_TFLUSH		BIT(8)
>> +#define JZ_AIC_CTRL_RFLUSH		BIT(7)
>>  #define JZ_AIC_CTRL_ENABLE_ROR_INT BIT(6)
>>  #define JZ_AIC_CTRL_ENABLE_TUR_INT BIT(5)
>>  #define JZ_AIC_CTRL_ENABLE_RFS_INT BIT(4)
>> @@ -90,6 +91,8 @@ enum jz47xx_i2s_version {
>>  struct i2s_soc_info {
>>  	enum jz47xx_i2s_version version;
>>  	struct snd_soc_dai_driver *dai;
>> +
>> +	bool shared_fifo_flush;
>>  };
>>  struct jz4740_i2s {
>> @@ -116,19 +119,44 @@ static inline void jz4740_i2s_write(const struct
>> jz4740_i2s *i2s,
>>  	writel(value, i2s->base + reg);
>>  }
>> +static inline void jz4740_i2s_set_bits(const struct jz4740_i2s *i2s,
>> +	unsigned int reg, uint32_t bits)
>> +{
>> +	uint32_t value = jz4740_i2s_read(i2s, reg);
>> +	value |= bits;
>> +	jz4740_i2s_write(i2s, reg, value);
>> +}
>> +
>>  static int jz4740_i2s_startup(struct snd_pcm_substream *substream,
>>  	struct snd_soc_dai *dai)
>>  {
>>  	struct jz4740_i2s *i2s = snd_soc_dai_get_drvdata(dai);
>> -	uint32_t conf, ctrl;
>> +	uint32_t conf;
>>  	int ret;
>> +	/*
>> +	 * When we can flush FIFOs independently, only flush the FIFO
>> +	 * that is starting up. We can do this when the DAI is active
>> +	 * because it does not disturb other active substreams.
>> +	 */
>> +	if (!i2s->soc_info->shared_fifo_flush) {
>> +		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>> +			jz4740_i2s_set_bits(i2s, JZ_REG_AIC_CTRL, JZ_AIC_CTRL_TFLUSH);
>> +		else
>> +			jz4740_i2s_set_bits(i2s, JZ_REG_AIC_CTRL, JZ_AIC_CTRL_RFLUSH);
>> +	}
>> +
>>  	if (snd_soc_dai_active(dai))
>>  		return 0;
>> -	ctrl = jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL);
>> -	ctrl |= JZ_AIC_CTRL_FLUSH;
>> -	jz4740_i2s_write(i2s, JZ_REG_AIC_CTRL, ctrl);
>> +	/*
>> +	 * When there is a shared flush bit for both FIFOs, the TFLUSH
>> +	 * bit flushes both FIFOs. Flushing while the DAI is active would
>> +	 * cause FIFO underruns in other active substreams so we have to
>> +	 * guard this behind the snd_soc_dai_active() check.
>> +	 */
>> +	if (i2s->soc_info->shared_fifo_flush)
>> +		jz4740_i2s_set_bits(i2s, JZ_REG_AIC_CTRL, JZ_AIC_CTRL_TFLUSH);
>>  	ret = clk_prepare_enable(i2s->clk_i2s);
>>  	if (ret)
>> @@ -443,6 +471,7 @@ static struct snd_soc_dai_driver jz4740_i2s_dai = {
>>  static const struct i2s_soc_info jz4740_i2s_soc_info = {
>>  	.version = JZ_I2S_JZ4740,
>>  	.dai = &jz4740_i2s_dai,
>> +	.shared_fifo_flush = true,
>>  };
>>  static const struct i2s_soc_info jz4760_i2s_soc_info = {
>> --
>> 2.38.1
>>