[alsa-devel] [PATCH v4 0/9]ASoC: sun4i-i2s: Updates to the driver
From: Marcus Cooper codekipper@gmail.com
Hi All,
here is a patch series to add some improvements to the sun4i-i2s driver found whilst getting slave clocking and hdmi audio working on the newer SoCs. As the LibreELEC project is progressing extremely well then there has been some activity getting surround sound working and this is included.
The functionality included with the new patch set has been extended to cover more sample resolutions, multi-lane data output for HDMI audio and some bug fixes that have been discovered along the way.
I can see more usage of the tdm property since I last attempted to push these patches and the examples currently in mainline sort of the opposite to what I'm trying to achieve. When we first started looking at the i2s driver, the codecs that we were using allowed for the frame width to be determined based on the sampling resolution but in most use cases it seems that a fixed width is required(my highest priority should be to get HDMI audio support in). We're using the tdm property to override the old way to calculate the frame width. What I've seen in what has already been mainlined is that the i2s driver has a frame width that is fixed to 32 bits and this can be overridden using the tdm property.
I still need to investigate the FIFO syncing issues which i've not had a chance to change or address the concerns that broonie and wens brought up. This change has been moved to the top of the patch stack.
BR, CK
--- v4 changes compared to v3 are: - Moved patches around so that the more controversial of patches are at the top of the stack. - Added more details to commit messages. - Fixed 20bit audio PCM format to use 4 bytes. - Reduced number of flags used to indicate a new SoC.
v3 changes compared to v2 are: - added back slave mode changes - added back the use of tdm properties - changes to regmap and caching - removed loopback functionality - fixes to the channel offset mask
v2 changes compared to v1 are: - removed slave mode changes which didn't set mclk and bclk div. - removed use of tdm and now use a dedicated property. - fix commit message to better explain reason for sign extending - add divider calculations for newer SoCs. - add support for multi-lane i2s data output. - add support for 20, 24 and 32 bit samples. - add loopback property so blocks can be tested without a codec.
Marcus Cooper (9): ASoC: sun4i-i2s: Fix sun8i tx channel offset mask ASoC: sun4i-i2s: Add offset to RX channel select ASoC: sun4i-i2s: Add regmap field to sign extend sample ASoC: sun4i-i2s: Reduce quirks for sun8i-h3 ASoC: sun4i-i2s: Add set_tdm_slot functionality ASoC: sun4i-i2s: Add multi-lane functionality ASoC: sun4i-i2s: Add multichannel functionality ASoc: sun4i-i2s: Add 20, 24 and 32 bit support ASoC: sun4i-i2s: Adjust regmap settings
sound/soc/sunxi/sun4i-i2s.c | 242 ++++++++++++++++++++++++------------ 1 file changed, 164 insertions(+), 78 deletions(-)
From: Marcus Cooper codekipper@gmail.com
Although not causing any noticeable issues, the mask for the channel offset is covering too many bits.
Signed-off-by: Marcus Cooper codekipper@gmail.com --- sound/soc/sunxi/sun4i-i2s.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index c53bfed8d4c2..90bd3963d8ae 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -106,7 +106,7 @@
#define SUN8I_I2S_TX_CHAN_MAP_REG 0x44 #define SUN8I_I2S_TX_CHAN_SEL_REG 0x34 -#define SUN8I_I2S_TX_CHAN_OFFSET_MASK GENMASK(13, 11) +#define SUN8I_I2S_TX_CHAN_OFFSET_MASK GENMASK(13, 12) #define SUN8I_I2S_TX_CHAN_OFFSET(offset) (offset << 12) #define SUN8I_I2S_TX_CHAN_EN_MASK GENMASK(11, 4) #define SUN8I_I2S_TX_CHAN_EN(num_chan) (((1 << num_chan) - 1) << 4)
On Mon, Jun 03, 2019 at 07:47:27PM +0200, codekipper@gmail.com wrote:
From: Marcus Cooper codekipper@gmail.com
Although not causing any noticeable issues, the mask for the channel offset is covering too many bits.
Signed-off-by: Marcus Cooper codekipper@gmail.com
Acked-by: Maxime Ripard maxime.ripard@bootlin.com
Maxime
-- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Tue, Jun 4, 2019 at 3:34 PM Maxime Ripard maxime.ripard@bootlin.com wrote:
On Mon, Jun 03, 2019 at 07:47:27PM +0200, codekipper@gmail.com wrote:
From: Marcus Cooper codekipper@gmail.com
Although not causing any noticeable issues, the mask for the channel offset is covering too many bits.
Signed-off-by: Marcus Cooper codekipper@gmail.com
Acked-by: Maxime Ripard maxime.ripard@bootlin.com
Would be nice to have
Fixes: 7d2993811a1e ("ASoC: sun4i-i2s: Add support for H3")
But otherwise,
Acked-by: Chen-Yu Tsai wens@csie.org
On Tue, 4 Jun 2019 at 09:39, Chen-Yu Tsai wens@csie.org wrote:
On Tue, Jun 4, 2019 at 3:34 PM Maxime Ripard maxime.ripard@bootlin.com wrote:
On Mon, Jun 03, 2019 at 07:47:27PM +0200, codekipper@gmail.com wrote:
From: Marcus Cooper codekipper@gmail.com
Although not causing any noticeable issues, the mask for the channel offset is covering too many bits.
Signed-off-by: Marcus Cooper codekipper@gmail.com
Acked-by: Maxime Ripard maxime.ripard@bootlin.com
Would be nice to have
Fixes: 7d2993811a1e ("ASoC: sun4i-i2s: Add support for H3")
Thanks....I'll keep this in mind for future reference as jernej also mention this to me. BR, CK
But otherwise,
Acked-by: Chen-Yu Tsai wens@csie.org
The patch
ASoC: sun4i-i2s: Fix sun8i tx channel offset mask
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.2
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From 7e46169a5f35762f335898a75d1b8a242f2ae0f5 Mon Sep 17 00:00:00 2001
From: Marcus Cooper codekipper@gmail.com Date: Mon, 3 Jun 2019 19:47:27 +0200 Subject: [PATCH] ASoC: sun4i-i2s: Fix sun8i tx channel offset mask
Although not causing any noticeable issues, the mask for the channel offset is covering too many bits.
Signed-off-by: Marcus Cooper codekipper@gmail.com Acked-by: Maxime Ripard maxime.ripard@bootlin.com Acked-by: Chen-Yu Tsai wens@csie.org Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/sunxi/sun4i-i2s.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index d5ec1a20499d..8162e107e50b 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -110,7 +110,7 @@
#define SUN8I_I2S_TX_CHAN_MAP_REG 0x44 #define SUN8I_I2S_TX_CHAN_SEL_REG 0x34 -#define SUN8I_I2S_TX_CHAN_OFFSET_MASK GENMASK(13, 11) +#define SUN8I_I2S_TX_CHAN_OFFSET_MASK GENMASK(13, 12) #define SUN8I_I2S_TX_CHAN_OFFSET(offset) (offset << 12) #define SUN8I_I2S_TX_CHAN_EN_MASK GENMASK(11, 4) #define SUN8I_I2S_TX_CHAN_EN(num_chan) (((1 << num_chan) - 1) << 4)
From: Marcus Cooper codekipper@gmail.com
Whilst testing the capture functionality of the i2s on the newer SoCs it was noticed that the recording was somewhat distorted. This was due to the offset not being set correctly on the receiver side.
Signed-off-by: Marcus Cooper codekipper@gmail.com --- sound/soc/sunxi/sun4i-i2s.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index 90bd3963d8ae..fd7c37596f21 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -456,6 +456,10 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) regmap_update_bits(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG, SUN8I_I2S_TX_CHAN_OFFSET_MASK, SUN8I_I2S_TX_CHAN_OFFSET(offset)); + + regmap_update_bits(i2s->regmap, SUN8I_I2S_RX_CHAN_SEL_REG, + SUN8I_I2S_TX_CHAN_OFFSET_MASK, + SUN8I_I2S_TX_CHAN_OFFSET(offset)); }
regmap_field_write(i2s->field_fmt_mode, val);
On Mon, Jun 03, 2019 at 07:47:28PM +0200, codekipper@gmail.com wrote:
From: Marcus Cooper codekipper@gmail.com
Whilst testing the capture functionality of the i2s on the newer SoCs it was noticed that the recording was somewhat distorted. This was due to the offset not being set correctly on the receiver side.
Signed-off-by: Marcus Cooper codekipper@gmail.com
Acked-by: Maxime Ripard maxime.ripard@bootlin.com
Maxime
-- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Tue, Jun 4, 2019 at 3:37 PM Maxime Ripard maxime.ripard@bootlin.com wrote:
On Mon, Jun 03, 2019 at 07:47:28PM +0200, codekipper@gmail.com wrote:
From: Marcus Cooper codekipper@gmail.com
Whilst testing the capture functionality of the i2s on the newer SoCs it was noticed that the recording was somewhat distorted. This was due to the offset not being set correctly on the receiver side.
Signed-off-by: Marcus Cooper codekipper@gmail.com
Acked-by: Maxime Ripard maxime.ripard@bootlin.com
Would be nice to have
Fixes: 7d2993811a1e ("ASoC: sun4i-i2s: Add support for H3")
But otherwise,
Acked-by: Chen-Yu Tsai wens@csie.org
The patch
ASoC: sun4i-i2s: Add offset to RX channel select
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.2
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From f9927000cb35f250051f0f1878db12ee2626eea1 Mon Sep 17 00:00:00 2001
From: Marcus Cooper codekipper@gmail.com Date: Mon, 3 Jun 2019 19:47:28 +0200 Subject: [PATCH] ASoC: sun4i-i2s: Add offset to RX channel select
Whilst testing the capture functionality of the i2s on the newer SoCs it was noticed that the recording was somewhat distorted. This was due to the offset not being set correctly on the receiver side.
Signed-off-by: Marcus Cooper codekipper@gmail.com Acked-by: Maxime Ripard maxime.ripard@bootlin.com Acked-by: Chen-Yu Tsai wens@csie.org Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/sunxi/sun4i-i2s.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index 8162e107e50b..bc128e2a6096 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -460,6 +460,10 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) regmap_update_bits(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG, SUN8I_I2S_TX_CHAN_OFFSET_MASK, SUN8I_I2S_TX_CHAN_OFFSET(offset)); + + regmap_update_bits(i2s->regmap, SUN8I_I2S_RX_CHAN_SEL_REG, + SUN8I_I2S_TX_CHAN_OFFSET_MASK, + SUN8I_I2S_TX_CHAN_OFFSET(offset)); }
regmap_field_write(i2s->field_fmt_mode, val);
From: Marcus Cooper codekipper@gmail.com
On the newer SoCs this is set by default to transfer a 0 after each sample in each slot. However the platform that this driver was developed on had the 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 it should be fixed to also pad. Without this the audio gets distorted.
Signed-off-by: Marcus Cooper codekipper@gmail.com --- sound/soc/sunxi/sun4i-i2s.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index fd7c37596f21..e2961d8f6e8c 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -134,6 +134,7 @@ * @field_fmt_bclk: regmap field to set clk polarity. * @field_fmt_lrclk: regmap field to set frame polarity. * @field_fmt_mode: regmap field to set the operational mode. + * @field_fmt_sext: regmap field to set the sign extension. * @field_txchanmap: location of the tx channel mapping register. * @field_rxchanmap: location of the rx channel mapping register. * @field_txchansel: location of the tx channel select bit fields. @@ -159,6 +160,7 @@ struct sun4i_i2s_quirks { struct reg_field field_fmt_bclk; struct reg_field field_fmt_lrclk; struct reg_field field_fmt_mode; + struct reg_field field_fmt_sext; struct reg_field field_txchanmap; struct reg_field field_rxchanmap; struct reg_field field_txchansel; @@ -183,6 +185,7 @@ struct sun4i_i2s { struct regmap_field *field_fmt_bclk; struct regmap_field *field_fmt_lrclk; struct regmap_field *field_fmt_mode; + struct regmap_field *field_fmt_sext; struct regmap_field *field_txchanmap; struct regmap_field *field_rxchanmap; struct regmap_field *field_txchansel; @@ -342,6 +345,9 @@ static int sun4i_i2s_set_clk_rate(struct snd_soc_dai *dai, SUN8I_I2S_FMT0_LRCK_PERIOD_MASK, SUN8I_I2S_FMT0_LRCK_PERIOD(32));
+ /* Set sign extension to pad out LSB with 0 */ + regmap_field_write(i2s->field_fmt_sext, 0); + return 0; }
@@ -887,6 +893,7 @@ static const struct sun4i_i2s_quirks sun4i_a10_i2s_quirks = { .field_fmt_lrclk = REG_FIELD(SUN4I_I2S_FMT0_REG, 7, 7), .has_slave_select_bit = true, .field_fmt_mode = REG_FIELD(SUN4I_I2S_FMT0_REG, 0, 1), + .field_fmt_sext = REG_FIELD(SUN4I_I2S_FMT1_REG, 8, 8), .field_txchanmap = REG_FIELD(SUN4I_I2S_TX_CHAN_MAP_REG, 0, 31), .field_rxchanmap = REG_FIELD(SUN4I_I2S_RX_CHAN_MAP_REG, 0, 31), .field_txchansel = REG_FIELD(SUN4I_I2S_TX_CHAN_SEL_REG, 0, 2), @@ -904,6 +911,7 @@ static const struct sun4i_i2s_quirks sun6i_a31_i2s_quirks = { .field_fmt_lrclk = REG_FIELD(SUN4I_I2S_FMT0_REG, 7, 7), .has_slave_select_bit = true, .field_fmt_mode = REG_FIELD(SUN4I_I2S_FMT0_REG, 0, 1), + .field_fmt_sext = REG_FIELD(SUN4I_I2S_FMT1_REG, 8, 8), .field_txchanmap = REG_FIELD(SUN4I_I2S_TX_CHAN_MAP_REG, 0, 31), .field_rxchanmap = REG_FIELD(SUN4I_I2S_RX_CHAN_MAP_REG, 0, 31), .field_txchansel = REG_FIELD(SUN4I_I2S_TX_CHAN_SEL_REG, 0, 2), @@ -944,6 +952,7 @@ static const struct sun4i_i2s_quirks sun8i_h3_i2s_quirks = { .field_fmt_bclk = REG_FIELD(SUN4I_I2S_FMT0_REG, 7, 7), .field_fmt_lrclk = REG_FIELD(SUN4I_I2S_FMT0_REG, 19, 19), .field_fmt_mode = REG_FIELD(SUN4I_I2S_CTRL_REG, 4, 5), + .field_fmt_sext = REG_FIELD(SUN4I_I2S_FMT1_REG, 4, 5), .field_txchanmap = REG_FIELD(SUN8I_I2S_TX_CHAN_MAP_REG, 0, 31), .field_rxchanmap = REG_FIELD(SUN8I_I2S_RX_CHAN_MAP_REG, 0, 31), .field_txchansel = REG_FIELD(SUN8I_I2S_TX_CHAN_SEL_REG, 0, 2), @@ -1006,6 +1015,12 @@ static int sun4i_i2s_init_regmap_fields(struct device *dev, if (IS_ERR(i2s->field_fmt_mode)) return PTR_ERR(i2s->field_fmt_mode);
+ i2s->field_fmt_sext = + devm_regmap_field_alloc(dev, i2s->regmap, + i2s->variant->field_fmt_sext); + if (IS_ERR(i2s->field_fmt_sext)) + return PTR_ERR(i2s->field_fmt_sext); + i2s->field_txchanmap = devm_regmap_field_alloc(dev, i2s->regmap, i2s->variant->field_txchanmap);
On Mon, Jun 03, 2019 at 07:47:29PM +0200, codekipper@gmail.com wrote:
From: Marcus Cooper codekipper@gmail.com
On the newer SoCs this is set by default to transfer a 0 after
Which SoCs?
each sample in each slot. However the platform that this driver
Which platform?
was developed on had the 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 it should be fixed to also pad. Without this the audio gets distorted.
Signed-off-by: Marcus Cooper codekipper@gmail.com
Once the commit log fixed, Acked-by: Maxime Ripard maxime.ripard@bootlin.com
Maxime
-- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Tue, Jun 4, 2019 at 1:47 AM codekipper@gmail.com wrote:
From: Marcus Cooper codekipper@gmail.com
On the newer SoCs this is set by default to transfer a 0 after each sample in each slot. However the platform that this driver was developed on had the 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 it should be fixed to also pad. Without this the audio gets distorted.
Curious, both the A10 and A20 manuals say the default value for this field is 0, which means 0 padding.
sun4i_i2s_reg_defaults[] also has that field set to 0.
You're saying you are seeing the field set to 1?
ChenYu
Signed-off-by: Marcus Cooper codekipper@gmail.com
sound/soc/sunxi/sun4i-i2s.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index fd7c37596f21..e2961d8f6e8c 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -134,6 +134,7 @@
- @field_fmt_bclk: regmap field to set clk polarity.
- @field_fmt_lrclk: regmap field to set frame polarity.
- @field_fmt_mode: regmap field to set the operational mode.
- @field_fmt_sext: regmap field to set the sign extension.
- @field_txchanmap: location of the tx channel mapping register.
- @field_rxchanmap: location of the rx channel mapping register.
- @field_txchansel: location of the tx channel select bit fields.
@@ -159,6 +160,7 @@ struct sun4i_i2s_quirks { struct reg_field field_fmt_bclk; struct reg_field field_fmt_lrclk; struct reg_field field_fmt_mode;
struct reg_field field_fmt_sext; struct reg_field field_txchanmap; struct reg_field field_rxchanmap; struct reg_field field_txchansel;@@ -183,6 +185,7 @@ struct sun4i_i2s { struct regmap_field *field_fmt_bclk; struct regmap_field *field_fmt_lrclk; struct regmap_field *field_fmt_mode;
struct regmap_field *field_fmt_sext; struct regmap_field *field_txchanmap; struct regmap_field *field_rxchanmap; struct regmap_field *field_txchansel;@@ -342,6 +345,9 @@ static int sun4i_i2s_set_clk_rate(struct snd_soc_dai *dai, SUN8I_I2S_FMT0_LRCK_PERIOD_MASK, SUN8I_I2S_FMT0_LRCK_PERIOD(32));
/* Set sign extension to pad out LSB with 0 */regmap_field_write(i2s->field_fmt_sext, 0);return 0;}
@@ -887,6 +893,7 @@ static const struct sun4i_i2s_quirks sun4i_a10_i2s_quirks = { .field_fmt_lrclk = REG_FIELD(SUN4I_I2S_FMT0_REG, 7, 7), .has_slave_select_bit = true, .field_fmt_mode = REG_FIELD(SUN4I_I2S_FMT0_REG, 0, 1),
.field_fmt_sext = REG_FIELD(SUN4I_I2S_FMT1_REG, 8, 8), .field_txchanmap = REG_FIELD(SUN4I_I2S_TX_CHAN_MAP_REG, 0, 31), .field_rxchanmap = REG_FIELD(SUN4I_I2S_RX_CHAN_MAP_REG, 0, 31), .field_txchansel = REG_FIELD(SUN4I_I2S_TX_CHAN_SEL_REG, 0, 2),@@ -904,6 +911,7 @@ static const struct sun4i_i2s_quirks sun6i_a31_i2s_quirks = { .field_fmt_lrclk = REG_FIELD(SUN4I_I2S_FMT0_REG, 7, 7), .has_slave_select_bit = true, .field_fmt_mode = REG_FIELD(SUN4I_I2S_FMT0_REG, 0, 1),
.field_fmt_sext = REG_FIELD(SUN4I_I2S_FMT1_REG, 8, 8), .field_txchanmap = REG_FIELD(SUN4I_I2S_TX_CHAN_MAP_REG, 0, 31), .field_rxchanmap = REG_FIELD(SUN4I_I2S_RX_CHAN_MAP_REG, 0, 31), .field_txchansel = REG_FIELD(SUN4I_I2S_TX_CHAN_SEL_REG, 0, 2),@@ -944,6 +952,7 @@ static const struct sun4i_i2s_quirks sun8i_h3_i2s_quirks = { .field_fmt_bclk = REG_FIELD(SUN4I_I2S_FMT0_REG, 7, 7), .field_fmt_lrclk = REG_FIELD(SUN4I_I2S_FMT0_REG, 19, 19), .field_fmt_mode = REG_FIELD(SUN4I_I2S_CTRL_REG, 4, 5),
.field_fmt_sext = REG_FIELD(SUN4I_I2S_FMT1_REG, 4, 5), .field_txchanmap = REG_FIELD(SUN8I_I2S_TX_CHAN_MAP_REG, 0, 31), .field_rxchanmap = REG_FIELD(SUN8I_I2S_RX_CHAN_MAP_REG, 0, 31), .field_txchansel = REG_FIELD(SUN8I_I2S_TX_CHAN_SEL_REG, 0, 2),@@ -1006,6 +1015,12 @@ static int sun4i_i2s_init_regmap_fields(struct device *dev, if (IS_ERR(i2s->field_fmt_mode)) return PTR_ERR(i2s->field_fmt_mode);
i2s->field_fmt_sext =devm_regmap_field_alloc(dev, i2s->regmap,i2s->variant->field_fmt_sext);if (IS_ERR(i2s->field_fmt_sext))return PTR_ERR(i2s->field_fmt_sext);i2s->field_txchanmap = devm_regmap_field_alloc(dev, i2s->regmap, i2s->variant->field_txchanmap);-- 2.21.0
-- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com. To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20190603174735.21002-4-codekip.... For more options, visit https://groups.google.com/d/optout.
On Tue, 4 Jun 2019 at 09:53, Chen-Yu Tsai wens@csie.org wrote:
On Tue, Jun 4, 2019 at 1:47 AM codekipper@gmail.com wrote:
From: Marcus Cooper codekipper@gmail.com
On the newer SoCs this is set by default to transfer a 0 after each sample in each slot. However the platform that this driver was developed on had the 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 it should be fixed to also pad. Without this the audio gets distorted.
Curious, both the A10 and A20 manuals say the default value for this field is 0, which means 0 padding.
sun4i_i2s_reg_defaults[] also has that field set to 0.
You're saying you are seeing the field set to 1?
On the newer SoCs (H3 onwards) this setting defaults to 3 which is "Transfer 0 after each sample in each slot" which resulted in distortion. Setting SEXT to 0 "Zeros or audio gain padding at LSB" alligns the setup with that of the earlier block and fixed the issue we were hearing. It's really noticeable with HDMI audio. BR, CK
ChenYu
Signed-off-by: Marcus Cooper codekipper@gmail.com
sound/soc/sunxi/sun4i-i2s.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index fd7c37596f21..e2961d8f6e8c 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -134,6 +134,7 @@
- @field_fmt_bclk: regmap field to set clk polarity.
- @field_fmt_lrclk: regmap field to set frame polarity.
- @field_fmt_mode: regmap field to set the operational mode.
- @field_fmt_sext: regmap field to set the sign extension.
- @field_txchanmap: location of the tx channel mapping register.
- @field_rxchanmap: location of the rx channel mapping register.
- @field_txchansel: location of the tx channel select bit fields.
@@ -159,6 +160,7 @@ struct sun4i_i2s_quirks { struct reg_field field_fmt_bclk; struct reg_field field_fmt_lrclk; struct reg_field field_fmt_mode;
struct reg_field field_fmt_sext; struct reg_field field_txchanmap; struct reg_field field_rxchanmap; struct reg_field field_txchansel;@@ -183,6 +185,7 @@ struct sun4i_i2s { struct regmap_field *field_fmt_bclk; struct regmap_field *field_fmt_lrclk; struct regmap_field *field_fmt_mode;
struct regmap_field *field_fmt_sext; struct regmap_field *field_txchanmap; struct regmap_field *field_rxchanmap; struct regmap_field *field_txchansel;@@ -342,6 +345,9 @@ static int sun4i_i2s_set_clk_rate(struct snd_soc_dai *dai, SUN8I_I2S_FMT0_LRCK_PERIOD_MASK, SUN8I_I2S_FMT0_LRCK_PERIOD(32));
/* Set sign extension to pad out LSB with 0 */regmap_field_write(i2s->field_fmt_sext, 0);return 0;}
@@ -887,6 +893,7 @@ static const struct sun4i_i2s_quirks sun4i_a10_i2s_quirks = { .field_fmt_lrclk = REG_FIELD(SUN4I_I2S_FMT0_REG, 7, 7), .has_slave_select_bit = true, .field_fmt_mode = REG_FIELD(SUN4I_I2S_FMT0_REG, 0, 1),
.field_fmt_sext = REG_FIELD(SUN4I_I2S_FMT1_REG, 8, 8), .field_txchanmap = REG_FIELD(SUN4I_I2S_TX_CHAN_MAP_REG, 0, 31), .field_rxchanmap = REG_FIELD(SUN4I_I2S_RX_CHAN_MAP_REG, 0, 31), .field_txchansel = REG_FIELD(SUN4I_I2S_TX_CHAN_SEL_REG, 0, 2),@@ -904,6 +911,7 @@ static const struct sun4i_i2s_quirks sun6i_a31_i2s_quirks = { .field_fmt_lrclk = REG_FIELD(SUN4I_I2S_FMT0_REG, 7, 7), .has_slave_select_bit = true, .field_fmt_mode = REG_FIELD(SUN4I_I2S_FMT0_REG, 0, 1),
.field_fmt_sext = REG_FIELD(SUN4I_I2S_FMT1_REG, 8, 8), .field_txchanmap = REG_FIELD(SUN4I_I2S_TX_CHAN_MAP_REG, 0, 31), .field_rxchanmap = REG_FIELD(SUN4I_I2S_RX_CHAN_MAP_REG, 0, 31), .field_txchansel = REG_FIELD(SUN4I_I2S_TX_CHAN_SEL_REG, 0, 2),@@ -944,6 +952,7 @@ static const struct sun4i_i2s_quirks sun8i_h3_i2s_quirks = { .field_fmt_bclk = REG_FIELD(SUN4I_I2S_FMT0_REG, 7, 7), .field_fmt_lrclk = REG_FIELD(SUN4I_I2S_FMT0_REG, 19, 19), .field_fmt_mode = REG_FIELD(SUN4I_I2S_CTRL_REG, 4, 5),
.field_fmt_sext = REG_FIELD(SUN4I_I2S_FMT1_REG, 4, 5), .field_txchanmap = REG_FIELD(SUN8I_I2S_TX_CHAN_MAP_REG, 0, 31), .field_rxchanmap = REG_FIELD(SUN8I_I2S_RX_CHAN_MAP_REG, 0, 31), .field_txchansel = REG_FIELD(SUN8I_I2S_TX_CHAN_SEL_REG, 0, 2),@@ -1006,6 +1015,12 @@ static int sun4i_i2s_init_regmap_fields(struct device *dev, if (IS_ERR(i2s->field_fmt_mode)) return PTR_ERR(i2s->field_fmt_mode);
i2s->field_fmt_sext =devm_regmap_field_alloc(dev, i2s->regmap,i2s->variant->field_fmt_sext);if (IS_ERR(i2s->field_fmt_sext))return PTR_ERR(i2s->field_fmt_sext);i2s->field_txchanmap = devm_regmap_field_alloc(dev, i2s->regmap, i2s->variant->field_txchanmap);-- 2.21.0
-- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com. To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20190603174735.21002-4-codekip.... For more options, visit https://groups.google.com/d/optout.
From: Marcus Cooper codekipper@gmail.com
We have a number of flags used to identify the functionality of the IP block found on the sun8i-h3 and later devices. As it is only neccessary to identify this new block then replace these flags with just one.
Signed-off-by: Marcus Cooper codekipper@gmail.com --- sound/soc/sunxi/sun4i-i2s.c | 23 +++++++---------------- 1 file changed, 7 insertions(+), 16 deletions(-)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index e2961d8f6e8c..329883750d6f 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -119,10 +119,7 @@ * * @has_reset: SoC needs reset deasserted. * @has_slave_select_bit: SoC has a bit to enable slave mode. - * @has_fmt_set_lrck_period: SoC requires lrclk period to be set. - * @has_chcfg: tx and rx slot number need to be set. - * @has_chsel_tx_chen: SoC requires that the tx channels are enabled. - * @has_chsel_offset: SoC uses offset for selecting dai operational mode. + * @is_h3_i2s_based: This block is similiar to what is found on the h3. * @reg_offset_txdata: offset of the tx fifo. * @sun4i_i2s_regmap: regmap config to use. * @mclk_offset: Value by which mclkdiv needs to be adjusted. @@ -143,10 +140,7 @@ struct sun4i_i2s_quirks { bool has_reset; bool has_slave_select_bit; - bool has_fmt_set_lrck_period; - bool has_chcfg; - bool has_chsel_tx_chen; - bool has_chsel_offset; + bool is_h3_i2s_based; unsigned int reg_offset_txdata; /* TX FIFO */ const struct regmap_config *sun4i_i2s_regmap; unsigned int mclk_offset; @@ -340,7 +334,7 @@ static int sun4i_i2s_set_clk_rate(struct snd_soc_dai *dai, regmap_field_write(i2s->field_clkdiv_mclk_en, 1);
/* Set sync period */ - if (i2s->variant->has_fmt_set_lrck_period) + if (i2s->variant->is_h3_i2s_based) regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT0_REG, SUN8I_I2S_FMT0_LRCK_PERIOD_MASK, SUN8I_I2S_FMT0_LRCK_PERIOD(32)); @@ -366,7 +360,7 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, return -EINVAL; }
- if (i2s->variant->has_chcfg) { + if (i2s->variant->is_h3_i2s_based) { 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)); @@ -386,7 +380,7 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, regmap_field_write(i2s->field_rxchansel, SUN4I_I2S_CHAN_SEL(params_channels(params)));
- if (i2s->variant->has_chsel_tx_chen) + if (i2s->variant->is_h3_i2s_based) regmap_update_bits(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG, SUN8I_I2S_TX_CHAN_EN_MASK, SUN8I_I2S_TX_CHAN_EN(channels)); @@ -449,7 +443,7 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) return -EINVAL; }
- if (i2s->variant->has_chsel_offset) { + if (i2s->variant->is_h3_i2s_based) { /* * offset being set indicates that we're connected to an i2s * device, however offset is only used on the sun8i block and @@ -942,10 +936,7 @@ static const struct sun4i_i2s_quirks sun8i_h3_i2s_quirks = { .mclk_offset = 1, .bclk_offset = 2, .fmt_offset = 3, - .has_fmt_set_lrck_period = true, - .has_chcfg = true, - .has_chsel_tx_chen = true, - .has_chsel_offset = true, + .is_h3_i2s_based = true, .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),
On Mon, Jun 03, 2019 at 07:47:30PM +0200, codekipper@gmail.com wrote:
From: Marcus Cooper codekipper@gmail.com
We have a number of flags used to identify the functionality of the IP block found on the sun8i-h3 and later devices. As it is only neccessary to identify this new block then replace these flags with just one.
Signed-off-by: Marcus Cooper codekipper@gmail.com
This carries exactly the same meaning than the compatible, so this is entirely redundant.
The more I think of it, the more I fell like we should have function pointers instead, and have hooks to deal with these kind of things.
I've been working a lot on that driver recently, and there's some many parameters and regmap_fields that it becomes pretty hard to work on.
Maxime
-- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Tue, 4 Jun 2019 at 09:46, Maxime Ripard maxime.ripard@bootlin.com wrote:
On Mon, Jun 03, 2019 at 07:47:30PM +0200, codekipper@gmail.com wrote:
From: Marcus Cooper codekipper@gmail.com
We have a number of flags used to identify the functionality of the IP block found on the sun8i-h3 and later devices. As it is only neccessary to identify this new block then replace these flags with just one.
Signed-off-by: Marcus Cooper codekipper@gmail.com
This carries exactly the same meaning than the compatible, so this is entirely redundant.
The more I think of it, the more I fell like we should have function pointers instead, and have hooks to deal with these kind of things.
I've been working a lot on that driver recently, and there's some many parameters and regmap_fields that it becomes pretty hard to work on.
Hi Maxime, let's sync with what you're doing as you're more lightly to see it through to delivery! I was trying to clean up the driver as some of this seemed a bit unnecessary, hooks sounds like the way forward, CK
Maxime
-- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
From: Marcus Cooper codekipper@gmail.com
Some codecs require a different amount of a bit clocks per frame than what is calculated by the sample width. Use the tdm slot bindings to provide this mechanism.
Signed-off-by: Marcus Cooper codekipper@gmail.com --- sound/soc/sunxi/sun4i-i2s.c | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index 329883750d6f..bca73b3c0d74 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -186,6 +186,9 @@ struct sun4i_i2s { struct regmap_field *field_rxchansel;
const struct sun4i_i2s_quirks *variant; + + unsigned int tdm_slots; + unsigned int slot_width; };
struct sun4i_i2s_clk_div { @@ -337,7 +340,7 @@ static int sun4i_i2s_set_clk_rate(struct snd_soc_dai *dai, if (i2s->variant->is_h3_i2s_based) regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT0_REG, SUN8I_I2S_FMT0_LRCK_PERIOD_MASK, - SUN8I_I2S_FMT0_LRCK_PERIOD(32)); + SUN8I_I2S_FMT0_LRCK_PERIOD(word_size));
/* Set sign extension to pad out LSB with 0 */ regmap_field_write(i2s->field_fmt_sext, 0); @@ -414,7 +417,8 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, sr + i2s->variant->fmt_offset);
return sun4i_i2s_set_clk_rate(dai, params_rate(params), - params_width(params)); + i2s->tdm_slots ? + i2s->slot_width : params_width(params)); }
static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) @@ -657,11 +661,25 @@ static int sun4i_i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id, return 0; }
+static int sun4i_i2s_set_dai_tdm_slot(struct snd_soc_dai *dai, + unsigned int tx_mask, unsigned int rx_mask, + int slots, int width) +{ + struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai); + + i2s->tdm_slots = slots; + + i2s->slot_width = width; + + return 0; +} + static const struct snd_soc_dai_ops sun4i_i2s_dai_ops = { .hw_params = sun4i_i2s_hw_params, .set_fmt = sun4i_i2s_set_fmt, .set_sysclk = sun4i_i2s_set_sysclk, .trigger = sun4i_i2s_trigger, + .set_tdm_slot = sun4i_i2s_set_dai_tdm_slot, };
static int sun4i_i2s_dai_probe(struct snd_soc_dai *dai)
On Mon, Jun 03, 2019 at 07:47:31PM +0200, codekipper@gmail.com wrote:
From: Marcus Cooper codekipper@gmail.com
Some codecs require a different amount of a bit clocks per frame than
Which codec? And what are the actual requirements?
what is calculated by the sample width. Use the tdm slot bindings to provide this mechanism.
Signed-off-by: Marcus Cooper codekipper@gmail.com
sound/soc/sunxi/sun4i-i2s.c | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index 329883750d6f..bca73b3c0d74 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -186,6 +186,9 @@ struct sun4i_i2s { struct regmap_field *field_rxchansel;
const struct sun4i_i2s_quirks *variant;
- unsigned int tdm_slots;
- unsigned int slot_width;
};
struct sun4i_i2s_clk_div { @@ -337,7 +340,7 @@ static int sun4i_i2s_set_clk_rate(struct snd_soc_dai *dai, if (i2s->variant->is_h3_i2s_based) regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT0_REG, SUN8I_I2S_FMT0_LRCK_PERIOD_MASK,
SUN8I_I2S_FMT0_LRCK_PERIOD(32));
SUN8I_I2S_FMT0_LRCK_PERIOD(word_size));
This is an unrelated change, it should be in a separate patch.
/* Set sign extension to pad out LSB with 0 */ regmap_field_write(i2s->field_fmt_sext, 0); @@ -414,7 +417,8 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, sr + i2s->variant->fmt_offset);
return sun4i_i2s_set_clk_rate(dai, params_rate(params),
params_width(params));
i2s->tdm_slots ?i2s->slot_width : params_width(params));}
static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) @@ -657,11 +661,25 @@ static int sun4i_i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id, return 0; }
+static int sun4i_i2s_set_dai_tdm_slot(struct snd_soc_dai *dai,
- unsigned int tx_mask, unsigned int rx_mask,
- int slots, int width)
The alignment after the wraping should be at the opening parenthesis.
+{
- struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
- i2s->tdm_slots = slots;
- i2s->slot_width = width;
- return 0;
+}
static const struct snd_soc_dai_ops sun4i_i2s_dai_ops = { .hw_params = sun4i_i2s_hw_params, .set_fmt = sun4i_i2s_set_fmt, .set_sysclk = sun4i_i2s_set_sysclk, .trigger = sun4i_i2s_trigger,
- .set_tdm_slot = sun4i_i2s_set_dai_tdm_slot,
Please sort them by alphabetical order.
Thanks! Maxime
-- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
From: Marcus Cooper codekipper@gmail.com
The i2s block supports multi-lane i2s output however this functionality is only possible in earlier SoCs where the pins are exposed and for the i2s block used for HDMI audio on the later SoCs.
To enable this functionality, an optional property has been added to the bindings.
Signed-off-by: Marcus Cooper codekipper@gmail.com --- sound/soc/sunxi/sun4i-i2s.c | 44 ++++++++++++++++++++++++++++++++----- 1 file changed, 39 insertions(+), 5 deletions(-)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index bca73b3c0d74..75217fb52bfa 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -23,7 +23,7 @@
#define SUN4I_I2S_CTRL_REG 0x00 #define SUN4I_I2S_CTRL_SDO_EN_MASK GENMASK(11, 8) -#define SUN4I_I2S_CTRL_SDO_EN(sdo) BIT(8 + (sdo)) +#define SUN4I_I2S_CTRL_SDO_EN(lines) (((1 << lines) - 1) << 8) #define SUN4I_I2S_CTRL_MODE_MASK BIT(5) #define SUN4I_I2S_CTRL_MODE_SLAVE (1 << 5) #define SUN4I_I2S_CTRL_MODE_MASTER (0 << 5) @@ -355,14 +355,23 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai); int sr, wss, channels; u32 width; + int lines;
channels = params_channels(params); - if (channels != 2) { + if ((channels > dai->driver->playback.channels_max) || + (channels < dai->driver->playback.channels_min)) { dev_err(dai->dev, "Unsupported number of channels: %d\n", channels); return -EINVAL; }
+ lines = (channels + 1) / 2; + + /* Enable the required output lines */ + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG, + SUN4I_I2S_CTRL_SDO_EN_MASK, + SUN4I_I2S_CTRL_SDO_EN(lines)); + if (i2s->variant->is_h3_i2s_based) { regmap_update_bits(i2s->regmap, SUN8I_I2S_CHAN_CFG_REG, SUN8I_I2S_CHAN_CFG_TX_SLOT_NUM_MASK, @@ -373,8 +382,19 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, }
/* Map the channels for playback and capture */ - regmap_field_write(i2s->field_txchanmap, 0x76543210); regmap_field_write(i2s->field_rxchanmap, 0x00003210); + regmap_field_write(i2s->field_txchanmap, 0x10); + if (i2s->variant->is_h3_i2s_based) { + if (channels > 2) + regmap_write(i2s->regmap, + SUN8I_I2S_TX_CHAN_MAP_REG+4, 0x32); + if (channels > 4) + regmap_write(i2s->regmap, + SUN8I_I2S_TX_CHAN_MAP_REG+8, 0x54); + if (channels > 6) + regmap_write(i2s->regmap, + SUN8I_I2S_TX_CHAN_MAP_REG+12, 0x76); + }
/* Configure the channels */ regmap_field_write(i2s->field_txchansel, @@ -1057,9 +1077,10 @@ static int sun4i_i2s_init_regmap_fields(struct device *dev, static int sun4i_i2s_probe(struct platform_device *pdev) { struct sun4i_i2s *i2s; + struct snd_soc_dai_driver *soc_dai; struct resource *res; void __iomem *regs; - int irq, ret; + int irq, ret, val;
i2s = devm_kzalloc(&pdev->dev, sizeof(*i2s), GFP_KERNEL); if (!i2s) @@ -1126,6 +1147,19 @@ static int sun4i_i2s_probe(struct platform_device *pdev) i2s->capture_dma_data.addr = res->start + SUN4I_I2S_FIFO_RX_REG; i2s->capture_dma_data.maxburst = 8;
+ soc_dai = devm_kmemdup(&pdev->dev, &sun4i_i2s_dai, + sizeof(*soc_dai), GFP_KERNEL); + if (!soc_dai) { + ret = -ENOMEM; + goto err_pm_disable; + } + + if (!of_property_read_u32(pdev->dev.of_node, + "allwinner,playback-channels", &val)) { + if (val >= 2 && val <= 8) + soc_dai->playback.channels_max = val; + } + pm_runtime_enable(&pdev->dev); if (!pm_runtime_enabled(&pdev->dev)) { ret = sun4i_i2s_runtime_resume(&pdev->dev); @@ -1135,7 +1169,7 @@ static int sun4i_i2s_probe(struct platform_device *pdev)
ret = devm_snd_soc_register_component(&pdev->dev, &sun4i_i2s_component, - &sun4i_i2s_dai, 1); + soc_dai, 1); if (ret) { dev_err(&pdev->dev, "Could not register DAI\n"); goto err_suspend;
On Mon, Jun 03, 2019 at 07:47:32PM +0200, codekipper@gmail.com wrote:
From: Marcus Cooper codekipper@gmail.com
The i2s block supports multi-lane i2s output however this functionality is only possible in earlier SoCs where the pins are exposed and for the i2s block used for HDMI audio on the later SoCs.
To enable this functionality, an optional property has been added to the bindings.
Signed-off-by: Marcus Cooper codekipper@gmail.com
I'd like to have Mark's input on this, but I'm really worried about the interaction with the proper TDM support.
Our fundamental issue is that the controller can have up to 8 channels, but either on 4 lines (instead of 1), or 8 channels on 1 (like proper TDM) (or any combination between the two, but that should be pretty rare).
You're trying to do the first one, and I'm trying to do the second one.
There's a number of assumptions later on that will break the TDM case, see below for examples
sound/soc/sunxi/sun4i-i2s.c | 44 ++++++++++++++++++++++++++++++++----- 1 file changed, 39 insertions(+), 5 deletions(-)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index bca73b3c0d74..75217fb52bfa 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -23,7 +23,7 @@
#define SUN4I_I2S_CTRL_REG 0x00 #define SUN4I_I2S_CTRL_SDO_EN_MASK GENMASK(11, 8) -#define SUN4I_I2S_CTRL_SDO_EN(sdo) BIT(8 + (sdo)) +#define SUN4I_I2S_CTRL_SDO_EN(lines) (((1 << lines) - 1) << 8) #define SUN4I_I2S_CTRL_MODE_MASK BIT(5) #define SUN4I_I2S_CTRL_MODE_SLAVE (1 << 5) #define SUN4I_I2S_CTRL_MODE_MASTER (0 << 5) @@ -355,14 +355,23 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai); int sr, wss, channels; u32 width;
int lines;
channels = params_channels(params);
- if (channels != 2) {
if ((channels > dai->driver->playback.channels_max) ||
(channels < dai->driver->playback.channels_min)) {dev_err(dai->dev, "Unsupported number of channels: %d\n", channels); return -EINVAL; }
lines = (channels + 1) / 2;
/* Enable the required output lines */
regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
SUN4I_I2S_CTRL_SDO_EN_MASK,SUN4I_I2S_CTRL_SDO_EN(lines));
This has the assumption that each line will have 2 channels, which is wrong.
if (i2s->variant->is_h3_i2s_based) { regmap_update_bits(i2s->regmap, SUN8I_I2S_CHAN_CFG_REG, SUN8I_I2S_CHAN_CFG_TX_SLOT_NUM_MASK, @@ -373,8 +382,19 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, }
/* Map the channels for playback and capture */
- regmap_field_write(i2s->field_txchanmap, 0x76543210); regmap_field_write(i2s->field_rxchanmap, 0x00003210);
- regmap_field_write(i2s->field_txchanmap, 0x10);
- if (i2s->variant->is_h3_i2s_based) {
if (channels > 2)regmap_write(i2s->regmap,SUN8I_I2S_TX_CHAN_MAP_REG+4, 0x32);if (channels > 4)regmap_write(i2s->regmap,SUN8I_I2S_TX_CHAN_MAP_REG+8, 0x54);if (channels > 6)regmap_write(i2s->regmap,SUN8I_I2S_TX_CHAN_MAP_REG+12, 0x76);- }
And this creates a mapping matching that.
/* Configure the channels */ regmap_field_write(i2s->field_txchansel, @@ -1057,9 +1077,10 @@ static int sun4i_i2s_init_regmap_fields(struct device *dev, static int sun4i_i2s_probe(struct platform_device *pdev) { struct sun4i_i2s *i2s;
- struct snd_soc_dai_driver *soc_dai; struct resource *res; void __iomem *regs;
- int irq, ret;
int irq, ret, val;
i2s = devm_kzalloc(&pdev->dev, sizeof(*i2s), GFP_KERNEL); if (!i2s)
@@ -1126,6 +1147,19 @@ static int sun4i_i2s_probe(struct platform_device *pdev) i2s->capture_dma_data.addr = res->start + SUN4I_I2S_FIFO_RX_REG; i2s->capture_dma_data.maxburst = 8;
- soc_dai = devm_kmemdup(&pdev->dev, &sun4i_i2s_dai,
sizeof(*soc_dai), GFP_KERNEL);- if (!soc_dai) {
ret = -ENOMEM;goto err_pm_disable;- }
- if (!of_property_read_u32(pdev->dev.of_node,
"allwinner,playback-channels", &val)) {if (val >= 2 && val <= 8)soc_dai->playback.channels_max = val;- }
I'm not quite sure how this works.
of_property_read_u32 will return 0, so you will enter in the condition. But what happens if the property is missing?
Maxime
-- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Tue, 4 Jun 2019 at 09:58, Maxime Ripard maxime.ripard@bootlin.com wrote:
On Mon, Jun 03, 2019 at 07:47:32PM +0200, codekipper@gmail.com wrote:
From: Marcus Cooper codekipper@gmail.com
The i2s block supports multi-lane i2s output however this functionality is only possible in earlier SoCs where the pins are exposed and for the i2s block used for HDMI audio on the later SoCs.
To enable this functionality, an optional property has been added to the bindings.
Signed-off-by: Marcus Cooper codekipper@gmail.com
I'd like to have Mark's input on this, but I'm really worried about the interaction with the proper TDM support.
Our fundamental issue is that the controller can have up to 8 channels, but either on 4 lines (instead of 1), or 8 channels on 1 (like proper TDM) (or any combination between the two, but that should be pretty rare).
I understand...maybe the TDM needs to be extended to support this to consider channel mapping and multiple transfer lines. I was thinking about the later when someone was requesting support on IIRC a while ago, I thought masking might of been a solution. These can wait as the only consumer at the moment is LibreELEC and we can patch it there. Do you have any ideas Master? CK
You're trying to do the first one, and I'm trying to do the second one.
There's a number of assumptions later on that will break the TDM case, see below for examples
sound/soc/sunxi/sun4i-i2s.c | 44 ++++++++++++++++++++++++++++++++----- 1 file changed, 39 insertions(+), 5 deletions(-)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index bca73b3c0d74..75217fb52bfa 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -23,7 +23,7 @@
#define SUN4I_I2S_CTRL_REG 0x00 #define SUN4I_I2S_CTRL_SDO_EN_MASK GENMASK(11, 8) -#define SUN4I_I2S_CTRL_SDO_EN(sdo) BIT(8 + (sdo)) +#define SUN4I_I2S_CTRL_SDO_EN(lines) (((1 << lines) - 1) << 8) #define SUN4I_I2S_CTRL_MODE_MASK BIT(5) #define SUN4I_I2S_CTRL_MODE_SLAVE (1 << 5) #define SUN4I_I2S_CTRL_MODE_MASTER (0 << 5) @@ -355,14 +355,23 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai); int sr, wss, channels; u32 width;
int lines; channels = params_channels(params);
if (channels != 2) {
if ((channels > dai->driver->playback.channels_max) ||(channels < dai->driver->playback.channels_min)) { dev_err(dai->dev, "Unsupported number of channels: %d\n", channels); return -EINVAL; }lines = (channels + 1) / 2;/* Enable the required output lines */regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,SUN4I_I2S_CTRL_SDO_EN_MASK,SUN4I_I2S_CTRL_SDO_EN(lines));This has the assumption that each line will have 2 channels, which is wrong.
if (i2s->variant->is_h3_i2s_based) { regmap_update_bits(i2s->regmap, SUN8I_I2S_CHAN_CFG_REG, SUN8I_I2S_CHAN_CFG_TX_SLOT_NUM_MASK,@@ -373,8 +382,19 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, }
/* Map the channels for playback and capture */
regmap_field_write(i2s->field_txchanmap, 0x76543210); regmap_field_write(i2s->field_rxchanmap, 0x00003210);
regmap_field_write(i2s->field_txchanmap, 0x10);if (i2s->variant->is_h3_i2s_based) {if (channels > 2)regmap_write(i2s->regmap,SUN8I_I2S_TX_CHAN_MAP_REG+4, 0x32);if (channels > 4)regmap_write(i2s->regmap,SUN8I_I2S_TX_CHAN_MAP_REG+8, 0x54);if (channels > 6)regmap_write(i2s->regmap,SUN8I_I2S_TX_CHAN_MAP_REG+12, 0x76);}And this creates a mapping matching that.
/* Configure the channels */ regmap_field_write(i2s->field_txchansel,@@ -1057,9 +1077,10 @@ static int sun4i_i2s_init_regmap_fields(struct device *dev, static int sun4i_i2s_probe(struct platform_device *pdev) { struct sun4i_i2s *i2s;
struct snd_soc_dai_driver *soc_dai; struct resource *res; void __iomem *regs;
int irq, ret;
int irq, ret, val; i2s = devm_kzalloc(&pdev->dev, sizeof(*i2s), GFP_KERNEL); if (!i2s)@@ -1126,6 +1147,19 @@ static int sun4i_i2s_probe(struct platform_device *pdev) i2s->capture_dma_data.addr = res->start + SUN4I_I2S_FIFO_RX_REG; i2s->capture_dma_data.maxburst = 8;
soc_dai = devm_kmemdup(&pdev->dev, &sun4i_i2s_dai,sizeof(*soc_dai), GFP_KERNEL);if (!soc_dai) {ret = -ENOMEM;goto err_pm_disable;}if (!of_property_read_u32(pdev->dev.of_node,"allwinner,playback-channels", &val)) {if (val >= 2 && val <= 8)soc_dai->playback.channels_max = val;}I'm not quite sure how this works.
of_property_read_u32 will return 0, so you will enter in the condition. But what happens if the property is missing?
Maxime
-- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Tue, 4 Jun 2019 at 09:43, Code Kipper codekipper@gmail.com wrote:
On Tue, 4 Jun 2019 at 09:58, Maxime Ripard maxime.ripard@bootlin.com wrote:
On Mon, Jun 03, 2019 at 07:47:32PM +0200, codekipper@gmail.com wrote:
From: Marcus Cooper codekipper@gmail.com
The i2s block supports multi-lane i2s output however this functionality is only possible in earlier SoCs where the pins are exposed and for the i2s block used for HDMI audio on the later SoCs.
To enable this functionality, an optional property has been added to the bindings.
Signed-off-by: Marcus Cooper codekipper@gmail.com
I'd like to have Mark's input on this, but I'm really worried about the interaction with the proper TDM support.
Our fundamental issue is that the controller can have up to 8 channels, but either on 4 lines (instead of 1), or 8 channels on 1 (like proper TDM) (or any combination between the two, but that should be pretty rare).
I understand...maybe the TDM needs to be extended to support this to consider channel mapping and multiple transfer lines. I was thinking about the later when someone was requesting support on IIRC a while ago, I thought masking might of been a solution. These can wait as the only consumer at the moment is LibreELEC and we can patch it there.
Hi Marcus,
FWIW, the TI McASP driver has support for TDM & (i think?) multiple transfer lines which are called serializers. Maybe this can help with inspiration? see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/soun... sample DTS:
&mcasp0 { #sound-dai-cells = <0>; status = "okay"; pinctrl-names = "default"; pinctrl-0 = <&mcasp0_pins>;
op-mode = <0>; tdm-slots = <8>; serial-dir = < 2 0 1 0 0 0 0 0 0 0 0 0 0 0 0 0 >; tx-num-evt = <1>; rx-num-evt = <1>; };
Cheers!
Do you have any ideas Master? CK
You're trying to do the first one, and I'm trying to do the second one.
There's a number of assumptions later on that will break the TDM case, see below for examples
sound/soc/sunxi/sun4i-i2s.c | 44 ++++++++++++++++++++++++++++++++----- 1 file changed, 39 insertions(+), 5 deletions(-)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index bca73b3c0d74..75217fb52bfa 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -23,7 +23,7 @@
#define SUN4I_I2S_CTRL_REG 0x00 #define SUN4I_I2S_CTRL_SDO_EN_MASK GENMASK(11, 8) -#define SUN4I_I2S_CTRL_SDO_EN(sdo) BIT(8 + (sdo)) +#define SUN4I_I2S_CTRL_SDO_EN(lines) (((1 << lines) - 1) << 8) #define SUN4I_I2S_CTRL_MODE_MASK BIT(5) #define SUN4I_I2S_CTRL_MODE_SLAVE (1 << 5) #define SUN4I_I2S_CTRL_MODE_MASTER (0 << 5) @@ -355,14 +355,23 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai); int sr, wss, channels; u32 width;
int lines; channels = params_channels(params);
if (channels != 2) {
if ((channels > dai->driver->playback.channels_max) ||(channels < dai->driver->playback.channels_min)) { dev_err(dai->dev, "Unsupported number of channels: %d\n", channels); return -EINVAL; }lines = (channels + 1) / 2;/* Enable the required output lines */regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,SUN4I_I2S_CTRL_SDO_EN_MASK,SUN4I_I2S_CTRL_SDO_EN(lines));This has the assumption that each line will have 2 channels, which is wrong.
if (i2s->variant->is_h3_i2s_based) { regmap_update_bits(i2s->regmap, SUN8I_I2S_CHAN_CFG_REG, SUN8I_I2S_CHAN_CFG_TX_SLOT_NUM_MASK,@@ -373,8 +382,19 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, }
/* Map the channels for playback and capture */
regmap_field_write(i2s->field_txchanmap, 0x76543210); regmap_field_write(i2s->field_rxchanmap, 0x00003210);
regmap_field_write(i2s->field_txchanmap, 0x10);if (i2s->variant->is_h3_i2s_based) {if (channels > 2)regmap_write(i2s->regmap,SUN8I_I2S_TX_CHAN_MAP_REG+4, 0x32);if (channels > 4)regmap_write(i2s->regmap,SUN8I_I2S_TX_CHAN_MAP_REG+8, 0x54);if (channels > 6)regmap_write(i2s->regmap,SUN8I_I2S_TX_CHAN_MAP_REG+12, 0x76);}And this creates a mapping matching that.
/* Configure the channels */ regmap_field_write(i2s->field_txchansel,@@ -1057,9 +1077,10 @@ static int sun4i_i2s_init_regmap_fields(struct device *dev, static int sun4i_i2s_probe(struct platform_device *pdev) { struct sun4i_i2s *i2s;
struct snd_soc_dai_driver *soc_dai; struct resource *res; void __iomem *regs;
int irq, ret;
int irq, ret, val; i2s = devm_kzalloc(&pdev->dev, sizeof(*i2s), GFP_KERNEL); if (!i2s)@@ -1126,6 +1147,19 @@ static int sun4i_i2s_probe(struct platform_device *pdev) i2s->capture_dma_data.addr = res->start + SUN4I_I2S_FIFO_RX_REG; i2s->capture_dma_data.maxburst = 8;
soc_dai = devm_kmemdup(&pdev->dev, &sun4i_i2s_dai,sizeof(*soc_dai), GFP_KERNEL);if (!soc_dai) {ret = -ENOMEM;goto err_pm_disable;}if (!of_property_read_u32(pdev->dev.of_node,"allwinner,playback-channels", &val)) {if (val >= 2 && val <= 8)soc_dai->playback.channels_max = val;}I'm not quite sure how this works.
of_property_read_u32 will return 0, so you will enter in the condition. But what happens if the property is missing?
Maxime
-- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
-- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com. To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/CAEKpxB%3DRdYF9eEvAJ%2BR7sT6Ot.... For more options, visit https://groups.google.com/d/optout.
On Tue, 4 Jun 2019 at 11:02, Christopher Obbard chris@64studio.com wrote:
On Tue, 4 Jun 2019 at 09:43, Code Kipper codekipper@gmail.com wrote:
On Tue, 4 Jun 2019 at 09:58, Maxime Ripard maxime.ripard@bootlin.com wrote:
On Mon, Jun 03, 2019 at 07:47:32PM +0200, codekipper@gmail.com wrote:
From: Marcus Cooper codekipper@gmail.com
The i2s block supports multi-lane i2s output however this functionality is only possible in earlier SoCs where the pins are exposed and for the i2s block used for HDMI audio on the later SoCs.
To enable this functionality, an optional property has been added to the bindings.
Signed-off-by: Marcus Cooper codekipper@gmail.com
I'd like to have Mark's input on this, but I'm really worried about the interaction with the proper TDM support.
Our fundamental issue is that the controller can have up to 8 channels, but either on 4 lines (instead of 1), or 8 channels on 1 (like proper TDM) (or any combination between the two, but that should be pretty rare).
I understand...maybe the TDM needs to be extended to support this to consider channel mapping and multiple transfer lines. I was thinking about the later when someone was requesting support on IIRC a while ago, I thought masking might of been a solution. These can wait as the only consumer at the moment is LibreELEC and we can patch it there.
Hi Marcus,
FWIW, the TI McASP driver has support for TDM & (i think?) multiple transfer lines which are called serializers. Maybe this can help with inspiration? see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/soun... sample DTS:
&mcasp0 { #sound-dai-cells = <0>; status = "okay"; pinctrl-names = "default"; pinctrl-0 = <&mcasp0_pins>;
op-mode = <0>; tdm-slots = <8>; serial-dir = < 2 0 1 0 0 0 0 0 0 0 0 0 0 0 0 0 >; tx-num-evt = <1>; rx-num-evt = <1>;};
Cheers!
Thanks, this looks good. CK
Do you have any ideas Master? CK
You're trying to do the first one, and I'm trying to do the second one.
There's a number of assumptions later on that will break the TDM case, see below for examples
sound/soc/sunxi/sun4i-i2s.c | 44 ++++++++++++++++++++++++++++++++----- 1 file changed, 39 insertions(+), 5 deletions(-)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index bca73b3c0d74..75217fb52bfa 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -23,7 +23,7 @@
#define SUN4I_I2S_CTRL_REG 0x00 #define SUN4I_I2S_CTRL_SDO_EN_MASK GENMASK(11, 8) -#define SUN4I_I2S_CTRL_SDO_EN(sdo) BIT(8 + (sdo)) +#define SUN4I_I2S_CTRL_SDO_EN(lines) (((1 << lines) - 1) << 8) #define SUN4I_I2S_CTRL_MODE_MASK BIT(5) #define SUN4I_I2S_CTRL_MODE_SLAVE (1 << 5) #define SUN4I_I2S_CTRL_MODE_MASTER (0 << 5) @@ -355,14 +355,23 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai); int sr, wss, channels; u32 width;
int lines; channels = params_channels(params);
if (channels != 2) {
if ((channels > dai->driver->playback.channels_max) ||(channels < dai->driver->playback.channels_min)) { dev_err(dai->dev, "Unsupported number of channels: %d\n", channels); return -EINVAL; }lines = (channels + 1) / 2;/* Enable the required output lines */regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,SUN4I_I2S_CTRL_SDO_EN_MASK,SUN4I_I2S_CTRL_SDO_EN(lines));This has the assumption that each line will have 2 channels, which is wrong.
if (i2s->variant->is_h3_i2s_based) { regmap_update_bits(i2s->regmap, SUN8I_I2S_CHAN_CFG_REG, SUN8I_I2S_CHAN_CFG_TX_SLOT_NUM_MASK,@@ -373,8 +382,19 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, }
/* Map the channels for playback and capture */
regmap_field_write(i2s->field_txchanmap, 0x76543210); regmap_field_write(i2s->field_rxchanmap, 0x00003210);
regmap_field_write(i2s->field_txchanmap, 0x10);if (i2s->variant->is_h3_i2s_based) {if (channels > 2)regmap_write(i2s->regmap,SUN8I_I2S_TX_CHAN_MAP_REG+4, 0x32);if (channels > 4)regmap_write(i2s->regmap,SUN8I_I2S_TX_CHAN_MAP_REG+8, 0x54);if (channels > 6)regmap_write(i2s->regmap,SUN8I_I2S_TX_CHAN_MAP_REG+12, 0x76);}And this creates a mapping matching that.
/* Configure the channels */ regmap_field_write(i2s->field_txchansel,@@ -1057,9 +1077,10 @@ static int sun4i_i2s_init_regmap_fields(struct device *dev, static int sun4i_i2s_probe(struct platform_device *pdev) { struct sun4i_i2s *i2s;
struct snd_soc_dai_driver *soc_dai; struct resource *res; void __iomem *regs;
int irq, ret;
int irq, ret, val; i2s = devm_kzalloc(&pdev->dev, sizeof(*i2s), GFP_KERNEL); if (!i2s)@@ -1126,6 +1147,19 @@ static int sun4i_i2s_probe(struct platform_device *pdev) i2s->capture_dma_data.addr = res->start + SUN4I_I2S_FIFO_RX_REG; i2s->capture_dma_data.maxburst = 8;
soc_dai = devm_kmemdup(&pdev->dev, &sun4i_i2s_dai,sizeof(*soc_dai), GFP_KERNEL);if (!soc_dai) {ret = -ENOMEM;goto err_pm_disable;}if (!of_property_read_u32(pdev->dev.of_node,"allwinner,playback-channels", &val)) {if (val >= 2 && val <= 8)soc_dai->playback.channels_max = val;}I'm not quite sure how this works.
of_property_read_u32 will return 0, so you will enter in the condition. But what happens if the property is missing?
Maxime
-- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
-- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com. To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/CAEKpxB%3DRdYF9eEvAJ%2BR7sT6Ot.... For more options, visit https://groups.google.com/d/optout.
Hi!
Dne torek, 04. junij 2019 ob 11:38:44 CEST je Code Kipper napisal(a):
On Tue, 4 Jun 2019 at 11:02, Christopher Obbard chris@64studio.com wrote:
On Tue, 4 Jun 2019 at 09:43, Code Kipper codekipper@gmail.com wrote:
On Tue, 4 Jun 2019 at 09:58, Maxime Ripard maxime.ripard@bootlin.com
wrote:
On Mon, Jun 03, 2019 at 07:47:32PM +0200, codekipper@gmail.com wrote:
From: Marcus Cooper codekipper@gmail.com
The i2s block supports multi-lane i2s output however this functionality is only possible in earlier SoCs where the pins are exposed and for the i2s block used for HDMI audio on the later SoCs.
To enable this functionality, an optional property has been added to the bindings.
Signed-off-by: Marcus Cooper codekipper@gmail.com
I'd like to have Mark's input on this, but I'm really worried about the interaction with the proper TDM support.
Our fundamental issue is that the controller can have up to 8 channels, but either on 4 lines (instead of 1), or 8 channels on 1 (like proper TDM) (or any combination between the two, but that should be pretty rare).
I understand...maybe the TDM needs to be extended to support this to consider channel mapping and multiple transfer lines. I was thinking about the later when someone was requesting support on IIRC a while ago, I thought masking might of been a solution. These can wait as the only consumer at the moment is LibreELEC and we can patch it there.
Hi Marcus,
FWIW, the TI McASP driver has support for TDM & (i think?) multiple transfer lines which are called serializers. Maybe this can help with inspiration? see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/s ound/soc/ti/davinci-mcasp.c sample DTS:
&mcasp0 {
#sound-dai-cells = <0>; status = "okay"; pinctrl-names = "default"; pinctrl-0 = <&mcasp0_pins>; op-mode = <0>; tdm-slots = <8>; serial-dir = < 2 0 1 0 0 0 0 0 0 0 0 0 0 0 0 0 > >; tx-num-evt = <1>; rx-num-evt = <1>;};
Cheers!
Thanks, this looks good.
I would really like to see this issue resolved, because HDMI audio support in mainline Linux for those SoCs is long overdue.
However, there is a possibility to still add HDMI audio suport (stereo only) even if this issue is not completely solved. If we agree that configuration of channels would be solved with additional property as Christopher suggested, support for >2 channels can be left for a later time when support for that property would be implemented. Currently, stereo HDMI audio support can be added with a few patches.
I think all I2S cores are really the same, no matter if internally connected to HDMI controller or routed to pins, so it would make sense to use same compatible for all of them. It's just that those I2S cores which are routed to pins can use only one lane and >2 channels can be used only in TDM mode of operation, if I understand this correctly.
New property would have to be optional, so it's omission would result in same channel configuration as it is already present, to preserve compatibility with old device trees. And this mode is already sufficient for stereo HDMI audio support.
Side note: HDMI audio with current sun4i-i2s driver has a delay (about a second), supposedly because DW HDMI controller automatically generates CTS value based on I2S clock (auto CTS value generation is enabled per DesignWare recomendation for DW HDMI I2S interface). I2S driver from BSP Linux solves that by having I2S clock output enabled all the time. Should this be flagged with some additional property in DT?
Best regards, Jernej
CK
Do you have any ideas Master? CK
You're trying to do the first one, and I'm trying to do the second one.
There's a number of assumptions later on that will break the TDM case, see below for examples
sound/soc/sunxi/sun4i-i2s.c | 44 ++++++++++++++++++++++++++++++++----- 1 file changed, 39 insertions(+), 5 deletions(-)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index bca73b3c0d74..75217fb52bfa 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -23,7 +23,7 @@
#define SUN4I_I2S_CTRL_REG 0x00 #define SUN4I_I2S_CTRL_SDO_EN_MASK GENMASK(11, 8)
-#define SUN4I_I2S_CTRL_SDO_EN(sdo) BIT(8 + (sdo)) +#define SUN4I_I2S_CTRL_SDO_EN(lines) (((1 << lines) - 1) << 8)
#define SUN4I_I2S_CTRL_MODE_MASK BIT(5) #define SUN4I_I2S_CTRL_MODE_SLAVE (1 << 5) #define SUN4I_I2S_CTRL_MODE_MASTER (0 << 5)
@@ -355,14 +355,23 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,> > > > struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai); int sr, wss, channels; u32 width;
int lines; channels = params_channels(params);
if (channels != 2) {
if ((channels > dai->driver->playback.channels_max) ||(channels < dai->driver->playback.channels_min)) { dev_err(dai->dev, "Unsupported number of channels: %d\n", channels); return -EINVAL; }lines = (channels + 1) / 2;/* Enable the required output lines */regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,SUN4I_I2S_CTRL_SDO_EN_MASK,SUN4I_I2S_CTRL_SDO_EN(lines));This has the assumption that each line will have 2 channels, which is wrong.> > >
if (i2s->variant->is_h3_i2s_based) { regmap_update_bits(i2s->regmap, SUN8I_I2S_CHAN_CFG_REG, SUN8I_I2S_CHAN_CFG_TX_SLOT_NUM_MASK ,@@ -373,8 +382,19 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,> > > > }
/* Map the channels for playback and capture */
regmap_field_write(i2s->field_txchanmap, 0x76543210); regmap_field_write(i2s->field_rxchanmap, 0x00003210);
regmap_field_write(i2s->field_txchanmap, 0x10);if (i2s->variant->is_h3_i2s_based) {if (channels > 2)regmap_write(i2s->regmap,SUN8I_I2S_TX_CHAN_MAP_REG+4,0x32);
if (channels > 4)regmap_write(i2s->regmap,SUN8I_I2S_TX_CHAN_MAP_REG+8,0x54);
if (channels > 6)regmap_write(i2s->regmap,SUN8I_I2S_TX_CHAN_MAP_REG+12,0x76);
}And this creates a mapping matching that.
/* Configure the channels */ regmap_field_write(i2s->field_txchansel,@@ -1057,9 +1077,10 @@ static int sun4i_i2s_init_regmap_fields(struct device *dev,> > > > static int sun4i_i2s_probe(struct platform_device *pdev) {
struct sun4i_i2s *i2s;
struct snd_soc_dai_driver *soc_dai; struct resource *res; void __iomem *regs;
int irq, ret;
int irq, ret, val; i2s = devm_kzalloc(&pdev->dev, sizeof(*i2s), GFP_KERNEL); if (!i2s)@@ -1126,6 +1147,19 @@ static int sun4i_i2s_probe(struct platform_device *pdev)> > > > i2s->capture_dma_data.addr = res->start + SUN4I_I2S_FIFO_RX_REG; i2s->capture_dma_data.maxburst = 8;
soc_dai = devm_kmemdup(&pdev->dev, &sun4i_i2s_dai,sizeof(*soc_dai), GFP_KERNEL);if (!soc_dai) {ret = -ENOMEM;goto err_pm_disable;}if (!of_property_read_u32(pdev->dev.of_node,"allwinner,playback-channels",&val)) {
if (val >= 2 && val <= 8)soc_dai->playback.channels_max = val;}I'm not quite sure how this works.
of_property_read_u32 will return 0, so you will enter in the condition. But what happens if the property is missing?
Maxime
-- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
-- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com. To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/CAEKpxB%3DRdYF9eEvAJ%2BR7 sT6OtdtBWjhMM1am%2BEhaN%3D9ZO9Gd2A%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
On Tue, Jul 30, 2019 at 07:57:10PM +0200, Jernej Škrabec wrote:
Dne torek, 04. junij 2019 ob 11:38:44 CEST je Code Kipper napisal(a):
On Tue, 4 Jun 2019 at 11:02, Christopher Obbard chris@64studio.com wrote:
On Tue, 4 Jun 2019 at 09:43, Code Kipper codekipper@gmail.com wrote:
On Tue, 4 Jun 2019 at 09:58, Maxime Ripard maxime.ripard@bootlin.com
wrote:
On Mon, Jun 03, 2019 at 07:47:32PM +0200, codekipper@gmail.com wrote:
From: Marcus Cooper codekipper@gmail.com
The i2s block supports multi-lane i2s output however this functionality is only possible in earlier SoCs where the pins are exposed and for the i2s block used for HDMI audio on the later SoCs.
To enable this functionality, an optional property has been added to the bindings.
Signed-off-by: Marcus Cooper codekipper@gmail.com
I'd like to have Mark's input on this, but I'm really worried about the interaction with the proper TDM support.
Our fundamental issue is that the controller can have up to 8 channels, but either on 4 lines (instead of 1), or 8 channels on 1 (like proper TDM) (or any combination between the two, but that should be pretty rare).
I understand...maybe the TDM needs to be extended to support this to consider channel mapping and multiple transfer lines. I was thinking about the later when someone was requesting support on IIRC a while ago, I thought masking might of been a solution. These can wait as the only consumer at the moment is LibreELEC and we can patch it there.
Hi Marcus,
FWIW, the TI McASP driver has support for TDM & (i think?) multiple transfer lines which are called serializers. Maybe this can help with inspiration? see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/s ound/soc/ti/davinci-mcasp.c sample DTS:
&mcasp0 {
#sound-dai-cells = <0>; status = "okay"; pinctrl-names = "default"; pinctrl-0 = <&mcasp0_pins>; op-mode = <0>; tdm-slots = <8>; serial-dir = < 2 0 1 0 0 0 0 0 0 0 0 0 0 0 0 0 > >; tx-num-evt = <1>; rx-num-evt = <1>;};
Cheers!
Thanks, this looks good.
I would really like to see this issue resolved, because HDMI audio support in mainline Linux for those SoCs is long overdue.
However, there is a possibility to still add HDMI audio suport (stereo only) even if this issue is not completely solved. If we agree that configuration of channels would be solved with additional property as Christopher suggested, support for >2 channels can be left for a later time when support for that property would be implemented. Currently, stereo HDMI audio support can be added with a few patches.
I think all I2S cores are really the same, no matter if internally connected to HDMI controller or routed to pins, so it would make sense to use same compatible for all of them. It's just that those I2S cores which are routed to pins can use only one lane and >2 channels can be used only in TDM mode of operation, if I understand this correctly.
New property would have to be optional, so it's omission would result in same channel configuration as it is already present, to preserve compatibility with old device trees. And this mode is already sufficient for stereo HDMI audio support.
Yeah, it looks like a good plan.
Side note: HDMI audio with current sun4i-i2s driver has a delay (about a second), supposedly because DW HDMI controller automatically generates CTS value based on I2S clock (auto CTS value generation is enabled per DesignWare recomendation for DW HDMI I2S interface).
Is that a constant delay during the audio playback, or only at startup?
I2S driver from BSP Linux solves that by having I2S clock output enabled all the time. Should this be flagged with some additional property in DT?
I'd say that if the codec has that requirement, then it should be between the codec and the DAI, the DT doesn't really have anything to do with this.
Maxime
-- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Dne sreda, 31. julij 2019 ob 14:29:53 CEST je Maxime Ripard napisal(a):
On Tue, Jul 30, 2019 at 07:57:10PM +0200, Jernej Škrabec wrote:
Dne torek, 04. junij 2019 ob 11:38:44 CEST je Code Kipper napisal(a):
On Tue, 4 Jun 2019 at 11:02, Christopher Obbard chris@64studio.com
wrote:
On Tue, 4 Jun 2019 at 09:43, Code Kipper codekipper@gmail.com wrote:
On Tue, 4 Jun 2019 at 09:58, Maxime Ripard maxime.ripard@bootlin.com
wrote:
On Mon, Jun 03, 2019 at 07:47:32PM +0200, codekipper@gmail.com
wrote:
> From: Marcus Cooper codekipper@gmail.com > > The i2s block supports multi-lane i2s output however this > functionality > is only possible in earlier SoCs where the pins are exposed and > for > the i2s block used for HDMI audio on the later SoCs. > > To enable this functionality, an optional property has been > added to > the bindings. > > Signed-off-by: Marcus Cooper codekipper@gmail.com
I'd like to have Mark's input on this, but I'm really worried about the interaction with the proper TDM support.
Our fundamental issue is that the controller can have up to 8 channels, but either on 4 lines (instead of 1), or 8 channels on 1 (like proper TDM) (or any combination between the two, but that should be pretty rare).
I understand...maybe the TDM needs to be extended to support this to consider channel mapping and multiple transfer lines. I was thinking about the later when someone was requesting support on IIRC a while ago, I thought masking might of been a solution. These can wait as the only consumer at the moment is LibreELEC and we can patch it there.
Hi Marcus,
FWIW, the TI McASP driver has support for TDM & (i think?) multiple transfer lines which are called serializers. Maybe this can help with inspiration? see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tre e/s ound/soc/ti/davinci-mcasp.c sample DTS:
&mcasp0 {
#sound-dai-cells = <0>; status = "okay"; pinctrl-names = "default"; pinctrl-0 = <&mcasp0_pins>; op-mode = <0>; tdm-slots = <8>; serial-dir = < 2 0 1 0 0 0 0 0 0 0 0 0 0 0 0 0 > >; tx-num-evt = <1>; rx-num-evt = <1>;};
Cheers!
Thanks, this looks good.
I would really like to see this issue resolved, because HDMI audio support in mainline Linux for those SoCs is long overdue.
However, there is a possibility to still add HDMI audio suport (stereo only) even if this issue is not completely solved. If we agree that configuration of channels would be solved with additional property as Christopher suggested, support for >2 channels can be left for a later time when support for that property would be implemented. Currently, stereo HDMI audio support can be added with a few patches.
I think all I2S cores are really the same, no matter if internally connected to HDMI controller or routed to pins, so it would make sense to use same compatible for all of them. It's just that those I2S cores which are routed to pins can use only one lane and >2 channels can be used only in TDM mode of operation, if I understand this correctly.
New property would have to be optional, so it's omission would result in same channel configuration as it is already present, to preserve compatibility with old device trees. And this mode is already sufficient for stereo HDMI audio support.
Yeah, it looks like a good plan.
Side note: HDMI audio with current sun4i-i2s driver has a delay (about a second), supposedly because DW HDMI controller automatically generates CTS value based on I2S clock (auto CTS value generation is enabled per DesignWare recomendation for DW HDMI I2S interface).
Is that a constant delay during the audio playback, or only at startup?
I think it's just at startup, e.g. if you're watching movie, audio is in sync, it's just that first second or so is silent.
I2S driver from BSP Linux solves that by having I2S clock output enabled all the time. Should this be flagged with some additional property in DT?
I'd say that if the codec has that requirement, then it should be between the codec and the DAI, the DT doesn't really have anything to do with this.
Ok, but how to communicate that fact to I2S driver then? BSP driver solves that by using different compatible, but as I said before, I2S cores are not really different, so this seems wrong.
Best regards, Jernej
On Thu, Aug 1, 2019 at 1:32 PM Jernej Škrabec jernej.skrabec@gmail.com wrote:
Dne sreda, 31. julij 2019 ob 14:29:53 CEST je Maxime Ripard napisal(a):
On Tue, Jul 30, 2019 at 07:57:10PM +0200, Jernej Škrabec wrote:
Dne torek, 04. junij 2019 ob 11:38:44 CEST je Code Kipper napisal(a):
On Tue, 4 Jun 2019 at 11:02, Christopher Obbard chris@64studio.com
wrote:
On Tue, 4 Jun 2019 at 09:43, Code Kipper codekipper@gmail.com wrote:
On Tue, 4 Jun 2019 at 09:58, Maxime Ripard maxime.ripard@bootlin.com
wrote:
> On Mon, Jun 03, 2019 at 07:47:32PM +0200, codekipper@gmail.com
wrote:
> > From: Marcus Cooper codekipper@gmail.com > > > > The i2s block supports multi-lane i2s output however this > > functionality > > is only possible in earlier SoCs where the pins are exposed and > > for > > the i2s block used for HDMI audio on the later SoCs. > > > > To enable this functionality, an optional property has been > > added to > > the bindings. > > > > Signed-off-by: Marcus Cooper codekipper@gmail.com > > I'd like to have Mark's input on this, but I'm really worried > about > the interaction with the proper TDM support. > > Our fundamental issue is that the controller can have up to 8 > channels, but either on 4 lines (instead of 1), or 8 channels on 1 > (like proper TDM) (or any combination between the two, but that > should > be pretty rare).
I understand...maybe the TDM needs to be extended to support this to consider channel mapping and multiple transfer lines. I was thinking about the later when someone was requesting support on IIRC a while ago, I thought masking might of been a solution. These can wait as the only consumer at the moment is LibreELEC and we can patch it there.
Hi Marcus,
FWIW, the TI McASP driver has support for TDM & (i think?) multiple transfer lines which are called serializers. Maybe this can help with inspiration? see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tre e/s ound/soc/ti/davinci-mcasp.c sample DTS:
&mcasp0 {
#sound-dai-cells = <0>; status = "okay"; pinctrl-names = "default"; pinctrl-0 = <&mcasp0_pins>; op-mode = <0>; tdm-slots = <8>; serial-dir = < 2 0 1 0 0 0 0 0 0 0 0 0 0 0 0 0 > >; tx-num-evt = <1>; rx-num-evt = <1>;};
Cheers!
Thanks, this looks good.
I would really like to see this issue resolved, because HDMI audio support in mainline Linux for those SoCs is long overdue.
However, there is a possibility to still add HDMI audio suport (stereo only) even if this issue is not completely solved. If we agree that configuration of channels would be solved with additional property as Christopher suggested, support for >2 channels can be left for a later time when support for that property would be implemented. Currently, stereo HDMI audio support can be added with a few patches.
I think all I2S cores are really the same, no matter if internally connected to HDMI controller or routed to pins, so it would make sense to use same compatible for all of them. It's just that those I2S cores which are routed to pins can use only one lane and >2 channels can be used only in TDM mode of operation, if I understand this correctly.
New property would have to be optional, so it's omission would result in same channel configuration as it is already present, to preserve compatibility with old device trees. And this mode is already sufficient for stereo HDMI audio support.
Yeah, it looks like a good plan.
Side note: HDMI audio with current sun4i-i2s driver has a delay (about a second), supposedly because DW HDMI controller automatically generates CTS value based on I2S clock (auto CTS value generation is enabled per DesignWare recomendation for DW HDMI I2S interface).
Is that a constant delay during the audio playback, or only at startup?
I think it's just at startup, e.g. if you're watching movie, audio is in sync, it's just that first second or so is silent.
I2S driver from BSP Linux solves that by having I2S clock output enabled all the time. Should this be flagged with some additional property in DT?
I'd say that if the codec has that requirement, then it should be between the codec and the DAI, the DT doesn't really have anything to do with this.
Ok, but how to communicate that fact to I2S driver then? BSP driver solves that by using different compatible, but as I said before, I2S cores are not really different, so this seems wrong.
Maybe we could make the DW-HDMI I2S driver require the I2S clock be on all the time? You wouldn't need any changes to the DT.
ChenYu
On Tue, Aug 06, 2019 at 02:22:13PM +0800, Chen-Yu Tsai wrote:
On Thu, Aug 1, 2019 at 1:32 PM Jernej Škrabec jernej.skrabec@gmail.com wrote:
Dne sreda, 31. julij 2019 ob 14:29:53 CEST je Maxime Ripard napisal(a):
On Tue, Jul 30, 2019 at 07:57:10PM +0200, Jernej Škrabec wrote:
Dne torek, 04. junij 2019 ob 11:38:44 CEST je Code Kipper napisal(a):
On Tue, 4 Jun 2019 at 11:02, Christopher Obbard chris@64studio.com
wrote:
On Tue, 4 Jun 2019 at 09:43, Code Kipper codekipper@gmail.com wrote: > On Tue, 4 Jun 2019 at 09:58, Maxime Ripard > maxime.ripard@bootlin.com
wrote:
> > On Mon, Jun 03, 2019 at 07:47:32PM +0200, codekipper@gmail.com
wrote:
> > > From: Marcus Cooper codekipper@gmail.com > > > > > > The i2s block supports multi-lane i2s output however this > > > functionality > > > is only possible in earlier SoCs where the pins are exposed and > > > for > > > the i2s block used for HDMI audio on the later SoCs. > > > > > > To enable this functionality, an optional property has been > > > added to > > > the bindings. > > > > > > Signed-off-by: Marcus Cooper codekipper@gmail.com > > > > I'd like to have Mark's input on this, but I'm really worried > > about > > the interaction with the proper TDM support. > > > > Our fundamental issue is that the controller can have up to 8 > > channels, but either on 4 lines (instead of 1), or 8 channels on 1 > > (like proper TDM) (or any combination between the two, but that > > should > > be pretty rare). > > I understand...maybe the TDM needs to be extended to support this to > consider channel mapping and multiple transfer lines. I was thinking > about the later when someone was requesting support on IIRC a while > ago, I thought masking might of been a solution. These can wait as > the > only consumer at the moment is LibreELEC and we can patch it there.
Hi Marcus,
FWIW, the TI McASP driver has support for TDM & (i think?) multiple transfer lines which are called serializers. Maybe this can help with inspiration? see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tre e/s ound/soc/ti/davinci-mcasp.c sample DTS:
&mcasp0 {
#sound-dai-cells = <0>; status = "okay"; pinctrl-names = "default"; pinctrl-0 = <&mcasp0_pins>; op-mode = <0>; tdm-slots = <8>; serial-dir = < 2 0 1 0 0 0 0 0 0 0 0 0 0 0 0 0 > >; tx-num-evt = <1>; rx-num-evt = <1>;};
Cheers!
Thanks, this looks good.
I would really like to see this issue resolved, because HDMI audio support in mainline Linux for those SoCs is long overdue.
However, there is a possibility to still add HDMI audio suport (stereo only) even if this issue is not completely solved. If we agree that configuration of channels would be solved with additional property as Christopher suggested, support for >2 channels can be left for a later time when support for that property would be implemented. Currently, stereo HDMI audio support can be added with a few patches.
I think all I2S cores are really the same, no matter if internally connected to HDMI controller or routed to pins, so it would make sense to use same compatible for all of them. It's just that those I2S cores which are routed to pins can use only one lane and >2 channels can be used only in TDM mode of operation, if I understand this correctly.
New property would have to be optional, so it's omission would result in same channel configuration as it is already present, to preserve compatibility with old device trees. And this mode is already sufficient for stereo HDMI audio support.
Yeah, it looks like a good plan.
Side note: HDMI audio with current sun4i-i2s driver has a delay (about a second), supposedly because DW HDMI controller automatically generates CTS value based on I2S clock (auto CTS value generation is enabled per DesignWare recomendation for DW HDMI I2S interface).
Is that a constant delay during the audio playback, or only at startup?
I think it's just at startup, e.g. if you're watching movie, audio is in sync, it's just that first second or so is silent.
I2S driver from BSP Linux solves that by having I2S clock output enabled all the time. Should this be flagged with some additional property in DT?
I'd say that if the codec has that requirement, then it should be between the codec and the DAI, the DT doesn't really have anything to do with this.
Ok, but how to communicate that fact to I2S driver then? BSP driver solves that by using different compatible, but as I said before, I2S cores are not really different, so this seems wrong.
Maybe we could make the DW-HDMI I2S driver require the I2S clock be on all the time? You wouldn't need any changes to the DT.
That's an option, but I'd really like to avoid it if possible.
I guess we could also just add a delay in the powerup path in the HDMI case? Would it work?
maxime
-- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
From: Marcus Cooper codekipper@gmail.com
The i2s block can be used to pass PCM data over multiple channels and is sometimes used for the audio side of an HDMI connection.
Signed-off-by: Marcus Cooper codekipper@gmail.com --- sound/soc/sunxi/sun4i-i2s.c | 121 +++++++++++++++++++----------------- 1 file changed, 64 insertions(+), 57 deletions(-)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index 75217fb52bfa..3549a87ed9e9 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -189,6 +189,7 @@ struct sun4i_i2s {
unsigned int tdm_slots; unsigned int slot_width; + unsigned int offset; };
struct sun4i_i2s_clk_div { @@ -358,56 +359,71 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, int lines;
channels = params_channels(params); - if ((channels > dai->driver->playback.channels_max) || - (channels < dai->driver->playback.channels_min)) { - dev_err(dai->dev, "Unsupported number of channels: %d\n", - channels); - return -EINVAL; - } - - lines = (channels + 1) / 2; - - /* Enable the required output lines */ - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG, - SUN4I_I2S_CTRL_SDO_EN_MASK, - SUN4I_I2S_CTRL_SDO_EN(lines)); + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { + if ((channels > dai->driver->playback.channels_max) || + (channels < dai->driver->playback.channels_min)) { + dev_err(dai->dev, "Unsupported number of channels: %d\n", + channels); + return -EINVAL; + }
- if (i2s->variant->is_h3_i2s_based) { - 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)); - } + lines = (channels + 1) / 2;
- /* Map the channels for playback and capture */ - regmap_field_write(i2s->field_rxchanmap, 0x00003210); - regmap_field_write(i2s->field_txchanmap, 0x10); - if (i2s->variant->is_h3_i2s_based) { - if (channels > 2) - regmap_write(i2s->regmap, - SUN8I_I2S_TX_CHAN_MAP_REG+4, 0x32); - if (channels > 4) - regmap_write(i2s->regmap, - SUN8I_I2S_TX_CHAN_MAP_REG+8, 0x54); - if (channels > 6) - regmap_write(i2s->regmap, - SUN8I_I2S_TX_CHAN_MAP_REG+12, 0x76); + /* Enable the required output lines */ + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG, + SUN4I_I2S_CTRL_SDO_EN_MASK, + SUN4I_I2S_CTRL_SDO_EN(lines)); + + if (i2s->variant->is_h3_i2s_based) + 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_field_write(i2s->field_txchanmap, 0x10); + /* Configure the channels */ + regmap_field_write(i2s->field_txchansel, SUN4I_I2S_CHAN_SEL(2)); + + if (i2s->variant->is_h3_i2s_based) { + u32 chan_sel = SUN8I_I2S_TX_CHAN_OFFSET(i2s->offset) | 0x1; + regmap_write(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG, + chan_sel | 0x30); + + if (channels > 2) { + regmap_write(i2s->regmap, + SUN8I_I2S_TX_CHAN_MAP_REG+4, 0x32); + regmap_write(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG+4, + chan_sel | 0x30); + } + if (channels > 4) { + regmap_write(i2s->regmap, + SUN8I_I2S_TX_CHAN_MAP_REG+8, 0x54); + regmap_write(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG+8, + chan_sel | 0x30); + } + if (channels > 6) { + regmap_write(i2s->regmap, + SUN8I_I2S_TX_CHAN_MAP_REG+12, 0x76); + regmap_write(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG+12, + chan_sel | 0x30); + } + } + } else { + /* Map the channels for capture */ + regmap_field_write(i2s->field_rxchanmap, 0x00003210); + regmap_field_write(i2s->field_rxchansel, + SUN4I_I2S_CHAN_SEL(params_channels(params))); + + if (i2s->variant->is_h3_i2s_based) { + 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)); + + regmap_update_bits(i2s->regmap, SUN8I_I2S_RX_CHAN_SEL_REG, + SUN8I_I2S_TX_CHAN_OFFSET_MASK, + SUN8I_I2S_TX_CHAN_OFFSET(i2s->offset)); + } }
- /* Configure the channels */ - regmap_field_write(i2s->field_txchansel, - SUN4I_I2S_CHAN_SEL(params_channels(params))); - - regmap_field_write(i2s->field_rxchansel, - SUN4I_I2S_CHAN_SEL(params_channels(params))); - - if (i2s->variant->is_h3_i2s_based) - regmap_update_bits(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG, - SUN8I_I2S_TX_CHAN_EN_MASK, - SUN8I_I2S_TX_CHAN_EN(channels)); - switch (params_physical_width(params)) { case 16: width = DMA_SLAVE_BUSWIDTH_2_BYTES; @@ -445,7 +461,6 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) { struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai); u32 val; - u32 offset = 0; u32 bclk_polarity = SUN4I_I2S_FMT0_POLARITY_NORMAL; u32 lrclk_polarity = SUN4I_I2S_FMT0_POLARITY_NORMAL;
@@ -453,7 +468,7 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) { case SND_SOC_DAIFMT_I2S: val = SUN4I_I2S_FMT0_FMT_I2S; - offset = 1; + i2s->offset = 1; break; case SND_SOC_DAIFMT_LEFT_J: val = SUN4I_I2S_FMT0_FMT_LEFT_J; @@ -474,16 +489,8 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) * i2s shares the same setting with the LJ format. Increment * val so that the bit to value to write is correct. */ - if (offset > 0) + if (i2s->offset > 0) val++; - /* blck offset determines whether i2s or LJ */ - regmap_update_bits(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG, - SUN8I_I2S_TX_CHAN_OFFSET_MASK, - SUN8I_I2S_TX_CHAN_OFFSET(offset)); - - regmap_update_bits(i2s->regmap, SUN8I_I2S_RX_CHAN_SEL_REG, - SUN8I_I2S_TX_CHAN_OFFSET_MASK, - SUN8I_I2S_TX_CHAN_OFFSET(offset)); }
regmap_field_write(i2s->field_fmt_mode, val);
From: Marcus Cooper codekipper@gmail.com
Extend the functionality of the driver to include support of 20 and 24 bits per sample for the earlier SoCs.
Newer SoCs can also handle 32bit samples.
Signed-off-by: Marcus Cooper codekipper@gmail.com --- sound/soc/sunxi/sun4i-i2s.c | 34 +++++++++++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 3 deletions(-)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index 3549a87ed9e9..351b8021173b 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -428,6 +428,11 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, case 16: width = DMA_SLAVE_BUSWIDTH_2_BYTES; break; + case 20: + case 24: + case 32: + width = DMA_SLAVE_BUSWIDTH_4_BYTES; + break; default: dev_err(dai->dev, "Unsupported physical sample width: %d\n", params_physical_width(params)); @@ -440,7 +445,18 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, sr = 0; wss = 0; break; - + case 20: + sr = 1; + wss = 1; + break; + case 24: + sr = 2; + wss = 2; + break; + case 32: + sr = 4; + wss = 4; + break; default: dev_err(dai->dev, "Unsupported sample width: %d\n", params_width(params)); @@ -722,6 +738,13 @@ 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) + +#define SUN8I_FORMATS (SUN4I_FORMATS | \ + SNDRV_PCM_FMTBIT_S32_LE) + static struct snd_soc_dai_driver sun4i_i2s_dai = { .probe = sun4i_i2s_dai_probe, .capture = { @@ -729,14 +752,14 @@ static struct snd_soc_dai_driver sun4i_i2s_dai = { .channels_min = 2, .channels_max = 2, .rates = SNDRV_PCM_RATE_8000_192000, - .formats = SNDRV_PCM_FMTBIT_S16_LE, + .formats = SUN4I_FORMATS, }, .playback = { .stream_name = "Playback", .channels_min = 2, .channels_max = 2, .rates = SNDRV_PCM_RATE_8000_192000, - .formats = SNDRV_PCM_FMTBIT_S16_LE, + .formats = SUN4I_FORMATS, }, .ops = &sun4i_i2s_dai_ops, .symmetric_rates = 1, @@ -1161,6 +1184,11 @@ static int sun4i_i2s_probe(struct platform_device *pdev) goto err_pm_disable; }
+ if (i2s->variant->is_h3_i2s_based) { + soc_dai->playback.formats = SUN8I_FORMATS; + soc_dai->capture.formats = SUN8I_FORMATS; + } + if (!of_property_read_u32(pdev->dev.of_node, "allwinner,playback-channels", &val)) { if (val >= 2 && val <= 8)
On Mon, Jun 03, 2019 at 07:47:34PM +0200, codekipper@gmail.com wrote:
From: Marcus Cooper codekipper@gmail.com
Extend the functionality of the driver to include support of 20 and 24 bits per sample for the earlier SoCs.
Newer SoCs can also handle 32bit samples.
Signed-off-by: Marcus Cooper codekipper@gmail.com
sound/soc/sunxi/sun4i-i2s.c | 34 +++++++++++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 3 deletions(-)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index 3549a87ed9e9..351b8021173b 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -428,6 +428,11 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, case 16: width = DMA_SLAVE_BUSWIDTH_2_BYTES; break;
- case 20:
- case 24:
- case 32:
width = DMA_SLAVE_BUSWIDTH_4_BYTES;break;
Doesn't this test the physical width? If so, then I'm not sure that 20 even exists, and that we can support 24.
default: dev_err(dai->dev, "Unsupported physical sample width: %d\n", params_physical_width(params)); @@ -440,7 +445,18 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, sr = 0; wss = 0; break;
- case 20:
sr = 1;wss = 1;break;- case 24:
sr = 2;wss = 2;break;- case 32:
sr = 4;wss = 4;break;
This doesn't really works, wss being the slot size, and you can have a different slot size and sample size. I have a patch that reworks this, I'll send it.
Maxime
-- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
From: Marcus Cooper codekipper@gmail.com
Bypass the regmap cache when flushing the i2s FIFOs and modify the tables to reflect this.
Signed-off-by: Marcus Cooper codekipper@gmail.com --- sound/soc/sunxi/sun4i-i2s.c | 29 +++++++++-------------------- 1 file changed, 9 insertions(+), 20 deletions(-)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index 351b8021173b..92828a84902d 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -595,9 +595,11 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) static void sun4i_i2s_start_capture(struct sun4i_i2s *i2s) { /* Flush RX FIFO */ + regcache_cache_bypass(i2s->regmap, true); regmap_update_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG, SUN4I_I2S_FIFO_CTRL_FLUSH_RX, SUN4I_I2S_FIFO_CTRL_FLUSH_RX); + regcache_cache_bypass(i2s->regmap, false);
/* Clear RX counter */ regmap_write(i2s->regmap, SUN4I_I2S_RX_CNT_REG, 0); @@ -616,9 +618,11 @@ static void sun4i_i2s_start_capture(struct sun4i_i2s *i2s) static void sun4i_i2s_start_playback(struct sun4i_i2s *i2s) { /* Flush TX FIFO */ + regcache_cache_bypass(i2s->regmap, true); regmap_update_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG, SUN4I_I2S_FIFO_CTRL_FLUSH_TX, SUN4I_I2S_FIFO_CTRL_FLUSH_TX); + regcache_cache_bypass(i2s->regmap, false);
/* Clear TX counter */ regmap_write(i2s->regmap, SUN4I_I2S_TX_CNT_REG, 0); @@ -771,13 +775,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) @@ -796,6 +794,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: @@ -806,23 +806,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); } @@ -877,7 +866,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, };
On Mon, Jun 03, 2019 at 07:47:35PM +0200, codekipper@gmail.com wrote:
From: Marcus Cooper codekipper@gmail.com
Bypass the regmap cache when flushing the i2s FIFOs and modify the tables to reflect this.
Signed-off-by: Marcus Cooper codekipper@gmail.com
sound/soc/sunxi/sun4i-i2s.c | 29 +++++++++-------------------- 1 file changed, 9 insertions(+), 20 deletions(-)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index 351b8021173b..92828a84902d 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -595,9 +595,11 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) static void sun4i_i2s_start_capture(struct sun4i_i2s *i2s) { /* Flush RX FIFO */
- regcache_cache_bypass(i2s->regmap, true); regmap_update_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG, SUN4I_I2S_FIFO_CTRL_FLUSH_RX, SUN4I_I2S_FIFO_CTRL_FLUSH_RX);
- regcache_cache_bypass(i2s->regmap, false);
Your commit log should say why you need to do this in the first place.
@@ -771,13 +775,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;
That doesn't seem related?
Maxime
-- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
participants (7)
-
Chen-Yu Tsai -
Christopher Obbard -
Code Kipper -
codekipper＠gmail.com
-
Jernej Škrabec -
Mark Brown -
Maxime Ripard