Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

topology2: cavs-nocodec: include SSP1 pipelines conditionally #7239

Merged
merged 1 commit into from
Mar 29, 2023

Conversation

aiChaoSONG
Copy link
Collaborator

There is pinmux conflict between SSP1 and DMIC on MTL, as there are other SSPs can be used, we don't include SSP1 pipelines only for mtl to unlock DMIC test.

@aiChaoSONG
Copy link
Collaborator Author

This patch will require alsatplg change in alsa-project/alsa-utils#195

@lgirdwood
Copy link
Member

@jsarha @ranj063 any comments ?

@aiChaoSONG aiChaoSONG force-pushed the mtl_ssp1 branch 2 times, most recently from e2cf2b8 to 895db65 Compare March 15, 2023 06:06
@aiChaoSONG
Copy link
Collaborator Author

Updates:

@aiChaoSONG aiChaoSONG marked this pull request as ready for review March 15, 2023 06:08
@aiChaoSONG
Copy link
Collaborator Author

@ranj063 @ujfalusi @kv2019i could you please help to give this PR a review?

@plbossart
Copy link
Member

@ranj063 @ujfalusi @kv2019i could you please help to give this PR a review?

the dependency on alsa-project/alsa-utils#195 prevents any merge from happening. Why are you asking for approvals @aiChaoSONG if there is no agreement on the dependencies and plan to update the docker image?

@kv2019i
Copy link
Collaborator

kv2019i commented Mar 21, 2023

@plbossart wrote:

the dependency on alsa-project/alsa-utils#195 prevents any merge from happening. Why are you asking for approvals @aiChaoSONG if there is no agreement on the dependencies and plan to update the docker image?

I think the alsa-utils #195 dependency is now removed. In this https://github.com/thesofproject/sof/compare/b0b1be1251048b57ce1488085181048b1cdd3aca..e2cf2b866c925859e5db9f827a947c2f425d65c6

}

# override defaults with platform-specific config
IncludeByKey.PLATFORM {
"tgl" "platform/intel/tgl.conf"
"adl" "platform/intel/tgl.conf"
Copy link
Member

@plbossart plbossart Mar 21, 2023

Choose a reason for hiding this comment

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

I am having a bit of a heartburn with this PR.

Is the pinmux is a problem on one instance of the MTL RVP, or the MTL SOC in general? It's got to be the former, there is not general incompatibility between SSP1 and DMIC, it's just that the configuration of ONE board that has a restriction.

So rather than having a blanket removal for SSP1, we should only remove SSP1 on specific targets, and use udev rules or a tplg_name kernel option to work-around this limitation.

Copy link
Collaborator Author

@aiChaoSONG aiChaoSONG Mar 22, 2023

Choose a reason for hiding this comment

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

@plbossart Let's extinguish the fire that burns your heart, how about generating two topology targets, see updated code.

  • sof-mtl-nocodec-ssp0-ssp2.tplg, use this one for MTL RVP, only SSP0 and SSP2 are enabled
  • sof-mtl-nocodec.tplg, use this for other MTL device on which all SSPs are enabled.

I think this should address your concern that there are boards have/haven't the pin-mux issue. And configurations are all in the cmake, and only needed if we want to disable SSP1, make it simple also for LNL.

Copy link
Member

Choose a reason for hiding this comment

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

@plbossart good for you now ?

@@ -0,0 +1,4 @@
# TGL-specific variable definitions
Define {
SSP1_ENABLED "true"
Copy link
Member

Choose a reason for hiding this comment

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

so we are going to have to add SSP1_ENABLED for LNL as well? It'd be smarter to disable SSP1 when needed, fewer configurations to maintain in the end.

Copy link
Collaborator

Choose a reason for hiding this comment

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

so we are going to have to add SSP1_ENABLED for LNL as well? It'd be smarter to disable SSP1 when needed, fewer configurations to maintain in the end.

@plbossart @aiChaoSONG We're not sure if LNL will have the same pin mux conflict between SSP1 and MIC as MTL, but probably it will. So it's helpful if we can disable SSP1 when needed.

There is pinmux conflict between SSP1 and DMIC
on MTL RVP, add a new nocodec topology target
for MTL, on which only SSP0 and SSP2 are enabled.

Signed-off-by: Chao Song <chao.song@linux.intel.com>
@kv2019i
Copy link
Collaborator

kv2019i commented Mar 29, 2023

One multiple-pause fail in https://sof-ci.01.org/sofpr/PR7239/build4940/devicetest/index.html , but seems to be a case of #7324 so not related to this PR. Proceeding with merge.

@kv2019i kv2019i merged commit 843fd70 into thesofproject:main Mar 29, 2023
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.

6 participants