[PATCH 00/16] Add Allwinner H3/H5/H6/A64 HDMI audio

[PATCH 00/16] Add Allwinner H3/H5/H6/A64 HDMI audio

[Clément Péron]

Hi,

This a merge of serie:
- Add Add H6 I2S support https://patchwork.kernel.org/cover/11497007/
- Add Allwinner H3/H5/A64 HDMI audio https://patchwork.kernel.org/cover/11510511/

I merge both serie because there is a similar issue regarding the I2S polarity.
This need to be investigated under a scope and see if the comment is still true
for Allwinner H6 and previous SoC.
LibreElec team found that we have to introduce the
"simple-audio-card,frame-inversion" property with recent mainline change.
In order to make HDMI audio work.
Maybe the I2S polarity is good or maybe the silicium has an issue but not present
for HDMI I2S or not present in TDM mode...
I cannot do it myself, so if someone want to to do it please feel free.

Regarding the discussion we had here: https://patchwork.kernel.org/patch/11510521/
I didn't switch to generic hdmi card-name and used name like:
sun8i-h3-hdmi, sun50i-h6-hdmi, etc....

Despite this wrong comment the rest of the serie introduce some fix that should
be merged even if it's without the H6 support.

Regards
Clement

Change since v1:
- drop allwinner,playback-channels property
- use coherent hdmi,card-name 
- indentation fix
- collect tags

Clément Péron (2):
  ASoC: sun4i-i2s: Fix sun8i volatile regs
  arm64: dts: allwinner: h6: Enable HDMI sound for Beelink GS1

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 (11):
  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
  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

 .../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/sunxi-h3-h5.dtsi            |  33 ++
 .../boot/dts/allwinner/sun50i-a64-pine64.dts  |   8 +
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi |  35 +++
 .../dts/allwinner/sun50i-h6-beelink-gs1.dts   |   8 +
 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi  |  33 ++
 sound/soc/sunxi/sun4i-i2s.c                   | 281 +++++++++++++++++-
 9 files changed, 410 insertions(+), 6 deletions(-)

-- 
2.25.1

[PATCH 01/16] ASoC: sun4i-i2s: Add support for H6 I2S

[Clément Péron]

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 | 227 ++++++++++++++++++++++++++++++++++++
 1 file changed, 227 insertions(+)

diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
index d0a8d5810c0a..9690389cb68e 100644
--- a/sound/soc/sunxi/sun4i-i2s.c
+++ b/sound/soc/sunxi/sun4i-i2s.c
@@ -124,6 +124,21 @@
 #define SUN8I_I2S_RX_CHAN_SEL_REG	0x54
 #define SUN8I_I2S_RX_CHAN_MAP_REG	0x58
 
+/* Defines required for sun50i-h6 support */
+#define SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET_MASK	GENMASK(21, 20)
+#define SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET(offset)	((offset) << 20)
+#define SUN50I_H6_I2S_TX_CHAN_SEL_MASK		GENMASK(19, 16)
+#define SUN50I_H6_I2S_TX_CHAN_SEL(chan)		((chan - 1) << 16)
+#define SUN50I_H6_I2S_TX_CHAN_EN_MASK		GENMASK(15, 0)
+#define SUN50I_H6_I2S_TX_CHAN_EN(num_chan)	(((1 << num_chan) - 1))
+
+#define SUN50I_H6_I2S_TX_CHAN_MAP0_REG	0x44
+#define SUN50I_H6_I2S_TX_CHAN_MAP1_REG	0x48
+
+#define SUN50I_H6_I2S_RX_CHAN_SEL_REG	0x64
+#define SUN50I_H6_I2S_RX_CHAN_MAP0_REG	0x68
+#define SUN50I_H6_I2S_RX_CHAN_MAP1_REG	0x6C
+
 struct sun4i_i2s;
 
 /**
@@ -466,6 +481,65 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
 	return 0;
 }
 
+static int sun50i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
+				   const struct snd_pcm_hw_params *params)
+{
+	unsigned int channels = params_channels(params);
+	unsigned int slots = channels;
+	unsigned int lrck_period;
+
+	if (i2s->slots)
+		slots = i2s->slots;
+
+	/* Map the channels for playback and capture */
+	regmap_write(i2s->regmap, SUN50I_H6_I2S_TX_CHAN_MAP1_REG, 0x76543210);
+	regmap_write(i2s->regmap, SUN50I_H6_I2S_RX_CHAN_MAP1_REG, 0x76543210);
+
+	/* Configure the channels */
+	regmap_update_bits(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG,
+			   SUN50I_H6_I2S_TX_CHAN_SEL_MASK,
+			   SUN50I_H6_I2S_TX_CHAN_SEL(channels));
+	regmap_update_bits(i2s->regmap, SUN50I_H6_I2S_RX_CHAN_SEL_REG,
+			   SUN50I_H6_I2S_TX_CHAN_SEL_MASK,
+			   SUN50I_H6_I2S_TX_CHAN_SEL(channels));
+
+	regmap_update_bits(i2s->regmap, SUN8I_I2S_CHAN_CFG_REG,
+			   SUN8I_I2S_CHAN_CFG_TX_SLOT_NUM_MASK,
+			   SUN8I_I2S_CHAN_CFG_TX_SLOT_NUM(channels));
+	regmap_update_bits(i2s->regmap, SUN8I_I2S_CHAN_CFG_REG,
+			   SUN8I_I2S_CHAN_CFG_RX_SLOT_NUM_MASK,
+			   SUN8I_I2S_CHAN_CFG_RX_SLOT_NUM(channels));
+
+	switch (i2s->format & SND_SOC_DAIFMT_FORMAT_MASK) {
+	case SND_SOC_DAIFMT_DSP_A:
+	case SND_SOC_DAIFMT_DSP_B:
+	case SND_SOC_DAIFMT_LEFT_J:
+	case SND_SOC_DAIFMT_RIGHT_J:
+		lrck_period = params_physical_width(params) * slots;
+		break;
+
+	case SND_SOC_DAIFMT_I2S:
+		lrck_period = params_physical_width(params);
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	if (i2s->slot_width)
+		lrck_period = i2s->slot_width;
+
+	regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT0_REG,
+			   SUN8I_I2S_FMT0_LRCK_PERIOD_MASK,
+			   SUN8I_I2S_FMT0_LRCK_PERIOD(lrck_period));
+
+	regmap_update_bits(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG,
+			   SUN50I_H6_I2S_TX_CHAN_EN_MASK,
+			   SUN50I_H6_I2S_TX_CHAN_EN(channels));
+
+	return 0;
+}
+
 static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
 			       struct snd_pcm_hw_params *params,
 			       struct snd_soc_dai *dai)
@@ -691,6 +765,108 @@ static int sun8i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s,
 	return 0;
 }
 
+static int sun50i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s,
+				  unsigned int fmt)
+{
+	u32 mode, val;
+	u8 offset;
+
+	/*
+	 * DAI clock polarity
+	 *
+	 * The setup for LRCK contradicts the datasheet, but under a
+	 * scope it's clear that the LRCK polarity is reversed
+	 * compared to the expected polarity on the bus.
+	 */
+	switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
+	case SND_SOC_DAIFMT_IB_IF:
+		/* Invert both clocks */
+		val = SUN8I_I2S_FMT0_BCLK_POLARITY_INVERTED;
+		break;
+	case SND_SOC_DAIFMT_IB_NF:
+		/* Invert bit clock */
+		val = SUN8I_I2S_FMT0_BCLK_POLARITY_INVERTED |
+		      SUN8I_I2S_FMT0_LRCLK_POLARITY_INVERTED;
+		break;
+	case SND_SOC_DAIFMT_NB_IF:
+		/* Invert frame clock */
+		val = 0;
+		break;
+	case SND_SOC_DAIFMT_NB_NF:
+		val = SUN8I_I2S_FMT0_LRCLK_POLARITY_INVERTED;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	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);
@@ -971,6 +1147,22 @@ static const struct reg_default sun8i_i2s_reg_defaults[] = {
 	{ SUN8I_I2S_RX_CHAN_MAP_REG, 0x00000000 },
 };
 
+static const struct reg_default sun50i_i2s_reg_defaults[] = {
+	{ SUN4I_I2S_CTRL_REG, 0x00060000 },
+	{ SUN4I_I2S_FMT0_REG, 0x00000033 },
+	{ SUN4I_I2S_FMT1_REG, 0x00000030 },
+	{ SUN4I_I2S_FIFO_CTRL_REG, 0x000400f0 },
+	{ SUN4I_I2S_DMA_INT_CTRL_REG, 0x00000000 },
+	{ SUN4I_I2S_CLK_DIV_REG, 0x00000000 },
+	{ SUN8I_I2S_CHAN_CFG_REG, 0x00000000 },
+	{ SUN8I_I2S_TX_CHAN_SEL_REG, 0x00000000 },
+	{ SUN50I_H6_I2S_TX_CHAN_MAP0_REG, 0x00000000 },
+	{ SUN50I_H6_I2S_TX_CHAN_MAP1_REG, 0x00000000 },
+	{ SUN50I_H6_I2S_RX_CHAN_SEL_REG, 0x00000000 },
+	{ SUN50I_H6_I2S_RX_CHAN_MAP0_REG, 0x00000000 },
+	{ SUN50I_H6_I2S_RX_CHAN_MAP1_REG, 0x00000000 },
+};
+
 static const struct regmap_config sun4i_i2s_regmap_config = {
 	.reg_bits	= 32,
 	.reg_stride	= 4,
@@ -998,6 +1190,19 @@ static const struct regmap_config sun8i_i2s_regmap_config = {
 	.volatile_reg	= sun8i_i2s_volatile_reg,
 };
 
+static const struct regmap_config sun50i_i2s_regmap_config = {
+	.reg_bits	= 32,
+	.reg_stride	= 4,
+	.val_bits	= 32,
+	.max_register	= SUN50I_H6_I2S_RX_CHAN_MAP1_REG,
+	.cache_type	= REGCACHE_FLAT,
+	.reg_defaults	= sun50i_i2s_reg_defaults,
+	.num_reg_defaults	= ARRAY_SIZE(sun50i_i2s_reg_defaults),
+	.writeable_reg	= sun4i_i2s_wr_reg,
+	.readable_reg	= sun8i_i2s_rd_reg,
+	.volatile_reg	= sun8i_i2s_volatile_reg,
+};
+
 static int sun4i_i2s_runtime_resume(struct device *dev)
 {
 	struct sun4i_i2s *i2s = dev_get_drvdata(dev);
@@ -1156,6 +1361,24 @@ static const struct sun4i_i2s_quirks sun50i_a64_codec_i2s_quirks = {
 	.set_fmt		= sun4i_i2s_set_soc_fmt,
 };
 
+static const struct sun4i_i2s_quirks sun50i_h6_i2s_quirks = {
+	.has_reset		= true,
+	.reg_offset_txdata	= SUN8I_I2S_FIFO_TX_REG,
+	.sun4i_i2s_regmap	= &sun50i_i2s_regmap_config,
+	.field_clkdiv_mclk_en	= REG_FIELD(SUN4I_I2S_CLK_DIV_REG, 8, 8),
+	.field_fmt_wss		= REG_FIELD(SUN4I_I2S_FMT0_REG, 0, 2),
+	.field_fmt_sr		= REG_FIELD(SUN4I_I2S_FMT0_REG, 4, 6),
+	.bclk_dividers		= sun8i_i2s_clk_div,
+	.num_bclk_dividers	= ARRAY_SIZE(sun8i_i2s_clk_div),
+	.mclk_dividers		= sun8i_i2s_clk_div,
+	.num_mclk_dividers	= ARRAY_SIZE(sun8i_i2s_clk_div),
+	.get_bclk_parent_rate	= sun8i_i2s_get_bclk_parent_rate,
+	.get_sr			= sun8i_i2s_get_sr_wss,
+	.get_wss		= sun8i_i2s_get_sr_wss,
+	.set_chan_cfg		= sun50i_i2s_set_chan_cfg,
+	.set_fmt		= sun50i_i2s_set_soc_fmt,
+};
+
 static int sun4i_i2s_init_regmap_fields(struct device *dev,
 					struct sun4i_i2s *i2s)
 {
@@ -1325,6 +1548,10 @@ static const struct of_device_id sun4i_i2s_match[] = {
 		.compatible = "allwinner,sun50i-a64-codec-i2s",
 		.data = &sun50i_a64_codec_i2s_quirks,
 	},
+	{
+		.compatible = "allwinner,sun50i-h6-i2s",
+		.data = &sun50i_h6_i2s_quirks,
+	},
 	{}
 };
 MODULE_DEVICE_TABLE(of, sun4i_i2s_match);
-- 
2.25.1

[PATCH 02/16] ASoC: sun4i-i2s: Adjust LRCLK width

[Clément Péron]

From: Marcus Cooper <codekipper@gmail.com>

Some codecs such as i2s based HDMI audio and the Pine64 DAC require
a different amount of bit clocks per frame than what is calculated
by the sample width. Use the values obtained by the tdm slot bindings
to adjust the LRCLK width accordingly.

Signed-off-by: Marcus Cooper <codekipper@gmail.com>
Signed-off-by: Clément Péron <peron.clem@gmail.com>
Acked-by: Maxime Ripard <maxime@cerno.tech>
---
 sound/soc/sunxi/sun4i-i2s.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
index 9690389cb68e..8bae97efea30 100644
--- a/sound/soc/sunxi/sun4i-i2s.c
+++ b/sound/soc/sunxi/sun4i-i2s.c
@@ -470,6 +470,9 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
 		return -EINVAL;
 	}
 
+	if (i2s->slot_width)
+		lrck_period = i2s->slot_width;
+
 	regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT0_REG,
 			   SUN8I_I2S_FMT0_LRCK_PERIOD_MASK,
 			   SUN8I_I2S_FMT0_LRCK_PERIOD(lrck_period));
-- 
2.25.1

[PATCH 03/16] dt-bindings: ASoC: sun4i-i2s: Add H6 compatible

[Clément Péron]

From: Jernej Skrabec <jernej.skrabec@siol.net>

H6 I2S is very similar to H3, except that it supports up to 16 channels
and thus few registers have fields on different position.

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>
Acked-by: Maxime Ripard <mripard@kernel.org>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/sound/allwinner,sun4i-a10-i2s.yaml      | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-i2s.yaml b/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-i2s.yaml
index 112ae00d63c1..606ad2d884a8 100644
--- a/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-i2s.yaml
+++ b/Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-i2s.yaml
@@ -24,6 +24,7 @@ properties:
       - items:
           - const: allwinner,sun50i-a64-i2s
           - const: allwinner,sun8i-h3-i2s
+      - const: allwinner,sun50i-h6-i2s
 
   reg:
     maxItems: 1
@@ -59,6 +60,7 @@ allOf:
               - allwinner,sun8i-a83t-i2s
               - allwinner,sun8i-h3-i2s
               - allwinner,sun50i-a64-codec-i2s
+              - allwinner,sun50i-h6-i2s
 
     then:
       required:
-- 
2.25.1

[PATCH 04/16] ASoC: sun4i-i2s: Set sign extend sample

[Clément Péron]

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 while 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.

Set sign extend sample for all the sunxi generations even if they
are not affected. This will keep coherency and avoid relying on
default.

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 8bae97efea30..f78167e152ce 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));
+
 	return 0;
 }
 
-- 
2.25.1

[PATCH 05/16] ASoc: sun4i-i2s: Add 20 and 24 bit support

[Clément Péron]

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>
---
 sound/soc/sunxi/sun4i-i2s.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
index f78167e152ce..bc7f9343bc7a 100644
--- a/sound/soc/sunxi/sun4i-i2s.c
+++ b/sound/soc/sunxi/sun4i-i2s.c
@@ -577,6 +577,9 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
 	case 16:
 		width = DMA_SLAVE_BUSWIDTH_2_BYTES;
 		break;
+	case 32:
+		width = DMA_SLAVE_BUSWIDTH_4_BYTES;
+		break;
 	default:
 		dev_err(dai->dev, "Unsupported physical sample width: %d\n",
 			params_physical_width(params));
@@ -1063,6 +1066,10 @@ static int sun4i_i2s_dai_probe(struct snd_soc_dai *dai)
 	return 0;
 }
 
+#define SUN4I_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | \
+			 SNDRV_PCM_FMTBIT_S20_LE | \
+			 SNDRV_PCM_FMTBIT_S24_LE)
+
 static struct snd_soc_dai_driver sun4i_i2s_dai = {
 	.probe = sun4i_i2s_dai_probe,
 	.capture = {
@@ -1070,14 +1077,14 @@ static struct snd_soc_dai_driver sun4i_i2s_dai = {
 		.channels_min = 1,
 		.channels_max = 8,
 		.rates = SNDRV_PCM_RATE_8000_192000,
-		.formats = SNDRV_PCM_FMTBIT_S16_LE,
+		.formats = SUN4I_FORMATS,
 	},
 	.playback = {
 		.stream_name = "Playback",
 		.channels_min = 1,
 		.channels_max = 8,
 		.rates = SNDRV_PCM_RATE_8000_192000,
-		.formats = SNDRV_PCM_FMTBIT_S16_LE,
+		.formats = SUN4I_FORMATS,
 	},
 	.ops = &sun4i_i2s_dai_ops,
 	.symmetric_rates = 1,
-- 
2.25.1

[PATCH 06/16] ASoC: sun4i-i2s: Adjust regmap settings

[Clément Péron]

From: Marcus Cooper <codekipper@gmail.com>

Bypass the regmap cache when flushing or reading the i2s FIFOs.

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 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
index bc7f9343bc7a..d7484c7e8fa2 100644
--- a/sound/soc/sunxi/sun4i-i2s.c
+++ b/sound/soc/sunxi/sun4i-i2s.c
@@ -1120,7 +1120,10 @@ static bool sun4i_i2s_wr_reg(struct device *dev, unsigned int reg)
 static bool sun4i_i2s_volatile_reg(struct device *dev, unsigned int reg)
 {
 	switch (reg) {
+	case SUN4I_I2S_FIFO_CTRL_REG:
 	case SUN4I_I2S_FIFO_RX_REG:
+	case SUN4I_I2S_FIFO_STA_REG:
+	case SUN4I_I2S_FIFO_TX_REG:
 	case SUN4I_I2S_INT_STA_REG:
 	case SUN4I_I2S_RX_CNT_REG:
 	case SUN4I_I2S_TX_CNT_REG:
-- 
2.25.1

[PATCH 07/16] ASoC: sun4i-i2s: Fix sun8i volatile regs

[Clément Péron]

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>
---
 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 d7484c7e8fa2..109c10296d83 100644
--- a/sound/soc/sunxi/sun4i-i2s.c
+++ b/sound/soc/sunxi/sun4i-i2s.c
@@ -1147,12 +1147,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:
+	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[] = {
-- 
2.25.1

[PATCH 08/16] arm64: dts: allwinner: h6: Add HDMI audio node

[Clément Péron]

From: Jernej Skrabec <jernej.skrabec@siol.net>

Add a simple-soundcard to link audio between HDMI and I2S.

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>
---
 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 33 ++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
index 78b1361dfbb9..ae169d07b939 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
@@ -67,6 +67,25 @@ de: display-engine {
 		status = "disabled";
 	};
 
+	hdmi_sound: hdmi-sound {
+		compatible = "simple-audio-card";
+		simple-audio-card,format = "i2s";
+		simple-audio-card,name = "sun50i-h6-hdmi";
+		simple-audio-card,mclk-fs = <128>;
+		simple-audio-card,frame-inversion;
+		status = "disabled";
+
+		simple-audio-card,codec {
+			sound-dai = <&hdmi>;
+		};
+
+		simple-audio-card,cpu {
+			sound-dai = <&i2s1>;
+			dai-tdm-slot-num = <2>;
+			dai-tdm-slot-width = <32>;
+		};
+	};
+
 	osc24M: osc24M_clk {
 		#clock-cells = <0>;
 		compatible = "fixed-clock";
@@ -607,6 +626,19 @@ mdio: mdio {
 			};
 		};
 
+		i2s1: i2s@5091000 {
+			#sound-dai-cells = <0>;
+			compatible = "allwinner,sun50i-h6-i2s";
+			reg = <0x05091000 0x1000>;
+			interrupts = <GIC_SPI 19 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_BUS_I2S1>, <&ccu CLK_I2S1>;
+			clock-names = "apb", "mod";
+			dmas = <&dma 4>, <&dma 4>;
+			resets = <&ccu RST_BUS_I2S1>;
+			dma-names = "rx", "tx";
+			status = "disabled";
+		};
+
 		spdif: spdif@5093000 {
 			#sound-dai-cells = <0>;
 			compatible = "allwinner,sun50i-h6-spdif";
@@ -737,6 +769,7 @@ ohci3: usb@5311400 {
 		};
 
 		hdmi: hdmi@6000000 {
+			#sound-dai-cells = <0>;
 			compatible = "allwinner,sun50i-h6-dw-hdmi";
 			reg = <0x06000000 0x10000>;
 			reg-io-width = <1>;
-- 
2.25.1

[PATCH 09/16] arm64: dts: allwinner: h6: Enable HDMI sound for Beelink GS1

[Clément Péron]

Now that HDMI sound node is available in the SoC dtsi.
Enable it for this board.

Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
index 3f7ceeb1a767..049c21718846 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
@@ -118,6 +118,14 @@ hdmi_out_con: endpoint {
 	};
 };
 
+&hdmi_sound {
+	status = "okay";
+};
+
+&i2s1 {
+	status = "okay";
+};
+
 &mdio {
 	ext_rgmii_phy: ethernet-phy@1 {
 		compatible = "ethernet-phy-ieee802.3-c22";
-- 
2.25.1

[PATCH 10/16] arm: dts: sunxi: h3/h5: Add DAI node for HDMI

[Clément Péron]

From: Marcus Cooper <codekipper@gmail.com>

Add the new DAI block for I2S2 which is used for HDMI audio.

Signed-off-by: Marcus Cooper <codekipper@gmail.com>
Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 arch/arm/boot/dts/sunxi-h3-h5.dtsi | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/arm/boot/dts/sunxi-h3-h5.dtsi b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
index 22d533d18992..9be13378d4df 100644
--- a/arch/arm/boot/dts/sunxi-h3-h5.dtsi
+++ b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
@@ -662,6 +662,19 @@ i2s1: i2s@1c22400 {
 			status = "disabled";
 		};
 
+		i2s2: i2s@1c22800 {
+			#sound-dai-cells = <0>;
+			compatible = "allwinner,sun8i-h3-i2s";
+			reg = <0x01c22800 0x400>;
+			interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_BUS_I2S2>, <&ccu CLK_I2S2>;
+			clock-names = "apb", "mod";
+			dmas = <&dma 27>;
+			resets = <&ccu RST_BUS_I2S2>;
+			dma-names = "tx";
+			status = "disabled";
+		};
+
 		codec: codec@1c22c00 {
 			#sound-dai-cells = <0>;
 			compatible = "allwinner,sun8i-h3-codec";
-- 
2.25.1

[PATCH 11/16] arm: dts: sunxi: h3/h5: Add HDMI audio

[Clément Péron]

From: Marcus Cooper <codekipper@gmail.com>

Add a simple-soundcard to link audio between HDMI and I2S.

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>
---
 arch/arm/boot/dts/sunxi-h3-h5.dtsi | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/arm/boot/dts/sunxi-h3-h5.dtsi b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
index 9be13378d4df..91d8c2690feb 100644
--- a/arch/arm/boot/dts/sunxi-h3-h5.dtsi
+++ b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
@@ -105,6 +105,25 @@ de: display-engine {
 		status = "disabled";
 	};
 
+	hdmi_sound: hdmi-sound {
+		compatible = "simple-audio-card";
+		simple-audio-card,format = "i2s";
+		simple-audio-card,name = "sun8i-h3-hdmi";
+		simple-audio-card,mclk-fs = <128>;
+		simple-audio-card,frame-inversion;
+		status = "disabled";
+
+		simple-audio-card,codec {
+			sound-dai = <&hdmi>;
+		};
+
+		simple-audio-card,cpu {
+			sound-dai = <&i2s2>;
+			dai-tdm-slot-num = <2>;
+			dai-tdm-slot-width = <32>;
+		};
+	};
+
 	soc {
 		compatible = "simple-bus";
 		#address-cells = <1>;
@@ -806,6 +825,7 @@ csi: camera@1cb0000 {
 		};
 
 		hdmi: hdmi@1ee0000 {
+			#sound-dai-cells = <0>;
 			compatible = "allwinner,sun8i-h3-dw-hdmi",
 				     "allwinner,sun8i-a83t-dw-hdmi";
 			reg = <0x01ee0000 0x10000>;
-- 
2.25.1

[PATCH 12/16] arm64: dts: allwinner: a64: Add DAI node for HDMI

[Clément Péron]

From: Marcus Cooper <codekipper@gmail.com>

Add the new DAI block for I2S2 which is used for HDMI audio.

Signed-off-by: Marcus Cooper <codekipper@gmail.com>
Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index 8dfbcd144072..c662f6a170ce 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -845,6 +845,20 @@ i2s1: i2s@1c22400 {
 			status = "disabled";
 		};
 
+		i2s2: i2s@1c22800 {
+			#sound-dai-cells = <0>;
+			compatible = "allwinner,sun50i-a64-i2s",
+				     "allwinner,sun8i-h3-i2s";
+			reg = <0x01c22800 0x400>;
+			interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_BUS_I2S2>, <&ccu CLK_I2S2>;
+			clock-names = "apb", "mod";
+			resets = <&ccu RST_BUS_I2S2>;
+			dma-names = "tx";
+			dmas = <&dma 27>;
+			status = "disabled";
+		};
+
 		dai: dai@1c22c00 {
 			#sound-dai-cells = <0>;
 			compatible = "allwinner,sun50i-a64-codec-i2s";
-- 
2.25.1

[PATCH 13/16] arm64: dts: allwinner: a64: Add HDMI audio

[Clément Péron]

From: Marcus Cooper <codekipper@gmail.com>

Add a simple-soundcard to link audio between HDMI and I2S.

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>
---
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 21 +++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index c662f6a170ce..6a321fdc8e90 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -102,6 +102,25 @@ de: display-engine {
 		status = "disabled";
 	};
 
+	hdmi_sound: hdmi-sound {
+		compatible = "simple-audio-card";
+		simple-audio-card,format = "i2s";
+		simple-audio-card,name = "sun50i-a64-hdmi";
+		simple-audio-card,mclk-fs = <128>;
+		simple-audio-card,frame-inversion;
+		status = "disabled";
+
+		simple-audio-card,codec {
+			sound-dai = <&hdmi>;
+		};
+
+		simple-audio-card,cpu {
+			sound-dai = <&i2s2>;
+			dai-tdm-slot-num = <2>;
+			dai-tdm-slot-width = <32>;
+		};
+	};
+
 	osc24M: osc24M_clk {
 		#clock-cells = <0>;
 		compatible = "fixed-clock";
@@ -856,6 +875,7 @@ i2s2: i2s@1c22800 {
 			resets = <&ccu RST_BUS_I2S2>;
 			dma-names = "tx";
 			dmas = <&dma 27>;
+			allwinner,playback-channels = <8>;
 			status = "disabled";
 		};
 
@@ -1155,6 +1175,7 @@ deinterlace: deinterlace@1e00000 {
 		};
 
 		hdmi: hdmi@1ee0000 {
+			#sound-dai-cells = <0>;
 			compatible = "allwinner,sun50i-a64-dw-hdmi",
 				     "allwinner,sun8i-a83t-dw-hdmi";
 			reg = <0x01ee0000 0x10000>;
-- 
2.25.1

[PATCH 14/16] arm: sun8i: h3: Add HDMI audio to Orange Pi 2

[Clément Péron]

From: Marcus Cooper <codekipper@gmail.com>

Enable HDMI audio on the Orange Pi 2.

Signed-off-by: Marcus Cooper <codekipper@gmail.com>
Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 arch/arm/boot/dts/sun8i-h3-orangepi-2.dts | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-h3-orangepi-2.dts b/arch/arm/boot/dts/sun8i-h3-orangepi-2.dts
index 597c425d08ec..64e8e2829f27 100644
--- a/arch/arm/boot/dts/sun8i-h3-orangepi-2.dts
+++ b/arch/arm/boot/dts/sun8i-h3-orangepi-2.dts
@@ -144,6 +144,14 @@ hdmi_out_con: endpoint {
 	};
 };
 
+&hdmi_sound {
+	status = "okay";
+};
+
+&i2s2 {
+	status = "okay";
+};
+
 &ir {
 	pinctrl-names = "default";
 	pinctrl-0 = <&r_ir_rx_pin>;
-- 
2.25.1

[PATCH 15/16] arm: sun8i: h3: Add HDMI audio to Beelink X2

[Clément Péron]

From: Marcus Cooper <codekipper@gmail.com>

Enable HDMI audio on the Beelink X2.

Signed-off-by: Marcus Cooper <codekipper@gmail.com>
Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 arch/arm/boot/dts/sun8i-h3-beelink-x2.dts | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-h3-beelink-x2.dts b/arch/arm/boot/dts/sun8i-h3-beelink-x2.dts
index 45a24441ff18..f9bec6935120 100644
--- a/arch/arm/boot/dts/sun8i-h3-beelink-x2.dts
+++ b/arch/arm/boot/dts/sun8i-h3-beelink-x2.dts
@@ -142,6 +142,14 @@ hdmi_out_con: endpoint {
 	};
 };
 
+&hdmi_sound {
+	status = "okay";
+};
+
+&i2s2 {
+	status = "okay";
+};
+
 &ir {
 	linux,rc-map-name = "rc-tanix-tx3mini";
 	pinctrl-names = "default";
-- 
2.25.1

[PATCH 16/16] arm64: dts: allwinner: a64: Add HDMI audio to Pine64

[Clément Péron]

From: Marcus Cooper <codekipper@gmail.com>

Enable HDMI audio on Pine64.

Signed-off-by: Marcus Cooper <codekipper@gmail.com>
Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
index 2165f238af13..c5939ba52f19 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
@@ -99,6 +99,10 @@ hdmi_out_con: endpoint {
 	};
 };
 
+&hdmi_sound {
+	status = "okay";
+};
+
 &i2c1 {
 	status = "okay";
 };
@@ -107,6 +111,10 @@ &i2c1_pins {
 	bias-pull-up;
 };
 
+&i2s2 {
+	status = "okay";
+};
+
 &mdio {
 	ext_rmii_phy1: ethernet-phy@1 {
 		compatible = "ethernet-phy-ieee802.3-c22";
-- 
2.25.1

Re: [PATCH 01/16] ASoC: sun4i-i2s: Add support for H6 I2S

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1971 bytes --]

Hi,

On Sat, Jul 04, 2020 at 01:38:47PM +0200, 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 | 227 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 227 insertions(+)
> 
> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> index d0a8d5810c0a..9690389cb68e 100644
> --- a/sound/soc/sunxi/sun4i-i2s.c
> +++ b/sound/soc/sunxi/sun4i-i2s.c
> @@ -124,6 +124,21 @@
>  #define SUN8I_I2S_RX_CHAN_SEL_REG	0x54
>  #define SUN8I_I2S_RX_CHAN_MAP_REG	0x58
>  
> +/* Defines required for sun50i-h6 support */
> +#define SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET_MASK	GENMASK(21, 20)
> +#define SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET(offset)	((offset) << 20)
> +#define SUN50I_H6_I2S_TX_CHAN_SEL_MASK		GENMASK(19, 16)
> +#define SUN50I_H6_I2S_TX_CHAN_SEL(chan)		((chan - 1) << 16)
> +#define SUN50I_H6_I2S_TX_CHAN_EN_MASK		GENMASK(15, 0)
> +#define SUN50I_H6_I2S_TX_CHAN_EN(num_chan)	(((1 << num_chan) - 1))
> +
> +#define SUN50I_H6_I2S_TX_CHAN_MAP0_REG	0x44
> +#define SUN50I_H6_I2S_TX_CHAN_MAP1_REG	0x48
> +
> +#define SUN50I_H6_I2S_RX_CHAN_SEL_REG	0x64
> +#define SUN50I_H6_I2S_RX_CHAN_MAP0_REG	0x68
> +#define SUN50I_H6_I2S_RX_CHAN_MAP1_REG	0x6C
> +
>  struct sun4i_i2s;
>  
>  /**
> @@ -466,6 +481,65 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
>  	return 0;
>  }
>  
> +static int sun50i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
> +				   const struct snd_pcm_hw_params *params)

We should have sun50i_h6 as prefix. The A64 is also part of the sun50i
family and supported through the sun8i callbacks, so it's pretty
confusing if you don't have the soc name in there.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 04/16] ASoC: sun4i-i2s: Set sign extend sample

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 845 bytes --]

On Sat, Jul 04, 2020 at 01:38:50PM +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 while 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.
> 
> Set sign extend sample for all the sunxi generations even if they
> are not affected. This will keep coherency and avoid relying on
> default.

Isn't it coherence? But I guess consistency would be a better fit here.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 05/16] ASoc: sun4i-i2s: Add 20 and 24 bit support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 389 bytes --]

On Sat, Jul 04, 2020 at 01:38:51PM +0200, 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>

Thanks!
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 06/16] ASoC: sun4i-i2s: Adjust regmap settings

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 361 bytes --]

On Sat, Jul 04, 2020 at 01:38:52PM +0200, Clément Péron wrote:
> From: Marcus Cooper <codekipper@gmail.com>
> 
> Bypass the regmap cache when flushing or reading the i2s FIFOs.
> 
> 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>

Thanks
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 07/16] ASoC: sun4i-i2s: Fix sun8i volatile regs

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 383 bytes --]

On Sat, Jul 04, 2020 at 01:38:53PM +0200, 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>
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 08/16] arm64: dts: allwinner: h6: Add HDMI audio node

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1483 bytes --]

Hi,

On Sat, Jul 04, 2020 at 01:38:54PM +0200, Clément Péron wrote:
> From: Jernej Skrabec <jernej.skrabec@siol.net>
> 
> Add a simple-soundcard to link audio between HDMI and I2S.
> 
> 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>
> ---
>  arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 33 ++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> index 78b1361dfbb9..ae169d07b939 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> @@ -67,6 +67,25 @@ de: display-engine {
>  		status = "disabled";
>  	};
>  
> +	hdmi_sound: hdmi-sound {
> +		compatible = "simple-audio-card";
> +		simple-audio-card,format = "i2s";
> +		simple-audio-card,name = "sun50i-h6-hdmi";
> +		simple-audio-card,mclk-fs = <128>;
> +		simple-audio-card,frame-inversion;

Have you figured that one out?

> +		status = "disabled";
> +
> +		simple-audio-card,codec {
> +			sound-dai = <&hdmi>;
> +		};
> +
> +		simple-audio-card,cpu {
> +			sound-dai = <&i2s1>;
> +			dai-tdm-slot-num = <2>;
> +			dai-tdm-slot-width = <32>;

I'm not sure why you need to use the TDM stuff here. IIRC the HDMI
controller can output on up to 6 channels, so how would that work out?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 13/16] arm64: dts: allwinner: a64: Add HDMI audio

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1615 bytes --]

On Sat, Jul 04, 2020 at 01:38:59PM +0200, Clément Péron wrote:
> From: Marcus Cooper <codekipper@gmail.com>
> 
> Add a simple-soundcard to link audio between HDMI and I2S.
> 
> 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>
> ---
>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 21 +++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> index c662f6a170ce..6a321fdc8e90 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> @@ -102,6 +102,25 @@ de: display-engine {
>  		status = "disabled";
>  	};
>  
> +	hdmi_sound: hdmi-sound {
> +		compatible = "simple-audio-card";
> +		simple-audio-card,format = "i2s";
> +		simple-audio-card,name = "sun50i-a64-hdmi";
> +		simple-audio-card,mclk-fs = <128>;
> +		simple-audio-card,frame-inversion;
> +		status = "disabled";
> +
> +		simple-audio-card,codec {
> +			sound-dai = <&hdmi>;
> +		};
> +
> +		simple-audio-card,cpu {
> +			sound-dai = <&i2s2>;
> +			dai-tdm-slot-num = <2>;
> +			dai-tdm-slot-width = <32>;
> +		};
> +	};
> +
>  	osc24M: osc24M_clk {
>  		#clock-cells = <0>;
>  		compatible = "fixed-clock";
> @@ -856,6 +875,7 @@ i2s2: i2s@1c22800 {
>  			resets = <&ccu RST_BUS_I2S2>;
>  			dma-names = "tx";
>  			dmas = <&dma 27>;
> +			allwinner,playback-channels = <8>;

This isn't documented anywhere

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 13/16] arm64: dts: allwinner: a64: Add HDMI audio

[Jernej Škrabec]

Hi!

Dne ponedeljek, 06. julij 2020 ob 07:31:23 CEST je Maxime Ripard napisal(a):
> On Sat, Jul 04, 2020 at 01:38:59PM +0200, Clément Péron wrote:
> > From: Marcus Cooper <codekipper@gmail.com>
> > 
> > Add a simple-soundcard to link audio between HDMI and I2S.
> > 
> > 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>
> > ---
> > 
> >  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 21 +++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi index
> > c662f6a170ce..6a321fdc8e90 100644
> > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > @@ -102,6 +102,25 @@ de: display-engine {
> > 
> >  		status = "disabled";
> >  	
> >  	};
> > 
> > +	hdmi_sound: hdmi-sound {
> > +		compatible = "simple-audio-card";
> > +		simple-audio-card,format = "i2s";
> > +		simple-audio-card,name = "sun50i-a64-hdmi";
> > +		simple-audio-card,mclk-fs = <128>;
> > +		simple-audio-card,frame-inversion;
> > +		status = "disabled";
> > +
> > +		simple-audio-card,codec {
> > +			sound-dai = <&hdmi>;
> > +		};
> > +
> > +		simple-audio-card,cpu {
> > +			sound-dai = <&i2s2>;
> > +			dai-tdm-slot-num = <2>;
> > +			dai-tdm-slot-width = <32>;
> > +		};
> > +	};
> > +
> > 
> >  	osc24M: osc24M_clk {
> >  	
> >  		#clock-cells = <0>;
> >  		compatible = "fixed-clock";
> > 
> > @@ -856,6 +875,7 @@ i2s2: i2s@1c22800 {
> > 
> >  			resets = <&ccu RST_BUS_I2S2>;
> >  			dma-names = "tx";
> >  			dmas = <&dma 27>;
> > 
> > +			allwinner,playback-channels = <8>;
> 
> This isn't documented anywhere

This can be safely dropped. It is just leftover from multi-channel (>2) work, 
which will have to be redesigned anyway.

Best regards,
Jernej

Re: [PATCH 08/16] arm64: dts: allwinner: h6: Add HDMI audio node

[Jernej Škrabec]

Hi!

Dne ponedeljek, 06. julij 2020 ob 07:29:37 CEST je Maxime Ripard napisal(a):
> Hi,
> 
> On Sat, Jul 04, 2020 at 01:38:54PM +0200, Clément Péron wrote:
> > From: Jernej Skrabec <jernej.skrabec@siol.net>
> > 
> > Add a simple-soundcard to link audio between HDMI and I2S.
> > 
> > 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>
> > ---
> > 
> >  arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 33 ++++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> > b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi index
> > 78b1361dfbb9..ae169d07b939 100644
> > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> > @@ -67,6 +67,25 @@ de: display-engine {
> > 
> >  		status = "disabled";
> >  	
> >  	};
> > 
> > +	hdmi_sound: hdmi-sound {
> > +		compatible = "simple-audio-card";
> > +		simple-audio-card,format = "i2s";
> > +		simple-audio-card,name = "sun50i-h6-hdmi";
> > +		simple-audio-card,mclk-fs = <128>;
> > +		simple-audio-card,frame-inversion;
> 
> Have you figured that one out?
> 
> > +		status = "disabled";
> > +
> > +		simple-audio-card,codec {
> > +			sound-dai = <&hdmi>;
> > +		};
> > +
> > +		simple-audio-card,cpu {
> > +			sound-dai = <&i2s1>;
> > +			dai-tdm-slot-num = <2>;
> > +			dai-tdm-slot-width = <32>;
> 
> I'm not sure why you need to use the TDM stuff here. IIRC the HDMI
> controller can output on up to 6 channels, so how would that work out?

dai-tdm-slot-width is needed to override automatic slot width selection. It 
should always be 32 for HDMI, no matter what is actual physical sample width. 
In this case this property is abused to set width also for I2S mode of 
operation. IMO there is no sense to duplicate code because I2S variant would 
work exactly the same, except name would be different.

I'm not sure about dai-tdm-slot-num. Marcus, can you explain the need for this 
property?

Would it be better to implement separate link driver instead of using simple-
card to hide all this properties into the code?

Best regards,
Jernej

Re: [linux-sunxi] [PATCH 01/16] ASoC: sun4i-i2s: Add support for H6 I2S

[Samuel Holland]

On 7/4/20 6:38 AM, 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 | 227 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 227 insertions(+)
> 
> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> index d0a8d5810c0a..9690389cb68e 100644
> --- a/sound/soc/sunxi/sun4i-i2s.c
> +++ b/sound/soc/sunxi/sun4i-i2s.c
> @@ -124,6 +124,21 @@
>  #define SUN8I_I2S_RX_CHAN_SEL_REG	0x54
>  #define SUN8I_I2S_RX_CHAN_MAP_REG	0x58
>  
> +/* Defines required for sun50i-h6 support */
> +#define SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET_MASK	GENMASK(21, 20)
> +#define SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET(offset)	((offset) << 20)
> +#define SUN50I_H6_I2S_TX_CHAN_SEL_MASK		GENMASK(19, 16)
> +#define SUN50I_H6_I2S_TX_CHAN_SEL(chan)		((chan - 1) << 16)
> +#define SUN50I_H6_I2S_TX_CHAN_EN_MASK		GENMASK(15, 0)
> +#define SUN50I_H6_I2S_TX_CHAN_EN(num_chan)	(((1 << num_chan) - 1))
> +
> +#define SUN50I_H6_I2S_TX_CHAN_MAP0_REG	0x44
> +#define SUN50I_H6_I2S_TX_CHAN_MAP1_REG	0x48
> +
> +#define SUN50I_H6_I2S_RX_CHAN_SEL_REG	0x64
> +#define SUN50I_H6_I2S_RX_CHAN_MAP0_REG	0x68
> +#define SUN50I_H6_I2S_RX_CHAN_MAP1_REG	0x6C
> +
>  struct sun4i_i2s;
>  
>  /**
> @@ -466,6 +481,65 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
>  	return 0;
>  }
>  
> +static int sun50i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
> +				   const struct snd_pcm_hw_params *params)
> +{
> +	unsigned int channels = params_channels(params);
> +	unsigned int slots = channels;
> +	unsigned int lrck_period;
> +
> +	if (i2s->slots)
> +		slots = i2s->slots;
> +
> +	/* Map the channels for playback and capture */
> +	regmap_write(i2s->regmap, SUN50I_H6_I2S_TX_CHAN_MAP1_REG, 0x76543210);
> +	regmap_write(i2s->regmap, SUN50I_H6_I2S_RX_CHAN_MAP1_REG, 0x76543210);
> +
> +	/* Configure the channels */
> +	regmap_update_bits(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG,
> +			   SUN50I_H6_I2S_TX_CHAN_SEL_MASK,
> +			   SUN50I_H6_I2S_TX_CHAN_SEL(channels));
> +	regmap_update_bits(i2s->regmap, SUN50I_H6_I2S_RX_CHAN_SEL_REG,
> +			   SUN50I_H6_I2S_TX_CHAN_SEL_MASK,
> +			   SUN50I_H6_I2S_TX_CHAN_SEL(channels));
> +
> +	regmap_update_bits(i2s->regmap, SUN8I_I2S_CHAN_CFG_REG,
> +			   SUN8I_I2S_CHAN_CFG_TX_SLOT_NUM_MASK,
> +			   SUN8I_I2S_CHAN_CFG_TX_SLOT_NUM(channels));
> +	regmap_update_bits(i2s->regmap, SUN8I_I2S_CHAN_CFG_REG,
> +			   SUN8I_I2S_CHAN_CFG_RX_SLOT_NUM_MASK,
> +			   SUN8I_I2S_CHAN_CFG_RX_SLOT_NUM(channels));
> +
> +	switch (i2s->format & SND_SOC_DAIFMT_FORMAT_MASK) {
> +	case SND_SOC_DAIFMT_DSP_A:
> +	case SND_SOC_DAIFMT_DSP_B:
> +	case SND_SOC_DAIFMT_LEFT_J:
> +	case SND_SOC_DAIFMT_RIGHT_J:

According to the manual, LEFT_J and RIGHT_J should use the same calculation as
I2S, not the one for PCM/DSP.

> +		lrck_period = params_physical_width(params) * slots;
> +		break;
> +
> +	case SND_SOC_DAIFMT_I2S:
> +		lrck_period = params_physical_width(params);
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (i2s->slot_width)
> +		lrck_period = i2s->slot_width;
> +
> +	regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT0_REG,
> +			   SUN8I_I2S_FMT0_LRCK_PERIOD_MASK,
> +			   SUN8I_I2S_FMT0_LRCK_PERIOD(lrck_period));

From the description in the manual, this looks off by one. The number of BCLKs
per LRCK is LRCK_PERIOD + 1.

> +
> +	regmap_update_bits(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG,
> +			   SUN50I_H6_I2S_TX_CHAN_EN_MASK,
> +			   SUN50I_H6_I2S_TX_CHAN_EN(channels));
> +
> +	return 0;
> +}
> +
>  static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
>  			       struct snd_pcm_hw_params *params,
>  			       struct snd_soc_dai *dai)
> @@ -691,6 +765,108 @@ static int sun8i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s,
>  	return 0;
>  }
>  
> +static int sun50i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s,
> +				  unsigned int fmt)
> +{
> +	u32 mode, val;
> +	u8 offset;
> +
> +	/*
> +	 * DAI clock polarity
> +	 *
> +	 * The setup for LRCK contradicts the datasheet, but under a
> +	 * scope it's clear that the LRCK polarity is reversed
> +	 * compared to the expected polarity on the bus.
> +	 */

This comment makes us sound a lot more confident than I think we actually are.

Regards,
Samuel

Re: [linux-sunxi] [PATCH 02/16] ASoC: sun4i-i2s: Adjust LRCLK width

[Samuel Holland]

On 7/4/20 6:38 AM, Clément Péron wrote:
> From: Marcus Cooper <codekipper@gmail.com>
> 
> Some codecs such as i2s based HDMI audio and the Pine64 DAC require
> a different amount of bit clocks per frame than what is calculated
> by the sample width. Use the values obtained by the tdm slot bindings
> to adjust the LRCLK width accordingly.
> 
> Signed-off-by: Marcus Cooper <codekipper@gmail.com>
> Signed-off-by: Clément Péron <peron.clem@gmail.com>
> Acked-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  sound/soc/sunxi/sun4i-i2s.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> index 9690389cb68e..8bae97efea30 100644
> --- a/sound/soc/sunxi/sun4i-i2s.c
> +++ b/sound/soc/sunxi/sun4i-i2s.c
> @@ -470,6 +470,9 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
>  		return -EINVAL;
>  	}
>  
> +	if (i2s->slot_width)
> +		lrck_period = i2s->slot_width;
> +
>  	regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT0_REG,
>  			   SUN8I_I2S_FMT0_LRCK_PERIOD_MASK,
>  			   SUN8I_I2S_FMT0_LRCK_PERIOD(lrck_period));
> 

It looks like the existing code would have the same problem, that this should be
lrck_period - 1 according to the manual (I checked H3).

Regards,
Samuel

Re: [linux-sunxi] [PATCH 04/16] ASoC: sun4i-i2s: Set sign extend sample

[Samuel Holland]

On 7/4/20 6:38 AM, 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 while 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.
> 
> Set sign extend sample for all the sunxi generations even if they
> are not affected. This will keep coherency and avoid relying on
> default.
> 
> 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 8bae97efea30..f78167e152ce 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));
> +

This is just a note; I'm not suggesting a change here:

This does nothing, because SUN4I_I2S_FMT1_REG only affects PCM mode, which is
not implemented in the driver for the sun4i generation of hardware. PCM mode
requires setting bit 4 of SUN4I_I2S_CTRL_REG, and then configuring
SUN4I_I2S_FMT1_REG instead of SUN4I_I2S_FMT0_REG.

Regards,
Samuel

Re: [linux-sunxi] [PATCH 05/16] ASoc: sun4i-i2s: Add 20 and 24 bit support

[Samuel Holland]

On 7/4/20 6:38 AM, 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>
> ---
>  sound/soc/sunxi/sun4i-i2s.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> index f78167e152ce..bc7f9343bc7a 100644
> --- a/sound/soc/sunxi/sun4i-i2s.c
> +++ b/sound/soc/sunxi/sun4i-i2s.c
> @@ -577,6 +577,9 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
>  	case 16:
>  		width = DMA_SLAVE_BUSWIDTH_2_BYTES;
>  		break;
> +	case 32:
> +		width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> +		break;

This breaks the sun4i variants, because sun4i_i2s_get_wss returns 4 for a 32 bit
width, but it needs to return 3.

As a side note, I wonder why we use the physical width (the spacing between
samples in RAM) to drive the slot width. S24_LE takes up 4 bytes per sample in
RAM, which we need for DMA. But I don't see why we would want to transmit the
padding over the wire. I would expect it to be transmitted the same as S24_3LE
(which has no padding). It did not matter before, because the only supported
format had no padding.

Regards,
Samuel

>  	default:
>  		dev_err(dai->dev, "Unsupported physical sample width: %d\n",
>  			params_physical_width(params));
> @@ -1063,6 +1066,10 @@ static int sun4i_i2s_dai_probe(struct snd_soc_dai *dai)
>  	return 0;
>  }
>  
> +#define SUN4I_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | \
> +			 SNDRV_PCM_FMTBIT_S20_LE | \
> +			 SNDRV_PCM_FMTBIT_S24_LE)
> +
>  static struct snd_soc_dai_driver sun4i_i2s_dai = {
>  	.probe = sun4i_i2s_dai_probe,
>  	.capture = {
> @@ -1070,14 +1077,14 @@ static struct snd_soc_dai_driver sun4i_i2s_dai = {
>  		.channels_min = 1,
>  		.channels_max = 8,
>  		.rates = SNDRV_PCM_RATE_8000_192000,
> -		.formats = SNDRV_PCM_FMTBIT_S16_LE,
> +		.formats = SUN4I_FORMATS,
>  	},
>  	.playback = {
>  		.stream_name = "Playback",
>  		.channels_min = 1,
>  		.channels_max = 8,
>  		.rates = SNDRV_PCM_RATE_8000_192000,
> -		.formats = SNDRV_PCM_FMTBIT_S16_LE,
> +		.formats = SUN4I_FORMATS,
>  	},
>  	.ops = &sun4i_i2s_dai_ops,
>  	.symmetric_rates = 1,
> 

Re: [linux-sunxi] [PATCH 01/16] ASoC: sun4i-i2s: Add support for H6 I2S

[Jernej Škrabec]

Hi Samuel!

Dne petek, 10. julij 2020 ob 07:44:33 CEST je Samuel Holland napisal(a):
> On 7/4/20 6:38 AM, 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 | 227 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 227 insertions(+)
> > 
> > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> > index d0a8d5810c0a..9690389cb68e 100644
> > --- a/sound/soc/sunxi/sun4i-i2s.c
> > +++ b/sound/soc/sunxi/sun4i-i2s.c
> > @@ -124,6 +124,21 @@
> > 
> >  #define SUN8I_I2S_RX_CHAN_SEL_REG	0x54
> >  #define SUN8I_I2S_RX_CHAN_MAP_REG	0x58
> > 
> > +/* Defines required for sun50i-h6 support */
> > +#define SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET_MASK	GENMASK(21, 20)
> > +#define SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET(offset)	((offset) << 20)
> > +#define SUN50I_H6_I2S_TX_CHAN_SEL_MASK		GENMASK(19, 16)
> > +#define SUN50I_H6_I2S_TX_CHAN_SEL(chan)		((chan - 1) << 16)
> > +#define SUN50I_H6_I2S_TX_CHAN_EN_MASK		GENMASK(15, 0)
> > +#define SUN50I_H6_I2S_TX_CHAN_EN(num_chan)	(((1 << num_chan) - 1))
> > +
> > +#define SUN50I_H6_I2S_TX_CHAN_MAP0_REG	0x44
> > +#define SUN50I_H6_I2S_TX_CHAN_MAP1_REG	0x48
> > +
> > +#define SUN50I_H6_I2S_RX_CHAN_SEL_REG	0x64
> > +#define SUN50I_H6_I2S_RX_CHAN_MAP0_REG	0x68
> > +#define SUN50I_H6_I2S_RX_CHAN_MAP1_REG	0x6C
> > +
> > 
> >  struct sun4i_i2s;
> >  
> >  /**
> > 
> > @@ -466,6 +481,65 @@ static int sun8i_i2s_set_chan_cfg(const struct
> > sun4i_i2s *i2s,> 
> >  	return 0;
> >  
> >  }
> > 
> > +static int sun50i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
> > +				   const struct snd_pcm_hw_params 
*params)
> > +{
> > +	unsigned int channels = params_channels(params);
> > +	unsigned int slots = channels;
> > +	unsigned int lrck_period;
> > +
> > +	if (i2s->slots)
> > +		slots = i2s->slots;
> > +
> > +	/* Map the channels for playback and capture */
> > +	regmap_write(i2s->regmap, SUN50I_H6_I2S_TX_CHAN_MAP1_REG, 
0x76543210);
> > +	regmap_write(i2s->regmap, SUN50I_H6_I2S_RX_CHAN_MAP1_REG, 
0x76543210);
> > +
> > +	/* Configure the channels */
> > +	regmap_update_bits(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG,
> > +			   SUN50I_H6_I2S_TX_CHAN_SEL_MASK,
> > +			   SUN50I_H6_I2S_TX_CHAN_SEL(channels));
> > +	regmap_update_bits(i2s->regmap, SUN50I_H6_I2S_RX_CHAN_SEL_REG,
> > +			   SUN50I_H6_I2S_TX_CHAN_SEL_MASK,
> > +			   SUN50I_H6_I2S_TX_CHAN_SEL(channels));
> > +
> > +	regmap_update_bits(i2s->regmap, SUN8I_I2S_CHAN_CFG_REG,
> > +			   SUN8I_I2S_CHAN_CFG_TX_SLOT_NUM_MASK,
> > +			   
SUN8I_I2S_CHAN_CFG_TX_SLOT_NUM(channels));
> > +	regmap_update_bits(i2s->regmap, SUN8I_I2S_CHAN_CFG_REG,
> > +			   SUN8I_I2S_CHAN_CFG_RX_SLOT_NUM_MASK,
> > +			   
SUN8I_I2S_CHAN_CFG_RX_SLOT_NUM(channels));
> > +
> > +	switch (i2s->format & SND_SOC_DAIFMT_FORMAT_MASK) {
> > +	case SND_SOC_DAIFMT_DSP_A:
> > +	case SND_SOC_DAIFMT_DSP_B:
> > +	case SND_SOC_DAIFMT_LEFT_J:
> 
> > +	case SND_SOC_DAIFMT_RIGHT_J:
> According to the manual, LEFT_J and RIGHT_J should use the same calculation
> as I2S, not the one for PCM/DSP.
> 
> > +		lrck_period = params_physical_width(params) * slots;
> > +		break;
> > +
> > +	case SND_SOC_DAIFMT_I2S:
> > +		lrck_period = params_physical_width(params);
> > +		break;
> > +
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (i2s->slot_width)
> > +		lrck_period = i2s->slot_width;
> > +
> > +	regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT0_REG,
> > +			   SUN8I_I2S_FMT0_LRCK_PERIOD_MASK,
> > +			   SUN8I_I2S_FMT0_LRCK_PERIOD(lrck_period));
> 
> From the description in the manual, this looks off by one. The number of
> BCLKs per LRCK is LRCK_PERIOD + 1.

Are you sure? Macro SUN8I_I2S_FMT0_LRCK_PERIOD() is defined as follows:

#define SUN8I_I2S_FMT0_LRCK_PERIOD(period)	((period - 1) << 8)

which already lowers value by 1.

Best regards,
Jernej

> 
> > +
> > +	regmap_update_bits(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG,
> > +			   SUN50I_H6_I2S_TX_CHAN_EN_MASK,
> > +			   SUN50I_H6_I2S_TX_CHAN_EN(channels));
> > +
> > +	return 0;
> > +}
> > +
> > 
> >  static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
> >  
> >  			       struct snd_pcm_hw_params *params,
> >  			       struct snd_soc_dai *dai)
> > 
> > @@ -691,6 +765,108 @@ static int sun8i_i2s_set_soc_fmt(const struct
> > sun4i_i2s *i2s,> 
> >  	return 0;
> >  
> >  }
> > 
> > +static int sun50i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s,
> > +				  unsigned int fmt)
> > +{
> > +	u32 mode, val;
> > +	u8 offset;
> > +
> > +	/*
> > +	 * DAI clock polarity
> > +	 *
> > +	 * The setup for LRCK contradicts the datasheet, but under a
> > +	 * scope it's clear that the LRCK polarity is reversed
> > +	 * compared to the expected polarity on the bus.
> > +	 */
> 
> This comment makes us sound a lot more confident than I think we actually
> are.
> 
> Regards,
> Samuel

Re: [linux-sunxi] [PATCH 01/16] ASoC: sun4i-i2s: Add support for H6 I2S

[Samuel Holland]

Jernej,

On 7/10/20 2:22 PM, Jernej Škrabec wrote:
>> From the description in the manual, this looks off by one. The number of
>> BCLKs per LRCK is LRCK_PERIOD + 1.
> 
> Are you sure? Macro SUN8I_I2S_FMT0_LRCK_PERIOD() is defined as follows:
> 
> #define SUN8I_I2S_FMT0_LRCK_PERIOD(period)	((period - 1) << 8)
> 
> which already lowers value by 1.

No, sorry, I had missed the subtraction happening in the macro. So there's no
problem here.

Thanks,
Samuel

Re: [linux-sunxi] [PATCH 10/16] arm: dts: sunxi: h3/h5: Add DAI node for HDMI

[Samuel Holland]

Clément,

On 7/4/20 6:38 AM, Clément Péron wrote:
> From: Marcus Cooper <codekipper@gmail.com>
> 
> Add the new DAI block for I2S2 which is used for HDMI audio.
> 
> Signed-off-by: Marcus Cooper <codekipper@gmail.com>
> Signed-off-by: Clément Péron <peron.clem@gmail.com>
> ---
>  arch/arm/boot/dts/sunxi-h3-h5.dtsi | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sunxi-h3-h5.dtsi b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> index 22d533d18992..9be13378d4df 100644
> --- a/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> +++ b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> @@ -662,6 +662,19 @@ i2s1: i2s@1c22400 {
>  			status = "disabled";
>  		};
>  
> +		i2s2: i2s@1c22800 {
> +			#sound-dai-cells = <0>;
> +			compatible = "allwinner,sun8i-h3-i2s";
> +			reg = <0x01c22800 0x400>;
> +			interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&ccu CLK_BUS_I2S2>, <&ccu CLK_I2S2>;
> +			clock-names = "apb", "mod";
> +			dmas = <&dma 27>;
> +			resets = <&ccu RST_BUS_I2S2>;
> +			dma-names = "tx";

`make dtbs_check` reports:

i2s@1c22800: dma-names:0: 'rx' was expected
i2s@1c22800: dma-names: ['tx'] is too short
i2s@1c22800: dmas: [[28, 27]] is too short

Regards,
Samuel

> +			status = "disabled";
> +		};
> +
>  		codec: codec@1c22c00 {
>  			#sound-dai-cells = <0>;
>  			compatible = "allwinner,sun8i-h3-codec";
> 

Re: [linux-sunxi] [PATCH 01/16] ASoC: sun4i-i2s: Add support for H6 I2S

[Clément Péron]

Hi Samuel,

On Fri, 10 Jul 2020 at 07:44, Samuel Holland <samuel@sholland.org> wrote:
>
> On 7/4/20 6:38 AM, 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 | 227 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 227 insertions(+)
> >
> > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> > index d0a8d5810c0a..9690389cb68e 100644
> > --- a/sound/soc/sunxi/sun4i-i2s.c
> > +++ b/sound/soc/sunxi/sun4i-i2s.c
> > @@ -124,6 +124,21 @@
> >  #define SUN8I_I2S_RX_CHAN_SEL_REG    0x54
> >  #define SUN8I_I2S_RX_CHAN_MAP_REG    0x58
> >
> > +/* Defines required for sun50i-h6 support */
> > +#define SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET_MASK        GENMASK(21, 20)
> > +#define SUN50I_H6_I2S_TX_CHAN_SEL_OFFSET(offset)     ((offset) << 20)
> > +#define SUN50I_H6_I2S_TX_CHAN_SEL_MASK               GENMASK(19, 16)
> > +#define SUN50I_H6_I2S_TX_CHAN_SEL(chan)              ((chan - 1) << 16)
> > +#define SUN50I_H6_I2S_TX_CHAN_EN_MASK                GENMASK(15, 0)
> > +#define SUN50I_H6_I2S_TX_CHAN_EN(num_chan)   (((1 << num_chan) - 1))
> > +
> > +#define SUN50I_H6_I2S_TX_CHAN_MAP0_REG       0x44
> > +#define SUN50I_H6_I2S_TX_CHAN_MAP1_REG       0x48
> > +
> > +#define SUN50I_H6_I2S_RX_CHAN_SEL_REG        0x64
> > +#define SUN50I_H6_I2S_RX_CHAN_MAP0_REG       0x68
> > +#define SUN50I_H6_I2S_RX_CHAN_MAP1_REG       0x6C
> > +
> >  struct sun4i_i2s;
> >
> >  /**
> > @@ -466,6 +481,65 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
> >       return 0;
> >  }
> >
> > +static int sun50i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
> > +                                const struct snd_pcm_hw_params *params)
> > +{
> > +     unsigned int channels = params_channels(params);
> > +     unsigned int slots = channels;
> > +     unsigned int lrck_period;
> > +
> > +     if (i2s->slots)
> > +             slots = i2s->slots;
> > +
> > +     /* Map the channels for playback and capture */
> > +     regmap_write(i2s->regmap, SUN50I_H6_I2S_TX_CHAN_MAP1_REG, 0x76543210);
> > +     regmap_write(i2s->regmap, SUN50I_H6_I2S_RX_CHAN_MAP1_REG, 0x76543210);
> > +
> > +     /* Configure the channels */
> > +     regmap_update_bits(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG,
> > +                        SUN50I_H6_I2S_TX_CHAN_SEL_MASK,
> > +                        SUN50I_H6_I2S_TX_CHAN_SEL(channels));
> > +     regmap_update_bits(i2s->regmap, SUN50I_H6_I2S_RX_CHAN_SEL_REG,
> > +                        SUN50I_H6_I2S_TX_CHAN_SEL_MASK,
> > +                        SUN50I_H6_I2S_TX_CHAN_SEL(channels));
> > +
> > +     regmap_update_bits(i2s->regmap, SUN8I_I2S_CHAN_CFG_REG,
> > +                        SUN8I_I2S_CHAN_CFG_TX_SLOT_NUM_MASK,
> > +                        SUN8I_I2S_CHAN_CFG_TX_SLOT_NUM(channels));
> > +     regmap_update_bits(i2s->regmap, SUN8I_I2S_CHAN_CFG_REG,
> > +                        SUN8I_I2S_CHAN_CFG_RX_SLOT_NUM_MASK,
> > +                        SUN8I_I2S_CHAN_CFG_RX_SLOT_NUM(channels));
> > +
> > +     switch (i2s->format & SND_SOC_DAIFMT_FORMAT_MASK) {
> > +     case SND_SOC_DAIFMT_DSP_A:
> > +     case SND_SOC_DAIFMT_DSP_B:
> > +     case SND_SOC_DAIFMT_LEFT_J:
> > +     case SND_SOC_DAIFMT_RIGHT_J:
>
> According to the manual, LEFT_J and RIGHT_J should use the same calculation as
> I2S, not the one for PCM/DSP.
>
> > +             lrck_period = params_physical_width(params) * slots;
> > +             break;
> > +
> > +     case SND_SOC_DAIFMT_I2S:
> > +             lrck_period = params_physical_width(params);
> > +             break;
> > +
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +
> > +     if (i2s->slot_width)
> > +             lrck_period = i2s->slot_width;
> > +
> > +     regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT0_REG,
> > +                        SUN8I_I2S_FMT0_LRCK_PERIOD_MASK,
> > +                        SUN8I_I2S_FMT0_LRCK_PERIOD(lrck_period));
>
> From the description in the manual, this looks off by one. The number of BCLKs
> per LRCK is LRCK_PERIOD + 1.
>
> > +
> > +     regmap_update_bits(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG,
> > +                        SUN50I_H6_I2S_TX_CHAN_EN_MASK,
> > +                        SUN50I_H6_I2S_TX_CHAN_EN(channels));
> > +
> > +     return 0;
> > +}
> > +
> >  static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
> >                              struct snd_pcm_hw_params *params,
> >                              struct snd_soc_dai *dai)
> > @@ -691,6 +765,108 @@ static int sun8i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s,
> >       return 0;
> >  }
> >
> > +static int sun50i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s,
> > +                               unsigned int fmt)
> > +{
> > +     u32 mode, val;
> > +     u8 offset;
> > +
> > +     /*
> > +      * DAI clock polarity
> > +      *
> > +      * The setup for LRCK contradicts the datasheet, but under a
> > +      * scope it's clear that the LRCK polarity is reversed
> > +      * compared to the expected polarity on the bus.
> > +      */
>
> This comment makes us sound a lot more confident than I think we actually are.

That's a comment that needs to be checked using a Logic analyzer (that
I don't have).
It's a copy paste from the previous generation.
But this seems wrong as we need to specify
"simple-audio-card,frame-inversion;" in the device-tree.

Regards,
Clement

>
> Regards,
> Samuel

Re: [linux-sunxi] [PATCH 04/16] ASoC: sun4i-i2s: Set sign extend sample

[Clément Péron]

Hi Samuel,

On Fri, 10 Jul 2020 at 07:44, Samuel Holland <samuel@sholland.org> wrote:
>
> On 7/4/20 6:38 AM, 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 while 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.
> >
> > Set sign extend sample for all the sunxi generations even if they
> > are not affected. This will keep coherency and avoid relying on
> > default.
> >
> > 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 8bae97efea30..f78167e152ce 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));
> > +
>
> This is just a note; I'm not suggesting a change here:
>
> This does nothing, because SUN4I_I2S_FMT1_REG only affects PCM mode, which is
> not implemented in the driver for the sun4i generation of hardware. PCM mode
> requires setting bit 4 of SUN4I_I2S_CTRL_REG, and then configuring
> SUN4I_I2S_FMT1_REG instead of SUN4I_I2S_FMT0_REG.

Thanks for the catch,
So i will drop it for sun4i.

Regards,
Clement

>
> Regards,
> Samuel

Re: [linux-sunxi] [PATCH 05/16] ASoc: sun4i-i2s: Add 20 and 24 bit support

[Jernej Škrabec]

Hi Samuel!

Dne petek, 10. julij 2020 ob 07:44:51 CEST je Samuel Holland napisal(a):
> On 7/4/20 6:38 AM, 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>
> > ---
> > 
> >  sound/soc/sunxi/sun4i-i2s.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> > index f78167e152ce..bc7f9343bc7a 100644
> > --- a/sound/soc/sunxi/sun4i-i2s.c
> > +++ b/sound/soc/sunxi/sun4i-i2s.c
> > @@ -577,6 +577,9 @@ static int sun4i_i2s_hw_params(struct
> > snd_pcm_substream *substream,> 
> >  	case 16:
> >  		width = DMA_SLAVE_BUSWIDTH_2_BYTES;
> >  		break;
> > 
> > +	case 32:
> > +		width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> > +		break;
> 
> This breaks the sun4i variants, because sun4i_i2s_get_wss returns 4 for a 32
> bit width, but it needs to return 3.

I'm not sure what has WSS with physical width and DMA?

> 
> As a side note, I wonder why we use the physical width (the spacing between
> samples in RAM) to drive the slot width. S24_LE takes up 4 bytes per sample
> in RAM, which we need for DMA. But I don't see why we would want to
> transmit the padding over the wire. I would expect it to be transmitted the
> same as S24_3LE (which has no padding). It did not matter before, because
> the only supported format had no padding.

Allwinner DMA engines support only 1, 2, 4 and sometimes 8 bytes for bus 
width, so if sample is 24 bits in size, we have no other way but to transmit 
padding too.

Best regards,
Jernej

> 
> Regards,
> Samuel
> 
> >  	default:
> >  		dev_err(dai->dev, "Unsupported physical sample width: 
%d\n",
> >  		
> >  			params_physical_width(params));
> > 
> > @@ -1063,6 +1066,10 @@ static int sun4i_i2s_dai_probe(struct snd_soc_dai
> > *dai)> 
> >  	return 0;
> >  
> >  }
> > 
> > +#define SUN4I_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | \
> > +			 SNDRV_PCM_FMTBIT_S20_LE | \
> > +			 SNDRV_PCM_FMTBIT_S24_LE)
> > +
> > 
> >  static struct snd_soc_dai_driver sun4i_i2s_dai = {
> >  
> >  	.probe = sun4i_i2s_dai_probe,
> >  	.capture = {
> > 
> > @@ -1070,14 +1077,14 @@ static struct snd_soc_dai_driver sun4i_i2s_dai = {
> > 
> >  		.channels_min = 1,
> >  		.channels_max = 8,
> >  		.rates = SNDRV_PCM_RATE_8000_192000,
> > 
> > -		.formats = SNDRV_PCM_FMTBIT_S16_LE,
> > +		.formats = SUN4I_FORMATS,
> > 
> >  	},
> >  	.playback = {
> >  	
> >  		.stream_name = "Playback",
> >  		.channels_min = 1,
> >  		.channels_max = 8,
> >  		.rates = SNDRV_PCM_RATE_8000_192000,
> > 
> > -		.formats = SNDRV_PCM_FMTBIT_S16_LE,
> > +		.formats = SUN4I_FORMATS,
> > 
> >  	},
> >  	.ops = &sun4i_i2s_dai_ops,
> >  	.symmetric_rates = 1,

Re: [linux-sunxi] [PATCH 05/16] ASoc: sun4i-i2s: Add 20 and 24 bit support

[Samuel Holland]

On 9/2/20 1:10 PM, Jernej Škrabec wrote:
> Hi Samuel!
> 
> Dne petek, 10. julij 2020 ob 07:44:51 CEST je Samuel Holland napisal(a):
>> On 7/4/20 6:38 AM, 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>
>>> ---
>>>
>>>  sound/soc/sunxi/sun4i-i2s.c | 11 +++++++++--
>>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
>>> index f78167e152ce..bc7f9343bc7a 100644
>>> --- a/sound/soc/sunxi/sun4i-i2s.c
>>> +++ b/sound/soc/sunxi/sun4i-i2s.c
>>> @@ -577,6 +577,9 @@ static int sun4i_i2s_hw_params(struct
>>> snd_pcm_substream *substream,> 
>>>  	case 16:
>>>  		width = DMA_SLAVE_BUSWIDTH_2_BYTES;
>>>  		break;
>>>
>>> +	case 32:
>>> +		width = DMA_SLAVE_BUSWIDTH_4_BYTES;
>>> +		break;
>>
>> This breaks the sun4i variants, because sun4i_i2s_get_wss returns 4 for a 32
>> bit width, but it needs to return 3.
> 
> I'm not sure what has WSS with physical width and DMA?

This is the change where creating a S24_LE stream no longer fails with -EINVAL.
So this is the change where userspace stops downsampling 24-bit audio sources.
So this is the change where playback of 24-bit audio sources breaks, because WSS
is programmed wrong.

>> As a side note, I wonder why we use the physical width (the spacing between
>> samples in RAM) to drive the slot width. S24_LE takes up 4 bytes per sample
>> in RAM, which we need for DMA. But I don't see why we would want to
>> transmit the padding over the wire. I would expect it to be transmitted the
>> same as S24_3LE (which has no padding). It did not matter before, because
>> the only supported format had no padding.
> 
> Allwinner DMA engines support only 1, 2, 4 and sometimes 8 bytes for bus 
> width, so if sample is 24 bits in size, we have no other way but to transmit 
> padding too.

I understand why we do 4 byte DMA from RAM <=> I2S FIFO; that was not my
question. I'm referring to the actual wire format (FIFO <=> PCM_DIN/DOUT). The
sample is already truncated from 32 bits to 24 bits in the FIFO -- that's what
TXIM and RXOM in FIFO_CTRL control.

If a sample is 24 bits wide, why would we send 32 BCLKs for every LRCK? I would
expect the slot width to match the sample resolution by default. But yet we have
this code in the driver:

    unsigned int word_size = params_width(params);
    unsigned int slot_width = params_physical_width(params);

I think slot_width should be the same as word_size, and I suggest changing it
before adding 20/24-bit support.

> Best regards,
> Jernej

Regards,
Samuel

>>>  	default:
>>>  		dev_err(dai->dev, "Unsupported physical sample width: 
> %d\n",
>>>  		
>>>  			params_physical_width(params));
>>>
>>> @@ -1063,6 +1066,10 @@ static int sun4i_i2s_dai_probe(struct snd_soc_dai
>>> *dai)> 
>>>  	return 0;
>>>  
>>>  }
>>>
>>> +#define SUN4I_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | \
>>> +			 SNDRV_PCM_FMTBIT_S20_LE | \
>>> +			 SNDRV_PCM_FMTBIT_S24_LE)
>>> +
>>>
>>>  static struct snd_soc_dai_driver sun4i_i2s_dai = {
>>>  
>>>  	.probe = sun4i_i2s_dai_probe,
>>>  	.capture = {
>>>
>>> @@ -1070,14 +1077,14 @@ static struct snd_soc_dai_driver sun4i_i2s_dai = {
>>>
>>>  		.channels_min = 1,
>>>  		.channels_max = 8,
>>>  		.rates = SNDRV_PCM_RATE_8000_192000,
>>>
>>> -		.formats = SNDRV_PCM_FMTBIT_S16_LE,
>>> +		.formats = SUN4I_FORMATS,
>>>
>>>  	},
>>>  	.playback = {
>>>  	
>>>  		.stream_name = "Playback",
>>>  		.channels_min = 1,
>>>  		.channels_max = 8,
>>>  		.rates = SNDRV_PCM_RATE_8000_192000,
>>>
>>> -		.formats = SNDRV_PCM_FMTBIT_S16_LE,
>>> +		.formats = SUN4I_FORMATS,
>>>
>>>  	},
>>>  	.ops = &sun4i_i2s_dai_ops,
>>>  	.symmetric_rates = 1,
> 
> 
> 
> 

Re: [linux-sunxi] [PATCH 05/16] ASoc: sun4i-i2s: Add 20 and 24 bit support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3538 bytes --]

On Wed, Sep 02, 2020 at 09:22:33PM -0500, Samuel Holland wrote:
> On 9/2/20 1:10 PM, Jernej Škrabec wrote:
> > Hi Samuel!
> > 
> > Dne petek, 10. julij 2020 ob 07:44:51 CEST je Samuel Holland napisal(a):
> >> On 7/4/20 6:38 AM, 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>
> >>> ---
> >>>
> >>>  sound/soc/sunxi/sun4i-i2s.c | 11 +++++++++--
> >>>  1 file changed, 9 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> >>> index f78167e152ce..bc7f9343bc7a 100644
> >>> --- a/sound/soc/sunxi/sun4i-i2s.c
> >>> +++ b/sound/soc/sunxi/sun4i-i2s.c
> >>> @@ -577,6 +577,9 @@ static int sun4i_i2s_hw_params(struct
> >>> snd_pcm_substream *substream,> 
> >>>  	case 16:
> >>>  		width = DMA_SLAVE_BUSWIDTH_2_BYTES;
> >>>  		break;
> >>>
> >>> +	case 32:
> >>> +		width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> >>> +		break;
> >>
> >> This breaks the sun4i variants, because sun4i_i2s_get_wss returns 4 for a 32
> >> bit width, but it needs to return 3.
> > 
> > I'm not sure what has WSS with physical width and DMA?
> 
> This is the change where creating a S24_LE stream no longer fails with -EINVAL.
> So this is the change where userspace stops downsampling 24-bit audio sources.
> So this is the change where playback of 24-bit audio sources breaks, because WSS
> is programmed wrong.
> 
> >> As a side note, I wonder why we use the physical width (the spacing between
> >> samples in RAM) to drive the slot width. S24_LE takes up 4 bytes per sample
> >> in RAM, which we need for DMA. But I don't see why we would want to
> >> transmit the padding over the wire. I would expect it to be transmitted the
> >> same as S24_3LE (which has no padding). It did not matter before, because
> >> the only supported format had no padding.
> > 
> > Allwinner DMA engines support only 1, 2, 4 and sometimes 8 bytes for bus 
> > width, so if sample is 24 bits in size, we have no other way but to transmit 
> > padding too.
> 
> I understand why we do 4 byte DMA from RAM <=> I2S FIFO; that was not my
> question. I'm referring to the actual wire format (FIFO <=> PCM_DIN/DOUT). The
> sample is already truncated from 32 bits to 24 bits in the FIFO -- that's what
> TXIM and RXOM in FIFO_CTRL control.
> 
> If a sample is 24 bits wide, why would we send 32 BCLKs for every LRCK? I would
> expect the slot width to match the sample resolution by default. But yet we have
> this code in the driver:
> 
>     unsigned int word_size = params_width(params);
>     unsigned int slot_width = params_physical_width(params);
> 
> I think slot_width should be the same as word_size, and I suggest changing it
> before adding 20/24-bit support.

Generally speaking, the slot width doesn't necessarily match the
physical width. With TDM for example you may very well have slots
larger than their samples.

That being said, S24 is explicitly a format where you send a sample of
24 bits in a 32-bit word (in the lowest three bytes, little endian)

See:
https://elixir.bootlin.com/linux/v5.9-rc3/source/sound/core/pcm_misc.c#L75
https://mailman.alsa-project.org/pipermail/alsa-devel/2013-April/061073.html

24 bits of data over three bytes like you suggest is S24_3LE

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [linux-sunxi] [PATCH 05/16] ASoc: sun4i-i2s: Add 20 and 24 bit support

[Charles Keepax]

On Thu, Sep 03, 2020 at 09:40:23AM +0200, Maxime Ripard wrote:
> On Wed, Sep 02, 2020 at 09:22:33PM -0500, Samuel Holland wrote:
> > On 9/2/20 1:10 PM, Jernej Škrabec wrote:
> > > Hi Samuel!
> > > 
> > > Dne petek, 10. julij 2020 ob 07:44:51 CEST je Samuel Holland napisal(a):
> > >> On 7/4/20 6:38 AM, 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>
> > >>> ---
> > >>>
> > >>>  sound/soc/sunxi/sun4i-i2s.c | 11 +++++++++--
> > >>>  1 file changed, 9 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> > >>> index f78167e152ce..bc7f9343bc7a 100644
> > >>> --- a/sound/soc/sunxi/sun4i-i2s.c
> > >>> +++ b/sound/soc/sunxi/sun4i-i2s.c
> > >>> @@ -577,6 +577,9 @@ static int sun4i_i2s_hw_params(struct
> > >>> snd_pcm_substream *substream,> 
> > >>>  	case 16:
> > >>>  		width = DMA_SLAVE_BUSWIDTH_2_BYTES;
> > >>>  		break;
> > >>>
> > >>> +	case 32:
> > >>> +		width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> > >>> +		break;
> > >>
> > >> This breaks the sun4i variants, because sun4i_i2s_get_wss returns 4 for a 32
> > >> bit width, but it needs to return 3.
> > > 
> > > I'm not sure what has WSS with physical width and DMA?
> > 
> > This is the change where creating a S24_LE stream no longer fails with -EINVAL.
> > So this is the change where userspace stops downsampling 24-bit audio sources.
> > So this is the change where playback of 24-bit audio sources breaks, because WSS
> > is programmed wrong.
> > 
> > >> As a side note, I wonder why we use the physical width (the spacing between
> > >> samples in RAM) to drive the slot width. S24_LE takes up 4 bytes per sample
> > >> in RAM, which we need for DMA. But I don't see why we would want to
> > >> transmit the padding over the wire. I would expect it to be transmitted the
> > >> same as S24_3LE (which has no padding). It did not matter before, because
> > >> the only supported format had no padding.
> > > 
> > > Allwinner DMA engines support only 1, 2, 4 and sometimes 8 bytes for bus 
> > > width, so if sample is 24 bits in size, we have no other way but to transmit 
> > > padding too.
> > 
> > I understand why we do 4 byte DMA from RAM <=> I2S FIFO; that was not my
> > question. I'm referring to the actual wire format (FIFO <=> PCM_DIN/DOUT). The
> > sample is already truncated from 32 bits to 24 bits in the FIFO -- that's what
> > TXIM and RXOM in FIFO_CTRL control.
> > 
> > If a sample is 24 bits wide, why would we send 32 BCLKs for every LRCK? I would
> > expect the slot width to match the sample resolution by default. But yet we have
> > this code in the driver:
> > 
> >     unsigned int word_size = params_width(params);
> >     unsigned int slot_width = params_physical_width(params);
> > 
> > I think slot_width should be the same as word_size, and I suggest changing it
> > before adding 20/24-bit support.
> 
> Generally speaking, the slot width doesn't necessarily match the
> physical width. With TDM for example you may very well have slots
> larger than their samples.
> 
> That being said, S24 is explicitly a format where you send a sample of
> 24 bits in a 32-bit word (in the lowest three bytes, little endian)
> 
> See:
> https://elixir.bootlin.com/linux/v5.9-rc3/source/sound/core/pcm_misc.c#L75
> https://mailman.alsa-project.org/pipermail/alsa-devel/2013-April/061073.html
> 
> 24 bits of data over three bytes like you suggest is S24_3LE
> 

My understanding is physical_width refers to the in memory
representation, but shouldn't be used to control the slot width
on the bus. If not specified otherwise (say through the set_tdm
callback), and if the appropriate BCLK is supported, then the slot
should be just large enough to hold the data.

Thanks,
Charles

Re: [linux-sunxi] [PATCH 05/16] ASoc: sun4i-i2s: Add 20 and 24 bit support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 538 bytes --]

On Fri, Sep 04, 2020 at 04:16:49PM +0000, Charles Keepax wrote:

> My understanding is physical_width refers to the in memory
> representation, but shouldn't be used to control the slot width
> on the bus. If not specified otherwise (say through the set_tdm
> callback), and if the appropriate BCLK is supported, then the slot
> should be just large enough to hold the data.

Indeed.  The framework isn't great here in tying the memory and wire
formats together, ideally there would be more support for them being
unrelated without DPCM.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]