topology2: ssp: Add blob version #7363

ranj063 · 2023-04-03T20:38:13Z

Add the SSP blob version and set it based on the platform.

This is a replacement for #7060

Fixes: #6837

aiChaoSONG

Changes look good and simpler than #7060.

the word version maybe too general, I think blob_version maybe better, but no big deal.

plbossart · 2023-04-04T13:38:54Z

tools/topology/topology2/platform/intel/tgl.conf

@juimonen can you remind us if the AUX TLV is part of the SSP blob 1.00 or 1.05? I wonder why would need to maintain a difference here.

Why not use 1.05 always?

@plbossart I think blob version 1.00 for TGL/ADL is what makes it possible for the alsatplg compiler to ignore the aux parameters that include the link clock_source

Only MTL newer platforms support aux. If you send a v1.05 blob to cAVS2.5 SOF SW, you'll get errors.

@kv2019i I don't think that's true any longer with the change suggested by @ranj063 to make zephyr deal with aux in all cases, and do nothing for TGL.

@kv2019i see zephyrproject-rtos/zephyr#56497

@plbossart if we always used blob version 1.05, the problem is that the ipc tester firmware might fail.

I think the SSP blob 1.05 was added before TGL. @mmaka1 @mwasko can you please confirm?

tools/topology/topology2/common_definitions.conf

kv2019i

Not entirely sure how well this aligns with #7370 , but put these two together seems to be a clean solution. Approving at least this one.

tools/topology/topology2/common_definitions.conf

kv2019i · 2023-04-05T12:38:11Z

tools/topology/topology2/platform/intel/tgl.conf

Only MTL newer platforms support aux. If you send a v1.05 blob to cAVS2.5 SOF SW, you'll get errors.

Add the SSP blob version and set it based on the platform. Co-developed-by: Jaska Uimonen <jaska.uimonen@intel.com> Signed-off-by: Jaska Uimonen <jaska.uimonen@intel.com> Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Move and rename the hw_config class to platform/intel/hw_config_cardinal_clk.conf. This hw_config is specific to Intel SSP DAI that use the cardinal clock for the mclk frequency. Ideally, this class should have immutable mclk_frequency and link clock source. But because the alsatplg compiler expects the name of the hw config class to be "hw_config" without any extensions, it is left as modifiable for now. Once the topology compiler is modified, this will be made immutable in a follow up PR. Also, introduce a new hw_config_simple.conf file that contains the hw_config definition for the HDA/DMIC/SDW type DAIs with only the ID and name attributes. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ranj063 · 2023-04-05T18:05:56Z

@plbossart @kv2019i @lgirdwood I got to the root of the problem:

There's a strict restriction wrt the alsatplg compiler which requires the "Object.Base.link_config.1" to be defined right within the Object.Base.hw_config.N{} node. This is wrong and should be fixed in the compiler so that the Object definition in hw_config_cardinal_clock.conf will work as expected.
blob version 0x100 is required for TGL with the above change so that the clock_source setting will be ignored.
Alternatively, if we set 0x105 for TGL, we need the zephyr patch drivers: ssp: Ignore link clock_source for non-ACE platforms zephyrproject-rtos/zephyr#56497

In conclusion, I think this is what we should do. Merge this PR so that we can unblock the alsabat tests in CI. Then work on fixing the alsatplg to allow link_config to come from the class definition instead of the object instance. Finally after the zephyr patch is merged, set the blob type to 0x105 for all platforms.

Let me know what you think.

kv2019i

One comment inline about BT loopback topologies. Otherwise this looks like a clean approach.

kv2019i · 2023-04-05T18:06:15Z

tools/topology/topology2/platform/intel/bt-ssp-config.conf

 			tx_slots	3
 			rx_slots	0
+			Object.Base.link_config.1 {
+				clock_source	0


Should this be the cardinal clock (i.e. "1"), so we can use integer divider for 1.536Mhz?
UPDATE: no, this is ok, clouc source 0 is 38Mhz, so this is ok.

no should 0 for xtal as xtal is 38.4 and 38.4M/1.536M = 25

@kv2019i @ranj063 if the BT frequencies are 256kHz, 1536kHz then we could use either xtal or audio cardinal clocks.
It's only for the 2.4MHz case that we would have a problem, but it's not used.

I am now wondering if we actually have a case where the audio cardinal clock does not work?

@kv2019i I will let you answer this question and follow up with another PR if we need to change the clock source for BT. For now, it actually makes no difference since we dont build the topology for MTL

tools/topology/topology2/platform/intel/bt-ssp-config-lbm.conf

fredoh9 · 2023-04-05T18:56:49Z

I check this PR with MTL nocodec device, it generates solid waveform, alsabat test consistently passes!

plbossart

See comment below, with the existing BT configurations should we always set the link source to the audio cardinal clock?

plbossart · 2023-04-05T20:09:41Z

tools/topology/topology2/platform/intel/bt-ssp-config.conf

 			tx_slots	3
 			rx_slots	0
+			Object.Base.link_config.1 {
+				clock_source	0


@kv2019i @ranj063 if the BT frequencies are 256kHz, 1536kHz then we could use either xtal or audio cardinal clocks.
It's only for the 2.4MHz case that we would have a problem, but it's not used.

I am now wondering if we actually have a case where the audio cardinal clock does not work?

ranj063 requested a review from jsarha as a code owner April 3, 2023 20:38

ranj063 requested review from juimonen, kv2019i and plbossart April 3, 2023 20:39

ranj063 force-pushed the fix/ssp_link_clock branch 2 times, most recently from bbe4716 to 3772582 Compare April 3, 2023 21:24

ranj063 mentioned this pull request Apr 4, 2023

[BUG] distortion during playback on MTL-NOCODEC #6837

Closed

aiChaoSONG approved these changes Apr 4, 2023

View reviewed changes

plbossart reviewed Apr 4, 2023

View reviewed changes

lgirdwood reviewed Apr 5, 2023

View reviewed changes

tools/topology/topology2/common_definitions.conf Outdated Show resolved Hide resolved

kv2019i approved these changes Apr 5, 2023

View reviewed changes

topology2: ssp: Add blob version

2a9d7f7

Add the SSP blob version and set it based on the platform. Co-developed-by: Jaska Uimonen <jaska.uimonen@intel.com> Signed-off-by: Jaska Uimonen <jaska.uimonen@intel.com> Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ranj063 force-pushed the fix/ssp_link_clock branch from 3772582 to 0fe1a5d Compare April 5, 2023 15:12

ranj063 mentioned this pull request Apr 5, 2023

topology2: Move the hw_config class definition #7370

Closed

ranj063 force-pushed the fix/ssp_link_clock branch from 0fe1a5d to 88fc764 Compare April 5, 2023 18:01

kv2019i reviewed Apr 5, 2023

View reviewed changes

plbossart approved these changes Apr 5, 2023

View reviewed changes

ranj063 merged commit 650423f into thesofproject:main Apr 5, 2023

ranj063 deleted the fix/ssp_link_clock branch April 5, 2023 20:32

topology2: ssp: Add blob version #7363

topology2: ssp: Add blob version #7363

Uh oh!

Conversation

ranj063 commented Apr 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aiChaoSONG left a comment • 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!

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!

ranj063 Apr 5, 2023 • 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!

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranj063 commented Apr 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

kv2019i Apr 5, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fredoh9 commented Apr 5, 2023

Uh oh!

plbossart left a comment

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

ranj063 commented Apr 3, 2023 •

edited

Loading

aiChaoSONG left a comment •

edited

Loading

ranj063 Apr 5, 2023 •

edited

Loading

ranj063 commented Apr 5, 2023 •

edited

Loading

kv2019i Apr 5, 2023 •

edited

Loading