[pwrmgr] Don't lock CONTROL until observing core_sleep #23500

a-will · 2024-06-05T05:16:56Z

To avoid race conditions with WFI and locking the CONTROL CSR prematurely, wait until the transition out of FastPwrStateActive that begins low power entry before locking the CONTROL CSR.

This allows software to clear CONTROL.LOW_POWER_HINT if an early interrupt caused the "core_sleep" pulse to be too short for pwrmgr to capture. Then, subsequent WFI instructions may still choose between full power and a low power mode.

When software wakes up, it must attempt to clear CONTROL.LOW_POWER_HINT, then busy-poll until it reads its value as 0. If pwrmgr triggered low power entry, the CPU will poll until pwrmgr completes the low power sequence. If pwrmgr did not trigger low power entry, the CPU's write will succeed, and the read will immediately return 0.

Also add a test that causes Ibex to decode WFI with an IRQ pending and not issue core_sleep (modified from a submission from @ecgh0). Check for correct recovery of the CONTROL CSR and that software can clear LOW_POWER_HINT.

To avoid race conditions with WFI and locking the CONTROL CSR prematurely, wait until the transition out of FastPwrStateActive that begins low power entry before locking the CONTROL CSR. This allows software to clear CONTROL.LOW_POWER_HINT if an early interrupt caused the "core_sleep" pulse to be too short for pwrmgr to capture. Then, subsequent WFI instructions may still choose between full power and a low power mode. When software wakes up, it must attempt to clear CONTROL.LOW_POWER_HINT, then busy-poll until it reads its value as 0. If pwrmgr triggered low power entry, the CPU will poll until pwrmgr completes the low power sequence. If pwrmgr did not trigger low power entry, the CPU's write will succeed, and the read will immediately return 0. Signed-off-by: Alexander Williams <[email protected]>

a-will · 2024-06-05T05:18:46Z

@johannheyszl @vogelpi I tagged you because this modifies the behavior of the PWRMGR.CTRL.CONFIG.REGWEN countermeasure. Instead of locking immediately upon writing a 1 to CONTROL.LOW_POWER_HINT, the lock now happens when pwrmgr begins the low power entry sequence.

a-will · 2024-06-05T05:19:06Z

Thanks, @matutem, for the great idea to defer the locking!

a-will · 2024-06-05T05:31:56Z

hw/top_earlgrey/ip_autogen/pwrmgr/rtl/pwrmgr.sv

@@ -339,7 +340,7 @@ module pwrmgr
      lowpwr_cfg_wen <= 1'b1;
    end else if (!lowpwr_cfg_wen && (clr_cfg_lock || wkup)) begin
      lowpwr_cfg_wen <= 1'b1;
-    end else if (low_power_hint) begin
+    end else if (low_power_entry) begin


If it's important to have the configuration locked for the synchronization into the slow domain, we can probably do something about that. I noticed that is one other visible bit that is dropped.

If it's not important, we can move on!

I think it should be okay.

Add pwrmgr test that checks for correct recovery of the CONTROL CSR when Ibex does not issue a long enough pulse for pwrmgr to sample a core sleeping state. This occurs when an IRQ is pending before entering WFI, and the prefetcher is active when WFI is executed. Software should be able to clear LOW_POWER_HINT when pwrmgr does not begin the low power entry sequence. (cherry picked from commit c7e72a6) Signed-off-by: Alexander Williams <[email protected]> Co-authored-by: Edward Hill <[email protected]>

GregAC · 2024-06-05T14:31:06Z

Nice work @a-will.

I'd just like to double check my understanding of the scenarios.

When you set the lower power hint and then execute the WFI you have two possibilities:

low_power_entry_i in pwrmgr_fsm is asserted for at least one cycle (low power hint has set and Ibex indicated sleep for enough cycles for a high value to make it over the CDC), so you transition out of FastPwrStateActive and clocks will be disabled. The hint will be cleared as part of this process. You then make your way back to FastPwrStateActive either due to a fall through or because you've actually slept.
low_power_entry_i in pwrmgr_fsm has not yet been asserted and will not be asserted until you execute another WFI. The hint will still be set as we haven't gone through the FSM path that clears it and there's no lock so software can instantly clear the lower power hint.

The tricky bit is in 1. after leaving FastPwrStateActive we begin to disable clocks but it is possible that Ibex could leave sleep before this is done and begin executing code following WFI. In this case we've got a fall through so the clocks will disable briefly and then re-enable. It's this the busy-poll deals with. If we've hit this case then the hint clear after WFI will be a no-op (it will be locked at this point) however we're going to fall through so it'll clear itself. The busy poll then effectively waits until the fallthrough has occurred and we're back to Active with no hint set.

Have I got that right?

a-will · 2024-06-05T14:36:10Z

Nice work @a-will.

I'd just like to double check my understanding of the scenarios.

When you set the lower power hint and then execute the WFI you have two possibilities:

low_power_entry_i in pwrmgr_fsm is asserted for at least one cycle (low power hint has set and Ibex indicated sleep for enough cycles for a high value to make it over the CDC), so you transition out of FastPwrStateActive and clocks will be disabled. The hint will be cleared as part of this process. You then make your way back to FastPwrStateActive either due to a fall through or because you've actually slept.

low_power_entry_i in pwrmgr_fsm has not yet been asserted and will not be asserted until you execute another WFI. The hint will still be set as we haven't gone through the FSM path that clears it and there's no lock so software can instantly clear the lower power hint.

The tricky bit is in 1. after leaving FastPwrStateActive we begin to disable clocks but it is possible that Ibex could leave sleep before this is done and begin executing code following WFI. In this case we've got a fall through so the clocks will disable briefly and then re-enable. It's this the busy-poll deals with. If we've hit this case then the hint clear after WFI will be a no-op (it will be locked at this point) however we're going to fall through so it'll clear itself. The busy poll then effectively waits until the fallthrough has occurred and we're back to Active with no hint set.

Have I got that right?

Yup! That's right on all counts. 😄

GregAC · 2024-06-05T14:39:15Z

Oh and we should have a follow PR that updates the pwrmgr docs to explain the proper sleep proceedure

vogelpi

Post merge but this looks good to me, thanks @a-will and @matutem !

vogelpi · 2024-06-06T08:27:13Z

hw/top_earlgrey/ip_autogen/pwrmgr/rtl/pwrmgr.sv

@@ -339,7 +340,7 @@ module pwrmgr
      lowpwr_cfg_wen <= 1'b1;
    end else if (!lowpwr_cfg_wen && (clr_cfg_lock || wkup)) begin
      lowpwr_cfg_wen <= 1'b1;
-    end else if (low_power_hint) begin
+    end else if (low_power_entry) begin


I think it should be okay.

vogelpi · 2024-06-06T08:29:32Z

Thanks @a-will and @GregAC for the discussion. I had the very same question and it was helpful to your discussion. :-)

a-will requested review from GregAC, andreaskurth, vogelpi, matutem and johannheyszl June 5, 2024 05:16

a-will requested review from a team as code owners June 5, 2024 05:16

a-will requested review from moidx and removed request for a team June 5, 2024 05:16

a-will force-pushed the pwrmgr-lock-at-transition branch from c4acc0b to c19785e Compare June 5, 2024 05:29

a-will commented Jun 5, 2024

View reviewed changes

a-will force-pushed the pwrmgr-lock-at-transition branch from c19785e to d3270fe Compare June 5, 2024 05:45

a-will mentioned this pull request Jun 5, 2024

[pwrmgr] Deep/Normal sleep enter and WFI interacttion seems incorrect #23351

Closed

moidx removed the request for review from a team June 5, 2024 13:56

moidx approved these changes Jun 5, 2024

View reviewed changes

a-will merged commit 061aedd into lowRISC:master Jun 5, 2024
31 checks passed

a-will deleted the pwrmgr-lock-at-transition branch June 5, 2024 14:30

a-will mentioned this pull request Jun 5, 2024

[pwrmgr,doc] Clarify how interrupts should be masked for low power #23390

Merged

vogelpi reviewed Jun 6, 2024

View reviewed changes

matutem mentioned this pull request Jun 11, 2024

[rtl,pwrmgr] Remove CONTROL lock due to low_power_hint #23630

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[pwrmgr] Don't lock CONTROL until observing core_sleep #23500

[pwrmgr] Don't lock CONTROL until observing core_sleep #23500

a-will commented Jun 5, 2024

a-will commented Jun 5, 2024

a-will commented Jun 5, 2024

a-will Jun 5, 2024

vogelpi Jun 6, 2024

GregAC commented Jun 5, 2024

a-will commented Jun 5, 2024

GregAC commented Jun 5, 2024

vogelpi left a comment

vogelpi Jun 6, 2024

vogelpi commented Jun 6, 2024

[pwrmgr] Don't lock CONTROL until observing core_sleep #23500

[pwrmgr] Don't lock CONTROL until observing core_sleep #23500

Conversation

a-will commented Jun 5, 2024

a-will commented Jun 5, 2024

a-will commented Jun 5, 2024

a-will Jun 5, 2024

Choose a reason for hiding this comment

vogelpi Jun 6, 2024

Choose a reason for hiding this comment

GregAC commented Jun 5, 2024

a-will commented Jun 5, 2024

GregAC commented Jun 5, 2024

vogelpi left a comment

Choose a reason for hiding this comment

vogelpi Jun 6, 2024

Choose a reason for hiding this comment

vogelpi commented Jun 6, 2024