| Message ID | 20180504111846.30750-1-carlo@caione.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] pinctrl: cherryview: Introduce quirk to mask GPE during suspend | expand |
On Fri, May 04, 2018 at 12:18:46PM +0100, Carlo Caione wrote: > From: Carlo Caione <carlo@endlessm.com> > > This code is not for inclusion, it's just an ugly patch to sparkle the > discussion on how to properly fix this issue. > > We have a problem with a Cherrytrail laptop (x5-Z8350) that is waking up > from s2idle after a few seconds without any external input. > > This laptop doesn't support (apparently) S3 and the Low Power Idle S0 > _DSM interface is missing functions 5 and 6. > > >From '/sys/power/pm_wakeup_irq' we know that the IRQ waking up the > laptop is coming from the GPIO controller and that the IRQ is handled as > ACPI event: > > 115: 0 0 0 94 chv-gpio 5 ACPI:Event > > This IRQ is being triggered every few seconds, when it is received in > s2idle, this is causing the laptop to wake up. > > Looking in the DSDT table (full dump at [0]) we have: > > Method (_AEI, 0, NotSerialized) // _AEI: ACPI Event Interrupts > { > Name (RBUF, ResourceTemplate () > { > GpioInt (Edge, ActiveBoth, ExclusiveAndWake, PullUp, 0x0000, > "\\_SB.GPO1", 0x00, ResourceConsumer, , > ) > { // Pin list > 0x0005 > } > }) > Return (RBUF) /* \_SB_.GPO1._AEI.RBUF */ > } > > Method (_E05, 0, NotSerialized) // _Exx: Edge-Triggered GPE > { > Local0 = Zero > If (CondRefOf (\_SB.PCI0.I2C3.BATC, Local1)) > { > Local0 = ^^PCI0.I2C3.BATC.INTR () > If (0xFF == Local0) > { > ADBG ("INTR RD FAIL") > Return (Zero) > } > > If (Zero == Local0) > { > Return (Zero) > } > > ADBG ("ULPMC INTR") > ADBG (Local0) > } > ... > > So, the IRQ is basically probing the battery status every few seconds > but since the GpioInt has argument ExclusiveAndWake, this is waking up > the laptop after a few seconds. > > The patch in this RFC gets the idea from commit 76380636280b4 ("ACPI / > EC: Add parameter to force disable the GPE on suspend") adding a quirk > to disable the IRQ (and then the GPE) triggered by a specific GPIO/pin. Since this is not specific to the cherryview driver, I wonder if it makes more sense to handle this in gpiolib-acpi.c::acpi_gpiochip_request_interrupt()? if (agpio->wake_capable == ACPI_WAKE_CAPABLE && !dmi_check_system(acpi_gpio_event_ignore_wake)) enable_irq_wake(irq); -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 7, 2018 at 2:45 PM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > Since this is not specific to the cherryview driver, I wonder if it > makes more sense to handle this in gpiolib-acpi.c::acpi_gpiochip_request_interrupt()? > > if (agpio->wake_capable == ACPI_WAKE_CAPABLE && > !dmi_check_system(acpi_gpio_event_ignore_wake)) > enable_irq_wake(irq); Thank you for this suggestion. In the end we went with a BIOS fix from the vendor: s/ExclusiveAndWake/Exclusive/ for the affecting GpioInt Cheers,
diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c index b1ae1618fefe..d5a76574b678 100644 --- a/drivers/pinctrl/intel/pinctrl-cherryview.c +++ b/drivers/pinctrl/intel/pinctrl-cherryview.c @@ -670,6 +670,13 @@ static const struct chv_community *chv_communities[] = { &southeast_community, }; +struct chv_irq_suspend_quirk { + struct pinctrl_pin_desc *pins; + size_t npins; +}; + +struct chv_irq_suspend_quirk *irq_suspend_quirk; + /* * Lock to serialize register accesses * @@ -1512,6 +1519,32 @@ static void chv_gpio_irq_handler(struct irq_desc *desc) chained_irq_exit(chip, desc); } +static int __init dmi_matched(const struct dmi_system_id *dmi) +{ + irq_suspend_quirk = dmi->driver_data; + return 1; +} + +static struct pinctrl_pin_desc irq_suspend_quirk_north_desc[] = { + PINCTRL_PIN(5, "GPIO_DFX_4"), +}; + +static struct chv_irq_suspend_quirk irq_suspend_quirk_north = { + .pins = irq_suspend_quirk_north_desc, + .npins = ARRAY_SIZE(irq_suspend_quirk_north_desc), +}; + +static const struct dmi_system_id chv_no_irq_suspend[] = { + { + .callback = dmi_matched, + .ident = "ECS EF20", + .matches = { + DMI_MATCH(DMI_PRODUCT_NAME, "EF20EA"), + }, + .driver_data = &irq_suspend_quirk_north, + }, +}; + /* * Certain machines seem to hardcode Linux IRQ numbers in their ACPI * tables. Since we leave GPIOs that are not capable of generating @@ -1561,6 +1594,8 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq) const struct chv_community *community = pctrl->community; int ret, i, irq_base; + dmi_check_system(chv_no_irq_suspend); + *chip = chv_gpio_chip; chip->ngpio = community->pins[community->npins - 1].number + 1; @@ -1765,6 +1800,7 @@ static int chv_pinctrl_suspend_noirq(struct device *dev) const struct pinctrl_pin_desc *desc; struct chv_pin_context *ctx; void __iomem *reg; + int j; desc = &pctrl->community->pins[i]; if (chv_pad_locked(pctrl, desc->number)) @@ -1777,6 +1813,23 @@ static int chv_pinctrl_suspend_noirq(struct device *dev) reg = chv_padreg(pctrl, desc->number, CHV_PADCTRL1); ctx->padctrl1 = readl(reg); + + for (j = 0; j < irq_suspend_quirk->npins; j++) { + const struct pinctrl_pin_desc *quirk_desc; + + quirk_desc = &irq_suspend_quirk->pins[j]; + if (quirk_desc->number == desc->number && + !strcmp(quirk_desc->name, desc->name)) { + u32 value, intr_line; + + intr_line = ctx->padctrl0; + intr_line &= CHV_PADCTRL0_INTSEL_MASK; + intr_line >>= CHV_PADCTRL0_INTSEL_SHIFT; + value = readl(pctrl->regs + CHV_INTMASK); + value &= ~BIT(intr_line); + chv_writel(value, pctrl->regs + CHV_INTMASK); + } + } } raw_spin_unlock_irqrestore(&chv_lock, flags);