nuttx icon indicating copy to clipboard operation
nuttx copied to clipboard

pm_idle.c add support for smp case.

Open jasonbu opened this issue 1 year ago • 1 comments

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.~~ We currently did not support the sched lock by tcb/cpu, so removed the sched_lock/unlock action in pm_idle.c when smp case, after we finished this feature, please revert the last patch to enable it again, or ping me.

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.

jasonbu avatar Sep 19 '24 09:09 jasonbu

[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.

nuttxpr avatar Sep 19 '24 10:09 nuttxpr