mbox series

[v2,0/7] Add H6 I2S support

Message ID 20200418224435.23672-1-peron.clem@gmail.com
Headers show
Series Add H6 I2S support | expand

Message

Clément Péron April 18, 2020, 10:44 p.m. UTC 
Hi,

This is a sequel of Marcus Cooper serie[0], where remarks made by Maxime
have been fixed.

I have tested it on my Beelink GS1 board.

Thanks,
Clement

0: https://lore.kernel.org/patchwork/cover/1139949/

Changes since v1:
 - Fix missing header in set sign extend sample

Jernej Skrabec (3):
  dt-bindings: ASoC: sun4i-i2s: Add H6 compatible
  ASoC: sun4i-i2s: Add support for H6 I2S
  arm64: dts: sun50i-h6: Add HDMI audio to H6 DTSI

Marcus Cooper (4):
  ASoC: sun4i-i2s: Adjust LRCLK width
  ASoC: sun4i-i2s: Set sign extend sample
  ASoc: sun4i-i2s: Add 20 and 24 bit support
  ASoC: sun4i-i2s: Adjust regmap settings

 .../sound/allwinner,sun4i-a10-i2s.yaml        |   2 +
 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi  |  31 ++
 sound/soc/sunxi/sun4i-i2s.c                   | 292 ++++++++++++++++--
 3 files changed, 301 insertions(+), 24 deletions(-)

Comments

Maxime Ripard April 20, 2020, 12:43 p.m. UTC | #1 
Hi,

On Sun, Apr 19, 2020 at 12:44:34AM +0200, Clément Péron wrote:
> From: Marcus Cooper <codekipper@gmail.com>
> 
> Bypass the regmap cache when flushing the i2s FIFOs and modify the tables
> to reflect this.

I think that commit log requires a bit more work.

As far as I can see, you're doing several things here:

> Signed-off-by: Marcus Cooper <codekipper@gmail.com>
> Signed-off-by: Clément Péron <peron.clem@gmail.com>
> ---
>  sound/soc/sunxi/sun4i-i2s.c | 31 ++++++++-----------------------
>  1 file changed, 8 insertions(+), 23 deletions(-)
> 
> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> index 9778af37fbca..9053d290dd87 100644
> --- a/sound/soc/sunxi/sun4i-i2s.c
> +++ b/sound/soc/sunxi/sun4i-i2s.c
> @@ -921,7 +921,7 @@ 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 */
> -	regmap_update_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG,
> +	regmap_write_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG,
>  			   SUN4I_I2S_FIFO_CTRL_FLUSH_RX,
>  			   SUN4I_I2S_FIFO_CTRL_FLUSH_RX);

You change regmap_update_bits to regmap_write_bits, I assume this is what the
commit log means by "bypassing the cache", so that every write actually gets
done even if the bit is already set.

I'm not quite sure why it's needed though, since that bit is self-clearing.

> @@ -942,7 +942,7 @@ static void sun4i_i2s_start_capture(struct sun4i_i2s *i2s)
>  static void sun4i_i2s_start_playback(struct sun4i_i2s *i2s)
>  {
>  	/* Flush TX FIFO */
> -	regmap_update_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG,
> +	regmap_write_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG,
>  			   SUN4I_I2S_FIFO_CTRL_FLUSH_TX,
>  			   SUN4I_I2S_FIFO_CTRL_FLUSH_TX);

Ditto.

>  
> @@ -1096,13 +1096,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;
>  }

You seem to be allowing reads to FIFO_TX_REG now for some reason?

>  static bool sun4i_i2s_wr_reg(struct device *dev, unsigned int reg)
> @@ -1121,6 +1115,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:

I assume that your issue was that the flushed bit got cached since the register
wasn't volatile, and therefore each time we were supposed to flush, we actually
ended up doing nothing. Marking it as volatile would prevent the cache to catch
that write and make regmap_update_bits work, actually fixing what you mention in
the commit log.

Either way, it should be in the log itself, and it doesn't really explain the
rest of the patch either.

Maxime
Maxime Ripard April 20, 2020, 12:44 p.m. UTC | #2 
On Sun, Apr 19, 2020 at 12:44:32AM +0200, Clément Péron wrote:
> From: Marcus Cooper <codekipper@gmail.com>
> 
> On the newer SoCs such as the H3 and A64 this is set by default
> to transfer a 0 after each sample in each slot. However the A10
> and A20 SoCs that this driver was developed on had a default
> setting where it padded the audio gain with zeros.
> 
> This isn't a problem whilst we have only support for 16bit audio
> but with larger sample resolution rates in the pipeline then SEXT
> bits should be cleared so that they also pad at the LSB. Without
> this the audio gets distorted.
> 
> Signed-off-by: Marcus Cooper <codekipper@gmail.com>
> Signed-off-by: Clément Péron <peron.clem@gmail.com>
> ---
>  sound/soc/sunxi/sun4i-i2s.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> index a23c9f2a3f8c..618bbc5156f1 100644
> --- a/sound/soc/sunxi/sun4i-i2s.c
> +++ b/sound/soc/sunxi/sun4i-i2s.c
> @@ -48,6 +48,9 @@
>  #define SUN4I_I2S_FMT0_FMT_I2S				(0 << 0)
>  
>  #define SUN4I_I2S_FMT1_REG		0x08
> +#define SUN4I_I2S_FMT1_REG_SEXT_MASK		BIT(8)
> +#define SUN4I_I2S_FMT1_REG_SEXT(sext)			((sext) << 8)
> +
>  #define SUN4I_I2S_FIFO_TX_REG		0x0c
>  #define SUN4I_I2S_FIFO_RX_REG		0x10
>  
> @@ -105,6 +108,9 @@
>  #define SUN8I_I2S_FMT0_BCLK_POLARITY_INVERTED		(1 << 7)
>  #define SUN8I_I2S_FMT0_BCLK_POLARITY_NORMAL		(0 << 7)
>  
> +#define SUN8I_I2S_FMT1_REG_SEXT_MASK		GENMASK(5,4)
> +#define SUN8I_I2S_FMT1_REG_SEXT(sext)			((sext) << 4)
> +
>  #define SUN8I_I2S_INT_STA_REG		0x0c
>  #define SUN8I_I2S_FIFO_TX_REG		0x20
>  
> @@ -663,6 +669,12 @@ static int sun4i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s,
>  	}
>  	regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
>  			   SUN4I_I2S_CTRL_MODE_MASK, val);
> +
> +	/* Set sign extension to pad out LSB with 0 */
> +	regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT1_REG,
> +			   SUN4I_I2S_FMT1_REG_SEXT_MASK,
> +			   SUN4I_I2S_FMT1_REG_SEXT(0));
> +
>  	return 0;
>  }
>  
> @@ -765,6 +777,11 @@ static int sun8i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s,
>  			   SUN8I_I2S_CTRL_BCLK_OUT | SUN8I_I2S_CTRL_LRCK_OUT,
>  			   val);
>  
> +	/* Set sign extension to pad out LSB with 0 */
> +	regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT1_REG,
> +			   SUN8I_I2S_FMT1_REG_SEXT_MASK,
> +			   SUN8I_I2S_FMT1_REG_SEXT(0));
> +
>  	return 0;
>  }
>  
> @@ -867,6 +884,11 @@ static int sun50i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s,
>  			   SUN8I_I2S_CTRL_BCLK_OUT | SUN8I_I2S_CTRL_LRCK_OUT,
>  			   val);
>  
> +	/* Set sign extension to pad out LSB with 0 */
> +	regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT1_REG,
> +			   SUN8I_I2S_FMT1_REG_SEXT_MASK,
> +			   SUN8I_I2S_FMT1_REG_SEXT(0));
> +

If this is an issue only on the A10 / A20, why are you setting it up on the
other generations too?

Maxime
Clément Péron April 20, 2020, 12:56 p.m. UTC | #3 
Hi Maxime,

On Mon, 20 Apr 2020 at 14:44, Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Sun, Apr 19, 2020 at 12:44:32AM +0200, Clément Péron wrote:
> > From: Marcus Cooper <codekipper@gmail.com>
> >
> > On the newer SoCs such as the H3 and A64 this is set by default
> > to transfer a 0 after each sample in each slot. However the A10
> > and A20 SoCs that this driver was developed on had a default
> > setting where it padded the audio gain with zeros.
> >
> > This isn't a problem whilst we have only support for 16bit audio
> > but with larger sample resolution rates in the pipeline then SEXT
> > bits should be cleared so that they also pad at the LSB. Without
> > this the audio gets distorted.
> >
> > Signed-off-by: Marcus Cooper <codekipper@gmail.com>
> > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > ---
> >  sound/soc/sunxi/sun4i-i2s.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> >
> > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> > index a23c9f2a3f8c..618bbc5156f1 100644
> > --- a/sound/soc/sunxi/sun4i-i2s.c
> > +++ b/sound/soc/sunxi/sun4i-i2s.c
> > @@ -48,6 +48,9 @@
> >  #define SUN4I_I2S_FMT0_FMT_I2S                               (0 << 0)
> >
> >  #define SUN4I_I2S_FMT1_REG           0x08
> > +#define SUN4I_I2S_FMT1_REG_SEXT_MASK         BIT(8)
> > +#define SUN4I_I2S_FMT1_REG_SEXT(sext)                        ((sext) << 8)
> > +
> >  #define SUN4I_I2S_FIFO_TX_REG                0x0c
> >  #define SUN4I_I2S_FIFO_RX_REG                0x10
> >
> > @@ -105,6 +108,9 @@
> >  #define SUN8I_I2S_FMT0_BCLK_POLARITY_INVERTED                (1 << 7)
> >  #define SUN8I_I2S_FMT0_BCLK_POLARITY_NORMAL          (0 << 7)
> >
> > +#define SUN8I_I2S_FMT1_REG_SEXT_MASK         GENMASK(5,4)
> > +#define SUN8I_I2S_FMT1_REG_SEXT(sext)                        ((sext) << 4)
> > +
> >  #define SUN8I_I2S_INT_STA_REG                0x0c
> >  #define SUN8I_I2S_FIFO_TX_REG                0x20
> >
> > @@ -663,6 +669,12 @@ static int sun4i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s,
> >       }
> >       regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> >                          SUN4I_I2S_CTRL_MODE_MASK, val);
> > +
> > +     /* Set sign extension to pad out LSB with 0 */
> > +     regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT1_REG,
> > +                        SUN4I_I2S_FMT1_REG_SEXT_MASK,
> > +                        SUN4I_I2S_FMT1_REG_SEXT(0));
> > +
> >       return 0;
> >  }
> >
> > @@ -765,6 +777,11 @@ static int sun8i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s,
> >                          SUN8I_I2S_CTRL_BCLK_OUT | SUN8I_I2S_CTRL_LRCK_OUT,
> >                          val);
> >
> > +     /* Set sign extension to pad out LSB with 0 */
> > +     regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT1_REG,
> > +                        SUN8I_I2S_FMT1_REG_SEXT_MASK,
> > +                        SUN8I_I2S_FMT1_REG_SEXT(0));
> > +
> >       return 0;
> >  }
> >
> > @@ -867,6 +884,11 @@ static int sun50i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s,
> >                          SUN8I_I2S_CTRL_BCLK_OUT | SUN8I_I2S_CTRL_LRCK_OUT,
> >                          val);
> >
> > +     /* Set sign extension to pad out LSB with 0 */
> > +     regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT1_REG,
> > +                        SUN8I_I2S_FMT1_REG_SEXT_MASK,
> > +                        SUN8I_I2S_FMT1_REG_SEXT(0));
> > +
>
> If this is an issue only on the A10 / A20, why are you setting it up on the
> other generations too?

To keep coherency between all set_soc_format(), and also avoid this
kind of issue for future generation.
As this doesn't cost much after all, and it avoid to rely on default,
but what do you think ?

Clement

>
> Maxime
Maxime Ripard April 21, 2020, 8:50 a.m. UTC | #4 
On Mon, Apr 20, 2020 at 02:56:31PM +0200, Clément Péron wrote:
> On Mon, 20 Apr 2020 at 14:44, Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > On Sun, Apr 19, 2020 at 12:44:32AM +0200, Clément Péron wrote:
> > > From: Marcus Cooper <codekipper@gmail.com>
> > >
> > > On the newer SoCs such as the H3 and A64 this is set by default
> > > to transfer a 0 after each sample in each slot. However the A10
> > > and A20 SoCs that this driver was developed on had a default
> > > setting where it padded the audio gain with zeros.
> > >
> > > This isn't a problem whilst we have only support for 16bit audio
> > > but with larger sample resolution rates in the pipeline then SEXT
> > > bits should be cleared so that they also pad at the LSB. Without
> > > this the audio gets distorted.
> > >
> > > Signed-off-by: Marcus Cooper <codekipper@gmail.com>
> > > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > > ---
> > >  sound/soc/sunxi/sun4i-i2s.c | 22 ++++++++++++++++++++++
> > >  1 file changed, 22 insertions(+)
> > >
> > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> > > index a23c9f2a3f8c..618bbc5156f1 100644
> > > --- a/sound/soc/sunxi/sun4i-i2s.c
> > > +++ b/sound/soc/sunxi/sun4i-i2s.c
> > > @@ -48,6 +48,9 @@
> > >  #define SUN4I_I2S_FMT0_FMT_I2S                               (0 << 0)
> > >
> > >  #define SUN4I_I2S_FMT1_REG           0x08
> > > +#define SUN4I_I2S_FMT1_REG_SEXT_MASK         BIT(8)
> > > +#define SUN4I_I2S_FMT1_REG_SEXT(sext)                        ((sext) << 8)
> > > +
> > >  #define SUN4I_I2S_FIFO_TX_REG                0x0c
> > >  #define SUN4I_I2S_FIFO_RX_REG                0x10
> > >
> > > @@ -105,6 +108,9 @@
> > >  #define SUN8I_I2S_FMT0_BCLK_POLARITY_INVERTED                (1 << 7)
> > >  #define SUN8I_I2S_FMT0_BCLK_POLARITY_NORMAL          (0 << 7)
> > >
> > > +#define SUN8I_I2S_FMT1_REG_SEXT_MASK         GENMASK(5,4)
> > > +#define SUN8I_I2S_FMT1_REG_SEXT(sext)                        ((sext) << 4)
> > > +
> > >  #define SUN8I_I2S_INT_STA_REG                0x0c
> > >  #define SUN8I_I2S_FIFO_TX_REG                0x20
> > >
> > > @@ -663,6 +669,12 @@ static int sun4i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s,
> > >       }
> > >       regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > >                          SUN4I_I2S_CTRL_MODE_MASK, val);
> > > +
> > > +     /* Set sign extension to pad out LSB with 0 */
> > > +     regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT1_REG,
> > > +                        SUN4I_I2S_FMT1_REG_SEXT_MASK,
> > > +                        SUN4I_I2S_FMT1_REG_SEXT(0));
> > > +
> > >       return 0;
> > >  }
> > >
> > > @@ -765,6 +777,11 @@ static int sun8i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s,
> > >                          SUN8I_I2S_CTRL_BCLK_OUT | SUN8I_I2S_CTRL_LRCK_OUT,
> > >                          val);
> > >
> > > +     /* Set sign extension to pad out LSB with 0 */
> > > +     regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT1_REG,
> > > +                        SUN8I_I2S_FMT1_REG_SEXT_MASK,
> > > +                        SUN8I_I2S_FMT1_REG_SEXT(0));
> > > +
> > >       return 0;
> > >  }
> > >
> > > @@ -867,6 +884,11 @@ static int sun50i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s,
> > >                          SUN8I_I2S_CTRL_BCLK_OUT | SUN8I_I2S_CTRL_LRCK_OUT,
> > >                          val);
> > >
> > > +     /* Set sign extension to pad out LSB with 0 */
> > > +     regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT1_REG,
> > > +                        SUN8I_I2S_FMT1_REG_SEXT_MASK,
> > > +                        SUN8I_I2S_FMT1_REG_SEXT(0));
> > > +
> >
> > If this is an issue only on the A10 / A20, why are you setting it up on the
> > other generations too?
> 
> To keep coherency between all set_soc_format(), and also avoid this
> kind of issue for future generation.
> As this doesn't cost much after all, and it avoid to rely on default,
> but what do you think ?

It makes sense, but it should be in the commit log ;)

Maxime