[PATCH v2 0/7] Add H6 I2S support
Hi,
This is a sequel of Marcus Cooper serie[0], where remarks made by Maxime have been fixed.
I have tested it on my Beelink GS1 board.
Thanks, Clement
0: https://lore.kernel.org/patchwork/cover/1139949/
Changes since v1: - Fix missing header in set sign extend sample
Jernej Skrabec (3): dt-bindings: ASoC: sun4i-i2s: Add H6 compatible ASoC: sun4i-i2s: Add support for H6 I2S arm64: dts: sun50i-h6: Add HDMI audio to H6 DTSI
Marcus Cooper (4): ASoC: sun4i-i2s: Adjust LRCLK width ASoC: sun4i-i2s: Set sign extend sample ASoc: sun4i-i2s: Add 20 and 24 bit support ASoC: sun4i-i2s: Adjust regmap settings
.../sound/allwinner,sun4i-a10-i2s.yaml | 2 + arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 31 ++ sound/soc/sunxi/sun4i-i2s.c | 292 ++++++++++++++++-- 3 files changed, 301 insertions(+), 24 deletions(-)
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 --- 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 d0a8d5810c0a..4198a5410bf9 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -455,6 +455,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));
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 --- .../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:
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 4198a5410bf9..a23c9f2a3f8c 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;
/** @@ -469,6 +484,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) @@ -694,6 +768,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); @@ -974,6 +1150,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, @@ -1001,6 +1193,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); @@ -1159,6 +1364,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) { @@ -1328,6 +1551,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);
From: Marcus Cooper codekipper@gmail.com
On the newer SoCs such as the H3 and A64 this is set by default to transfer a 0 after each sample in each slot. However the A10 and A20 SoCs that this driver was developed on had a default setting where it padded the audio gain with zeros.
This isn't a problem whilst we have only support for 16bit audio but with larger sample resolution rates in the pipeline then SEXT bits should be cleared so that they also pad at the LSB. Without this the audio gets distorted.
Signed-off-by: Marcus Cooper codekipper@gmail.com Signed-off-by: Clément Péron peron.clem@gmail.com --- sound/soc/sunxi/sun4i-i2s.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index a23c9f2a3f8c..618bbc5156f1 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -48,6 +48,9 @@ #define SUN4I_I2S_FMT0_FMT_I2S (0 << 0)
#define SUN4I_I2S_FMT1_REG 0x08 +#define SUN4I_I2S_FMT1_REG_SEXT_MASK BIT(8) +#define SUN4I_I2S_FMT1_REG_SEXT(sext) ((sext) << 8) + #define SUN4I_I2S_FIFO_TX_REG 0x0c #define SUN4I_I2S_FIFO_RX_REG 0x10
@@ -105,6 +108,9 @@ #define SUN8I_I2S_FMT0_BCLK_POLARITY_INVERTED (1 << 7) #define SUN8I_I2S_FMT0_BCLK_POLARITY_NORMAL (0 << 7)
+#define SUN8I_I2S_FMT1_REG_SEXT_MASK GENMASK(5,4) +#define SUN8I_I2S_FMT1_REG_SEXT(sext) ((sext) << 4) + #define SUN8I_I2S_INT_STA_REG 0x0c #define SUN8I_I2S_FIFO_TX_REG 0x20
@@ -663,6 +669,12 @@ static int sun4i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, } regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG, SUN4I_I2S_CTRL_MODE_MASK, val); + + /* Set sign extension to pad out LSB with 0 */ + regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT1_REG, + SUN4I_I2S_FMT1_REG_SEXT_MASK, + SUN4I_I2S_FMT1_REG_SEXT(0)); + return 0; }
@@ -765,6 +777,11 @@ static int sun8i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, SUN8I_I2S_CTRL_BCLK_OUT | SUN8I_I2S_CTRL_LRCK_OUT, val);
+ /* Set sign extension to pad out LSB with 0 */ + regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT1_REG, + SUN8I_I2S_FMT1_REG_SEXT_MASK, + SUN8I_I2S_FMT1_REG_SEXT(0)); + return 0; }
@@ -867,6 +884,11 @@ static int sun50i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, SUN8I_I2S_CTRL_BCLK_OUT | SUN8I_I2S_CTRL_LRCK_OUT, val);
+ /* Set sign extension to pad out LSB with 0 */ + regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT1_REG, + SUN8I_I2S_FMT1_REG_SEXT_MASK, + SUN8I_I2S_FMT1_REG_SEXT(0)); + return 0; }
On Sun, Apr 19, 2020 at 12:44:32AM +0200, Clément Péron wrote:
From: Marcus Cooper codekipper@gmail.com
On the newer SoCs such as the H3 and A64 this is set by default to transfer a 0 after each sample in each slot. However the A10 and A20 SoCs that this driver was developed on had a default setting where it padded the audio gain with zeros.
This isn't a problem whilst we have only support for 16bit audio but with larger sample resolution rates in the pipeline then SEXT bits should be cleared so that they also pad at the LSB. Without this the audio gets distorted.
Signed-off-by: Marcus Cooper codekipper@gmail.com Signed-off-by: Clément Péron peron.clem@gmail.com
sound/soc/sunxi/sun4i-i2s.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index a23c9f2a3f8c..618bbc5156f1 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -48,6 +48,9 @@ #define SUN4I_I2S_FMT0_FMT_I2S (0 << 0)
#define SUN4I_I2S_FMT1_REG 0x08 +#define SUN4I_I2S_FMT1_REG_SEXT_MASK BIT(8) +#define SUN4I_I2S_FMT1_REG_SEXT(sext) ((sext) << 8)
#define SUN4I_I2S_FIFO_TX_REG 0x0c #define SUN4I_I2S_FIFO_RX_REG 0x10
@@ -105,6 +108,9 @@ #define SUN8I_I2S_FMT0_BCLK_POLARITY_INVERTED (1 << 7) #define SUN8I_I2S_FMT0_BCLK_POLARITY_NORMAL (0 << 7)
+#define SUN8I_I2S_FMT1_REG_SEXT_MASK GENMASK(5,4) +#define SUN8I_I2S_FMT1_REG_SEXT(sext) ((sext) << 4)
#define SUN8I_I2S_INT_STA_REG 0x0c #define SUN8I_I2S_FIFO_TX_REG 0x20
@@ -663,6 +669,12 @@ static int sun4i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, } regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG, SUN4I_I2S_CTRL_MODE_MASK, val);
- /* Set sign extension to pad out LSB with 0 */
- regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT1_REG,
SUN4I_I2S_FMT1_REG_SEXT_MASK,SUN4I_I2S_FMT1_REG_SEXT(0));- return 0;
}
@@ -765,6 +777,11 @@ static int sun8i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, SUN8I_I2S_CTRL_BCLK_OUT | SUN8I_I2S_CTRL_LRCK_OUT, val);
- /* Set sign extension to pad out LSB with 0 */
- regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT1_REG,
SUN8I_I2S_FMT1_REG_SEXT_MASK,SUN8I_I2S_FMT1_REG_SEXT(0));- return 0;
}
@@ -867,6 +884,11 @@ static int sun50i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, SUN8I_I2S_CTRL_BCLK_OUT | SUN8I_I2S_CTRL_LRCK_OUT, val);
- /* Set sign extension to pad out LSB with 0 */
- regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT1_REG,
SUN8I_I2S_FMT1_REG_SEXT_MASK,SUN8I_I2S_FMT1_REG_SEXT(0));
If this is an issue only on the A10 / A20, why are you setting it up on the other generations too?
Maxime
Hi Maxime,
On Mon, 20 Apr 2020 at 14:44, Maxime Ripard maxime@cerno.tech wrote:
On Sun, Apr 19, 2020 at 12:44:32AM +0200, Clément Péron wrote:
From: Marcus Cooper codekipper@gmail.com
On the newer SoCs such as the H3 and A64 this is set by default to transfer a 0 after each sample in each slot. However the A10 and A20 SoCs that this driver was developed on had a default setting where it padded the audio gain with zeros.
This isn't a problem whilst we have only support for 16bit audio but with larger sample resolution rates in the pipeline then SEXT bits should be cleared so that they also pad at the LSB. Without this the audio gets distorted.
Signed-off-by: Marcus Cooper codekipper@gmail.com Signed-off-by: Clément Péron peron.clem@gmail.com
sound/soc/sunxi/sun4i-i2s.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index a23c9f2a3f8c..618bbc5156f1 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -48,6 +48,9 @@ #define SUN4I_I2S_FMT0_FMT_I2S (0 << 0)
#define SUN4I_I2S_FMT1_REG 0x08 +#define SUN4I_I2S_FMT1_REG_SEXT_MASK BIT(8) +#define SUN4I_I2S_FMT1_REG_SEXT(sext) ((sext) << 8)
#define SUN4I_I2S_FIFO_TX_REG 0x0c #define SUN4I_I2S_FIFO_RX_REG 0x10
@@ -105,6 +108,9 @@ #define SUN8I_I2S_FMT0_BCLK_POLARITY_INVERTED (1 << 7) #define SUN8I_I2S_FMT0_BCLK_POLARITY_NORMAL (0 << 7)
+#define SUN8I_I2S_FMT1_REG_SEXT_MASK GENMASK(5,4) +#define SUN8I_I2S_FMT1_REG_SEXT(sext) ((sext) << 4)
#define SUN8I_I2S_INT_STA_REG 0x0c #define SUN8I_I2S_FIFO_TX_REG 0x20
@@ -663,6 +669,12 @@ static int sun4i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, } regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG, SUN4I_I2S_CTRL_MODE_MASK, val);
/* Set sign extension to pad out LSB with 0 */regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT1_REG,SUN4I_I2S_FMT1_REG_SEXT_MASK,SUN4I_I2S_FMT1_REG_SEXT(0));return 0;}
@@ -765,6 +777,11 @@ static int sun8i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, SUN8I_I2S_CTRL_BCLK_OUT | SUN8I_I2S_CTRL_LRCK_OUT, val);
/* Set sign extension to pad out LSB with 0 */regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT1_REG,SUN8I_I2S_FMT1_REG_SEXT_MASK,SUN8I_I2S_FMT1_REG_SEXT(0));return 0;}
@@ -867,6 +884,11 @@ static int sun50i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, SUN8I_I2S_CTRL_BCLK_OUT | SUN8I_I2S_CTRL_LRCK_OUT, val);
/* Set sign extension to pad out LSB with 0 */regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT1_REG,SUN8I_I2S_FMT1_REG_SEXT_MASK,SUN8I_I2S_FMT1_REG_SEXT(0));If this is an issue only on the A10 / A20, why are you setting it up on the other generations too?
To keep coherency between all set_soc_format(), and also avoid this kind of issue for future generation. As this doesn't cost much after all, and it avoid to rely on default, but what do you think ?
Clement
Maxime
On Mon, Apr 20, 2020 at 02:56:31PM +0200, Clément Péron wrote:
On Mon, 20 Apr 2020 at 14:44, Maxime Ripard maxime@cerno.tech wrote:
On Sun, Apr 19, 2020 at 12:44:32AM +0200, Clément Péron wrote:
From: Marcus Cooper codekipper@gmail.com
On the newer SoCs such as the H3 and A64 this is set by default to transfer a 0 after each sample in each slot. However the A10 and A20 SoCs that this driver was developed on had a default setting where it padded the audio gain with zeros.
This isn't a problem whilst we have only support for 16bit audio but with larger sample resolution rates in the pipeline then SEXT bits should be cleared so that they also pad at the LSB. Without this the audio gets distorted.
Signed-off-by: Marcus Cooper codekipper@gmail.com Signed-off-by: Clément Péron peron.clem@gmail.com
sound/soc/sunxi/sun4i-i2s.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index a23c9f2a3f8c..618bbc5156f1 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -48,6 +48,9 @@ #define SUN4I_I2S_FMT0_FMT_I2S (0 << 0)
#define SUN4I_I2S_FMT1_REG 0x08 +#define SUN4I_I2S_FMT1_REG_SEXT_MASK BIT(8) +#define SUN4I_I2S_FMT1_REG_SEXT(sext) ((sext) << 8)
#define SUN4I_I2S_FIFO_TX_REG 0x0c #define SUN4I_I2S_FIFO_RX_REG 0x10
@@ -105,6 +108,9 @@ #define SUN8I_I2S_FMT0_BCLK_POLARITY_INVERTED (1 << 7) #define SUN8I_I2S_FMT0_BCLK_POLARITY_NORMAL (0 << 7)
+#define SUN8I_I2S_FMT1_REG_SEXT_MASK GENMASK(5,4) +#define SUN8I_I2S_FMT1_REG_SEXT(sext) ((sext) << 4)
#define SUN8I_I2S_INT_STA_REG 0x0c #define SUN8I_I2S_FIFO_TX_REG 0x20
@@ -663,6 +669,12 @@ static int sun4i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, } regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG, SUN4I_I2S_CTRL_MODE_MASK, val);
/* Set sign extension to pad out LSB with 0 */regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT1_REG,SUN4I_I2S_FMT1_REG_SEXT_MASK,SUN4I_I2S_FMT1_REG_SEXT(0));return 0;}
@@ -765,6 +777,11 @@ static int sun8i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, SUN8I_I2S_CTRL_BCLK_OUT | SUN8I_I2S_CTRL_LRCK_OUT, val);
/* Set sign extension to pad out LSB with 0 */regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT1_REG,SUN8I_I2S_FMT1_REG_SEXT_MASK,SUN8I_I2S_FMT1_REG_SEXT(0));return 0;}
@@ -867,6 +884,11 @@ static int sun50i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s, SUN8I_I2S_CTRL_BCLK_OUT | SUN8I_I2S_CTRL_LRCK_OUT, val);
/* Set sign extension to pad out LSB with 0 */regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT1_REG,SUN8I_I2S_FMT1_REG_SEXT_MASK,SUN8I_I2S_FMT1_REG_SEXT(0));If this is an issue only on the A10 / A20, why are you setting it up on the other generations too?
To keep coherency between all set_soc_format(), and also avoid this kind of issue for future generation. As this doesn't cost much after all, and it avoid to rely on default, but what do you think ?
It makes sense, but it should be in the commit log ;)
Maxime
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 618bbc5156f1..9778af37fbca 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,
From: Marcus Cooper codekipper@gmail.com
Bypass the regmap cache when flushing the i2s FIFOs and modify the tables to reflect this.
Signed-off-by: Marcus Cooper codekipper@gmail.com Signed-off-by: Clément Péron peron.clem@gmail.com --- sound/soc/sunxi/sun4i-i2s.c | 31 ++++++++----------------------- 1 file changed, 8 insertions(+), 23 deletions(-)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index 9778af37fbca..9053d290dd87 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -921,7 +921,7 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) static void sun4i_i2s_start_capture(struct sun4i_i2s *i2s) { /* Flush RX FIFO */ - regmap_update_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG, + regmap_write_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG, SUN4I_I2S_FIFO_CTRL_FLUSH_RX, SUN4I_I2S_FIFO_CTRL_FLUSH_RX);
@@ -942,7 +942,7 @@ static void sun4i_i2s_start_capture(struct sun4i_i2s *i2s) static void sun4i_i2s_start_playback(struct sun4i_i2s *i2s) { /* Flush TX FIFO */ - regmap_update_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG, + regmap_write_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG, SUN4I_I2S_FIFO_CTRL_FLUSH_TX, SUN4I_I2S_FIFO_CTRL_FLUSH_TX);
@@ -1096,13 +1096,7 @@ static const struct snd_soc_component_driver sun4i_i2s_component = {
static bool sun4i_i2s_rd_reg(struct device *dev, unsigned int reg) { - switch (reg) { - case SUN4I_I2S_FIFO_TX_REG: - return false; - - default: - return true; - } + return true; }
static bool sun4i_i2s_wr_reg(struct device *dev, unsigned int reg) @@ -1121,6 +1115,8 @@ static bool sun4i_i2s_volatile_reg(struct device *dev, unsigned int reg) { switch (reg) { case SUN4I_I2S_FIFO_RX_REG: + case SUN4I_I2S_FIFO_TX_REG: + case SUN4I_I2S_FIFO_STA_REG: case SUN4I_I2S_INT_STA_REG: case SUN4I_I2S_RX_CNT_REG: case SUN4I_I2S_TX_CNT_REG: @@ -1131,23 +1127,12 @@ static bool sun4i_i2s_volatile_reg(struct device *dev, unsigned int reg) } }
-static bool sun8i_i2s_rd_reg(struct device *dev, unsigned int reg) -{ - switch (reg) { - case SUN8I_I2S_FIFO_TX_REG: - return false; - - default: - return true; - } -} - static bool sun8i_i2s_volatile_reg(struct device *dev, unsigned int reg) { if (reg == SUN8I_I2S_INT_STA_REG) return true; if (reg == SUN8I_I2S_FIFO_TX_REG) - return false; + return true;
return sun4i_i2s_volatile_reg(dev, reg); } @@ -1218,7 +1203,7 @@ static const struct regmap_config sun8i_i2s_regmap_config = { .reg_defaults = sun8i_i2s_reg_defaults, .num_reg_defaults = ARRAY_SIZE(sun8i_i2s_reg_defaults), .writeable_reg = sun4i_i2s_wr_reg, - .readable_reg = sun8i_i2s_rd_reg, + .readable_reg = sun4i_i2s_rd_reg, .volatile_reg = sun8i_i2s_volatile_reg, };
@@ -1231,7 +1216,7 @@ static const struct regmap_config sun50i_i2s_regmap_config = { .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, + .readable_reg = sun4i_i2s_rd_reg, .volatile_reg = sun8i_i2s_volatile_reg, };
Hi,
On Sun, Apr 19, 2020 at 12:44:34AM +0200, Clément Péron wrote:
From: Marcus Cooper codekipper@gmail.com
Bypass the regmap cache when flushing the i2s FIFOs and modify the tables to reflect this.
I think that commit log requires a bit more work.
As far as I can see, you're doing several things here:
Signed-off-by: Marcus Cooper codekipper@gmail.com Signed-off-by: Clément Péron peron.clem@gmail.com
sound/soc/sunxi/sun4i-i2s.c | 31 ++++++++----------------------- 1 file changed, 8 insertions(+), 23 deletions(-)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index 9778af37fbca..9053d290dd87 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -921,7 +921,7 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) static void sun4i_i2s_start_capture(struct sun4i_i2s *i2s) { /* Flush RX FIFO */
- regmap_update_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG,
- regmap_write_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG, SUN4I_I2S_FIFO_CTRL_FLUSH_RX, SUN4I_I2S_FIFO_CTRL_FLUSH_RX);
You change regmap_update_bits to regmap_write_bits, I assume this is what the commit log means by "bypassing the cache", so that every write actually gets done even if the bit is already set.
I'm not quite sure why it's needed though, since that bit is self-clearing.
@@ -942,7 +942,7 @@ static void sun4i_i2s_start_capture(struct sun4i_i2s *i2s) static void sun4i_i2s_start_playback(struct sun4i_i2s *i2s) { /* Flush TX FIFO */
- regmap_update_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG,
- regmap_write_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG, SUN4I_I2S_FIFO_CTRL_FLUSH_TX, SUN4I_I2S_FIFO_CTRL_FLUSH_TX);
Ditto.
@@ -1096,13 +1096,7 @@ static const struct snd_soc_component_driver sun4i_i2s_component = {
static bool sun4i_i2s_rd_reg(struct device *dev, unsigned int reg) {
- switch (reg) {
- case SUN4I_I2S_FIFO_TX_REG:
return false;- default:
return true;- }
- return true;
}
You seem to be allowing reads to FIFO_TX_REG now for some reason?
static bool sun4i_i2s_wr_reg(struct device *dev, unsigned int reg) @@ -1121,6 +1115,8 @@ static bool sun4i_i2s_volatile_reg(struct device *dev, unsigned int reg) { switch (reg) { case SUN4I_I2S_FIFO_RX_REG:
- case SUN4I_I2S_FIFO_TX_REG:
- case SUN4I_I2S_FIFO_STA_REG:
I assume that your issue was that the flushed bit got cached since the register wasn't volatile, and therefore each time we were supposed to flush, we actually ended up doing nothing. Marking it as volatile would prevent the cache to catch that write and make regmap_update_bits work, actually fixing what you mention in the commit log.
Either way, it should be in the log itself, and it doesn't really explain the rest of the patch either.
Maxime
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 | 31 ++++++++++++++++++++ 1 file changed, 31 insertions(+)
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi index a5ee68388bd3..558fe63739cb 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi @@ -88,6 +88,24 @@ (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>; };
+ sound_hdmi: sound { + compatible = "simple-audio-card"; + simple-audio-card,format = "i2s"; + simple-audio-card,name = "allwinner-hdmi"; + simple-audio-card,mclk-fs = <128>; + simple-audio-card,frame-inversion; + + simple-audio-card,codec { + sound-dai = <&hdmi>; + }; + + simple-audio-card,cpu { + sound-dai = <&i2s1>; + dai-tdm-slot-num = <2>; + dai-tdm-slot-width = <32>; + }; + }; + soc { compatible = "simple-bus"; #address-cells = <1>; @@ -581,6 +599,18 @@ }; };
+ 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"; + }; + spdif: spdif@5093000 { #sound-dai-cells = <0>; compatible = "allwinner,sun50i-h6-spdif"; @@ -711,6 +741,7 @@ };
hdmi: hdmi@6000000 { + #sound-dai-cells = <0>; compatible = "allwinner,sun50i-h6-dw-hdmi"; reg = <0x06000000 0x10000>; reg-io-width = <1>;
participants (2)
-
Clément Péron -
Maxime Ripard