Messages in this thread | | | Subject | Re: [PATCH 1/1] pinctrl: amd: disable and mask interrupts on probe | From | "Limonciello, Mario" <> | Date | Fri, 8 Oct 2021 13:19:17 -0500 |
| |
On 10/1/2021 11:17, Sachi King wrote: > Some systems such as the Microsoft Surface Laptop 4 leave interrupts > enabled and configured for use in sleep states on boot, which cause > unexpected behaviour such as spurious wakes and failed resumes in > s2idle states. > > As interrupts should not be enabled until they are claimed and > explicitly enabled, disabling any interrupts mistakenly left enabled by > firmware should be safe. >
So I did test this on a handful of platforms and confirmed that the events declared in _AEI are still being enabled and passed properly.
So if no other changes needed you can add my: Tested-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Sachi King <nakato@nakato.io> > --- > drivers/pinctrl/pinctrl-amd.c | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c > index c001f2ed20f8..aa4136cd312d 100644 > --- a/drivers/pinctrl/pinctrl-amd.c > +++ b/drivers/pinctrl/pinctrl-amd.c > @@ -830,6 +830,32 @@ 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(INTERRUPT_MASK_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 + i * 4); > + pin_reg &= ~mask; > + writel(pin_reg, gpio_dev->base + i * 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) > { > @@ -967,6 +993,9 @@ 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); > +
As the pinctrl device was just registered, I do wonder if this actually needs a mutex in case another thread tries to enable the pins at the same time. I might be wrong here though and things are OK because the pin range isn't added until later on in probe.
> girq = &gpio_dev->gc.irq; > girq->chip = &amd_gpio_irqchip; > /* This will let us handle the parent IRQ in the driver */ >
| |