Message ID | 20211029204017.8223-2-mario.limonciello@amd.com (mailing list archive) |
---|---|
State | Not Applicable, archived |
Headers | show |
Series | [v6,1/2] ACPI: Add stubs for wakeup handler functions | expand |
On Fri, Oct 29, 2021 at 11:42 PM Mario Limonciello <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. > > Cc: stable@kernel.org > BugLink: https://gitlab.freedesktop.org/drm/amd/-/issues/1738 > Reported-by: Joerie de Gram <j.de.gram@gmail.com> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > Acked-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com> ... > +static bool _amd_gpio_irq_handler(int irq, void *dev_id) I know Linus does not like leading _* in the function names, what about 'do_' instead? ... > + /* called from resume context on a shared IRQ, just > + * checking wake source. > + */ Is this comment aligned with the style used elsewhere in the driver code? ... > + dev_dbg(&gpio_dev->pdev->dev, > + "Waking due to GPIO %ld: 0x%x", > + (long)(regs + i - ((u32 __iomem *)gpio_dev->base)), regval); Oy vey, these castings are ugly. The rule of thumb is that if one does such a thing for printf() it means something is really wrong (in 99% of the cases). AFAICS you may simply use 'irqnr + i' as the other message does. ... > 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; > @@ -1021,6 +1045,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); Thinking about making this in the generic GPIO library code, but this is out of scope of the patch...
[Public] > -----Original Message----- > From: Andy Shevchenko <andy.shevchenko@gmail.com> > Sent: Sunday, October 31, 2021 08:15 > To: Limonciello, Mario <Mario.Limonciello@amd.com> > Cc: Linus Walleij <linus.walleij@linaro.org>; Natikar, Basavaraj > <Basavaraj.Natikar@amd.com>; S-k, Shyam-sundar <Shyam-sundar.S- > k@amd.com>; open list:PIN CONTROL SUBSYSTEM <linux- > gpio@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>; ACPI Devel > Maling List <linux-acpi@vger.kernel.org>; Shah, Nehal-bakulchandra <Nehal- > bakulchandra.Shah@amd.com>; stable@kernel.org; Joerie de Gram > <j.de.gram@gmail.com> > Subject: Re: [PATCH v6 2/2] pinctrl: amd: Fix wakeups when IRQ is shared with > SCI > > On Fri, Oct 29, 2021 at 11:42 PM Mario Limonciello > <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. > > > > Cc: stable@kernel.org > > BugLink: > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.fr > eedesktop.org%2Fdrm%2Famd%2F- > %2Fissues%2F1738&data=04%7C01%7Cmario.limonciello%40amd.com%7 > C8962eb61c66843248eff08d99c708f19%7C3dd8961fe4884e608e11a82d994e18 > 3d%7C0%7C0%7C637712829517705057%7CUnknown%7CTWFpbGZsb3d8eyJWI > joiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C100 > 0&sdata=57LKx3moIAVwtjmncHiqDMgvYP5tkEL7JuAP76iaCHI%3D&re > served=0 > > Reported-by: Joerie de Gram <j.de.gram@gmail.com> > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > > Acked-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com> > > ... > > > +static bool _amd_gpio_irq_handler(int irq, void *dev_id) > > I know Linus does not like leading _* in the function names, what > about 'do_' instead? > > ... > > > + /* called from resume context on a shared IRQ, just > > + * checking wake source. > > + */ > > Is this comment aligned with the style used elsewhere in the driver code? > > ... > > > + dev_dbg(&gpio_dev->pdev->dev, > > + "Waking due to GPIO %ld: 0x%x", > > + (long)(regs + i - ((u32 __iomem *)gpio_dev->base)), > regval); > > Oy vey, these castings are ugly. The rule of thumb is that if one does > such a thing for printf() it means something is really wrong (in 99% > of the cases). > > AFAICS you may simply use 'irqnr + i' as the other message does. > Andy, Appreciate your comments. You're correct. I've sent a follow up addressing them. > ... > > > 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; > > @@ -1021,6 +1045,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); > > Thinking about making this in the generic GPIO library code, but this > is out of scope of the patch... Sure, we can think about this if/when there ends up being another consumer of it. > > -- > With Best Regards, > Andy Shevchenko
diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c index d19974aceb2e..c750ffd5c012 100644 --- a/drivers/pinctrl/pinctrl-amd.c +++ b/drivers/pinctrl/pinctrl-amd.c @@ -598,14 +598,14 @@ 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; + bool ret = false; u32 regval; u64 status, mask; @@ -627,6 +627,16 @@ 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))) { + dev_dbg(&gpio_dev->pdev->dev, + "Waking due to GPIO %ld: 0x%x", + (long)(regs + i - ((u32 __iomem *)gpio_dev->base)), regval); + return true; + } + if (!(regval & PIN_IRQ_PENDING) || !(regval & BIT(INTERRUPT_MASK_OFF))) continue; @@ -652,9 +662,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 false; /* Signal EOI to the GPIO unit */ raw_spin_lock_irqsave(&gpio_dev->lock, flags); @@ -666,6 +679,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 IRQ_RETVAL(_amd_gpio_irq_handler(irq, dev_id)); +} + +static bool __maybe_unused 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); @@ -1004,6 +1027,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; @@ -1021,6 +1045,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; }