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

pm_idle.c add support for smp case. #13541

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Conversation

jasonbu
Copy link
Contributor

@jasonbu jasonbu commented Sep 19, 2024

Summary

The current pm_idle is only support for not smp case, and we are now start facing the chip & board & project with both smp and power consume limit. These patchs will take smp support for PM.

We do a spinlock_irqsave and cpuset bit clear to ensure there is no running cpu. then the pm_handle can do cross-core relative operations with locked.

PM_IDLE_DOMAIN now only action as system domain, and only update when last core enter sleep, or first core leave sleep.
If want to get notification from specific core should register callback by specific cpu domain.

# define PM_SMP_CPU_DOMAIN(cpu) (CONFIG_PM_NDOMAINS - CONFIG_SMP_NCPUS + (cpu))

system domain will not deeper than cpudomain cur state.
this behavior is realized by stay/relax when cpudomain state change.

We exposed the lock/unlock behavior in pm_handler, so need manually unlock->WFI->lock in pm_handler, to make possible minium the cpus lock time. if not cross core relative required, operations can also do after unlock, and before WFI.

As it always run in idle threads. Need to sched_lock, so sched lock by tcb/core is required.
Before this feature ready will keep in draft status.

Impact

This method is optional, and will no impact if did not face the use case open COFIG_PM and CONFIG_SMP at the same time.

When open CONFIG_PM and CONFIG_SMP at the same time, the pm_idle function will replaced with smp version, and require the chip/board to implement pm handler, to take care of the cpu domain & system domain state changed.

Testing

CI-test & qemu-v8a manually open CONFIG_PM & cortex-A7 SMP board.

@nuttxpr
Copy link

nuttxpr commented Sep 19, 2024

[Experimental Bot, please feedback here]

Squash The Commits: This PR contains 4 Commits. Please Squash the Multiple Commits into a Single Commit.

The PR summary and impact sections seem to meet NuttX requirements. However, the testing section needs improvement.

Here's a breakdown:

✅ Summary:

  • Clearly states the necessity of the change (support for SMP systems with power constraints).
  • Explains the affected part of the code (pm_idle function).
  • Outlines the mechanism of the change (spinlock, cpuset manipulation, domain management).
  • Mentions the draft status due to the "sched lock" requirement.

✅ Impact:

  • Addresses feature addition and its optional nature.
  • Highlights the impact on users who enable both CONFIG_PM and CONFIG_SMP.
  • Mentions the need for chip/board specific pm handler implementation.

❌ Testing:

  • Simply stating "CI-test & qemu-v8a manually open CONFIG_PM & cortex-A7 SMP board" is insufficient.
  • Provide specific details:
    • Which CI tests were run and their results?
    • What exact steps were taken to test on qemu-v8a?
    • What were the expected outcomes and were they observed?
  • Include testing logs:
    • Before the change (demonstrating the issue).
    • After the change (showing the issue is resolved).

Recommendations:

  1. Expand the Testing section with specific details and logs as described above.
  2. Clarify the "sched lock" requirement:
    • Why is it necessary?
    • How should it be implemented by users?
    • Provide an example if possible.

Addressing these points will significantly improve the PR and bring it closer to meeting NuttX requirements.

add arm64 qemu pm compatible for demo pm_idle in not smp & smp usage
demo, chip should based on demo to add more operation in pm_idle_handler

Signed-off-by: buxiasen <[email protected]>
@github-actions github-actions bot added the Size: M The size of the change in this PR is medium label Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Size: M The size of the change in this PR is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants