Messages in this thread | | | Date | Sat, 22 Oct 2022 20:48:25 +0100 | From | Paul Cercueil <> | Subject | Re: [PATCH v5 1/9] ASoC: jz4740-i2s: Handle independent FIFO flush bits |
| |
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 >
| |