[rom/power_manager/system_reset_controller?] WAKE_STATUS reports 0x00 upon deep sleep wake in certain scenarios

[jettr]

Description

TLDR; The WAKE_INFO.REASONS register is reporting 0x00 upon every wake by any source in certain states.

We are working with the ULP wake feature on the SystemReset Controller. We noticed that the chip cannot go into normal/deep sleep if the AC_PRESENT pin is asserted high since the wake feature only supports high level trigger on ac_present_i. This is undesriable, so we are trying to work around this by inverting the AC_PRESENT signal in the KEY_INVERT_CTL when the chip needs to go into sleep while AC_PRESENT is asserted. This works to allow the chip to go and stay in deep sleep, but upon waking (from any wake source, e.g. UART, gpio), the WAKE_INFO.REASONS register (via retention ram copy from rom), reports 0x00.

Also worth noting, that I did look at the UART lines upon deep sleep resume to ensure that the chip wasn't resetting due to an unhandled exception similar to #22204. So ROM and ROM_EXT are not printing out any BFV:.

Two requests from this issue:

• Investigate the negative interaction with WAKE_INFO.REASONS value when KEY_INVERT_CTL is specifying 0x100 (ac_present) upon deep sleep enter and exit
• Request that the ULP wake up for ac_present move to rising edge (or both edge) trigger instead of level. I can create a separate issue to track the feature request, but I want to see if it even in the realm of possibilities right now.

[pamaury]

Just a quick comment that if you want to wake up on an edge, maybe you could use the lid_open_i or the pwrb_in pins? The pin name are somewhat irrelevant here, they basically are just three pins with three different wake-up modes?

[jesultra]

We do want to wake on edges on the two pins called lid_open and pwrb, and we also want to wake up on edges on ac_present. Or maybe equally good or better, we want to wake up on ac_present level opposite of what it was when going to sleep, so if only we had a bit to control the wakeup level of ac_present (which would not be locked along with KEY_INVERT_CTL ), that would be ideal.

[a-will]

I'm not reproducing the issue here. I made just this change to //sw/device/tests:pwrmgr_deep_sleep_all_wake_ups_silicon_owner_sival_rom_ext:

diff --git a/sw/device/tests/sim_dv/pwrmgr_sleep_all_wake_ups_impl.c b/sw/device/tests/sim_dv/pwrmgr_sleep_all_wake_ups_impl.c
index 65afc74b00..060574aca3 100644
--- a/sw/device/tests/sim_dv/pwrmgr_sleep_all_wake_ups_impl.c
+++ b/sw/device/tests/sim_dv/pwrmgr_sleep_all_wake_ups_impl.c
@@ -38,15 +38,23 @@ dif_usbdev_t usbdev;
  * . use IOR13 as pwrb_in for DV, and IOC0 otherwise
  */
 static void prgm_sysrst_ctrl_wakeup(void *dif) {
-  dif_sysrst_ctrl_input_change_config_t config = {
-      .input_changes = kDifSysrstCtrlInputPowerButtonH2L,
-      .debounce_time_threshold = 1,  // 5us
-  };
-  CHECK_DIF_OK(dif_sysrst_ctrl_input_change_detect_configure(dif, config));
   CHECK_DIF_OK(dif_pinmux_input_select(
-      &pinmux, kTopEarlgreyPinmuxPeripheralInSysrstCtrlAonPwrbIn,
+      &pinmux, kTopEarlgreyPinmuxPeripheralInSysrstCtrlAonAcPresent,
       kDeviceType == kDeviceSimDV ? kTopEarlgreyPinmuxInselIor13
-                                  : kTopEarlgreyPinmuxInselIoc0));
+                                  : kTopEarlgreyPinmuxInselIoc1));
+  CHECK_DIF_OK(dif_sysrst_ctrl_pins_set_inverted(
+        dif, kDifSysrstCtrlPinAcPowerPresentIn, true));
+  dif_sysrst_ctrl_ulp_wakeup_config_t wake_cfg = {
+    .enabled = kDifToggleDisabled,
+    .ac_power_debounce_time_threshold = 1,
+  };
+  CHECK_DIF_OK(dif_sysrst_ctrl_ulp_wakeup_configure(dif, wake_cfg));
+  CHECK_DIF_OK(dif_sysrst_ctrl_ulp_wakeup_set_enabled(dif, kDifToggleEnabled));
 }
 
 /**

I was lazy, so I ran the test once to program the flash, but afterwards, start with ac_present high and toggle the reset:

opentitantool --interface teacup gpio write IOC1 true
opentitantool --interface teacup gpio write RESET false
opentitantool --interface teacup gpio write RESET true

The test is waiting for ac_present to go low (because of the inversion):

Starting ROM_EXT
0: 40130000 NAPOT L--- sz=00001000
1: 40480000 NAPOT L--- sz=00000400
2: 20010400 ----- ---- sz=00000000
3: 20017b88   TOR -X-R sz=00007788
4: 00000000 ----- ---- sz=00000000
5: 00000000 ----- ---- sz=00000000
6: 00000000 ----- ---- sz=00000000
7: 00000000 ----- ---- sz=00000000
8: 00000000 ----- ---- sz=00000000
9: 00000000 ----- ---- sz=00000000
10: 20000400 ----- ---- sz=00000000
11: 20005d38   TOR -X-R sz=00005938
12: 20000000 NAPOT L--R sz=00100000
13: 00010000 NAPOT L--- sz=00001000
14: 40000000 NAPOT L-WR sz=10000000
15: 10000000 NAPOT L-WR sz=00020000
mseccfg = 00000002
entry: 0x20010480
I00001 ottf_main.c:154] Running sw/device/tests/pwrmgr_deep_sleep_all_wake_ups.c
I00002 pwrmgr_deep_sleep_all_wake_ups.c:65] POR reset
I00003 pwrmgr_sleep_all_wake_ups_impl.c:234] Issue WFI to enter sleep 0

Make ac_present go low (inverted form go to the high level):

opentitantool --interface teacup gpio write IOC1 false

Then the test correctly reports wakeup reason 0:

I00003 pwrmgr_sleep_all_wake_ups_impl.c:234] Issue WFI to enter sleep 0
Starting ROM_EXT
0: 40130000 NAPOT L--- sz=00001000
1: 40480000 NAPOT L--- sz=00000400
2: 20010400 ----- ---- sz=00000000
3: 20017b88   TOR -X-R sz=00007788
4: 00000000 ----- ---- sz=00000000
5: 00000000 ----- ---- sz=00000000
6: 00000000 ----- ---- sz=00000000
7: 00000000 ----- ---- sz=00000000
8: 00000000 ----- ---- sz=00000000
9: 00000000 ----- ---- sz=00000000
10: 20000400 ----- ---- sz=00000000
11: 20005d38   TOR -X-R sz=00005938
12: 20000000 NAPOT L--R sz=00100000
13: 00010000 NAPOT L--- sz=00001000
14: 40000000 NAPOT L-WR sz=10000000
15: 10000000 NAPOT L-WR sz=00020000
mseccfg = 00000002
entry: 0x20010480
I00001 ottf_main.c:154] Running sw/device/tests/pwrmgr_deep_sleep_all_wake_ups.c
I00002 pwrmgr_deep_sleep_all_wake_ups.c:74] Woke up by source 0
I00003 pwrmgr_sleep_all_wake_ups_impl.c:234] Issue WFI to enter sleep 2

[jettr]

Thank you for trying this out. I believe you are actually reproducing the issue based on the output you posted, specifically the line I00002 pwrmgr_deep_sleep_all_wake_ups.c:74] Woke up by source 0. It is showing a value of 0 for the wake up reason instead of a proper non-zero wake up reason.

[a-will]

No, that means it showed a value of 0x1 (i.e. bit 0, for sysrst_ctrl)). It would fail if it woke up for some other reason.

At least, that is what the software is supposed to be doing. I'll double check that there isn't a bug in the check.

Edit: So WAKE_INFO is reporting the expected values. I added an extra debug print as well:

I00001 ottf_main.c:154] Running sw/device/tests/pwrmgr_deep_sleep_all_wake_ups.c
I00002 pwrmgr_sleep_all_wake_ups_impl.c:246] wakeup source correct exp:0x1  obs:0x1
I00003 pwrmgr_sleep_all_wake_ups_impl.c:247] wakeup types were 0x1
I00004 pwrmgr_deep_sleep_all_wake_ups.c:74] Woke up by source 0
I00005 pwrmgr_sleep_all_wake_ups_impl.c:234] Issue WFI to enter sleep 2

The "source" in "Woke up by source" here is specific to the test's own utilities, but it is the expected value. The request "types" value is 1 iff WAKE_INFO.FALLTHROUGH and WAKE_INFO.ABORT are 0 and WAKE_INFO.REASONS is not 0:

https://github.com/lowRISC/opentitan/blob/be3d1c8615f54227fe66891accc35a1fb03b785a/sw/device/lib/dif/dif_pwrmgr.c#L420-L440

[a-will]

@GregAC @marnovandermaas @moidx for triage / discussion on the ac_present detector front. Jes suggests using KEY_INVERT_CTL is problematic because they intend to lock it.

@jesultra Is the REGWEN story similar for MIO_PAD_ATTR.invert?

[jesultra]

We use the system reset controller in part to ensure that the user can reset the GSC or perform certain other operations by means of holding some combination of keys or other things, even if our GSC firmware has gone into some weird and corrupt state. This works best if after initial configuration, we can lock down all the pieces that are required for the key combo detection, including pinmux.

The above being said, maybe we do not need to use the ultra low power wakeup function of sysrst_ctrl, if Ti50 plans to have pinmux powered anyway. If so, the fixed level detection would not be a problem. @jettr have you given that any thought?

[jesultra]

The issue with AC_PRESENT level trigger was due to a misunderstanding in the Ti50 team. It is ok to have ULP wakeup always trigger up on a high level.

[jettr]

There still might be some lingering issues with wake cause reporting and ulp wake, but seems like very low priority. We can either push this off or close it now. I am okay with either.

[andreaskurth]

Thx @jettr, are you okay with deprioritizing this from A1, then?

[jettr]

Thx @jettr, are you okay with deprioritizing this from A1, then?

Yes, that sounds okay to me.