[v1] pinctrl: amd: Take suspend type into consideration which pins are non-wake

[PATCH] pinctrl: amd: Take suspend type into consideration which pins are non-wake

Posted by Maciej S. Szmigiero 1 year, 4 months ago

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(-)

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;

Re: [PATCH] pinctrl: amd: Take suspend type into consideration which pins are non-wake

Posted by Mario Limonciello 1 year, 4 months ago

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;

Re: [PATCH] pinctrl: amd: Take suspend type into consideration which pins are non-wake

Posted by Maciej S. Szmigiero 1 year, 4 months ago

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