Skip to content

Conversation

@ranj063
Copy link
Collaborator

@ranj063 ranj063 commented Apr 4, 2023

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.

This is the replacement for the second part of #7060 to set the link clock source

@ranj063 ranj063 requested a review from jsarha as a code owner April 4, 2023 22:36
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not following how we can have the same class name for two different definitions, here and 07ffe5d#diff-6097d83c177264e03795b3cb083915a0153a69fe445ea6046004160e75209628R15

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is alsatplg compiler restriction for now. We can only include one or the other so we have only one class definition included. so the class name is the same "hw_config" because thats what the compiler expects

@gkbldcig
Copy link
Collaborator

gkbldcig commented Apr 4, 2023

Can one of the admins verify this patch?

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>
Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still not following how this handles the differences in SSP blob version, With v1, we want to specify mclk and let driver pick the source, with v1.5, we want to specify clock source in tplg and pass that. But we need to cover both options.

Could we have separate versions of hw_config_cardinal_clk.conf for these two cases? link-config definitions would always be in e.g. bt-ssp-config.*conf, but ignored on v1.5 blob platforms.

UPDATE: or hmm, or should this go together with #7363 ? that would explain how the blob verssion is set.

rx_slots 1
Object.Base.link_config.1 {
clock_source 0
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the blob version setting missing? This works on cAVS2.5 now, as blob parsing ignores the aux blob and driver selects the clock source by itself. If we'd set blob version to v1.5 (0x105), parsing would fail on cAVS2.5 platforms.

#
# where NAME is the unique instance name for the hw_config object
# within the same alsaconf node.
# where 1 is the unique instance ID for the hw_config object within the same alsaconf
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use a number when a name is more descriptive ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lgirdwood all objects need to have instance so that we can use arrays.

@plbossart
Copy link
Member

@kv2019i the main point is that the value of the MCLK and the link source have to be selected together. You cannot select the audio cardinal clock of 24.576 MHz and use link_source 0 (xtal). That's broken by design. The main idea is to bundle together fields that need to be handled consistently.

@ranj063
Copy link
Collaborator Author

ranj063 commented Apr 5, 2023

clubbed with #7363. Closing this one

@ranj063 ranj063 closed this Apr 5, 2023
@ranj063 ranj063 deleted the fix/hw_config branch April 5, 2023 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants