by Aidan MacDonald

[permalink] [raw]

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

Paul Cercueil <[email protected]> writes:

> Hi Aidan,
>
> Le sam. 22 oct. 2022 à 20:13:00 +0100, Aidan MacDonald
> <[email protected]> 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 <[email protected]>
>
> 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 <[email protected]>
>
> 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
>>