Detect SSP port number from NHLT table #4492

brentlu · 2023-07-26T04:03:59Z

In current machine driver we need to specify SSP port number for headphone codec, speaker amplifiers, and BT offload in driver data like this:

{
	.name = "adl_max98390_rt5682",
	.driver_data = (kernel_ulong_t)(SOF_RT5682_MCLK_EN |
				SOF_RT5682_SSP_CODEC(0) |
				SOF_SPEAKER_AMP_PRESENT |
				SOF_MAX98390_SPEAKER_AMP_PRESENT |
				SOF_RT5682_SSP_AMP(1) |
				SOF_RT5682_NUM_HDMIDEV(4) |
				SOF_BT_OFFLOAD_SSP(2) |
				SOF_SSP_BT_OFFLOAD_PRESENT),
},

If there is a design using different SSP port allocation, then we need to add DMI quirk for the board which is not easy to maintain. So far today I saw three quirks for MTL boards, there will be more as we scale the design in the near future. What if we could remove the port numbers from driver data? We could also remove the need to add DMI quirks to machine driver and OEM strings to DMI simply because the port usage is different.

{
	.callback = sof_rt5682_quirk_cb,
	.matches = {
		DMI_MATCH(DMI_PRODUCT_FAMILY, "Google_Brya"),
		DMI_MATCH(DMI_OEM_STRING, "AUDIO-MAX98390_ALC5682I_I2S"),
	},
	.driver_data = (void *)(SOF_RT5682_MCLK_EN |
				SOF_RT5682_SSP_CODEC(0) |
				SOF_SPEAKER_AMP_PRESENT |
				SOF_MAX98390_SPEAKER_AMP_PRESENT |
				SOF_RT5682_SSP_AMP(2) |
				SOF_RT5682_NUM_HDMIDEV(4)),
},

After reading the NHLT specification (#595976), I realized that the direction field in endpoint descriptor provides information to determine which port is for headphone and which one is for speakers. The headphone is supposed to use both direction 0(render) and 1(capture) and speakers should use 0(render) only. For smartamps or speaker with echo reference, the direction 2 and 3 should be used instead (if my understanding is correct).

Endpoint direction:
0 – Render
1 – Capture
2 – Render with loopback / feedback
3 – Feedback for render (smartamp)

As for BT offload, we could use the device type field to tell which SSP port is for BT offload. Device type of BT offload is 0 while other I2S codecs are using device type 4.

Type of device (unique to link type):
SSP Link:
0 – BT Sideband
1 – FM
2 – Modem
3 – reserved for future use
4 – SSP Analog Codec
5-7 – reserved for future use

I added code in sof-tplg layer to read and parse NHLT table found it's not working, then I checked the NHLT endpoint descriptors and found that all device type is set to BT (0) and speaker endpoints do not use direction 2 and 3.

[ 5.948273] intel_nhlt_ssp_dir_mask: ep=0, link=2, device=0, dir=1, vbus=0
[ 5.948276] intel_nhlt_ssp_dir_mask: ep=1, link=2, device=0, dir=1, vbus=1
=> DMIC
[ 5.948278] intel_nhlt_ssp_dir_mask: ep=2, link=3, device=0, dir=0, vbus=0
[ 5.948279] intel_nhlt_ssp_dir_mask: ep=3, link=3, device=0, dir=1, vbus=0
=> headphone codec, ssp 0
[ 5.948281] intel_nhlt_ssp_dir_mask: ep=4, link=3, device=0, dir=0, vbus=1
[ 5.948283] intel_nhlt_ssp_dir_mask: ep=5, link=3, device=0, dir=1, vbus=1
=> speaker amplifier, ssp 1
[ 5.948285] intel_nhlt_ssp_dir_mask: ep=6, link=3, device=0, dir=0, vbus=2
[ 5.948286] intel_nhlt_ssp_dir_mask: ep=7, link=3, device=0, dir=1, vbus=2
=> BT offload, ssp 2

Then I patched alsatplg and topology to generate a NHLT table with device types and directions I want: device type of i2s codec is set to 4 and direction of duplex speaker(smartamp or speaker pcm with echo reference) is set to 2/3.

[ 5.846447] intel_nhlt_ssp_dir_mask: ep=0, link=2, device=0, dir=1, vbus=0
[ 5.846449] intel_nhlt_ssp_dir_mask: ep=1, link=2, device=0, dir=1, vbus=1
=> DMIC
[ 5.846451] intel_nhlt_ssp_dir_mask: ep=2, link=3, device=4, dir=0, vbus=0
[ 5.846453] intel_nhlt_ssp_dir_mask: ep=3, link=3, device=4, dir=1, vbus=0
=> headphone codec
[ 5.846455] intel_nhlt_ssp_dir_mask: ep=4, link=3, device=4, dir=2, vbus=1
[ 5.846456] intel_nhlt_ssp_dir_mask: ep=5, link=3, device=4, dir=3, vbus=1
=> speaker amplifier
[ 5.846458] intel_nhlt_ssp_dir_mask: ep=6, link=3, device=0, dir=0, vbus=2
[ 5.846460] intel_nhlt_ssp_dir_mask: ep=7, link=3, device=0, dir=1, vbus=2
=> BT offload

This time with the new NHLT, ssp ports could be determined and dai links are created successfully but later SOF driver could not find endpoint descriptors blob when playback to speaker/headphone.

[ 242.272473] sof-audio-pci-intel-tgl 0000:00:1f.3: copier copier.SSP.2.1, type 27
[ 242.272475] sof-audio-pci-intel-tgl 0000:00:1f.3: sample rate: 48000 sample width: 32 channels: 2
[ 242.272477] sof-audio-pci-intel-tgl 0000:00:1f.3: dai index 0 nhlt type 3 direction 0
[ 242.272479] sof-audio-pci-intel-tgl 0000:00:1f.3: Looking for configuration:
[ 242.272481] sof-audio-pci-intel-tgl 0000:00:1f.3: vbus_id=0 link_type=3 dir=0, dev_type=0
[ 242.272482] sof-audio-pci-intel-tgl 0000:00:1f.3: ch=2 fmt=32/32 rate=48000
[ 242.272485] sof-audio-pci-intel-tgl 0000:00:1f.3: Endpoint count=8
[ 242.272486] sof-audio-pci-intel-tgl 0000:00:1f.3: Endpoint: vbus_id=0 link_type=2 dir=1 dev_type = 0
[ 242.272488] sof-audio-pci-intel-tgl 0000:00:1f.3: Endpoint: vbus_id=1 link_type=2 dir=1 dev_type = 0
[ 242.272490] sof-audio-pci-intel-tgl 0000:00:1f.3: Endpoint: vbus_id=0 link_type=3 dir=0 dev_type = 4
[ 242.272492] sof-audio-pci-intel-tgl 0000:00:1f.3: Endpoint: vbus_id=0 link_type=3 dir=1 dev_type = 4
[ 242.272493] sof-audio-pci-intel-tgl 0000:00:1f.3: Endpoint: vbus_id=1 link_type=3 dir=2 dev_type = 4
[ 242.272495] sof-audio-pci-intel-tgl 0000:00:1f.3: Endpoint: vbus_id=1 link_type=3 dir=3 dev_type = 4
[ 242.272497] sof-audio-pci-intel-tgl 0000:00:1f.3: Endpoint: vbus_id=2 link_type=3 dir=0 dev_type = 0
[ 242.272499] sof-audio-pci-intel-tgl 0000:00:1f.3: Endpoint: vbus_id=2 link_type=3 dir=1 dev_type = 0
[ 242.272515] sof-audio-pci-intel-tgl 0000:00:1f.3: no matching blob for sample rate: 48000 sample width: 32 channels: 2
[ 242.272519] sof-audio-pci-intel-tgl 0000:00:1f.3: failed to prepare widget copier.SSP.2.1

I checked the snd_sof_get_nhlt_endpoint_data() function and found it always calls intel_nhlt_get_endpoint_blob() with device type 0(bt sideband) so it couldn't returned endpoint blobs of headphone and speaker which device type is 4(ssp analog codec). Since we don't know the type in sof driver, we simply query the table multiple times to cover both bt and i2s device type.

Finally the board config in machine driver will be looking like this: SSP port numbers are removed and add the quirk flag SOF_TPLG_NHLT_SSP_DETECT to indicate that the number could be retrieve from topology NHLT.

{
.name = "adl_max98390_rt5682",
.driver_data = (kernel_ulong_t)(SOF_RT5682_MCLK_EN |
SOF_SPEAKER_AMP_PRESENT |
SOF_MAX98390_SPEAKER_AMP_PRESENT |
SOF_RT5682_NUM_HDMIDEV(4) |
SOF_SSP_BT_OFFLOAD_PRESENT |
SOF_TPLG_NHLT_SSP_DETECT),
},

7e3cf28 ALSA: hda: intel-nhlt: add intel_nhlt_ssp_dir_mask() function
=> a helper function for ipc4-topology
585b4d8 ASoC: SOF: ipc4-topology: get NHLT endpoints with codec device type
=> query NHLT endpoints with both bt(0) and I2s(4) and more directions (render with lookback, feedback for render),
c5fa18b ASoC: SOF: ipc4-topology: calculate ssp port number
=> detect ssp port number and register dai links in sof-topology layer
eb8d4f4 (HEAD -> nhlt_ssp_detect, origin/nhlt_ssp_detect) ASoC: Intel: sof_rt5682: support NHLT detection
=> machine driver change to use this detect feature in sof-topology

The code is testing on ADL brya board which kernel is Chrome-v6.1 and SOF is using main branch. I could create PR to alsatplg and topology if the idea of ssp detection is acceptable.

plbossart · 2023-07-26T06:23:57Z

Can you first share the topology and alsatplg changes @brentlu? I don't want to review kernel changes in isolation for what looks like a significant change in directions.

I have a bit of heartburn with the notion that the topology would include something related to the devices that are connected. It's blurring layers and pushing what should be in ACPI into topology. Yuck.

plbossart

that's a big change @brentlu. The NHLT change is controversial already, but the way you deal with dailinks is really problematic IMHO. dailinks should not be handled in the SOF driver.

plbossart · 2023-07-26T06:29:22Z

sound/soc/sof/ipc4-topology.c

 	u32 nhlt_type;
+	u8 nhlt_devices[] = {NHLT_DEVICE_I2S, NHLT_DEVICE_BT};


const and static?

plbossart · 2023-07-26T06:29:51Z

sound/soc/sof/ipc4-topology.c

@@ -1297,8 +1297,10 @@ static int snd_sof_get_nhlt_endpoint_data(struct snd_sof_dev *sdev, struct snd_s
 	struct sof_ipc4_fw_data *ipc4_data = sdev->private;
 	struct nhlt_specific_cfg *cfg;
 	int sample_rate, channel_count;
-	int bit_depth, ret;


commit message s/direcion/direction

plbossart · 2023-07-26T06:31:01Z

sound/soc/sof/ipc4-topology.c

+				goto cfg_found;
+		}
+	}
+cfg_found:


the main issue I have is that this is not backwards compatible. You need a new topology to get this to work. Is this not too late already?

@ranj063 please chime in here.

It's backward compatible. Because current implementation set the device type of all endpoints to 0 (BT), and use Render(0) and Capture(1) in direction field. So current topologies could work with the code.

plbossart · 2023-07-26T06:33:44Z

sound/soc/sof/ipc4-topology.c

@@ -2745,6 +2745,80 @@ static int sof_ipc4_dai_config(struct snd_sof_dev *sdev, struct snd_sof_widget *
 	return 0;
 }

+static int sof_ipc4_nhlt_register_dai_links(struct snd_soc_component *scomp)


adding dai links in the SOF ipc4 code is a major layering violation. This belongs in machine drivers IMHO. The SOF driver should only add CPU dais, not muck with dai links.

The problem is topology is loaded after machine driver registers soundcard. If we want to extract ssp numbers from NHLT, then we have to register the dai links in sof-topology layer...

it's a bit too much for me... I am not hot on hard-coding such names in a generic layer. need to think a bit more on all this.

plbossart · 2023-07-26T06:37:50Z

sound/soc/intel/boards/sof_rt5682.c

+		/* fixup and register this dai link later when we receive NHLT
+		 * table from IPC4 topology
+		 */
+		dai_link->ignore = true;


I don't know what this 'ignore' means but it certainly does not immediately translate as 'fixup and register later'.

soc-core(soc_check_tplg_fes() function) uses this flag to skip FE dai links in machine driver. Here we use this flag to make snd_soc_add_pcm_runtime() function skipping this dai link.

"borrow this flag" really means overloading a concept used for FE and extend it for BEs. That's not so good, this will add confusions left and right. We should be really careful with such moves.

I checked the soc.h and the flag means "do not create pcm". Maybe the reason to use this flag could be different? soc-core uses this flag to skip FE links and we use it to skip links which link/stream names need to be resolved later.

/* Do not create a PCM for this DAI link (Backend link) */ unsigned int ignore:1;

It's best to avoid using the same flag for two completely different purposes, which means adding a new one with a clearly defined behavior.

I am still in the dark on what the problem statement is and why the SOF driver is involved.

I agree we cannot repurpose an existing flag for a compltely different purpose

brentlu · 2023-07-26T06:40:09Z

Sure, there are three pull requests:

linux: #4492
alsa-utils: alsa-project/alsa-utils#226
topology: thesofproject/sof#7984

cujomalainey · 2023-07-26T18:14:28Z

I'm not sure I follow this, is this taking alsatplg and converting it into NHLT or using an existing NHLT in FW to influence SSP mappings and alsatplg just defines params and the pipe on top of that?

brentlu · 2023-07-27T00:56:30Z

It's using NHLT table inside the topology; not in coreboot/bios...

ranj063 · 2023-07-27T17:08:50Z

@brentlu I'm not following what the objective for this change is. Can you please start with whats wrong/missing with the current tooplogies, then provide the rationale for the change and finally what the solution is?

ranj063 · 2023-07-31T22:57:59Z

@brentlu Thank you for expanding the description with the rationale for the change. Now its a lot easier to see where you're coming from. But I have a few basic questions:

We have a chicken&egg problem with your proposal. In the current design, both the topology and the machine driver are self-sufficient wrt port numbers for each type of codec. But with your proposal, You have only partial information in the machine driver, so this means we have to split up the card registration in to 2 stages, one part during machine driver probe and the second during topology loading. Maybe we need to think more about how to do this a bit more elegantly.
This change would make the SOF driver incompatible with the current topologies that aren't encoded with the direction and device type attributes in the ssp blobs isnt it?
I think maybe we can take a different approach of reducing the burden of maintaing separate topologies where the same machine config with the same codecs require different topology files. We can use your idea of using the direction and device type to define the blobs for the codecs and make the blobc vbus_id agnostic but leave the driver data in the machine as is so that same topology file can be reused for products with similar machine configurations.

ranj063 · 2023-07-31T22:53:50Z

sound/soc/intel/boards/sof_rt5682.c

+		/* fixup and register this dai link later when we receive NHLT
+		 * table from IPC4 topology
+		 */
+		dai_link->ignore = true;


I agree we cannot repurpose an existing flag for a compltely different purpose

ranj063 · 2023-07-31T22:54:06Z

sound/soc/intel/boards/sof_rt5682.c

+		links[id].name = devm_kasprintf(dev, GFP_KERNEL,
+						"NHLT-RENDER-CAPTURE");
+	else
+		links[id].name = devm_kasprintf(dev, GFP_KERNEL, "SSP%d-Codec",


won't this break IPC3?

brentlu · 2023-08-07T08:06:40Z

@ranj063 Yes this change also require updating alsatplg and topology. I'm checking coreboot and realized NHLT support is already there for CNL and previous platforms. If we could enable the NHLT table in coreboot, then the change to sof-tplg will be unnecessary. We only need to read and parse NHLT table from machine driver.
Another question is about the direction field. Direction 2&3 is not implemented in coreboot for smartamps. Not sure the story behind this.

keqiaozhang · 2023-08-09T07:07:40Z

SOFCI TEST

This helper function returns bitmask of SSP ports which support specific direction according to endpoint descriptors. Smart amplifier's render feedback does not count when finding SSP ports with capture direction so caller could use this function to find out which port is for headset(both render and capture) and which port is for speakers(render only). Signed-off-by: Brent Lu <brent.lu@intel.com>

Currently we get the endpoint blob from NHLT which device type is BT Sideband(0). Here we expand the query to get the endpoint blob which device type could be SSP Analog Codec(4) or BT Sideband. Signed-off-by: Brent Lu <brent.lu@intel.com>

By checking the direction support of endpoints of each ssp port, we could conjecture which port is for headset and which port is for speakers. Here we assume headphone endpoints support both render and capture directions and speaker endpoints support render direction only. For BT sideband we could simply check the device type, BT Sideband is 0 and SSP Analog Codec is 4. Signed-off-by: Brent Lu <brent.lu@intel.com>

Add a quirk flag SOF_TPLG_NHLT_SSP_DETECT to detect the SSP port number and register dai links for codec, amplifier, and Bluetooth when receiving NHLT table in IPC4 topology. Signed-off-by: Brent Lu <brent.lu@intel.com>

brentlu requested review from lgirdwood, plbossart, ranj063, kv2019i, dbaluta, bardliao, ujfalusi, vijendarmukunda, kuanhsuncheng and yaochunhung as code owners July 26, 2023 04:04

brentlu requested review from cujomalainey, macchian and sathya-nujella July 26, 2023 04:07

brentlu mentioned this pull request Jul 26, 2023

topology: nhlt: intel: support more device types and directions alsa-project/alsa-utils#226

Closed

plbossart reviewed Jul 26, 2023

View reviewed changes

brentlu mentioned this pull request Jul 26, 2023

topology2: ssp: add quirks for NHLT table endpoints thesofproject/sof#7984

Closed

brentlu force-pushed the nhlt_ssp_detect branch from 58c2492 to eb8d4f4 Compare July 27, 2023 00:52

ranj063 reviewed Jul 31, 2023

View reviewed changes

brentlu marked this pull request as draft August 7, 2023 07:59

brentlu force-pushed the nhlt_ssp_detect branch from eb8d4f4 to 3a9fd36 Compare August 9, 2023 03:27

brentlu added 2 commits August 9, 2023 19:10

brentlu added 2 commits August 9, 2023 19:12

ASoC: Intel: sof_rt5682: support NHLT detection

3a9fd36

Add a quirk flag SOF_TPLG_NHLT_SSP_DETECT to detect the SSP port number and register dai links for codec, amplifier, and Bluetooth when receiving NHLT table in IPC4 topology. Signed-off-by: Brent Lu <brent.lu@intel.com>

brentlu mentioned this pull request Aug 15, 2023

Detect SSP port number from NHLT table - 2 #4524

Closed

brentlu closed this Sep 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect SSP port number from NHLT table #4492

Detect SSP port number from NHLT table #4492

brentlu commented Jul 26, 2023 •

edited

Loading

plbossart commented Jul 26, 2023

plbossart left a comment

plbossart Jul 26, 2023

brentlu Jul 27, 2023

plbossart Jul 26, 2023

brentlu Jul 27, 2023

plbossart Jul 26, 2023

brentlu Jul 26, 2023

plbossart Jul 26, 2023

brentlu Jul 26, 2023

plbossart Jul 26, 2023

plbossart Jul 26, 2023

brentlu Jul 26, 2023 •

edited

Loading

plbossart Jul 26, 2023

brentlu Jul 27, 2023

plbossart Jul 27, 2023

ranj063 Jul 31, 2023

brentlu commented Jul 26, 2023

cujomalainey commented Jul 26, 2023

brentlu commented Jul 27, 2023

ranj063 commented Jul 27, 2023 •

edited

Loading

ranj063 commented Jul 31, 2023 •

edited

Loading

ranj063 Jul 31, 2023

ranj063 Jul 31, 2023

brentlu commented Aug 7, 2023

keqiaozhang commented Aug 9, 2023

		u32 nhlt_type;
		u8 nhlt_devices[] = {NHLT_DEVICE_I2S, NHLT_DEVICE_BT};

Detect SSP port number from NHLT table #4492

Detect SSP port number from NHLT table #4492

Conversation

brentlu commented Jul 26, 2023 • edited Loading

plbossart commented Jul 26, 2023

plbossart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brentlu Jul 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brentlu commented Jul 26, 2023

cujomalainey commented Jul 26, 2023

brentlu commented Jul 27, 2023

ranj063 commented Jul 27, 2023 • edited Loading

ranj063 commented Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brentlu commented Aug 7, 2023

keqiaozhang commented Aug 9, 2023

brentlu commented Jul 26, 2023 •

edited

Loading

brentlu Jul 26, 2023 •

edited

Loading

ranj063 commented Jul 27, 2023 •

edited

Loading

ranj063 commented Jul 31, 2023 •

edited

Loading