Skip to content

platform: ace: notifying about idle thread readiness - #7174

Merged
kv2019i merged 2 commits into
thesofproject:mainfrom
tmleman:topic/upstream/pr/ace/pm/secondary_core_d3
May 11, 2023
Merged

platform: ace: notifying about idle thread readiness#7174
kv2019i merged 2 commits into
thesofproject:mainfrom
tmleman:topic/upstream/pr/ace/pm/secondary_core_d3

Conversation

@tmleman

@tmlemantmleman commented Feb 25, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

Informing the primary core that the Idle thread on secondary core is ready. During the D3 exit flow thread is not initialize again, but restored from previously saved context.

Requires:

@tmlemantmleman changed the title platform: ace: notifying about idle thread readiness[DNM] platform: ace: notifying about idle thread readinessFeb 26, 2023
@tmleman
tmleman marked this pull request as ready for review February 26, 2023 22:14
lgirdwood
lgirdwood reviewed Mar 1, 2023
View reviewed changes
Comment threadsrc/platform/intel/ace/platform.c Outdated
@RanderWangRanderWang mentioned this pull request Mar 20, 2023
Closed
@tmleman
tmlemanforce-pushed the topic/upstream/pr/ace/pm/secondary_core_d3 branch from 2d8cf4a to 305785aCompareMarch 30, 2023 08:29
aborisovich
aborisovich suggested changes Mar 31, 2023
View reviewed changes
Comment threadzephyr/lib/cpu.c Outdated
Comment threadsrc/platform/intel/ace/platform.c Outdated
@sys-pt1s

sys-pt1s commented Apr 6, 2023

Copy link
Copy Markdown

Can one of the admins verify this patch?

marcinszkudlinski
marcinszkudlinski reviewed Apr 6, 2023
View reviewed changes
Comment threadzephyr/lib/cpu.c Outdated
@tmleman
tmlemanforce-pushed the topic/upstream/pr/ace/pm/secondary_core_d3 branch from 305785a to affa6aaCompareApril 6, 2023 12:42
@lgirdwood

lgirdwood commented Apr 24, 2023

Copy link
Copy Markdown
Member

@tmleman any update ?

@tmleman

tmleman commented Apr 25, 2023

Copy link
Copy Markdown
ContributorAuthor

any update ?

@lgirdwood I'm still waiting for the merge on the zephyr side

@tmleman
zephyr: cpu: secondary core idle thread init
ec7cea2
Idle thread should be initialize only when core it booting for the first
time. Doing this again would overwrite the kernel structs and the idle
thread stack.
Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
@tmleman
platform: ace: notifying about idle thread readiness
69c559f
Informing the primary core that the Idle thread on secondary core is
ready. During the D3 exit flow thread is not initialize again, but
restored from previously saved context.
This patch includes also zephyr version update to aba3b12e31 (total 15
commits). Changes related to intel_adsp contain refactor and fixes for
ACE secondary cores power flows.
Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
@tmleman
tmlemanforce-pushed the topic/upstream/pr/ace/pm/secondary_core_d3 branch from affa6aa to 69c559fCompareApril 28, 2023 09:33
@tmlemantmleman changed the title [DNM] platform: ace: notifying about idle thread readinessplatform: ace: notifying about idle thread readinessApr 28, 2023
kv2019i
kv2019i requested changes Apr 28, 2023
View reviewed changes

@kv2019ikv2019i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Now I see in second commit this is related to state-save. This should be somehow conditional on the feature being used, given the generic code is used for all platform/vendors.

Comment threadzephyr/lib/cpu.c
* and the idle thread stack.
*/
if (pm_state_next_get(id)->state == PM_STATE_ACTIVE)
z_init_cpu(id);

@kv2019ikv2019iApr 28, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Given this is an open-coded of z_smp_start_cpu() (see comment above), I don't fully understand why we have a different implementation here, and if this is correct, why this is not needed in the Zephyr kernel/smp.c version. Wouldn't the same problem be there? Or is this specific to a configuration where state is saved to persistant memory? If yes, this would need to be ifdef'ed out so this applies only to applicable platforms.

@tmlemantmlemanMay 8, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I don't know how zephyr is handling it but right now it's not working properly. After secondary core exits D3 it will repeat the path that led him to this state. Power will not be shut off so eventually it will return to the Idle state but this is not correct behavior. Maybe zephyr doesn't support situation in which each core can be turned off and on multiple times.

@kv2019ikv2019iMay 10, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@nashif@teburd@dcpleung@andyross ^^ trying to understand why z_smp_start_cpu() does not need this?

@tmleman
tmleman requested a review from kv2019iMay 8, 2023 13:05
@kv2019ikv2019i mentioned this pull request May 11, 2023
Merged
kv2019i
kv2019i approved these changes May 11, 2023
View reviewed changes

@kv2019ikv2019i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We need to file a ticket to follow-up the change, but otherwise PR can proceed (and is potentially a blocker to update Zephyr baseline now).

@tmleman

tmleman commented May 11, 2023

Copy link
Copy Markdown
ContributorAuthor

@kv2019i I have submitted requested issue #7593. I think we can proceed with merge.

@kv2019i

kv2019i commented May 11, 2023

Copy link
Copy Markdown
Collaborator

Usual PM fail in https://sof-ci.01.org/sofpr/PR7174/build6662/devicetest/index.html (but passes on at least one device) and https://sof-ci.01.org/sofpr/PR7174/build6663/devicetest/index.html has one fail on system PM.

Proceeding with the fix. Thanks @tmleman for filing the issue.

@kv2019i
kv2019i merged commit 93947f9 into thesofproject:mainMay 11, 2023
@RanderWangRanderWang mentioned this pull request Nov 3, 2023
Closed
3 tasks
@tmleman
tmleman deleted the topic/upstream/pr/ace/pm/secondary_core_d3 branch May 23, 2024 11:18
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@paulstelian97paulstelian97paulstelian97 left review comments
@kv2019ikv2019ikv2019i approved these changes
@plbossartplbossartAwaiting requested review from plbossartplbossart is a code owner
@mmaka1mmaka1Awaiting requested review from mmaka1mmaka1 is a code owner
@lbetlejlbetlejAwaiting requested review from lbetlejlbetlej is a code owner
@dbalutadbalutaAwaiting requested review from dbalutadbaluta is a code owner
@abonislawskiabonislawskiAwaiting requested review from abonislawskiabonislawski is a code owner
@lyakhlyakhAwaiting requested review from lyakhlyakh is a code owner
@iuliana-prodaniuliana-prodanAwaiting requested review from iuliana-prodaniuliana-prodan is a code owner
@lgirdwoodlgirdwoodAwaiting requested review from lgirdwoodlgirdwood is a code owner
@marcinszkudlinskimarcinszkudlinskiAwaiting requested review from marcinszkudlinski
+1 more reviewer
@aborisovichaborisovichaborisovich approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@tmleman@sys-pt1s@lgirdwood@kv2019i@paulstelian97@aborisovich@marcinszkudlinski