diff mbox

pinctrl: cherryview: Do not mask all interrupts on probe

Message ID 20160818121328.GE30827@lahna.fi.intel.com
State New
Headers show

Commit Message

Mika Westerberg Aug. 18, 2016, 12:13 p.m. UTC 
On Wed, Aug 17, 2016 at 03:42:58PM +0200, Anisse Astier wrote:
> On Wed, Aug 17, 2016 at 10:13 AM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Tue, Aug 16, 2016 at 06:12:40PM +0200, Anisse Astier wrote:
> >> Hi Mika,
> >>
> >> Did you find a way to fix this issue ? I'm seeing a similar problem on a
> >> laptop where this masks the interrupt used for ACPI events (brightness,
> >> lid, battery).
> >
> > I seem to have forgotten this completely :-/
> >
> > Can you send me output of /sys/kernel/debug/pinctrl/INT33FF:*/pins for
> > that particular EC pin?
> >
> > In addition if you apply this patch do you see that ACPI events start
> > working?
> 
> >From what I've seen it's in the north range, I don't know which pin in
> particular it is yet.
> 
> If the interrupts aren't masked for the north community, ACPI events
> start working.
> 
> 
> # cat /sys/kernel/debug/pinctrl/INT33FF\:01/pins
> registered pins: 59
> pin 0 (GPIO_DFX_0) GPIO ctrl0 0x00118102 ctrl1 0x05c00000
> pin 1 (GPIO_DFX_3) GPIO ctrl0 0x2c018100 ctrl1 0x05c00000
> pin 2 (GPIO_DFX_7) GPIO ctrl0 0x00918102 ctrl1 0x05c00000
> pin 3 (GPIO_DFX_1) GPIO ctrl0 0x18118100 ctrl1 0x05c00000
> pin 4 (GPIO_DFX_5) GPIO ctrl0 0x00918102 ctrl1 0x05c00000
> pin 5 (GPIO_DFX_4) GPIO ctrl0 0x00118102 ctrl1 0x05c00000
> pin 6 (GPIO_DFX_8) GPIO ctrl0 0x00918102 ctrl1 0x05c00000
> pin 7 (GPIO_DFX_2) GPIO ctrl0 0x00118100 ctrl1 0x05c00000
> pin 8 (GPIO_DFX_6) GPIO ctrl0 0x00918102 ctrl1 0x05c00000
> pin 15 (GPIO_SUS0) GPIO ctrl0 0x3c018201 ctrl1 0x05c00001
> pin 16 (SEC_GPIO_SUS10) GPIO ctrl0 0x00118100 ctrl1 0x05c00000
> pin 17 (GPIO_SUS3) GPIO ctrl0 0x4c118100 ctrl1 0x05c00000
> pin 18 (GPIO_SUS7) GPIO ctrl0 0xfc918201 ctrl1 0x05c00001
> pin 19 (GPIO_SUS1) mode 6 ctrl0 0x00160301 ctrl1 0x05c00000
> pin 20 (GPIO_SUS5) mode 1 ctrl0 0x00910200 ctrl1 0x05c00000
> pin 21 (SEC_GPIO_SUS11) GPIO ctrl0 0x5c118100 ctrl1 0x05c00000
> pin 22 (GPIO_SUS4) mode 6 ctrl0 0x00960301 ctrl1 0x05c00000
> pin 23 (SEC_GPIO_SUS8) mode 1 ctrl0 0x00910300 ctrl1 0x05c00000
> pin 24 (GPIO_SUS2) GPIO ctrl0 0x00918102 ctrl1 0x05c00000
> pin 25 (GPIO_SUS6) GPIO ctrl0 0xec918201 ctrl1 0x05c00001

It is this one (GPIO_SUS6).

I wonder if we can relax the driver so that it only masks pins which are
not configured to generate interrupts by the BIOS. I quickly tried
following on one Braswell machine and it did not generate spurious
interrupts.

Can you check if this works for you?

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

Comments

Anisse Astier Aug. 18, 2016, 1:52 p.m. UTC | #1 
On Thu, Aug 18, 2016 at 2:13 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Wed, Aug 17, 2016 at 03:42:58PM +0200, Anisse Astier wrote:
>> pin 25 (GPIO_SUS6) GPIO ctrl0 0xec918201 ctrl1 0x05c00001
>
> It is this one (GPIO_SUS6).
>
> I wonder if we can relax the driver so that it only masks pins which are
> not configured to generate interrupts by the BIOS. I quickly tried
> following on one Braswell machine and it did not generate spurious
> interrupts.
>
> Can you check if this works for you?

I tried it, your patch is working. It gives the same result as not
clearing the north community. I receive the ACPI events.

Regards,

Anisse
--
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
Mika Westerberg Aug. 18, 2016, 1:58 p.m. UTC | #2 
On Thu, Aug 18, 2016 at 03:52:57PM +0200, Anisse Astier wrote:
> On Thu, Aug 18, 2016 at 2:13 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Wed, Aug 17, 2016 at 03:42:58PM +0200, Anisse Astier wrote:
> >> pin 25 (GPIO_SUS6) GPIO ctrl0 0xec918201 ctrl1 0x05c00001
> >
> > It is this one (GPIO_SUS6).
> >
> > I wonder if we can relax the driver so that it only masks pins which are
> > not configured to generate interrupts by the BIOS. I quickly tried
> > following on one Braswell machine and it did not generate spurious
> > interrupts.
> >
> > Can you check if this works for you?
> 
> I tried it, your patch is working. It gives the same result as not
> clearing the north community. I receive the ACPI events.

OK, thanks for testing.

I'll make a formal patch and submit it with you CC'd. Let's hope it will
not break anything :)
--
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
Phidias Chiang Sept. 8, 2016, 10:13 a.m. UTC | #3 
On Thu, Aug 18, 2016 at 04:58:13PM +0300, Mika Westerberg wrote:
> On Thu, Aug 18, 2016 at 03:52:57PM +0200, Anisse Astier wrote:
> > On Thu, Aug 18, 2016 at 2:13 PM, Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> > > On Wed, Aug 17, 2016 at 03:42:58PM +0200, Anisse Astier wrote:
> > >> pin 25 (GPIO_SUS6) GPIO ctrl0 0xec918201 ctrl1 0x05c00001
> > >
> > > It is this one (GPIO_SUS6).
> > >
> > > I wonder if we can relax the driver so that it only masks pins which are
> > > not configured to generate interrupts by the BIOS. I quickly tried
> > > following on one Braswell machine and it did not generate spurious
> > > interrupts.
> > >
> > > Can you check if this works for you?
> > 
> > I tried it, your patch is working. It gives the same result as not
> > clearing the north community. I receive the ACPI events.
> 
> OK, thanks for testing.
> 
> I'll make a formal patch and submit it with you CC'd. Let's hope it will
> not break anything :)

Hi, we've also found this issue on HP X360, but both patches don't work
with on it.

My current workaround is to save INTMASK before clearing then restore
it after, but I'm not sure if there's any side-effect by doing so.
--
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
Mika Westerberg Sept. 8, 2016, 10:24 a.m. UTC | #4 
On Thu, Sep 08, 2016 at 06:13:03PM +0800, Phidias Chiang wrote:
> On Thu, Aug 18, 2016 at 04:58:13PM +0300, Mika Westerberg wrote:
> > On Thu, Aug 18, 2016 at 03:52:57PM +0200, Anisse Astier wrote:
> > > On Thu, Aug 18, 2016 at 2:13 PM, Mika Westerberg
> > > <mika.westerberg@linux.intel.com> wrote:
> > > > On Wed, Aug 17, 2016 at 03:42:58PM +0200, Anisse Astier wrote:
> > > >> pin 25 (GPIO_SUS6) GPIO ctrl0 0xec918201 ctrl1 0x05c00001
> > > >
> > > > It is this one (GPIO_SUS6).
> > > >
> > > > I wonder if we can relax the driver so that it only masks pins which are
> > > > not configured to generate interrupts by the BIOS. I quickly tried
> > > > following on one Braswell machine and it did not generate spurious
> > > > interrupts.
> > > >
> > > > Can you check if this works for you?
> > > 
> > > I tried it, your patch is working. It gives the same result as not
> > > clearing the north community. I receive the ACPI events.
> > 
> > OK, thanks for testing.
> > 
> > I'll make a formal patch and submit it with you CC'd. Let's hope it will
> > not break anything :)
> 
> Hi, we've also found this issue on HP X360, but both patches don't work
> with on it.
> 
> My current workaround is to save INTMASK before clearing then restore
> it after, but I'm not sure if there's any side-effect by doing so.

Did you try the latest patch here?

https://patchwork.ozlabs.org/patch/661413/
--
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
Phidias Chiang Sept. 8, 2016, 4:28 p.m. UTC | #5 
On Thu, Sep 08, 2016 at 01:24:02PM +0300, Mika Westerberg wrote:
> On Thu, Sep 08, 2016 at 06:13:03PM +0800, Phidias Chiang wrote:
> > On Thu, Aug 18, 2016 at 04:58:13PM +0300, Mika Westerberg wrote:
> > > On Thu, Aug 18, 2016 at 03:52:57PM +0200, Anisse Astier wrote:
> > > > On Thu, Aug 18, 2016 at 2:13 PM, Mika Westerberg
> > > > <mika.westerberg@linux.intel.com> wrote:
> > > > > On Wed, Aug 17, 2016 at 03:42:58PM +0200, Anisse Astier wrote:
> > > > >> pin 25 (GPIO_SUS6) GPIO ctrl0 0xec918201 ctrl1 0x05c00001
> > > > >
> > > > > It is this one (GPIO_SUS6).
> > > > >
> > > > > I wonder if we can relax the driver so that it only masks pins which are
> > > > > not configured to generate interrupts by the BIOS. I quickly tried
> > > > > following on one Braswell machine and it did not generate spurious
> > > > > interrupts.
> > > > >
> > > > > Can you check if this works for you?
> > > > 
> > > > I tried it, your patch is working. It gives the same result as not
> > > > clearing the north community. I receive the ACPI events.
> > > 
> > > OK, thanks for testing.
> > > 
> > > I'll make a formal patch and submit it with you CC'd. Let's hope it will
> > > not break anything :)
> > 
> > Hi, we've also found this issue on HP X360, but both patches don't work
> > with on it.
> > 
> > My current workaround is to save INTMASK before clearing then restore
> > it after, but I'm not sure if there's any side-effect by doing so.
> 
> Did you try the latest patch here?
> 
> https://patchwork.ozlabs.org/patch/661413/

yes, that, including another patch in this thread. I inserted some debug
message and found out INTMASK was still cleared after `gpiochip_irqchip_add`,
hope it helps.
--
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
Mika Westerberg Sept. 9, 2016, 6:18 a.m. UTC | #6 
On Fri, Sep 09, 2016 at 12:28:43AM +0800, Phidias Chiang wrote:
> On Thu, Sep 08, 2016 at 01:24:02PM +0300, Mika Westerberg wrote:
> > On Thu, Sep 08, 2016 at 06:13:03PM +0800, Phidias Chiang wrote:
> > > On Thu, Aug 18, 2016 at 04:58:13PM +0300, Mika Westerberg wrote:
> > > > On Thu, Aug 18, 2016 at 03:52:57PM +0200, Anisse Astier wrote:
> > > > > On Thu, Aug 18, 2016 at 2:13 PM, Mika Westerberg
> > > > > <mika.westerberg@linux.intel.com> wrote:
> > > > > > On Wed, Aug 17, 2016 at 03:42:58PM +0200, Anisse Astier wrote:
> > > > > >> pin 25 (GPIO_SUS6) GPIO ctrl0 0xec918201 ctrl1 0x05c00001
> > > > > >
> > > > > > It is this one (GPIO_SUS6).
> > > > > >
> > > > > > I wonder if we can relax the driver so that it only masks pins which are
> > > > > > not configured to generate interrupts by the BIOS. I quickly tried
> > > > > > following on one Braswell machine and it did not generate spurious
> > > > > > interrupts.
> > > > > >
> > > > > > Can you check if this works for you?
> > > > > 
> > > > > I tried it, your patch is working. It gives the same result as not
> > > > > clearing the north community. I receive the ACPI events.
> > > > 
> > > > OK, thanks for testing.
> > > > 
> > > > I'll make a formal patch and submit it with you CC'd. Let's hope it will
> > > > not break anything :)
> > > 
> > > Hi, we've also found this issue on HP X360, but both patches don't work
> > > with on it.
> > > 
> > > My current workaround is to save INTMASK before clearing then restore
> > > it after, but I'm not sure if there's any side-effect by doing so.
> > 
> > Did you try the latest patch here?
> > 
> > https://patchwork.ozlabs.org/patch/661413/
> 
> yes, that, including another patch in this thread. I inserted some debug
> message and found out INTMASK was still cleared after `gpiochip_irqchip_add`,
> hope it helps.

Hmm, how can that happen? The patch removes clearing of INTMASK and only
other place where it is cleared temporarily is on resume. Can you add
dev_info() calls like:

	/* Clear all interrupts */
	chv_writel(0xffff, pctrl->regs + CHV_INTSTAT);
	dev_info(pctrl->dev, "INTMASK0: 0x%08x\n", readl(pctrl->regs + CHV_INTMASK));

	...

	gpiochip_set_chained_irqchip(chip, &chv_gpio_irqchip, irq,
				     chv_gpio_irq_handler);
	dev_info(pctrl->dev, "INTMASK1: 0x%08x\n", readl(pctrl->regs + CHV_INTMASK));
	return 0;

It should print the same values both time.

Also which interrupt does not work and can you send me output of
/proc/interrupts?
--
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
Phidias Chiang Sept. 9, 2016, 8:23 a.m. UTC | #7 
On Fri, Sep 09, 2016 at 09:18:34AM +0300, Mika Westerberg wrote:
> On Fri, Sep 09, 2016 at 12:28:43AM +0800, Phidias Chiang wrote:
> 
> Hmm, how can that happen? The patch removes clearing of INTMASK and only
> other place where it is cleared temporarily is on resume. Can you add
> dev_info() calls like:
> 
> 	/* Clear all interrupts */
> 	chv_writel(0xffff, pctrl->regs + CHV_INTSTAT);
> 	dev_info(pctrl->dev, "INTMASK0: 0x%08x\n", readl(pctrl->regs + CHV_INTMASK));
> 
> 	...
> 
> 	gpiochip_set_chained_irqchip(chip, &chv_gpio_irqchip, irq,
> 				     chv_gpio_irq_handler);
> 	dev_info(pctrl->dev, "INTMASK1: 0x%08x\n", readl(pctrl->regs + CHV_INTMASK));
> 	return 0;
> 
> It should print the same values both time.
> 
> Also which interrupt does not work and can you send me output of
> /proc/interrupts?

Output in dmesg:
[    2.054475] cherryview-pinctrl INT33FF:00: INTMASK0: 0x00000006
[    2.055247] cherryview-pinctrl INT33FF:00: INTMASK1: 0x00000000
[    2.055375] cherryview-pinctrl INT33FF:01: INTMASK0: 0x00004000
[    2.056931] cherryview-pinctrl INT33FF:01: INTMASK1: 0x00000000
[    2.057036] cherryview-pinctrl INT33FF:02: INTMASK0: 0x00000000
[    2.057367] cherryview-pinctrl INT33FF:02: INTMASK1: 0x00000000
[    2.057489] cherryview-pinctrl INT33FF:03: INTMASK0: 0x00000000
[    2.058337] cherryview-pinctrl INT33FF:03: INTMASK1: 0x00000000

So it's somehow got cleared in the process after.

And the following link is the complete /proc/interrupts:
http://pastebin.com/qwamyKZb

THe interrupt doesn't work is 9, when I made it work it responsed to
pressing hotkeys.
--
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
Mika Westerberg Sept. 9, 2016, 8:58 a.m. UTC | #8 
On Fri, Sep 09, 2016 at 04:23:58PM +0800, Phidias Chiang wrote:
> On Fri, Sep 09, 2016 at 09:18:34AM +0300, Mika Westerberg wrote:
> > On Fri, Sep 09, 2016 at 12:28:43AM +0800, Phidias Chiang wrote:
> > 
> > Hmm, how can that happen? The patch removes clearing of INTMASK and only
> > other place where it is cleared temporarily is on resume. Can you add
> > dev_info() calls like:
> > 
> > 	/* Clear all interrupts */
> > 	chv_writel(0xffff, pctrl->regs + CHV_INTSTAT);
> > 	dev_info(pctrl->dev, "INTMASK0: 0x%08x\n", readl(pctrl->regs + CHV_INTMASK));
> > 
> > 	...
> > 
> > 	gpiochip_set_chained_irqchip(chip, &chv_gpio_irqchip, irq,
> > 				     chv_gpio_irq_handler);
> > 	dev_info(pctrl->dev, "INTMASK1: 0x%08x\n", readl(pctrl->regs + CHV_INTMASK));
> > 	return 0;
> > 
> > It should print the same values both time.
> > 
> > Also which interrupt does not work and can you send me output of
> > /proc/interrupts?
> 
> Output in dmesg:
> [    2.054475] cherryview-pinctrl INT33FF:00: INTMASK0: 0x00000006
> [    2.055247] cherryview-pinctrl INT33FF:00: INTMASK1: 0x00000000
> [    2.055375] cherryview-pinctrl INT33FF:01: INTMASK0: 0x00004000
> [    2.056931] cherryview-pinctrl INT33FF:01: INTMASK1: 0x00000000
> [    2.057036] cherryview-pinctrl INT33FF:02: INTMASK0: 0x00000000
> [    2.057367] cherryview-pinctrl INT33FF:02: INTMASK1: 0x00000000
> [    2.057489] cherryview-pinctrl INT33FF:03: INTMASK0: 0x00000000
> [    2.058337] cherryview-pinctrl INT33FF:03: INTMASK1: 0x00000000
> 
> So it's somehow got cleared in the process after.

Only other place where we touch INTMASK register is
chv_gpio_irq_mask_unmask(). Can you add some debug there to find out the
caller?

> And the following link is the complete /proc/interrupts:
> http://pastebin.com/qwamyKZb

Looks quite normal.

> THe interrupt doesn't work is 9, when I made it work it responsed to
> pressing hotkeys.

9 is the SCI interrupt used by ACPI which explains.
--
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
diff mbox

Patch

diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index 5749a4eee746..579e0e48bdee 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -1513,6 +1513,7 @@  static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
 	const struct chv_gpio_pinrange *range;
 	struct gpio_chip *chip = &pctrl->chip;
 	int ret, i, offset;
+	u32 intmask = 0;
 
 	*chip = chv_gpio_chip;
 
@@ -1539,8 +1540,27 @@  static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
 		offset += range->npins;
 	}
 
-	/* Mask and clear all interrupts */
-	chv_writel(0, pctrl->regs + CHV_INTMASK);
+	/*
+	 * Mask all interrupts except those which BIOS has configured to
+	 * actually generate interrupts in their padctrl registers.
+	 */
+	for (i = 0; i < pctrl->community->npins; i++) {
+		unsigned pin = pctrl->community->pins[i].number;
+		u32 intsel, ctrl1;
+
+		intsel = readl(chv_padreg(pctrl, pin, CHV_PADCTRL0));
+		intsel &= CHV_PADCTRL0_INTSEL_MASK;
+		intsel >>= CHV_PADCTRL0_INTSEL_SHIFT;
+		ctrl1 = readl(chv_padreg(pctrl, pin, CHV_PADCTRL1));
+
+		if (intsel && (ctrl1 & CHV_PADCTRL1_INTWAKECFG_MASK))
+			intmask |= BIT(intsel);
+	}
+
+	intmask &= readl(pctrl->regs + CHV_INTMASK);
+	writel(intmask, pctrl->regs + CHV_INTMASK);
+
+	/* Clear all interrupts */
 	chv_writel(0xffff, pctrl->regs + CHV_INTSTAT);
 
 	ret = gpiochip_irqchip_add(chip, &chv_gpio_irqchip, 0,