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?
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.
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.
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.c b/drivers/pinctrl/pinctrl-amd.c
index fff6d4209ad5..072d44b0fc8c 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -908,12 +908,13 @@ static bool amd_gpio_should_save(struct amd_gpio *gpio_dev, unsigned int pin)
return false;
}
-static int amd_gpio_suspend(struct device *dev)
+static int amd_gpio_suspend_common(struct device *dev, bool is_s03)
{
struct amd_gpio *gpio_dev = dev_get_drvdata(dev);
struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
unsigned long flags;
int i;
+ u32 wake_mask = is_s03 ? WAKE_SOURCE_S03 : WAKE_SOURCE_S4;
for (i = 0; i < desc->npins; i++) {
int pin = desc->pins[i].number;
@@ -925,11 +926,11 @@ static int amd_gpio_suspend(struct device *dev)
gpio_dev->saved_regs[i] = readl(gpio_dev->base + pin * 4) & ~PIN_IRQ_PENDING;
/* mask any interrupts not intended to be a wake source */
- if (!(gpio_dev->saved_regs[i] & WAKE_SOURCE)) {
+ if (!(gpio_dev->saved_regs[i] & wake_mask)) {
writel(gpio_dev->saved_regs[i] & ~BIT(INTERRUPT_MASK_OFF),
gpio_dev->base + pin * 4);
- pm_pr_dbg("Disabling GPIO #%d interrupt for suspend.\n",
- pin);
+ pm_pr_dbg("Disabling GPIO #%d interrupt for %s suspend.\n",
+ pin, is_s03 ? "S0idle3/S3" : "S4/S5");
}
raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
@@ -938,6 +939,16 @@ static int amd_gpio_suspend(struct device *dev)
return 0;
}
+static int amd_gpio_suspend_s03(struct device *dev)
+{
+ return amd_gpio_suspend_common(dev, true);
+}
+
+static int amd_gpio_suspend_s45(struct device *dev)
+{
+ return amd_gpio_suspend_common(dev, false);
+}
+
static int amd_gpio_resume(struct device *dev)
{
struct amd_gpio *gpio_dev = dev_get_drvdata(dev);
@@ -961,8 +972,12 @@ static int amd_gpio_resume(struct device *dev)
}
static const struct dev_pm_ops amd_gpio_pm_ops = {
- SET_LATE_SYSTEM_SLEEP_PM_OPS(amd_gpio_suspend,
- amd_gpio_resume)
+ .suspend_late = amd_gpio_suspend_s03,
+ .resume_early = amd_gpio_resume,
+ .freeze_late = amd_gpio_suspend_s45,
+ .thaw_early = amd_gpio_resume,
+ .poweroff_late = amd_gpio_suspend_s45,
+ .restore_early = amd_gpio_resume,
};
#endif
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
struct amd_function {
const char *name;
