| Message ID | 20211015144332.700-1-mario.limonciello@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: amd: Fix wakeups when IRQ is shared with SCI | expand |
On Fri, Oct 15, 2021 at 09:43:32AM -0500, Mario Limonciello wrote: > + if (irq < 0 && !(regval & BIT(WAKE_STS_OFF))) > + return true; Shouldn't be negated. WAKE_STS_OFF is set on wakeup. With that change: Reported-by: Joerie de Gram <j.de.gram@gmail.com> Tested-by: Joerie de Gram <j.de.gram@gmail.com>
On 10/16/2021 13:22, Andy Shevchenko wrote: > > > On Friday, October 15, 2021, Mario Limonciello > <mario.limonciello@amd.com <mailto:mario.limonciello@amd.com>> wrote: > > On some Lenovo AMD Gen2 platforms the IRQ for the SCI and pinctrl > drivers > are shared. Due to how the s2idle loop handling works, this case needs > an extra explicit check whether the interrupt was caused by SCI or by > the GPIO controller. > > To fix this rework the existing IRQ handler function to function as a > checker and an IRQ handler depending on the calling arguments. > > BugLink: https://gitlab.freedesktop.org/drm/amd/-/issues/1738 > <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Famd%2F-%2Fissues%2F1738&data=04%7C01%7Cmario.limonciello%40amd.com%7C1c5f3ad58d0341eff45408d990d1f3b4%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637700053699095015%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=PsivJX46L3Msg6QfcOvlKz0ickWoqwvM7oR5hZp2oMg%3D&reserved=0> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com > <mailto:mario.limonciello@amd.com>> > --- > drivers/pinctrl/Kconfig | 2 +- > drivers/pinctrl/pinctrl-amd.c | 27 ++++++++++++++++++++++++--- > 2 files changed, 25 insertions(+), 4 deletions(-) > > diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig > index 31921108e456..bd67babe9e87 100644 > --- a/drivers/pinctrl/Kconfig > +++ b/drivers/pinctrl/Kconfig > @@ -96,7 +96,7 @@ config PINCTRL_AT91PIO4 > config PINCTRL_AMD > tristate "AMD GPIO pin control" > depends on HAS_IOMEM > - depends on ACPI || COMPILE_TEST > + depends on ACPI > > > > Stray change? I had thought the need for acpi_register_wakeup_handler would force it, but compile checking it appears to still work with: CONFIG_COMPILE_TEST=y CONFIG_ACPI=n Will drop it, thanks. > > select GPIOLIB > select GPIOLIB_IRQCHIP > select PINMUX > diff --git a/drivers/pinctrl/pinctrl-amd.c > b/drivers/pinctrl/pinctrl-amd.c > index 8d0f88e9ca88..f228e1dadef9 100644 > --- a/drivers/pinctrl/pinctrl-amd.c > +++ b/drivers/pinctrl/pinctrl-amd.c > @@ -598,16 +598,16 @@ static struct irq_chip amd_gpio_irqchip = { > > #define PIN_IRQ_PENDING (BIT(INTERRUPT_STS_OFF) | > BIT(WAKE_STS_OFF)) > > -static irqreturn_t amd_gpio_irq_handler(int irq, void *dev_id) > +static bool _amd_gpio_irq_handler(int irq, void *dev_id) > { > struct amd_gpio *gpio_dev = dev_id; > struct gpio_chip *gc = &gpio_dev->gc; > - irqreturn_t ret = IRQ_NONE; > unsigned int i, irqnr; > unsigned long flags; > u32 __iomem *regs; > u32 regval; > u64 status, mask; > + bool ret = false; > > /* Read the wake status */ > raw_spin_lock_irqsave(&gpio_dev->lock, flags); > @@ -627,6 +627,12 @@ static irqreturn_t amd_gpio_irq_handler(int > irq, void *dev_id) > /* Each status bit covers four pins */ > for (i = 0; i < 4; i++) { > regval = readl(regs + i); > + /* called from resume context on a shared > IRQ, just > + * checking wake source. > + */ > + if (irq < 0 && !(regval & BIT(WAKE_STS_OFF))) > + return true; > + > if (!(regval & PIN_IRQ_PENDING) || > !(regval & BIT(INTERRUPT_MASK_OFF))) > continue; > @@ -650,9 +656,12 @@ static irqreturn_t amd_gpio_irq_handler(int > irq, void *dev_id) > } > writel(regval, regs + i); > raw_spin_unlock_irqrestore(&gpio_dev->lock, > flags); > - ret = IRQ_HANDLED; > + ret = true; > } > } > + /* called from resume context on shared IRQ but didn't cause > wake */ > + if (irq < 0) > + return ret; > > /* Signal EOI to the GPIO unit */ > raw_spin_lock_irqsave(&gpio_dev->lock, flags); > @@ -664,6 +673,16 @@ static irqreturn_t amd_gpio_irq_handler(int > irq, void *dev_id) > return ret; > } > > +static irqreturn_t amd_gpio_irq_handler(int irq, void *dev_id) > +{ > + return _amd_gpio_irq_handler(irq, dev_id) ? IRQ_HANDLED : > IRQ_NONE; > +} > > > IRQ_RETVAL() Thanks, will change it. > > + > +static bool amd_gpio_check_wake(void *dev_id) > +{ > + return _amd_gpio_irq_handler(-1, dev_id); > +} > + > static int amd_get_groups_count(struct pinctrl_dev *pctldev) > { > struct amd_gpio *gpio_dev = pinctrl_dev_get_drvdata(pctldev); > @@ -1002,6 +1021,7 @@ static int amd_gpio_probe(struct > platform_device *pdev) > goto out2; > > platform_set_drvdata(pdev, gpio_dev); > + acpi_register_wakeup_handler(gpio_dev->irq, > amd_gpio_check_wake, gpio_dev); > > dev_dbg(&pdev->dev, "amd gpio driver loaded\n"); > return ret; > @@ -1019,6 +1039,7 @@ static int amd_gpio_remove(struct > platform_device *pdev) > gpio_dev = platform_get_drvdata(pdev); > > gpiochip_remove(&gpio_dev->gc); > + acpi_unregister_wakeup_handler(amd_gpio_check_wake, gpio_dev); > > return 0; > } > -- > 2.25.1 > > > > -- > With Best Regards, > Andy Shevchenko > >
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig index 31921108e456..bd67babe9e87 100644 --- a/drivers/pinctrl/Kconfig +++ b/drivers/pinctrl/Kconfig @@ -96,7 +96,7 @@ config PINCTRL_AT91PIO4 config PINCTRL_AMD tristate "AMD GPIO pin control" depends on HAS_IOMEM - depends on ACPI || COMPILE_TEST + depends on ACPI select GPIOLIB select GPIOLIB_IRQCHIP select PINMUX diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c index 8d0f88e9ca88..f228e1dadef9 100644 --- a/drivers/pinctrl/pinctrl-amd.c +++ b/drivers/pinctrl/pinctrl-amd.c @@ -598,16 +598,16 @@ static struct irq_chip amd_gpio_irqchip = { #define PIN_IRQ_PENDING (BIT(INTERRUPT_STS_OFF) | BIT(WAKE_STS_OFF)) -static irqreturn_t amd_gpio_irq_handler(int irq, void *dev_id) +static bool _amd_gpio_irq_handler(int irq, void *dev_id) { struct amd_gpio *gpio_dev = dev_id; struct gpio_chip *gc = &gpio_dev->gc; - irqreturn_t ret = IRQ_NONE; unsigned int i, irqnr; unsigned long flags; u32 __iomem *regs; u32 regval; u64 status, mask; + bool ret = false; /* Read the wake status */ raw_spin_lock_irqsave(&gpio_dev->lock, flags); @@ -627,6 +627,12 @@ static irqreturn_t amd_gpio_irq_handler(int irq, void *dev_id) /* Each status bit covers four pins */ for (i = 0; i < 4; i++) { regval = readl(regs + i); + /* called from resume context on a shared IRQ, just + * checking wake source. + */ + if (irq < 0 && !(regval & BIT(WAKE_STS_OFF))) + return true; + if (!(regval & PIN_IRQ_PENDING) || !(regval & BIT(INTERRUPT_MASK_OFF))) continue; @@ -650,9 +656,12 @@ static irqreturn_t amd_gpio_irq_handler(int irq, void *dev_id) } writel(regval, regs + i); raw_spin_unlock_irqrestore(&gpio_dev->lock, flags); - ret = IRQ_HANDLED; + ret = true; } } + /* called from resume context on shared IRQ but didn't cause wake */ + if (irq < 0) + return ret; /* Signal EOI to the GPIO unit */ raw_spin_lock_irqsave(&gpio_dev->lock, flags); @@ -664,6 +673,16 @@ static irqreturn_t amd_gpio_irq_handler(int irq, void *dev_id) return ret; } +static irqreturn_t amd_gpio_irq_handler(int irq, void *dev_id) +{ + return _amd_gpio_irq_handler(irq, dev_id) ? IRQ_HANDLED : IRQ_NONE; +} + +static bool amd_gpio_check_wake(void *dev_id) +{ + return _amd_gpio_irq_handler(-1, dev_id); +} + static int amd_get_groups_count(struct pinctrl_dev *pctldev) { struct amd_gpio *gpio_dev = pinctrl_dev_get_drvdata(pctldev); @@ -1002,6 +1021,7 @@ static int amd_gpio_probe(struct platform_device *pdev) goto out2; platform_set_drvdata(pdev, gpio_dev); + acpi_register_wakeup_handler(gpio_dev->irq, amd_gpio_check_wake, gpio_dev); dev_dbg(&pdev->dev, "amd gpio driver loaded\n"); return ret; @@ -1019,6 +1039,7 @@ static int amd_gpio_remove(struct platform_device *pdev) gpio_dev = platform_get_drvdata(pdev); gpiochip_remove(&gpio_dev->gc); + acpi_unregister_wakeup_handler(amd_gpio_check_wake, gpio_dev); return 0; }
On some Lenovo AMD Gen2 platforms the IRQ for the SCI and pinctrl drivers are shared. Due to how the s2idle loop handling works, this case needs an extra explicit check whether the interrupt was caused by SCI or by the GPIO controller. To fix this rework the existing IRQ handler function to function as a checker and an IRQ handler depending on the calling arguments. BugLink: https://gitlab.freedesktop.org/drm/amd/-/issues/1738 Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- drivers/pinctrl/Kconfig | 2 +- drivers/pinctrl/pinctrl-amd.c | 27 ++++++++++++++++++++++++--- 2 files changed, 25 insertions(+), 4 deletions(-)