From: Marcus Cooper <[email protected]>
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. Since the last push 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.
Anyway, I've moved the more controversial patches to the top of the stack
as I'm expecting feedback.
Have a great Xmas and New Year,
BR,
CK
---
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: Adjust regmap settings
ASoC: sun4i-i2s: Add regmap field to sign extend sample
ASoc: sun4i-i2s: Add 20, 24 and 32 bit support
ASoC: sun4i-i2s: Fix offset mask
ASoC: sun4i-i2s: Correct divider calculations
ASoC: sun4i-i2s: Add multi-lane functionality
ASoC: sun4i-i2s: Do not divide clocks when slave
ASoC: sun4i-i2s: Add set_tdm_slot functionality
ASoC: sun4i-i2s: Add multichannel functionality
sound/soc/sunxi/sun4i-i2s.c | 399 ++++++++++++++++++++++++------------
1 file changed, 267 insertions(+), 132 deletions(-)
--
2.20.1
From: Marcus Cooper <[email protected]>
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 <[email protected]>
---
sound/soc/sunxi/sun4i-i2s.c | 41 ++++++++++++++++++++++++++++++++++---
1 file changed, 38 insertions(+), 3 deletions(-)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
index 80bf29e0cc86..adb988ae9ac5 100644
--- a/sound/soc/sunxi/sun4i-i2s.c
+++ b/sound/soc/sunxi/sun4i-i2s.c
@@ -399,6 +399,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));
@@ -411,7 +416,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));
@@ -687,6 +703,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_3LE | \
+ 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 = {
@@ -694,14 +717,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,
@@ -1106,6 +1129,18 @@ 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 (i2s->variant->has_fmt_set_lrck_period) {
+ soc_dai->playback.formats = SUN8I_FORMATS;
+ soc_dai->capture.formats = SUN8I_FORMATS;
+ }
+
pm_runtime_enable(&pdev->dev);
if (!pm_runtime_enabled(&pdev->dev)) {
ret = sun4i_i2s_runtime_resume(&pdev->dev);
--
2.20.1
From: Marcus Cooper <[email protected]>
The clock division circuitry is different on the H3 and later SoCs.
The division of bclk is now based on pll2.
Signed-off-by: Marcus Cooper <[email protected]>
---
sound/soc/sunxi/sun4i-i2s.c | 82 +++++++++++++++++++++++++------------
1 file changed, 56 insertions(+), 26 deletions(-)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
index 93a484d7e228..b31f84787218 100644
--- a/sound/soc/sunxi/sun4i-i2s.c
+++ b/sound/soc/sunxi/sun4i-i2s.c
@@ -129,10 +129,10 @@
* @has_chsel_offset: SoC uses offset for selecting dai operational mode.
* @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.
- * @bclk_offset: Value by which bclkdiv needs to be adjusted.
* @fmt_offset: Value by which wss and sr needs to be adjusted.
* @field_clkdiv_mclk_en: regmap field to enable mclk output.
+ * @field_clkdiv_mclk: regmap field for mclkdiv.
+ * @field_clkdiv_bclk: regmap field for bclkdiv.
* @field_fmt_wss: regmap field to set word select size.
* @field_fmt_sr: regmap field to set sample resolution.
* @field_fmt_bclk: regmap field to set clk polarity.
@@ -153,8 +153,6 @@ struct sun4i_i2s_quirks {
bool has_chsel_offset;
unsigned int reg_offset_txdata; /* TX FIFO */
const struct regmap_config *sun4i_i2s_regmap;
- unsigned int mclk_offset;
- unsigned int bclk_offset;
unsigned int fmt_offset;
/* Register fields for i2s */
@@ -210,7 +208,25 @@ static const struct sun4i_i2s_clk_div sun4i_i2s_bclk_div[] = {
{ .div = 8, .val = 3 },
{ .div = 12, .val = 4 },
{ .div = 16, .val = 5 },
- /* TODO - extend divide ratio supported by newer SoCs */
+};
+
+static const struct sun4i_i2s_clk_div sun8i_i2s_clk_div[] = {
+ { .div = 0, .val = 0 },
+ { .div = 1, .val = 1 },
+ { .div = 2, .val = 2 },
+ { .div = 4, .val = 3 },
+ { .div = 6, .val = 4 },
+ { .div = 8, .val = 5 },
+ { .div = 12, .val = 6 },
+ { .div = 16, .val = 7 },
+ { .div = 24, .val = 8 },
+ { .div = 32, .val = 9 },
+ { .div = 48, .val = 10 },
+ { .div = 64, .val = 11 },
+ { .div = 96, .val = 12 },
+ { .div = 128, .val = 13 },
+ { .div = 176, .val = 14 },
+ { .div = 192, .val = 15 },
};
static const struct sun4i_i2s_clk_div sun4i_i2s_mclk_div[] = {
@@ -222,21 +238,21 @@ static const struct sun4i_i2s_clk_div sun4i_i2s_mclk_div[] = {
{ .div = 12, .val = 5 },
{ .div = 16, .val = 6 },
{ .div = 24, .val = 7 },
- /* TODO - extend divide ratio supported by newer SoCs */
};
static int sun4i_i2s_get_bclk_div(struct sun4i_i2s *i2s,
unsigned int oversample_rate,
- unsigned int word_size)
+ unsigned int word_size,
+ const struct sun4i_i2s_clk_div *bdiv,
+ unsigned int size)
{
int div = oversample_rate / word_size / 2;
int i;
- for (i = 0; i < ARRAY_SIZE(sun4i_i2s_bclk_div); i++) {
- const struct sun4i_i2s_clk_div *bdiv = &sun4i_i2s_bclk_div[i];
-
+ for (i = 0; i < size; i++) {
if (bdiv->div == div)
return bdiv->val;
+ bdiv++;
}
return -EINVAL;
@@ -245,16 +261,17 @@ static int sun4i_i2s_get_bclk_div(struct sun4i_i2s *i2s,
static int sun4i_i2s_get_mclk_div(struct sun4i_i2s *i2s,
unsigned int oversample_rate,
unsigned int module_rate,
- unsigned int sampling_rate)
+ unsigned int sampling_rate,
+ const struct sun4i_i2s_clk_div *mdiv,
+ unsigned int size)
{
int div = module_rate / sampling_rate / oversample_rate;
int i;
- for (i = 0; i < ARRAY_SIZE(sun4i_i2s_mclk_div); i++) {
- const struct sun4i_i2s_clk_div *mdiv = &sun4i_i2s_mclk_div[i];
-
+ for (i = 0; i < size; i++) {
if (mdiv->div == div)
return mdiv->val;
+ mdiv++;
}
return -EINVAL;
@@ -319,24 +336,39 @@ static int sun4i_i2s_set_clk_rate(struct snd_soc_dai *dai,
return -EINVAL;
}
- bclk_div = sun4i_i2s_get_bclk_div(i2s, oversample_rate,
- word_size);
+ if (i2s->variant->has_fmt_set_lrck_period)
+ bclk_div = sun4i_i2s_get_bclk_div(i2s, clk_rate / rate,
+ word_size,
+ sun8i_i2s_clk_div,
+ ARRAY_SIZE(sun8i_i2s_clk_div));
+ else
+ bclk_div = sun4i_i2s_get_bclk_div(i2s, oversample_rate,
+ word_size,
+ sun4i_i2s_bclk_div,
+ ARRAY_SIZE(sun4i_i2s_bclk_div));
if (bclk_div < 0) {
- dev_err(dai->dev, "Unsupported BCLK divider: %d\n", bclk_div);
+ dev_err(dai->dev, "Unsupported BCLK divider: %d\n",
+ bclk_div);
return -EINVAL;
}
- mclk_div = sun4i_i2s_get_mclk_div(i2s, oversample_rate,
- clk_rate, rate);
+
+ if (i2s->variant->has_fmt_set_lrck_period)
+ mclk_div = sun4i_i2s_get_mclk_div(i2s, oversample_rate,
+ clk_rate, rate,
+ sun8i_i2s_clk_div,
+ ARRAY_SIZE(sun8i_i2s_clk_div));
+ else
+ mclk_div = sun4i_i2s_get_mclk_div(i2s, oversample_rate,
+ clk_rate, rate,
+ sun4i_i2s_mclk_div,
+ ARRAY_SIZE(sun4i_i2s_mclk_div));
if (mclk_div < 0) {
- dev_err(dai->dev, "Unsupported MCLK divider: %d\n", mclk_div);
+ dev_err(dai->dev, "Unsupported MCLK divider: %d\n",
+ mclk_div);
return -EINVAL;
}
- /* Adjust the clock division values if needed */
- bclk_div += i2s->variant->bclk_offset;
- mclk_div += i2s->variant->mclk_offset;
-
regmap_write(i2s->regmap, SUN4I_I2S_CLK_DIV_REG,
SUN4I_I2S_CLK_DIV_BCLK(bclk_div) |
SUN4I_I2S_CLK_DIV_MCLK(mclk_div));
@@ -955,8 +987,6 @@ static const struct sun4i_i2s_quirks sun8i_h3_i2s_quirks = {
.has_reset = true,
.reg_offset_txdata = SUN8I_I2S_FIFO_TX_REG,
.sun4i_i2s_regmap = &sun8i_i2s_regmap_config,
- .mclk_offset = 1,
- .bclk_offset = 2,
.fmt_offset = 3,
.has_fmt_set_lrck_period = true,
.has_chcfg = true,
--
2.20.1
From: Marcus Cooper <[email protected]>
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 <[email protected]>
---
sound/soc/sunxi/sun4i-i2s.c | 37 ++++++++++++++++++++++++++++++++-----
1 file changed, 32 insertions(+), 5 deletions(-)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
index b31f84787218..e85789d84c0c 100644
--- a/sound/soc/sunxi/sun4i-i2s.c
+++ b/sound/soc/sunxi/sun4i-i2s.c
@@ -27,7 +27,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)
@@ -394,14 +394,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->has_chcfg) {
regmap_update_bits(i2s->regmap, SUN8I_I2S_CHAN_CFG_REG,
SUN8I_I2S_CHAN_CFG_TX_SLOT_NUM_MASK,
@@ -412,8 +421,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->has_chsel_tx_chen) {
+ 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,
@@ -1094,9 +1114,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)
@@ -1175,6 +1196,12 @@ static int sun4i_i2s_probe(struct platform_device *pdev)
soc_dai->capture.formats = SUN8I_FORMATS;
}
+ 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);
@@ -1184,7 +1211,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;
--
2.20.1
From: Marcus Cooper <[email protected]>
On the newer SoCs this is set by default to transfer a 0 after
each sample in each slot. Add the regmap field to configure this
and set it so that it pads the sample with 0s.
Signed-off-by: Marcus Cooper <[email protected]>
---
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 64d073cb2aee..80bf29e0cc86 100644
--- a/sound/soc/sunxi/sun4i-i2s.c
+++ b/sound/soc/sunxi/sun4i-i2s.c
@@ -138,6 +138,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.
@@ -163,6 +164,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;
@@ -187,6 +189,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;
@@ -346,6 +349,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;
}
@@ -876,6 +882,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),
@@ -893,6 +900,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),
@@ -933,6 +941,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),
@@ -995,6 +1004,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.20.1
From: Marcus Cooper <[email protected]>
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 <[email protected]>
---
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 cfcf427270fd..2be3a3e7a3c0 100644
--- a/sound/soc/sunxi/sun4i-i2s.c
+++ b/sound/soc/sunxi/sun4i-i2s.c
@@ -199,6 +199,7 @@ struct sun4i_i2s {
unsigned int tdm_slots;
unsigned int slot_width;
+ unsigned int offset;
};
struct sun4i_i2s_clk_div {
@@ -406,56 +407,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;
+ 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;
+ }
- /* 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->has_chcfg) {
- 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->has_chsel_tx_chen) {
- 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->has_chcfg)
+ 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->has_chsel_tx_chen) {
+ 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->has_chcfg)
+ 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));
+
+ if (i2s->variant->has_chsel_tx_chen)
+ 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->has_chsel_tx_chen)
- 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;
@@ -509,7 +525,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;
@@ -517,7 +532,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;
@@ -538,16 +553,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);
--
2.20.1
From: Marcus Cooper <[email protected]>
Bypass the regmap cache when flushing the i2s FIFOs and modify the tables
to reflect this.
Signed-off-by: Marcus Cooper <[email protected]>
---
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 d5ec1a20499d..64d073cb2aee 100644
--- a/sound/soc/sunxi/sun4i-i2s.c
+++ b/sound/soc/sunxi/sun4i-i2s.c
@@ -548,9 +548,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);
@@ -569,9 +571,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);
@@ -703,13 +707,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)
@@ -728,6 +726,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:
@@ -738,23 +738,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);
}
@@ -809,7 +798,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,
};
--
2.20.1
From: Marcus Cooper <[email protected]>
Also add offset to RX channel select
Signed-off-by: Marcus Cooper <[email protected]>
---
sound/soc/sunxi/sun4i-i2s.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
index adb988ae9ac5..93a484d7e228 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)
@@ -482,6 +482,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);
--
2.20.1
From: Marcus Cooper <[email protected]>
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 <[email protected]>
---
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 8cec2f42c94e..cfcf427270fd 100644
--- a/sound/soc/sunxi/sun4i-i2s.c
+++ b/sound/soc/sunxi/sun4i-i2s.c
@@ -196,6 +196,9 @@ struct sun4i_i2s {
const struct sun4i_i2s_quirks *variant;
bool bit_clk_master;
+
+ unsigned int tdm_slots;
+ unsigned int slot_width;
};
struct sun4i_i2s_clk_div {
@@ -385,7 +388,7 @@ static int sun4i_i2s_set_clk_rate(struct snd_soc_dai *dai,
if (i2s->variant->has_fmt_set_lrck_period)
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);
@@ -498,7 +501,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)
@@ -749,11 +753,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)
--
2.20.1
From: Marcus Cooper <[email protected]>
There is no need to set the clock and calculate the division of
the audio pll for the bclk and sync signals when they are not
required.
Signed-off-by: Marcus Cooper <[email protected]>
---
sound/soc/sunxi/sun4i-i2s.c | 144 +++++++++++++++++++-----------------
1 file changed, 77 insertions(+), 67 deletions(-)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
index e85789d84c0c..8cec2f42c94e 100644
--- a/sound/soc/sunxi/sun4i-i2s.c
+++ b/sound/soc/sunxi/sun4i-i2s.c
@@ -194,6 +194,8 @@ struct sun4i_i2s {
struct regmap_field *field_rxchansel;
const struct sun4i_i2s_quirks *variant;
+
+ bool bit_clk_master;
};
struct sun4i_i2s_clk_div {
@@ -298,82 +300,86 @@ static int sun4i_i2s_set_clk_rate(struct snd_soc_dai *dai,
int bclk_div, mclk_div;
int ret;
- switch (rate) {
- case 176400:
- case 88200:
- case 44100:
- case 22050:
- case 11025:
- clk_rate = 22579200;
- break;
+ if (i2s->bit_clk_master) {
+ switch (rate) {
+ case 176400:
+ case 88200:
+ case 44100:
+ case 22050:
+ case 11025:
+ clk_rate = 22579200;
+ break;
- case 192000:
- case 128000:
- case 96000:
- case 64000:
- case 48000:
- case 32000:
- case 24000:
- case 16000:
- case 12000:
- case 8000:
- clk_rate = 24576000;
- break;
+ case 192000:
+ case 128000:
+ case 96000:
+ case 64000:
+ case 48000:
+ case 32000:
+ case 24000:
+ case 16000:
+ case 12000:
+ case 8000:
+ clk_rate = 24576000;
+ break;
- default:
- dev_err(dai->dev, "Unsupported sample rate: %u\n", rate);
- return -EINVAL;
- }
+ default:
+ dev_err(dai->dev, "Unsupported sample rate: %u\n", rate);
+ return -EINVAL;
+ }
- ret = clk_set_rate(i2s->mod_clk, clk_rate);
- if (ret)
- return ret;
+ ret = clk_set_rate(i2s->mod_clk, clk_rate);
+ if (ret) {
+ dev_err(dai->dev, "Unable to set clock\n");
+ return ret;
+ }
- oversample_rate = i2s->mclk_freq / rate;
- if (!sun4i_i2s_oversample_is_valid(oversample_rate)) {
- dev_err(dai->dev, "Unsupported oversample rate: %d\n",
- oversample_rate);
- return -EINVAL;
- }
+ oversample_rate = i2s->mclk_freq / rate;
+ if (!sun4i_i2s_oversample_is_valid(oversample_rate)) {
+ dev_err(dai->dev, "Unsupported oversample rate: %d\n",
+ oversample_rate);
+ return -EINVAL;
+ }
- if (i2s->variant->has_fmt_set_lrck_period)
- bclk_div = sun4i_i2s_get_bclk_div(i2s, clk_rate / rate,
- word_size,
- sun8i_i2s_clk_div,
- ARRAY_SIZE(sun8i_i2s_clk_div));
- else
- bclk_div = sun4i_i2s_get_bclk_div(i2s, oversample_rate,
- word_size,
- sun4i_i2s_bclk_div,
- ARRAY_SIZE(sun4i_i2s_bclk_div));
- if (bclk_div < 0) {
- dev_err(dai->dev, "Unsupported BCLK divider: %d\n",
- bclk_div);
- return -EINVAL;
- }
+ if (i2s->variant->has_fmt_set_lrck_period)
+ bclk_div = sun4i_i2s_get_bclk_div(i2s, clk_rate / rate,
+ word_size,
+ sun8i_i2s_clk_div,
+ ARRAY_SIZE(sun8i_i2s_clk_div));
+ else
+ bclk_div = sun4i_i2s_get_bclk_div(i2s, oversample_rate,
+ word_size,
+ sun4i_i2s_bclk_div,
+ ARRAY_SIZE(sun4i_i2s_bclk_div));
+ if (bclk_div < 0) {
+ dev_err(dai->dev, "Unsupported BCLK divider: %d\n",
+ bclk_div);
+ return -EINVAL;
+ }
- if (i2s->variant->has_fmt_set_lrck_period)
- mclk_div = sun4i_i2s_get_mclk_div(i2s, oversample_rate,
- clk_rate, rate,
- sun8i_i2s_clk_div,
- ARRAY_SIZE(sun8i_i2s_clk_div));
- else
- mclk_div = sun4i_i2s_get_mclk_div(i2s, oversample_rate,
- clk_rate, rate,
- sun4i_i2s_mclk_div,
- ARRAY_SIZE(sun4i_i2s_mclk_div));
- if (mclk_div < 0) {
- dev_err(dai->dev, "Unsupported MCLK divider: %d\n",
- mclk_div);
- return -EINVAL;
- }
+ if (i2s->variant->has_fmt_set_lrck_period)
+ mclk_div = sun4i_i2s_get_mclk_div(i2s, oversample_rate,
+ clk_rate, rate,
+ sun8i_i2s_clk_div,
+ ARRAY_SIZE(sun8i_i2s_clk_div));
+ else
+ mclk_div = sun4i_i2s_get_mclk_div(i2s, oversample_rate,
+ clk_rate, rate,
+ sun4i_i2s_mclk_div,
+ ARRAY_SIZE(sun4i_i2s_mclk_div));
+ if (mclk_div < 0) {
+ dev_err(dai->dev, "Unsupported MCLK divider: %d\n",
+ mclk_div);
+ return -EINVAL;
+ }
- regmap_write(i2s->regmap, SUN4I_I2S_CLK_DIV_REG,
- SUN4I_I2S_CLK_DIV_BCLK(bclk_div) |
- SUN4I_I2S_CLK_DIV_MCLK(mclk_div));
+ regmap_write(i2s->regmap, SUN4I_I2S_CLK_DIV_REG,
+ SUN4I_I2S_CLK_DIV_BCLK(bclk_div) |
+ SUN4I_I2S_CLK_DIV_MCLK(mclk_div));
- regmap_field_write(i2s->field_clkdiv_mclk_en, 1);
+ regmap_field_write(i2s->field_clkdiv_mclk_en, 1);
+ }
/* Set sync period */
if (i2s->variant->has_fmt_set_lrck_period)
@@ -574,10 +580,12 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
case SND_SOC_DAIFMT_CBS_CFS:
/* BCLK and LRCLK master */
val = SUN4I_I2S_CTRL_MODE_MASTER;
+ i2s->bit_clk_master = true;
break;
case SND_SOC_DAIFMT_CBM_CFM:
/* BCLK and LRCLK slave */
val = SUN4I_I2S_CTRL_MODE_SLAVE;
+ i2s->bit_clk_master = false;
break;
default:
dev_err(dai->dev, "Unsupported slave setting: %d\n",
@@ -598,10 +606,12 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
/* BCLK and LRCLK master */
val = SUN8I_I2S_CTRL_BCLK_OUT |
SUN8I_I2S_CTRL_LRCK_OUT;
+ i2s->bit_clk_master = true;
break;
case SND_SOC_DAIFMT_CBM_CFM:
/* BCLK and LRCLK slave */
val = 0;
+ i2s->bit_clk_master = false;
break;
default:
dev_err(dai->dev, "Unsupported slave setting: %d\n",
--
2.20.1
On Fri, Dec 21, 2018 at 11:21 PM <[email protected]> wrote:
>
> From: Marcus Cooper <[email protected]>
>
> Bypass the regmap cache when flushing the i2s FIFOs and modify the tables
> to reflect this.
>
> Signed-off-by: Marcus Cooper <[email protected]>
> ---
> 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 d5ec1a20499d..64d073cb2aee 100644
> --- a/sound/soc/sunxi/sun4i-i2s.c
> +++ b/sound/soc/sunxi/sun4i-i2s.c
> @@ -548,9 +548,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);
IIRC the flush cache bit is self-clearing. So you likely want to mark
this register as volatile. If it is marked as volatile, then all access
to that register bypasses the cache, so the regcache_cache_bypass calls
are unneeded.
However, looking at the code, the write would seem to be ignored if the
regmap is in the cache_only state. We only set this when the bus clock
is disabled. Under such a condition, bypassing the cache and forcing a
write would be unwise, as the system either drops the write, or stalls
altogether.
>
> /* Clear RX counter */
> regmap_write(i2s->regmap, SUN4I_I2S_RX_CNT_REG, 0);
> @@ -569,9 +571,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);
> @@ -703,13 +707,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;
I don't understand why this is relevant. Do you need to read back from the TX
FIFO?
If so, just drop the call-back altogether. If no callback is provided and no
rd_table is provided either, it defaults to all registers under max_register
(if max_register < 0) are readable.
However this seems like it deserves to be a separate patch (where you explain
in the commit log why it's needed).
Regards
ChenYu
> }
>
> static bool sun4i_i2s_wr_reg(struct device *dev, unsigned int reg)
> @@ -728,6 +726,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:
> @@ -738,23 +738,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);
> }
> @@ -809,7 +798,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,
> };
>
> --
> 2.20.1
>
On Fri, Dec 21, 2018 at 11:21 PM <[email protected]> wrote:
>
> From: Marcus Cooper <[email protected]>
>
> 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 <[email protected]>
> ---
> sound/soc/sunxi/sun4i-i2s.c | 41 ++++++++++++++++++++++++++++++++++---
> 1 file changed, 38 insertions(+), 3 deletions(-)
>
> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> index 80bf29e0cc86..adb988ae9ac5 100644
> --- a/sound/soc/sunxi/sun4i-i2s.c
> +++ b/sound/soc/sunxi/sun4i-i2s.c
> @@ -399,6 +399,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));
> @@ -411,7 +416,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));
> @@ -687,6 +703,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_3LE | \
> + SNDRV_PCM_FMTBIT_S24_LE)
IIRC SNDRV_PCM_FMTBIT_S24_LE and SNDRV_PCM_FMTBIT_S32_LE with .sig_bits = 24
are different things, in regards to how the sample is aligned in a 4 byte
space. Have you checked that things actually work as expected? This was
something that did not work using SNDRV_PCM_FMTBIT_S24_LE on sun4i-codec.
> +
> +#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 = {
> @@ -694,14 +717,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,
> @@ -1106,6 +1129,18 @@ 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 (i2s->variant->has_fmt_set_lrck_period) {
This condition deserves a comment, particularly as to why you use it
in place of having a separate quirk field to represent 32 bit support.
Regards
ChenYu
> + soc_dai->playback.formats = SUN8I_FORMATS;
> + soc_dai->capture.formats = SUN8I_FORMATS;
> + }
> +
> pm_runtime_enable(&pdev->dev);
> if (!pm_runtime_enabled(&pdev->dev)) {
> ret = sun4i_i2s_runtime_resume(&pdev->dev);
> --
> 2.20.1
>
On Fri, Dec 21, 2018 at 11:21 PM <[email protected]> wrote:
>
> From: Marcus Cooper <[email protected]>
>
> On the newer SoCs this is set by default to transfer a 0 after
> each sample in each slot. Add the regmap field to configure this
> and set it so that it pads the sample with 0s.
>
> Signed-off-by: Marcus Cooper <[email protected]>
The code matches the description, however I lack the in depth
knowledge to understand the difference.
ChenYu
On Sat, Dec 22, 2018 at 12:48 AM Chen-Yu Tsai <[email protected]> wrote:
>
> On Fri, Dec 21, 2018 at 11:21 PM <[email protected]> wrote:
> >
> > From: Marcus Cooper <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > sound/soc/sunxi/sun4i-i2s.c | 41 ++++++++++++++++++++++++++++++++++---
> > 1 file changed, 38 insertions(+), 3 deletions(-)
> >
> > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> > index 80bf29e0cc86..adb988ae9ac5 100644
> > --- a/sound/soc/sunxi/sun4i-i2s.c
> > +++ b/sound/soc/sunxi/sun4i-i2s.c
> > @@ -399,6 +399,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));
> > @@ -411,7 +416,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));
> > @@ -687,6 +703,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_3LE | \
> > + SNDRV_PCM_FMTBIT_S24_LE)
>
> IIRC SNDRV_PCM_FMTBIT_S24_LE and SNDRV_PCM_FMTBIT_S32_LE with .sig_bits = 24
> are different things, in regards to how the sample is aligned in a 4 byte
> space. Have you checked that things actually work as expected? This was
> something that did not work using SNDRV_PCM_FMTBIT_S24_LE on sun4i-codec.
OK. I see the I2S driver is using FIFO mode 1, which expects the sample
to be in the lower portion of the 4 bytes. However it looks like you would
want SNDRV_PCM_FMTBIT_S20_LE (20 bits LSB justified in 4 bytes) instead of
SNDRV_PCM_FMTBIT_S20_3LE (20 bits LSB justified in 3 bytes).
ChenYu
> > +
> > +#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 = {
> > @@ -694,14 +717,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,
> > @@ -1106,6 +1129,18 @@ 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 (i2s->variant->has_fmt_set_lrck_period) {
>
> This condition deserves a comment, particularly as to why you use it
> in place of having a separate quirk field to represent 32 bit support.
>
> Regards
> ChenYu
>
> > + soc_dai->playback.formats = SUN8I_FORMATS;
> > + soc_dai->capture.formats = SUN8I_FORMATS;
> > + }
> > +
> > pm_runtime_enable(&pdev->dev);
> > if (!pm_runtime_enabled(&pdev->dev)) {
> > ret = sun4i_i2s_runtime_resume(&pdev->dev);
> > --
> > 2.20.1
> >
On Fri, Dec 21, 2018 at 11:21 PM <[email protected]> wrote:
>
> From: Marcus Cooper <[email protected]>
>
> Also add offset to RX channel select
>
> Signed-off-by: Marcus Cooper <[email protected]>
Commit log seems a bit lacking. You could probably explain how you
found this, either when comparing datasheet macros, or some actual
error manifested and you "heard" it. You could also state what the
fixed outcome should be.
Also please add a fixes tag.
The RX channel select offset should be added as a separate patch,
also with an explanation on how you found this missing, and a fixes
tag.
ChenYu
> ---
> sound/soc/sunxi/sun4i-i2s.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> index adb988ae9ac5..93a484d7e228 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)
> @@ -482,6 +482,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);
> --
> 2.20.1
>
On 2018-12-21 17:44, Chen-Yu Tsai wrote:
> On Fri, Dec 21, 2018 at 11:21 PM <[email protected]> wrote:
>> From: Marcus Cooper <[email protected]>
>>
>> Bypass the regmap cache when flushing the i2s FIFOs and modify the tables
>> to reflect this.
>>
>> Signed-off-by: Marcus Cooper <[email protected]>
>> ---
>> 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 d5ec1a20499d..64d073cb2aee 100644
>> --- a/sound/soc/sunxi/sun4i-i2s.c
>> +++ b/sound/soc/sunxi/sun4i-i2s.c
>> @@ -548,9 +548,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);
> IIRC the flush cache bit is self-clearing. So you likely want to mark
> this register as volatile. If it is marked as volatile, then all access
> to that register bypasses the cache, so the regcache_cache_bypass calls
> are unneeded.
I helped test this together with Marcus and when I tested with this
register marked
as volatile audio started to stutter, still unclear why audio starts to
stutter.
Without this cache bypass the flush TX/RX bits gets cached and flush
happens unexpectedly
resulting in multi channel audio getting mapped to wrong speaker.
Other ASoC codecs and fsl_spdif.c seems to use similar cache bypass for
reset/flush.
>
> However, looking at the code, the write would seem to be ignored if the
> regmap is in the cache_only state. We only set this when the bus clock
> is disabled. Under such a condition, bypassing the cache and forcing a
> write would be unwise, as the system either drops the write, or stalls
> altogether.
>
>> /* Clear RX counter */
>> regmap_write(i2s->regmap, SUN4I_I2S_RX_CNT_REG, 0);
>> @@ -569,9 +571,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);
>> @@ -703,13 +707,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;
> I don't understand why this is relevant. Do you need to read back from the TX
> FIFO?
This change was inspired by c66234cfedfc "ASoC: rockchip: i2s: fix
playback after runtime resume" [1]
On resume a cached sample would be written to FIFO_TX_REG unless it is
marked volatile,
the rockchip commit indicated that read is needed for volatile regs.
[1]
https://github.com/torvalds/linux/commit/c66234cfedfc3e6e3b62563a5f2c1562be09a35d
>
> If so, just drop the call-back altogether. If no callback is provided and no
> rd_table is provided either, it defaults to all registers under max_register
> (if max_register < 0) are readable.
>
> However this seems like it deserves to be a separate patch (where you explain
> in the commit log why it's needed).
>
> Regards
> ChenYu
>
>> }
>>
>> static bool sun4i_i2s_wr_reg(struct device *dev, unsigned int reg)
>> @@ -728,6 +726,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:
>> @@ -738,23 +738,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);
>> }
>> @@ -809,7 +798,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,
>> };
>>
>> --
>> 2.20.1
>>
> _______________________________________________
> Alsa-devel mailing list
> [email protected]
>
On Sun, Dec 23, 2018 at 6:12 AM Jonas Karlman <[email protected]> wrote:
>
> On 2018-12-21 17:44, Chen-Yu Tsai wrote:
> > On Fri, Dec 21, 2018 at 11:21 PM <[email protected]> wrote:
> >> From: Marcus Cooper <[email protected]>
> >>
> >> Bypass the regmap cache when flushing the i2s FIFOs and modify the tables
> >> to reflect this.
> >>
> >> Signed-off-by: Marcus Cooper <[email protected]>
> >> ---
> >> 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 d5ec1a20499d..64d073cb2aee 100644
> >> --- a/sound/soc/sunxi/sun4i-i2s.c
> >> +++ b/sound/soc/sunxi/sun4i-i2s.c
> >> @@ -548,9 +548,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);
> > IIRC the flush cache bit is self-clearing. So you likely want to mark
> > this register as volatile. If it is marked as volatile, then all access
> > to that register bypasses the cache, so the regcache_cache_bypass calls
> > are unneeded.
>
> I helped test this together with Marcus and when I tested with this
> register marked
> as volatile audio started to stutter, still unclear why audio starts to
> stutter.
Sounds like we're missing something. However the only other time we
update this register is to set the FIFO mode.
> Without this cache bypass the flush TX/RX bits gets cached and flush
> happens unexpectedly
> resulting in multi channel audio getting mapped to wrong speaker.
This sounds like the flush is happening after DMA transfers and/or I2S
operations have started, disrupting the order of the audio samples. I
think that might be the case since the regcache is synced sequentially,
and the FIFO control register is after the enable bits. That would imply
that the device is taken out of runtime suspend after the .start_capture
or .start_playback callbacks. Not sure if that's the case, but that would
mean the bus clocks are still off at this point, and bypassing the cache
and updating the bits is basically moot.
I think there's something else happening here, but we need to figure it
out instead of papering over it with something that "just works" but
we don't know why it works.
> Other ASoC codecs and fsl_spdif.c seems to use similar cache bypass for
> reset/flush.
fsl_spdif.c does it when forcing a soft reset, which would reset all the
registers. It then does a cache sync, restoring any values set up. That's
not what we're doing here.
> >
> > However, looking at the code, the write would seem to be ignored if the
> > regmap is in the cache_only state. We only set this when the bus clock
> > is disabled. Under such a condition, bypassing the cache and forcing a
> > write would be unwise, as the system either drops the write, or stalls
> > altogether.
> >
> >> /* Clear RX counter */
> >> regmap_write(i2s->regmap, SUN4I_I2S_RX_CNT_REG, 0);
> >> @@ -569,9 +571,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);
> >> @@ -703,13 +707,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;
> > I don't understand why this is relevant. Do you need to read back from the TX
> > FIFO?
>
> This change was inspired by c66234cfedfc "ASoC: rockchip: i2s: fix
> playback after runtime resume" [1]
> On resume a cached sample would be written to FIFO_TX_REG unless it is
> marked volatile,
> the rockchip commit indicated that read is needed for volatile regs.
>
> [1]
> https://github.com/torvalds/linux/commit/c66234cfedfc3e6e3b62563a5f2c1562be09a35d
What about simply marking the FIFO registers as both unreadable and
unwritable, thus excluding them from the regmap? That should get rid
of any unwanted syncs. We are doing DMA transfers to/from them. Do we
need regmap access?
Either way this context should be provided in the commit log.
Regards
ChenYu
> >
> > If so, just drop the call-back altogether. If no callback is provided and no
> > rd_table is provided either, it defaults to all registers under max_register
> > (if max_register < 0) are readable.
> >
> > However this seems like it deserves to be a separate patch (where you explain
> > in the commit log why it's needed).
> >
> > Regards
> > ChenYu
> >
> >> }
> >>
> >> static bool sun4i_i2s_wr_reg(struct device *dev, unsigned int reg)
> >> @@ -728,6 +726,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:
> >> @@ -738,23 +738,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);
> >> }
> >> @@ -809,7 +798,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,
> >> };
> >>
> >> --
> >> 2.20.1
> >>
> > _______________________________________________
> > Alsa-devel mailing list
> > [email protected]
> >
On Sat, Dec 22, 2018 at 12:44:07AM +0800, Chen-Yu Tsai wrote:
> On Fri, Dec 21, 2018 at 11:21 PM <[email protected]> wrote:
> > + 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);
> IIRC the flush cache bit is self-clearing. So you likely want to mark
> this register as volatile. If it is marked as volatile, then all access
> to that register bypasses the cache, so the regcache_cache_bypass calls
> are unneeded.
Yes, that should be the case.
> However, looking at the code, the write would seem to be ignored if the
> regmap is in the cache_only state. We only set this when the bus clock
> is disabled. Under such a condition, bypassing the cache and forcing a
> write would be unwise, as the system either drops the write, or stalls
> altogether.
Right, access to a cache only register while the device is in cache only
mode is not a great idea - the usual reason we're in cache only mode is
that the device is in a state where I/O isn't going to work. One thing
that can work for this if you need the register to be cached (but is a
bit gross) is to do a write setting the self clearing bit then another
immediately after resetting it back to the cleared state. That works OK
for cases where the bit is a strobe and never retains state, though if
the device isn't operational then needing to write to the register might
indicate a bigger picture logic error (or it could be that the register
map mixes random things into one register).
On Sat, Dec 22, 2018 at 12:54:48AM +0800, Chen-Yu Tsai wrote:
> On Fri, Dec 21, 2018 at 11:21 PM <[email protected]> wrote:
> > Also add offset to RX channel select
> Commit log seems a bit lacking. You could probably explain how you
> found this, either when comparing datasheet macros, or some actual
> error manifested and you "heard" it. You could also state what the
> fixed outcome should be.
> Also please add a fixes tag.
Plus send this at the start of the series so it can be applied as a fix
and sent to Linus during the development cycle.
On Sun, Dec 23, 2018 at 11:16:31AM +0800, Chen-Yu Tsai wrote:
> This sounds like the flush is happening after DMA transfers and/or I2S
> operations have started, disrupting the order of the audio samples. I
> think that might be the case since the regcache is synced sequentially,
> and the FIFO control register is after the enable bits. That would imply
> that the device is taken out of runtime suspend after the .start_capture
> or .start_playback callbacks. Not sure if that's the case, but that would
> mean the bus clocks are still off at this point, and bypassing the cache
> and updating the bits is basically moot.
I would expect that the device needs to be resumed from suspend before
we start actually trying to transfer audio - there is stuff in the ASoC
core which is supposed to have appropriate gets but it's possible
something is going wrong there.
> I think there's something else happening here, but we need to figure it
> out instead of papering over it with something that "just works" but
> we don't know why it works.
I agree.