[v3,1/9] ASoC: sun4i-i2s: Adjust regmap settings
diff mbox series

Message ID 20181221152110.17982-2-codekipper@gmail.com
State New
Headers show
Series
  • ASoC: sun4i-i2s: Updates to the driver
Related show

Commit Message

Code Kipper Dec. 21, 2018, 3:21 p.m. UTC
From: Marcus Cooper <codekipper@gmail.com>

Bypass the regmap cache when flushing the i2s FIFOs and modify the tables
to reflect this.

Signed-off-by: Marcus Cooper <codekipper@gmail.com>
---
 sound/soc/sunxi/sun4i-i2s.c | 29 +++++++++--------------------
 1 file changed, 9 insertions(+), 20 deletions(-)

Comments

Chen-Yu Tsai Dec. 21, 2018, 4:44 p.m. UTC | #1
On Fri, Dec 21, 2018 at 11:21 PM <codekipper@gmail.com> wrote:
>
> From: Marcus Cooper <codekipper@gmail.com>
>
> Bypass the regmap cache when flushing the i2s FIFOs and modify the tables
> to reflect this.
>
> Signed-off-by: Marcus Cooper <codekipper@gmail.com>
> ---
>  sound/soc/sunxi/sun4i-i2s.c | 29 +++++++++--------------------
>  1 file changed, 9 insertions(+), 20 deletions(-)
>
> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> index d5ec1a20499d..64d073cb2aee 100644
> --- a/sound/soc/sunxi/sun4i-i2s.c
> +++ b/sound/soc/sunxi/sun4i-i2s.c
> @@ -548,9 +548,11 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
>  static void sun4i_i2s_start_capture(struct sun4i_i2s *i2s)
>  {
>         /* Flush RX FIFO */
> +       regcache_cache_bypass(i2s->regmap, true);
>         regmap_update_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG,
>                            SUN4I_I2S_FIFO_CTRL_FLUSH_RX,
>                            SUN4I_I2S_FIFO_CTRL_FLUSH_RX);
> +       regcache_cache_bypass(i2s->regmap, false);

IIRC the flush cache bit is self-clearing. So you likely want to mark
this register as volatile. If it is marked as volatile, then all access
to that register bypasses the cache, so the regcache_cache_bypass calls
are unneeded.

However, looking at the code, the write would seem to be ignored if the
regmap is in the cache_only state. We only set this when the bus clock
is disabled. Under such a condition, bypassing the cache and forcing a
write would be unwise, as the system either drops the write, or stalls
altogether.

>
>         /* Clear RX counter */
>         regmap_write(i2s->regmap, SUN4I_I2S_RX_CNT_REG, 0);
> @@ -569,9 +571,11 @@ static void sun4i_i2s_start_capture(struct sun4i_i2s *i2s)
>  static void sun4i_i2s_start_playback(struct sun4i_i2s *i2s)
>  {
>         /* Flush TX FIFO */
> +       regcache_cache_bypass(i2s->regmap, true);
>         regmap_update_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG,
>                            SUN4I_I2S_FIFO_CTRL_FLUSH_TX,
>                            SUN4I_I2S_FIFO_CTRL_FLUSH_TX);
> +       regcache_cache_bypass(i2s->regmap, false);
>
>         /* Clear TX counter */
>         regmap_write(i2s->regmap, SUN4I_I2S_TX_CNT_REG, 0);
> @@ -703,13 +707,7 @@ static const struct snd_soc_component_driver sun4i_i2s_component = {
>
>  static bool sun4i_i2s_rd_reg(struct device *dev, unsigned int reg)
>  {
> -       switch (reg) {
> -       case SUN4I_I2S_FIFO_TX_REG:
> -               return false;
> -
> -       default:
> -               return true;
> -       }
> +       return true;

I don't understand why this is relevant. Do you need to read back from the TX
FIFO?

If so, just drop the call-back altogether. If no callback is provided and no
rd_table is provided either, it defaults to all registers under max_register
(if max_register < 0) are readable.

However this seems like it deserves to be a separate patch (where you explain
in the commit log why it's needed).

Regards
ChenYu

>  }
>
>  static bool sun4i_i2s_wr_reg(struct device *dev, unsigned int reg)
> @@ -728,6 +726,8 @@ static bool sun4i_i2s_volatile_reg(struct device *dev, unsigned int reg)
>  {
>         switch (reg) {
>         case SUN4I_I2S_FIFO_RX_REG:
> +       case SUN4I_I2S_FIFO_TX_REG:
> +       case SUN4I_I2S_FIFO_STA_REG:
>         case SUN4I_I2S_INT_STA_REG:
>         case SUN4I_I2S_RX_CNT_REG:
>         case SUN4I_I2S_TX_CNT_REG:
> @@ -738,23 +738,12 @@ static bool sun4i_i2s_volatile_reg(struct device *dev, unsigned int reg)
>         }
>  }
>
> -static bool sun8i_i2s_rd_reg(struct device *dev, unsigned int reg)
> -{
> -       switch (reg) {
> -       case SUN8I_I2S_FIFO_TX_REG:
> -               return false;
> -
> -       default:
> -               return true;
> -       }
> -}
> -
>  static bool sun8i_i2s_volatile_reg(struct device *dev, unsigned int reg)
>  {
>         if (reg == SUN8I_I2S_INT_STA_REG)
>                 return true;
>         if (reg == SUN8I_I2S_FIFO_TX_REG)
> -               return false;
> +               return true;
>
>         return sun4i_i2s_volatile_reg(dev, reg);
>  }
> @@ -809,7 +798,7 @@ static const struct regmap_config sun8i_i2s_regmap_config = {
>         .reg_defaults   = sun8i_i2s_reg_defaults,
>         .num_reg_defaults       = ARRAY_SIZE(sun8i_i2s_reg_defaults),
>         .writeable_reg  = sun4i_i2s_wr_reg,
> -       .readable_reg   = sun8i_i2s_rd_reg,
> +       .readable_reg   = sun4i_i2s_rd_reg,
>         .volatile_reg   = sun8i_i2s_volatile_reg,
>  };
>
> --
> 2.20.1
>
Jonas Karlman Dec. 22, 2018, 10:12 p.m. UTC | #2
On 2018-12-21 17:44, Chen-Yu Tsai wrote:
> On Fri, Dec 21, 2018 at 11:21 PM <codekipper@gmail.com> wrote:
>> From: Marcus Cooper <codekipper@gmail.com>
>>
>> Bypass the regmap cache when flushing the i2s FIFOs and modify the tables
>> to reflect this.
>>
>> Signed-off-by: Marcus Cooper <codekipper@gmail.com>
>> ---
>>  sound/soc/sunxi/sun4i-i2s.c | 29 +++++++++--------------------
>>  1 file changed, 9 insertions(+), 20 deletions(-)
>>
>> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
>> index d5ec1a20499d..64d073cb2aee 100644
>> --- a/sound/soc/sunxi/sun4i-i2s.c
>> +++ b/sound/soc/sunxi/sun4i-i2s.c
>> @@ -548,9 +548,11 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
>>  static void sun4i_i2s_start_capture(struct sun4i_i2s *i2s)
>>  {
>>         /* Flush RX FIFO */
>> +       regcache_cache_bypass(i2s->regmap, true);
>>         regmap_update_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG,
>>                            SUN4I_I2S_FIFO_CTRL_FLUSH_RX,
>>                            SUN4I_I2S_FIFO_CTRL_FLUSH_RX);
>> +       regcache_cache_bypass(i2s->regmap, false);
> IIRC the flush cache bit is self-clearing. So you likely want to mark
> this register as volatile. If it is marked as volatile, then all access
> to that register bypasses the cache, so the regcache_cache_bypass calls
> are unneeded.

I helped test this together with Marcus and when I tested with this
register marked
as volatile audio started to stutter, still unclear why audio starts to
stutter.

Without this cache bypass the flush TX/RX bits gets cached and flush
happens unexpectedly
resulting in multi channel audio getting mapped to wrong speaker.
Other ASoC codecs and fsl_spdif.c seems to use similar cache bypass for
reset/flush.

>
> However, looking at the code, the write would seem to be ignored if the
> regmap is in the cache_only state. We only set this when the bus clock
> is disabled. Under such a condition, bypassing the cache and forcing a
> write would be unwise, as the system either drops the write, or stalls
> altogether.
>
>>         /* Clear RX counter */
>>         regmap_write(i2s->regmap, SUN4I_I2S_RX_CNT_REG, 0);
>> @@ -569,9 +571,11 @@ static void sun4i_i2s_start_capture(struct sun4i_i2s *i2s)
>>  static void sun4i_i2s_start_playback(struct sun4i_i2s *i2s)
>>  {
>>         /* Flush TX FIFO */
>> +       regcache_cache_bypass(i2s->regmap, true);
>>         regmap_update_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG,
>>                            SUN4I_I2S_FIFO_CTRL_FLUSH_TX,
>>                            SUN4I_I2S_FIFO_CTRL_FLUSH_TX);
>> +       regcache_cache_bypass(i2s->regmap, false);
>>
>>         /* Clear TX counter */
>>         regmap_write(i2s->regmap, SUN4I_I2S_TX_CNT_REG, 0);
>> @@ -703,13 +707,7 @@ static const struct snd_soc_component_driver sun4i_i2s_component = {
>>
>>  static bool sun4i_i2s_rd_reg(struct device *dev, unsigned int reg)
>>  {
>> -       switch (reg) {
>> -       case SUN4I_I2S_FIFO_TX_REG:
>> -               return false;
>> -
>> -       default:
>> -               return true;
>> -       }
>> +       return true;
> I don't understand why this is relevant. Do you need to read back from the TX
> FIFO?

This change was inspired by c66234cfedfc "ASoC: rockchip: i2s: fix
playback after runtime resume" [1]
On resume a cached sample would be written to FIFO_TX_REG unless it is
marked volatile,
the rockchip commit indicated that read is needed for volatile regs.

[1]
https://github.com/torvalds/linux/commit/c66234cfedfc3e6e3b62563a5f2c1562be09a35d

>
> If so, just drop the call-back altogether. If no callback is provided and no
> rd_table is provided either, it defaults to all registers under max_register
> (if max_register < 0) are readable.
>
> However this seems like it deserves to be a separate patch (where you explain
> in the commit log why it's needed).
>
> Regards
> ChenYu
>
>>  }
>>
>>  static bool sun4i_i2s_wr_reg(struct device *dev, unsigned int reg)
>> @@ -728,6 +726,8 @@ static bool sun4i_i2s_volatile_reg(struct device *dev, unsigned int reg)
>>  {
>>         switch (reg) {
>>         case SUN4I_I2S_FIFO_RX_REG:
>> +       case SUN4I_I2S_FIFO_TX_REG:
>> +       case SUN4I_I2S_FIFO_STA_REG:
>>         case SUN4I_I2S_INT_STA_REG:
>>         case SUN4I_I2S_RX_CNT_REG:
>>         case SUN4I_I2S_TX_CNT_REG:
>> @@ -738,23 +738,12 @@ static bool sun4i_i2s_volatile_reg(struct device *dev, unsigned int reg)
>>         }
>>  }
>>
>> -static bool sun8i_i2s_rd_reg(struct device *dev, unsigned int reg)
>> -{
>> -       switch (reg) {
>> -       case SUN8I_I2S_FIFO_TX_REG:
>> -               return false;
>> -
>> -       default:
>> -               return true;
>> -       }
>> -}
>> -
>>  static bool sun8i_i2s_volatile_reg(struct device *dev, unsigned int reg)
>>  {
>>         if (reg == SUN8I_I2S_INT_STA_REG)
>>                 return true;
>>         if (reg == SUN8I_I2S_FIFO_TX_REG)
>> -               return false;
>> +               return true;
>>
>>         return sun4i_i2s_volatile_reg(dev, reg);
>>  }
>> @@ -809,7 +798,7 @@ static const struct regmap_config sun8i_i2s_regmap_config = {
>>         .reg_defaults   = sun8i_i2s_reg_defaults,
>>         .num_reg_defaults       = ARRAY_SIZE(sun8i_i2s_reg_defaults),
>>         .writeable_reg  = sun4i_i2s_wr_reg,
>> -       .readable_reg   = sun8i_i2s_rd_reg,
>> +       .readable_reg   = sun4i_i2s_rd_reg,
>>         .volatile_reg   = sun8i_i2s_volatile_reg,
>>  };
>>
>> --
>> 2.20.1
>>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
>
Chen-Yu Tsai Dec. 23, 2018, 3:16 a.m. UTC | #3
On Sun, Dec 23, 2018 at 6:12 AM Jonas Karlman <jonas@kwiboo.se> wrote:
>
> On 2018-12-21 17:44, Chen-Yu Tsai wrote:
> > On Fri, Dec 21, 2018 at 11:21 PM <codekipper@gmail.com> wrote:
> >> From: Marcus Cooper <codekipper@gmail.com>
> >>
> >> Bypass the regmap cache when flushing the i2s FIFOs and modify the tables
> >> to reflect this.
> >>
> >> Signed-off-by: Marcus Cooper <codekipper@gmail.com>
> >> ---
> >>  sound/soc/sunxi/sun4i-i2s.c | 29 +++++++++--------------------
> >>  1 file changed, 9 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> >> index d5ec1a20499d..64d073cb2aee 100644
> >> --- a/sound/soc/sunxi/sun4i-i2s.c
> >> +++ b/sound/soc/sunxi/sun4i-i2s.c
> >> @@ -548,9 +548,11 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
> >>  static void sun4i_i2s_start_capture(struct sun4i_i2s *i2s)
> >>  {
> >>         /* Flush RX FIFO */
> >> +       regcache_cache_bypass(i2s->regmap, true);
> >>         regmap_update_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG,
> >>                            SUN4I_I2S_FIFO_CTRL_FLUSH_RX,
> >>                            SUN4I_I2S_FIFO_CTRL_FLUSH_RX);
> >> +       regcache_cache_bypass(i2s->regmap, false);
> > IIRC the flush cache bit is self-clearing. So you likely want to mark
> > this register as volatile. If it is marked as volatile, then all access
> > to that register bypasses the cache, so the regcache_cache_bypass calls
> > are unneeded.
>
> I helped test this together with Marcus and when I tested with this
> register marked
> as volatile audio started to stutter, still unclear why audio starts to
> stutter.

Sounds like we're missing something. However the only other time we
update this register is to set the FIFO mode.

> Without this cache bypass the flush TX/RX bits gets cached and flush
> happens unexpectedly
> resulting in multi channel audio getting mapped to wrong speaker.

This sounds like the flush is happening after DMA transfers and/or I2S
operations have started, disrupting the order of the audio samples. I
think that might be the case since the regcache is synced sequentially,
and the FIFO control register is after the enable bits. That would imply
that the device is taken out of runtime suspend after the .start_capture
or .start_playback callbacks. Not sure if that's the case, but that would
mean the bus clocks are still off at this point, and bypassing the cache
and updating the bits is basically moot.

I think there's something else happening here, but we need to figure it
out instead of papering over it with something that "just works" but
we don't know why it works.

> Other ASoC codecs and fsl_spdif.c seems to use similar cache bypass for
> reset/flush.

fsl_spdif.c does it when forcing a soft reset, which would reset all the
registers. It then does a cache sync, restoring any values set up. That's
not what we're doing here.

> >
> > However, looking at the code, the write would seem to be ignored if the
> > regmap is in the cache_only state. We only set this when the bus clock
> > is disabled. Under such a condition, bypassing the cache and forcing a
> > write would be unwise, as the system either drops the write, or stalls
> > altogether.
> >
> >>         /* Clear RX counter */
> >>         regmap_write(i2s->regmap, SUN4I_I2S_RX_CNT_REG, 0);
> >> @@ -569,9 +571,11 @@ static void sun4i_i2s_start_capture(struct sun4i_i2s *i2s)
> >>  static void sun4i_i2s_start_playback(struct sun4i_i2s *i2s)
> >>  {
> >>         /* Flush TX FIFO */
> >> +       regcache_cache_bypass(i2s->regmap, true);
> >>         regmap_update_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG,
> >>                            SUN4I_I2S_FIFO_CTRL_FLUSH_TX,
> >>                            SUN4I_I2S_FIFO_CTRL_FLUSH_TX);
> >> +       regcache_cache_bypass(i2s->regmap, false);
> >>
> >>         /* Clear TX counter */
> >>         regmap_write(i2s->regmap, SUN4I_I2S_TX_CNT_REG, 0);
> >> @@ -703,13 +707,7 @@ static const struct snd_soc_component_driver sun4i_i2s_component = {
> >>
> >>  static bool sun4i_i2s_rd_reg(struct device *dev, unsigned int reg)
> >>  {
> >> -       switch (reg) {
> >> -       case SUN4I_I2S_FIFO_TX_REG:
> >> -               return false;
> >> -
> >> -       default:
> >> -               return true;
> >> -       }
> >> +       return true;
> > I don't understand why this is relevant. Do you need to read back from the TX
> > FIFO?
>
> This change was inspired by c66234cfedfc "ASoC: rockchip: i2s: fix
> playback after runtime resume" [1]
> On resume a cached sample would be written to FIFO_TX_REG unless it is
> marked volatile,
> the rockchip commit indicated that read is needed for volatile regs.
>
> [1]
> https://github.com/torvalds/linux/commit/c66234cfedfc3e6e3b62563a5f2c1562be09a35d

What about simply marking the FIFO registers as both unreadable and
unwritable, thus excluding them from the regmap? That should get rid
of any unwanted syncs. We are doing DMA transfers to/from them. Do we
need regmap access?

Either way this context should be provided in the commit log.

Regards
ChenYu

> >
> > If so, just drop the call-back altogether. If no callback is provided and no
> > rd_table is provided either, it defaults to all registers under max_register
> > (if max_register < 0) are readable.
> >
> > However this seems like it deserves to be a separate patch (where you explain
> > in the commit log why it's needed).
> >
> > Regards
> > ChenYu
> >
> >>  }
> >>
> >>  static bool sun4i_i2s_wr_reg(struct device *dev, unsigned int reg)
> >> @@ -728,6 +726,8 @@ static bool sun4i_i2s_volatile_reg(struct device *dev, unsigned int reg)
> >>  {
> >>         switch (reg) {
> >>         case SUN4I_I2S_FIFO_RX_REG:
> >> +       case SUN4I_I2S_FIFO_TX_REG:
> >> +       case SUN4I_I2S_FIFO_STA_REG:
> >>         case SUN4I_I2S_INT_STA_REG:
> >>         case SUN4I_I2S_RX_CNT_REG:
> >>         case SUN4I_I2S_TX_CNT_REG:
> >> @@ -738,23 +738,12 @@ static bool sun4i_i2s_volatile_reg(struct device *dev, unsigned int reg)
> >>         }
> >>  }
> >>
> >> -static bool sun8i_i2s_rd_reg(struct device *dev, unsigned int reg)
> >> -{
> >> -       switch (reg) {
> >> -       case SUN8I_I2S_FIFO_TX_REG:
> >> -               return false;
> >> -
> >> -       default:
> >> -               return true;
> >> -       }
> >> -}
> >> -
> >>  static bool sun8i_i2s_volatile_reg(struct device *dev, unsigned int reg)
> >>  {
> >>         if (reg == SUN8I_I2S_INT_STA_REG)
> >>                 return true;
> >>         if (reg == SUN8I_I2S_FIFO_TX_REG)
> >> -               return false;
> >> +               return true;
> >>
> >>         return sun4i_i2s_volatile_reg(dev, reg);
> >>  }
> >> @@ -809,7 +798,7 @@ static const struct regmap_config sun8i_i2s_regmap_config = {
> >>         .reg_defaults   = sun8i_i2s_reg_defaults,
> >>         .num_reg_defaults       = ARRAY_SIZE(sun8i_i2s_reg_defaults),
> >>         .writeable_reg  = sun4i_i2s_wr_reg,
> >> -       .readable_reg   = sun8i_i2s_rd_reg,
> >> +       .readable_reg   = sun4i_i2s_rd_reg,
> >>         .volatile_reg   = sun8i_i2s_volatile_reg,
> >>  };
> >>
> >> --
> >> 2.20.1
> >>
> > _______________________________________________
> > Alsa-devel mailing list
> > Alsa-devel@alsa-project.org
> >
Mark Brown Jan. 9, 2019, 6:46 p.m. UTC | #4
On Sat, Dec 22, 2018 at 12:44:07AM +0800, Chen-Yu Tsai wrote:
> On Fri, Dec 21, 2018 at 11:21 PM <codekipper@gmail.com> wrote:

> > +       regcache_cache_bypass(i2s->regmap, true);
> >         regmap_update_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG,
> >                            SUN4I_I2S_FIFO_CTRL_FLUSH_RX,
> >                            SUN4I_I2S_FIFO_CTRL_FLUSH_RX);
> > +       regcache_cache_bypass(i2s->regmap, false);

> IIRC the flush cache bit is self-clearing. So you likely want to mark
> this register as volatile. If it is marked as volatile, then all access
> to that register bypasses the cache, so the regcache_cache_bypass calls
> are unneeded.

Yes, that should be the case.

> However, looking at the code, the write would seem to be ignored if the
> regmap is in the cache_only state. We only set this when the bus clock
> is disabled. Under such a condition, bypassing the cache and forcing a
> write would be unwise, as the system either drops the write, or stalls
> altogether.

Right, access to a cache only register while the device is in cache only
mode is not a great idea - the usual reason we're in cache only mode is
that the device is in a state where I/O isn't going to work.  One thing
that can work for this if you need the register to be cached (but is a
bit gross) is to do a write setting the self clearing bit then another
immediately after resetting it back to the cleared state.  That works OK
for cases where the bit is a strobe and never retains state, though if
the device isn't operational then needing to write to the register might
indicate a bigger picture logic error (or it could be that the register
map mixes random things into one register).
Mark Brown Jan. 9, 2019, 6:49 p.m. UTC | #5
On Sun, Dec 23, 2018 at 11:16:31AM +0800, Chen-Yu Tsai wrote:

> This sounds like the flush is happening after DMA transfers and/or I2S
> operations have started, disrupting the order of the audio samples. I
> think that might be the case since the regcache is synced sequentially,
> and the FIFO control register is after the enable bits. That would imply
> that the device is taken out of runtime suspend after the .start_capture
> or .start_playback callbacks. Not sure if that's the case, but that would
> mean the bus clocks are still off at this point, and bypassing the cache
> and updating the bits is basically moot.

I would expect that the device needs to be resumed from suspend before
we start actually trying to transfer audio - there is stuff in the ASoC
core which is supposed to have appropriate gets but it's possible
something is going wrong there.

> I think there's something else happening here, but we need to figure it
> out instead of papering over it with something that "just works" but
> we don't know why it works.

I agree.

Patch
diff mbox series

diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
index d5ec1a20499d..64d073cb2aee 100644
--- a/sound/soc/sunxi/sun4i-i2s.c
+++ b/sound/soc/sunxi/sun4i-i2s.c
@@ -548,9 +548,11 @@  static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 static void sun4i_i2s_start_capture(struct sun4i_i2s *i2s)
 {
 	/* Flush RX FIFO */
+	regcache_cache_bypass(i2s->regmap, true);
 	regmap_update_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG,
 			   SUN4I_I2S_FIFO_CTRL_FLUSH_RX,
 			   SUN4I_I2S_FIFO_CTRL_FLUSH_RX);
+	regcache_cache_bypass(i2s->regmap, false);
 
 	/* Clear RX counter */
 	regmap_write(i2s->regmap, SUN4I_I2S_RX_CNT_REG, 0);
@@ -569,9 +571,11 @@  static void sun4i_i2s_start_capture(struct sun4i_i2s *i2s)
 static void sun4i_i2s_start_playback(struct sun4i_i2s *i2s)
 {
 	/* Flush TX FIFO */
+	regcache_cache_bypass(i2s->regmap, true);
 	regmap_update_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG,
 			   SUN4I_I2S_FIFO_CTRL_FLUSH_TX,
 			   SUN4I_I2S_FIFO_CTRL_FLUSH_TX);
+	regcache_cache_bypass(i2s->regmap, false);
 
 	/* Clear TX counter */
 	regmap_write(i2s->regmap, SUN4I_I2S_TX_CNT_REG, 0);
@@ -703,13 +707,7 @@  static const struct snd_soc_component_driver sun4i_i2s_component = {
 
 static bool sun4i_i2s_rd_reg(struct device *dev, unsigned int reg)
 {
-	switch (reg) {
-	case SUN4I_I2S_FIFO_TX_REG:
-		return false;
-
-	default:
-		return true;
-	}
+	return true;
 }
 
 static bool sun4i_i2s_wr_reg(struct device *dev, unsigned int reg)
@@ -728,6 +726,8 @@  static bool sun4i_i2s_volatile_reg(struct device *dev, unsigned int reg)
 {
 	switch (reg) {
 	case SUN4I_I2S_FIFO_RX_REG:
+	case SUN4I_I2S_FIFO_TX_REG:
+	case SUN4I_I2S_FIFO_STA_REG:
 	case SUN4I_I2S_INT_STA_REG:
 	case SUN4I_I2S_RX_CNT_REG:
 	case SUN4I_I2S_TX_CNT_REG:
@@ -738,23 +738,12 @@  static bool sun4i_i2s_volatile_reg(struct device *dev, unsigned int reg)
 	}
 }
 
-static bool sun8i_i2s_rd_reg(struct device *dev, unsigned int reg)
-{
-	switch (reg) {
-	case SUN8I_I2S_FIFO_TX_REG:
-		return false;
-
-	default:
-		return true;
-	}
-}
-
 static bool sun8i_i2s_volatile_reg(struct device *dev, unsigned int reg)
 {
 	if (reg == SUN8I_I2S_INT_STA_REG)
 		return true;
 	if (reg == SUN8I_I2S_FIFO_TX_REG)
-		return false;
+		return true;
 
 	return sun4i_i2s_volatile_reg(dev, reg);
 }
@@ -809,7 +798,7 @@  static const struct regmap_config sun8i_i2s_regmap_config = {
 	.reg_defaults	= sun8i_i2s_reg_defaults,
 	.num_reg_defaults	= ARRAY_SIZE(sun8i_i2s_reg_defaults),
 	.writeable_reg	= sun4i_i2s_wr_reg,
-	.readable_reg	= sun8i_i2s_rd_reg,
+	.readable_reg	= sun4i_i2s_rd_reg,
 	.volatile_reg	= sun8i_i2s_volatile_reg,
 };