mbox series

[v3,00/19] Add Allwinner H3/H5/H6/A64 HDMI audio

Message ID 20200920180758.592217-1-peron.clem@gmail.com
Headers show
Series Add Allwinner H3/H5/H6/A64 HDMI audio | expand

Message

Clément Péron Sept. 20, 2020, 6:07 p.m. UTC 
Hi,

New test done by Maxime using TDM show that's LRCK is indeed inverted
so I drop the patch reverted in v2.

And HDMI requires an inverted LRCK so let's readd the frame-inversion
in the device-tree.

I have also added a patch to change set_chan_cfg.

Please note that I can't test TDM and only have a Allwinner H6.
So test and comment on other Allwinner chips are welcome!

Regards,
Clement

Change since v2:
- rebase on next-20200918
- drop revert LRCK polarity patch
- readd simple-audio-card,frame-inversion in dts
- Add patch for changing set_chan_cfg params

Change since v1:
- rebase on next-20200828
- add revert LRCK polarity
- remove all simple-audio-card,frame-inversion in dts
- add Ondrej patches for Orange Pi board
- Add arm64 defconfig patch

Clément Péron (4):
  ASoC: sun4i-i2s: Change set_chan_cfg params
  ASoC: sun4i-i2s: Fix sun8i volatile regs
  arm64: dts: allwinner: h6: Enable HDMI sound for Beelink GS1
  arm64: defconfig: Enable Allwinner i2s driver

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

Marcus Cooper (9):
  ASoC: sun4i-i2s: Set sign extend sample
  ASoc: sun4i-i2s: Add 20 and 24 bit support
  arm: dts: sunxi: h3/h5: Add DAI node for HDMI
  arm: dts: sunxi: h3/h5: Add HDMI audio
  arm64: dts: allwinner: a64: Add DAI node for HDMI
  arm64: dts: allwinner: a64: Add HDMI audio
  arm: sun8i: h3: Add HDMI audio to Orange Pi 2
  arm: sun8i: h3: Add HDMI audio to Beelink X2
  arm64: dts: allwinner: a64: Add HDMI audio to Pine64

Ondrej Jirman (3):
  arm64: dts: allwinner: Enable HDMI audio on Orange Pi PC 2
  ARM: dts: sun8i-h3: Enable HDMI audio on Orange Pi PC/One
  arm64: dts: sun50i-h6-orangepi-3: Enable HDMI audio

 .../sound/allwinner,sun4i-a10-i2s.yaml        |   2 +
 arch/arm/boot/dts/sun8i-h3-beelink-x2.dts     |   8 +
 arch/arm/boot/dts/sun8i-h3-orangepi-2.dts     |   8 +
 arch/arm/boot/dts/sun8i-h3-orangepi-one.dts   |   8 +
 arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts    |   8 +
 arch/arm/boot/dts/sunxi-h3-h5.dtsi            |  33 +++
 .../boot/dts/allwinner/sun50i-a64-pine64.dts  |   8 +
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi |  34 +++
 .../dts/allwinner/sun50i-h5-orangepi-pc2.dts  |   8 +
 .../dts/allwinner/sun50i-h6-beelink-gs1.dts   |   8 +
 .../dts/allwinner/sun50i-h6-orangepi-3.dts    |   8 +
 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi  |  33 +++
 arch/arm64/configs/defconfig                  |   1 +
 sound/soc/sunxi/sun4i-i2s.c                   | 280 ++++++++++++++++--
 14 files changed, 427 insertions(+), 20 deletions(-)

Comments

Samuel Holland Sept. 20, 2020, 6:39 p.m. UTC | #1 
On 9/20/20 1:07 PM, Clément Péron wrote:
> From: Jernej Skrabec <jernej.skrabec@siol.net>
> 
> H6 I2S is very similar to that in H3, except it supports up to 16
> channels.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> 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 | 218 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 218 insertions(+)
> 
> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> index f23ff29e7c1d..348057464bed 100644
> --- a/sound/soc/sunxi/sun4i-i2s.c
> +++ b/sound/soc/sunxi/sun4i-i2s.c
...
> @@ -699,6 +770,102 @@ static int sun8i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s,
>  	return 0;
>  }
>  
> +static int sun50i_h6_i2s_set_soc_fmt(const struct sun4i_i2s *i2s,
> +				     unsigned int fmt)
> +{
> +	u32 mode, val;
> +	u8 offset;
> +
> +	/* DAI clock polarity */
> +	switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
> +	case SND_SOC_DAIFMT_IB_IF:
> +		/* Invert both clocks */
> +		val = SUN8I_I2S_FMT0_BCLK_POLARITY_INVERTED |
> +		      SUN8I_I2S_FMT0_LRCLK_POLARITY_INVERTED;
> +		break;
> +	case SND_SOC_DAIFMT_IB_NF:
> +		/* Invert bit clock */
> +		val = SUN8I_I2S_FMT0_BCLK_POLARITY_INVERTED;
> +		break;
> +	case SND_SOC_DAIFMT_NB_IF:
> +		/* Invert frame clock */
> +		val = SUN8I_I2S_FMT0_LRCLK_POLARITY_INVERTED;
> +		break;
> +	case SND_SOC_DAIFMT_NB_NF:
> +		val = 0;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

Maxime's testing that showed LRCK inversion was necessary was done on the H6. So
in addition to dropping the patch that removed the LRCK inversion for other
sun8i variants, you need to re-add it to this patch for the H6 variant.

Cheers,
Samuel

> +
> +	regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT0_REG,
> +			   SUN8I_I2S_FMT0_LRCLK_POLARITY_MASK |
> +			   SUN8I_I2S_FMT0_BCLK_POLARITY_MASK,
> +			   val);
> +
> +	/* DAI Mode */
> +	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
> +	case SND_SOC_DAIFMT_DSP_A:
> +		mode = SUN8I_I2S_CTRL_MODE_PCM;
> +		offset = 1;
> +		break;
> +
> +	case SND_SOC_DAIFMT_DSP_B:
> +		mode = SUN8I_I2S_CTRL_MODE_PCM;
> +		offset = 0;
> +		break;
> +
> +	case SND_SOC_DAIFMT_I2S:
> +		mode = SUN8I_I2S_CTRL_MODE_LEFT;
> +		offset = 1;
> +		break;
> +
> +	case SND_SOC_DAIFMT_LEFT_J:
> +		mode = SUN8I_I2S_CTRL_MODE_LEFT;
> +		offset = 0;
> +		break;
> +
> +	case SND_SOC_DAIFMT_RIGHT_J:
> +		mode = SUN8I_I2S_CTRL_MODE_RIGHT;
> +		offset = 0;
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> +			   SUN8I_I2S_CTRL_MODE_MASK, mode);
> +	regmap_update_bits(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG,
> +			   SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET_MASK,
> +			   SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET(offset));
> +	regmap_update_bits(i2s->regmap, SUN50I_H6_I2S_RX_CHAN_SEL_REG,
> +			   SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET_MASK,
> +			   SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET(offset));
> +
> +	/* DAI clock master masks */
> +	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> +	case SND_SOC_DAIFMT_CBS_CFS:
> +		/* BCLK and LRCLK master */
> +		val = SUN8I_I2S_CTRL_BCLK_OUT |	SUN8I_I2S_CTRL_LRCK_OUT;
> +		break;
> +
> +	case SND_SOC_DAIFMT_CBM_CFM:
> +		/* BCLK and LRCLK slave */
> +		val = 0;
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> +			   SUN8I_I2S_CTRL_BCLK_OUT | SUN8I_I2S_CTRL_LRCK_OUT,
> +			   val);
> +
> +	return 0;
> +}
> +
>  static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
>  {
>  	struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
...
Samuel Holland Sept. 20, 2020, 6:45 p.m. UTC | #2 
On 9/20/20 1:07 PM, Clément Péron wrote:
> From: Marcus Cooper <codekipper@gmail.com>
> 
> Extend the functionality of the driver to include support of 20 and
> 24 bits per sample.
> 
> Signed-off-by: Marcus Cooper <codekipper@gmail.com>
> Signed-off-by: Clément Péron <peron.clem@gmail.com>
> Acked-by: Maxime Ripard <mripard@kernel.org>
> ---
>  sound/soc/sunxi/sun4i-i2s.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
As I have mentioned before, if you want to support a 32-bit slot width on sun4i
variants (which patch 2 does via TDM and this patch does via PCM format), you
need to fix sun4i_i2s_get_wss() to return "3", not "4", for a 32-bit input.

Cheers,
Samuel
Samuel Holland Sept. 20, 2020, 6:52 p.m. UTC | #3 
On 9/20/20 1:07 PM, Clément Péron wrote:
> The FIFO TX reg is volatile and sun8i i2s register
> mapping is different from sun4i.
> 
> Even if in this case it's doesn't create an issue,
> Avoid setting some regs that are undefined in sun8i.
> 
> Signed-off-by: Clément Péron <peron.clem@gmail.com>
> Acked-by: Maxime Ripard <mripard@kernel.org>
> ---
>  sound/soc/sunxi/sun4i-i2s.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> index ce4913f0ffe4..a35be0e2baf5 100644
> --- a/sound/soc/sunxi/sun4i-i2s.c
> +++ b/sound/soc/sunxi/sun4i-i2s.c
> @@ -1126,12 +1126,19 @@ static bool sun8i_i2s_rd_reg(struct device *dev, unsigned int reg)
>  
>  static bool sun8i_i2s_volatile_reg(struct device *dev, unsigned int reg)
>  {
> -	if (reg == SUN8I_I2S_INT_STA_REG)
> +	switch (reg) {
> +	case SUN4I_I2S_FIFO_CTRL_REG:

Please check if this breaks audio recording with runtime PM enabled. I noticed
this with an older revision of the series that also changed
sun4i_i2s_volatile_reg. Marking SUN4I_I2S_FIFO_CTRL_REG as volatile broke
setting of SUN4I_I2S_FIFO_CTRL_TX_MODE/RX_MODE, because the set_fmt() callback
is not run with a runtime PM reference held, and volatile registers are not
written by regcache_sync() during sun4i_i2s_runtime_resume().

As a workaround, I moved the TX_MODE/RX_MODE initialization to hw_params(), but
I am not sure it is the right thing to do:

https://github.com/smaeul/linux/commit/5e40ac610986.patch

Cheers,
Samuel

> +	case SUN4I_I2S_FIFO_RX_REG:
> +	case SUN4I_I2S_FIFO_STA_REG:
> +	case SUN4I_I2S_RX_CNT_REG:
> +	case SUN4I_I2S_TX_CNT_REG:
> +	case SUN8I_I2S_FIFO_TX_REG:
> +	case SUN8I_I2S_INT_STA_REG:
>  		return true;
> -	if (reg == SUN8I_I2S_FIFO_TX_REG)
> -		return false;
>  
> -	return sun4i_i2s_volatile_reg(dev, reg);
> +	default:
> +		return false;
> +	}
>  }
>  
>  static const struct reg_default sun4i_i2s_reg_defaults[] = {
>
Clément Péron Sept. 20, 2020, 7:38 p.m. UTC | #4 
Hi Samuel,

On Sun, 20 Sep 2020 at 20:39, Samuel Holland <samuel@sholland.org> wrote:
>
> On 9/20/20 1:07 PM, Clément Péron wrote:
> > From: Jernej Skrabec <jernej.skrabec@siol.net>
> >
> > H6 I2S is very similar to that in H3, except it supports up to 16
> > channels.
> >
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > 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 | 218 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 218 insertions(+)
> >
> > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> > index f23ff29e7c1d..348057464bed 100644
> > --- a/sound/soc/sunxi/sun4i-i2s.c
> > +++ b/sound/soc/sunxi/sun4i-i2s.c
> ...
> > @@ -699,6 +770,102 @@ static int sun8i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s,
> >       return 0;
> >  }
> >
> > +static int sun50i_h6_i2s_set_soc_fmt(const struct sun4i_i2s *i2s,
> > +                                  unsigned int fmt)
> > +{
> > +     u32 mode, val;
> > +     u8 offset;
> > +
> > +     /* DAI clock polarity */
> > +     switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
> > +     case SND_SOC_DAIFMT_IB_IF:
> > +             /* Invert both clocks */
> > +             val = SUN8I_I2S_FMT0_BCLK_POLARITY_INVERTED |
> > +                   SUN8I_I2S_FMT0_LRCLK_POLARITY_INVERTED;
> > +             break;
> > +     case SND_SOC_DAIFMT_IB_NF:
> > +             /* Invert bit clock */
> > +             val = SUN8I_I2S_FMT0_BCLK_POLARITY_INVERTED;
> > +             break;
> > +     case SND_SOC_DAIFMT_NB_IF:
> > +             /* Invert frame clock */
> > +             val = SUN8I_I2S_FMT0_LRCLK_POLARITY_INVERTED;
> > +             break;
> > +     case SND_SOC_DAIFMT_NB_NF:
> > +             val = 0;
> > +             break;
> > +     default:
> > +             return -EINVAL;
> > +     }
>
> Maxime's testing that showed LRCK inversion was necessary was done on the H6. So
> in addition to dropping the patch that removed the LRCK inversion for other
> sun8i variants, you need to re-add it to this patch for the H6 variant.

Thanks, you're right!
Clement

>
> Cheers,
> Samuel
>
> > +
> > +     regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT0_REG,
> > +                        SUN8I_I2S_FMT0_LRCLK_POLARITY_MASK |
> > +                        SUN8I_I2S_FMT0_BCLK_POLARITY_MASK,
> > +                        val);
> > +
> > +     /* DAI Mode */
> > +     switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
> > +     case SND_SOC_DAIFMT_DSP_A:
> > +             mode = SUN8I_I2S_CTRL_MODE_PCM;
> > +             offset = 1;
> > +             break;
> > +
> > +     case SND_SOC_DAIFMT_DSP_B:
> > +             mode = SUN8I_I2S_CTRL_MODE_PCM;
> > +             offset = 0;
> > +             break;
> > +
> > +     case SND_SOC_DAIFMT_I2S:
> > +             mode = SUN8I_I2S_CTRL_MODE_LEFT;
> > +             offset = 1;
> > +             break;
> > +
> > +     case SND_SOC_DAIFMT_LEFT_J:
> > +             mode = SUN8I_I2S_CTRL_MODE_LEFT;
> > +             offset = 0;
> > +             break;
> > +
> > +     case SND_SOC_DAIFMT_RIGHT_J:
> > +             mode = SUN8I_I2S_CTRL_MODE_RIGHT;
> > +             offset = 0;
> > +             break;
> > +
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +
> > +     regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > +                        SUN8I_I2S_CTRL_MODE_MASK, mode);
> > +     regmap_update_bits(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG,
> > +                        SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET_MASK,
> > +                        SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET(offset));
> > +     regmap_update_bits(i2s->regmap, SUN50I_H6_I2S_RX_CHAN_SEL_REG,
> > +                        SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET_MASK,
> > +                        SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET(offset));
> > +
> > +     /* DAI clock master masks */
> > +     switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> > +     case SND_SOC_DAIFMT_CBS_CFS:
> > +             /* BCLK and LRCLK master */
> > +             val = SUN8I_I2S_CTRL_BCLK_OUT | SUN8I_I2S_CTRL_LRCK_OUT;
> > +             break;
> > +
> > +     case SND_SOC_DAIFMT_CBM_CFM:
> > +             /* BCLK and LRCLK slave */
> > +             val = 0;
> > +             break;
> > +
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +
> > +     regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > +                        SUN8I_I2S_CTRL_BCLK_OUT | SUN8I_I2S_CTRL_LRCK_OUT,
> > +                        val);
> > +
> > +     return 0;
> > +}
> > +
> >  static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
> >  {
> >       struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> ...
Clément Péron Sept. 20, 2020, 8:05 p.m. UTC | #5 
Hi Samuel,

On Sun, 20 Sep 2020 at 20:52, Samuel Holland <samuel@sholland.org> wrote:
>
> On 9/20/20 1:07 PM, Clément Péron wrote:
> > The FIFO TX reg is volatile and sun8i i2s register
> > mapping is different from sun4i.
> >
> > Even if in this case it's doesn't create an issue,
> > Avoid setting some regs that are undefined in sun8i.
> >
> > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > Acked-by: Maxime Ripard <mripard@kernel.org>
> > ---
> >  sound/soc/sunxi/sun4i-i2s.c | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> > index ce4913f0ffe4..a35be0e2baf5 100644
> > --- a/sound/soc/sunxi/sun4i-i2s.c
> > +++ b/sound/soc/sunxi/sun4i-i2s.c
> > @@ -1126,12 +1126,19 @@ static bool sun8i_i2s_rd_reg(struct device *dev, unsigned int reg)
> >
> >  static bool sun8i_i2s_volatile_reg(struct device *dev, unsigned int reg)
> >  {
> > -     if (reg == SUN8I_I2S_INT_STA_REG)
> > +     switch (reg) {
> > +     case SUN4I_I2S_FIFO_CTRL_REG:
>
> Please check if this breaks audio recording with runtime PM enabled. I noticed
> this with an older revision of the series that also changed
> sun4i_i2s_volatile_reg. Marking SUN4I_I2S_FIFO_CTRL_REG as volatile broke
> setting of SUN4I_I2S_FIFO_CTRL_TX_MODE/RX_MODE, because the set_fmt() callback
> is not run with a runtime PM reference held, and volatile registers are not
> written by regcache_sync() during sun4i_i2s_runtime_resume().
>
> As a workaround, I moved the TX_MODE/RX_MODE initialization to hw_params(), but
> I am not sure it is the right thing to do:

Thanks for the catch,
I never tried to suspend/resume my board actually.
But your explanation and the fix seems legit to me.

I don't think it's a workaround as settings the fifo size is not
related to set_fmt and could also be set in hw_params.

I will add your fix in the next version.

Regards,
Clement

>
> https://github.com/smaeul/linux/commit/5e40ac610986.patch
>
> Cheers,
> Samuel
>
> > +     case SUN4I_I2S_FIFO_RX_REG:
> > +     case SUN4I_I2S_FIFO_STA_REG:
> > +     case SUN4I_I2S_RX_CNT_REG:
> > +     case SUN4I_I2S_TX_CNT_REG:
> > +     case SUN8I_I2S_FIFO_TX_REG:
> > +     case SUN8I_I2S_INT_STA_REG:
> >               return true;
> > -     if (reg == SUN8I_I2S_FIFO_TX_REG)
> > -             return false;
> >
> > -     return sun4i_i2s_volatile_reg(dev, reg);
> > +     default:
> > +             return false;
> > +     }
> >  }
> >
> >  static const struct reg_default sun4i_i2s_reg_defaults[] = {
> >
>
Clément Péron Sept. 20, 2020, 9:32 p.m. UTC | #6 
Hi Samuel,

On Sun, 20 Sep 2020 at 20:45, Samuel Holland <samuel@sholland.org> wrote:
>
> On 9/20/20 1:07 PM, Clément Péron wrote:
> > From: Marcus Cooper <codekipper@gmail.com>
> >
> > Extend the functionality of the driver to include support of 20 and
> > 24 bits per sample.
> >
> > Signed-off-by: Marcus Cooper <codekipper@gmail.com>
> > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > Acked-by: Maxime Ripard <mripard@kernel.org>
> > ---
> >  sound/soc/sunxi/sun4i-i2s.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >
> As I have mentioned before, if you want to support a 32-bit slot width on sun4i
> variants (which patch 2 does via TDM and this patch does via PCM format), you
> need to fix sun4i_i2s_get_wss() to return "3", not "4", for a 32-bit input.

Sorry I didn't get it the first time.

Is using a switch case is a correct solution?

static s8 sun4i_i2s_get_wss(const struct sun4i_i2s *i2s, int width)
{
switch (width)
{
case 16:
return 0x0;
case 20:
return 0x1;
case 24:
return 0x2;
case 32:
return 0x3;
}

return -EINVAL;
}

Clement

>
> Cheers,
> Samuel