add support of tdm setting for DSM feature #2178

RanderWang · 2020-06-09T07:33:11Z

Max98373 supports two-channels playback and two-channels capture. Now enable two channels by setting rx|tx_mask to 0x3 and user uses mixer setting to select channel for each codec.

fixes #2169

bardliao · 2020-06-09T09:50:36Z

sound/soc/intel/boards/sof_sdw_max98373.c

+	/*
+	 * enable two channels and use mixer control 'DAI Sel Mux'
+	 * to select channel for each codec
+	 */


Maybe add a comment to describe that we assume all codecs are max98373?

plbossart

@RanderWang this sounds fine for playback, but for capture I am not sure at all that we can use the same TX slot configuration for both amps. see below.
Marking as 'Request Changes' to make sure this doesn't get merged before we agree on this.

plbossart · 2020-06-09T13:12:49Z

sound/soc/intel/boards/sof_sdw_max98373.c

+	 * to select channel for each codec
+	 */
+	for_each_rtd_codec_dais(rtd, i, codec_dai)
+		snd_soc_dai_set_tdm_slot(codec_dai, 0x3, 0x3, 2, 32);


Hang on, there could be a problem here.
The idea of using TDM slots is that when the slot configuration is the same, all amplifiers would receive the same data from the same slots and extract it
For capture, you cannot use the same slots for the two amplifiers, that would lead to a bus clash.
Can you clarify @RanderWang what setting the same TX mask for the amplifiers? How does this impact the bit allocation for the Source port of the amplifiers?

@plbossart a dumb question here. The other PR that was recently approved had similar TDM slot settings too. Is the problem you're point out SDW specific?

I meant this one: #2172

@ranj063 the RX mask was 0x3 for both amps, but the TX mask was different (0xC and 0x3) respectively. Same thing, you can extract data from common slots, you cannot drive the same slots.

Ahh I see. makes sense. Thanks @plbossart

Hang on, there could be a problem here.
The idea of using TDM slots is that when the slot configuration is the same, all amplifiers would receive the same data from the same slots and extract it
For capture, you cannot use the same slots for the two amplifiers, that would lead to a bus clash.
Can you clarify @RanderWang what setting the same TX mask for the amplifiers? How does this impact the bit allocation for the Source port of the amplifiers?

The DSM usage is: four channel capture stream and each amp generates two channels of IV. I first tried 0xc and 0x3, but only left channel feedback data was returned. I struggled at here for a long time until I tested both 0x3. I got reply from @ryans-lee by email. please check his reply:

" I found ‘tx_mask’ is directly applied to 0x320 register which is Soundwire DP3_ChannelEn in ‘sdw_enable_disable_slave_ports’ function.
Each amp uses channel1 and channel2 for I and V respectively, so this value needs to be 3.
0x3(0y00000011) and 0xc(0y00001100) were for 8 slot TDM bitmask on I2S and this is no longer valid for Soundwire. In Soundwire, slot allocation is done by 0x324, DP offsetctrl register. Offset configuration is done automatically in ‘sdw_compute_slave_ports'. ‘stream_config.ch_count' in ‘max98373_sdw_dai_hw_params' is used to calculate the offset.
This was 4, so offset was calculated to 0x91. Now channel size become 2, so offset is changed to 0x61 which is good value. "

My idea is: the two amp will work on different DP offset based on ch_mask in sdw_compute_slave_ports, so no clash. But 0x3 & 0xc also can generate correct DP offset as 0x3 & 0x3 in sdw_compute_slave_ports, the only different is at function sdw_enable_disable_slave_ports. I found both slave should be set 0x3 to channel enable mask. Please correct me.

kv2019i

Patch looks fine, but I'll postpone approval until consensus is reached on TDM mask semantics for SDW (and DP offsetctrl).

plbossart

@RanderWang see comments below, I don't think we need the concept of TDM slots for capture. We do need to set the ch_mask though for capture, that's a miss in the current code

plbossart · 2020-06-10T18:24:41Z

sound/soc/codecs/max98373-sdw.c

+		if (direction == SDW_DATA_DIR_RX)
+			port_config.ch_mask = max98373->rx_mask;
+		else
+			port_config.ch_mask = max98373->tx_mask;


@RanderWang I don't see the point of using the tx_mask here. You could just as well use

if (max98373->slot) { stream_config.ch_count = max98373->slot; if (direction == SDW_DATA_DIR_RX) port_config.ch_mask = max98373->rx_mask; else port_config.ch_mask = GENMASK(stream_config.ch_count - 1, 0);

Please re-test, but I would suspect this works fine because the channel count for the port is not equal to the stream channel count, and we will use a different offset for all ports (unlike for the amplifier mirror case)

In contrast to the I2S/TDM case, there is nothing in SoundWire that mandates that we use the same number of channels for playback and capture, so we should really only use set_tdm for playback and leave the hw_params decide what happens for capture.

@plbossart is it " we should really only use set_tdm for capture and leave the hw_params decide what happends for playback ? I tested this case. we capture stream with ch = 4 so we need to set tdm slot = 2 for each codec, but for playback ch is always 2. I use set_tdm for playback just for unification.

My point was: don't use tdm_slot for capture. It's not useful. use the hw_params for capture, it should be just fine.

@plbossart Capture can work without tdm_slot for its ch_mask is alway 0x3 (I & V channel). But the ch count in hw_params is 4ch, so it can't be used for capture. And we also don't need tdm slot for playback since we have mirror mode support. Please check my patch.

I can't follow your explanations, too many negatives and contradicting arguments. Please re-explain what works and what does not.

please check the latest PR. Now we don't need to set tdm in our machine driver. For original code in capture case, params_channels(params) = 4, so we need tdm slot to constrain it to 2 for each codec.

And for playback, we go with mirror mode and set mixer to choose input channel. And Now we also do with mixer for two 1308-sdw in one link, no tdm setting.

For DSM usage: we do with aplay -c 2 xxx.wav & arecord -c 4 xxx.wav. In original codec for playback, ch = 2, mask = 0x3, the same as rx_mask in tdm setting, so we can skip it. And for capture, we need set tx_mask = 0x3 to constrain ch_mask.

plbossart · 2020-06-11T14:49:24Z

@RanderWang it's simpler if I provide the following patch to explain my points.

0001-ASoC-codec-max98373-sdw-refine-tdm-support.patch.txt

We only use tdm_slots for playback as required by the machine driver. For the capture the number of channels is fixed to two and the tdm_slot is irrelevant. Signed-off-by: Ryan Lee <ryans.lee@maximintegrated.com> Signed-off-by: Rander Wang <rander.wang@intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

RanderWang · 2020-06-12T01:16:58Z

@RanderWang it's simpler if I provide the following patch to explain my points.

0001-ASoC-codec-max98373-sdw-refine-tdm-support.patch.txt

It is cool, thanks! tested on TGL and updated my PR.

lyakh

the comment is minor, maybe it isn't needed indeed

lyakh · 2020-06-12T09:25:28Z

sound/soc/codecs/max98373-sdw.c


-	if (!tx_mask && !rx_mask && !slots && !slot_width)
+	/* tx_mask is ignored since it's irrelevant for I/V feedback */
+	if (!rx_mask && !slots && !slot_width)


would it not make sense to print a debug message if someone is trying to set tx_mask != 0?

good point @lyakh , for rt1308-sdw we used this:

int set_tdm_slot() { .... if (tx_mask) return -EINVAL; ... }

so maybe use the same solution for consistency?

@RanderWang maybe you can add this as a fixup! patch, i'd like to merge this to unlock the issue #2169. The additional check is does not add any functional value but is nice to have indeed, a really good candidate for a fixup.

done, please check #2202

RanderWang requested review from dbaluta, kv2019i, lgirdwood, plbossart and ranj063 as code owners June 9, 2020 07:33

RanderWang requested review from bardliao and naveen-manohar June 9, 2020 07:33

RanderWang mentioned this pull request Jun 9, 2020

Max98373 SDW review #2062

Merged

bardliao reviewed Jun 9, 2020

View reviewed changes

plbossart requested changes Jun 9, 2020

View reviewed changes

kv2019i reviewed Jun 10, 2020

View reviewed changes

plbossart requested changes Jun 10, 2020

View reviewed changes

RanderWang force-pushed the volteer-tdm branch from 5f7c7f3 to 2c2b6d6 Compare June 11, 2020 00:52

RanderWang force-pushed the volteer-tdm branch from 2c2b6d6 to 1a48ad0 Compare June 12, 2020 01:15

YvonneYang2 mentioned this pull request Jun 12, 2020

[BUG][TGL]two sides amplifiers both output “front left” and no amplifier output "front Right" when do test speaker test on speaker #2169

Closed

lyakh approved these changes Jun 12, 2020

View reviewed changes

plbossart approved these changes Jun 12, 2020

View reviewed changes

plbossart merged commit 5fa59c1 into thesofproject:topic/sof-dev Jun 12, 2020

RanderWang mentioned this pull request Jun 16, 2020

fixup: ASoC: codec: max98373: return error if tx_mask is set #2202

Merged

add support of tdm setting for DSM feature #2178

add support of tdm setting for DSM feature #2178

Uh oh!

Conversation

RanderWang commented Jun 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RanderWang Jun 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RanderWang Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plbossart commented Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RanderWang commented Jun 12, 2020

Uh oh!

lyakh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

RanderWang commented Jun 9, 2020 •

edited

Loading

RanderWang Jun 10, 2020 •

edited

Loading

RanderWang Jun 11, 2020 •

edited

Loading

plbossart commented Jun 11, 2020 •

edited

Loading