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.

Sign up for GitHub

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

Jump to bottom

intel_adsp: ace: secondary core context save and restore #55182

Merged
merged 5 commits into from
Apr 28, 2023
Merged

intel_adsp: ace: secondary core context save and restore #55182

merged 5 commits into from
Apr 28, 2023

Conversation

tmleman
Copy link
Collaborator

@tmleman tmleman commented Feb 24, 2023 •
edited
Loading

Reusing primary core context save/restore flow for purpose of secondary core D0 -> D3 -> D0 transitions.

FW on secondary core when preparing for D3 will save its context. When core is re-enabled we use dsp_restore_vector as the FW entry point.

@tmleman tmleman mentioned this pull request Feb 25, 2023
Merged
1 task
@tmleman tmleman force-pushed the topic/upstream/pr/ace/d3/secondary_core_save branch from 8f726ec to 73d8177 Compare February 26, 2023 22:32
@tmleman tmleman marked this pull request as ready for review February 26, 2023 22:32
@tmleman tmleman requested review from marcinszkudlinski and removed request for nashif, andyross and dcpleung February 26, 2023 22:33
@tmleman tmleman requested a review from ceolin February 26, 2023 22:33
andyross
andyross requested changes Feb 27, 2023
View reviewed changes
Copy link
Contributor

@andyross andyross left a comment

Choose a reason for hiding this comment

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

Can we clean this up? It seems like you needed a way to tell whether or not a core was being started up after initial boot, and decided to abuse the kernel thread state for the idle thread? That's... no, that's just not right. Those are different subsystems. If the arch/soc code needs to track state for its own purposes, it needs to do it on its own and not repurpose other code from other areas.

FWIW: looking at that soc_cpus_active[] array, I'm guessing that's the right kind of abstraction to be using (though it seems to have been broken by this PR?).

soc/xtensa/intel_adsp/ace/power.c Outdated Show resolved Hide resolved
soc/xtensa/intel_adsp/ace/power.c Outdated Show resolved Hide resolved
soc/xtensa/intel_adsp/ace/power.c Show resolved Hide resolved
@RanderWang RanderWang mentioned this pull request Mar 20, 2023
Closed
@tmleman tmleman force-pushed the topic/upstream/pr/ace/d3/secondary_core_save branch from 73d8177 to 7abfc06 Compare March 30, 2023 10:42
@tmleman
Copy link
Collaborator Author

tmleman commented Mar 30, 2023

@andyross please take a look at the updated versions now.

@tmleman tmleman requested a review from ceolin April 12, 2023 12:50
@tmleman
Copy link
Collaborator Author

tmleman commented Apr 12, 2023

@andyross friendly reminder

aborisovich
aborisovich previously approved these changes Apr 12, 2023
View reviewed changes
@tmleman tmleman added the DNM This PR should not be merged (Do Not Merge) label Apr 17, 2023
@tmleman
Copy link
Collaborator Author

tmleman commented Apr 17, 2023 •
edited
Loading

Adding DNM label because of thesofproject/sof#7433. After this PR is merged zephyr update on SOF side will require additional changes. This would make later bisect more difficult in case of any new problems.

But you can still review and approve.

@aborisovich aborisovich mentioned this pull request Apr 19, 2023
Merged
1 task
ceolin
ceolin previously approved these changes Apr 25, 2023
View reviewed changes
Copy link
Member

@ceolin ceolin left a comment

Choose a reason for hiding this comment

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

I'm still a little bit skeptical about the need of all flush/invalidate cache but you know better than me about it here

@tmleman tmleman removed the DNM This PR should not be merged (Do Not Merge) label Apr 25, 2023
@tmleman
intel_adsp: ace: add stack for other cores
1b06ef1 
This patch replace temporary stack of the restore vector with interrupt
stack to reduce memory usage. Additionally we can assign seprate stack
for each core. This will allow to reuse this vector for secondary cores.

Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
@tmleman
intel_adsp: ace: cpu context save refactor
45a4871 
This patch is preparing cpu context save and restore code so it can be
later used by the multiple cores.

Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
@tmleman
intel_adsp: ace: secondary core context restore
643ef65 
Reusing primary core context save/restore flow for purpose of secondary
core D0 -> D3 -> D0 transitions. If core is re-enabled we use
dsp_restore_vector as the FW entry point.

Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
@tmleman
intel_adsp: power: ace: interrupts on and off
e7e4df2 
Masking out all interrupt during power state transition and restoring
them after is now common thing for all power states. No need to
duplicate code.

Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
@tmleman
intel_adsp: power: ace: cache invalidation
7b1d204 
This patch moves cache flush/invalidation to section executed only when
IMR context saving is enabled. If this option is disabled no FW context
is stored so any lost data doesn't matter.

Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
@tmleman tmleman dismissed stale reviews from ceolin and aborisovich via 7b1d204 April 26, 2023 12:09
@tmleman tmleman force-pushed the topic/upstream/pr/ace/d3/secondary_core_save branch from 167901a to 7b1d204 Compare April 26, 2023 12:09
@tmleman
Copy link
Collaborator Author

tmleman commented Apr 26, 2023

Rebase after merge of #50136

@aborisovich aborisovich changed the title intel_adsp: ace: secondary core context save and restore [DNM] intel_adsp: ace: secondary core context save and restore Apr 26, 2023
@aborisovich aborisovich added DNM This PR should not be merged (Do Not Merge) and removed DNM This PR should not be merged (Do Not Merge) labels Apr 26, 2023
@aborisovich aborisovich changed the title [DNM] intel_adsp: ace: secondary core context save and restore intel_adsp: ace: secondary core context save and restore Apr 26, 2023
@aborisovich
Copy link
Collaborator

Ignore, added DNM by accident to wrong PR.

andyross
andyross approved these changes Apr 26, 2023
View reviewed changes
Copy link
Contributor

@andyross andyross left a comment

Choose a reason for hiding this comment

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

Saw this come by again. Looks great to me, my concerns earlier were all addressed.

@carlescufi carlescufi merged commit aba3b12 into zephyrproject-rtos:main Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andyross andyross andyross approved these changes

@ceolin ceolin ceolin approved these changes

@jxstelter jxstelter jxstelter approved these changes

@softwarecki softwarecki softwarecki approved these changes

@marcinszkudlinski marcinszkudlinski Awaiting requested review from marcinszkudlinski

@kv2019i kv2019i Awaiting requested review from kv2019i

@lgirdwood lgirdwood Awaiting requested review from lgirdwood

@lyakh lyakh Awaiting requested review from lyakh

@marc-hb marc-hb Awaiting requested review from marc-hb

@MaureenHelm MaureenHelm Awaiting requested review from MaureenHelm

@nashif nashif Awaiting requested review from nashif

@peter-mitsis peter-mitsis Awaiting requested review from peter-mitsis

@dcpleung dcpleung Awaiting requested review from dcpleung

@aborisovich aborisovich Awaiting requested review from aborisovich

Assignees

@nashif nashif

Labels
area: Kernel platform: Intel ADSP Intel Audio platforms
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@tmleman @ceolin @aborisovich @andyross @dcpleung @jxstelter @softwarecki @nashif @carlescufi @zephyrbot