This patchset adds support for TDM audio on Tegra30 hardware.
It adds the DAI's `set_tdm_slot` callback and enables a tegra
pcm to have up to 8 channels.
It also includes support for other audio formats supported by
the Tegra30 HW and fixes a broken macro needed for setting the
TDM on the registers.
Based on Linux 4.18-rc3 tag.
Edward Cragg (4):
ASoC: tegra: i2s: Fix typo/broken macro
ASoC: tegra: Add a TDM configuration callback
ASoC: tegra: Allow 32-bit and 24-bit samples
ASoC: tegra: i2s: Add support for more than 2 channels
sound/soc/tegra/tegra30_i2s.c | 72 ++++++++++++++++++++++++++++++++++++-------
sound/soc/tegra/tegra30_i2s.h | 2 +-
2 files changed, 62 insertions(+), 12 deletions(-)
--
2.11.0
From: Edward Cragg <[email protected]>
Add a callback to configure TDM settings for the Tegra30
I2S ASoC 'platform' driver.
Signed-off-by: Ben Dooks <[email protected]>
Signed-off-by: Edward Cragg <[email protected]>
[[email protected]: Style fixes]
Signed-off-by: Jorge Sanjuan <[email protected]>
---
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 0b176ea24914..ff1996f215ed 100644
--- a/sound/soc/tegra/tegra30_i2s.c
+++ b/sound/soc/tegra/tegra30_i2s.c
@@ -265,6 +265,39 @@ static int tegra30_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
return 0;
}
+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_dbg(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);
+
+ pm_runtime_get_sync(dai->dev);
+ regmap_update_bits(i2s->regmap, TEGRA30_I2S_SLOT_CTRL, mask, val);
+
+ /* Set FSYNC width */
+ mask = TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_MASK;
+ val = (slot_width - 1) << TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_SHIFT;
+
+ regmap_update_bits(i2s->regmap, TEGRA30_I2S_CH_CTRL, mask, val);
+ pm_runtime_put(dai->dev);
+
+ return 0;
+}
+
static int tegra30_i2s_probe(struct snd_soc_dai *dai)
{
struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai);
@@ -279,6 +312,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 = {
--
2.11.0
From: Edward Cragg <[email protected]>
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 <[email protected]>
Signed-off-by: Jorge Sanjuan <[email protected]>
---
sound/soc/tegra/tegra30_i2s.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c
index e26c19ef7439..0f240d7989d0 100644
--- a/sound/soc/tegra/tegra30_i2s.c
+++ b/sound/soc/tegra/tegra30_i2s.c
@@ -138,16 +138,17 @@ 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, channels;
struct tegra30_ahub_cif_conf cif_conf;
- if (params_channels(params) != 2)
+ if (params_channels(params) > 8)
return -EINVAL;
mask = TEGRA30_I2S_CTRL_BIT_SIZE_MASK;
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:
@@ -157,6 +158,7 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream,
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:
@@ -166,9 +168,10 @@ 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);
+ channels = params_channels(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)
@@ -188,10 +191,10 @@ 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_bits = TEGRA30_AUDIOCIF_BITS_16;
- cif_conf.client_bits = TEGRA30_AUDIOCIF_BITS_16;
+ 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;
cif_conf.stereo_conv = 0;
cif_conf.replicate = 0;
@@ -329,7 +332,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 |
@@ -338,7 +341,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 |
--
2.11.0
From: Edward Cragg <[email protected]>
The tegra3 audio can support 32 and 24 bit sample sizes so add
the option to the tegra30_i2s_hw_params to configure the S32_LE/S24_LE
format when requested.
Signed-off-by: Ben Dooks <[email protected]>
Signed-off-by: Edward Cragg <[email protected]>
[[email protected]: Squashed multiple patches]
Signed-off-by: Jorge Sanjuan <[email protected]>
---
sound/soc/tegra/tegra30_i2s.c | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c
index ff1996f215ed..e26c19ef7439 100644
--- a/sound/soc/tegra/tegra30_i2s.c
+++ b/sound/soc/tegra/tegra30_i2s.c
@@ -150,6 +150,15 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream,
val = TEGRA30_I2S_CTRL_BIT_SIZE_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;
+ sample_size = 32;
+ break;
default:
return -EINVAL;
}
@@ -322,14 +331,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,
--
2.11.0
From: Edward Cragg <[email protected]>
Fix typo in macro TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK.
Signed-off-by: Edward Cragg <[email protected]>
Signed-off-by: Jorge Sanjuan <[email protected]>
---
sound/soc/tegra/tegra30_i2s.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/tegra/tegra30_i2s.h b/sound/soc/tegra/tegra30_i2s.h
index 774fc6ad2026..2e561e946de2 100644
--- a/sound/soc/tegra/tegra30_i2s.h
+++ b/sound/soc/tegra/tegra30_i2s.h
@@ -173,7 +173,7 @@
/* Number of slots in frame, minus 1 */
#define TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_SHIFT 16
#define TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK_US 7
-#define TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK (TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOT_MASK_US << TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOT_SHIFT)
+#define TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK (TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK_US << TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_SHIFT)
/* TDM mode slot enable bitmask */
#define TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_SHIFT 8
--
2.11.0
Hi Edward,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on tegra/for-next]
[also build test ERROR on v4.18-rc6 next-20180727]
[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/Jorge-Sanjuan/ASoC-Tegra30-TDM-support/20180728-163720
base: https://git.kernel.org/pub/scm/linux/kernel/git/tegra/linux.git for-next
config: arm-multi_v7_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.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.2.0 make.cross ARCH=arm
Note: the linux-review/Jorge-Sanjuan/ASoC-Tegra30-TDM-support/20180728-163720 HEAD 14bbc96df0fa027f7bc057eb2da8181baff4e22c builds fine.
It only hurts bisectibility.
All errors (new ones prefixed by >>):
sound/soc/tegra/tegra30_i2s.c: In function 'tegra30_i2s_hw_params':
>> sound/soc/tegra/tegra30_i2s.c:155:3: error: 'audio_bits' undeclared (first use in this function); did you mean 'audit_names'?
audio_bits = TEGRA30_AUDIOCIF_BITS_24;
^~~~~~~~~~
audit_names
sound/soc/tegra/tegra30_i2s.c:155:3: note: each undeclared identifier is reported only once for each function it appears in
vim +155 sound/soc/tegra/tegra30_i2s.c
133
134 static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream,
135 struct snd_pcm_hw_params *params,
136 struct snd_soc_dai *dai)
137 {
138 struct device *dev = dai->dev;
139 struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai);
140 unsigned int mask, val, reg;
141 int ret, sample_size, srate, i2sclock, bitcnt;
142 struct tegra30_ahub_cif_conf cif_conf;
143
144 if (params_channels(params) != 2)
145 return -EINVAL;
146
147 mask = TEGRA30_I2S_CTRL_BIT_SIZE_MASK;
148 switch (params_format(params)) {
149 case SNDRV_PCM_FORMAT_S16_LE:
150 val = TEGRA30_I2S_CTRL_BIT_SIZE_16;
151 sample_size = 16;
152 break;
153 case SNDRV_PCM_FORMAT_S24_LE:
154 val = TEGRA30_I2S_CTRL_BIT_SIZE_24;
> 155 audio_bits = TEGRA30_AUDIOCIF_BITS_24;
156 sample_size = 24;
157 break;
158 case SNDRV_PCM_FORMAT_S32_LE:
159 val = TEGRA30_I2S_CTRL_BIT_SIZE_32;
160 sample_size = 32;
161 break;
162 default:
163 return -EINVAL;
164 }
165
166 regmap_update_bits(i2s->regmap, TEGRA30_I2S_CTRL, mask, val);
167
168 srate = params_rate(params);
169
170 /* Final "* 2" required by Tegra hardware */
171 i2sclock = srate * params_channels(params) * sample_size * 2;
172
173 bitcnt = (i2sclock / (2 * srate)) - 1;
174 if (bitcnt < 0 || bitcnt > TEGRA30_I2S_TIMING_CHANNEL_BIT_COUNT_MASK_US)
175 return -EINVAL;
176
177 ret = clk_set_rate(i2s->clk_i2s, i2sclock);
178 if (ret) {
179 dev_err(dev, "Can't set I2S clock rate: %d\n", ret);
180 return ret;
181 }
182
183 val = bitcnt << TEGRA30_I2S_TIMING_CHANNEL_BIT_COUNT_SHIFT;
184
185 if (i2sclock % (2 * srate))
186 val |= TEGRA30_I2S_TIMING_NON_SYM_ENABLE;
187
188 regmap_write(i2s->regmap, TEGRA30_I2S_TIMING, val);
189
190 cif_conf.threshold = 0;
191 cif_conf.audio_channels = 2;
192 cif_conf.client_channels = 2;
193 cif_conf.audio_bits = TEGRA30_AUDIOCIF_BITS_16;
194 cif_conf.client_bits = TEGRA30_AUDIOCIF_BITS_16;
195 cif_conf.expand = 0;
196 cif_conf.stereo_conv = 0;
197 cif_conf.replicate = 0;
198 cif_conf.truncate = 0;
199 cif_conf.mono_conv = 0;
200
201 if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
202 cif_conf.direction = TEGRA30_AUDIOCIF_DIRECTION_RX;
203 reg = TEGRA30_I2S_CIF_RX_CTRL;
204 } else {
205 cif_conf.direction = TEGRA30_AUDIOCIF_DIRECTION_TX;
206 reg = TEGRA30_I2S_CIF_TX_CTRL;
207 }
208
209 i2s->soc_data->set_audio_cif(i2s->regmap, reg, &cif_conf);
210
211 val = (1 << TEGRA30_I2S_OFFSET_RX_DATA_OFFSET_SHIFT) |
212 (1 << TEGRA30_I2S_OFFSET_TX_DATA_OFFSET_SHIFT);
213 regmap_write(i2s->regmap, TEGRA30_I2S_OFFSET, val);
214
215 return 0;
216 }
217
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 2018-07-28 23:28, kbuild test robot wrote:
> Hi Edward,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on tegra/for-next]
> [also build test ERROR on v4.18-rc6 next-20180727]
> [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/Jorge-Sanjuan/ASoC-Tegra30-TDM-support/20180728-163720
> base: https://git.kernel.org/pub/scm/linux/kernel/git/tegra/linux.git
> for-next
> config: arm-multi_v7_defconfig (attached as .config)
> compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.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.2.0 make.cross ARCH=arm
>
> Note: the
> linux-review/Jorge-Sanjuan/ASoC-Tegra30-TDM-support/20180728-163720
> HEAD 14bbc96df0fa027f7bc057eb2da8181baff4e22c builds fine.
> It only hurts bisectibility.
>
> All errors (new ones prefixed by >>):
>
> sound/soc/tegra/tegra30_i2s.c: In function 'tegra30_i2s_hw_params':
>>> sound/soc/tegra/tegra30_i2s.c:155:3: error: 'audio_bits' undeclared
>>> (first use in this function); did you mean 'audit_names'?
> audio_bits = TEGRA30_AUDIOCIF_BITS_24;
> ^~~~~~~~~~
> audit_names
> sound/soc/tegra/tegra30_i2s.c:155:3: note: each undeclared
> identifier is reported only once for each function it appears in
>
> vim +155 sound/soc/tegra/tegra30_i2s.c
>
> 133
> 134 static int tegra30_i2s_hw_params(struct snd_pcm_substream
> *substream,
> 135 struct snd_pcm_hw_params *params,
> 136 struct snd_soc_dai *dai)
> 137 {
> 138 struct device *dev = dai->dev;
> 139 struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> 140 unsigned int mask, val, reg;
> 141 int ret, sample_size, srate, i2sclock, bitcnt;
> 142 struct tegra30_ahub_cif_conf cif_conf;
> 143
> 144 if (params_channels(params) != 2)
> 145 return -EINVAL;
> 146
> 147 mask = TEGRA30_I2S_CTRL_BIT_SIZE_MASK;
> 148 switch (params_format(params)) {
> 149 case SNDRV_PCM_FORMAT_S16_LE:
> 150 val = TEGRA30_I2S_CTRL_BIT_SIZE_16;
> 151 sample_size = 16;
> 152 break;
> 153 case SNDRV_PCM_FORMAT_S24_LE:
> 154 val = TEGRA30_I2S_CTRL_BIT_SIZE_24;
> > 155 audio_bits = TEGRA30_AUDIOCIF_BITS_24;
> 156 sample_size = 24;
> 157 break;
> 158 case SNDRV_PCM_FORMAT_S32_LE:
> 159 val = TEGRA30_I2S_CTRL_BIT_SIZE_32;
> 160 sample_size = 32;
> 161 break;
> 162 default:
> 163 return -EINVAL;
> 164 }
>
looks like we failed to merge in a fix from later in the internal
series we have.
jorge: can we get the channel fix from here into this patch and
resubmit?
commit dd439f5f0b748eba43da7f18cabec8850dcd18b1
Author: Edward Cragg <[email protected]>
Date: Thu Sep 15 17:01:49 2016 +0100
ASoC: tegra: i2s: Add support for more than 2 channels
On Fri, Jul 27, 2018 at 01:59:29PM +0100, Jorge Sanjuan wrote:
> From: Edward Cragg <[email protected]>
>
> Add a callback to configure TDM settings for the Tegra30
> I2S ASoC 'platform' driver.
>
> Signed-off-by: Ben Dooks <[email protected]>
> Signed-off-by: Edward Cragg <[email protected]>
This says it was britten by Edward but there's a signoff from Ben before
his?
> + dev_dbg(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);
Please don't split log messages over lines, it makes it harder to grep
for them. Just use a long line.
I'm also not seeing any validation of the parameters?
On 27/07/18 13:59, Jorge Sanjuan wrote:
> From: Edward Cragg <[email protected]>
>
> Fix typo in macro TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK.
>
> Signed-off-by: Edward Cragg <[email protected]>
> Signed-off-by: Jorge Sanjuan <[email protected]>
> ---
> sound/soc/tegra/tegra30_i2s.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sound/soc/tegra/tegra30_i2s.h b/sound/soc/tegra/tegra30_i2s.h
> index 774fc6ad2026..2e561e946de2 100644
> --- a/sound/soc/tegra/tegra30_i2s.h
> +++ b/sound/soc/tegra/tegra30_i2s.h
> @@ -173,7 +173,7 @@
> /* Number of slots in frame, minus 1 */
> #define TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_SHIFT 16
> #define TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK_US 7
> -#define TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK (TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOT_MASK_US << TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOT_SHIFT)
> +#define TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK (TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK_US << TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_SHIFT)
>
> /* TDM mode slot enable bitmask */
> #define TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_SHIFT 8
Thanks for fixing.
Reviewed-by: Jon Hunter <[email protected]>
Cheers
Jon
--
nvpublic
On 27/07/18 13:59, Jorge Sanjuan wrote:
> From: Edward Cragg <[email protected]>
>
> Add a callback to configure TDM settings for the Tegra30
> I2S ASoC 'platform' driver.
>
> Signed-off-by: Ben Dooks <[email protected]>
> Signed-off-by: Edward Cragg <[email protected]>
> [[email protected]: Style fixes]
> Signed-off-by: Jorge Sanjuan <[email protected]>
> ---
> 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 0b176ea24914..ff1996f215ed 100644
> --- a/sound/soc/tegra/tegra30_i2s.c
> +++ b/sound/soc/tegra/tegra30_i2s.c
> @@ -265,6 +265,39 @@ static int tegra30_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
> return 0;
> }
>
> +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_dbg(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);
> +
> + pm_runtime_get_sync(dai->dev);
> + regmap_update_bits(i2s->regmap, TEGRA30_I2S_SLOT_CTRL, mask, val);
> +
> + /* Set FSYNC width */
> + mask = TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_MASK;
> + val = (slot_width - 1) << TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_SHIFT;
> +
> + regmap_update_bits(i2s->regmap, TEGRA30_I2S_CH_CTRL, mask, val);
> + pm_runtime_put(dai->dev);
> +
> + return 0;
> +}
> +
Looking at the TRM for Tegra30 and Tegra124, the I2S_SLOT_CTRL register is different
where for Tegra30 the 'TOTAL_SLOTS' bit are in position 18:16, but for Tegra124 they
are 3:0. This driver supports both Tegra30 and Tegra124, and so this function will
need to handle both.
It can be quite common for the fsync-width for DSP modes to be a single clock and so
I am not sure that is makes sense to set this here always to the slot width. It maybe
worth considering add a DT property for specifying the fsync width.
Cheers
Jon
--
nvpublic
On Mon, Jul 30, 2018 at 09:49:08AM +0100, Mark Brown wrote:
> On Fri, Jul 27, 2018 at 01:59:29PM +0100, Jorge Sanjuan wrote:
> > From: Edward Cragg <[email protected]>
> >
> > Add a callback to configure TDM settings for the Tegra30
> > I2S ASoC 'platform' driver.
> >
> > Signed-off-by: Ben Dooks <[email protected]>
> > Signed-off-by: Edward Cragg <[email protected]>
>
> This says it was britten by Edward but there's a signoff from Ben before
> his?
Editing accdient, I was originally going to submit this series.
> > + dev_dbg(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);
>
> Please don't split log messages over lines, it makes it harder to grep
> for them. Just use a long line.
>
> I'm also not seeing any validation of the parameters?
> _______________________________________________
> Alsa-devel mailing list
> [email protected]
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
--
Ben Dooks, [email protected], http://www.fluff.org/ben/
Large Hadron Colada: A large Pina Colada that makes the universe disappear.
On 27/07/18 13:59, Jorge Sanjuan wrote:
> From: Edward Cragg <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Jorge Sanjuan <[email protected]>
> ---
> sound/soc/tegra/tegra30_i2s.c | 21 ++++++++++++---------
> 1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c
> index e26c19ef7439..0f240d7989d0 100644
> --- a/sound/soc/tegra/tegra30_i2s.c
> +++ b/sound/soc/tegra/tegra30_i2s.c
> @@ -138,16 +138,17 @@ 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, channels;
> struct tegra30_ahub_cif_conf cif_conf;
>
> - if (params_channels(params) != 2)
> + if (params_channels(params) > 8)
> return -EINVAL;
For normal I2S mode, channels should always be 2 and so it could be worth checking
if we are using TDM mode here or not.
>
> mask = TEGRA30_I2S_CTRL_BIT_SIZE_MASK;
> 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:
> @@ -157,6 +158,7 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream,
> 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:
> @@ -166,9 +168,10 @@ 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);
> + channels = params_channels(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)
> @@ -188,10 +191,10 @@ 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_bits = TEGRA30_AUDIOCIF_BITS_16;
> - cif_conf.client_bits = TEGRA30_AUDIOCIF_BITS_16;
> + 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;
> cif_conf.stereo_conv = 0;
> cif_conf.replicate = 0;
> @@ -329,7 +332,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 |
> @@ -338,7 +341,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 |
>
Otherwise, assuming that you fix patch 3/4 and rebase this one, looks good to me.
Cheers
Jon
--
nvpublic
On Mon, Jul 30, 2018 at 10:31:16AM +0100, Jon Hunter wrote:
> It can be quite common for the fsync-width for DSP modes to be a single clock and so
> I am not sure that is makes sense to set this here always to the slot width. It maybe
> worth considering add a DT property for specifying the fsync width.
DSP modes only care about the rising edge of the LRCLK, the pulse can be
any width without causing interoperability problems.
On Mon, Jul 30, 2018 at 10:46:14AM +0100, Jon Hunter wrote:
> On 27/07/18 13:59, Jorge Sanjuan wrote:
> > - if (params_channels(params) != 2)
> > + if (params_channels(params) > 8)
> > return -EINVAL;
> For normal I2S mode, channels should always be 2 and so it could be worth checking
> if we are using TDM mode here or not.
Yes, there's some question if a multi-channel I2S setup is going to be
all the left channels then all the right channels, have multiple data
lines in parallel (this especially common for high end applications) or
something else. Usually it's safer to use a DSP mode for those.
Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns. Doing this makes your messages much
easier to read and reply to.
The patch
ASoC: tegra: i2s: Fix typo/broken macro
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
From 279fef50b607f9cee94f10bae84f6730e97ccd7c Mon Sep 17 00:00:00 2001
From: Edward Cragg <[email protected]>
Date: Fri, 27 Jul 2018 13:59:28 +0100
Subject: [PATCH] ASoC: tegra: i2s: Fix typo/broken macro
Fix typo in macro TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK.
Signed-off-by: Edward Cragg <[email protected]>
Signed-off-by: Jorge Sanjuan <[email protected]>
Reviewed-by: Jon Hunter <[email protected]>
Signed-off-by: Mark Brown <[email protected]>
---
sound/soc/tegra/tegra30_i2s.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/tegra/tegra30_i2s.h b/sound/soc/tegra/tegra30_i2s.h
index 774fc6ad2026..2e561e946de2 100644
--- a/sound/soc/tegra/tegra30_i2s.h
+++ b/sound/soc/tegra/tegra30_i2s.h
@@ -173,7 +173,7 @@
/* Number of slots in frame, minus 1 */
#define TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_SHIFT 16
#define TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK_US 7
-#define TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK (TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOT_MASK_US << TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOT_SHIFT)
+#define TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK (TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK_US << TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_SHIFT)
/* TDM mode slot enable bitmask */
#define TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_SHIFT 8
--
2.18.0
On 30/07/18 11:18, Mark Brown wrote:
> On Mon, Jul 30, 2018 at 10:31:16AM +0100, Jon Hunter wrote:
>
>> It can be quite common for the fsync-width for DSP modes to be a single clock and so
>> I am not sure that is makes sense to set this here always to the slot width. It maybe
>> worth considering add a DT property for specifying the fsync width.
>
> DSP modes only care about the rising edge of the LRCLK, the pulse can be
> any width without causing interoperability problems.
OK, thanks I was not able to find a spec that defines this, but I saw a
lot of codecs use a single bit clock width. So then equally making the
default '1' should also be fine.
I still do not like configuring the fsync width in this function. The
fsync width needs to be configured for both DSP modes and normal I2S
modes and so it seems it would be more appropriate to do this in the
hw_params function for this driver.
Cheers
Jon
--
nvpublic
On 30/07/18 15:04, Jon Hunter wrote:
> I still do not like configuring the fsync width in this function. The
> fsync width needs to be configured for both DSP modes and normal I2S
> modes and so it seems it would be more appropriate to do this in the
> hw_params function for this driver.
That said, it does not look like this current driver ever programs the
fsync width for normal I2S mode (which I thought was necessary as we do
in other Tegra I2S drivers). So I will check on this and confirm.
Jon
--
nvpublic
On Mon, Jul 30, 2018 at 03:04:46PM +0100, Jon Hunter wrote:
> On 30/07/18 11:18, Mark Brown wrote:
> > DSP modes only care about the rising edge of the LRCLK, the pulse can be
> > any width without causing interoperability problems.
> OK, thanks I was not able to find a spec that defines this, but I saw a
> lot of codecs use a single bit clock width. So then equally making the
> default '1' should also be fine.
There's not really a spec for this, it's just what tends to be
implemented.
> I still do not like configuring the fsync width in this function. The
> fsync width needs to be configured for both DSP modes and normal I2S
> modes and so it seems it would be more appropriate to do this in the
> hw_params function for this driver.
You *could* just always use the I2S width, it's going to look odd when
people use a scope but it will work most of the time.
On 2018-07-27 13:59, Jorge Sanjuan wrote:
> This patchset adds support for TDM audio on Tegra30 hardware.
> It adds the DAI's `set_tdm_slot` callback and enables a tegra
> pcm to have up to 8 channels.
>
> It also includes support for other audio formats supported by
> the Tegra30 HW and fixes a broken macro needed for setting the
> TDM on the registers.
>
> Based on Linux 4.18-rc3 tag.
>
> Edward Cragg (4):
> ASoC: tegra: i2s: Fix typo/broken macro
> ASoC: tegra: Add a TDM configuration callback
> ASoC: tegra: Allow 32-bit and 24-bit samples
> ASoC: tegra: i2s: Add support for more than 2 channels
>
> sound/soc/tegra/tegra30_i2s.c | 72
> ++++++++++++++++++++++++++++++++++++-------
> sound/soc/tegra/tegra30_i2s.h | 2 +-
> 2 files changed, 62 insertions(+), 12 deletions(-)
Just as a note, Ed Cragg has moved on from Codethink and I've not
got a new address for him. We've been cleaning some of the work Ed,
Jorge, Terry and myself have been doing.
--
Ben
On 2018-07-30 16:07, Mark Brown wrote:
> On Mon, Jul 30, 2018 at 03:04:46PM +0100, Jon Hunter wrote:
>> On 30/07/18 11:18, Mark Brown wrote:
>
>> > DSP modes only care about the rising edge of the LRCLK, the pulse can be
>> > any width without causing interoperability problems.
>
>> OK, thanks I was not able to find a spec that defines this, but I saw
>> a
>> lot of codecs use a single bit clock width. So then equally making the
>> default '1' should also be fine.
>
> There's not really a spec for this, it's just what tends to be
> implemented.
>
>> I still do not like configuring the fsync width in this function. The
>> fsync width needs to be configured for both DSP modes and normal I2S
>> modes and so it seems it would be more appropriate to do this in the
>> hw_params function for this driver.
>
> You *could* just always use the I2S width, it's going to look odd when
> people use a scope but it will work most of the time.
We did this as we were dealing with a legacy system in which we didn't
know if this was important setting or not, so we tried to make the
settings as close as possible to the original nvidia supplied source.
--
Ben