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

[PATCH 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.

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

 drivers/pinctrl/pinctrl-amd.c | 59 +++++++++++++----------------------
 drivers/pinctrl/pinctrl-amd.h |  1 -
 2 files changed, 22 insertions(+), 38 deletions(-)


base-commit: 9f0648f13e34a01f2e1a7a0d5801988a7bca6988
-- 
2.34.1

[PATCH 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: 968ab9261627f ("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 2/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 02d9f9f245707..cd46a5200f9b4 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -221,7 +221,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;
@@ -329,14 +328,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, "    |");
 			}
@@ -763,7 +757,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:
@@ -810,10 +804,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 3/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>
---
 drivers/pinctrl/pinctrl-amd.c | 38 +++++++++++++++--------------------
 1 file changed, 16 insertions(+), 22 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index cd46a5200f9b4..a22e02e2f5d35 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 offset,
+				 unsigned 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,23 +179,10 @@ 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;
 }
 
-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)
 {
@@ -794,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;
 
 		case PIN_CONFIG_BIAS_PULL_DOWN:
 			pin_reg &= ~BIT(PULL_DOWN_ENABLE_OFF);
@@ -823,6 +805,7 @@ static int amd_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
 
 		writel(pin_reg, gpio_dev->base + pin*4);
 	}
+out:
 	raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
 
 	return ret;
@@ -864,6 +847,17 @@ static int amd_pinconf_group_set(struct pinctrl_dev *pctldev,
 	return 0;
 }
 
+static int amd_gpio_set_config(struct gpio_chip *gc, unsigned pin,
+			       unsigned long config)
+{
+	struct amd_gpio *gpio_dev = gpiochip_get_data(gc);
+
+	if (pinconf_to_config_param(config) != PIN_CONFIG_INPUT_DEBOUNCE)
+		return -ENOTSUPP;
+
+	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

[PATCH 4/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: 2956b5d94a76b ("pinctrl / gpio: Introduce .set_config() callback for GPIO chips")
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/pinctrl/pinctrl-amd.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index a22e02e2f5d35..76fabf4404a6c 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -852,9 +852,6 @@ static int amd_gpio_set_config(struct gpio_chip *gc, unsigned pin,
 {
 	struct amd_gpio *gpio_dev = gpiochip_get_data(gc);
 
-	if (pinconf_to_config_param(config) != PIN_CONFIG_INPUT_DEBOUNCE)
-		return -ENOTSUPP;
-
 	return amd_pinconf_set(gpio_dev->pctrl, pin, &config, 1);
 }
 
-- 
2.34.1

Re: [PATCH 2/4] pinctrl: amd: Drop pull up select configuration

[andy.shevchenko]

Fri, Jun 30, 2023 at 02:47:14PM -0500, Mario Limonciello kirjoitti:
> 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.

Can it be that documentation is not (yet) updated?

Where, btw, did the original code come from? Perhaps it may shed some light
on this.

-- 
With Best Regards,
Andy Shevchenko

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

[andy.shevchenko]

Fri, Jun 30, 2023 at 02:47:15PM -0500, Mario Limonciello kirjoitti:
> Debounce handling is done in two different entry points in the driver.
> Unify this to make sure that it's always handled the same.

...

> -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 offset,
> +				 unsigned debounce)

Side note: Are you going to fix unsigned --> unsigned int?
The former is discouraged.

...

> +out:

out_unlock: ?

>  	raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
>  
>  	return ret;

-- 
With Best Regards,
Andy Shevchenko

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

[andy.shevchenko]

Fri, Jun 30, 2023 at 02:47:16PM -0500, Mario Limonciello kirjoitti:
> 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
>                         }

I beleive we do not need so many heading spaces in the above

> ```
> 
> 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`.

...

> Fixes: 2956b5d94a76b ("pinctrl / gpio: Introduce .set_config() callback for GPIO chips")

Can you group fixes at the beginning of the series? 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/4] pinctrl: amd: Drop pull up select configuration

[Mario Limonciello]

On 7/3/23 16:32, andy.shevchenko@gmail.com wrote:
> Fri, Jun 30, 2023 at 02:47:14PM -0500, Mario Limonciello kirjoitti:
>> 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.
> 
> Can it be that documentation is not (yet) updated?
> 

No; I double checked documentation across products from last few years 
as well as products to be coming out in the next few years and this bit 
is consistently marked "Reserved".

> Where, btw, did the original code come from? Perhaps it may shed some light
> on this.
> 

It's from the *very beginning* of the driver.

dbad75dd1f25 ("pinctrl: add AMD GPIO driver support.")

Original author isn't at AMD anymore, so I can't ask them.
I would guess that it was something that was discussed to be supported 
but never actually was.

Maybe Shyam and Basavaraj have some other thoughts on this bit.

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

[Mario Limonciello]

On 7/3/23 16:35, andy.shevchenko@gmail.com wrote:
> Fri, Jun 30, 2023 at 02:47:16PM -0500, Mario Limonciello kirjoitti:
>> 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
>>                          }
> 
> I beleive we do not need so many heading spaces in the above
> 

OK, will fix it up.

>> ```
>>
>> 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`.
> 
> ...
> 
>> Fixes: 2956b5d94a76b ("pinctrl / gpio: Introduce .set_config() callback for GPIO chips")
> 
> Can you group fixes at the beginning of the series?
> 

I'm a bit wary of this because the other commits fix it so that debounce 
handling works properly before this is wired up.  If I put this patch 
earlier I'll need to make sure it's only used for the new options and 
not debounce.

But I'll think about how to do it.

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

[Mario Limonciello]

On 7/3/23 16:33, andy.shevchenko@gmail.com wrote:
> Fri, Jun 30, 2023 at 02:47:15PM -0500, Mario Limonciello kirjoitti:
>> Debounce handling is done in two different entry points in the driver.
>> Unify this to make sure that it's always handled the same.
> 
> ...
> 
>> -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 offset,
>> +				 unsigned debounce)
> 
> Side note: Are you going to fix unsigned --> unsigned int?
> The former is discouraged.
> 
> ...
> 
>> +out:
> 
> out_unlock: ?
> 
>>   	raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
>>   
>>   	return ret;
> 

Ack, thanks will adjust both.