[RFC] pinctrl/amd: Clear interrupt enable bits on probe

[RFC] pinctrl/amd: Clear interrupt enable bits on probe

[Leonard Crestez]

My Acer Nitro 5 AN515-42 laptop with a Ryzen 2700U hangs on boot because
of spurious interrupts from pinctrl-amd.

This seems to happen because the touchpad interrupt is enabled on boot
in "level" mode and there is no way to clear it until a touchpad driver
probes.

Fix by disabling all gpio interrupts at probe time until they are
explicitly requested by drivers.

Signed-off-by: Leonard Crestez <cdleonard@gmail.com>

---

It's strange that nobody else has run into this problem, AMD hardware is
relatively common. Maybe firmware generally disables GPIO interrupts
itself?

This patch fixes boot but this same laptop has other issues:

 * Suspend is broken
 * Ethernet is broken (only sometimes)
 * The CPU freq gets stuck at 400 Mhz (sometimes)

Those issues happen on maybe 80% of boots without a clear pattern. It
seems that inserting/removing the ethernet jack during boot helps
cpufreq? It's possible that these problems are also caused by pin
misconfiguration so this fix might be incomplete.

When the cpufreq issue happens `rdmsr 0xc0010061 -a` shows 0x22 for all
cpus; maybe this is some broken thermal throttling?

Also, perhaps amd_gpio_irq_disable_all should enumerate in a nicer less
verbose way?

Previously: https://lore.kernel.org/patchwork/patch/1028047/

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index 2a7d638978d8..3cb7ea46f32c 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -592,10 +592,53 @@ static irqreturn_t amd_gpio_irq_handler(int irq, void *dev_id)
 	raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
 
 	return ret;
 }
 
+static void amd_gpio_irq_disable_all(struct amd_gpio *gpio_dev)
+{
+	unsigned int bank, i, pin_num;
+	u32 regval;
+
+	for (bank = 0; bank < gpio_dev->hwbank_num; bank++) {
+		switch (bank) {
+		case 0:
+			i = 0;
+			pin_num = AMD_GPIO_PINS_BANK0;
+			break;
+		case 1:
+			i = 64;
+			pin_num = AMD_GPIO_PINS_BANK1 + i;
+			break;
+		case 2:
+			i = 128;
+			pin_num = AMD_GPIO_PINS_BANK2 + i;
+			break;
+		case 3:
+			i = 192;
+			pin_num = AMD_GPIO_PINS_BANK3 + i;
+			break;
+		default:
+			/* Illegal bank number, ignore */
+			continue;
+		}
+
+		for (; i < pin_num; i++) {
+			unsigned long flags;
+			raw_spin_lock_irqsave(&gpio_dev->lock, flags);
+			regval = readl(gpio_dev->base + i * 4);
+			if (regval & BIT(INTERRUPT_ENABLE_OFF)) {
+				dev_info(&gpio_dev->pdev->dev,
+						"Pin %d interrupt enabled on boot: disable\n", i);
+				regval &= ~BIT(INTERRUPT_ENABLE_OFF);
+				writel(regval, gpio_dev->base + i * 4);
+			}
+			raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
+		}
+	}
+}
+
 static int amd_get_groups_count(struct pinctrl_dev *pctldev)
 {
 	struct amd_gpio *gpio_dev = pinctrl_dev_get_drvdata(pctldev);
 
 	return gpio_dev->ngroups;
@@ -910,10 +953,12 @@ static int amd_gpio_probe(struct platform_device *pdev)
 
 	ret = gpiochip_add_data(&gpio_dev->gc, gpio_dev);
 	if (ret)
 		return ret;
 
+	amd_gpio_irq_disable_all(gpio_dev);
+
 	ret = gpiochip_add_pin_range(&gpio_dev->gc, dev_name(&pdev->dev),
 				0, 0, gpio_dev->gc.ngpio);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to add pin range\n");
 		goto out2;
-- 
2.20.1

Re: [RFC] pinctrl/amd: Clear interrupt enable bits on probe

[Hans de Goede]

Hi,

On 19-02-19 11:59, Leonard Crestez wrote:
> My Acer Nitro 5 AN515-42 laptop with a Ryzen 2700U hangs on boot because
> of spurious interrupts from pinctrl-amd.
> 
> This seems to happen because the touchpad interrupt is enabled on boot
> in "level" mode and there is no way to clear it until a touchpad driver
> probes.
> 
> Fix by disabling all gpio interrupts at probe time until they are
> explicitly requested by drivers.
> 
> Signed-off-by: Leonard Crestez <cdleonard@gmail.com>
> 
> ---
> 
> It's strange that nobody else has run into this problem, AMD hardware is
> relatively common. Maybe firmware generally disables GPIO interrupts
> itself?

Others have run into the problem, but no-one so far has had both time and
access to the hardware to actually get to the bottom of this, see:
https://bugzilla.kernel.org/show_bug.cgi?id=201817#c4

Your patch looks like it is exactly what is needed to fix this,
thank you very much for the patch!

I will attach a copy of your patch to that bug and ask people who are
having similar problems to test it.

Regards,

Hans




>
> This patch fixes boot but this same laptop has other issues:
> 
>   * Suspend is broken
>   * Ethernet is broken (only sometimes)
>   * The CPU freq gets stuck at 400 Mhz (sometimes)
> 
> Those issues happen on maybe 80% of boots without a clear pattern. It
> seems that inserting/removing the ethernet jack during boot helps
> cpufreq? It's possible that these problems are also caused by pin
> misconfiguration so this fix might be incomplete.
> 
> When the cpufreq issue happens `rdmsr 0xc0010061 -a` shows 0x22 for all
> cpus; maybe this is some broken thermal throttling?
> 
> Also, perhaps amd_gpio_irq_disable_all should enumerate in a nicer less
> verbose way?
> 
> Previously: https://lore.kernel.org/patchwork/patch/1028047/
> 
> diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
> index 2a7d638978d8..3cb7ea46f32c 100644
> --- a/drivers/pinctrl/pinctrl-amd.c
> +++ b/drivers/pinctrl/pinctrl-amd.c
> @@ -592,10 +592,53 @@ static irqreturn_t amd_gpio_irq_handler(int irq, void *dev_id)
>   	raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
>   
>   	return ret;
>   }
>   
> +static void amd_gpio_irq_disable_all(struct amd_gpio *gpio_dev)
> +{
> +	unsigned int bank, i, pin_num;
> +	u32 regval;
> +
> +	for (bank = 0; bank < gpio_dev->hwbank_num; bank++) {
> +		switch (bank) {
> +		case 0:
> +			i = 0;
> +			pin_num = AMD_GPIO_PINS_BANK0;
> +			break;
> +		case 1:
> +			i = 64;
> +			pin_num = AMD_GPIO_PINS_BANK1 + i;
> +			break;
> +		case 2:
> +			i = 128;
> +			pin_num = AMD_GPIO_PINS_BANK2 + i;
> +			break;
> +		case 3:
> +			i = 192;
> +			pin_num = AMD_GPIO_PINS_BANK3 + i;
> +			break;
> +		default:
> +			/* Illegal bank number, ignore */
> +			continue;
> +		}
> +
> +		for (; i < pin_num; i++) {
> +			unsigned long flags;
> +			raw_spin_lock_irqsave(&gpio_dev->lock, flags);
> +			regval = readl(gpio_dev->base + i * 4);
> +			if (regval & BIT(INTERRUPT_ENABLE_OFF)) {
> +				dev_info(&gpio_dev->pdev->dev,
> +						"Pin %d interrupt enabled on boot: disable\n", i);
> +				regval &= ~BIT(INTERRUPT_ENABLE_OFF);
> +				writel(regval, gpio_dev->base + i * 4);
> +			}
> +			raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
> +		}
> +	}
> +}
> +
>   static int amd_get_groups_count(struct pinctrl_dev *pctldev)
>   {
>   	struct amd_gpio *gpio_dev = pinctrl_dev_get_drvdata(pctldev);
>   
>   	return gpio_dev->ngroups;
> @@ -910,10 +953,12 @@ static int amd_gpio_probe(struct platform_device *pdev)
>   
>   	ret = gpiochip_add_data(&gpio_dev->gc, gpio_dev);
>   	if (ret)
>   		return ret;
>   
> +	amd_gpio_irq_disable_all(gpio_dev);
> +
>   	ret = gpiochip_add_pin_range(&gpio_dev->gc, dev_name(&pdev->dev),
>   				0, 0, gpio_dev->gc.ngpio);
>   	if (ret) {
>   		dev_err(&pdev->dev, "Failed to add pin range\n");
>   		goto out2;
>