ASoC: SOF: ipc3-topology: subtract ALH dai_index by 2 #5217

bardliao · 2024-10-24T09:20:27Z

We use SDW pin 2 as for ALH dai 0. However, IPC3 ALH dai index should start from 0.

lgirdwood

@bardliao do we need to state this is a fix in our commit message as some users SDW wont work without this patch ?

kv2019i

I think this is way to go. A few comments on the commit message and inline commets. I wonder if we could somehow also highlight that we have to support existing topology files, so renumbering (in Linux code) is not an option.

kv2019i · 2024-10-24T12:11:38Z

sound/soc/sof/ipc3-topology.c

+			if (flags & SOF_DAI_CONFIG_FLAGS_HW_PARAMS) {
+				/*
+				 * DAI 2 is used as the first DAI for SDW in topology, normalize
+				 * it to 0


Maybe refer to sof_sdw.c (something like "see SOC_SDW_INTEL_BIDIR_PDI_BASE in sof_sdw.c") defitinion to explain where the value "2" comes from?

Maybe refer to sof_sdw.c (something like "see SOC_SDW_INTEL_BIDIR_PDI_BASE in sof_sdw.c") defitinion to explain where the value "2" comes from?

Added. But on the second thought, this is Intel specific. Should we do it here?

Added sdw_bidir_base in sof_dev_desc struct. But I am not sure if it is the best place to add the base.

sound/soc/sof/ipc3-topology.c

ranj063 · 2024-10-24T18:56:16Z

sound/soc/sof/ipc3-topology.c

+		comp_dai->dai_index -= 2;
+		if (comp_dai->dai_index < 0) {
+			dev_err(sdev->dev,
+				"Invalid ALH dai index %d, Please use SDW Pin 2 and above\n",


maybe we can say that only Pin numbers > 1 are supported instead of saying please use Pin 2

maybe we can say that only Pin numbers > 1 are supported instead of saying please use Pin 2

Updated

bardliao · 2024-10-25T03:06:15Z

@bardliao do we need to state this is a fix in our commit message as some users SDW wont work without this patch ?

Yes, Fixes tag added.

kv2019i

Thanks @bardliao . All my concerns covered. I'll let other kernel folks comment on the sdw_bidir_base added -- not sure whether this is ok or not (it's always 2 now). It does make the logic more explicit, which I do like.

ranj063 · 2024-10-25T15:37:45Z

@bardliao I think we should squash both patches or at least split out the addition of the base field and setting the base into a seaprate patch first following by the next patch using it

bardliao · 2024-10-28T02:00:11Z

@bardliao I think we should squash both patches or at least split out the addition of the base field and setting the base into a seaprate patch first following by the next patch using it

Done

lgirdwood · 2024-10-28T13:36:03Z

@ujfalusi pls review. Thanks

ujfalusi · 2024-10-28T13:53:10Z

sound/soc/sof/ipc3-topology.c

+	/* Subtract the base to match the IPC3 FW dai index. */
+	if (comp_dai->type == SOF_DAI_INTEL_ALH) {
+		comp_dai->dai_index -= desc->sdw_bidir_base;
+		if (comp_dai->dai_index < 0) {


dai_index is uint32_t ...

dai_index is uint32_t ...

Thanks, move the check before subtracting.

ujfalusi · 2024-10-28T13:54:48Z

sound/soc/sof/intel/pci-icl.c

@@ -27,6 +27,7 @@ static const struct sof_dev_desc icl_desc = {
 	.resindex_pcicfg_base   = -1,
 	.resindex_imr_base      = -1,
 	.irqindex_host_ipc      = -1,
+	.sdw_bidir_base		= 2,


This is not really platform related, does it? It is IPC3 related, IPC4's bidir base is 0, right?

This is not really platform related, does it? It is IPC3 related, IPC4's bidir base is 0, right?

That is platform related. SDW bidirectional pins are the same on IPC3 and IPC4. But, IPC4 FW supports all SDW pins. So, we don't need to shift the dai index in IPC4.

Right, so, if you boot cAVS2.5 (TGL/ADL/etc) the sdw_bidir_base is applicable, but it is applicable only if you boot to use IPC3. Same platform, different IPC version.
sdw_bidir_base is not platform related option, it is IPC version specific, no?

or you are saying that with both IPC3/4 the topology's sdw bidir index starts at 2 and we don't need to normalize this in case of IPC4?

Or to put this the other way: On MTL+ the bidir_base is 0 in (IPC4) topology and it is 2 in (IPC4) cAVS2.5?

sdw_bidir_base will be applied to IPC3 only because dai_index 4, and 5 are acceptable by IPC4. As far as I know we always use 2+ SDW Pins even on MTL+ platforms.

you are not touching pci-mtl, pci-lnl, for those platforms the sdw_bidir_base is 0.

Why do you even need this in the struct sof_dev_desc when the subtraction is only applicable for INTEL_ALH with IPC3 and it is always applicable in this case?

Because AMD or other vendors may not need the offset.

If you are saying that this is somehow a platform variable and applicable to all Intel platforms, then you should set the correct value for all platforms that this applies.

Sure I will add sdw_bidir_base to all Intel platforms. We define SOC_SDW_INTEL_BIDIR_PDI_BASE 2 and apply it to all Intel platforms. But, that doesn't match to the HW capabilities. I am not sure about older platforms, but I checked TGL, MTL, and LNL, all SDW pins are bidirectional. You can check the HW capabilities by the dmesg log below.

soundwire_intel:intel_pdi_init: soundwire_intel soundwire_intel.link.0: PCM cap bd:12 in:0 out:0

AMD and other vendors don't use dai type SOF_DAI_INTEL_ALH?

Added sdw_bidir_base to all Intel platforms.

AMD and other vendors don't use dai type SOF_DAI_INTEL_ALH?

Oh, yes, that is a valid point. Then I think we don't need sdw_bidir_base.

ujfalusi · 2024-10-29T11:33:19Z

sound/soc/sof/ipc3-topology.c

-			if (flags & SOF_DAI_CONFIG_FLAGS_HW_PARAMS)
-				config->dai_index = data->dai_index;
+			if (flags & SOF_DAI_CONFIG_FLAGS_HW_PARAMS) {
+				/* Subtract the base to match the IPC3 FW dai index. */


The IPC3 is not needed, we are in ipc3-topology.c file anyways.

I think the comment should be somehow different, probably 'Convert the topology pin index to ALH dai index, the mapping is using 2-off indexing, iow, pin #2 is ALH dai #0' or around these lines?

This looks much cleaner already, imho.

…ndex Intel SoundWire machine driver always uses Pin number 2 and above. Currently, the pin number is used as the FW DAI index directly. As a result, FW DAI 0 and 1 are never used. That worked fine because we use up to 2 DAIs in a SDW link. Convert the topology pin index to ALH dai index, the mapping is using 2-off indexing, iow, pin #2 is ALH dai #0. The issue exists since beginning. And the Fixes tag is the first commit that this commit can be applied. Fixes: b66bfc3 ("ASoC: SOF: sof-audio: Fix broken early bclk feature for SSP") Signed-off-by: Bard Liao <[email protected]>

lgirdwood

@kv2019i good for you ?

bardliao requested review from ujfalusi and kv2019i October 24, 2024 09:20

bardliao requested review from lgirdwood, plbossart, ranj063 and dbaluta as code owners October 24, 2024 09:20

bardliao force-pushed the ipc3-alh-index branch from 04f1c53 to 71a1f8e Compare October 24, 2024 09:46

lgirdwood reviewed Oct 24, 2024

View reviewed changes

kv2019i reviewed Oct 24, 2024

View reviewed changes

ranj063 reviewed Oct 24, 2024

View reviewed changes

bardliao force-pushed the ipc3-alh-index branch from 71a1f8e to ee39ffe Compare October 25, 2024 03:00

bardliao force-pushed the ipc3-alh-index branch from ee39ffe to f8fca0e Compare October 25, 2024 06:16

lgirdwood previously approved these changes Oct 25, 2024

View reviewed changes

kv2019i reviewed Oct 25, 2024

View reviewed changes

bardliao force-pushed the ipc3-alh-index branch from f8fca0e to 5d37865 Compare October 28, 2024 01:59

ujfalusi reviewed Oct 28, 2024

View reviewed changes

ranj063 previously approved these changes Oct 28, 2024

View reviewed changes

bardliao dismissed stale reviews from ranj063 and lgirdwood via 93a27ba October 29, 2024 04:58

bardliao force-pushed the ipc3-alh-index branch 3 times, most recently from 78946f4 to c6e8d5e Compare October 29, 2024 11:22

ujfalusi reviewed Oct 29, 2024

View reviewed changes

bardliao force-pushed the ipc3-alh-index branch from c6e8d5e to 8f0b1cc Compare October 29, 2024 11:51

ujfalusi approved these changes Oct 29, 2024

View reviewed changes

lgirdwood approved these changes Oct 29, 2024

View reviewed changes

kv2019i approved these changes Oct 29, 2024

View reviewed changes

ranj063 approved these changes Oct 29, 2024

View reviewed changes

ranj063 merged commit 3592316 into thesofproject:topic/sof-dev Oct 29, 2024
12 of 14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ASoC: SOF: ipc3-topology: subtract ALH dai_index by 2 #5217

ASoC: SOF: ipc3-topology: subtract ALH dai_index by 2 #5217

bardliao commented Oct 24, 2024

lgirdwood left a comment

kv2019i left a comment

kv2019i Oct 24, 2024

bardliao Oct 25, 2024

bardliao Oct 25, 2024

ranj063 Oct 24, 2024

bardliao Oct 25, 2024

bardliao commented Oct 25, 2024

kv2019i left a comment

ranj063 commented Oct 25, 2024

bardliao commented Oct 28, 2024

lgirdwood commented Oct 28, 2024

ujfalusi Oct 28, 2024

bardliao Oct 29, 2024

ujfalusi Oct 28, 2024

bardliao Oct 29, 2024

ujfalusi Oct 29, 2024

ujfalusi Oct 29, 2024 •

edited

Loading

bardliao Oct 29, 2024

bardliao Oct 29, 2024

ujfalusi Oct 29, 2024

bardliao Oct 29, 2024

bardliao Oct 29, 2024

bardliao Oct 29, 2024

ujfalusi Oct 29, 2024 •

edited

Loading

lgirdwood left a comment

ASoC: SOF: ipc3-topology: subtract ALH dai_index by 2 #5217

ASoC: SOF: ipc3-topology: subtract ALH dai_index by 2 #5217

Conversation

bardliao commented Oct 24, 2024

lgirdwood left a comment

Choose a reason for hiding this comment

kv2019i 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

bardliao commented Oct 25, 2024

kv2019i left a comment

Choose a reason for hiding this comment

ranj063 commented Oct 25, 2024

bardliao commented Oct 28, 2024

lgirdwood commented Oct 28, 2024

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

ujfalusi Oct 29, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ujfalusi Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

lgirdwood left a comment

Choose a reason for hiding this comment

ujfalusi Oct 29, 2024 •

edited

Loading

ujfalusi Oct 29, 2024 •

edited

Loading