| Message ID | e61858fe70face71226727618dfaa9d5e54da0bd.1735490511.git.mail@maciej.szmigiero.name |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pinctrl: amd: Take suspend type into consideration which pins are non-wake | expand |
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@maciej.szmigiero.name> > --- > 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;
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@maciej.szmigiero.name> >> --- >> 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
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) struct amd_function { const char *name;
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). 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. Fixes: 2fff0b5e1a6b ("pinctrl: amd: Mask non-wake source pins with interrupt enabled at suspend") Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name> --- drivers/pinctrl/pinctrl-amd.c | 27 +++++++++++++++++++++------ drivers/pinctrl/pinctrl-amd.h | 7 +++---- 2 files changed, 24 insertions(+), 10 deletions(-)