[RFC] pinctrl: cherryview: Introduce quirk to mask GPE during suspend

Message ID 20180504111846.30750-1-carlo@caione.org
State New
Headers show
Series
  • [RFC] pinctrl: cherryview: Introduce quirk to mask GPE during suspend
Related show

Commit Message

Carlo Caione May 4, 2018, 11:18 a.m.
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.

[0] https://gist.github.com/carlocaione/b7a72fffb4ef4cc50f14386e616706b9

Signed-off-by: Carlo Caione <carlo@endlessm.com>
---
 drivers/pinctrl/intel/pinctrl-cherryview.c | 53 ++++++++++++++++++++++
 1 file changed, 53 insertions(+)

Comments

Mika Westerberg May 7, 2018, 1:45 p.m. | #1
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
Carlo Caione May 14, 2018, 3:43 p.m. | #2
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,

Patch

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);