Unify NVIDIA Tegra ASoC machine drivers

Dne 20. 05. 21 v 19:50 Dmitry Osipenko napsal(a):

sound/soc/tegra/tegra_asoc_machine.c line 472

Quoted 9 lines

Squash all machine drivers into a single-universal one. This reduces code duplication, eases addition of a new drivers and upgrades older code to a modern Linux kernel APIs.

sound/soc/tegra/tegra_asoc_machine.c new file +5 -0

+static struct snd_soc_card snd_soc_tegra_wm9712 = {
+	.dai_link = &tegra_wm9712_dai,
+	.num_links = 1,
+	.fully_routed = true,
+};

Please, could you also initialize snd_soc_card->components? It may be useful to pass the codec identification to the user space like:

.components = "codec:wm9712"

The passed information should be consistent. You may look into the Intel ASoC drivers for the examples (card->components initialization). There are also hints about the number of connected microphones ("cfg-mic:2" - configuration with 2 microphones) or the codec purpose ("hs:rt711" - headset codec is RT711) etc.

Quoted 13 lines

Dne 20. 05. 21 v 19:50 Dmitry Osipenko napsal(a):

sound/soc/tegra/tegra_asoc_machine.c line 472

Quoted 9 lines

Squash all machine drivers into a single-universal one. This reduces code duplication, eases addition of a new drivers and upgrades older code to a modern Linux kernel APIs.

sound/soc/tegra/tegra_asoc_machine.c new file +5 -0

+static struct snd_soc_card snd_soc_tegra_wm9712 = {
+	.dai_link = &tegra_wm9712_dai,
+	.num_links = 1,
+	.fully_routed = true,
+};

Please, could you also initialize snd_soc_card->components? It may be useful to pass the codec identification to the user space like:

Quoted 1 line

.components = "codec:wm9712"

Hrm, if this is important to userspace shouldn't the core be doing something by default given that it already knows all the components going into the card?

Quoted 5 lines

The passed information should be consistent. You may look into the Intel ASoC drivers for the examples (card->components initialization). There are also hints about the number of connected microphones ("cfg-mic:2" - configuration with 2 microphones) or the codec purpose ("hs:rt711" - headset codec is RT711) etc.

This sort of stuff is more something that the card should layer on top.

Dne 20. 05. 21 v 19:50 Dmitry Osipenko napsal(a):

sound/soc/tegra/tegra_asoc_machine.c line 472

Quoted 11 lines

Squash all machine drivers into a single-universal one. This reduces code duplication, eases addition of a new drivers and upgrades older code to a modern Linux kernel APIs.

sound/soc/tegra/tegra_asoc_machine.c new file +5 -0

+static struct snd_soc_card snd_soc_tegra_wm9712 = {
+	.dai_link = &tegra_wm9712_dai,
+	.num_links = 1,
+	.fully_routed = true,
+};

Please, could you also initialize snd_soc_card->components? It may be useful to pass the codec identification to the user space like:

.components = "codec:wm9712"

The passed information should be consistent. You may look into the Intel ASoC drivers for the examples (card->components initialization). There are also hints about the number of connected microphones ("cfg-mic:2" - configuration with 2 microphones) or the codec purpose ("hs:rt711" - headset codec is RT711) etc.

Squash all machine drivers into a single-universal one. This reduces code duplication, eases addition of a new drivers and upgrades older code to a modern Linux kernel APIs.

Suggested-by Jonathan Hunter <[email protected]>

Co-developed-by Ion Agorria <[email protected]>

Signed-off-by Ion Agorria <[email protected]>

Co-developed-by Svyatoslav Ryhel <[email protected]>

Signed-off-by Svyatoslav Ryhel <[email protected]>

Signed-off-by Dmitry Osipenko <[email protected]>

---
 sound/soc/tegra/Kconfig              |  12 +
 sound/soc/tegra/Makefile             |  18 +-
 sound/soc/tegra/tegra20_ac97.c       |   1 -
 sound/soc/tegra/tegra_alc5632.c      | 260 ----------
 sound/soc/tegra/tegra_asoc_machine.c | 732 +++++++++++++++++++++++++++
 sound/soc/tegra/tegra_asoc_machine.h |  45 ++
 sound/soc/tegra/tegra_max98090.c     | 277 ----------
 sound/soc/tegra/tegra_rt5640.c       | 223 --------
 sound/soc/tegra/tegra_rt5677.c       | 325 ------------
 sound/soc/tegra/tegra_sgtl5000.c     | 212 --------
 sound/soc/tegra/tegra_wm8753.c       | 186 -------
 sound/soc/tegra/tegra_wm8903.c       | 358 +++----------
 sound/soc/tegra/tegra_wm9712.c       | 167 ------
 sound/soc/tegra/trimslice.c          | 173 -------
 14 files changed, 862 insertions(+), 2127 deletions(-)
 delete mode 100644 sound/soc/tegra/tegra_alc5632.c
 create mode 100644 sound/soc/tegra/tegra_asoc_machine.c
 create mode 100644 sound/soc/tegra/tegra_asoc_machine.h
 delete mode 100644 sound/soc/tegra/tegra_max98090.c
 delete mode 100644 sound/soc/tegra/tegra_rt5640.c
 delete mode 100644 sound/soc/tegra/tegra_rt5677.c
 delete mode 100644 sound/soc/tegra/tegra_sgtl5000.c
 delete mode 100644 sound/soc/tegra/tegra_wm8753.c
 delete mode 100644 sound/soc/tegra/tegra_wm9712.c
 delete mode 100644 sound/soc/tegra/trimslice.c

sound/soc/tegra/tegra_asoc_machine.c new file +33 -0

+	unsigned int mclk;
+
+	switch (srate) {
+	case 8000:
+	case 16000:
+	case 24000:
+	case 32000:
+	case 48000:
+	case 64000:
+	case 96000:
+		mclk = 12288000;
+		break;
+	case 11025:
+	case 22050:
+	case 44100:
+	case 88200:
+		mclk = 11289600;
+		break;
+	default:
+		mclk = 12000000;
+		break;
+	}
+
+	return mclk;
+}
+
+unsigned int tegra_asoc_machine_mclk_rate(unsigned int srate)
+{
+	unsigned int mclk;
+
+	switch (srate) {
+	case 64000:
+	case 88200:

sound/soc/tegra/tegra_asoc_machine.c new file +13 -0

+
+	return mclk;
+}
+EXPORT_SYMBOL_GPL(tegra_asoc_machine_mclk_rate);> +
+static int tegra_machine_hw_params(struct snd_pcm_substream *substream,
+				   struct snd_pcm_hw_params *params)
+{
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
+	struct snd_soc_card *card = rtd->card;
+	struct tegra_machine *machine = snd_soc_card_get_drvdata(card);
+	unsigned int srate = params_rate(params);
+	unsigned int mclk = machine->asoc->mclk_rate(srate);

Quoted 36 lines

Squash all machine drivers into a single-universal one. This reduces code duplication, eases addition of a new drivers and upgrades older code to a modern Linux kernel APIs.

Suggested-by Jonathan Hunter <[email protected]>

Co-developed-by Ion Agorria <[email protected]>

Signed-off-by Ion Agorria <[email protected]>

Co-developed-by Svyatoslav Ryhel <[email protected]>

Signed-off-by Svyatoslav Ryhel <[email protected]>

Signed-off-by Dmitry Osipenko <[email protected]>

---
 sound/soc/tegra/Kconfig              |  12 +
 sound/soc/tegra/Makefile             |  18 +-
 sound/soc/tegra/tegra20_ac97.c       |   1 -
 sound/soc/tegra/tegra_alc5632.c      | 260 ----------
 sound/soc/tegra/tegra_asoc_machine.c | 732 +++++++++++++++++++++++++++
 sound/soc/tegra/tegra_asoc_machine.h |  45 ++
 sound/soc/tegra/tegra_max98090.c     | 277 ----------
 sound/soc/tegra/tegra_rt5640.c       | 223 --------
 sound/soc/tegra/tegra_rt5677.c       | 325 ------------
 sound/soc/tegra/tegra_sgtl5000.c     | 212 --------
 sound/soc/tegra/tegra_wm8753.c       | 186 -------
 sound/soc/tegra/tegra_wm8903.c       | 358 +++----------
 sound/soc/tegra/tegra_wm9712.c       | 167 ------
 sound/soc/tegra/trimslice.c          | 173 -------
 14 files changed, 862 insertions(+), 2127 deletions(-)
 delete mode 100644 sound/soc/tegra/tegra_alc5632.c
 create mode 100644 sound/soc/tegra/tegra_asoc_machine.c
 create mode 100644 sound/soc/tegra/tegra_asoc_machine.h
 delete mode 100644 sound/soc/tegra/tegra_max98090.c
 delete mode 100644 sound/soc/tegra/tegra_rt5640.c
 delete mode 100644 sound/soc/tegra/tegra_rt5677.c
 delete mode 100644 sound/soc/tegra/tegra_sgtl5000.c
 delete mode 100644 sound/soc/tegra/tegra_wm8753.c
 delete mode 100644 sound/soc/tegra/tegra_wm9712.c
 delete mode 100644 sound/soc/tegra/trimslice.c

...

Review

Quoted 2 lines

(quoted diff) +2 -0

+static unsigned int tegra_max98090_mclk_rate(unsigned int srate)
+{

Minor comment, but I wonder if there is a better name for the above function? This function is using a fixed rate as opposed to scaling it with sample rate which can be common and not really specific to the max98090 codec.

Quoted 40 lines

sound/soc/tegra/tegra_asoc_machine.c new file +40 -0

+	unsigned int mclk;
+
+	switch (srate) {
+	case 8000:
+	case 16000:
+	case 24000:
+	case 32000:
+	case 48000:
+	case 64000:
+	case 96000:
+		mclk = 12288000;
+		break;
+	case 11025:
+	case 22050:
+	case 44100:
+	case 88200:
+		mclk = 11289600;
+		break;
+	default:
+		mclk = 12000000;
+		break;
+	}
+
+	return mclk;
+}
+
+unsigned int tegra_asoc_machine_mclk_rate(unsigned int srate)
+{
+	unsigned int mclk;
+
+	switch (srate) {
+	case 64000:
+	case 88200:
+	case 96000:
+		mclk = 128 * srate;
+		break;
+	default:
+		mclk = 256 * srate;
+		break;
+	}

sound/soc/tegra/tegra_asoc_machine.c line 254

Quoted 3 lines

sound/soc/tegra/tegra_asoc_machine.c new file +3 -0

+	/* FIXME: Codec only requires >= 3MHz if OSR==0 */
+	while (mclk < 6000000)
+		mclk *= 2;

So this appears to be specific to the wm8903 codec or at least this is where it came from. And given that the switch statement is not complete in terms of the sample rates (ie. only has a subset), I am wondering if set should keep this specific to the wm8903 codec?

Quoted 19 lines

sound/soc/tegra/tegra_asoc_machine.c new file +19 -0

+
+	return mclk;
+}
+EXPORT_SYMBOL_GPL(tegra_asoc_machine_mclk_rate);> +
+static int tegra_machine_hw_params(struct snd_pcm_substream *substream,
+				   struct snd_pcm_hw_params *params)
+{
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
+	struct snd_soc_card *card = rtd->card;
+	struct tegra_machine *machine = snd_soc_card_get_drvdata(card);
+	unsigned int srate = params_rate(params);
+	unsigned int mclk = machine->asoc->mclk_rate(srate);
+	const unsigned int clk_id = 0;
+	int err;
+
+	err = tegra_asoc_utils_set_rate(&machine->util_data, srate, mclk);
+	if (err < 0) {
+		dev_err(card->dev, "Can't configure clocks: %d\n", err);

sound/soc/tegra/tegra_asoc_machine.c line 278

Quoted 4 lines

sound/soc/tegra/tegra_asoc_machine.c new file +4 -0

+		return err;
+	}
+
+	err = snd_soc_dai_set_sysclk(codec_dai, clk_id, mclk, SND_SOC_CLOCK_IN);

Looks like clk_id is always 0. Most likely all the clock ids passed are 0 by default but I wonder if we should not assume this in case something changes in the future?

Quoted 14 lines

Quoted 14 lines

sound/soc/tegra/tegra_asoc_machine.c new file +14 -0

+unsigned int tegra_asoc_machine_mclk_rate(unsigned int srate)
+{
+	unsigned int mclk;
+
+	switch (srate) {
+	case 64000:
+	case 88200:
+	case 96000:
+		mclk = 128 * srate;
+		break;
+	default:
+		mclk = 256 * srate;
+		break;
+	}

Quoted 8 lines

sound/soc/tegra/tegra_asoc_machine.c line 254

Quoted 3 lines

sound/soc/tegra/tegra_asoc_machine.c new file +3 -0

+	/* FIXME: Codec only requires >= 3MHz if OSR==0 */
+	while (mclk < 6000000)
+		mclk *= 2;

So this appears to be specific to the wm8903 codec or at least this is where it came from. And given that the switch statement is not complete in terms of the sample rates (ie. only has a subset), I am wondering if set should keep this specific to the wm8903 codec?

The RT5631 codec of Asus Transformers will re-use this function.

IIUC, the default switch-case works properly for all rates below 64KHz, at least I haven't had any problems with it. Could you please clarify why you are saying that the switch statement appears to be incomplete?

...

Quoted 23 lines

Quoted 22 lines

Quoted 7 lines

sound/soc/tegra/tegra_asoc_machine.c new file +7 -0

+unsigned int tegra_asoc_machine_mclk_rate(unsigned int srate)
+{
+	unsigned int mclk;
+
+	switch (srate) {
+	case 64000:
+	case 88200:

sound/soc/tegra/tegra_asoc_machine.c line 254

Quoted 10 lines

sound/soc/tegra/tegra_asoc_machine.c new file +10 -0

+	case 96000:
+		mclk = 128 * srate;
+		break;
+	default:
+		mclk = 256 * srate;
+		break;
+	}
+	/* FIXME: Codec only requires >= 3MHz if OSR==0 */
+	while (mclk < 6000000)
+		mclk *= 2;

So this appears to be specific to the wm8903 codec or at least this is where it came from. And given that the switch statement is not complete in terms of the sample rates (ie. only has a subset), I am wondering if set should keep this specific to the wm8903 codec?

Quoted 1 line

The RT5631 codec of Asus Transformers will re-use this function.

OK, but does it need this FIXME part? That appears to be codec specific.

Quoted 3 lines

IIUC, the default switch-case works properly for all rates below 64KHz, at least I haven't had any problems with it. Could you please clarify why you are saying that the switch statement appears to be incomplete?

It looks a bit weird because less than 64kHz and greater than 96kHz we use 256 and for only 64kHz, 88.2kHz and 96kHz we use 128. So it is not clear to me which sample rates have actually been tested with this and if this is complete or not?

Is it intended that we use 256 for sample rates greater than 96kHz?

24.05.2021 15:22, Jon Hunter пишет:

Quoted 29 lines · Dmitry Osipenko

...

Quoted 23 lines

Quoted 22 lines

Quoted 7 lines

sound/soc/tegra/tegra_asoc_machine.c new file +7 -0

+unsigned int tegra_asoc_machine_mclk_rate(unsigned int srate)
+{
+	unsigned int mclk;
+
+	switch (srate) {
+	case 64000:
+	case 88200:

sound/soc/tegra/tegra_asoc_machine.c line 254

Quoted 10 lines

sound/soc/tegra/tegra_asoc_machine.c new file +10 -0

+	case 96000:
+		mclk = 128 * srate;
+		break;
+	default:
+		mclk = 256 * srate;
+		break;
+	}
+	/* FIXME: Codec only requires >= 3MHz if OSR==0 */
+	while (mclk < 6000000)
+		mclk *= 2;

So this appears to be specific to the wm8903 codec or at least this is where it came from. And given that the switch statement is not complete in terms of the sample rates (ie. only has a subset), I am wondering if set should keep this specific to the wm8903 codec?

Quoted 3 lines

Quoted 1 line

The RT5631 codec of Asus Transformers will re-use this function.

OK, but does it need this FIXME part? That appears to be codec specific.

Downstream RT5631 Tegra driver has exactly the same FIXME.

Although, I now see that downstream RT5631 uses 384*srate for the default cases.

I also see that WM8994 driver that we have in grate-kernel for Galaxy Tab and SGH-I927 also re-uses that mclk_rate function.

Quoted 9 lines

Quoted 3 lines

IIUC, the default switch-case works properly for all rates below 64KHz, at least I haven't had any problems with it. Could you please clarify why you are saying that the switch statement appears to be incomplete?

It looks a bit weird because less than 64kHz and greater than 96kHz we use 256 and for only 64kHz, 88.2kHz and 96kHz we use 128. So it is not clear to me which sample rates have actually been tested with this and if this is complete or not?

Quoted 1 line

Is it intended that we use 256 for sample rates greater than 96kHz?

The 128*srate gives MCLK >6MHZ for 64/88/96, 256*srate gives MCLK >6MHZ for rates below 64kHZ. Looks like the goal is to get MCLK >6MHZ.

The WM8903 datasheet says:

"The following operating frequency limits must be observed when

 configuring  CLK_SYS.  Failure  to  observe   these   limits   will
result   in   degraded  noise   performance   and/or   incorrect

ADC/DAC functionality.

If DAC_OSR = 0 then CLK_SYS 3MHz
If DAC_OSR = 1 then CLK_SYS 6MHz"

Where DAC_OSR is DAC Oversampling Control
0 = Low power (normal oversample)
1 = High performance (double rate)

I see that DAC_OSR=0 by default, it can be switched to 1 by userspace ALSA control.

Quoted 34 lines

24.05.2021 15:22, Jon Hunter пишет:

Quoted 32 lines · Dmitry Osipenko

...

Quoted 23 lines

Quoted 14 lines

Quoted 14 lines

sound/soc/tegra/tegra_asoc_machine.c new file +14 -0

+unsigned int tegra_asoc_machine_mclk_rate(unsigned int srate)
+{
+	unsigned int mclk;
+
+	switch (srate) {
+	case 64000:
+	case 88200:
+	case 96000:
+		mclk = 128 * srate;
+		break;
+	default:
+		mclk = 256 * srate;
+		break;
+	}

Quoted 8 lines

sound/soc/tegra/tegra_asoc_machine.c line 254

Quoted 3 lines

sound/soc/tegra/tegra_asoc_machine.c new file +3 -0

+	/* FIXME: Codec only requires >= 3MHz if OSR==0 */
+	while (mclk < 6000000)
+		mclk *= 2;

So this appears to be specific to the wm8903 codec or at least this is where it came from. And given that the switch statement is not complete in terms of the sample rates (ie. only has a subset), I am wondering if set should keep this specific to the wm8903 codec?

Quoted 1 line

The RT5631 codec of Asus Transformers will re-use this function.

OK, but does it need this FIXME part? That appears to be codec specific.

Quoted 1 line

Downstream RT5631 Tegra driver has exactly the same FIXME.

What downstream branch are you referring to? I still don't think that that is a good reason to make this 'FIXME' the standard going forward and hence I would prefer that it is kept specific the wm8903. Otherwise people will keep using this code without understanding if this is needed/correct.

Quoted 15 lines

Although, I now see that downstream RT5631 uses 384*srate for the default cases.

I also see that WM8994 driver that we have in grate-kernel for Galaxy Tab and SGH-I927 also re-uses that mclk_rate function.

Quoted 9 lines

Quoted 3 lines

IIUC, the default switch-case works properly for all rates below 64KHz, at least I haven't had any problems with it. Could you please clarify why you are saying that the switch statement appears to be incomplete?

It looks a bit weird because less than 64kHz and greater than 96kHz we use 256 and for only 64kHz, 88.2kHz and 96kHz we use 128. So it is not clear to me which sample rates have actually been tested with this and if this is complete or not?

Quoted 4 lines

Quoted 1 line

Is it intended that we use 256 for sample rates greater than 96kHz?

The 128*srate gives MCLK >6MHZ for 64/88/96, 256*srate gives MCLK >6MHZ for rates below 64kHZ. Looks like the goal is to get MCLK >6MHZ.

The wm8903 supports 8kHz sample rates and 256*8000 is less than 6MHz. Yes the FIXME loop corrects this, but you could also extend the case statement to multiply by 512 for 8kHz.

Quoted 14 lines

The WM8903 datasheet says:

"The following operating frequency limits must be observed when

 configuring  CLK_SYS.  Failure  to  observe   these   limits   will
result   in   degraded  noise   performance   and/or   incorrect

ADC/DAC functionality.

If DAC_OSR = 0 then CLK_SYS 3MHz
If DAC_OSR = 1 then CLK_SYS 6MHz"

Where DAC_OSR is DAC Oversampling Control
0 = Low power (normal oversample)
1 = High performance (double rate)

Quoted 3 lines

I see that DAC_OSR=0 by default, it can be switched to 1 by userspace ALSA control.

Yes that is all fine, but again this is specific to the wm8903.

Quoted 6 lines

Quoted 2 lines

The 128*srate gives MCLK >6MHZ for 64/88/96, 256*srate gives MCLK >6MHZ for rates below 64kHZ. Looks like the goal is to get MCLK >6MHZ.

The wm8903 supports 8kHz sample rates and 256*8000 is less than 6MHz. Yes the FIXME loop corrects this, but you could also extend the case statement to multiply by 512 for 8kHz.

But what benefits this extension will bring to us if the end result is the same?

Quoted 14 lines

Quoted 14 lines

The WM8903 datasheet says:

"The following operating frequency limits must be observed when

 configuring  CLK_SYS.  Failure  to  observe   these   limits   will
result   in   degraded  noise   performance   and/or   incorrect

ADC/DAC functionality.

If DAC_OSR = 0 then CLK_SYS 3MHz
If DAC_OSR = 1 then CLK_SYS 6MHz"

Where DAC_OSR is DAC Oversampling Control
0 = Low power (normal oversample)
1 = High performance (double rate)

Quoted 5 lines

Quoted 3 lines

I see that DAC_OSR=0 by default, it can be switched to 1 by userspace ALSA control.

Yes that is all fine, but again this is specific to the wm8903.

Alright, I'll move it to the WM8903 driver in v4. It won't be a problem to make that function shared once will be actually needed.