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

[jettr]

Description

When entering enter deep sleep via wfi instruction (while performing all of the correct procedures), the core can wake up from the wfi instruction and proceed to execute the next instructions while the deep sleep mechanism is in the process of turning the core off. The deep sleep hardware will eventually turn off the core at some later point; this doesn't seem like the correct behavior.

I think either of the following are more consistent:

• If the core returns from the wfi instruction, the deep sleep mechanism is cancelled
• If the deep sleep mechanism has already started engaging, then the core should not be released to exectue anything after the wfi instruction.

---

This is not blocking us, since we can disable all of the "second-level" interrupt enable bits (e.g. mie::msoft::CLEAR + mie::mtimer::CLEAR + mie::mext::CLEAR) to ensure that wfi can never return back to the core.

[jettr]

We actually are seeing this for normal sleep enter as well. The wfi instruction will return execution back to the core, but the machinery to enter normal sleep is asynchronously continuing to put the chip into a normal sleep mode. At some point in the future, the core is stopped for normal sleep (but it is many instructions after the wfi). This seems much worse than the deep sleep case as there is not as good of a firmware workaround for normal sleep other than just no-op'ing in a loop after wfi, which does not seem great.

[pamaury]

I am might be wrong since I am not a RISC-V expert but I think the spec makes no guarantee about the cirmcumstances in which a core wakes up from wfi. This is why the firmware should ideally call wfi in a loop and check it everytime if it was "woken up" for the intended reason. In fact, I believe that nop is a valid implementation of wfi so just calling wfi and hoping that the core will sleep before the next instruction is never correct.

Maybe @nbdd0121 or @andreaskurth can say more on this?

[nbdd0121]

Yeah, wfi can spuriously wake up or be implemented as nop. Moreover, even with wfi implemented to not spuriously wake up, it also needs wake up (by spec) when there are interrupts pending but masked by global interrupt enables (MIE), so I wouldn't suggest rely on WFI stalling the processsor until deep sleep kicks in.

[GregAC]

As @pamaury and @nbdd0121 say WFI doesn't really guarantee any specific behaviour. Though in the case of Ibex it should be ceasing execution until an interrupt occurs.

@jettr can you describe in more detail what's going on in the case you're seeing?

[a-will]

@jettr Are you sure you're disabling all interrupts that aren't supposed to wake up the device?

Note that this does not mean merely disabling Ibex's global interrupt enable bit. That is not at all sufficient, as it doesn't "disable" interrupts from a sleep/wake perspective. The global interrupt enable bit only controls whether the branch to an ISR is taken--Despite its name, it should not be associated with the concept of "disabling interrupts" for this purpose. From the system's sleep/wake perspective (including WFI), interrupts are still "enabled."

What you're calling "second-level" interrupts are the actual final gates of the interrupt signals. Those are the roots for disabling interrupts to Ibex. If you expect normal sleep to wake up from some external interrupts, that means you'll need to disable interrupts at the PLIC (or individual IP instances, if you prefer).

We probably ought to clarify this in the docs here:

opentitan/hw/ip_templates/pwrmgr/doc/programmers_guide.md

Lines 8 to 10 in 71f4edb

	## Programmer Sequence for Entering Low Power
	1. Disable interrupts

Maybe we should use different lingo, though. Perhaps "disable interrupts" can continue to mean "disable taking the branch to the ISR," and we can use "mask interrupts" to mean "prevent the interrupt from signaling inside Ibex's controller."

[jettr]

The deep sleep case seems somewhat reasonable, so let's not spend too much time discussing that case.

However, the normal sleep case is the more concerning case as the FW expects to resume execution after the wfi instruction after normal sleep wakes up (assuming FW did all of the normal sleep preparation, e.g. setting LOW_POWER_HINT and syncing the clock domains, etc). If that is not the expectation, what is it?

Maybe we should use different lingo, though. Perhaps "disable interrupts" can continue to mean "disable taking the branch to the ISR," and we can use "mask interrupts" to mean "prevent the interrupt from signaling inside Ibex's controller."

Are you suggesting that even for normal sleep (where the core should continuing executing from the same PC after normal sleep wake up), that we should be setting mie::msoft::CLEAR + mie::mtimer::CLEAR + mie::mext::CLEAR as well (not just for deep sleep)? When I try that, the chip doesn't wake up correctly.

What should the OT programmers guide for putting the chip into a normal sleep be?

1. Disable interrupts ([Jett] Seems like just setting mie to false is what this is asking for)
2. Enable desired wakeup and reset sources in [`WAKEUP_EN`]
3. Perform any system-specific low power entry steps, e.g.
   - Interrupt checks (if something became pending prior to disable)
4. Configure low power mode in [`CONTROL`]
5. Set low power hint in [`LOW_POWER_HINT`]
6. Set and poll [`CFG_CDC_SYNC`] to ensure above settings propagate across clock domains.
7. Execute wait-for-interrupt instruction on the processing host. 
    a.  [Jett] NOTE: leaving the `wfi` instruction does not mean that chip has entered and exited low power mode
8. [Jett] Continuing no-op looping until ?????

It does appear that the interrupt that is allowing wfi to exit is not a normal sleep wake up source, so we still want the chip to go to normal sleep until a true normal-sleep wake source has triggered.

I have included our sleep function to make the discussion a little more concrete:

fn enter_sleep(&self) {
        let rbox = unsafe { &RBOX0 };
        let pinmux = unsafe { &PINMUX };
        let deep_sleep = self.deep_sleep_enabled.get();

        if deep_sleep {
            self.log_breadcrumb(BreadcrumbValue::PmuDeepSleep);
            direct_debug!("\nEntering deep sleep zzz\n");
            let _ = self.set_long_life_value(LongLifeValue::EnteredDeepSleep, true as u32);
            let _ = self.set_long_life_value(
                LongLifeValue::DeepSleepCounter,
                self.get_long_life_value(LongLifeValue::DeepSleepCounter) + 1,
            );
        } else {
            self.log_breadcrumb(BreadcrumbValue::PmuNormalSleep);
            direct_debug!("\nEntering normal sleep\n");
        }
        // Wait until main UART HW FIFO is flushed
        flush_uart();
        let sleep_start_ms = Chip::ticks_ms() as u32;
        // Disable interrupts to prepare for sleep, following the steps from
        // https://opentitan.org/book/hw/top_earlgrey/ip_autogen/pwrmgr/doc/programmers_guide.html
        CSR.mstatus.modify(mstatus::mie::CLEAR);
        // For deep sleep we want the WFI below to never return (because resume will follow boot
        // path). Ensure this by disabling interrupts in CSR.mie in addition to CSR.mstatus
        // (because WFI ignores CSR.mstatus).
        if deep_sleep {
            CSR.mie
                .modify(mie::msoft::CLEAR + mie::mtimer::CLEAR + mie::mext::CLEAR);
        }

        rbox.enter_sleep(deep_sleep);
        pinmux.enter_sleep(deep_sleep);

        // Enable wake on RBOX, ADC, and pinmux.
        self.registers.wakeup_en[0].set(RBOX_WAKE_MASK + ADC_WAKE_MASK + PIN_WAKE_MASK);
        self.registers.wake_info.set(self.registers.wake_info.get());
        self.registers
            .wake_info_capture_dis
            .write(WAKE_INFO_CAPTURE_DIS::VAL::CLEAR);
        if deep_sleep {
            self.registers.control.set(0);
        } else {
            self.registers.control.write(CONTROL::MAIN_PD_N::SET);
        }
        self.registers.control.modify(CONTROL::LOW_POWER_HINT::SET);
        // Wait for wakeup_en and control writes.
        self.cfg_cdc_sync();

        // This WFI will attempt to sleep, because we set LOW_POWER_HINT.
        unsafe { rv32i::support::wfi() };

        // Deep sleep should never get here because on exit from deep sleep we start in ROM and
        // follow the boot path. If we do get here, then force a reset to recover.
        if deep_sleep {
            self.log_breadcrumb(long_life_value::BreadcrumbValue::DeepSleepWfiShortCircuit);
            self.trigger_hard_reset()
        }
        // Now we have resumed from normal sleep (or fell through without sleeping).

        // Restore control register to default. This clears LOW_POWER_HINT.
        self.registers
            .control
            .write(CONTROL::MAIN_PD_N::SET + CONTROL::USB_CLK_EN_ACTIVE::SET);
        // Wait for control write.
        self.cfg_cdc_sync();
        self.registers.intr_state.write(INTR::WAKEUP::SET);
        self.registers
            .wake_info_capture_dis
            .write(WAKE_INFO_CAPTURE_DIS::VAL::SET);
        CSR.mstatus.modify(mstatus::mie::SET);
        let sleep_end_ms = Chip::ticks_ms() as u32;
        self.log_breadcrumb(BreadcrumbValue::PmuResumeFromSleep);
        // Delay sleep to give time to stabilize after processing wakeup event.
        self.delay_sleep_for(DelaySleepReason::ResumeNormalSleep);
        console_info!("Sleep duration: {}ms", sleep_end_ms - sleep_start_ms);
        console_info!("Wake source: {:#010x}", self.wake_source());
        if self.wake_caused_by(WakeCause::Pin) {
            console_info!("Which pin: {:#018x}", self.exit_pins_status());
        }
    }

Note the comment // Now we have resumed from normal sleep (or fell through without sleeping).. It is a lie ;)

We have noticed we get to somewhere around console_info!("Sleep duration: {}ms", sleep_end_ms - sleep_start_ms); before the asynchronous normal sleep puts the core into a paused/low_power state. The chip can stay in normal sleep for a indefinitely amount of time and wake up correct with a normal-sleep wake source. The chip then starts executing from where it left off. It also doesn't appear that the point to which the core gets before normal sleep starts is always the same either.

[a-will]

For any low power mode, breaking out of WFI can cause a fall through exit that ends the request. To enter low-power mode, the CPU must remain in WFI continuously through pwrmgr's fast FSM transition. Thus, for any interrupts that could trigger, you must mask interrupt sources that you don't want to cause that fall through exit.

Are you suggesting that even for normal sleep (where the core should continuing executing from the same PC after normal sleep wake up), that we should be setting mie::msoft::CLEAR + mie::mtimer::CLEAR + mie::mext::CLEAR as well (not just for deep sleep)? When I try that, the chip doesn't wake up correctly.

No, definitely not specifically that, haha. In normal sleep, you still need an interrupt to satisfy WFI! You can't mask all the interrupts.

By contrast, deep sleep causes a reset of the CPU, so it "breaks out" of WFI as a sort of side effect. But again, you still need to handle interrupt sources here, since the possibility of a fall through exit remains.

To clarify, there are four important elements in this system. The main participants are...

1. The CPU
2. The pwrmgr fast FSM
3. The pwrmgr slow FSM
4. The collection of wakeup sources

For entering low-power mode, the CPU interacts with the pwrmgr Fast FSM by telling it when it is in WFI. Then, the pwrmgr Fast FSM tells the pwrmgr Slow FSM when it has cleared checks, disabled clocks, and entered low-power mode. Finally, the Slow FSM monitors for wakeup sources.

For exiting low-power mode, the Slow FSM gets a kick from a wakeup source, and it tells the pwrmgr Fast FSM to perform its power-on sequence (e.g. turn on clocks). Critically, if the CPU is still in WFI, it will stay here if there is no pending interrupt, and the CPU is effectively frozen (though technically, an NMI or entering debug mode could still break it out, haha).

From that ti50 code, there's no indication that the interrupt sources are being addressed, but it's required. The interrupts and wakeups work together to manage low power entry/exit.

The entry sequence is what I wrote in #23390:

1. Disable interrupt handling.
2. Mask all interrupt sources that should not prevent low power entry.
   - Note that merely *disabling* interrupt handling with the `mie` global interrupt-enable bit on the processing host is insufficient.
   - Interrupt sources that are not masked can cause the [fall through exit](theory_of_operation.md#fall-through-handling).
3. Enable desired wakeup and reset sources in [`WAKEUP_EN`](registers.md#wakeup_en) and [`RESET_EN`](registers.md#reset_en).
4. Perform any system-specific low power entry steps, e.g.
   - Interrupt checks (if something became pending prior to disable)
5. Configure low power mode in [`CONTROL`](registers.md#control).
6. Set low power hint in [`LOW_POWER_HINT`](registers.md#control--low_power_hint).
7. Set and poll [`CFG_CDC_SYNC`](registers.md#cfg_cdc_sync) to ensure above settings propagate across clock domains.
8. Execute wait-for-interrupt instruction on the processing host.

If you follow the sequence, then low power exit should only occur from interrupts you wanted as wakeups in the first place.

[jettr]

Specifically you have Interrupt sources that are not masked can cause the [fall through exit] and the fall through exit says that the normal sleep enter is cancelled, i.e. "thus terminating the entry process". That is what is not happening. The fast and slow FSMs are still going even though the CPU core is continuing to execute the next instructions.

[a-will]

Specifically you have Interrupt sources that are not masked can cause the [fall through exit] and the fall through exit says that the normal sleep enter is cancelled, i.e. "thus terminating the entry process". That is what is not happening. The fast and slow FSMs are still going even though the CPU core is continuing to execute the next instructions.

Ah, that was the original thing you mentioned, wasn't it? Sorry, I got into the discussion later and latched onto the chat about the processes.

So I'd have to ask how you know that is the case, since there would be a lot of difficult-to-observe state here.

However, if the CPU did indeed continue executing after the Fast FSM transitioned from FastPwrStateFallThrough to FastPwrStateNvmIdleCheck, then it was lied to back at FastPwrStateDisClks about the clocks actually being disabled (or it didn't try to turn off the core clock for some reason). The sequence is supposed to be "turn off clocks" -> "confirm clocks are off" -> "check for fall through (CPU out of WFI)" -> "check for abort (NVM not idle)" -> "enter low power".

Or we simply went through a full cycle and came back to the same place but didn't fall through the second time, hehe.

[jettr]

So I'd have to ask how you know that is the case, since there would be a lot of difficult-to-observe state here.

It is the best explanation for what we see on our UART lines when this occurrs.

Here is an example UART output when we hit the delayed sleep after wfi scenario

[     142.145] Entering normal sleep

[  �����r�����Sleep duration: 0ms
[     145.249] Wake source: 0x00000000

Here is a typical normal sleep:

[     219.305] Entering normal sleep

[     222.292] Sleep duration: 2987ms
[     222.292] Wake source: 0x00000004
[     222.292] Which pin: 0x0000000000000004

You can see on a scope capture of the UART that the [ characters are getting printed before the chip goes to normal sleep and the � characters are actually from the UART HW going to sleep and no longer driving the line high.

The [ print comes from lines after the wfi, specifically the console_info!("Sleep duration: {}ms", sleep_end_ms - sleep_start_ms); line. It also explains why the 3 second sleep it reported as a 0ms sleep, and why the wake source would reports 0x0 instead of the actual gpio wake source that woke the device up (since the collection of the wake source register value happened before the chip went to sleep).

[ecgh0]

Or we simply went through a full cycle and came back to the same place but didn't fall through the second time, hehe.

Thank you for this hint. I should have thought of it sooner, now it seems obvious. We fall through the first WFI and then sleep on a second WFI.

The problem is that I want to avoid sleeping on that second WFI, but that doesn't seem to be working. My write to set pwrmgr.CONTROL.LOW_POWER_HINT=0 after the WFI fallthrough seems to be ignored.
I've created a test to show the problem: ecgh0@c7e72a6d80
And the result I get is: https://gist.github.com/ecgh0/b00c6d303688cddaaf527655a0af5245
This write:

  LOG_INFO("Normal WFI");
  // This should clear LOW_POWER_HINT_BIT, but it doesn't.
  mmio_region_write32(pwrmgr.base_addr, PWRMGR_CONTROL_REG_OFFSET,
      (1 << PWRMGR_CONTROL_MAIN_PD_N_BIT));

doesn't work, because CONTROL=0x101 afterwards shows LOW_POWER_HINT_BIT is still set.

[a-will]

Or we simply went through a full cycle and came back to the same place but didn't fall through the second time, hehe.

Thank you for this hint. I should have thought of it sooner, now it seems obvious. We fall through the first WFI and then sleep on a second WFI.

The problem is that I want to avoid sleeping on that second WFI, but that doesn't seem to be working. My write to set pwrmgr.CONTROL.LOW_POWER_HINT=0 after the WFI fallthrough seems to be ignored. I've created a test to show the problem: ecgh0@c7e72a6d80 And the result I get is: https://gist.github.com/ecgh0/b00c6d303688cddaaf527655a0af5245 This write:

  LOG_INFO("Normal WFI");
  // This should clear LOW_POWER_HINT_BIT, but it doesn't.
  mmio_region_write32(pwrmgr.base_addr, PWRMGR_CONTROL_REG_OFFSET,
      (1 << PWRMGR_CONTROL_MAIN_PD_N_BIT));

doesn't work, because CONTROL=0x101 afterwards shows LOW_POWER_HINT_BIT is still set.

The docs here are bad, I think (particularly, the programming guide). LOW_POWER_HINT can only automatically clear, and it does so as soon as the clocks are disabled. As soon as you set LOW_POWER_HINT to 1, the CONTROL register locks!

However, that doesn't seem to be what's happening here, since LOW_POWER_HINT is 1, not 0. Instead, I suspect Ibex never manages to enter WFI (or at least, to successfully signal that it is sleeping to pwrmgr). For one thing, you have a pending interrupt before even executing the wfi instruction, so we know this will have minimal sleep time. We'll have something like 1 CPU cycle of reported sleep time (or @GregAC, maybe even 0 cycles? If, say, we decode wfi, but have a pending interrupt + a busy LSU or prefetcher?).

Meanwhile, pwrmgr's clock runs at potentially less than 1/4 the rate of Ibex, and it merely has a 2-flop synchronizer to bring over the core_sleeping signal. @andreaskurth This is a CDC bug. Likely, a prim_sync_reqack should've been used here, so pwrmgr has a proper handshake with Ibex. It might also be desirable for it to be more complex than that, so we don't miss any immediate falling edges and can handle those later. Also, it might not be that simple as we consider which clocks can turn off and when.

I'm not sure the FSM generally has the right model to handle all the events from this faster domain. It might need a critical eye in light of these possibilities.

Here are some extra bits that don't look to be immediately relevant to this particular case, though:
Now that I'm looking at this aspect, I see another bug! CTRL_CFG_REGWEN is not reset (to enable writes again) in either the fall through case or the abort case. This configuration locking bug does work together with the bad handling of fast core_sleeping transitions to cause firmware to lose control of pwrmgr, though!

There is also a little bit of a race in the FastPwrStateDisClks state, since the FSM is not observing what happens to the CPU's WFI state. This might be too small to really matter (i.e. we miss some really short WFI -> out-of-WFI -> WFI sequence), but it is technically there.

(Hopefully someone can show me that I've missed something and at least some of these statements are errors on my part, haha)

[ecgh0]

LOW_POWER_HINT can only automatically clear

Ok, thanks. I see "this bit is automatically cleared by HW" in the docs, but wasn't sure this meant I can't clear it.
In that case I will try to change my code to retry my sleep WFI until that bit gets cleared by HW.

[moidx]

Hi @ecgh0, I added your test case to #23470. Please feel free to submit a PR instead and I will close mine. We can take the test once merged to reproduce the behavior in simulation. Please sync with @a-will and @cfrantz on slack for next steps. Thanks!

[a-will]

Here are some extra bits that don't look to be immediately relevant to this particular case, though:
Now that I'm looking at this aspect, I see another bug! CTRL_CFG_REGWEN is not reset (to enable writes again) in either the fall through case or the abort case. This configuration locking bug does work together with the bad handling of fast core_sleeping transitions to cause firmware to lose control of pwrmgr, though!

Aha, I missed that the FSM has a wkup_o that does signal on fall through and abort cases (including an interrupt, if enabled), and it clears the configuration lock. So this part of it is fine; nothing to see here. ;)

The WFI signaling still needs to be addressed.

[matutem]

It seems the unclear behavior in this issue is progressively shrinking. I am still having a problem sorting out what is the actual problem: Is it

The WFI signaling still needs to be addressed.

To go to sleep the fast FSM needs low_power_entry_i == 1 in the FastPwrStateActive state, and to stay at 1 until past the FastPwrStateFallThrough state, and low_power_entry_i == core_sleeping & low_power_hint. Can someone explain what is the actual problem with this logic, meaning what signaling is suspicious?

[GregAC]

I've reviewed the RTL behind the core_sleep_o signal in Ibex and the short answer is an execution of WFI does not guarantee core_sleep_o will be asserted (even for a single cycle). In particular with the scenario being observed where we get an instant wakeup due to a pending interrupt when WFI is executed the core will being going into its sleep state and potentially you could get a single cycle pulse on core_sleep_o but it's not guaranteed (it comes down to when the interrupt becomes pending vs when the WFI is being executed).

Internally the core_sleep_o simply signals the core is sleeping, that is the top-level clock gate has been disabled (Controlled by the clock_en signal in ibex_top). When a WFI is executed and ibex_controller goes into its sleep state it signals the controller isn't busy but there's multiple parts of Ibex that have a busy signal and it will only sleep when all aren't busy. The other parts are the load store unit and the instruction fetch unit. Load store should be idle for WFI to enter sleep (we need to wait for pending loads/store to resolve before we can execute WFI) but it's possible the icache could still be busy doing some prefetching.

I'd rather not alter the behaviour of the current core_sleep_o. I think a straight-forward indication the core is now sleeping is useful. What we could do is add some extra signals if they're useful e.g. a wfi_entry and wfi_exit which would indicate we've executed a WFI and we're leaving the sleep state that we earlier entered from a wfi respectively. We can also produce some guarantees around how the new signals behave vs core_sleep_o and pending interrupts etc.

[GregAC]

An alternative would be a missed_sleep or similar name which would give a single cycle pulse where you enter and exit a WFI without the core going to sleep (this could of course be derived from the proposed wfi_entry/wfi_exit signals).

[a-will]

@GregAC If we can change the point of locking the CONTROL CSR, it becomes okay for pwrmgr not to see short or absent core_sleep_o. Instead, software can clean up: #23500

[vogelpi]

Now that @a-will 's PR to fix up pwrmgr got merged, I think what is left here is documenting the correct SW routine around sleep entry and making sure software follows that guidance, correct?

[jettr]

I have no further concerns. Thank you!

[moidx]

Lowering the priority to P2 now that an RTL fix has been merged. Moving to M5 since remaining work is related to documentation.

@a-will, please let us know if you have any objections to this. Thanks!

[andreaskurth]

Just discussed in triage: @a-will, if you have a chance to complete this documentation for M5 (after your ongoing SDC contributions), this would be helpful. Otherwise we'll take this to the next milestone and cherry pick to the release branch (or master).

[a-will]

@andreaskurth #23390 is already queued to close this issue and awaiting review.