topology2: ssp: add link clock aux data for mtl #7060

juimonen · 2023-02-08T08:22:07Z

Add conditional link clock settings for mtl ssp using auxiliary data in the blob generation.

Signed-off-by: Jaska Uimonen jaska.uimonen@intel.com

lgirdwood

tools/topology/topology2/platform/intel/ssp-default.conf

plbossart

seems a bit intrusive an repetitive @juimonen, can we not have a mtl default with the values added instead of changing all topologies.

plbossart · 2023-02-08T15:12:59Z

tools/topology/topology2/cavs-nocodec-bt.conf

can we not have some sort of enum that is self explanatory or a comment. This is very hard to review.

I'd not pollute the hw_config class with NHLT-specific stuff at all. The clock_source is a SSP-specific value that can be set in ssp.conf based on PLATFORM definition

what i'd think you need is something like this:

diff --git a/tools/topology/topology2/include/dais/ssp.conf b/tools/topology/topology2/include/dais/ssp.conf index 9c73e7562..99fc9024e 100644 --- a/tools/topology/topology2/include/dais/ssp.conf +++ b/tools/topology/topology2/include/dais/ssp.conf @@ -111,6 +111,25 @@ Class.Dai."SSP" { # platform clock frequency DefineAttribute.io_clk {} + DefineAttribute."clock_source" { + constraints { + !valid_values [ + "XTAL" + "CARDINAL" + "PLL" + "MCLK" + "WOV_CRO" + ] + !tuple_values [ + 0 + 1 + 2 + 3 + 4 + ] + } + } + attributes { !constructor [ "name" @@ -140,4 +159,10 @@ Class.Dai."SSP" { frame_pulse_width 0 tdm_padding_per_slot false version $SSP_BLOB_VERSION + + IncludeByKey.PLATFORM { + "mtl" { + clock_source PLL + } + } }

IncludeByKey.PLATFORM {

"mtl" {

clock_source PLL

I hope we are using the 24.576MHz cardinal clock that comes for free and not the PLL? That index 2 seems completely wrong...

@plbossart I could be wrong with the indices, I was just giving an example. @juimonen please correct me

all this is good and dandy, but it would require me to rewrite/move the alsa-utils auxdata parser

We can debate about the sw-philosophical issues for a long time... So ssp blob is created per hw_config, like in BT we have 3 different hw_configs, we create 3 different blobs into nhlt. Aux data is embedded by the spec after the blob. So if the aux data is moved into ssp level, it would mean that all blobs would need a copy of the same aux data. I understand that in the link clock case this could be fine, but link clock is not the only aux data item. So if someone can guarantee me that any aux data items will not be different in any hw_configs per ssp, the move to ssp level would be justified. Otherwise I see the hw_config quite logical place for it.

I will repeat my suggestion to ALWAYS provide the AUX information with the clock source, and have the firmware discard what it cannot understand.

The point is that if you select a bitclock that divided from 24.576 MHz then you have to select the clock_source 1. If you derive the bitclock from 19.2 then clock_source 0 it is.

There's no differences between platforms, the mapping between clock sources is always the same, and we only have to select between 0 and 1 for now.

In other words, rather than keep this optional make it a mandatory one-size-fits-all.

plbossart · 2023-02-08T15:15:44Z

tools/topology/topology2/cavs-nocodec.conf

do we really want to add this everywhere, when this is clearly a SOC-specific configuration that should be default in all topologies?

This should be part of a platform definition, not in all topologies IMHO.

@juimonen can we take this from platform.conf i.e. using the same method as SSP_BLOB_VERSION above ? This could be an attribute in teh base class that would default to PLATFORM_CLOCK_blah_DEFAULT ?

I don't currently know a way to add this to all hw_configs in a certain platform in some other way than this

tools/topology/topology2/cavs-nocodec-bt.conf

juimonen · 2023-02-08T15:24:42Z

seems a bit intrusive an repetitive @juimonen, can we not have a mtl default with the values added instead of changing all topologies.

yes agreed. @ranj063 any idea how to add this only once based on platform?

lgirdwood · 2023-02-13T12:46:55Z

seems a bit intrusive an repetitive @juimonen, can we not have a mtl default with the values added instead of changing all topologies.

yes agreed. @ranj063 any idea how to add this only once based on platform?

Dont we have a platform file that can be included like in topology1 ? if not, please add one.

ranj063 · 2023-02-13T18:48:29Z

Dont we have a platform file that can be included like in topology1 ? if not, please add one

@lgirdwood yes we do. There's mtl.conf that we can add this to

kv2019i · 2023-02-17T07:18:50Z

@juimonen array-support merged in #7113 so rebase will be needed

Add conditional link clock settings for mtl ssp using auxiliary data in the blob generation. Signed-off-by: Jaska Uimonen <jaska.uimonen@intel.com>

lgirdwood · 2023-02-21T16:06:56Z

@juimonen still RFC ?

juimonen · 2023-02-21T16:21:02Z

@juimonen still RFC ?

I want to see the CI after rebase (zephyr update) and there's still topology "cleanup" issues to be discussed...

juimonen · 2023-03-08T10:02:04Z

@ranj063 @plbossart we need to figure this out, related code is merged and used in alsa-utils and zephyr ssp driver. This is a working solution, but if you want this cleaner, I need some guidance how to proceed. I would really like to get this for 2.5...

lgirdwood · 2023-03-13T21:16:01Z

@ranj063 @plbossart we need to figure this out, related code is merged and used in alsa-utils and zephyr ssp driver. This is a working solution, but if you want this cleaner, I need some guidance how to proceed. I would really like to get this for 2.5...

@juimonen Looks like we need the clock source enum and the mtl.conf

kv2019i · 2023-03-30T11:47:05Z

@juimonen Can you check how this could be moved to mtl.conf and evaluate the effort? I understand the concern that this bit now is needed in every topology used with mtl, and would b ebetter to have this in one place.

@plbossart @lgirdwood Ok to merge this now if Jaska commits to making a follow-up change. We really do need this change, we are getting corruption in audio signal without and this could mask all kinds of other things.

juimonen · 2023-03-30T13:01:55Z

probably people would like something like this: https://github.com/juimonen/sof/tree/aux_test

problem is that it just doesn't work with alsa-utils and I kind of don't know why.

ranj063

im OK to merge this to unblock certain tests. We can work on beautifying it later

ranj063 · 2023-04-04T16:06:23Z

@lgirdwood @kv2019i lets not merge this one just yet. I have alternative PRs for this

sys-pt1s · 2023-04-06T10:18:56Z

Can one of the admins verify this patch?

kv2019i · 2023-04-24T09:25:27Z

@juimonen @ranj063 Close this one as a different solution got merged...?

juimonen requested review from dbaluta, lbetlej, lgirdwood, mmaka1 and plbossart as code owners February 8, 2023 08:22

lgirdwood reviewed Feb 8, 2023

View reviewed changes

tools/topology/topology2/platform/intel/ssp-default.conf Outdated Show resolved Hide resolved

plbossart reviewed Feb 8, 2023

View reviewed changes

topology2: ssp: add link clock aux data for mtl

d155d70

Add conditional link clock settings for mtl ssp using auxiliary data in the blob generation. Signed-off-by: Jaska Uimonen <jaska.uimonen@intel.com>

juimonen force-pushed the mtl_link_clock branch from 08cc93a to d155d70 Compare February 21, 2023 15:50

juimonen requested a review from jsarha as a code owner February 21, 2023 15:50

juimonen mentioned this pull request Mar 7, 2023

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

Closed

juimonen changed the title ~~[RFC] topology2: ssp: add link clock aux data for mtl~~ topology2: ssp: add link clock aux data for mtl Mar 8, 2023

ranj063 approved these changes Mar 31, 2023

View reviewed changes

ranj063 mentioned this pull request Apr 3, 2023

topology2: ssp: Add blob version #7363

Merged

lgirdwood approved these changes Apr 4, 2023

View reviewed changes

ranj063 mentioned this pull request Apr 4, 2023

topology2: Move the hw_config class definition #7370

Closed

juimonen closed this Apr 24, 2023

topology2: ssp: add link clock aux data for mtl #7060

topology2: ssp: add link clock aux data for mtl #7060

Uh oh!

Conversation

juimonen commented Feb 8, 2023

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

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!

ranj063 Feb 9, 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!

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!

juimonen commented Feb 8, 2023

Uh oh!

lgirdwood commented Feb 13, 2023

Uh oh!

ranj063 commented Feb 13, 2023

Uh oh!

kv2019i commented Feb 17, 2023

Uh oh!

lgirdwood commented Feb 21, 2023

Uh oh!

juimonen commented Feb 21, 2023

Uh oh!

juimonen commented Mar 8, 2023

Uh oh!

lgirdwood commented Mar 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kv2019i commented Mar 30, 2023

Uh oh!

juimonen commented Mar 30, 2023

Uh oh!

ranj063 left a comment

Choose a reason for hiding this comment

Uh oh!

ranj063 commented Apr 4, 2023

Uh oh!

sys-pt1s commented Apr 6, 2023

Uh oh!

kv2019i commented Apr 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ranj063 Feb 9, 2023 •

edited

Loading

lgirdwood commented Mar 13, 2023 •

edited

Loading