On 6.01.2025 04:18, Mario Limonciello wrote:
On 12/29/2024 10:42, Maciej S. Szmigiero wrote:
Some laptops have pins which are a wake source for S0i3/S3 but which
aren't a wake source for S4/S5 and which cause issues when left unmasked
during hibernation (S4).
For example HP EliteBook 855 G7 has pin #24 that causes instant wakeup
(hibernation failure) if left unmasked (it is a wake source only for
S0i3/S3).
On your machine do you know what pin 24 is connected to?
Yes, it's connected to WWAN modem, since this GPIO pin triggers wake notify
to this modem's parent PCIe port:
From \_SB.GPIO._EVT:
Case (0x18)
{
MSTP (0x3918)
Notify (\_SB.PCI0.GP12, 0x02) // Device Wake
}
WWAN modem is _SB.PCI0.GP12.PWAN, there's no other device under _SB.PCI0.GP12.
Will add this note to v2 commit message.
If not, can you run https://gitlab.freedesktop.org/drm/amd/-/blob/master/scripts/amd_s2idle.py and share the text report it generates to me?
I'll see if I can make sense in that report what it's most likely connected to.
Just want to make sure we're not papering over an issue in another component by making this change.
I think if firmware wants some pins as a wake source just for S4/S5
then it still should be respected.
Fix this by considering a pin a wake source only if it is marked as one
for the current suspend type (S0i3/S3 vs S4/S5).
Since I'm not sure if Z-wake pins should be included in either suspend
category I excluded them from both, so pins with only the Z-wake flag set
are treated as non-wake pins.
Z only makes sense at runtime. As long as it's restored to previous value after exiting suspend or hibernate that should be totally fine.
Ack.
Fixes: 2fff0b5e1a6b ("pinctrl: amd: Mask non-wake source pins with interrupt enabled at suspend")
Signed-off-by: Maciej S. Szmigiero <mail@xxxxxxxxxxxxxxxxxxxxx>
---
drivers/pinctrl/pinctrl-amd.c | 27 +++++++++++++++++++++------
drivers/pinctrl/pinctrl-amd.h | 7 +++----
2 files changed, 24 insertions(+), 10 deletions(-)
(..)
diff --git a/drivers/pinctrl/pinctrl-amd.h b/drivers/pinctrl/pinctrl-amd.h
index 667be49c3f48..8bf9f410d7fb 100644
--- a/drivers/pinctrl/pinctrl-amd.h
+++ b/drivers/pinctrl/pinctrl-amd.h
@@ -80,10 +80,9 @@
#define FUNCTION_MASK GENMASK(1, 0)
#define FUNCTION_INVALID GENMASK(7, 0)
-#define WAKE_SOURCE (BIT(WAKE_CNTRL_OFF_S0I3) | \
- BIT(WAKE_CNTRL_OFF_S3) | \
- BIT(WAKE_CNTRL_OFF_S4) | \
- BIT(WAKECNTRL_Z_OFF))
+#define WAKE_SOURCE_S03 (BIT(WAKE_CNTRL_OFF_S0I3) | \
+ BIT(WAKE_CNTRL_OFF_S3))
+#define WAKE_SOURCE_S4 BIT(WAKE_CNTRL_OFF_S4)
Since s03 doesn't make sense and s0i3 is wrong for s3 and s3 is wrong for s0i3 as a personal preference I would just call it
WAKE_SOURCE_SUSPEND
WAKE_SOURCE_HIBERNATE
Will change them accordingly.
Thanks,
Maciej
