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

[Limonciello, Mario]

[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@xxxxxxx>

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