Skip to content

topology: add ADL SoundWire support#3978

Merged
lgirdwood merged 7 commits intothesofproject:mainfrom
plbossart:sdw/adl-topologies
Apr 13, 2021
Merged

topology: add ADL SoundWire support#3978
lgirdwood merged 7 commits intothesofproject:mainfrom
plbossart:sdw/adl-topologies

Conversation

@plbossart
Copy link
Member

add topologies based on early hardware information

For tests and checks only for now, need to make sure new macros don't introduce regressions

replaces with regular base10 indices that are easier to correlate with
kernel/DAPM logs

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Some devices are starting to remove the 3.5mm jack, we need a topology
for these cases.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
remove hard-coded link numbers and make them configurable, this will
allow for reuse of the same topology when the devices are swapped

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
@plbossart plbossart force-pushed the sdw/adl-topologies branch from c1995cb to f62b1cf Compare March 29, 2021 22:47
@plbossart
Copy link
Member Author

@bardliao @RanderWang I am completely lost on the meaning of the 'SDW%d-Playback/Capture' string, is this actually used for anything? If yes, what is the convention that needs to be followed when we swap devices around?

use eval w/ macros instead of hard-coded values

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
No need for a dedicated topology just to deal with swapped links

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
copy/pasted from tgl.m4, will need to differentiate later

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
@plbossart plbossart force-pushed the sdw/adl-topologies branch from f62b1cf to 99ed239 Compare March 29, 2021 23:07
@plbossart
Copy link
Member Author

FWIW I tested the output of the first patch (with the removal of hex values) and the last, and the .tplg binaries are identical. The .conf are different only with blanks, no content difference.

@bardliao
Copy link
Collaborator

@bardliao @RanderWang I am completely lost on the meaning of the 'SDW%d-Playback/Capture' string, is this actually used for anything? If yes, what is the convention that needs to be followed when we swap devices around?

@plbossart 'SDW%d-Playback/Capture' is the stream name, and the %d represents sdw ink id.
See https://github.com/thesofproject/linux/blob/topic/sof-dev/sound/soc/intel/boards/sof_sdw.c#L787

@plbossart
Copy link
Member Author

@bardliao @RanderWang I am completely lost on the meaning of the 'SDW%d-Playback/Capture' string, is this actually used for anything? If yes, what is the convention that needs to be followed when we swap devices around?

@plbossart 'SDW%d-Playback/Capture' is the stream name, and the %d represents sdw ink id.
See https://github.com/thesofproject/linux/blob/topic/sof-dev/sound/soc/intel/boards/sof_sdw.c#L787

Hehe. If I asked the question @RanderWang, that's precisely because I have no idea what this code does. What I was asking is a high-level description of how these link IDs are determined.

@lgirdwood
Copy link
Member

SOFCI TEST

@RanderWang
Copy link
Collaborator

@bardliao @RanderWang I am completely lost on the meaning of the 'SDW%d-Playback/Capture' string, is this actually used for anything? If yes, what is the convention that needs to be followed when we swap devices around?

@plbossart 'SDW%d-Playback/Capture' is the stream name, and the %d represents sdw ink id.
See https://github.com/thesofproject/linux/blob/topic/sof-dev/sound/soc/intel/boards/sof_sdw.c#L787

Hehe. If I asked the question @RanderWang, that's precisely because I have no idea what this code does. What I was asking is a high-level description of how these link IDs are determined.

ASoC use 'SDW%d-Playback/Capture' to match FE in topology and BE in machine driver. the %d here means link id, this requires that DAI name in topology should match this stream name in BE, .e.g if rt711 is at link0, it should always set DAI name to SDW0-playback/capture in topology. We don't care its position in ADR table in ACPI_MACH

@RanderWang
Copy link
Collaborator

And if the definition sequence of rt711 in topology and _ADR table is different, be care of DAI index in topology since machine driver allocates DAI index based on the definition sequence in _ADR table (id++)

@plbossart
Copy link
Member Author

And if the definition sequence of rt711 in topology and _ADR table is different, be care of DAI index in topology since machine driver allocates DAI index based on the definition sequence in _ADR table (id++)

Not getting the idea, sorry. The wording above refers twice to _ADR table and I don't understand it at all.

The _ADR table also have some inversions, and I wonder if this was intentional, harmless or a mistake that the links are not ordered in the following case:

static const struct snd_soc_acpi_link_adr tgl_sdw_rt711_link1_rt1308_link2_rt715_link0[] = {
	{
		.mask = BIT(1),
		.num_adr = ARRAY_SIZE(rt711_1_adr),
		.adr_d = rt711_1_adr,
	},
	{
		.mask = BIT(2),
		.num_adr = ARRAY_SIZE(rt1308_2_single_adr),
		.adr_d = rt1308_2_single_adr,
	},
	{
		.mask = BIT(0),
		.num_adr = ARRAY_SIZE(rt715_0_adr),
		.adr_d = rt715_0_adr,
	},
	{}
};

@marc-hb
Copy link
Collaborator

marc-hb commented Mar 31, 2021

Github Actions hang fixed by #3992, please git commit --amend and force push if you want to re-trigger.

@RanderWang
Copy link
Collaborator

RanderWang commented Apr 1, 2021

And if the definition sequence of rt711 in topology and _ADR table is different, be care of DAI index in topology since machine driver allocates DAI index based on the definition sequence in _ADR table (id++)

Not getting the idea, sorry. The wording above refers twice to _ADR table and I don't understand it at all.

The _ADR table also have some inversions, and I wonder if this was intentional, harmless or a mistake that the links are not ordered in the following case:

static const struct snd_soc_acpi_link_adr tgl_sdw_rt711_link1_rt1308_link2_rt715_link0[] = {
	{
		.mask = BIT(1),
		.num_adr = ARRAY_SIZE(rt711_1_adr),
		.adr_d = rt711_1_adr,
	},
	{
		.mask = BIT(2),
		.num_adr = ARRAY_SIZE(rt1308_2_single_adr),
		.adr_d = rt1308_2_single_adr,
	},
	{
		.mask = BIT(0),
		.num_adr = ARRAY_SIZE(rt715_0_adr),
		.adr_d = rt715_0_adr,
	},
	{}
};

For this case, we need to set SDW1-playback & capture to rt711 , and SDW0-capture to rt715, SDW2-playback to rt1308 in topology

And even we put rt715 at first entry in this table, we still need to follow above setting since we only care link id (.mask). I mean the order is harmless and we only check link id (.mask)

This is based on early hardware information and subject to change.

The ADL topologies use the link information to avoid confusions on
configurations. The kernel tables will use the same conventions.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
@plbossart plbossart force-pushed the sdw/adl-topologies branch from 99ed239 to 898be9c Compare April 1, 2021 17:03
@plbossart plbossart requested a review from libinyang April 1, 2021 17:32
@plbossart plbossart marked this pull request as ready for review April 12, 2021 18:29
@plbossart plbossart requested a review from mmaka1 as a code owner April 12, 2021 18:29
@plbossart
Copy link
Member Author

This PR was tested by @libinyang, the ACPI changes were merged with thesofproject/acpi-scripts#11

This looks good enough for a review now.

@plbossart plbossart requested a review from RanderWang April 12, 2021 18:31
Copy link
Collaborator

@RanderWang RanderWang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lgirdwood lgirdwood merged commit fbdc330 into thesofproject:main Apr 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants