Messages in this thread | | | Date | Mon, 18 Oct 2021 16:10:08 -0500 | Subject | Re: [PATCH] pinctrl: amd: Fix wakeups when IRQ is shared with SCI | From | "Limonciello, Mario" <> |
| |
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 > >
| |