[alsa-devel] RFC: TDM mode support on the tegra30
This is an initial set that adds support for both the TDM and for formats other than 8/16bit audio through the audio system. It also fixes a minor issue with the FIFO stopping order.
I have further patches following on which deal with selection of clocks to allow the use of the tegra's audio unit to run as clock slave (all current examples have the tegra as clock master).
From: Edward Cragg edward.cragg@codethink.co.uk
Add a callback to configure TDM settings for the Tegra30 I2S ASoC 'platform' driver.
Signed-off-by: Edward Cragg edward.cragg@codethink.co.uk --- sound/soc/tegra/tegra30_i2s.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)
diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c index ac6983c6bd72..d36b4662b420 100644 --- a/sound/soc/tegra/tegra30_i2s.c +++ b/sound/soc/tegra/tegra30_i2s.c @@ -254,6 +254,39 @@ static int tegra30_i2s_trigger(struct snd_pcm_substream *substream, int cmd, return 0; }
+/* + * Set up TDM + */ +static int tegra30_i2s_set_tdm(struct snd_soc_dai *dai, + unsigned int tx_mask, unsigned int rx_mask, + int slots, int slot_width) +{ + struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai); + unsigned int mask = 0, val = 0; + + dev_info(dai->dev, "%s: setting TDM: tx_mask: 0x%08x rx_mask: 0x%08x " + "slots: 0x%08x " "width: %d\n", + __func__, tx_mask, rx_mask, slots, slot_width); + + /* Set up slots and tx/rx masks */ + mask = TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK | + TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_MASK | + TEGRA30_I2S_SLOT_CTRL_TX_SLOT_ENABLES_MASK; + + val = (tx_mask << TEGRA30_I2S_SLOT_CTRL_TX_SLOT_ENABLES_SHIFT) | + (rx_mask << TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_SHIFT) | + ((slots - 1) << TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_SHIFT); + + regmap_update_bits(i2s->regmap, TEGRA30_I2S_SLOT_CTRL, mask, val); + + /* Set FSYNC width */ + regmap_update_bits(i2s->regmap, TEGRA30_I2S_CH_CTRL, + TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_MASK, + (slot_width - 1) << TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_SHIFT); + + return 0; +} + static int tegra30_i2s_probe(struct snd_soc_dai *dai) { struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai); @@ -268,6 +301,7 @@ static const struct snd_soc_dai_ops tegra30_i2s_dai_ops = { .set_fmt = tegra30_i2s_set_fmt, .hw_params = tegra30_i2s_hw_params, .trigger = tegra30_i2s_trigger, + .set_tdm_slot = tegra30_i2s_set_tdm, };
static const struct snd_soc_dai_driver tegra30_i2s_dai_template = {
On 9/17/19 1:12 PM, Ben Dooks wrote:
From: Edward Cragg edward.cragg@codethink.co.uk
Add a callback to configure TDM settings for the Tegra30 I2S ASoC 'platform' driver.
Signed-off-by: Edward Cragg edward.cragg@codethink.co.uk
sound/soc/tegra/tegra30_i2s.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)
diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c index ac6983c6bd72..d36b4662b420 100644 --- a/sound/soc/tegra/tegra30_i2s.c +++ b/sound/soc/tegra/tegra30_i2s.c @@ -254,6 +254,39 @@ static int tegra30_i2s_trigger(struct snd_pcm_substream *substream, int cmd, return 0; }
+/*
- Set up TDM
- */
+static int tegra30_i2s_set_tdm(struct snd_soc_dai *dai,
unsigned int tx_mask, unsigned int rx_mask,int slots, int slot_width)+{
- struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai);
- unsigned int mask = 0, val = 0;
initialization is not needed.
- dev_info(dai->dev, "%s: setting TDM: tx_mask: 0x%08x rx_mask: 0x%08x "
"slots: 0x%08x " "width: %d\n",__func__, tx_mask, rx_mask, slots, slot_width);- /* Set up slots and tx/rx masks */
- mask = TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK |
TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_MASK |TEGRA30_I2S_SLOT_CTRL_TX_SLOT_ENABLES_MASK;- val = (tx_mask << TEGRA30_I2S_SLOT_CTRL_TX_SLOT_ENABLES_SHIFT) |
(rx_mask << TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_SHIFT) |((slots - 1) << TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_SHIFT);- regmap_update_bits(i2s->regmap, TEGRA30_I2S_SLOT_CTRL, mask, val);
- /* Set FSYNC width */
- regmap_update_bits(i2s->regmap, TEGRA30_I2S_CH_CTRL,
TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_MASK,(slot_width - 1) << TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_SHIFT);- return 0;
+}
- static int tegra30_i2s_probe(struct snd_soc_dai *dai) { struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai);
@@ -268,6 +301,7 @@ static const struct snd_soc_dai_ops tegra30_i2s_dai_ops = { .set_fmt = tegra30_i2s_set_fmt, .hw_params = tegra30_i2s_hw_params, .trigger = tegra30_i2s_trigger,
.set_tdm_slot = tegra30_i2s_set_tdm, };
static const struct snd_soc_dai_driver tegra30_i2s_dai_template = {
On 17/09/2019 19:12, Ben Dooks wrote:
From: Edward Cragg edward.cragg@codethink.co.uk
Add a callback to configure TDM settings for the Tegra30 I2S ASoC 'platform' driver.
Signed-off-by: Edward Cragg edward.cragg@codethink.co.uk
sound/soc/tegra/tegra30_i2s.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)
diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c index ac6983c6bd72..d36b4662b420 100644 --- a/sound/soc/tegra/tegra30_i2s.c +++ b/sound/soc/tegra/tegra30_i2s.c @@ -254,6 +254,39 @@ static int tegra30_i2s_trigger(struct snd_pcm_substream *substream, int cmd, return 0; }
+/*
- Set up TDM
- */
+static int tegra30_i2s_set_tdm(struct snd_soc_dai *dai,
unsigned int tx_mask, unsigned int rx_mask,int slots, int slot_width)+{
- struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai);
- unsigned int mask = 0, val = 0;
- dev_info(dai->dev, "%s: setting TDM: tx_mask: 0x%08x rx_mask: 0x%08x "
dev_dbg() please. Also I don't think it is necessary to print both the function name and 'setting TDM', the function name should be sufficient.
"slots: 0x%08x " "width: %d\n",
Why are there extra quotes here?
__func__, tx_mask, rx_mask, slots, slot_width)> +- /* Set up slots and tx/rx masks */
- mask = TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK |
TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_MASK |TEGRA30_I2S_SLOT_CTRL_TX_SLOT_ENABLES_MASK;- val = (tx_mask << TEGRA30_I2S_SLOT_CTRL_TX_SLOT_ENABLES_SHIFT) |
(rx_mask << TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_SHIFT) |((slots - 1) << TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_SHIFT);- regmap_update_bits(i2s->regmap, TEGRA30_I2S_SLOT_CTRL, mask, val);
- /* Set FSYNC width */
- regmap_update_bits(i2s->regmap, TEGRA30_I2S_CH_CTRL,
TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_MASK,(slot_width - 1) << TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_SHIFT);
What happens if there is only one slot? The fsync will be the width of the slot. Typically, TDM is used with DSP-A/B formats and although the driver does not appear to program the fsync width, it probably should during the tegra30_i2s_set_fmt() depending on the format used rather than here.
Cheers Jon
On 2019-09-18 09:42, Jon Hunter wrote:
On 17/09/2019 19:12, Ben Dooks wrote:
From: Edward Cragg edward.cragg@codethink.co.uk
Add a callback to configure TDM settings for the Tegra30 I2S ASoC 'platform' driver.
Signed-off-by: Edward Cragg edward.cragg@codethink.co.uk
sound/soc/tegra/tegra30_i2s.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)
diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c index ac6983c6bd72..d36b4662b420 100644 --- a/sound/soc/tegra/tegra30_i2s.c +++ b/sound/soc/tegra/tegra30_i2s.c @@ -254,6 +254,39 @@ static int tegra30_i2s_trigger(struct snd_pcm_substream *substream, int cmd, return 0; }
+/*
- Set up TDM
- */
+static int tegra30_i2s_set_tdm(struct snd_soc_dai *dai,
unsigned int tx_mask, unsigned int rx_mask,int slots, int slot_width)+{
- struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai);
- unsigned int mask = 0, val = 0;
- dev_info(dai->dev, "%s: setting TDM: tx_mask: 0x%08x rx_mask: 0x%08x
"
dev_dbg() please. Also I don't think it is necessary to print both the function name and 'setting TDM', the function name should be sufficient.
Yes, already sorted from previous review.
"slots: 0x%08x " "width: %d\n",Why are there extra quotes here?
No idea, I'll remove these later.
__func__, tx_mask, rx_mask, slots, slot_width)> +- /* Set up slots and tx/rx masks */
- mask = TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK |
TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_MASK |TEGRA30_I2S_SLOT_CTRL_TX_SLOT_ENABLES_MASK;- val = (tx_mask << TEGRA30_I2S_SLOT_CTRL_TX_SLOT_ENABLES_SHIFT) |
(rx_mask << TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_SHIFT) |((slots - 1) << TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_SHIFT);- regmap_update_bits(i2s->regmap, TEGRA30_I2S_SLOT_CTRL, mask, val);
- /* Set FSYNC width */
- regmap_update_bits(i2s->regmap, TEGRA30_I2S_CH_CTRL,
TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_MASK,(slot_width - 1) << TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_SHIFT);What happens if there is only one slot? The fsync will be the width of the slot. Typically, TDM is used with DSP-A/B formats and although the driver does not appear to program the fsync width, it probably should during the tegra30_i2s_set_fmt() depending on the format used rather than here.
Hmm, should we check.
The work was done to keep as close to the original client's 2.6 kernel as possible which set the fsync field to a whole data word. We could try and just set this to say 2 here and have a much shorter frame-sync pulse.
I'll add a check for slots < 2 and set the fsync width to 2.
Thanks for the review.
Cheers Jon
On 18/09/2019 11:11, Ben Dooks wrote:
On 2019-09-18 09:42, Jon Hunter wrote:
On 17/09/2019 19:12, Ben Dooks wrote:
From: Edward Cragg edward.cragg@codethink.co.uk
Add a callback to configure TDM settings for the Tegra30 I2S ASoC 'platform' driver.
Signed-off-by: Edward Cragg edward.cragg@codethink.co.uk
sound/soc/tegra/tegra30_i2s.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)
diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c index ac6983c6bd72..d36b4662b420 100644 --- a/sound/soc/tegra/tegra30_i2s.c +++ b/sound/soc/tegra/tegra30_i2s.c @@ -254,6 +254,39 @@ static int tegra30_i2s_trigger(struct snd_pcm_substream *substream, int cmd, return 0; }
+/*
- Set up TDM
- */
+static int tegra30_i2s_set_tdm(struct snd_soc_dai *dai, + unsigned int tx_mask, unsigned int rx_mask, + int slots, int slot_width) +{ + struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai); + unsigned int mask = 0, val = 0;
+ dev_info(dai->dev, "%s: setting TDM: tx_mask: 0x%08x rx_mask: 0x%08x "
dev_dbg() please. Also I don't think it is necessary to print both the function name and 'setting TDM', the function name should be sufficient.
Yes, already sorted from previous review.
+ "slots: 0x%08x " "width: %d\n",
Why are there extra quotes here?
No idea, I'll remove these later.
+ __func__, tx_mask, rx_mask, slots, slot_width)> + + /* Set up slots and tx/rx masks */ + mask = TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK | + TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_MASK | + TEGRA30_I2S_SLOT_CTRL_TX_SLOT_ENABLES_MASK;
+ val = (tx_mask << TEGRA30_I2S_SLOT_CTRL_TX_SLOT_ENABLES_SHIFT) | + (rx_mask << TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_SHIFT) | + ((slots - 1) << TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_SHIFT);
+ regmap_update_bits(i2s->regmap, TEGRA30_I2S_SLOT_CTRL, mask, val);
+ /* Set FSYNC width */ + regmap_update_bits(i2s->regmap, TEGRA30_I2S_CH_CTRL, + TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_MASK, + (slot_width - 1) << TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_SHIFT);
What happens if there is only one slot? The fsync will be the width of the slot. Typically, TDM is used with DSP-A/B formats and although the driver does not appear to program the fsync width, it probably should during the tegra30_i2s_set_fmt() depending on the format used rather than here.
Hmm, should we check.
The work was done to keep as close to the original client's 2.6 kernel as possible which set the fsync field to a whole data word. We could try and just set this to say 2 here and have a much shorter frame-sync pulse.
I'll add a check for slots < 2 and set the fsync width to 2.
Why 2? From looking at various codecs that support dsp-a/b modes, it is more common for the f-sync to be 1 regardless of the number of slots.
Jon
On Wed, Sep 18, 2019 at 11:25:39AM +0100, Jon Hunter wrote:
Why 2? From looking at various codecs that support dsp-a/b modes, it is more common for the f-sync to be 1 regardless of the number of slots.
In DSP modes only one edge really matters anyway so it's not super important how long the pulse is.
On 2019-09-18 11:48, Mark Brown wrote:
On Wed, Sep 18, 2019 at 11:25:39AM +0100, Jon Hunter wrote:
Why 2? From looking at various codecs that support dsp-a/b modes, it is more common for the f-sync to be 1 regardless of the number of slots.
In DSP modes only one edge really matters anyway so it's not super important how long the pulse is.
I could never get an answer for why this was set as-is on the customer's setup and not sure if I have the ability to re-test this.
On 9/18/19 5:48 AM, Mark Brown wrote:
On Wed, Sep 18, 2019 at 11:25:39AM +0100, Jon Hunter wrote:
Why 2? From looking at various codecs that support dsp-a/b modes, it is more common for the f-sync to be 1 regardless of the number of slots.
In DSP modes only one edge really matters anyway so it's not super important how long the pulse is.
There are exceptions to the rule. In the early days of SOF, we had to provide support for amplifiers that did require a pulse larger than a bit. In the SOF IPC we added an 'frame_pulse_width' field to pass the configuration all the way from topology to the firmware and Intel SSP driver. The other quirk we added is the ability to control zero-padding per slot instead of at the end of the frame, e.g. 1 bit of padding after 24 bits when using 4 slots w/ 25 bits in a 100-bit frame.
On Wed, Sep 18, 2019 at 08:33:50AM -0500, Pierre-Louis Bossart wrote:
On 9/18/19 5:48 AM, Mark Brown wrote:
In DSP modes only one edge really matters anyway so it's not super important how long the pulse is.
There are exceptions to the rule. In the early days of SOF, we had to provide support for amplifiers that did require a pulse larger than a bit. In the SOF IPC we added an 'frame_pulse_width' field to pass the configuration all the way from topology to the firmware and Intel SSP driver. The other quirk we added is the ability to control zero-padding per slot instead of at the end of the frame, e.g. 1 bit of padding after 24 bits when using 4 slots w/ 25 bits in a 100-bit frame.
Neither of those is part of the core DSP mode definition though in the same way that constraints like MCLK or BCLK ratios aren't. They're modifiers on top.
From: Edward Cragg edward.cragg@codethink.co.uk
The tegra3 audio can support 24 and 32 bit sample sizes so add the option to the tegra30_i2s_hw_params to configure the S24_LE or S32_LE formats when requested.
Signed-off-by: Edward Cragg edward.cragg@codethink.co.uk [ben.dooks@codethink.co.uk: fixup merge of 24 and 32bit] [ben.dooks@codethink.co.uk: add pm calls around ytdm config] [ben.dooks@codethink.co.uk: remove debug prints] --- squash 5aeca5a055fd ASoC: tegra: i2s: pm_runtime_get_sync() is needed in tdm code
ASoC: tegra: i2s: pm_runtime_get_sync() is needed in tdm code --- sound/soc/tegra/tegra30_i2s.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-)
diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c index d36b4662b420..b5372839f672 100644 --- a/sound/soc/tegra/tegra30_i2s.c +++ b/sound/soc/tegra/tegra30_i2s.c @@ -127,7 +127,7 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream, struct device *dev = dai->dev; struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai); unsigned int mask, val, reg; - int ret, sample_size, srate, i2sclock, bitcnt; + int ret, sample_size, srate, i2sclock, bitcnt, audio_bits; struct tegra30_ahub_cif_conf cif_conf;
if (params_channels(params) != 2) @@ -137,8 +137,19 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream, switch (params_format(params)) { case SNDRV_PCM_FORMAT_S16_LE: val = TEGRA30_I2S_CTRL_BIT_SIZE_16; + audio_bits = TEGRA30_AUDIOCIF_BITS_16; sample_size = 16; break; + case SNDRV_PCM_FORMAT_S24_LE: + val = TEGRA30_I2S_CTRL_BIT_SIZE_24; + audio_bits = TEGRA30_AUDIOCIF_BITS_24; + sample_size = 24; + break; + case SNDRV_PCM_FORMAT_S32_LE: + val = TEGRA30_I2S_CTRL_BIT_SIZE_32; + audio_bits = TEGRA30_AUDIOCIF_BITS_32; + sample_size = 32; + break; default: return -EINVAL; } @@ -170,8 +181,8 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream, cif_conf.threshold = 0; cif_conf.audio_channels = 2; cif_conf.client_channels = 2; - cif_conf.audio_bits = TEGRA30_AUDIOCIF_BITS_16; - cif_conf.client_bits = TEGRA30_AUDIOCIF_BITS_16; + cif_conf.audio_bits = audio_bits; + cif_conf.client_bits = audio_bits; cif_conf.expand = 0; cif_conf.stereo_conv = 0; cif_conf.replicate = 0; @@ -264,10 +275,6 @@ static int tegra30_i2s_set_tdm(struct snd_soc_dai *dai, struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai); unsigned int mask = 0, val = 0;
- dev_info(dai->dev, "%s: setting TDM: tx_mask: 0x%08x rx_mask: 0x%08x " - "slots: 0x%08x " "width: %d\n", - __func__, tx_mask, rx_mask, slots, slot_width); - /* Set up slots and tx/rx masks */ mask = TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK | TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_MASK | @@ -277,6 +284,8 @@ static int tegra30_i2s_set_tdm(struct snd_soc_dai *dai, (rx_mask << TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_SHIFT) | ((slots - 1) << TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_SHIFT);
+ pm_runtime_get_sync(dai->dev); + regmap_update_bits(i2s->regmap, TEGRA30_I2S_SLOT_CTRL, mask, val);
/* Set FSYNC width */ @@ -284,6 +293,7 @@ static int tegra30_i2s_set_tdm(struct snd_soc_dai *dai, TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_MASK, (slot_width - 1) << TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_SHIFT);
+ pm_runtime_put(dai->dev); return 0; }
@@ -311,14 +321,18 @@ static const struct snd_soc_dai_driver tegra30_i2s_dai_template = { .channels_min = 2, .channels_max = 2, .rates = SNDRV_PCM_RATE_8000_96000, - .formats = SNDRV_PCM_FMTBIT_S16_LE, + .formats = SNDRV_PCM_FMTBIT_S32_LE | + SNDRV_PCM_FMTBIT_S24_LE | + SNDRV_PCM_FMTBIT_S16_LE, }, .capture = { .stream_name = "Capture", .channels_min = 2, .channels_max = 2, .rates = SNDRV_PCM_RATE_8000_96000, - .formats = SNDRV_PCM_FMTBIT_S16_LE, + .formats = SNDRV_PCM_FMTBIT_S32_LE | + SNDRV_PCM_FMTBIT_S24_LE | + SNDRV_PCM_FMTBIT_S16_LE, }, .ops = &tegra30_i2s_dai_ops, .symmetric_rates = 1,
On 9/17/19 1:12 PM, Ben Dooks wrote:
From: Edward Cragg edward.cragg@codethink.co.uk
The tegra3 audio can support 24 and 32 bit sample sizes so add the option to the tegra30_i2s_hw_params to configure the S24_LE or S32_LE formats when requested.
Signed-off-by: Edward Cragg edward.cragg@codethink.co.uk [ben.dooks@codethink.co.uk: fixup merge of 24 and 32bit] [ben.dooks@codethink.co.uk: add pm calls around ytdm config] [ben.dooks@codethink.co.uk: remove debug prints]
You need to add your own Signed-off-by when sending patches from other people
squash 5aeca5a055fd ASoC: tegra: i2s: pm_runtime_get_sync() is needed in tdm code
ASoC: tegra: i2s: pm_runtime_get_sync() is needed in tdm code
sound/soc/tegra/tegra30_i2s.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-)
diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c index d36b4662b420..b5372839f672 100644 --- a/sound/soc/tegra/tegra30_i2s.c +++ b/sound/soc/tegra/tegra30_i2s.c @@ -127,7 +127,7 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream, struct device *dev = dai->dev; struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai); unsigned int mask, val, reg;
- int ret, sample_size, srate, i2sclock, bitcnt;
int ret, sample_size, srate, i2sclock, bitcnt, audio_bits; struct tegra30_ahub_cif_conf cif_conf;
if (params_channels(params) != 2)
@@ -137,8 +137,19 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream, switch (params_format(params)) { case SNDRV_PCM_FORMAT_S16_LE: val = TEGRA30_I2S_CTRL_BIT_SIZE_16;
audio_bits = TEGRA30_AUDIOCIF_BITS_16;- case SNDRV_PCM_FORMAT_S24_LE:
val = TEGRA30_I2S_CTRL_BIT_SIZE_24;audio_bits = TEGRA30_AUDIOCIF_BITS_24;sample_size = 24;break;- case SNDRV_PCM_FORMAT_S32_LE:
val = TEGRA30_I2S_CTRL_BIT_SIZE_32;audio_bits = TEGRA30_AUDIOCIF_BITS_32;sample_size = 32;break;@@ -170,8 +181,8 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream, cif_conf.threshold = 0; cif_conf.audio_channels = 2; cif_conf.client_channels = 2;
- cif_conf.audio_bits = TEGRA30_AUDIOCIF_BITS_16;
- cif_conf.client_bits = TEGRA30_AUDIOCIF_BITS_16;
- cif_conf.audio_bits = audio_bits;
- cif_conf.client_bits = audio_bits; cif_conf.expand = 0; cif_conf.stereo_conv = 0; cif_conf.replicate = 0;
@@ -264,10 +275,6 @@ static int tegra30_i2s_set_tdm(struct snd_soc_dai *dai, struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai); unsigned int mask = 0, val = 0;
- dev_info(dai->dev, "%s: setting TDM: tx_mask: 0x%08x rx_mask: 0x%08x "
"slots: 0x%08x " "width: %d\n",__func__, tx_mask, rx_mask, slots, slot_width);
This should be squashed in the previous patch
/* Set up slots and tx/rx masks */ mask = TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK | TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_MASK | @@ -277,6 +284,8 @@ static int tegra30_i2s_set_tdm(struct snd_soc_dai *dai, (rx_mask << TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_SHIFT) | ((slots - 1) << TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_SHIFT);
pm_runtime_get_sync(dai->dev);
regmap_update_bits(i2s->regmap, TEGRA30_I2S_SLOT_CTRL, mask, val);
/* Set FSYNC width */
@@ -284,6 +293,7 @@ static int tegra30_i2s_set_tdm(struct snd_soc_dai *dai, TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_MASK, (slot_width - 1) << TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_SHIFT);
- pm_runtime_put(dai->dev);
same for PM stuff, it's not related to 24/32 bit samples
return 0; }
@@ -311,14 +321,18 @@ static const struct snd_soc_dai_driver tegra30_i2s_dai_template = { .channels_min = 2, .channels_max = 2, .rates = SNDRV_PCM_RATE_8000_96000,
.formats = SNDRV_PCM_FMTBIT_S16_LE,
.formats = SNDRV_PCM_FMTBIT_S32_LE |SNDRV_PCM_FMTBIT_S24_LE |SNDRV_PCM_FMTBIT_S16_LE,
.formats = SNDRV_PCM_FMTBIT_S16_LE,
.formats = SNDRV_PCM_FMTBIT_S32_LE |SNDRV_PCM_FMTBIT_S24_LE |SNDRV_PCM_FMTBIT_S16_LE,
On 2019-09-17 19:26, Pierre-Louis Bossart wrote:
On 9/17/19 1:12 PM, Ben Dooks wrote:
From: Edward Cragg edward.cragg@codethink.co.uk
The tegra3 audio can support 24 and 32 bit sample sizes so add the option to the tegra30_i2s_hw_params to configure the S24_LE or S32_LE formats when requested.
Signed-off-by: Edward Cragg edward.cragg@codethink.co.uk [ben.dooks@codethink.co.uk: fixup merge of 24 and 32bit] [ben.dooks@codethink.co.uk: add pm calls around ytdm config] [ben.dooks@codethink.co.uk: remove debug prints]
You need to add your own Signed-off-by when sending patches from other people
Yes, will do when this series has been reviewed and modifications done.
squash 5aeca5a055fd ASoC: tegra: i2s: pm_runtime_get_sync() is needed in tdm code
ASoC: tegra: i2s: pm_runtime_get_sync() is needed in tdm code
sound/soc/tegra/tegra30_i2s.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-)
diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c index d36b4662b420..b5372839f672 100644 --- a/sound/soc/tegra/tegra30_i2s.c +++ b/sound/soc/tegra/tegra30_i2s.c @@ -127,7 +127,7 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream, struct device *dev = dai->dev; struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai); unsigned int mask, val, reg;
- int ret, sample_size, srate, i2sclock, bitcnt;
- int ret, sample_size, srate, i2sclock, bitcnt, audio_bits; struct tegra30_ahub_cif_conf cif_conf; if (params_channels(params) != 2)
@@ -137,8 +137,19 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream, switch (params_format(params)) { case SNDRV_PCM_FORMAT_S16_LE: val = TEGRA30_I2S_CTRL_BIT_SIZE_16;
audio_bits = TEGRA30_AUDIOCIF_BITS_16;- case SNDRV_PCM_FORMAT_S24_LE:
val = TEGRA30_I2S_CTRL_BIT_SIZE_24;audio_bits = TEGRA30_AUDIOCIF_BITS_24;sample_size = 24;break;- case SNDRV_PCM_FORMAT_S32_LE:
val = TEGRA30_I2S_CTRL_BIT_SIZE_32;audio_bits = TEGRA30_AUDIOCIF_BITS_32;sample_size = 32;break;@@ -170,8 +181,8 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream, cif_conf.threshold = 0; cif_conf.audio_channels = 2; cif_conf.client_channels = 2;
- cif_conf.audio_bits = TEGRA30_AUDIOCIF_BITS_16;
- cif_conf.client_bits = TEGRA30_AUDIOCIF_BITS_16;
- cif_conf.audio_bits = audio_bits;
- cif_conf.client_bits = audio_bits; cif_conf.expand = 0; cif_conf.stereo_conv = 0; cif_conf.replicate = 0;
@@ -264,10 +275,6 @@ static int tegra30_i2s_set_tdm(struct snd_soc_dai *dai, struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai); unsigned int mask = 0, val = 0;
- dev_info(dai->dev, "%s: setting TDM: tx_mask: 0x%08x rx_mask:
0x%08x "
"slots: 0x%08x " "width: %d\n",__func__, tx_mask, rx_mask, slots, slot_width);This should be squashed in the previous patch
Thanks, looks like I missed a bad rebase and hit this patch instead of the previous.
/* Set up slots and tx/rx masks */ mask = TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK | TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_MASK | @@ -277,6 +284,8 @@ static int tegra30_i2s_set_tdm(struct snd_soc_dai *dai, (rx_mask << TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_SHIFT) | ((slots - 1) << TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_SHIFT);
- pm_runtime_get_sync(dai->dev);
- regmap_update_bits(i2s->regmap, TEGRA30_I2S_SLOT_CTRL, mask, val); /* Set FSYNC width */
@@ -284,6 +293,7 @@ static int tegra30_i2s_set_tdm(struct snd_soc_dai *dai, TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_MASK, (slot_width - 1) << TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_SHIFT);
- pm_runtime_put(dai->dev);
same for PM stuff, it's not related to 24/32 bit samples
Yes, will do. Thank you for reviewing.
return 0; } @@ -311,14 +321,18 @@ static const struct snd_soc_dai_driver tegra30_i2s_dai_template = { .channels_min = 2, .channels_max = 2, .rates = SNDRV_PCM_RATE_8000_96000,
.formats = SNDRV_PCM_FMTBIT_S16_LE,
.formats = SNDRV_PCM_FMTBIT_S32_LE |SNDRV_PCM_FMTBIT_S24_LE |SNDRV_PCM_FMTBIT_S16_LE,
.formats = SNDRV_PCM_FMTBIT_S16_LE,
.formats = SNDRV_PCM_FMTBIT_S32_LE |SNDRV_PCM_FMTBIT_S24_LE |SNDRV_PCM_FMTBIT_S16_LE,
On Wed, Sep 18, 2019 at 08:44:50AM +0100, Ben Dooks wrote:
On 2019-09-17 19:26, Pierre-Louis Bossart wrote:
You need to add your own Signed-off-by when sending patches from other people
Yes, will do when this series has been reviewed and modifications done.
I didn't look at it due to the lack of signoffs.
On 2019-09-18 11:08, Mark Brown wrote:
On Wed, Sep 18, 2019 at 08:44:50AM +0100, Ben Dooks wrote:
On 2019-09-17 19:26, Pierre-Louis Bossart wrote:
You need to add your own Signed-off-by when sending patches from other people
Yes, will do when this series has been reviewed and modifications done.
I didn't look at it due to the lack of signoffs.
Thanks, although you could have just flagged this and reviewed the rest anyway. I'll post a new version with signoff sorted at the end of the week as I cannot get in to the office to test any changes until Friday.
On Wed, Sep 18, 2019 at 12:50:13PM +0100, Ben Dooks wrote:
On 2019-09-18 11:08, Mark Brown wrote:
Yes, will do when this series has been reviewed and modifications done.
I didn't look at it due to the lack of signoffs.
Thanks, although you could have just flagged this and reviewed the rest anyway. I'll post a new version with signoff sorted at the end of the week as I cannot get in to the office to test any changes until Friday.
None of the patches I looked at had signoffs, Pierre had already told you about that problem and there were a bunch of other review comments anyway before I saw the series so it was fairly clear that a new version is needed anyway. Once you've got some review you shouldn't rely on getting extra review explicitly since it's not generally useful to repeat the same review comments others have already made.
From: Edward Cragg edward.cragg@codethink.co.uk
The CIF configuration and clock setting is currently hard coded for 2 channels. Since the hardware is capable of supporting 1-8 channels add support for reading the channel count from the supplied parameters to allow for better TDM support. It seems the original implementation of this driver was fixed at 2 channels for simplicity, and not implementing TDM.
Signed-off-by: Edward Cragg edward.cragg@codethink.co.uk [ben.dooks@codethink.co.uk: added is_tdm and channel nr check] --- sound/soc/tegra/tegra30_i2s.c | 21 +++++++++++++-------- sound/soc/tegra/tegra30_i2s.h | 1 + 2 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c index b5372839f672..40bcc05a9dbb 100644 --- a/sound/soc/tegra/tegra30_i2s.c +++ b/sound/soc/tegra/tegra30_i2s.c @@ -86,14 +86,17 @@ static int tegra30_i2s_set_fmt(struct snd_soc_dai *dai, return -EINVAL; }
+ i2s->is_tdm = false; mask |= TEGRA30_I2S_CTRL_FRAME_FORMAT_MASK | TEGRA30_I2S_CTRL_LRCK_MASK; switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) { case SND_SOC_DAIFMT_DSP_A: + i2s->is_tdm = true; val |= TEGRA30_I2S_CTRL_FRAME_FORMAT_FSYNC; val |= TEGRA30_I2S_CTRL_LRCK_L_LOW; break; case SND_SOC_DAIFMT_DSP_B: + i2s->is_tdm = true; val |= TEGRA30_I2S_CTRL_FRAME_FORMAT_FSYNC; val |= TEGRA30_I2S_CTRL_LRCK_R_LOW; break; @@ -127,10 +130,13 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream, struct device *dev = dai->dev; struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai); unsigned int mask, val, reg; - int ret, sample_size, srate, i2sclock, bitcnt, audio_bits; + int ret, sample_size, srate, i2sclock, bitcnt, audio_bits, channels; struct tegra30_ahub_cif_conf cif_conf;
- if (params_channels(params) != 2) + channels = params_channels(params); + if (channels > 8) + return -EINVAL; + if (channels != 2 && !i2s->is_tdm) return -EINVAL;
mask = TEGRA30_I2S_CTRL_BIT_SIZE_MASK; @@ -157,9 +163,8 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream, regmap_update_bits(i2s->regmap, TEGRA30_I2S_CTRL, mask, val);
srate = params_rate(params); - /* Final "* 2" required by Tegra hardware */ - i2sclock = srate * params_channels(params) * sample_size * 2; + i2sclock = srate * channels * sample_size * 2;
bitcnt = (i2sclock / (2 * srate)) - 1; if (bitcnt < 0 || bitcnt > TEGRA30_I2S_TIMING_CHANNEL_BIT_COUNT_MASK_US) @@ -179,8 +184,8 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream, regmap_write(i2s->regmap, TEGRA30_I2S_TIMING, val);
cif_conf.threshold = 0; - cif_conf.audio_channels = 2; - cif_conf.client_channels = 2; + cif_conf.audio_channels = channels; + cif_conf.client_channels = channels; cif_conf.audio_bits = audio_bits; cif_conf.client_bits = audio_bits; cif_conf.expand = 0; @@ -319,7 +324,7 @@ static const struct snd_soc_dai_driver tegra30_i2s_dai_template = { .playback = { .stream_name = "Playback", .channels_min = 2, - .channels_max = 2, + .channels_max = 8, .rates = SNDRV_PCM_RATE_8000_96000, .formats = SNDRV_PCM_FMTBIT_S32_LE | SNDRV_PCM_FMTBIT_S24_LE | @@ -328,7 +333,7 @@ static const struct snd_soc_dai_driver tegra30_i2s_dai_template = { .capture = { .stream_name = "Capture", .channels_min = 2, - .channels_max = 2, + .channels_max = 8, .rates = SNDRV_PCM_RATE_8000_96000, .formats = SNDRV_PCM_FMTBIT_S32_LE | SNDRV_PCM_FMTBIT_S24_LE | diff --git a/sound/soc/tegra/tegra30_i2s.h b/sound/soc/tegra/tegra30_i2s.h index 0b1f3125a7c0..ae30e3c96337 100644 --- a/sound/soc/tegra/tegra30_i2s.h +++ b/sound/soc/tegra/tegra30_i2s.h @@ -224,6 +224,7 @@ struct tegra30_i2s { const struct tegra30_i2s_soc_data *soc_data; struct snd_soc_dai_driver dai; int cif_id; + bool is_tdm; struct clk *clk_i2s; enum tegra30_ahub_txcif capture_i2s_cif; enum tegra30_ahub_rxcif capture_fifo_cif;
On 17/09/2019 19:12, Ben Dooks wrote:
From: Edward Cragg edward.cragg@codethink.co.uk
The CIF configuration and clock setting is currently hard coded for 2 channels. Since the hardware is capable of supporting 1-8 channels add support for reading the channel count from the supplied parameters to allow for better TDM support. It seems the original implementation of this driver was fixed at 2 channels for simplicity, and not implementing TDM.
Signed-off-by: Edward Cragg edward.cragg@codethink.co.uk [ben.dooks@codethink.co.uk: added is_tdm and channel nr check]
sound/soc/tegra/tegra30_i2s.c | 21 +++++++++++++-------- sound/soc/tegra/tegra30_i2s.h | 1 + 2 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c index b5372839f672..40bcc05a9dbb 100644 --- a/sound/soc/tegra/tegra30_i2s.c +++ b/sound/soc/tegra/tegra30_i2s.c @@ -86,14 +86,17 @@ static int tegra30_i2s_set_fmt(struct snd_soc_dai *dai, return -EINVAL; }
- i2s->is_tdm = false; mask |= TEGRA30_I2S_CTRL_FRAME_FORMAT_MASK | TEGRA30_I2S_CTRL_LRCK_MASK; switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) { case SND_SOC_DAIFMT_DSP_A:
i2s->is_tdm = true;i2s->is_tdm = true;@@ -127,10 +130,13 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream, struct device *dev = dai->dev; struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai); unsigned int mask, val, reg;
- int ret, sample_size, srate, i2sclock, bitcnt, audio_bits;
- int ret, sample_size, srate, i2sclock, bitcnt, audio_bits, channels; struct tegra30_ahub_cif_conf cif_conf;
- if (params_channels(params) != 2)
- channels = params_channels(params);
- if (channels > 8)
return -EINVAL;- if (channels != 2 && !i2s->is_tdm)
I don't think that this additional test is really necessary. I would just drop this 'is_tdm' variable.
Cheers Jon
On 2019-09-18 09:50, Jon Hunter wrote:
On 17/09/2019 19:12, Ben Dooks wrote:
From: Edward Cragg edward.cragg@codethink.co.uk
The CIF configuration and clock setting is currently hard coded for 2 channels. Since the hardware is capable of supporting 1-8 channels add support for reading the channel count from the supplied parameters to allow for better TDM support. It seems the original implementation of this driver was fixed at 2 channels for simplicity, and not implementing TDM.
Signed-off-by: Edward Cragg edward.cragg@codethink.co.uk [ben.dooks@codethink.co.uk: added is_tdm and channel nr check]
sound/soc/tegra/tegra30_i2s.c | 21 +++++++++++++-------- sound/soc/tegra/tegra30_i2s.h | 1 + 2 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c index b5372839f672..40bcc05a9dbb 100644 --- a/sound/soc/tegra/tegra30_i2s.c +++ b/sound/soc/tegra/tegra30_i2s.c @@ -86,14 +86,17 @@ static int tegra30_i2s_set_fmt(struct snd_soc_dai *dai, return -EINVAL; }
- i2s->is_tdm = false; mask |= TEGRA30_I2S_CTRL_FRAME_FORMAT_MASK | TEGRA30_I2S_CTRL_LRCK_MASK; switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) { case SND_SOC_DAIFMT_DSP_A:
i2s->is_tdm = true;i2s->is_tdm = true;@@ -127,10 +130,13 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream, struct device *dev = dai->dev; struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai); unsigned int mask, val, reg;
- int ret, sample_size, srate, i2sclock, bitcnt, audio_bits;
- int ret, sample_size, srate, i2sclock, bitcnt, audio_bits, channels; struct tegra30_ahub_cif_conf cif_conf;
- if (params_channels(params) != 2)
- channels = params_channels(params);
- if (channels > 8)
return -EINVAL;- if (channels != 2 && !i2s->is_tdm)
I don't think that this additional test is really necessary. I would just drop this 'is_tdm' variable.
I needed it elsewhere so would prefer to leave this here.
We see odd FIFO overruns with this, we assume the best thing to do is to disable the RX I2S frontend first, and then disable the FIFO that is using it.
This also fixes an issue where using multi-word frames (TDM) have partial samples stuck in the FIFO which then get read out when the next capture is started.
Signed-off-by: Ben Dooks ben.dooks@codethink.co.uk --- sound/soc/tegra/tegra30_i2s.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c index 40bcc05a9dbb..4222839b63bd 100644 --- a/sound/soc/tegra/tegra30_i2s.c +++ b/sound/soc/tegra/tegra30_i2s.c @@ -236,9 +236,9 @@ static void tegra30_i2s_start_capture(struct tegra30_i2s *i2s)
static void tegra30_i2s_stop_capture(struct tegra30_i2s *i2s) { - tegra30_ahub_disable_rx_fifo(i2s->capture_fifo_cif); regmap_update_bits(i2s->regmap, TEGRA30_I2S_CTRL, TEGRA30_I2S_CTRL_XFER_EN_RX, 0); + tegra30_ahub_disable_rx_fifo(i2s->capture_fifo_cif); }
static int tegra30_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
In TDM, use the negative edge to drive data and the positive edge to sample data.
Signed-off-by: Ben Dooks ben.dooks@codethink.co.uk --- sound/soc/tegra/tegra30_i2s.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c index 4222839b63bd..d75ce12fe177 100644 --- a/sound/soc/tegra/tegra30_i2s.c +++ b/sound/soc/tegra/tegra30_i2s.c @@ -117,6 +117,8 @@ static int tegra30_i2s_set_fmt(struct snd_soc_dai *dai, }
pm_runtime_get_sync(dai->dev); + regmap_update_bits(i2s->regmap, TEGRA30_I2S_CH_CTRL_EGDE_CTRL_MASK, + i2s->is_tdm ? TEGRA30_I2S_CH_CTRL_EGDE_CTRL_NEG_EDGE : 0); regmap_update_bits(i2s->regmap, TEGRA30_I2S_CTRL, mask, val); pm_runtime_put(dai->dev);
Hi Ben,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master] [cannot apply to v5.3 next-20190916] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Ben-Dooks/ASoC-tegra-Add-a-TDM-conf... config: mips-allmodconfig (attached as .config) compiler: mips-linux-gcc (GCC) 7.4.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=7.4.0 make.cross ARCH=mips
If you fix the issue, kindly add following tag Reported-by: kbuild test robot lkp@intel.com
All errors (new ones prefixed by >>):
sound/soc/tegra/tegra30_i2s.c: In function 'tegra30_i2s_set_fmt':
sound/soc/tegra/tegra30_i2s.c:121:63: error: macro "regmap_update_bits" requires 4 arguments, but only 3 given
i2s->is_tdm ? TEGRA30_I2S_CH_CTRL_EGDE_CTRL_NEG_EDGE : 0); ^
sound/soc/tegra/tegra30_i2s.c:120:2: error: 'regmap_update_bits' undeclared (first use in this function); did you mean 'regmap_update_bits_base'?
regmap_update_bits(i2s->regmap, TEGRA30_I2S_CH_CTRL_EGDE_CTRL_MASK, ^~~~~~~~~~~~~~~~~~ regmap_update_bits_base sound/soc/tegra/tegra30_i2s.c:120:2: note: each undeclared identifier is reported only once for each function it appears in
vim +/regmap_update_bits +121 sound/soc/tegra/tegra30_i2s.c
64 65 static int tegra30_i2s_set_fmt(struct snd_soc_dai *dai, 66 unsigned int fmt) 67 { 68 struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai); 69 unsigned int mask = 0, val = 0; 70 71 switch (fmt & SND_SOC_DAIFMT_INV_MASK) { 72 case SND_SOC_DAIFMT_NB_NF: 73 break; 74 default: 75 return -EINVAL; 76 } 77 78 mask |= TEGRA30_I2S_CTRL_MASTER_ENABLE; 79 switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) { 80 case SND_SOC_DAIFMT_CBS_CFS: 81 val |= TEGRA30_I2S_CTRL_MASTER_ENABLE; 82 break; 83 case SND_SOC_DAIFMT_CBM_CFM: 84 break; 85 default: 86 return -EINVAL; 87 } 88 89 i2s->is_tdm = false; 90 mask |= TEGRA30_I2S_CTRL_FRAME_FORMAT_MASK | 91 TEGRA30_I2S_CTRL_LRCK_MASK; 92 switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) { 93 case SND_SOC_DAIFMT_DSP_A: 94 i2s->is_tdm = true; 95 val |= TEGRA30_I2S_CTRL_FRAME_FORMAT_FSYNC; 96 val |= TEGRA30_I2S_CTRL_LRCK_L_LOW; 97 break; 98 case SND_SOC_DAIFMT_DSP_B: 99 i2s->is_tdm = true; 100 val |= TEGRA30_I2S_CTRL_FRAME_FORMAT_FSYNC; 101 val |= TEGRA30_I2S_CTRL_LRCK_R_LOW; 102 break; 103 case SND_SOC_DAIFMT_I2S: 104 val |= TEGRA30_I2S_CTRL_FRAME_FORMAT_LRCK; 105 val |= TEGRA30_I2S_CTRL_LRCK_L_LOW; 106 break; 107 case SND_SOC_DAIFMT_RIGHT_J: 108 val |= TEGRA30_I2S_CTRL_FRAME_FORMAT_LRCK; 109 val |= TEGRA30_I2S_CTRL_LRCK_L_LOW; 110 break; 111 case SND_SOC_DAIFMT_LEFT_J: 112 val |= TEGRA30_I2S_CTRL_FRAME_FORMAT_LRCK; 113 val |= TEGRA30_I2S_CTRL_LRCK_L_LOW; 114 break; 115 default: 116 return -EINVAL; 117 } 118 119 pm_runtime_get_sync(dai->dev);
120 regmap_update_bits(i2s->regmap, TEGRA30_I2S_CH_CTRL_EGDE_CTRL_MASK, 121 i2s->is_tdm ? TEGRA30_I2S_CH_CTRL_EGDE_CTRL_NEG_EDGE : 0);
122 regmap_update_bits(i2s->regmap, TEGRA30_I2S_CTRL, mask, val); 123 pm_runtime_put(dai->dev); 124 125 return 0; 126 } 127
--- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 17/09/2019 19:12, Ben Dooks wrote:
In TDM, use the negative edge to drive data and the positive edge to sample data.
Signed-off-by: Ben Dooks ben.dooks@codethink.co.uk
sound/soc/tegra/tegra30_i2s.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c index 4222839b63bd..d75ce12fe177 100644 --- a/sound/soc/tegra/tegra30_i2s.c +++ b/sound/soc/tegra/tegra30_i2s.c @@ -117,6 +117,8 @@ static int tegra30_i2s_set_fmt(struct snd_soc_dai *dai, }
pm_runtime_get_sync(dai->dev);
- regmap_update_bits(i2s->regmap, TEGRA30_I2S_CH_CTRL_EGDE_CTRL_MASK,
i2s->is_tdm ? TEGRA30_I2S_CH_CTRL_EGDE_CTRL_NEG_EDGE : 0);
I would rather set this in the case statement above where the format is parsed and again drop this 'is_tdm' variable.
Jon
On 18/09/2019 09:54, Jon Hunter wrote:
On 17/09/2019 19:12, Ben Dooks wrote:
In TDM, use the negative edge to drive data and the positive edge to sample data.
Signed-off-by: Ben Dooks ben.dooks@codethink.co.uk
sound/soc/tegra/tegra30_i2s.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c index 4222839b63bd..d75ce12fe177 100644 --- a/sound/soc/tegra/tegra30_i2s.c +++ b/sound/soc/tegra/tegra30_i2s.c @@ -117,6 +117,8 @@ static int tegra30_i2s_set_fmt(struct snd_soc_dai *dai, }
pm_runtime_get_sync(dai->dev);
- regmap_update_bits(i2s->regmap, TEGRA30_I2S_CH_CTRL_EGDE_CTRL_MASK,
i2s->is_tdm ? TEGRA30_I2S_CH_CTRL_EGDE_CTRL_NEG_EDGE : 0);I would rather set this in the case statement above where the format is parsed and again drop this 'is_tdm' variable.
Actually, this should be implemented as shown in the following ...
https://nv-tegra.nvidia.com/gitweb/?p=linux-nvidia.git;a=blob;f=sound/soc/te...
Jon
On 2019-09-18 10:02, Jon Hunter wrote:
On 18/09/2019 09:54, Jon Hunter wrote:
On 17/09/2019 19:12, Ben Dooks wrote:
In TDM, use the negative edge to drive data and the positive edge to sample data.
Signed-off-by: Ben Dooks ben.dooks@codethink.co.uk
sound/soc/tegra/tegra30_i2s.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c index 4222839b63bd..d75ce12fe177 100644 --- a/sound/soc/tegra/tegra30_i2s.c +++ b/sound/soc/tegra/tegra30_i2s.c @@ -117,6 +117,8 @@ static int tegra30_i2s_set_fmt(struct snd_soc_dai *dai, }
pm_runtime_get_sync(dai->dev);
- regmap_update_bits(i2s->regmap, TEGRA30_I2S_CH_CTRL_EGDE_CTRL_MASK,
i2s->is_tdm ? TEGRA30_I2S_CH_CTRL_EGDE_CTRL_NEG_EDGE : 0);I would rather set this in the case statement above where the format is parsed and again drop this 'is_tdm' variable.
Actually, this should be implemented as shown in the following ...
https://nv-tegra.nvidia.com/gitweb/?p=linux-nvidia.git;a=blob;f=sound/soc/te...
Jon
Ok, will look at that.
Note, nv-tegra.nvidia.com seems to have a security problem .
Set the offset to 0 for TDM mode, as per the current setup.
Signed-off-by: Ben Dooks ben.dooks@codethink.co.uk --- sound/soc/tegra/tegra30_i2s.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c index d75ce12fe177..3efef87ed8d8 100644 --- a/sound/soc/tegra/tegra30_i2s.c +++ b/sound/soc/tegra/tegra30_i2s.c @@ -206,8 +206,11 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream,
i2s->soc_data->set_audio_cif(i2s->regmap, reg, &cif_conf);
- val = (1 << TEGRA30_I2S_OFFSET_RX_DATA_OFFSET_SHIFT) | - (1 << TEGRA30_I2S_OFFSET_TX_DATA_OFFSET_SHIFT); + if (i2s->is_tdm) + val = 0; + else + val = (1 << TEGRA30_I2S_OFFSET_RX_DATA_OFFSET_SHIFT) | + (1 << TEGRA30_I2S_OFFSET_TX_DATA_OFFSET_SHIFT); regmap_write(i2s->regmap, TEGRA30_I2S_OFFSET, val);
return 0;
On 17/09/2019 19:12, Ben Dooks wrote:
Set the offset to 0 for TDM mode, as per the current setup.
Signed-off-by: Ben Dooks ben.dooks@codethink.co.uk
sound/soc/tegra/tegra30_i2s.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c index d75ce12fe177..3efef87ed8d8 100644 --- a/sound/soc/tegra/tegra30_i2s.c +++ b/sound/soc/tegra/tegra30_i2s.c @@ -206,8 +206,11 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream,
i2s->soc_data->set_audio_cif(i2s->regmap, reg, &cif_conf);
- val = (1 << TEGRA30_I2S_OFFSET_RX_DATA_OFFSET_SHIFT) |
(1 << TEGRA30_I2S_OFFSET_TX_DATA_OFFSET_SHIFT);
if (i2s->is_tdm)
val = 0;else
val = (1 << TEGRA30_I2S_OFFSET_RX_DATA_OFFSET_SHIFT) |(1 << TEGRA30_I2S_OFFSET_TX_DATA_OFFSET_SHIFT);regmap_write(i2s->regmap, TEGRA30_I2S_OFFSET, val);
return 0;
Please move this code into tegra30_i2s_set_fmt() as it only needs to be set once.
BTW, if you refer to the following I2S driver for Tegra210, you will see how I think that we should handle this ...
https://nv-tegra.nvidia.com/gitweb/?p=linux-nvidia.git;a=blob;f=sound/soc/te...
Cheers Jon
On 2019-09-18 10:02, Jon Hunter wrote:
On 17/09/2019 19:12, Ben Dooks wrote:
Set the offset to 0 for TDM mode, as per the current setup.
Signed-off-by: Ben Dooks ben.dooks@codethink.co.uk
sound/soc/tegra/tegra30_i2s.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c index d75ce12fe177..3efef87ed8d8 100644 --- a/sound/soc/tegra/tegra30_i2s.c +++ b/sound/soc/tegra/tegra30_i2s.c @@ -206,8 +206,11 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream,
i2s->soc_data->set_audio_cif(i2s->regmap, reg, &cif_conf);
- val = (1 << TEGRA30_I2S_OFFSET_RX_DATA_OFFSET_SHIFT) |
(1 << TEGRA30_I2S_OFFSET_TX_DATA_OFFSET_SHIFT);
if (i2s->is_tdm)
val = 0;else
val = (1 << TEGRA30_I2S_OFFSET_RX_DATA_OFFSET_SHIFT) |(1 << TEGRA30_I2S_OFFSET_TX_DATA_OFFSET_SHIFT);regmap_write(i2s->regmap, TEGRA30_I2S_OFFSET, val);
return 0;
Please move this code into tegra30_i2s_set_fmt() as it only needs to be set once.
BTW, if you refer to the following I2S driver for Tegra210, you will see how I think that we should handle this ...
Ok, thanks.
If the hw_params uses a different bit or channel count, then we need to change both the I2S unit's CIF configuration as well as the APBIF one.
To allow changing the APBIF, add a call to reconfigure the RX or TX FIFO without changing the DMA or allocation, and get the I2S driver to call it once the hw params have been calculate.
Signed-off-by: Ben Dooks ben.dooks@codethink.co.uk --- sound/soc/tegra/tegra30_ahub.c | 114 ++++++++++++++++++--------------- sound/soc/tegra/tegra30_ahub.h | 5 ++ sound/soc/tegra/tegra30_i2s.c | 2 + 3 files changed, 69 insertions(+), 52 deletions(-)
diff --git a/sound/soc/tegra/tegra30_ahub.c b/sound/soc/tegra/tegra30_ahub.c index 952381260dc3..58e05ceb86da 100644 --- a/sound/soc/tegra/tegra30_ahub.c +++ b/sound/soc/tegra/tegra30_ahub.c @@ -84,12 +84,40 @@ static int tegra30_ahub_runtime_resume(struct device *dev) return 0; }
+int tegra30_ahub_setup_rx_fifo(enum tegra30_ahub_rxcif rxcif, + struct tegra30_ahub_cif_conf *cif_conf) +{ + int channel = rxcif - TEGRA30_AHUB_RXCIF_APBIF_RX0; + u32 reg, val; + + pm_runtime_get_sync(ahub->dev); + + reg = TEGRA30_AHUB_CHANNEL_CTRL + + (channel * TEGRA30_AHUB_CHANNEL_CTRL_STRIDE); + val = tegra30_apbif_read(reg); + val &= ~(TEGRA30_AHUB_CHANNEL_CTRL_RX_THRESHOLD_MASK | + TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_MASK); + val |= (7 << TEGRA30_AHUB_CHANNEL_CTRL_RX_THRESHOLD_SHIFT) | + TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_EN | + TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_16; + tegra30_apbif_write(reg, val); + + cif_conf->direction = TEGRA30_AUDIOCIF_DIRECTION_RX; + + reg = TEGRA30_AHUB_CIF_RX_CTRL + + (channel * TEGRA30_AHUB_CIF_RX_CTRL_STRIDE); + ahub->soc_data->set_audio_cif(ahub->regmap_apbif, reg, cif_conf); + + pm_runtime_put(ahub->dev); + return 0; +} +EXPORT_SYMBOL_GPL(tegra30_ahub_setup_rx_fifo); + int tegra30_ahub_allocate_rx_fifo(enum tegra30_ahub_rxcif *rxcif, char *dmachan, int dmachan_len, dma_addr_t *fiforeg) { int channel; - u32 reg, val; struct tegra30_ahub_cif_conf cif_conf;
channel = find_first_zero_bit(ahub->rx_usage, @@ -104,37 +132,14 @@ int tegra30_ahub_allocate_rx_fifo(enum tegra30_ahub_rxcif *rxcif, *fiforeg = ahub->apbif_addr + TEGRA30_AHUB_CHANNEL_RXFIFO + (channel * TEGRA30_AHUB_CHANNEL_RXFIFO_STRIDE);
- pm_runtime_get_sync(ahub->dev); + memset(&cif_conf, 0, sizeof(cif_conf));
- reg = TEGRA30_AHUB_CHANNEL_CTRL + - (channel * TEGRA30_AHUB_CHANNEL_CTRL_STRIDE); - val = tegra30_apbif_read(reg); - val &= ~(TEGRA30_AHUB_CHANNEL_CTRL_RX_THRESHOLD_MASK | - TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_MASK); - val |= (7 << TEGRA30_AHUB_CHANNEL_CTRL_RX_THRESHOLD_SHIFT) | - TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_EN | - TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_16; - tegra30_apbif_write(reg, val); - - cif_conf.threshold = 0; cif_conf.audio_channels = 2; cif_conf.client_channels = 2; cif_conf.audio_bits = TEGRA30_AUDIOCIF_BITS_16; cif_conf.client_bits = TEGRA30_AUDIOCIF_BITS_16; - cif_conf.expand = 0; - cif_conf.stereo_conv = 0; - cif_conf.replicate = 0; - cif_conf.direction = TEGRA30_AUDIOCIF_DIRECTION_RX; - cif_conf.truncate = 0; - cif_conf.mono_conv = 0; - - reg = TEGRA30_AHUB_CIF_RX_CTRL + - (channel * TEGRA30_AHUB_CIF_RX_CTRL_STRIDE); - ahub->soc_data->set_audio_cif(ahub->regmap_apbif, reg, &cif_conf); - - pm_runtime_put(ahub->dev);
- return 0; + return tegra30_ahub_setup_rx_fifo(*rxcif, &cif_conf); } EXPORT_SYMBOL_GPL(tegra30_ahub_allocate_rx_fifo);
@@ -186,12 +191,40 @@ int tegra30_ahub_free_rx_fifo(enum tegra30_ahub_rxcif rxcif) } EXPORT_SYMBOL_GPL(tegra30_ahub_free_rx_fifo);
+int tegra30_ahub_setup_tx_fifo(enum tegra30_ahub_txcif txcif, + struct tegra30_ahub_cif_conf *cif_conf) +{ + int channel = txcif - TEGRA30_AHUB_TXCIF_APBIF_TX0; + u32 reg, val; + + pm_runtime_get_sync(ahub->dev); + + reg = TEGRA30_AHUB_CHANNEL_CTRL + + (channel * TEGRA30_AHUB_CHANNEL_CTRL_STRIDE); + val = tegra30_apbif_read(reg); + val &= ~(TEGRA30_AHUB_CHANNEL_CTRL_TX_THRESHOLD_MASK | + TEGRA30_AHUB_CHANNEL_CTRL_TX_PACK_MASK); + val |= (7 << TEGRA30_AHUB_CHANNEL_CTRL_TX_THRESHOLD_SHIFT) | + TEGRA30_AHUB_CHANNEL_CTRL_TX_PACK_EN | + TEGRA30_AHUB_CHANNEL_CTRL_TX_PACK_16; + tegra30_apbif_write(reg, val); + + cif_conf->direction = TEGRA30_AUDIOCIF_DIRECTION_TX; + + reg = TEGRA30_AHUB_CIF_TX_CTRL + + (channel * TEGRA30_AHUB_CIF_TX_CTRL_STRIDE); + ahub->soc_data->set_audio_cif(ahub->regmap_apbif, reg, cif_conf); + + pm_runtime_put(ahub->dev); + return 0; +} +EXPORT_SYMBOL_GPL(tegra30_ahub_setup_tx_fifo); + int tegra30_ahub_allocate_tx_fifo(enum tegra30_ahub_txcif *txcif, char *dmachan, int dmachan_len, dma_addr_t *fiforeg) { int channel; - u32 reg, val; struct tegra30_ahub_cif_conf cif_conf;
channel = find_first_zero_bit(ahub->tx_usage, @@ -206,37 +239,14 @@ int tegra30_ahub_allocate_tx_fifo(enum tegra30_ahub_txcif *txcif, *fiforeg = ahub->apbif_addr + TEGRA30_AHUB_CHANNEL_TXFIFO + (channel * TEGRA30_AHUB_CHANNEL_TXFIFO_STRIDE);
- pm_runtime_get_sync(ahub->dev); - - reg = TEGRA30_AHUB_CHANNEL_CTRL + - (channel * TEGRA30_AHUB_CHANNEL_CTRL_STRIDE); - val = tegra30_apbif_read(reg); - val &= ~(TEGRA30_AHUB_CHANNEL_CTRL_TX_THRESHOLD_MASK | - TEGRA30_AHUB_CHANNEL_CTRL_TX_PACK_MASK); - val |= (7 << TEGRA30_AHUB_CHANNEL_CTRL_TX_THRESHOLD_SHIFT) | - TEGRA30_AHUB_CHANNEL_CTRL_TX_PACK_EN | - TEGRA30_AHUB_CHANNEL_CTRL_TX_PACK_16; - tegra30_apbif_write(reg, val); - - cif_conf.threshold = 0; + memset(&cif_conf, 0, sizeof(cif_conf)); cif_conf.audio_channels = 2; cif_conf.client_channels = 2; cif_conf.audio_bits = TEGRA30_AUDIOCIF_BITS_16; cif_conf.client_bits = TEGRA30_AUDIOCIF_BITS_16; - cif_conf.expand = 0; - cif_conf.stereo_conv = 0; - cif_conf.replicate = 0; cif_conf.direction = TEGRA30_AUDIOCIF_DIRECTION_TX; - cif_conf.truncate = 0; - cif_conf.mono_conv = 0; - - reg = TEGRA30_AHUB_CIF_TX_CTRL + - (channel * TEGRA30_AHUB_CIF_TX_CTRL_STRIDE); - ahub->soc_data->set_audio_cif(ahub->regmap_apbif, reg, &cif_conf);
- pm_runtime_put(ahub->dev); - - return 0; + return tegra30_ahub_setup_tx_fifo(*txcif, &cif_conf); } EXPORT_SYMBOL_GPL(tegra30_ahub_allocate_tx_fifo);
diff --git a/sound/soc/tegra/tegra30_ahub.h b/sound/soc/tegra/tegra30_ahub.h index 6889c5f23d02..26120aee64b3 100644 --- a/sound/soc/tegra/tegra30_ahub.h +++ b/sound/soc/tegra/tegra30_ahub.h @@ -490,6 +490,11 @@ void tegra30_ahub_set_cif(struct regmap *regmap, unsigned int reg, void tegra124_ahub_set_cif(struct regmap *regmap, unsigned int reg, struct tegra30_ahub_cif_conf *conf);
+extern int tegra30_ahub_setup_tx_fifo(enum tegra30_ahub_txcif txcif, + struct tegra30_ahub_cif_conf *cif_conf); +extern int tegra30_ahub_setup_rx_fifo(enum tegra30_ahub_rxcif, + struct tegra30_ahub_cif_conf *cif_conf); + struct tegra30_ahub_soc_data { u32 mod_list_mask; void (*set_audio_cif)(struct regmap *regmap, diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c index 3efef87ed8d8..805e0f4da52a 100644 --- a/sound/soc/tegra/tegra30_i2s.c +++ b/sound/soc/tegra/tegra30_i2s.c @@ -198,9 +198,11 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream,
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { cif_conf.direction = TEGRA30_AUDIOCIF_DIRECTION_RX; + tegra30_ahub_setup_tx_fifo(i2s->playback_fifo_cif, &cif_conf); reg = TEGRA30_I2S_CIF_RX_CTRL; } else { cif_conf.direction = TEGRA30_AUDIOCIF_DIRECTION_TX; + tegra30_ahub_setup_rx_fifo(i2s->capture_fifo_cif, &cif_conf); reg = TEGRA30_I2S_CIF_TX_CTRL; }
On 17/09/2019 19:12, Ben Dooks wrote:
If the hw_params uses a different bit or channel count, then we need to change both the I2S unit's CIF configuration as well as the APBIF one.
To allow changing the APBIF, add a call to reconfigure the RX or TX FIFO without changing the DMA or allocation, and get the I2S driver to call it once the hw params have been calculate.
Signed-off-by: Ben Dooks ben.dooks@codethink.co.uk
sound/soc/tegra/tegra30_ahub.c | 114 ++++++++++++++++++--------------- sound/soc/tegra/tegra30_ahub.h | 5 ++ sound/soc/tegra/tegra30_i2s.c | 2 + 3 files changed, 69 insertions(+), 52 deletions(-)
diff --git a/sound/soc/tegra/tegra30_ahub.c b/sound/soc/tegra/tegra30_ahub.c index 952381260dc3..58e05ceb86da 100644 --- a/sound/soc/tegra/tegra30_ahub.c +++ b/sound/soc/tegra/tegra30_ahub.c @@ -84,12 +84,40 @@ static int tegra30_ahub_runtime_resume(struct device *dev) return 0; }
+int tegra30_ahub_setup_rx_fifo(enum tegra30_ahub_rxcif rxcif,
struct tegra30_ahub_cif_conf *cif_conf)+{
- int channel = rxcif - TEGRA30_AHUB_RXCIF_APBIF_RX0;
- u32 reg, val;
- pm_runtime_get_sync(ahub->dev);
- reg = TEGRA30_AHUB_CHANNEL_CTRL +
(channel * TEGRA30_AHUB_CHANNEL_CTRL_STRIDE);- val = tegra30_apbif_read(reg);
- val &= ~(TEGRA30_AHUB_CHANNEL_CTRL_RX_THRESHOLD_MASK |
TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_MASK);- val |= (7 << TEGRA30_AHUB_CHANNEL_CTRL_RX_THRESHOLD_SHIFT) |
TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_EN |TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_16;- tegra30_apbif_write(reg, val);
Aren't you just programming the same value to the APBIF here that was previously programmed by the allocate function? I don't see the point in moving this? What am I missing here?
Cheers Jon
On 2019-09-18 10:14, Jon Hunter wrote:
On 17/09/2019 19:12, Ben Dooks wrote:
If the hw_params uses a different bit or channel count, then we need to change both the I2S unit's CIF configuration as well as the APBIF one.
To allow changing the APBIF, add a call to reconfigure the RX or TX FIFO without changing the DMA or allocation, and get the I2S driver to call it once the hw params have been calculate.
Signed-off-by: Ben Dooks ben.dooks@codethink.co.uk
sound/soc/tegra/tegra30_ahub.c | 114 ++++++++++++++++++--------------- sound/soc/tegra/tegra30_ahub.h | 5 ++ sound/soc/tegra/tegra30_i2s.c | 2 + 3 files changed, 69 insertions(+), 52 deletions(-)
diff --git a/sound/soc/tegra/tegra30_ahub.c b/sound/soc/tegra/tegra30_ahub.c index 952381260dc3..58e05ceb86da 100644 --- a/sound/soc/tegra/tegra30_ahub.c +++ b/sound/soc/tegra/tegra30_ahub.c @@ -84,12 +84,40 @@ static int tegra30_ahub_runtime_resume(struct device *dev) return 0; }
+int tegra30_ahub_setup_rx_fifo(enum tegra30_ahub_rxcif rxcif,
struct tegra30_ahub_cif_conf *cif_conf)+{
- int channel = rxcif - TEGRA30_AHUB_RXCIF_APBIF_RX0;
- u32 reg, val;
- pm_runtime_get_sync(ahub->dev);
- reg = TEGRA30_AHUB_CHANNEL_CTRL +
(channel * TEGRA30_AHUB_CHANNEL_CTRL_STRIDE);- val = tegra30_apbif_read(reg);
- val &= ~(TEGRA30_AHUB_CHANNEL_CTRL_RX_THRESHOLD_MASK |
TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_MASK);- val |= (7 << TEGRA30_AHUB_CHANNEL_CTRL_RX_THRESHOLD_SHIFT) |
TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_EN |TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_16;- tegra30_apbif_write(reg, val);
Aren't you just programming the same value to the APBIF here that was previously programmed by the allocate function? I don't see the point in moving this? What am I missing here?
IIRC, this was due to trying different types of playback with a 4ch 32bit sound output. The ahub and the i2s unit don't have to agreet.
If the CIF is not configured as 16 or 8 bit, then the packing for 8/16 bits should not be enabled as the hardware only supports 8 or 16 bit packing.
Signed-off-by: Ben Dooks ben.dooks@codethink.co.uk --- sound/soc/tegra/tegra30_ahub.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/sound/soc/tegra/tegra30_ahub.c b/sound/soc/tegra/tegra30_ahub.c index 58e05ceb86da..c2f2e29dd32e 100644 --- a/sound/soc/tegra/tegra30_ahub.c +++ b/sound/soc/tegra/tegra30_ahub.c @@ -96,10 +96,17 @@ int tegra30_ahub_setup_rx_fifo(enum tegra30_ahub_rxcif rxcif, (channel * TEGRA30_AHUB_CHANNEL_CTRL_STRIDE); val = tegra30_apbif_read(reg); val &= ~(TEGRA30_AHUB_CHANNEL_CTRL_RX_THRESHOLD_MASK | - TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_MASK); - val |= (7 << TEGRA30_AHUB_CHANNEL_CTRL_RX_THRESHOLD_SHIFT) | - TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_EN | - TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_16; + TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_MASK | + TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_EN); + val |= (7 << TEGRA30_AHUB_CHANNEL_CTRL_RX_THRESHOLD_SHIFT); + if (cif_conf->audio_bits == TEGRA30_AUDIOCIF_BITS_16 || + cif_conf->audio_bits == TEGRA30_AUDIOCIF_BITS_8) + val |= TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_EN; + if (cif_conf->audio_bits == TEGRA30_AUDIOCIF_BITS_16) + val |= TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_16; + if (cif_conf->audio_bits == TEGRA30_AUDIOCIF_BITS_8) + val |= TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_8_4; + tegra30_apbif_write(reg, val);
cif_conf->direction = TEGRA30_AUDIOCIF_DIRECTION_RX; @@ -203,10 +210,16 @@ int tegra30_ahub_setup_tx_fifo(enum tegra30_ahub_txcif txcif, (channel * TEGRA30_AHUB_CHANNEL_CTRL_STRIDE); val = tegra30_apbif_read(reg); val &= ~(TEGRA30_AHUB_CHANNEL_CTRL_TX_THRESHOLD_MASK | - TEGRA30_AHUB_CHANNEL_CTRL_TX_PACK_MASK); - val |= (7 << TEGRA30_AHUB_CHANNEL_CTRL_TX_THRESHOLD_SHIFT) | - TEGRA30_AHUB_CHANNEL_CTRL_TX_PACK_EN | - TEGRA30_AHUB_CHANNEL_CTRL_TX_PACK_16; + TEGRA30_AHUB_CHANNEL_CTRL_TX_PACK_MASK | + TEGRA30_AHUB_CHANNEL_CTRL_TX_PACK_EN); + val |= (7 << TEGRA30_AHUB_CHANNEL_CTRL_TX_THRESHOLD_SHIFT); + if (cif_conf->audio_bits == TEGRA30_AUDIOCIF_BITS_16 || + cif_conf->audio_bits == TEGRA30_AUDIOCIF_BITS_8) + val |= TEGRA30_AHUB_CHANNEL_CTRL_TX_PACK_EN; + if (cif_conf->audio_bits == TEGRA30_AUDIOCIF_BITS_16) + val |= TEGRA30_AHUB_CHANNEL_CTRL_TX_PACK_16; + if (cif_conf->audio_bits == TEGRA30_AUDIOCIF_BITS_8) + val |= TEGRA30_AHUB_CHANNEL_CTRL_TX_PACK_8_4; tegra30_apbif_write(reg, val);
cif_conf->direction = TEGRA30_AUDIOCIF_DIRECTION_TX;
On 17/09/2019 19:12, Ben Dooks wrote:
If the CIF is not configured as 16 or 8 bit, then the packing for 8/16 bits should not be enabled as the hardware only supports 8 or 16 bit packing.
Signed-off-by: Ben Dooks ben.dooks@codethink.co.uk
sound/soc/tegra/tegra30_ahub.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/sound/soc/tegra/tegra30_ahub.c b/sound/soc/tegra/tegra30_ahub.c index 58e05ceb86da..c2f2e29dd32e 100644 --- a/sound/soc/tegra/tegra30_ahub.c +++ b/sound/soc/tegra/tegra30_ahub.c @@ -96,10 +96,17 @@ int tegra30_ahub_setup_rx_fifo(enum tegra30_ahub_rxcif rxcif, (channel * TEGRA30_AHUB_CHANNEL_CTRL_STRIDE); val = tegra30_apbif_read(reg); val &= ~(TEGRA30_AHUB_CHANNEL_CTRL_RX_THRESHOLD_MASK |
TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_MASK);- val |= (7 << TEGRA30_AHUB_CHANNEL_CTRL_RX_THRESHOLD_SHIFT) |
TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_EN |TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_16;
TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_MASK |TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_EN);- val |= (7 << TEGRA30_AHUB_CHANNEL_CTRL_RX_THRESHOLD_SHIFT);
- if (cif_conf->audio_bits == TEGRA30_AUDIOCIF_BITS_16 ||
cif_conf->audio_bits == TEGRA30_AUDIOCIF_BITS_8)val |= TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_EN;- if (cif_conf->audio_bits == TEGRA30_AUDIOCIF_BITS_16)
val |= TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_16;- if (cif_conf->audio_bits == TEGRA30_AUDIOCIF_BITS_8)
val |= TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_8_4;
Ah maybe this is what I am missing from the previous patch. So the last patch was a preparatory patch for this one.
Sameer, how is this handled in the case of Tegra210?
Cheers Jon
On 9/18/2019 2:46 PM, Jon Hunter wrote:
On 17/09/2019 19:12, Ben Dooks wrote:
If the CIF is not configured as 16 or 8 bit, then the packing for 8/16 bits should not be enabled as the hardware only supports 8 or 16 bit packing.
Signed-off-by: Ben Dooks ben.dooks@codethink.co.uk
sound/soc/tegra/tegra30_ahub.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/sound/soc/tegra/tegra30_ahub.c b/sound/soc/tegra/tegra30_ahub.c index 58e05ceb86da..c2f2e29dd32e 100644 --- a/sound/soc/tegra/tegra30_ahub.c +++ b/sound/soc/tegra/tegra30_ahub.c @@ -96,10 +96,17 @@ int tegra30_ahub_setup_rx_fifo(enum tegra30_ahub_rxcif rxcif, (channel * TEGRA30_AHUB_CHANNEL_CTRL_STRIDE); val = tegra30_apbif_read(reg); val &= ~(TEGRA30_AHUB_CHANNEL_CTRL_RX_THRESHOLD_MASK |
TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_MASK);- val |= (7 << TEGRA30_AHUB_CHANNEL_CTRL_RX_THRESHOLD_SHIFT) |
TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_EN |TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_16;
TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_MASK |TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_EN);- val |= (7 << TEGRA30_AHUB_CHANNEL_CTRL_RX_THRESHOLD_SHIFT);
- if (cif_conf->audio_bits == TEGRA30_AUDIOCIF_BITS_16 ||
cif_conf->audio_bits == TEGRA30_AUDIOCIF_BITS_8)val |= TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_EN;- if (cif_conf->audio_bits == TEGRA30_AUDIOCIF_BITS_16)
val |= TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_16;- if (cif_conf->audio_bits == TEGRA30_AUDIOCIF_BITS_8)
val |= TEGRA30_AHUB_CHANNEL_CTRL_RX_PACK_8_4;Ah maybe this is what I am missing from the previous patch. So the last patch was a preparatory patch for this one.
Sameer, how is this handled in the case of Tegra210?
For Tegra210 we have a separate driver for ADMAIF. Packing and CIF configuration is programmed in its hw_param() callback.
Cheers Jon
participants (6)
-
Ben Dooks -
Jon Hunter -
kbuild test robot -
Mark Brown -
Pierre-Louis Bossart -
Sameer Pujar