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
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 <[email protected]>
---
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
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 <[email protected]>
Reported-by: Nathan Schulte <[email protected]>
Reported-by: Friedrich Vock <[email protected]>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217336
Reported-by: [email protected]
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217493
Link: https://lore.kernel.org/linux-input/[email protected]/
Fixes: 2956b5d94a76 ("pinctrl / gpio: Introduce .set_config() callback for GPIO chips")
Signed-off-by: Mario Limonciello <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
--
With Best Regards,
Andy Shevchenko
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 <[email protected]>
>
Thanks!
On Wed, Jul 5, 2023 at 3:30 PM Mario Limonciello
<[email protected]> 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
On 7/7/2023 08:50, Linus Walleij wrote:
> On Wed, Jul 5, 2023 at 3:30 PM Mario Limonciello
> <[email protected]> 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.
This fixes keyboard issues on my Asus TUF Gaming A16 Advantage Edition FA617NS-N3085W
Thanks!
Tested-by: Jan Visser <[email protected]>