[PATCH v2 0/4] Fix for interrupt storm on ASUS TUF A16

[PATCH v2 0/4] Fix for interrupt storm on ASUS TUF A16

[Mario Limonciello]

An interrupt storm is reported for the GPIO controller on ASUS TUF A16
but only on Linux.  In comparing the GPIO registers from Windows and
Linux the configuration for several bits specified in _AEI() was never
actually loaded into the hardware on Linux.

This meant that many values were taking the default hardware strapping
for pull up/pull down.

This series reworks the programming of various bits to unify the
expectations for the hardware.

It's confirmed to fix the interrupt storm on this system.
This series is based off of the tag pinctrl-v6.5-1.

v1->v2:
 * Update for Andy's feedback

Mario Limonciello (4):
  pinctrl: amd: Only use special debounce behavior for GPIO 0
  pinctrl: amd: Use amd_pinconf_set() for all config options
  pinctrl: amd: Drop pull up select configuration
  pinctrl: amd: Unify debounce handling into amd_pinconf_set()

 drivers/pinctrl/pinctrl-amd.c | 61 +++++++++++++----------------------
 drivers/pinctrl/pinctrl-amd.h |  1 -
 2 files changed, 23 insertions(+), 39 deletions(-)

base-commit: 9f0648f13e34a01f2e1a7a0d5801988a7bca6988
-- 
2.34.1

[PATCH v2 1/4] pinctrl: amd: Only use special debounce behavior for GPIO 0

[Mario Limonciello]

It's uncommon to use debounce on any other pin, but technically
we should only set debounce to 0 when working off GPIO0.

Cc: stable@vger.kernel.org
Fixes: 968ab9261627 ("pinctrl: amd: Detect internal GPIO0 debounce handling")
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/pinctrl/pinctrl-amd.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index 7a4dd0c861abc..02d9f9f245707 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -127,9 +127,11 @@ static int amd_gpio_set_debounce(struct gpio_chip *gc, unsigned offset,
 	raw_spin_lock_irqsave(&gpio_dev->lock, flags);
 
 	/* Use special handling for Pin0 debounce */
-	pin_reg = readl(gpio_dev->base + WAKE_INT_MASTER_REG);
-	if (pin_reg & INTERNAL_GPIO0_DEBOUNCE)
-		debounce = 0;
+	if (offset == 0) {
+		pin_reg = readl(gpio_dev->base + WAKE_INT_MASTER_REG);
+		if (pin_reg & INTERNAL_GPIO0_DEBOUNCE)
+			debounce = 0;
+	}
 
 	pin_reg = readl(gpio_dev->base + offset * 4);
 
-- 
2.34.1

[PATCH v2 2/4] pinctrl: amd: Use amd_pinconf_set() for all config options

[Mario Limonciello]

On ASUS TUF A16 it is reported that the ITE5570 ACPI device connected to
GPIO 7 is causing an interrupt storm.  This issue doesn't happen on
Windows.

Comparing the GPIO register configuration between Windows and Linux
bit 20 has been configured as a pull up on Windows, but not on Linux.
Checking GPIO declaration from the firmware it is clear it *should* have
been a pull up on Linux as well.

```
GpioInt (Level, ActiveLow, Exclusive, PullUp, 0x0000,
	 "\\_SB.GPIO", 0x00, ResourceConsumer, ,)
{   // Pin list
0x0007
}
```

On Linux amd_gpio_set_config() is currently only used for programming
the debounce. Actually the GPIO core calls it with all the arguments
that are supported by a GPIO, pinctrl-amd just responds `-ENOTSUPP`.

To solve this issue expand amd_gpio_set_config() to support the other
arguments amd_pinconf_set() supports, namely `PIN_CONFIG_BIAS_PULL_DOWN`,
`PIN_CONFIG_BIAS_PULL_UP`, and `PIN_CONFIG_DRIVE_STRENGTH`.

Reported-by: Nik P <npliashechnikov@gmail.com>
Reported-by: Nathan Schulte <nmschulte@gmail.com>
Reported-by: Friedrich Vock <friedrich.vock@gmx.de>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217336
Reported-by: dridri85@gmail.com
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217493
Link: https://lore.kernel.org/linux-input/20230530154058.17594-1-friedrich.vock@gmx.de/
Fixes: 2956b5d94a76 ("pinctrl / gpio: Introduce .set_config() callback for GPIO chips")
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v1->v2:
 * Adjust commit message alignment
 * Move earlier in the series
---
 drivers/pinctrl/pinctrl-amd.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index 02d9f9f245707..eeaf80fdc13a2 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -188,18 +188,6 @@ static int amd_gpio_set_debounce(struct gpio_chip *gc, unsigned offset,
 	return ret;
 }
 
-static int amd_gpio_set_config(struct gpio_chip *gc, unsigned offset,
-			       unsigned long config)
-{
-	u32 debounce;
-
-	if (pinconf_to_config_param(config) != PIN_CONFIG_INPUT_DEBOUNCE)
-		return -ENOTSUPP;
-
-	debounce = pinconf_to_config_argument(config);
-	return amd_gpio_set_debounce(gc, offset, debounce);
-}
-
 #ifdef CONFIG_DEBUG_FS
 static void amd_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc)
 {
@@ -782,7 +770,7 @@ static int amd_pinconf_get(struct pinctrl_dev *pctldev,
 }
 
 static int amd_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
-				unsigned long *configs, unsigned num_configs)
+			   unsigned long *configs, unsigned int num_configs)
 {
 	int i;
 	u32 arg;
@@ -872,6 +860,20 @@ static int amd_pinconf_group_set(struct pinctrl_dev *pctldev,
 	return 0;
 }
 
+static int amd_gpio_set_config(struct gpio_chip *gc, unsigned int pin,
+			       unsigned long config)
+{
+	struct amd_gpio *gpio_dev = gpiochip_get_data(gc);
+
+	if (pinconf_to_config_param(config) == PIN_CONFIG_INPUT_DEBOUNCE) {
+		u32 debounce = pinconf_to_config_argument(config);
+
+		return amd_gpio_set_debounce(gc, pin, debounce);
+	}
+
+	return amd_pinconf_set(gpio_dev->pctrl, pin, &config, 1);
+}
+
 static const struct pinconf_ops amd_pinconf_ops = {
 	.pin_config_get		= amd_pinconf_get,
 	.pin_config_set		= amd_pinconf_set,
-- 
2.34.1

Re: [PATCH v2 2/4] pinctrl: amd: Use amd_pinconf_set() for all config options

[Andy Shevchenko]

On Wed, Jul 05, 2023 at 08:30:03AM -0500, Mario Limonciello wrote:
> On ASUS TUF A16 it is reported that the ITE5570 ACPI device connected to
> GPIO 7 is causing an interrupt storm.  This issue doesn't happen on
> Windows.
> 
> Comparing the GPIO register configuration between Windows and Linux
> bit 20 has been configured as a pull up on Windows, but not on Linux.
> Checking GPIO declaration from the firmware it is clear it *should* have
> been a pull up on Linux as well.
> 
> ```
> GpioInt (Level, ActiveLow, Exclusive, PullUp, 0x0000,
> 	 "\\_SB.GPIO", 0x00, ResourceConsumer, ,)
> {   // Pin list
> 0x0007
> }
> ```
> 
> On Linux amd_gpio_set_config() is currently only used for programming
> the debounce. Actually the GPIO core calls it with all the arguments
> that are supported by a GPIO, pinctrl-amd just responds `-ENOTSUPP`.
> 
> To solve this issue expand amd_gpio_set_config() to support the other
> arguments amd_pinconf_set() supports, namely `PIN_CONFIG_BIAS_PULL_DOWN`,
> `PIN_CONFIG_BIAS_PULL_UP`, and `PIN_CONFIG_DRIVE_STRENGTH`.

...

> @@ -782,7 +770,7 @@ static int amd_pinconf_get(struct pinctrl_dev *pctldev,
>  }
>  
>  static int amd_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
> -				unsigned long *configs, unsigned num_configs)
> +			   unsigned long *configs, unsigned int num_configs)

Seems like a stray change.

>  {
>  	int i;
>  	u32 arg;

Otherwise entire series looks good to me,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/4] pinctrl: amd: Use amd_pinconf_set() for all config options

[Limonciello, Mario]

On 7/6/2023 04:16, Andy Shevchenko wrote:
> On Wed, Jul 05, 2023 at 08:30:03AM -0500, Mario Limonciello wrote:
>> On ASUS TUF A16 it is reported that the ITE5570 ACPI device connected to
>> GPIO 7 is causing an interrupt storm.  This issue doesn't happen on
>> Windows.
>>
>> Comparing the GPIO register configuration between Windows and Linux
>> bit 20 has been configured as a pull up on Windows, but not on Linux.
>> Checking GPIO declaration from the firmware it is clear it *should* have
>> been a pull up on Linux as well.
>>
>> ```
>> GpioInt (Level, ActiveLow, Exclusive, PullUp, 0x0000,
>> 	 "\\_SB.GPIO", 0x00, ResourceConsumer, ,)
>> {   // Pin list
>> 0x0007
>> }
>> ```
>>
>> On Linux amd_gpio_set_config() is currently only used for programming
>> the debounce. Actually the GPIO core calls it with all the arguments
>> that are supported by a GPIO, pinctrl-amd just responds `-ENOTSUPP`.
>>
>> To solve this issue expand amd_gpio_set_config() to support the other
>> arguments amd_pinconf_set() supports, namely `PIN_CONFIG_BIAS_PULL_DOWN`,
>> `PIN_CONFIG_BIAS_PULL_UP`, and `PIN_CONFIG_DRIVE_STRENGTH`.
> 
> ...
> 
>> @@ -782,7 +770,7 @@ static int amd_pinconf_get(struct pinctrl_dev *pctldev,
>>   }
>>   
>>   static int amd_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
>> -				unsigned long *configs, unsigned num_configs)
>> +			   unsigned long *configs, unsigned int num_configs)
> 
> Seems like a stray change.

Right; if necessary I'll pull this into it's own patch instead of 
s,unsigned,unsigned long, in this one.  It just seemed sensible while 
calling this function.

> 
>>   {
>>   	int i;
>>   	u32 arg;
> 
> Otherwise entire series looks good to me,
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 

Thanks!

[PATCH v2 3/4] pinctrl: amd: Drop pull up select configuration

[Mario Limonciello]

pinctrl-amd currently tries to program bit 19 of all GPIOs to select
either a 4kΩ or 8hΩ pull up, but this isn't what bit 19 does.  Bit
19 is marked as reserved, even in the latest platforms documentation.

Drop this programming functionality.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/pinctrl/pinctrl-amd.c | 16 ++++------------
 drivers/pinctrl/pinctrl-amd.h |  1 -
 2 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index eeaf80fdc13a2..44d3193a81f2a 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -209,7 +209,6 @@ static void amd_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc)
 	char *pin_sts;
 	char *interrupt_sts;
 	char *wake_sts;
-	char *pull_up_sel;
 	char *orientation;
 	char debounce_value[40];
 	char *debounce_enable;
@@ -317,14 +316,9 @@ static void amd_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc)
 			seq_printf(s, "   %s|", wake_sts);
 
 			if (pin_reg & BIT(PULL_UP_ENABLE_OFF)) {
-				if (pin_reg & BIT(PULL_UP_SEL_OFF))
-					pull_up_sel = "8k";
-				else
-					pull_up_sel = "4k";
-				seq_printf(s, "%s ↑|",
-					   pull_up_sel);
+				seq_puts(s, "  ↑ |");
 			} else if (pin_reg & BIT(PULL_DOWN_ENABLE_OFF)) {
-				seq_puts(s, "   ↓|");
+				seq_puts(s, "  ↓ |");
 			} else  {
 				seq_puts(s, "    |");
 			}
@@ -751,7 +745,7 @@ static int amd_pinconf_get(struct pinctrl_dev *pctldev,
 		break;
 
 	case PIN_CONFIG_BIAS_PULL_UP:
-		arg = (pin_reg >> PULL_UP_SEL_OFF) & (BIT(0) | BIT(1));
+		arg = (pin_reg >> PULL_UP_ENABLE_OFF) & BIT(0);
 		break;
 
 	case PIN_CONFIG_DRIVE_STRENGTH:
@@ -798,10 +792,8 @@ static int amd_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
 			break;
 
 		case PIN_CONFIG_BIAS_PULL_UP:
-			pin_reg &= ~BIT(PULL_UP_SEL_OFF);
-			pin_reg |= (arg & BIT(0)) << PULL_UP_SEL_OFF;
 			pin_reg &= ~BIT(PULL_UP_ENABLE_OFF);
-			pin_reg |= ((arg>>1) & BIT(0)) << PULL_UP_ENABLE_OFF;
+			pin_reg |= (arg & BIT(0)) << PULL_UP_ENABLE_OFF;
 			break;
 
 		case PIN_CONFIG_DRIVE_STRENGTH:
diff --git a/drivers/pinctrl/pinctrl-amd.h b/drivers/pinctrl/pinctrl-amd.h
index 1cf2d06bbd8c4..34c5c3e71fb26 100644
--- a/drivers/pinctrl/pinctrl-amd.h
+++ b/drivers/pinctrl/pinctrl-amd.h
@@ -36,7 +36,6 @@
 #define WAKE_CNTRL_OFF_S4               15
 #define PIN_STS_OFF			16
 #define DRV_STRENGTH_SEL_OFF		17
-#define PULL_UP_SEL_OFF			19
 #define PULL_UP_ENABLE_OFF		20
 #define PULL_DOWN_ENABLE_OFF		21
 #define OUTPUT_VALUE_OFF		22
-- 
2.34.1

[PATCH v2 4/4] pinctrl: amd: Unify debounce handling into amd_pinconf_set()

[Mario Limonciello]

Debounce handling is done in two different entry points in the driver.
Unify this to make sure that it's always handled the same.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v1->v2:
 * Move later in the series
 * Unsigned -> unsigned int
 * s/out/out_unlock/
---
 drivers/pinctrl/pinctrl-amd.c | 21 +++++----------------
 1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index 44d3193a81f2a..b129d7c76b3e9 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -115,16 +115,12 @@ static void amd_gpio_set_value(struct gpio_chip *gc, unsigned offset, int value)
 	raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
 }
 
-static int amd_gpio_set_debounce(struct gpio_chip *gc, unsigned offset,
-		unsigned debounce)
+static int amd_gpio_set_debounce(struct amd_gpio *gpio_dev, unsigned int offset,
+				 unsigned int debounce)
 {
 	u32 time;
 	u32 pin_reg;
 	int ret = 0;
-	unsigned long flags;
-	struct amd_gpio *gpio_dev = gpiochip_get_data(gc);
-
-	raw_spin_lock_irqsave(&gpio_dev->lock, flags);
 
 	/* Use special handling for Pin0 debounce */
 	if (offset == 0) {
@@ -183,7 +179,6 @@ static int amd_gpio_set_debounce(struct gpio_chip *gc, unsigned offset,
 		pin_reg &= ~(DB_CNTRl_MASK << DB_CNTRL_OFF);
 	}
 	writel(pin_reg, gpio_dev->base + offset * 4);
-	raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
 
 	return ret;
 }
@@ -782,9 +777,8 @@ static int amd_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
 
 		switch (param) {
 		case PIN_CONFIG_INPUT_DEBOUNCE:
-			pin_reg &= ~DB_TMR_OUT_MASK;
-			pin_reg |= arg & DB_TMR_OUT_MASK;
-			break;
+			ret = amd_gpio_set_debounce(gpio_dev, pin, arg);
+			goto out_unlock;
 
 		case PIN_CONFIG_BIAS_PULL_DOWN:
 			pin_reg &= ~BIT(PULL_DOWN_ENABLE_OFF);
@@ -811,6 +805,7 @@ static int amd_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
 
 		writel(pin_reg, gpio_dev->base + pin*4);
 	}
+out_unlock:
 	raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
 
 	return ret;
@@ -857,12 +852,6 @@ static int amd_gpio_set_config(struct gpio_chip *gc, unsigned int pin,
 {
 	struct amd_gpio *gpio_dev = gpiochip_get_data(gc);
 
-	if (pinconf_to_config_param(config) == PIN_CONFIG_INPUT_DEBOUNCE) {
-		u32 debounce = pinconf_to_config_argument(config);
-
-		return amd_gpio_set_debounce(gc, pin, debounce);
-	}
-
 	return amd_pinconf_set(gpio_dev->pctrl, pin, &config, 1);
 }
 
-- 
2.34.1

Re: [PATCH v2 0/4] Fix for interrupt storm on ASUS TUF A16

[Linus Walleij]

On Wed, Jul 5, 2023 at 3:30 PM Mario Limonciello
<mario.limonciello@amd.com> wrote:

> An interrupt storm is reported for the GPIO controller on ASUS TUF A16
> but only on Linux.  In comparing the GPIO registers from Windows and
> Linux the configuration for several bits specified in _AEI() was never
> actually loaded into the hardware on Linux.

I queued this up for fixes as it looks pretty urgent, perhaps I will
even send it to Torvalds before -rc1 if there is positive feedback.

Yours,
Linus Walleij

Re: [PATCH v2 0/4] Fix for interrupt storm on ASUS TUF A16

[Limonciello, Mario]

On 7/7/2023 08:50, Linus Walleij wrote:
> On Wed, Jul 5, 2023 at 3:30 PM Mario Limonciello
> <mario.limonciello@amd.com> wrote:
> 
>> An interrupt storm is reported for the GPIO controller on ASUS TUF A16
>> but only on Linux.  In comparing the GPIO registers from Windows and
>> Linux the configuration for several bits specified in _AEI() was never
>> actually loaded into the hardware on Linux.
> 
> I queued this up for fixes as it looks pretty urgent, perhaps I will
> even send it to Torvalds before -rc1 if there is positive feedback.
> 
> Yours,
> Linus Walleij

Appreciated, thanks!

To all those that reported this issue, the branch that has all the fixes 
committed is:

https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/log/?h=fixes

If you can please check this out and confirm everything works as intended.
It should - the patch was split up into 4 but the meat should be the same.

Re: [PATCH v2 0/4] Fix for interrupt storm on ASUS TUF A16

[Jan Visser]

This fixes keyboard issues on my Asus TUF Gaming A16 Advantage Edition FA617NS-N3085W

Thanks!

Tested-by: Jan Visser <starquake@linuxeverywhere.org>