diff mbox series

[4/4] pinctrl: amd: Revert "pinctrl: amd: disable and mask interrupts on probe"

Message ID 20230421120625.3366-5-mario.limonciello@amd.com
State New
Headers
Series pinctrl: amd: Adjust handling for firmware misconfigurations |

Commit Message

Mario Limonciello April 21, 2023, 12:06 p.m. UTC 
  commit 4e5a04be88fe ("pinctrl: amd: disable and mask interrupts on probe")
was well intentioned to mask a firmware issue on a surface laptop, but it
has a few problems:
1. It had a bug in the loop handling for iteration 63 that lead to other
   problems with GPIO0 handling.
2. It disables interrupts that are used internally by the SOC but masked
   by default.
3. It masked a real firmware problem in some chromebooks that should have
   been caught during development but wasn't.

There has been a lot of other development around s2idle; particularly
around handling of the spurious wakeups.  If there is still a problem on
the original reported surface laptop it should be avoided by adding a quirk
to gpiolib-acpi for that system instead.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/pinctrl/pinctrl-amd.c | 31 -------------------------------
 1 file changed, 31 deletions(-)

Comments

Mario Limonciello April 28, 2023, 1:54 p.m. UTC | #1 
[Public]

> commit 4e5a04be88fe ("pinctrl: amd: disable and mask interrupts on probe")
> was well intentioned to mask a firmware issue on a surface laptop, but it
> has a few problems:
> 1. It had a bug in the loop handling for iteration 63 that lead to other
>    problems with GPIO0 handling.
> 2. It disables interrupts that are used internally by the SOC but masked
>    by default.
> 3. It masked a real firmware problem in some chromebooks that should have
>    been caught during development but wasn't.
> 
> There has been a lot of other development around s2idle; particularly
> around handling of the spurious wakeups.  If there is still a problem on
> the original reported surface laptop it should be avoided by adding a quirk
> to gpiolib-acpi for that system instead.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

Sachi,

Any feedback on this series, particularly on this patch?  I'd like to understand
what GPIO pins prompted the original patch and if 3/4 of this series helps the
original behavior so that we can have confirmation the revert is not going to
cause a regression for you.

Thanks,

> ---
>  drivers/pinctrl/pinctrl-amd.c | 31 -------------------------------
>  1 file changed, 31 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
> index 675c9826b78a..e9fef2391b38 100644
> --- a/drivers/pinctrl/pinctrl-amd.c
> +++ b/drivers/pinctrl/pinctrl-amd.c
> @@ -877,34 +877,6 @@ static const struct pinconf_ops amd_pinconf_ops = {
>  	.pin_config_group_set = amd_pinconf_group_set,
>  };
> 
> -static void amd_gpio_irq_init(struct amd_gpio *gpio_dev)
> -{
> -	struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
> -	unsigned long flags;
> -	u32 pin_reg, mask;
> -	int i;
> -
> -	mask = BIT(WAKE_CNTRL_OFF_S0I3) | BIT(WAKE_CNTRL_OFF_S3) |
> -		BIT(INTERRUPT_MASK_OFF) | BIT(INTERRUPT_ENABLE_OFF)
> |
> -		BIT(WAKE_CNTRL_OFF_S4);
> -
> -	for (i = 0; i < desc->npins; i++) {
> -		int pin = desc->pins[i].number;
> -		const struct pin_desc *pd = pin_desc_get(gpio_dev->pctrl,
> pin);
> -
> -		if (!pd)
> -			continue;
> -
> -		raw_spin_lock_irqsave(&gpio_dev->lock, flags);
> -
> -		pin_reg = readl(gpio_dev->base + pin * 4);
> -		pin_reg &= ~mask;
> -		writel(pin_reg, gpio_dev->base + pin * 4);
> -
> -		raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
> -	}
> -}
> -
>  #ifdef CONFIG_PM_SLEEP
>  static bool amd_gpio_should_save(struct amd_gpio *gpio_dev, unsigned int
> pin)
>  {
> @@ -1142,9 +1114,6 @@ static int amd_gpio_probe(struct platform_device
> *pdev)
>  		return PTR_ERR(gpio_dev->pctrl);
>  	}
> 
> -	/* Disable and mask interrupts */
> -	amd_gpio_irq_init(gpio_dev);
> -
>  	girq = &gpio_dev->gc.irq;
>  	gpio_irq_chip_set_chip(girq, &amd_gpio_irqchip);
>  	/* This will let us handle the parent IRQ in the driver */
> --
> 2.34.1
diff mbox series

Patch

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index 675c9826b78a..e9fef2391b38 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -877,34 +877,6 @@  static const struct pinconf_ops amd_pinconf_ops = {
 	.pin_config_group_set = amd_pinconf_group_set,
 };
 
-static void amd_gpio_irq_init(struct amd_gpio *gpio_dev)
-{
-	struct pinctrl_desc *desc = gpio_dev->pctrl->desc;
-	unsigned long flags;
-	u32 pin_reg, mask;
-	int i;
-
-	mask = BIT(WAKE_CNTRL_OFF_S0I3) | BIT(WAKE_CNTRL_OFF_S3) |
-		BIT(INTERRUPT_MASK_OFF) | BIT(INTERRUPT_ENABLE_OFF) |
-		BIT(WAKE_CNTRL_OFF_S4);
-
-	for (i = 0; i < desc->npins; i++) {
-		int pin = desc->pins[i].number;
-		const struct pin_desc *pd = pin_desc_get(gpio_dev->pctrl, pin);
-
-		if (!pd)
-			continue;
-
-		raw_spin_lock_irqsave(&gpio_dev->lock, flags);
-
-		pin_reg = readl(gpio_dev->base + pin * 4);
-		pin_reg &= ~mask;
-		writel(pin_reg, gpio_dev->base + pin * 4);
-
-		raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
-	}
-}
-
 #ifdef CONFIG_PM_SLEEP
 static bool amd_gpio_should_save(struct amd_gpio *gpio_dev, unsigned int pin)
 {
@@ -1142,9 +1114,6 @@  static int amd_gpio_probe(struct platform_device *pdev)
 		return PTR_ERR(gpio_dev->pctrl);
 	}
 
-	/* Disable and mask interrupts */
-	amd_gpio_irq_init(gpio_dev);
-
 	girq = &gpio_dev->gc.irq;
 	gpio_irq_chip_set_chip(girq, &amd_gpio_irqchip);
 	/* This will let us handle the parent IRQ in the driver */